On Tue, Nov 29, 2016 at 10:28:03AM -0500, Sean Paul wrote: > On Tue, Nov 29, 2016 at 7:02 AM, Chris Wilson <chris at chris-wilson.co.uk> > wrote: > > drm_fb_helper_probe_connector_modes() is always called before > > drm_setup_crtcs(), so just move the call into drm_setup_crtcs for a > > small bit of code compaction. > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk> > > Reviewed-by: Daniel Vetter <daniel.vetter at intel.com> > > --- > > drivers/gpu/drm/drm_fb_helper.c | 37 +++++++++++-------------------------- > > 1 file changed, 11 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > b/drivers/gpu/drm/drm_fb_helper.c > > index 380a0ec01033..90da28d2fcf3 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -2099,20 +2099,19 @@ static int drm_pick_crtcs(struct drm_fb_helper > > *fb_helper, > > return best_score; > > } > > > > -static void drm_setup_crtcs(struct drm_fb_helper *fb_helper) > > +static void drm_setup_crtcs(struct drm_fb_helper *fb_helper, > > + u32 width, u32 height) > > { > > struct drm_device *dev = fb_helper->dev; > > struct drm_fb_helper_crtc **crtcs; > > struct drm_display_mode **modes; > > struct drm_fb_offset *offsets; > > bool *enabled; > > - int width, height; > > int i; > > > > 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"); > > > > /* prevent concurrent modification of connector_count by hotplug */ > > lockdep_assert_held(&fb_helper->dev->mode_config.mutex); > > @@ -2236,23 +2235,15 @@ int drm_fb_helper_initial_config(struct > > drm_fb_helper *fb_helper, int bpp_sel) > > { > > struct drm_device *dev = fb_helper->dev; > > struct fb_info *info; > > - int count = 0; > > int ret; > > > > if (!drm_fbdev_emulation) > > return 0; > > > > mutex_lock(&dev->mode_config.mutex); > > - count = drm_fb_helper_probe_connector_modes(fb_helper, > > - > > dev->mode_config.max_width, > > - > > dev->mode_config.max_height); > > - /* > > - * we shouldn't end up with no modes here. > > - */ > > - if (count == 0) > > - dev_info(fb_helper->dev->dev, "No connectors reported > > connected with modes\n"); > > - > > - drm_setup_crtcs(fb_helper); > > + drm_setup_crtcs(fb_helper, > > + dev->mode_config.max_width, > > + dev->mode_config.max_height); > > ret = drm_fb_helper_single_fb_probe(fb_helper, bpp_sel); > > mutex_unlock(&dev->mode_config.mutex); > > if (ret) > > @@ -2300,28 +2291,22 @@ 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; > > - u32 max_width, max_height; > > > > if (!drm_fbdev_emulation) > > return 0; > > > > - mutex_lock(&fb_helper->dev->mode_config.mutex); > > + mutex_lock(&dev->mode_config.mutex); > > if (!fb_helper->fb || !drm_fb_helper_is_bound(fb_helper)) { > > fb_helper->delayed_hotplug = true; > > - mutex_unlock(&fb_helper->dev->mode_config.mutex); > > + mutex_unlock(&dev->mode_config.mutex); > > return 0; > > } > > DRM_DEBUG_KMS("\n"); > > > > - max_width = fb_helper->fb->width; > > - max_height = fb_helper->fb->height; > > + drm_setup_crtcs(fb_helper, fb_helper->fb->width, > > fb_helper->fb->height); > > > > - drm_fb_helper_probe_connector_modes(fb_helper, max_width, > > max_height); > > - mutex_unlock(&fb_helper->dev->mode_config.mutex); > > + mutex_unlock(&dev->mode_config.mutex); > > > > - drm_modeset_lock_all(dev); > > Stupid question: Why don't we need to do this any longer?
We don't do (never did) any modesets along this path, so locking everything is overkill and misleading, i.e. I was trimming it down to the locking as required by setup_crtc which is just that the config is stable. -Chris -- Chris Wilson, Intel Open Source Technology Centre