Re: [Intel-gfx] [PATCH] drm/i915: Check DP link status on long hpd too

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 01, 2015 at 11:36:25PM +0530, Sivakumar Thulasimani wrote:
> 
> 
> On 8/20/2015 10:07 PM, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We are no longer checkling the DP link status on long hpd. We used to do
> > that from the .hot_plug() handler, but it was removed when MST got
> > introduced.
> >
> > If there's no userspace we now fail to retrain the link if the sink
> > power is toggled (or cable yanked and replugged), meaning the user is
> > left staring at a blank screen. With the retraining put back that should
> > be fixed.
> >
> > Also remove the leftover comment that referred to the old retraining
> > from .hot_plug().
> >
> > Fixes a regression introduced in:
> > commit 0e32b39ceed665bfa4a77a4bc307b6652b991632
> > Author: Dave Airlie <airlied@redhat.com>
> > Date:   Fri May 2 14:02:48 2014 +1000
> >
> >      drm/i915: add DP 1.2 MST support (v0.7)
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89453
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91407
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89461
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89594
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85641
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 11 +++++------
> >   1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index d32ce48..b014158 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5024,9 +5024,12 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >   
> >   		intel_dp_probe_oui(intel_dp);
> >   
> > -		if (!intel_dp_probe_mst(intel_dp))
> > +		if (!intel_dp_probe_mst(intel_dp)) {
> > +			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> > +			intel_dp_check_link_status(intel_dp);
> > +			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> couple of queries for my understanding.
>  > why should we check for link status for long pulse, which is supposed 
> to be for plug in or plug out ?
>  > goto mst_fail will indicate we are falling back to intel_dp_detect, 
> isnt this better suited there ?

->detect() should do what it says and no more.

In any case I think it's better to have the code to maintain the current
link in one place. That should also make the locking rules easier to
understand since we hpd_pulse and detect are executed from different
works.

> 
> Also the following bug indicates failure in mst panel, but the changes 
> here are in non-mst path
>       https://bugs.freedesktop.org/show_bug.cgi?id=89453, how is this 
> patch helping this bug ?

Presumably the monitor is being used in non-MST mode.

> >   			goto mst_fail;
> > -
> > +		}
> >   	} else {
> >   		if (intel_dp->is_mst) {
> >   			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> > @@ -5034,10 +5037,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >   		}
> >   
> >   		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);
> 
> -- 
> regards,
> Sivakumar

-- 
Ville Syrjälä
Intel OTC