Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Ander Conselvan De Oliveira <conselvan2@gmail.com>
Cc: intel-gfx@lists.freedesktop.org,
	Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
Subject: Re: [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse
Date: Fri, 18 Nov 2016 09:51:10 +0000	[thread overview]
Message-ID: <20161118095110.GE16779@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1479462097.2718.9.camel@gmail.com>

On Fri, Nov 18, 2016 at 11:41:37AM +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-11-17 at 22:01 +0000, Chris Wilson wrote:
> > On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote:
> > > 
> > > On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote:
> > > > 
> > > > 
> > > > On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> > > > > 
> > > > > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> > > > > > 
> > > > > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira
> > > > > > wrote:
> > > > > > > 
> > > > > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > > > > > > 
> > > > > > > > Current DP detection has DPCD operations split across
> > > > > > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > > > > > duplicates as well. Also intel_dp_detect is called
> > > > > > > > during modes enumeration as well which will result
> > > > > > > > in multiple dpcd operations. So this patch tries
> > > > > > > > to solve both these by bringing all DPCD operations
> > > > > > > > in one single function and make intel_dp_detect
> > > > > > > > use existing values instead of repeating same steps.
> > > > > > > > 
> > > > > > > > v2: Pulled in a hunk from last patch of the series to
> > > > > > > >       this patch. (Ander)
> > > > > > > > v3: Added MST hotplug handling. (Ander)
> > > > > > > > 
> > > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.
> > > > > > > > com>
> > > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.
> > > > > > > > com>
> > > > > > > > ---
> > > > > > > >    drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++-
> > > > > > > > ----
> > > > > > > > -----
> > > > > > > > -----
> > > > > > > > -
> > > > > > > >    1 file changed, 44 insertions(+), 27 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > index 8969ff9..82ee18d 100644
> > > > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > > [...]
> > > > > > > 
> > > > > > > > 
> > > > > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector
> > > > > > > > *connector,
> > > > > > > > bool
> > > > > > > > force)
> > > > > > > >    		return connector_status_disconnected;
> > > > > > > >    	}
> > > > > > > >    
> > > > > > > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > > > > +	if (force)
> > > > > > > > +		intel_dp_long_pulse(intel_dp-
> > > > > > > > >attached_connector);
> > > > > > > I didn't notice this at first, but force is not the right thing to
> > > > > > > check
> > > > > > > for
> > > > > > > here. It is basically intended to avoid doing load detection (see
> > > > > > > intel_get_load_detect_pipe()) on automated polling. But we still
> > > > > > > have to
> > > > > > > try
> > > > > > > detection here when force = false, otherwise this will cause a
> > > > > > > regression.
> > > > > > > 
> > > > > > > If you plug in a DP device while suspended, that device won't get
> > > > > > > detected,
> > > > > > > since we don't get an HPD for it. Previously, the call do
> > > > > > > intel_dp_detect()
> > > > > > > with
> > > > > > > force = false from intel_drm_resume() (via
> > > > > > > drm_helper_hpd_irq_event())
> > > > > > > would
> > > > > > > cause a full detection.
> > > > > > > 
> > > > > > > To avoid the repeated DPCD operations, I think we need a more
> > > > > > > explicit
> > > > > > > mechanism
> > > > > > > to signal that we already handled the long pulse via the HPD
> > > > > > > handler. In
> > > > > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled
> > > > > > > a
> > > > > > > long
> > > > > > > pulse
> > > > > > > for the given port. The call to intel_dp_long_pulse() in
> > > > > > > intel_dp_detect()
> > > > > > > would
> > > > > > > then be dependent on that flag.
> > > > > > > 
> > > > > > > For that reason I have to retract my R-b from this patch.
> > > > > > > 
> > > > > > > Ander
> > > > > > Call to intel_dp_detect() from get_modes is with force set to true
> > > > > > while
> > > > > > from resume the call is with force set to false.. It should be in the
> > > > > > opposite manner as get_modes should not require full detection whereas
> > > > > > resume should. So, this needs to be cleaned up there. After merge of
> > > > > > these patches, will look into cleaning up that part of the code.
> > > > > That really depends on what the force parameter is supposed to mean. The
> > > > > documentation states that "force is set to false whilst polling, true
> > > > > when
> > > > > checking the connector due to a user request". A look through git
> > > > > history
> > > > > shows
> > > > > the parameter was added to reduce time wasted doing load detection
> > > > > (doing a
> > > > > mode
> > > > > set in order to check if there is a device connected) for hardware that
> > > > > needs it
> > > > > (commit 7b334fcb45b7).
> > > > > 
> > > > > As far as I can tell, across all the drm drivers, that parameter is only
> > > > > used to
> > > > > avoid doing load detection.
> > > > > 
> > > > > Another thing to consider is that the driver may switch to polling if it
> > > > > detects
> > > > > an HPD storm. When the detect calls come from polling, the code in this
> > > > > patch
> > > > > will simply avoid any detection.
> > > > > 
> > > > hmm i think this discussion will prolong for a while :)
> > > > how about we call intel_dp_long_pulse() always for now.
> > > > this will be a compromise to not break any of the existing code
> > > > but will still result in getting a clean detection code, which
> > > > will can then be improved upon in the next iteration ?
> > > > i.e post the change it should look like. i.e skip this change alone
> > > > 
> > > > 	intel_dp_long_pulse(intel_dp->attached_connector);
> > > Sounds good. The code gets cleaner and there is no regression in terms of
> > > repeated DPCD operations.
> > So this conversation seems to have had little bearing on reality,
> > especially in terms of how intel_dp_detect() is supposed to behave. This
> > patch is causing WARNs as it presumed that intel_dp_detect() is only
> > called after a modeset.
> > 
> > Should we send a revert to stable?
> 
> Ville's 27d4efc5591a ("drm/i915: Move long hpd handling into the hotplug work")
> and 16c83fad79ca ("drm/i915: Allow DP to work w/o EDID") were sent to stable and
> are in 4.8.6. Are the WARNs happening with those patches too?

Yes. The WARNs are still happening in -nightly, they only take calling
GETCONNECTOR twice on a connected monitor before setting a mode.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx