| Message ID | 20230125200415.14123-1-tzimmermann@suse.de |
|---|---|
| Headers | show |
| Series | drm/fb-helper: Various cleanups | expand |
Hi Thomas, On Wed, Jan 25, 2023 at 09:04:07PM +0100, Thomas Zimmermann wrote: > Signal failed hotplugging with a flag in struct drm_client_dev. If set, > the client helpers will not further try to set up the fbdev display. > > This used to be signalled with a combination of cleared pointers in > struct drm_fb_helper, I failed to find where we clear the pointers. What do I miss? (I had assumed we would stop clearing the pointers after this change). Sam which prevents us from initializing these pointers > early after allocation. > > The change also harmonizes behavior among DRM clients. Additional DRM > clients will now handle failed hotplugging like fbdev does. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/gpu/drm/drm_client.c | 5 +++++ > drivers/gpu/drm/drm_fbdev_generic.c | 4 ---- > include/drm/drm_client.h | 8 ++++++++ > 3 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > index 09ac191c202d..009e7b10455c 100644 > --- a/drivers/gpu/drm/drm_client.c > +++ b/drivers/gpu/drm/drm_client.c > @@ -208,8 +208,13 @@ void drm_client_dev_hotplug(struct drm_device *dev) > if (!client->funcs || !client->funcs->hotplug) > continue; > > + if (client->hotplug_failed) > + continue; > + > ret = client->funcs->hotplug(client); > drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); > + if (ret) > + client->hotplug_failed = true; > } > mutex_unlock(&dev->clientlist_mutex); > } > diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c > index 3d455a2e3fb5..135d58b8007b 100644 > --- a/drivers/gpu/drm/drm_fbdev_generic.c > +++ b/drivers/gpu/drm/drm_fbdev_generic.c > @@ -382,10 +382,6 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) > struct drm_device *dev = client->dev; > int ret; > > - /* Setup is not retried if it has failed */ > - if (!fb_helper->dev && fb_helper->funcs) > - return 0; > - > if (dev->fb_helper) > return drm_fb_helper_hotplug_event(dev->fb_helper); > > diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h > index 4fc8018eddda..39482527a775 100644 > --- a/include/drm/drm_client.h > +++ b/include/drm/drm_client.h > @@ -106,6 +106,14 @@ struct drm_client_dev { > * @modesets: CRTC configurations > */ > struct drm_mode_set *modesets; > + > + /** > + * @hotplug failed: > + * > + * Set by client hotplug helpers if the hotplugging failed > + * before. It is usually not tried again. > + */ > + bool hotplug_failed; > }; > > int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, > -- > 2.39.0
Hi Am 25.01.23 um 21:52 schrieb Sam Ravnborg: > Hi Thomas, > > On Wed, Jan 25, 2023 at 09:04:06PM +0100, 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> >> --- >> drivers/gpu/drm/drm_client.c | 5 +++++ >> drivers/gpu/drm/drm_fbdev_generic.c | 5 ----- >> 2 files changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 262ec64d4397..09ac191c202d 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -198,6 +198,11 @@ void drm_client_dev_hotplug(struct drm_device *dev) >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> return; >> >> + if (!dev->mode_config.num_connector) { >> + drm_dbg_kms(dev, "No connectors found, will not send hotplug events!\n"); >> + return; > This deserves a more visible logging - if a driver fails here it would > be good to spot it in the normal kernel log. > drm_info or drm_notice? But is that really noteworthy? AFAIK, this situation can legally happen. So if it's expected, why should we print a message about it? Best regards Thomas > > The original code had this on the debug level, but when moving the log > level could also be updated. > > Sam > >> + } >> + >> mutex_lock(&dev->clientlist_mutex); >> list_for_each_entry(client, &dev->clientlist, list) { >> if (!client->funcs || !client->funcs->hotplug) >> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c >> index 0a4c160e0e58..3d455a2e3fb5 100644 >> --- 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; >> - } >> - >> drm_fb_helper_prepare(dev, fb_helper, &drm_fb_helper_generic_funcs); >> >> ret = drm_fb_helper_init(dev, fb_helper); >> -- >> 2.39.0
Hi Am 25.01.23 um 21:57 schrieb Sam Ravnborg: > Hi Thomas, > > On Wed, Jan 25, 2023 at 09:04:07PM +0100, Thomas Zimmermann wrote: >> Signal failed hotplugging with a flag in struct drm_client_dev. If set, >> the client helpers will not further try to set up the fbdev display. >> >> This used to be signalled with a combination of cleared pointers in >> struct drm_fb_helper, > I failed to find where we clear the pointers. What do I miss? Those pointer fields, dev and funcs, where allocated with kzalloc(). The error path in drm_fbdev_client_hotplug() later reset them to NULL again if an error occured. Best regards Thomas > (I had assumed we would stop clearing the pointers after this change). > > Sam > > which prevents us from initializing these pointers >> early after allocation. >> >> The change also harmonizes behavior among DRM clients. Additional DRM >> clients will now handle failed hotplugging like fbdev does. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> >> --- >> drivers/gpu/drm/drm_client.c | 5 +++++ >> drivers/gpu/drm/drm_fbdev_generic.c | 4 ---- >> include/drm/drm_client.h | 8 ++++++++ >> 3 files changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c >> index 09ac191c202d..009e7b10455c 100644 >> --- a/drivers/gpu/drm/drm_client.c >> +++ b/drivers/gpu/drm/drm_client.c >> @@ -208,8 +208,13 @@ void drm_client_dev_hotplug(struct drm_device *dev) >> if (!client->funcs || !client->funcs->hotplug) >> continue; >> >> + if (client->hotplug_failed) >> + continue; >> + >> ret = client->funcs->hotplug(client); >> drm_dbg_kms(dev, "%s: ret=%d\n", client->name, ret); >> + if (ret) >> + client->hotplug_failed = true; >> } >> mutex_unlock(&dev->clientlist_mutex); >> } >> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c >> index 3d455a2e3fb5..135d58b8007b 100644 >> --- a/drivers/gpu/drm/drm_fbdev_generic.c >> +++ b/drivers/gpu/drm/drm_fbdev_generic.c >> @@ -382,10 +382,6 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client) >> struct drm_device *dev = client->dev; >> int ret; >> >> - /* Setup is not retried if it has failed */ >> - if (!fb_helper->dev && fb_helper->funcs) >> - return 0; >> - >> if (dev->fb_helper) >> return drm_fb_helper_hotplug_event(dev->fb_helper); >> >> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h >> index 4fc8018eddda..39482527a775 100644 >> --- a/include/drm/drm_client.h >> +++ b/include/drm/drm_client.h >> @@ -106,6 +106,14 @@ struct drm_client_dev { >> * @modesets: CRTC configurations >> */ >> struct drm_mode_set *modesets; >> + >> + /** >> + * @hotplug failed: >> + * >> + * Set by client hotplug helpers if the hotplugging failed >> + * before. It is usually not tried again. >> + */ >> + bool hotplug_failed; >> }; >> >> int drm_client_init(struct drm_device *dev, struct drm_client_dev *client, >> -- >> 2.39.0
On Fri, Jan 27, 2023 at 03:13:50PM +0100, Thomas Zimmermann wrote: > Hi > > Am 25.01.23 um 21:52 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Jan 25, 2023 at 09:04:06PM +0100, 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> > > > --- > > > drivers/gpu/drm/drm_client.c | 5 +++++ > > > drivers/gpu/drm/drm_fbdev_generic.c | 5 ----- > > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > > > index 262ec64d4397..09ac191c202d 100644 > > > --- a/drivers/gpu/drm/drm_client.c > > > +++ b/drivers/gpu/drm/drm_client.c > > > @@ -198,6 +198,11 @@ void drm_client_dev_hotplug(struct drm_device *dev) > > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > > > return; > > > + if (!dev->mode_config.num_connector) { > > > + drm_dbg_kms(dev, "No connectors found, will not send hotplug events!\n"); > > > + return; > > This deserves a more visible logging - if a driver fails here it would > > be good to spot it in the normal kernel log. > > drm_info or drm_notice? > > But is that really noteworthy? AFAIK, this situation can legally happen. So > if it's expected, why should we print a message about it? I was reading it as a driver error - as that's not the case current code is fine. Sam
On Wednesday, January 25th, 2023 at 21:04, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Not having connectors indicates a driver bug.
Is it? What if all connectors are of the DP-MST type, ie. they are
created on-the-fly?
Hi Am 27.01.23 um 19:02 schrieb Simon Ser: > On Wednesday, January 25th, 2023 at 21:04, Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> Not having connectors indicates a driver bug. > > Is it? What if all connectors are of the DP-MST type, ie. they are > created on-the-fly? My commit message was nonsense. I even write this here that having no connectors is legitimate. Best regards Thomas
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. v3: * various minor fixes (Javier)) * build with CONFIG_DRM_FBDEV_EMULATION unset (kernel test robot) 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 | 16 ++- 13 files changed, 138 insertions(+), 127 deletions(-) base-commit: 7d3e7f64a42d66ba8da6e7b66a8d85457ef84570