On Tue, Nov 29, 2016 at 10:34 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
>> > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxxxx>
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
