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

[Daniel Vetter]

On Wed, Mar 11, 2015 at 09:10:29PM +0200, Ville Syrjälä wrote:
> 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.

cpu edp on ilk+ and hsw+ dp are the ones that definitely get cranky if you
just kill the port. Not sure whether we've even seen hard hangs in dp
enabling on hsw, it's been a very long time ago. Well we've seen hard
hangs on hsw because of modeset bugs, but I don't remember whether it was
this on here too.

> > > 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.

Yeah on hsw+ and cpu edp we bring up the prot first iirc, then fire up the
pipe later on. Dunno what we do for external dp on ilk-ivb or what exactly
we should be doing - we iirc still have the occasionally dp port stuck bug
floating about.

In any case there's a very well defined sequence to go about things, and
anything else tends to anger the machines a lot. If we can't just use the
setCrtc ioctl from userspace we should at least reuse the link shutdown
machinery we have properly. Of course I'd prefer if we don't have to since
that would be less extra code to keep working.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch