| Message ID | 20191123005459.155383-4-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] drm/i915/display: Refactor intel_commit_modeset_disables() | expand |
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+. > > 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
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. 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. > > > > > 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
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. > > > > > > > > > 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
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 > > > > > > > > > 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
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
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); } intel_disable_ddi_buf(encoder, old_crtc_state);
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(-)