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

[Daniel Vetter]

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:
> >> Update the hot plug function to handle the SST case. Instead of placing
> >> the SST case within the long/short pulse block, it is now handled after
> >> determining that MST mode is not in use. This way, the topology management
> >> layer can handle any MST-related operations while SST operations are still
> >> correctly handled afterwards.
> >>
> >> This patch also corrects the problem of SST mode only being handled in the
> >> case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes
> >> both short and long pulses are used by the different tests, thus both cases
> >> need to be addressed for SST.
> >>
> >> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
> >> previous compliance testing patch sequence. Review feedback on that patch
> >> indicated that updating intel_dp_hot_plug() was not the correct place for
> >> the test handler.
> >>
> >> For the SST case, the main stream is disabled for long HPD pulses as this
> >> generally indicates either a connect/disconnect event or link failure. For
> >> a number of case in compliance testing, the source is required to disable
> >> the main link upon detection of a long HPD.
> >>
> >> V2:
> >> - N/A
> >> V3:
> >> - Place the SST mode link status check into the mst_fail case
> >> - Remove obsolete comment regarding SST mode operation
> >> - Removed an erroneous line of code that snuck in during rebasing
> >> V4:
> >> - Added a disable of the main stream (DP transport) for the long pulse case
> >>   for SST to support compliance testing
> >>
> >> Signed-off-by: Todd PRevite <tprevite at gmail.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++----------
> >>  1 file changed, 15 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 080cc23..2460d14 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -4618,16 +4618,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> >>  				goto mst_fail;
> >>  		}
> >> -
> >> -		if (!intel_dp->is_mst) {
> >> -			/*
> >> -			 * we'll check the link status via the normal hot plug path later -
> >> -			 * but for short hpds we should check it now
> >> -			 */
> >> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> >> -			intel_dp_check_link_status(intel_dp);
> >> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> >> -		}
> >>  	}
> >>  
> >>  	ret = IRQ_HANDLED;
> >> @@ -4639,6 +4629,21 @@ mst_fail:
> >>  		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> >>  		intel_dp->is_mst = false;
> >>  		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
> >> +	} 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch