On Tue, Nov 29, 2016 at 10:34 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote: > 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.
Thanks for the explanation, looks good to me. Reviewed-by: Sean Paul <seanpaul at chromium.org> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre