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

[Maarten Lankhorst]

Op 13-09-17 om 12:37 schreef Ville Syrjälä:
> 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 at linux.intel.com>
>>>>>>> Cc: Lyude <lyude at redhat.com>
>>>>>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at 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.
>
It doesn't make sense to pass crtc_state, there's no guarantee that the crtc_state
from the same crtc is passed into first MST enable, and last MST disable.
To be sure we don't rely on it NULL was passed in, which is the only valid thing here.

~Maarten