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

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

On Wed, Sep 13, 2017 at 09:32:40AM +0200, Maarten Lankhorst wrote:
> Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
> >
> >
> > On Tue, 2017-09-12 at 19:08 +0000, Pandiyan, Dhinakaran wrote:
> >> On Tue, 2017-09-12 at 19:17 +0300, Ville Syrjälä wrote:
> >>> 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)
> >>>>
> > old_crtc_state, which intel_crtc_has_type() needs, is NULL for MST and
> > the reason Maarten provided in his commit is
> >
> > " We also shouldn't pass crtc_state, because in that case the
> >   disabling sequence may potentially be different depending on
> >   which crtc is disabled last. Nice way to introduce bugs.
> > "
> > I am not 100% sure I understand the concern. But since that change was
> > intentional I guess we can use the NULL values, like I've done, to
> > identify the mst sinks. I am open to ideas.
> I think checking for NULL crtc_state is enough, in case we ever decide to pass the real connector state.
> For clarity, I would add something like bool is_dsp_mst = !crtc_state; /* When enabling the link for MST, this connector is never bound to a crtc */.

The NULL state is rather ugly IMO. With a comment I might be able to
stomach it. However, after another look I see that we already pass the
crtc state to ddi_pre_enable() from the MST code, so even just from a
symmetry POV it would make sense to pass it to ddi_post_disable as well.

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