| Message ID | 20200408082641.590-11-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set up generic fbdev after registering device | expand |
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
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
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(). * * 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;