[Intel-gfx] [PATCH] drm/i915/dp_mst: Fix enabling pipe clock for all streams

[Rodrigo Vivi]

On Tue, Sep 04, 2018 at 03:53:51PM -0700, Dhinakaran Pandiyan wrote:
> On Tuesday, September 4, 2018 5:54:16 AM PDT Imre Deak wrote:
> > On Tue, Sep 04, 2018 at 03:08:16PM +0300, Jani Nikula wrote:
> > > On Fri, 31 Aug 2018, Imre Deak <imre.deak at intel.com> wrote:
> > > > commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to
> > > > encoders")
> > > > inadvertently stopped enabling the pipe clock for any DP-MST stream
> > > > after the first one. It also rearranged the pipe clock enabling wrt.
> > > > initial MST payload allocation step (which may or may not be a
> > > > problem, but it's contrary to the spec.).
> > > > 
> > > > Fix things by making the above commit truly a non-functional change.
> > > 
> > > What kind of MST setups do we have in CI? Why didn't they catch this?
> > 
> > What we'd need is a dock/other branch device with an DP-MST input and
> > two outputs. That would exercise the case that broke here.
> > 
> I had the same question. We have  these two in CI, but both have only one 
> external display attached.
> fi-kbl-7560u	Dell XPS 13	Kaby Lake / i7-7560u / Iris Plus Graphics 640 GT3e	
> eDP, DELL TB16->TB->DP-MST
> fi-cfl-s3	Intel Coffee Lake-S RVP	Coffee Lake	eDP-PSR, DP-MST (HDMI)
> 
> Tomi,
> Is it possible to have another external display connected to one of these?

or to both of them?!

I'm seeing MST related regressions since 4.16 so I'd like to have few
different configuration if possible with more than one monitor.

at least one kind of dock and one kind of chain both with 2 monitors
plugged in would be good.

> 
> 
> > > BR,
> > > Jani.
> > > 
> > > > Fixes: commit afb2c4437dae ("drm/i915/ddi: Push pipe clock enabling to
> > > > encoders") Bugzilla:
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=107365
> > > > Reported-by: Lyude Paul <lyude at redhat.com>
> > > > Reported-by: dmummenschanz at web.de
> > > > Tested-by: dmummenschanz at web.de
> > > > Cc: Lyude Paul <lyude at redhat.com>
> > > > Cc: dmummenschanz at web.de
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > > 
> > > >  drivers/gpu/drm/i915/intel_ddi.c    | 17 +++++++++--------
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c |  4 ++++
> > > >  2 files changed, 13 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > > > b/drivers/gpu/drm/i915/intel_ddi.c index f3b115ce4029..dcb1a98d624d
> > > > 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2912,7 +2912,8 @@ static void intel_ddi_pre_enable_dp(struct
> > > > intel_encoder *encoder,> > 
> > > >  	icl_enable_phy_clock_gating(dig_port);
> > > > 
> > > > -	intel_ddi_enable_pipe_clock(crtc_state);
> > > > +	if (!is_mst)
> > > > +		intel_ddi_enable_pipe_clock(crtc_state);
> > > > 
> > > >  }
> > > >  
> > > >  static void intel_ddi_pre_enable_hdmi(struct intel_encoder *encoder,
> > > > 
> > > > @@ -3015,14 +3016,14 @@ static void intel_ddi_post_disable_dp(struct
> > > > intel_encoder *encoder,> > 
> > > >  	bool is_mst = intel_crtc_has_type(old_crtc_state,
> > > >  	
> > > >  					  INTEL_OUTPUT_DP_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.
> > > > -	 */
> > > > -	if (!is_mst)
> > > > +	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_disable_ddi_buf(encoder);
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > b/drivers/gpu/drm/i915/intel_dp_mst.c index 352e5216cc65..77920f1a3da1
> > > > 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > > @@ -166,6 +166,8 @@ static void intel_mst_post_disable_dp(struct
> > > > intel_encoder *encoder,> > 
> > > >  	struct intel_connector *connector =
> > > >  	
> > > >  		to_intel_connector(old_conn_state->connector);
> > > > 
> > > > +	intel_ddi_disable_pipe_clock(old_crtc_state);
> > > > +
> > > > 
> > > >  	/* this can fail */
> > > >  	drm_dp_check_act_status(&intel_dp->mst_mgr);
> > > >  	/* and this can also fail */
> > > > 
> > > > @@ -249,6 +251,8 @@ static void intel_mst_pre_enable_dp(struct
> > > > intel_encoder *encoder,> > 
> > > >  	I915_WRITE(DP_TP_STATUS(port), temp);
> > > >  	
> > > >  	ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr);
> > > > 
> > > > +
> > > > +	intel_ddi_enable_pipe_clock(pipe_config);
> > > > 
> > > >  }
> > > >  
> > > >  static void intel_mst_enable_dp(struct intel_encoder *encoder,
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
>