Message-ID: <1478189469.28703.8.camel@redhat.com>
Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Remove redundant reprobe in
 i915_drm_resume
From: Lyude Paul <cpaul@redhat.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, Lyude <lyude@redhat.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
        intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        dri-devel@lists.freedesktop.org
Date: Thu, 03 Nov 2016 12:11:09 -0400
In-Reply-To: <20161103160253.GA24715@nuc-i3427.alporthouse.com>
References: <1478187758-32740-1-git-send-email-lyude@redhat.com>
         <1478187758-32740-2-git-send-email-lyude@redhat.com>
         <20161103160253.GA24715@nuc-i3427.alporthouse.com>
Organization: Red Hat
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2772
Lines: 67

On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote:
> On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote:
> > 
> > Weine's investigation on benchmarking the suspend/resume process pointed
> > out a lot of the time in suspend/resume is being spent reprobing. While
> > the reprobing process is a lengthy one for good reason, we don't need to
> > hold up the entire suspend/resume process while we wait for it to
> > finish. Luckily as it turns out, we already trigger a full connector
> > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in
> > i915_drm_resume() entirely.
> > 
> > This won't lead to less time spent resuming just yet since now the
> > bottleneck will be waiting for the mode_config lock in
> > drm_kms_helper_poll_enable(), since that will be held as long as
> > i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll
> > address that in the next patch.
> > 
> > Signed-off-by: Lyude <lyude@redhat.com>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 2 --
> >  1 file changed, 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index bfb2efd..532cc0f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >  	 * notifications.
> >  	 * */
> >  	intel_hpd_init(dev_priv);
> > -	/* Config may have changed between suspend and resume */
> > -	drm_helper_hpd_irq_event(dev);
> 
> The comment is still apt. This code is known to be broken since it
> doesn't detect a change in monitors (e.g. a change in external connectors
> from docking) between suspend and resend. We still have to send the uevent.
> 
> +	drm_kms_helper_hotplug_event(dev);

I might not have explained myself very well. The way things should look with
this patch is like this:

i915_drm_resume()
 -> intel_hpd_init()
   -> sets dev_priv->hotplug.poll_enabled to true
   -> schedules dev_priv->hotplug.poll_init_work
 -> continue resume…

at the same time:

i915_hpd_poll_init_work() gets scheduled and starts
 -> since dev_priv->hotplug.poll_enabled == false, drm_helper_hpd_irq_event() is called
  -> drm_helper_hpd_irq_event() reprobes connectors
   -> if anything changed, drm_kms_helper_hotplug_event() gets called.

So we're still polling the connectors when coming out of resume just like
before, except now we're doing it without needlessly making the whole resume
process stall until we're done. We're also no longer reprobing display
connectors twice…

> 
> which also depends upon us actually reseting the connector->status to
> unknown in drm_mode_config_reset(), Daniel!
> -Chris
>