| Message ID | 20170321081358.27237-7-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > FB helper code falls back to a 1024x768 mode if no outputs are connected > or don't report back any modes upon initialization. This can be annoying > because outputs that are added to FB helper later on can't be used with > FB helper if they don't support a matching mode. > > The fallback is in place because VGA connectors can happen to report an > unknown connection status even when they are in fact connected. > > Some drivers have custom solutions in place to defer FB helper setup > until at least one output is connected. But the logic behind these > solutions is always the same and there is nothing driver-specific about > it, so a better alterative is to fix the FB helper core and add support > for all drivers automatically. > > This patch adds support for deferred FB helper setup. It checks all the > connectors for their connection status, and if all of them report to be > disconnected marks the FB helper as needing deferred setup. Whet setup > is deferred, the FB helper core will automatically retry setup after a > hotplug event, and it will keep trying until it succeeds. > > Tested-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Thierry Reding <treding@nvidia.com> Tiny nitpicks below, with those addressed: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/drm_fb_helper.c | 60 +++++++++++++++++++++++++++++++++++++---- > include/drm/drm_fb_helper.h | 21 +++++++++++++++ > 2 files changed, 76 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 9060adcf7cf8..d4a2c97d8b02 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -511,6 +511,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > if (!drm_fbdev_emulation) > return -ENODEV; > > + if (fb_helper->deferred_setup) > + return 0; READ_ONCE or put it under the ->lock protection. Just for clarity. > + > mutex_lock(&fb_helper->lock); > drm_modeset_lock_all(dev); > > @@ -1597,6 +1600,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > } > EXPORT_SYMBOL(drm_fb_helper_pan_display); > > +static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper) > +{ > + bool connected = false; > + unsigned int i; > + > + for (i = 0; i < helper->connector_count; i++) { > + struct drm_fb_helper_connector *fb = helper->connector_info[i]; > + > + if (fb->connector->status != connector_status_disconnected) { > + connected = true; > + break; > + } > + } > + > + return connected; > +} > + > /* > * Allocates the backing storage and sets up the fbdev info structure through > * the ->fb_probe callback and then registers the fbdev and sets up the panic > @@ -2254,8 +2274,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > int i; > > DRM_DEBUG_KMS("\n"); > - if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > - DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > > /* prevent concurrent modification of connector_count by hotplug */ > lockdep_assert_held(&fb_helper->dev->mode_config.mutex); > @@ -2378,6 +2396,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) > { > struct drm_device *dev = fb_helper->dev; > + unsigned int width, height; > struct fb_info *info; > int ret; > > @@ -2385,14 +2404,34 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) > return 0; > > mutex_lock(&dev->mode_config.mutex); > - drm_setup_crtcs(fb_helper, > - dev->mode_config.max_width, > - dev->mode_config.max_height); > + > + width = dev->mode_config.max_width; > + height = dev->mode_config.max_height; > + > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > + > + /* > + * If everything's disconnected, there's no use in attempting to set > + * up fbdev. > + */ > + if (!drm_fb_helper_maybe_connected(fb_helper)) { > + DRM_INFO("No outputs connected, deferring setup\n"); > + fb_helper->preferred_bpp = bpp_sel; > + fb_helper->deferred_setup = true; > + mutex_unlock(&dev->mode_config.mutex); > + return 0; > + } > + > + drm_setup_crtcs(fb_helper, width, height); > + > ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); > mutex_unlock(&dev->mode_config.mutex); > if (ret) > return ret; > > + fb_helper->deferred_setup = false; > + > info = fb_helper->fbdev; > info->var.pixclock = 0; > ret = register_framebuffer(info); > @@ -2437,11 +2476,16 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > struct drm_device *dev = fb_helper->dev; > + unsigned int width, height; > int err = 0; > > if (!drm_fbdev_emulation) > return 0; > > + if (fb_helper->deferred_setup) > + return drm_fb_helper_initial_config(fb_helper, > + fb_helper->preferred_bpp); I think this must be moved under the protection of ->lock, you might race otherwise (e.g. hpd vs. userspace forcing a re-probe, both noticing the change). > + > mutex_lock(&fb_helper->lock); > mutex_lock(&dev->mode_config.mutex); > > @@ -2453,6 +2497,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > > DRM_DEBUG_KMS("\n"); > > + width = dev->mode_config.max_width; > + height = dev->mode_config.max_height; > + > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > + > drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height); > > mutex_unlock(&dev->mode_config.mutex); > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index 33771d0c44e3..41c3587751f8 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -222,6 +222,27 @@ struct drm_fb_helper { > * needs to be reprobe when fbdev is in control again. > */ > bool delayed_hotplug; > + > + /** > + * @deferred_setup: > + * > + * If no outputs are connected (disconnected or unknown) the FB helper > + * code will defer setup until at least one of the outputs shows up. > + * This field keeps track of the status so that setup can be retried > + * at every hotplug event until it succeeds eventually. Please add: "Protected by @lock." > + */ > + bool deferred_setup; > + > + /** > + * @preferred_bpp: > + * > + * Temporary storage for the driver's preferred BPP setting passed to > + * FB helper initialization. This needs to be tracked so that deferred > + * FB helper setup can pass this on. > + * > + * See also: @deferred_setup > + */ > + int preferred_bpp; > }; > > /** > -- > 2.12.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Mar 21, 2017 at 11:10:22AM +0100, Daniel Vetter wrote: > On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote: [...] > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c [...] > > @@ -2437,11 +2476,16 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > > { > > struct drm_device *dev = fb_helper->dev; > > + unsigned int width, height; > > int err = 0; > > > > if (!drm_fbdev_emulation) > > return 0; > > > > + if (fb_helper->deferred_setup) > > + return drm_fb_helper_initial_config(fb_helper, > > + fb_helper->preferred_bpp); > > I think this must be moved under the protection of ->lock, you might race > otherwise (e.g. hpd vs. userspace forcing a re-probe, both noticing the > change). I think I had originally put this under the lock only to see that result in a deadlock. I can't quite remember why that was, but testing shows that this still happens. It's getting rather late, so I'll have to defer tracking this down to tomorrow. Thierry
On Wed, Mar 22, 2017 at 10:06 PM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Mar 21, 2017 at 11:10:22AM +0100, Daniel Vetter wrote: >> On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote: > [...] >> > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > [...] >> > @@ -2437,11 +2476,16 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); >> > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) >> > { >> > struct drm_device *dev = fb_helper->dev; >> > + unsigned int width, height; >> > int err = 0; >> > >> > if (!drm_fbdev_emulation) >> > return 0; >> > >> > + if (fb_helper->deferred_setup) >> > + return drm_fb_helper_initial_config(fb_helper, >> > + fb_helper->preferred_bpp); >> >> I think this must be moved under the protection of ->lock, you might race >> otherwise (e.g. hpd vs. userspace forcing a re-probe, both noticing the >> change). > > I think I had originally put this under the lock only to see that result > in a deadlock. I can't quite remember why that was, but testing shows > that this still happens. It's getting rather late, so I'll have to defer > tracking this down to tomorrow. initial_config also grabs your new lock, which means you'd need more static _unlocked variants to make it work. I didn't see any other obvious problem this would cause. -Daniel
On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > FB helper code falls back to a 1024x768 mode if no outputs are connected > or don't report back any modes upon initialization. This can be annoying > because outputs that are added to FB helper later on can't be used with > FB helper if they don't support a matching mode. > > The fallback is in place because VGA connectors can happen to report an > unknown connection status even when they are in fact connected. > > Some drivers have custom solutions in place to defer FB helper setup > until at least one output is connected. But the logic behind these > solutions is always the same and there is nothing driver-specific about > it, so a better alterative is to fix the FB helper core and add support > for all drivers automatically. > > This patch adds support for deferred FB helper setup. It checks all the > connectors for their connection status, and if all of them report to be > disconnected marks the FB helper as needing deferred setup. Whet setup > is deferred, the FB helper core will automatically retry setup after a > hotplug event, and it will keep trying until it succeeds. > > Tested-by: John Stultz <john.stultz@linaro.org> > Signed-off-by: Thierry Reding <treding@nvidia.com> Ok 2nd attempt at making this work, probably easier to go back to v2. > --- > drivers/gpu/drm/drm_fb_helper.c | 60 +++++++++++++++++++++++++++++++++++++---- > include/drm/drm_fb_helper.h | 21 +++++++++++++++ > 2 files changed, 76 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 9060adcf7cf8..d4a2c97d8b02 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -511,6 +511,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) > if (!drm_fbdev_emulation) > return -ENODEV; > > + if (fb_helper->deferred_setup) > + return 0; Please wrap in READ_ONCE to make it clear we're doing lockless checking here. > + > mutex_lock(&fb_helper->lock); > drm_modeset_lock_all(dev); > > @@ -1597,6 +1600,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, > } > EXPORT_SYMBOL(drm_fb_helper_pan_display); > > +static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper) > +{ > + bool connected = false; > + unsigned int i; > + > + for (i = 0; i < helper->connector_count; i++) { > + struct drm_fb_helper_connector *fb = helper->connector_info[i]; > + > + if (fb->connector->status != connector_status_disconnected) { > + connected = true; > + break; > + } > + } > + > + return connected; > +} > + > /* > * Allocates the backing storage and sets up the fbdev info structure through > * the ->fb_probe callback and then registers the fbdev and sets up the panic > @@ -2254,8 +2274,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > int i; > > DRM_DEBUG_KMS("\n"); > - if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > - DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > > /* prevent concurrent modification of connector_count by hotplug */ > lockdep_assert_held(&fb_helper->dev->mode_config.mutex); > @@ -2378,6 +2396,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) > { > struct drm_device *dev = fb_helper->dev; > + unsigned int width, height; > struct fb_info *info; > int ret; > > @@ -2385,14 +2404,34 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) > return 0; > From here ... > mutex_lock(&dev->mode_config.mutex); > - drm_setup_crtcs(fb_helper, > - dev->mode_config.max_width, > - dev->mode_config.max_height); > + > + width = dev->mode_config.max_width; > + height = dev->mode_config.max_height; > + > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > + > + /* > + * If everything's disconnected, there's no use in attempting to set > + * up fbdev. > + */ > + if (!drm_fb_helper_maybe_connected(fb_helper)) { > + DRM_INFO("No outputs connected, deferring setup\n"); > + fb_helper->preferred_bpp = bpp_sel; > + fb_helper->deferred_setup = true; > + mutex_unlock(&dev->mode_config.mutex); > + return 0; > + } > + > + drm_setup_crtcs(fb_helper, width, height); > + > ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); > mutex_unlock(&dev->mode_config.mutex); > if (ret) > return ret; > > + fb_helper->deferred_setup = false; To here you need to hold the new fb_helper->mutex. Plus at the top, after you've grabbed the lock, you need to re-check ->deferred_setup. This way we guaranteed that only 1 thread can ever do the deferred setup, while not holding the lock while we call register_framebuffer. If you hold any fbdev or kms lock while calling register_framebuffer you will deadlock. > + > info = fb_helper->fbdev; > info->var.pixclock = 0; > ret = register_framebuffer(info); > @@ -2437,11 +2476,16 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); > int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > { > struct drm_device *dev = fb_helper->dev; > + unsigned int width, height; > int err = 0; > > if (!drm_fbdev_emulation) > return 0; > > + if (fb_helper->deferred_setup) Same here, please wrap in READ_ONCE, this is just a fastpath check. With those changes your prep patch to roll out fb_helper->mutex more shouldn't be needed at all. Cheers, Daniel > + return drm_fb_helper_initial_config(fb_helper, > + fb_helper->preferred_bpp); > + > mutex_lock(&fb_helper->lock); > mutex_lock(&dev->mode_config.mutex); > > @@ -2453,6 +2497,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) > > DRM_DEBUG_KMS("\n"); > > + width = dev->mode_config.max_width; > + height = dev->mode_config.max_height; > + > + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) > + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); > + > drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height); > > mutex_unlock(&dev->mode_config.mutex); > diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h > index 33771d0c44e3..41c3587751f8 100644 > --- a/include/drm/drm_fb_helper.h > +++ b/include/drm/drm_fb_helper.h > @@ -222,6 +222,27 @@ struct drm_fb_helper { > * needs to be reprobe when fbdev is in control again. > */ > bool delayed_hotplug; > + > + /** > + * @deferred_setup: > + * > + * If no outputs are connected (disconnected or unknown) the FB helper > + * code will defer setup until at least one of the outputs shows up. > + * This field keeps track of the status so that setup can be retried > + * at every hotplug event until it succeeds eventually. > + */ > + bool deferred_setup; > + > + /** > + * @preferred_bpp: > + * > + * Temporary storage for the driver's preferred BPP setting passed to > + * FB helper initialization. This needs to be tracked so that deferred > + * FB helper setup can pass this on. > + * > + * See also: @deferred_setup > + */ > + int preferred_bpp; > }; > > /** > -- > 2.12.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 9060adcf7cf8..d4a2c97d8b02 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -511,6 +511,9 @@ int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper) if (!drm_fbdev_emulation) return -ENODEV; + if (fb_helper->deferred_setup) + return 0; + mutex_lock(&fb_helper->lock); drm_modeset_lock_all(dev); @@ -1597,6 +1600,23 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, } EXPORT_SYMBOL(drm_fb_helper_pan_display); +static bool drm_fb_helper_maybe_connected(struct drm_fb_helper *helper) +{ + bool connected = false; + unsigned int i; + + for (i = 0; i < helper->connector_count; i++) { + struct drm_fb_helper_connector *fb = helper->connector_info[i]; + + if (fb->connector->status != connector_status_disconnected) { + connected = true; + break; + } + } + + return connected; +} + /* * Allocates the backing storage and sets up the fbdev info structure through * the ->fb_probe callback and then registers the fbdev and sets up the panic @@ -2254,8 +2274,6 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, int i; DRM_DEBUG_KMS("\n"); - if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) - DRM_DEBUG_KMS("No connectors reported connected with modes\n"); /* prevent concurrent modification of connector_count by hotplug */ lockdep_assert_held(&fb_helper->dev->mode_config.mutex); @@ -2378,6 +2396,7 @@ static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) { struct drm_device *dev = fb_helper->dev; + unsigned int width, height; struct fb_info *info; int ret; @@ -2385,14 +2404,34 @@ int drm_fb_helper_initial_config(struct drm_fb_helper *fb_helper, int bpp_sel) return 0; mutex_lock(&dev->mode_config.mutex); - drm_setup_crtcs(fb_helper, - dev->mode_config.max_width, - dev->mode_config.max_height); + + width = dev->mode_config.max_width; + height = dev->mode_config.max_height; + + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); + + /* + * If everything's disconnected, there's no use in attempting to set + * up fbdev. + */ + if (!drm_fb_helper_maybe_connected(fb_helper)) { + DRM_INFO("No outputs connected, deferring setup\n"); + fb_helper->preferred_bpp = bpp_sel; + fb_helper->deferred_setup = true; + mutex_unlock(&dev->mode_config.mutex); + return 0; + } + + drm_setup_crtcs(fb_helper, width, height); + ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); mutex_unlock(&dev->mode_config.mutex); if (ret) return ret; + fb_helper->deferred_setup = false; + info = fb_helper->fbdev; info->var.pixclock = 0; ret = register_framebuffer(info); @@ -2437,11 +2476,16 @@ EXPORT_SYMBOL(drm_fb_helper_initial_config); int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) { struct drm_device *dev = fb_helper->dev; + unsigned int width, height; int err = 0; if (!drm_fbdev_emulation) return 0; + if (fb_helper->deferred_setup) + return drm_fb_helper_initial_config(fb_helper, + fb_helper->preferred_bpp); + mutex_lock(&fb_helper->lock); mutex_lock(&dev->mode_config.mutex); @@ -2453,6 +2497,12 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper) DRM_DEBUG_KMS("\n"); + width = dev->mode_config.max_width; + height = dev->mode_config.max_height; + + if (drm_fb_helper_probe_connector_modes(fb_helper, width, height) == 0) + DRM_DEBUG_KMS("No connectors reported connected with modes\n"); + drm_setup_crtcs(fb_helper, fb_helper->fb->width, fb_helper->fb->height); mutex_unlock(&dev->mode_config.mutex); diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 33771d0c44e3..41c3587751f8 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -222,6 +222,27 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug; + + /** + * @deferred_setup: + * + * If no outputs are connected (disconnected or unknown) the FB helper + * code will defer setup until at least one of the outputs shows up. + * This field keeps track of the status so that setup can be retried + * at every hotplug event until it succeeds eventually. + */ + bool deferred_setup; + + /** + * @preferred_bpp: + * + * Temporary storage for the driver's preferred BPP setting passed to + * FB helper initialization. This needs to be tracked so that deferred + * FB helper setup can pass this on. + * + * See also: @deferred_setup + */ + int preferred_bpp; }; /**