[Intel-gfx] [PATCH] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Ville Syrjälä]

On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote:
> On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
> > On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
> >> On 03/09/2015 10:29 AM, Daniel Vetter wrote:
> >>> On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
> >>>> On 03/06/2015 08:34 AM, Daniel Vetter wrote:
> >>>>> On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
> >>>>>> +	} else {
> >>>>>> +		/* SST mode - handle short/long pulses here */
> >>>>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >>>>>> +		/* Clear compliance testing flags/data here to prevent
> >>>>>> +		 * false detection in userspace */
> >>>>>> +		intel_dp->compliance_test_data = 0;
> >>>>>> +		intel_dp->compliance_testing_active = 0;
> >>>>>> +		/* For a long pulse in SST mode, disable the main link */
> >>>>>> +		if (long_hpd) {
> >>>>>> +			I915_WRITE(DP_TP_CTL(intel_dig_port->port),
> >>>>>> +					      ~DP_TP_CTL_ENABLE);
> >>>>>> +		}
> >>>>>
> >>>>> Disabling the  main link should be done in userspace. All long pulse
> >>>>> requests should be forwarded to userspace as a hotplug event. Userspace
> >>>>> can then react to that hotplug appropriately. This way we can again
> >>>>> exercise the normal operation of all our dp code.
> >>>>
> >>>> What's your concern here?  Do you want to make sure we get coverage on
> >>>> dp_link_down()?  It looks like that might be safe to use here instead of
> >>>> flipping the disable bit directly.  Or did you want to go through the
> >>>> whole pipe/port shutdown sequence as well?  If so, I think the dpms
> >>>> tests will already cover that, separate from simple compliance.
> >>>
> >>> This is likely to upset the state checker, we've already had some fun with
> >>> killing the hard dp pipe disable that the hdp code occasionally did. Well,
> >>> still have. The other reason is that dp compliance testing with
> >>> special-case code is somewhat pointless, except when the compliance test
> >>> contracts what real-world experience forces us to do. For these exceptions
> >>> I'd like that we fully understand them and also document them. Disabling
> >>> the link on a full hot-unplug is something we can (and most DE actually
> >>> do) do.
> >>
> >> If we end up hitting the checker while testing, then yeah it would spew.
> >>
> >> But I thought this was mainly about testing the DP code, making sure we
> >> can up/down links, train at different parameters, etc, not about going
> >> through full mode sets all the time...
> >>
> >> But either way, I agree we should be documenting this behavior so we
> >> don't get stuck trying to figure it out later.
> > 
> > I don't think we should be killing the port like this. It'll also kill
> > the pipe on some platforms and then we get all kinds of pipe stuck
> > warnings. So I think we'd definitely want a more graceful shutdown of
> > things.
> 
> Does that affect current platforms?  Or just CTG and ILK?  I can guess
> BYT & BSW might be affected, but I haven't tested.  As long as we just
> up/down the port w/o anything else it might be able to work...

I think you have that reversed. Old platforms were generally fine with
enabling/disabling ports while the pipe kept running, but I think that
changed at some point (with ilk I suppose), and killing the port is then
a bad idea.

That's at least the case with DP. I seem to recall we had at least stuck
pipes (and maybe even some lockups or something?) when we had the
dp_link_down() call in the hpd handler.

> 
> > I thought we actually discussed about going to the other direction, ie.
> > potentially allowing the link to brought up without the pipe and
> > enabling/disabling the pipe independently while the link remains up and
> > running?
> 
> I guess I was thinking the reverse: that bringing up the port w/o a pipe
> driving it would be more likely to cause problems, but I guess we'll
> need testing.

Bringing up the port on its own is perfectly fine. We do that for link
training already, and if you consider MST you'll notice it's the only
way really.

-- 
Ville Syrjälä
Intel OTC