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

[Maarten Lankhorst]

Op 13-09-17 om 13:48 schreef Ville Syrjälä:
> On Wed, Sep 13, 2017 at 12:46:47PM +0200, Maarten Lankhorst wrote:
>> 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.
> Doesn't really matter. If they don't match sufficiently we've already
> messed up somewhere.
>
> Ideally we should have some kind of separate state for the DP link, but
> since we don't atm we just abuse the crtc state to contain the link
> parameters.

Could we stuff it in the real DP's connector state, and then make sure we add the master connector state?