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

[Ville Syrjälä]

On Wed, Sep 13, 2017 at 01:55:34PM +0200, Maarten Lankhorst wrote:
> 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 +, 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ä 
> > Cc: Lyude 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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?

Not sure we want a disabled connector to affect the entire thing. I was
thinking that maybe we want some kind of encoder state thing. Not really
sure.

At some point I did have the idea that maybe we could stick the main 

Re: [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 +, 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ä 
> Cc: Lyude 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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?

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 +, 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ä 
> >>> Cc: Lyude 
> >>> Signed-off-by: Dhinakaran Pandiyan 
> >>> ---
> >>>  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.

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

Re: [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 +, 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ä 
>>> Cc: Lyude 
>>> Signed-off-by: Dhinakaran Pandiyan 
>>> ---
>>>  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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[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 +, 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ä 
> > Cc: Lyude 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 12-09-17 om 22:11 schreef Pandiyan, Dhinakaran:
>
>
> On Tue, 2017-09-12 at 19:08 +, 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ä 
> Cc: Lyude 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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 */.
>
>
>   }
>  
>   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(_dig_port->base,
> NULL, NULL);
> -
> - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> - }
> + drm_dp_send_power_updown_phy(_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.

>> Interesting, that patch makes a lot of sense.
>>
 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 

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

[Pandiyan, Dhinakaran]

On Tue, 2017-09-12 at 19:08 +, 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ä 
> > > > Cc: Lyude 
> > > > Signed-off-by: Dhinakaran Pandiyan 
> > > > ---
> > > >  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.



> > > > }
> > > >  
> > > > 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(_dig_port->base,
> > > >   NULL, NULL);
> > > > -
> > > > -   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > > -   }
> > > > +   drm_dp_send_power_updown_phy(_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.
> > > 
> 
> Interesting, that patch makes a lot of sense.
> 
> > > 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 

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

[Pandiyan, Dhinakaran]

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ä 
> > > Cc: Lyude 
> > > Signed-off-by: Dhinakaran Pandiyan 
> > > ---
> > >  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(_dig_port->base,
> > > NULL, NULL);
> > > -
> > > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > > - }
> > > + drm_dp_send_power_updown_phy(_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.
> > 

Interesting, that patch makes a lot of sense.

> > 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.
> 

Sure.

> > 
> > >  }
> > >  
> > >  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(_dp->mst_mgr, connector->port, true);
> > >   if (intel_dp->active_mst_links == 0)
> > >   intel_dig_port->base.pre_enable(_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
> 
___
Intel-gfx mailing 

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

[Ville Syrjälä]

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ä 
> > Cc: Lyude 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  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(_dig_port->base,
> >   NULL, NULL);
> > -
> > -   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> > -   }
> > +   drm_dp_send_power_updown_phy(_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(_dp->mst_mgr, connector->port, true);
> > if (intel_dp->active_mst_links == 0)
> > intel_dig_port->base.pre_enable(_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

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

[Ville Syrjälä]

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ä 
> Cc: Lyude 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  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(_dig_port->base,
> NULL, NULL);
> -
> - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
> - }
> + drm_dp_send_power_updown_phy(_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.

>  }
>  
>  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(_dp->mst_mgr, connector->port, true);
>   if (intel_dp->active_mst_links == 0)
>   intel_dig_port->base.pre_enable(_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

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

[Stefan Assmann]

On 2017-09-05 18:26, 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ä 
> Cc: Lyude 
> Signed-off-by: Dhinakaran Pandiyan 

Tested-by: Stefan Assmann 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Dhinakaran Pandiyan]

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ä 
Cc: Lyude 
Signed-off-by: Dhinakaran Pandiyan 
---
 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);
}
 
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(_dig_port->base,
  NULL, NULL);
-
-   intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
-   }
+   drm_dp_send_power_updown_phy(_dp->mst_mgr, connector->port,
+false);
 }
 
 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(_dp->mst_mgr, connector->port, true);
if (intel_dp->active_mst_links == 0)
intel_dig_port->base.pre_enable(_dig_port->base,
pipe_config, NULL);
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx