Re: [PATCH v2 10/10] drm/fb-helper: Remove return value from drm_fbdev_generic_setup()

[Sam Ravnborg <sam@ravnborg.org>]

Hi Thomas.

You missed my ack on the first 9 patches:
https://lore.kernel.org/dri-devel/20200407101354.GA12686@ravnborg.org/
Not that it matters as others have acked/reviewed them.

On Wed, Apr 08, 2020 at 10:26:41AM +0200, Thomas Zimmermann wrote:
> Generic fbdev emulation is a DRM client. Drivers should invoke the
> setup function, but not depend on its success. Hence remove the return
> value.
> 
> v2:
> 	* warn if fbdev device has not been registered yet
> 	* document the new behavior
> 	* convert the existing warning to the new dev_ interface
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> Acked-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 25 +++++++++++++------------
>  include/drm/drm_fb_helper.h     |  5 +++--
>  2 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 165c8dab50797..97f5e43837486 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -2169,7 +2169,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   *                 @dev->mode_config.preferred_depth is used if this is zero.
>   *
>   * This function sets up generic fbdev emulation for drivers that supports
> - * dumb buffers with a virtual address and that can be mmap'ed.
> + * dumb buffers with a virtual address and that can be mmap'ed. It's supposed
> + * to run after the DRM driver registered the new DRM device with
> + * drm_dev_register().
OR maybe be more explicit - something like:
drm_fbdev_generic_setup() shall be called after the DRM is registered
with drm_dev_register().

Either way is fine.

	Sam

>   *
>   * Restore, hotplug events and teardown are all taken care of. Drivers that do
>   * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
> @@ -2186,29 +2188,30 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>   * Setup will be retried on the next hotplug event.
>   *
>   * The fbdev is destroyed by drm_dev_unregister().
> - *
> - * Returns:
> - * Zero on success or negative error code on failure.
>   */
> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
> +void drm_fbdev_generic_setup(struct drm_device *dev,
> +			     unsigned int preferred_bpp)
>  {
>  	struct drm_fb_helper *fb_helper;
>  	int ret;
>  
> -	WARN(dev->fb_helper, "fb_helper is already set!\n");
> +	drm_WARN(dev, !dev->registered, "Device has not been registered.\n");
> +	drm_WARN(dev, dev->fb_helper, "fb_helper is already set!\n");
>  
>  	if (!drm_fbdev_emulation)
> -		return 0;
> +		return;
>  
>  	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
> -	if (!fb_helper)
> -		return -ENOMEM;
> +	if (!fb_helper) {
> +		drm_err(dev, "Failed to allocate fb_helper\n");
> +		return;
> +	}
>  
>  	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
>  	if (ret) {
>  		kfree(fb_helper);
>  		drm_err(dev, "Failed to register client: %d\n", ret);
> -		return ret;
> +		return;
>  	}
>  
>  	if (!preferred_bpp)
> @@ -2222,8 +2225,6 @@ int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>  
>  	drm_client_register(&fb_helper->client);
> -
> -	return 0;
>  }
>  EXPORT_SYMBOL(drm_fbdev_generic_setup);
>  
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 208dbf87afa3e..fb037be83997d 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -269,7 +269,8 @@ int drm_fb_helper_debug_leave(struct fb_info *info);
>  void drm_fb_helper_lastclose(struct drm_device *dev);
>  void drm_fb_helper_output_poll_changed(struct drm_device *dev);
>  
> -int drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp);
> +void drm_fbdev_generic_setup(struct drm_device *dev,
> +			     unsigned int preferred_bpp);
>  #else
>  static inline void drm_fb_helper_prepare(struct drm_device *dev,
>  					struct drm_fb_helper *helper,
> @@ -443,7 +444,7 @@ static inline void drm_fb_helper_output_poll_changed(struct drm_device *dev)
>  {
>  }
>  
> -static inline int
> +static inline void
>  drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>  {
>  	return 0;
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel