[PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup.

[PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup.

[Rodrigo Vivi]

As pointed out by Ville we were using inverted logic here.
According to spec:
For link standby mode set 170h[1] = 1.
For full link disabling set 170h[1] = 0.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 50381f7..4ecda72 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 	/* Enable PSR in sink */
 	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
 		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
-					    DP_PSR_ENABLE &
-					    ~DP_PSR_MAIN_LINK_ACTIVE);
-	else
-		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
 					    DP_PSR_ENABLE |
 					    DP_PSR_MAIN_LINK_ACTIVE);
+	else
+		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
+					    DP_PSR_ENABLE &
+					    ~DP_PSR_MAIN_LINK_ACTIVE);
 
 	/* Setup AUX registers */
 	I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
-- 
1.7.11.7

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite.

[Rodrigo Vivi]

On the current structure HSW doesn't support PSR with sprites enabled
but sprites can be enabled after PSR was enabled what would cause
user to miss screen updates.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..3132686 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 	I915_WRITE(SPRSURF(pipe),
 		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
 	POSTING_READ(SPRSURF(pipe));
+
+	intel_edp_psr_update(dev);
 }
 
 static void
-- 
1.7.11.7

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite.

[Daniel Vetter]

On Fri, Feb 07, 2014 at 04:09:48PM -0200, Rodrigo Vivi wrote:
> On the current structure HSW doesn't support PSR with sprites enabled
> but sprites can be enabled after PSR was enabled what would cause
> user to miss screen updates.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

Have you checked whether your current psr testcase hits this issue? If so
please add the Testcase: tag with the right subtest to this patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 336ae6c..3132686 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_WRITE(SPRSURF(pipe),
>  		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
> +
> +	intel_edp_psr_update(dev);
>  }
>  
>  static void
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup.

[Ville Syrjälä]

On Fri, Feb 07, 2014 at 04:09:47PM -0200, Rodrigo Vivi wrote:
> As pointed out by Ville we were using inverted logic here.
> According to spec:
> For link standby mode set 170h[1] = 1.
> For full link disabling set 170h[1] = 0.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 50381f7..4ecda72 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>  	/* Enable PSR in sink */
>  	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>  		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> -					    DP_PSR_ENABLE &
> -					    ~DP_PSR_MAIN_LINK_ACTIVE);
> -	else
> -		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>  					    DP_PSR_ENABLE |
>  					    DP_PSR_MAIN_LINK_ACTIVE);
> +	else
> +		intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
> +					    DP_PSR_ENABLE &
> +					    ~DP_PSR_MAIN_LINK_ACTIVE);

I think this is now the opposite of what we want. Ie. if the sink
doesn't require training we should disable the main link. Otherwise we
should keep the main link on, and that way avoid the need to train on
PSR exit.

Actually I'm not sure that's really what we want. I think the hardware
can do the training on its own, so in theory we should just always disable
the main link. Although the PM guide has a comment indicating that the
hardware training can fail, in which case software must repeat it. We
don't have code to do that, so I guess leaving the main link on is the
safer option. Would be nice to have a comment in the code stating as
much, if this is indeed the reason why the code was written this way.

>  
>  	/* Setup AUX registers */
>  	I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite.

[Ville Syrjälä]

On Fri, Feb 07, 2014 at 04:09:48PM -0200, Rodrigo Vivi wrote:
> On the current structure HSW doesn't support PSR with sprites enabled
> but sprites can be enabled after PSR was enabled what would cause
> user to miss screen updates.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 336ae6c..3132686 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	I915_WRITE(SPRSURF(pipe),
>  		   i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>  	POSTING_READ(SPRSURF(pipe));
> +
> +	intel_edp_psr_update(dev);

I was thinking this might be better placed in intel_update_plane()
since the fbc/ips stuff is there, but I can live with it being here too.

But should if have a HSW check on it? BDW doesn't have this restriction
anymore, right?

>  }
>  
>  static void
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/2] drm/i915: PSR HSW: update after enabling sprite.

[Rodrigo Vivi]

On Fri, Feb 7, 2014 at 5:17 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 07, 2014 at 04:09:48PM -0200, Rodrigo Vivi wrote:
>> On the current structure HSW doesn't support PSR with sprites enabled
>> but sprites can be enabled after PSR was enabled what would cause
>> user to miss screen updates.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 336ae6c..3132686 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -318,6 +318,8 @@ ivb_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>       I915_WRITE(SPRSURF(pipe),
>>                  i915_gem_obj_ggtt_offset(obj) + sprsurf_offset);
>>       POSTING_READ(SPRSURF(pipe));
>> +
>> +     intel_edp_psr_update(dev);
>
> I was thinking this might be better placed in intel_update_plane()
> since the fbc/ips stuff is there, but I can live with it being here too.

I agree there is better.

>
> But should if have a HSW check on it? BDW doesn't have this restriction
> anymore, right?

yes, it should... you are right!
>
>>  }
>>
>>  static void
>> --
>> 1.7.11.7
>
> --
> Ville Syrjälä
> Intel OTC

I also agree with Daniel we need a test case for that...
so, will provide this and a new patch version later

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 1/2] drm/i915: HSW PSR fix inverted sink DP_PSR_CFG link setup.

[Rodrigo Vivi]

On Fri, Feb 7, 2014 at 5:14 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 07, 2014 at 04:09:47PM -0200, Rodrigo Vivi wrote:
>> As pointed out by Ville we were using inverted logic here.
>> According to spec:
>> For link standby mode set 170h[1] = 1.
>> For full link disabling set 170h[1] = 0.
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 50381f7..4ecda72 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1661,12 +1661,12 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>>       /* Enable PSR in sink */
>>       if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
>>               intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>> -                                         DP_PSR_ENABLE &
>> -                                         ~DP_PSR_MAIN_LINK_ACTIVE);
>> -     else
>> -             intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>>                                           DP_PSR_ENABLE |
>>                                           DP_PSR_MAIN_LINK_ACTIVE);
>> +     else
>> +             intel_dp_aux_native_write_1(intel_dp, DP_PSR_EN_CFG,
>> +                                         DP_PSR_ENABLE &
>> +                                         ~DP_PSR_MAIN_LINK_ACTIVE);
>
> I think this is now the opposite of what we want. Ie. if the sink
> doesn't require training we should disable the main link. Otherwise we
> should keep the main link on, and that way avoid the need to train on
> PSR exit.

To be honest, I think I agree with you, but apparently performance
counter inc improved on this way...

>
> Actually I'm not sure that's really what we want. I think the hardware
> can do the training on its own, so in theory we should just always disable
> the main link. Although the PM guide has a comment indicating that the
> hardware training can fail, in which case software must repeat it. We
> don't have code to do that, so I guess leaving the main link on is the
> safer option. Would be nice to have a comment in the code stating as
> much, if this is indeed the reason why the code was written this way.

 I'll do a carefull check and local tests and send new version fixed
or with good comments.

>
>>
>>       /* Setup AUX registers */
>>       I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
>> --
>> 1.7.11.7
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br