Re: [PATCH v2 03/10] drm/fb-helper: Introduce drm_fb_helper_unprepare()

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3600 bytes --]

Hi

Am 25.01.23 um 10:18 schrieb Javier Martinez Canillas:
> On 1/24/23 14:40, Thomas Zimmermann wrote:
>> Move the fb-helper clean-up code into drm_fb_helper_unprepare(). No
>> functional changes.
>>
>> v2:
>> 	* declare as static inline (kernel test robot)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 14 +++++++++++++-
>>   include/drm/drm_fb_helper.h     |  5 +++++
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index c5c13e192b64..4379bcd7718b 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -435,6 +435,18 @@ void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_prepare);
>>   
>> +/**
>> + * drm_fb_helper_unprepare - clean up a drm_fb_helper structure
>> + * @fb_helper: driver-allocated fbdev helper structure to set up
>> + *
>> + * Cleans up the framebuffer helper. Inverse of drm_fb_helper_prepare().
>> + */
>> +void drm_fb_helper_unprepare(struct drm_fb_helper *fb_helper)
>> +{
>> +	mutex_destroy(&fb_helper->lock);
>> +}
> 
> I like that we have an _unprepare that is the inverse of the _prepare, but
> since is only destroying the mutex, maybe is an unneeded indirection level?
> 
> Or do you plan to add more cleanup to that _unprepare function? Otherwise I
> would just make it an inline function.

There could possibly be more; not sure yet. With patch 04, the call is 
being invoked from drm_fbdev_generic.c. And I have prototype patches 
that convert a number of driver-internal fbdevs to use struct 
drm_client. They all follow the same design/structure/pattern that is 
being layed out in generic-fbdev in this patchset. In the end 
drm_fb_helper_unprepare will definitely be a public interface of the 
fbdev helpers.

Best regards
Thomas

> 
>> +EXPORT_SYMBOL(drm_fb_helper_unprepare);
>> +
> 
> Does it have to be an exported symbol? AFAICT the only user for now is the
> drm_fb_helper_fini() function, so the function could be a static inline.
> 
> [...]
> 
>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>> index f443e1f11654..39710c570a04 100644
>> --- a/include/drm/drm_fb_helper.h
>> +++ b/include/drm/drm_fb_helper.h
>> @@ -230,6 +230,7 @@ drm_fb_helper_from_client(struct drm_client_dev *client)
>>   #ifdef CONFIG_DRM_FBDEV_EMULATION
>>   void drm_fb_helper_prepare(struct drm_device *dev, struct drm_fb_helper *helper,
>>   			   const struct drm_fb_helper_funcs *funcs);
>> +void drm_fb_helper_unprepare(struct drm_fb_helper *fb_helper);
>>   int drm_fb_helper_init(struct drm_device *dev, struct drm_fb_helper *helper);
>>   void drm_fb_helper_fini(struct drm_fb_helper *helper);
>>   int drm_fb_helper_blank(int blank, struct fb_info *info);
>> @@ -296,6 +297,10 @@ static inline void drm_fb_helper_prepare(struct drm_device *dev,
>>   {
>>   }
>>   
>> +static inline void drm_fb_helper_unprepare(struct drm_fb_helper *fb_helper)
>> +{
>> +}
>> +
> 
> And you should be able to remove this stub if you limit the scope of the helper.
> 
> No strong opinion though. So if you prefer to keep it as is, feel free to add:
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]