Re: [PATCH 4/7] drm/i915/dp: Power down sink before disable pipe/transcoder clock

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Souza, Jose" <jose.souza@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"De Marchi, Lucas" <lucas.demarchi@intel.com>
Subject: Re: [PATCH 4/7] drm/i915/dp: Power down sink before disable pipe/transcoder clock
Date: Thu, 28 Nov 2019 20:30:41 +0200	[thread overview]
Message-ID: <20191128183041.GV1208@intel.com> (raw)
In-Reply-To: <a4857777dddd1ba864f6bbd0718561e78c5a542d.camel@intel.com>

On Thu, Nov 28, 2019 at 01:08:36AM +0000, Souza, Jose wrote:
> On Wed, 2019-11-27 at 21:24 +0200, Ville Syrjälä wrote:
> > On Tue, Nov 26, 2019 at 10:12:52PM +0000, Souza, Jose wrote:
> > > On Tue, 2019-11-26 at 22:15 +0200, Ville Syrjälä wrote:
> > > > On Fri, Nov 22, 2019 at 04:54:56PM -0800, José Roberto de Souza
> > > > wrote:
> > > > > Disabling pipe/transcoder clock before power down sink could
> > > > > cause
> > > > > sink lost signal, causing it to trigger a hotplug to notify
> > > > > source
> > > > > that link signal was lost.
> > > > > 
> > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_ddi.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > index d2f0d393d3ee..7d3a6e3c7f57 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > @@ -3808,12 +3808,12 @@ static void
> > > > > intel_ddi_post_disable_dp(struct intel_encoder *encoder,
> > > > >  	enum phy phy = intel_port_to_phy(dev_priv, encoder-
> > > > > >port);
> > > > >  
> > > > >  	if (!is_mst) {
> > > > > -		intel_ddi_disable_pipe_clock(old_crtc_state);
> > > > >  		/*
> > > > >  		 * Power down sink before disabling the port,
> > > > > otherwise
> > > > > we end
> > > > >  		 * up getting interrupts from the sink on
> > > > > detecting
> > > > > link loss.
> > > > >  		 */
> > > > >  		intel_dp_sink_dpms(intel_dp,
> > > > > DRM_MODE_DPMS_OFF);
> > > > > +		intel_ddi_disable_pipe_clock(old_crtc_state);
> > > > >  	}
> > > > 
> > > > The spec seems to say that we should do this after turning off
> > > > DDI_BUF_CTL on tgl+.
> > > 
> > > What step? I can't find any step talking about AUX DP_SET_POWER.
> > 
> > I was talking about DDI_BUF disable vs. transcoder clock disable.
> > 
> > > My understating is that we should power off sink before interfering
> > > in
> > > the mainlink signal otherwise sink could trigger hotplugs to notify
> > > source about link loss.
> > 
> > Pretty much. Nothing wrong with your patch for pre-tgl I think, but
> > for
> > tgl+ you didn't move the clock disable quite far enough to match the
> > bspec sequence.
> 
> Aaahh
> That is fixed patch 6 "drm/i915/display/tgl: Fix the order of the step
> to turn transcoder clock off" :D

Ah, should have kept on reading.

In that case this one is 
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> > 
> > > > >  
> > > > >  	intel_disable_ddi_buf(encoder, old_crtc_state);
> > > > > -- 
> > > > > 2.24.0
> > > > > 
> > > > > _______________________________________________
> > > > > Intel-gfx mailing list
> > > > > Intel-gfx@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx