From: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>,
Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: dri-devel@lists.freedesktop.org, daniel@ffwll.ch,
maarten.lankhorst@linux.intel.com, airlied@gmail.com,
tzimmermann@suse.de, intel-xe@lists.freedesktop.org,
Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
Subject: Re: [PATCH v3 1/4] drm: add devm release action
Date: Thu, 25 Apr 2024 20:12:14 +0530 [thread overview]
Message-ID: <ab3bcaa5-15b3-48a0-9916-58806b20ae06@linux.intel.com> (raw)
In-Reply-To: <20240425-diligent-literate-terrier-2e787d@penduick>
On 25/04/24 18:22, Maxime Ripard wrote:
> On Wed, Apr 24, 2024 at 08:20:32AM -0400, Rodrigo Vivi wrote:
>> On Wed, Apr 24, 2024 at 01:49:16PM +0200, Maxime Ripard wrote:
>>> On Tue, Apr 23, 2024 at 01:42:22PM -0400, Rodrigo Vivi wrote:
>>>> On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
>>>>> On 23/04/24 02:24, Rodrigo Vivi wrote:
>>>>>> On Mon, Apr 22, 2024 at 12:27:53PM +0530, Aravind Iddamsetty wrote:
>>>>>>> In scenarios where drm_dev_put is directly called by driver we want to
>>>>>>> release devm_drm_dev_init_release action associated with struct
>>>>>>> drm_device.
>>>>>>>
>>>>>>> v2: Directly expose the original function, instead of introducing a
>>>>>>> helper (Rodrigo)
>>>>>>>
>>>>>>> v3: add kernel-doc (Maxime Ripard)
>>>>>>>
>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>>>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>>>
>>>>>> please avoid these empty lines here.... cc, rv-b, sign-offs, links,
>>>>>> etc are all in the same block.
>>>>> ok.
>>>>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>>>>> ---
>>>>>>> drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
>>>>>>> include/drm/drm_drv.h | 2 ++
>>>>>>> 2 files changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>>>>> index 243cacb3575c..9d0409165f1e 100644
>>>>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>>>>> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>>>>>>> devm_drm_dev_init_release, dev);
>>>>>>> }
>>>>>>>
>>>>>>> +/**
>>>>>>> + * devm_drm_dev_release_action - Call the final release action of the device
>>>>>> Seeing the doc here gave me a second thought....
>>>>>>
>>>>>> the original release should be renamed to _devm_drm_dev_release
>>>>>> and this should be called devm_drm_dev_release without the 'action' word.
>>>>> i believe, was suggested earlier to directly expose the main function, is
>>>>> there any reason to have a __ version ?
>>>> No no, just ignore me. Just remove the '_action' and don't change the other.
>>>>
>>>> I don't like exposing the a function with '__'. what would '__' that mean?
>>>> This is what I meant on the first comment.
>>>>
>>>> Now, I believe that we don't need the '_action'. What does the 'action' mean?
>>>>
>>>> the devm_drm_dev_release should be enough. But then I got confused and
>>>> I thought it would conflict with the original released function name.
>>>> But I misread it.
>>> I don't think devm_drm_dev_release is a good name either. Just like any
>>> other devm_* function that cancels what a previous one has been doing
>>> (devm_kfree, devm_backlight_device_unregister, devm_nvmem_device_put,
>>> etc.) it should be called devm_drm_dev_put or something similar.
>> I see what you mean, but I don't believe the 'put' is the best option,
>> for 2 reasons:
>> - in general, we have put paired with gets and this has not get equivalent
> Yeah, that's true. _release is fine then I guess.
>
>> - this bypass the regular get/put mechanism and forces the releases that
>> would be done only after all drm_dev_put() taking ref to zero.
> I don't think it does? devm_release_action will only remove the devm
> action and execute it directly, but this action here is a call to
> drm_dev_put, so we might still have other references taken that would
> defer the device being freed.
yes i.e right, i assumed drm_dev_unplug would close all client handles but no.
So i was thinking if it is ok to iterate over no of clients and call drm_dev_put in either
drm_dev_unplug or as part of this devm_release.
Thanks,
Aravind.
>
> Maxime
next prev parent reply other threads:[~2024-04-25 14:39 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-22 6:57 [PATCH v4 0/4] drm/xe: Support PCIe FLR Aravind Iddamsetty
2024-04-22 6:57 ` [PATCH v3 1/4] drm: add devm release action Aravind Iddamsetty
2024-04-22 20:54 ` Rodrigo Vivi
2024-04-23 8:55 ` Aravind Iddamsetty
2024-04-23 17:42 ` Rodrigo Vivi
2024-04-24 11:30 ` Aravind Iddamsetty
2024-04-24 11:49 ` Maxime Ripard
2024-04-24 12:20 ` Rodrigo Vivi
2024-04-25 12:52 ` Maxime Ripard
2024-04-25 14:42 ` Aravind Iddamsetty [this message]
2024-04-24 11:51 ` Maxime Ripard
2024-04-24 12:36 ` Aravind Iddamsetty
2024-05-02 13:42 ` Maxime Ripard
2024-04-22 6:57 ` [PATCH 2/4] drm/xe: Save and restore PCI state Aravind Iddamsetty
2024-04-23 4:18 ` Ghimiray, Himal Prasad
2024-04-22 6:57 ` [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper Aravind Iddamsetty
2024-04-22 6:57 ` [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR Aravind Iddamsetty
2024-04-23 4:18 ` Ghimiray, Himal Prasad
2024-04-23 15:04 ` Nilawar, Badal
2024-04-24 3:12 ` Aravind Iddamsetty
2024-04-24 11:12 ` Nilawar, Badal
2024-04-25 4:07 ` Aravind Iddamsetty
2024-04-23 23:49 ` Michał Winiarski
2024-04-24 5:12 ` Aravind Iddamsetty
2024-04-24 23:29 ` Michał Winiarski
2024-04-25 6:17 ` Aravind Iddamsetty
2024-04-26 0:53 ` Michał Winiarski
2024-04-22 8:21 ` ✓ CI.Patch_applied: success for drm/xe: Support PCIe FLR (rev5) Patchwork
2024-04-22 8:21 ` ✗ CI.checkpatch: warning " Patchwork
2024-04-22 8:23 ` ✓ CI.KUnit: success " Patchwork
2024-04-22 8:34 ` ✓ CI.Build: " Patchwork
2024-04-22 8:37 ` ✓ CI.Hooks: " Patchwork
2024-04-22 8:38 ` ✗ CI.checksparse: warning " Patchwork
2024-04-22 15:54 ` ✓ CI.Patch_applied: success " Patchwork
2024-04-22 15:55 ` ✗ CI.checkpatch: warning " Patchwork
2024-04-22 15:56 ` ✓ CI.KUnit: success " Patchwork
2024-04-22 16:13 ` ✓ CI.Build: " Patchwork
2024-04-22 16:15 ` ✓ CI.Hooks: " Patchwork
2024-04-22 16:17 ` ✗ CI.checksparse: warning " Patchwork
2024-04-23 14:09 ` ✓ CI.Patch_applied: success for drm/xe: Support PCIe FLR (rev6) Patchwork
2024-04-23 14:09 ` ✗ CI.checkpatch: warning " Patchwork
2024-04-23 14:10 ` ✓ CI.KUnit: success " Patchwork
2024-04-23 14:22 ` ✓ CI.Build: " Patchwork
2024-04-23 14:24 ` ✓ CI.Hooks: " Patchwork
2024-04-23 14:26 ` ✗ CI.checksparse: warning " Patchwork
2024-04-23 14:48 ` ✓ CI.BAT: success " Patchwork
2024-04-23 21:41 ` ✓ CI.FULL: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ab3bcaa5-15b3-48a0-9916-58806b20ae06@linux.intel.com \
--to=aravind.iddamsetty@linux.intel.com \
--cc=airlied@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=rodrigo.vivi@intel.com \
--cc=thomas.hellstrom@linux.intel.com \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.