| Message ID | 20230124134010.30263-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm/fb-helper: Various cleanups | expand |
Hello Thomas, On 1/24/23 14:40, Thomas Zimmermann wrote: > Test for connectors in the client code and remove a similar test > from the generic fbdev emulation. Do nothing if the test fails. > Not having connectors indicates a driver bug. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> but I've a question below. > drivers/gpu/drm/drm_client.c | 5 +++++ > drivers/gpu/drm/drm_fbdev_generic.c | 5 ----- > 2 files changed, 5 insertions(+), 5 deletions(-) [...] > --- a/drivers/gpu/drm/drm_fbdev_generic.c > +++ b/drivers/gpu/drm/drm_fbdev_generic.c > @@ -389,11 +389,6 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) > if (dev->fb_helper) > return drm_fb_helper_hotplug_event(dev->fb_helper); > > - if (!dev->mode_config.num_connector) { > - drm_dbg_kms(dev, "No connectors found, will not create framebuffer!\n"); > - return 0; > - } > - What about the following code snippet: if (!drm_drv_uses_atomic_modeset(dev)) drm_helper_disable_unused_functions(dev); that seems to be something that should be in the core client dev hotplug as well, since it isn't specific to the fbdev emulation client ?
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>
On 1/24/23 14:40, Thomas Zimmermann wrote: > Initialize the fb-helper structure immediately after its allocation > in drm_fbdev_generic_setup(). That will make it easier to fill it with > driver-specific values, such as the preferred BPP. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- [...] > @@ -456,12 +454,12 @@ void drm_fbdev_generic_setup(struct drm_device *dev, > fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL); > if (!fb_helper) > return; Maybe add a new line here? > + drm_fb_helper_prepare(dev, fb_helper, &drm_fb_helper_generic_funcs); > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On 1/24/23 14:40, Thomas Zimmermann wrote: > Initialize the fb-helper's preferred_bpp field early from within > drm_fb_helper_prepare(); instead of the later client hot-plugging > callback. This simplifies the generic fbdev setup function. > > No real changes, but all drivers' fbdev code has to be adapted. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On 1/24/23 14:40, Thomas Zimmermann wrote: > Call drm_fb_helper_init() in the generic-fbdev hotplug helper drm_fb_helper_fini() > to revert the effects of drm_fb_helper_init(). No full cleanup > is required. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
On 1/24/23 14:40, Thomas Zimmermann wrote: > For uninitialized framebuffers, only release the DRM client and > free the fbdev memory. Do not attempt to clean up the framebuffer. > > DRM fbdev clients have a two-step initialization: first create > the DRM client; then create the framebuffer device on the first > successful hotplug event. In cases where the client never creates > the framebuffer, only the client state needs to be released. We > can detect which case it is, full or client-only cleanup, be s/be/by > looking at the presence of fb_helper's info field. > > v2: > * remove test for (fbi != NULL) in drm_fbdev_cleanup() (Sam) > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Am 25.01.23 um 09:30 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 1/24/23 14:40, Thomas Zimmermann wrote: >> Test for connectors in the client code and remove a similar test >> from the generic fbdev emulation. Do nothing if the test fails. >> Not having connectors indicates a driver bug. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > > but I've a question below. > >> drivers/gpu/drm/drm_client.c | 5 +++++ >> drivers/gpu/drm/drm_fbdev_generic.c | 5 ----- >> 2 files changed, 5 insertions(+), 5 deletions(-) > > [...] > >> --- a/drivers/gpu/drm/drm_fbdev_generic.c >> +++ b/drivers/gpu/drm/drm_fbdev_generic.c >> @@ -389,11 +389,6 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) >> if (dev->fb_helper) >> return drm_fb_helper_hotplug_event(dev->fb_helper); >> >> - if (!dev->mode_config.num_connector) { >> - drm_dbg_kms(dev, "No connectors found, will not create framebuffer!\n"); >> - return 0; >> - } >> - > > What about the following code snippet: > > if (!drm_drv_uses_atomic_modeset(dev)) > drm_helper_disable_unused_functions(dev); > > that seems to be something that should be in the core client dev hotplug > as well, since it isn't specific to the fbdev emulation client ? That's in the middle of the initial probing code and disables pipeline elements in non-atomic modesetting. TBH I don't dare to move it around. If we ever have other clients, we can attempt the put it into the client. Best regards Thomas >
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> >
Add various cleanups and changes to DRM's fbdev helpers and the generic fbdev emulation. There's no clear theme here, just lots of small things that need to be updated. In the end, the code will better reflect which parts are in the DRM client, which is fbdev emulation, and which are shared fbdev helpers. v2: * cleanups in drm_fbdev_fb_destroy() (Sam) * fix declaration of drm_fb_helper_unprepare() Thomas Zimmermann (10): drm/client: Test for connectors before sending hotplug event drm/client: Add hotplug_failed flag drm/fb-helper: Introduce drm_fb_helper_unprepare() drm/fbdev-generic: Initialize fb-helper structure in generic setup drm/fb-helper: Remove preferred_bpp parameter from fbdev internals drm/fb-helper: Initialize fb-helper's preferred BPP in prepare function drm/fbdev-generic: Minimize hotplug error handling drm/fbdev-generic: Minimize client unregistering drm/fbdev-generic: Inline clean-up helpers into drm_fbdev_fb_destroy() drm/fbdev-generic: Rename struct fb_info 'fbi' to 'info' drivers/gpu/drm/armada/armada_fbdev.c | 4 +- drivers/gpu/drm/drm_client.c | 10 ++ drivers/gpu/drm/drm_fb_helper.c | 58 ++++++--- drivers/gpu/drm/drm_fbdev_generic.c | 131 ++++++++------------- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 4 +- drivers/gpu/drm/gma500/framebuffer.c | 4 +- drivers/gpu/drm/i915/display/intel_fbdev.c | 11 +- drivers/gpu/drm/msm/msm_fbdev.c | 4 +- drivers/gpu/drm/omapdrm/omap_fbdev.c | 4 +- drivers/gpu/drm/radeon/radeon_fb.c | 4 +- drivers/gpu/drm/tegra/fb.c | 7 +- include/drm/drm_client.h | 8 ++ include/drm/drm_fb_helper.h | 8 +- 13 files changed, 134 insertions(+), 123 deletions(-) base-commit: 7d3e7f64a42d66ba8da6e7b66a8d85457ef84570