[PATCH v3 1/4] drm: add devm release action

[Aravind Iddamsetty]

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 at kernel.org>
>> Cc: Thomas Hellstr_m <thomas.hellstrom at linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi at 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 at intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at 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 ?
>
>> + * @dev: DRM device
>> + *
>> + * 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.
> But also, this made me more confusing on why this is needed.
> Why can't we call put and get back?
IIUC, the ownership of drm_dev_get is with devm_drm_dev_alloc
and the release is tied to a devm action hence we needed this.

>
> The next needs to make it clear on why we need to release in these
> scenarios regarless of the number of counters. But do we really
> want this?
in our case post tFLR we need to reprobe the device, but the previousdrm instance
is not destroyed with just calling pci_remove as it is tied to PDEV lifetime
which is not destroyed hence we need to call the last release action ourself
so that the final release is called.
>
> Can we block the flr if we have users? Perhaps this is the reason
> that on my side the flr was not that clean? beucase I had display
> so I had clients connected?
the display side error is due to power wells, post FLR the power wells are already
disabled while we try to disable them from driver again it is throwing warnings.

Thanks,

Aravind.
>
>> + */
>> +void devm_drm_dev_release_action(struct drm_device *dev)
>> +{
>> +	devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
>> +}
>> +EXPORT_SYMBOL(devm_drm_dev_release_action);
>> +
>>  void *__devm_drm_dev_alloc(struct device *parent,
>>  			   const struct drm_driver *driver,
>>  			   size_t size, size_t offset)
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 8878260d7529..fa9123684874 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -444,6 +444,8 @@ struct drm_driver {
>>  	const struct file_operations *fops;
>>  };
>>  
>> +void devm_drm_dev_release_action(struct drm_device *dev);
>> +
>>  void *__devm_drm_dev_alloc(struct device *parent,
>>  			   const struct drm_driver *driver,
>>  			   size_t size, size_t offset);
>> -- 
>> 2.25.1
>>