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

Hi Sam

Am 08.04.20 um 10:50 schrieb Sam Ravnborg:

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.

This got lost. I'll add you acks. Thanks for taking another look at the patches.

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().

I think this one is even better.

Best regards Thomas

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