On Tue, Mar 21, 2017 at 09:13:54AM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@xxxxxxxxxx> > > 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@xxxxxxxxxx> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel