diff mbox series

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

Message ID 20230124134010.30263-4-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/fb-helper: Various cleanups | expand

Commit Message

Thomas Zimmermann Jan. 24, 2023, 1:40 p.m. UTC
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(-)

Comments

Javier Martinez Canillas Jan. 25, 2023, 9:18 a.m. UTC | #1
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.

> +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 Jan. 25, 2023, 2:11 p.m. UTC | #2
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>
>
diff mbox series

Patch

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);
+}
+EXPORT_SYMBOL(drm_fb_helper_unprepare);
+
 /**
  * drm_fb_helper_init - initialize a &struct drm_fb_helper
  * @dev: drm device
@@ -561,7 +573,7 @@  void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	}
 	mutex_unlock(&kernel_fb_helper_lock);
 
-	mutex_destroy(&fb_helper->lock);
+	drm_fb_helper_unprepare(fb_helper);
 
 	if (!fb_helper->client.funcs)
 		drm_client_release(&fb_helper->client);
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)
+{
+}
+
 static inline int drm_fb_helper_init(struct drm_device *dev,
 		       struct drm_fb_helper *helper)
 {