Re: [PATCH 2/2] drm/i915/mst: Use MST sideband message transaction for dpms

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

On Tue, Sep 12, 2017 at 07:11:32PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 05, 2017 at 06:26:34PM -0700, Dhinakaran Pandiyan wrote:
> > Use the POWER_DOWN_PHY and POWER_UP_PHY sideband message trasactions to
> > set power states for downstream sinks. Apart from giving us the ability
> > to set power state for individual sinks, this fixes the below test for
> > me
> > 
> > $ xrandr --display :0 --output DP-2-2-8 --off
> > $ xrandr --display :0 --output DP-2-2-1 --off
> > $ xrandr --display :0 --output DP-2-2-8 --auto #Black screen
> > $ xrandr --display :0 --output DP-2-2-1 --auto
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lyude <lyude@redhat.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c    | 6 ++++--
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 8 ++++----
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 1da3bb2cc4b4..8aebacc0aa31 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2161,7 +2161,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  		intel_prepare_dp_ddi_buffers(encoder);
> >  
> >  	intel_ddi_init_dp_buf_reg(encoder);
> > -	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> > +	if (!link_mst)
> > +		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> >  	intel_dp_start_link_train(intel_dp);
> >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >  		intel_dp_stop_link_train(intel_dp);
> > @@ -2240,7 +2241,8 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder,
> >  	if (type == INTEL_OUTPUT_DP || type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >  
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > +		if (old_crtc_state && old_conn_state)
> > +			intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> 
> I would make that
> !intel_crtc_has_type(DP_MST)
> 
> >  	}
> >  
> >  	val = I915_READ(DDI_BUF_CTL(port));
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 8e3aad0ea60b..81e63724e24b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -167,12 +167,11 @@ static void intel_mst_post_disable_dp(struct intel_encoder *encoder,
> >  	intel_dp->active_mst_links--;
> >  
> >  	intel_mst->connector = NULL;
> > -	if (intel_dp->active_mst_links == 0) {
> > +	if (intel_dp->active_mst_links == 0)
> >  		intel_dig_port->base.post_disable(&intel_dig_port->base,
> >  						  NULL, NULL);
> > -
> > -		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > -	}
> > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port,
> > +				     false);
> 
> One thing that bothers me here is whether we'll start getting bogus link
> training requests during shutdown since we'll do the D3 only after the
> stream has been stopped. If you'll look at commit 7618138d8b40
> ("drm/i915/ddi: Avoid long delays during system suspend / eDP
> disabling") Imre explicitly moved the D3 earlier for SST to avoid this.
> 
> Hmm. Actually by the time we call mst_post_disable() we should have
> deallocated the VC etc. so the current stream should be already
> terminated at that point. So I think we should be able to do the
> PHY_POWER_DOWN before we shut down the link, just like we do for SST.

Might also be nice to add comments near all the D3 calls to document
that the order we use is there to prevent the sink from complaining
about link loss when we shut down the link. Otherwise someone might
get tempted to reorder things as a cleanup or something.

> 
> >  }
> >  
> >  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> > @@ -197,6 +196,7 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder,
> >  
> >  	DRM_DEBUG_KMS("%d\n", intel_dp->active_mst_links);
> >  
> > +	drm_dp_send_power_updown_phy(&intel_dp->mst_mgr, connector->port, true);
> >  	if (intel_dp->active_mst_links == 0)
> >  		intel_dig_port->base.pre_enable(&intel_dig_port->base,
> >  						pipe_config, NULL);
> > -- 
> > 2.11.0
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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