All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"
@ 2016-09-08  0:42 Rodrigo Vivi
  2016-09-08  1:53 ` ✗ Fi.CI.BAT: warning for " Patchwork
  2016-09-08  8:01 ` [PATCH] " Ville Syrjälä
  0 siblings, 2 replies; 5+ messages in thread
From: Rodrigo Vivi @ 2016-09-08  0:42 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula, Daniel Vetter, Dominik Brodowski, Rodrigo Vivi

This reverts commit 1c80c25fb622973dd135878e98d172be20859049.

There are panels that needs 4 idle frames before entering PSR,
but VBT is unproperly set.

Also lately it was identified that idle frame count calculated at HW
can be off by 1, what makes the minimum of 2, at least.

Without the current vbt+1 we are with the risk of having HW calculating
0 idle frames and entering PSR when it shouldn't. Regardless the lack
of link training.

Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 59a21c9..108ba1e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	uint32_t max_sleep_time = 0x1f;
-	/* Lately it was identified that depending on panel idle frame count
-	 * calculated at HW can be off by 1. So let's use what came
-	 * from VBT + 1.
-	 * There are also other cases where panel demands at least 4
-	 * but VBT is not being set. To cover these 2 cases lets use
-	 * at least 5 when VBT isn't set to be on the safest side.
+	/*
+	 * Let's respect VBT in case VBT asks a higher idle_frame value.
+	 * Let's use 6 as the minimum to cover all known cases including
+	 * the off-by-one issue that HW has in some cases. Also there are
+	 * cases where sink should be able to train
+	 * with the 5 or 6 idle patterns.
 	 */
-	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
+	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
 	uint32_t val = EDP_PSR_ENABLE;
 
 	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* ✗ Fi.CI.BAT: warning for Revert "drm/i915/psr: Make idle_frames sensible again"
  2016-09-08  0:42 [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" Rodrigo Vivi
@ 2016-09-08  1:53 ` Patchwork
  2016-09-08  8:01 ` [PATCH] " Ville Syrjälä
  1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2016-09-08  1:53 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: intel-gfx

== Series Details ==

Series: Revert "drm/i915/psr: Make idle_frames sensible again"
URL   : https://patchwork.freedesktop.org/series/12148/
State : warning

== Summary ==

Series 12148v1 Revert "drm/i915/psr: Make idle_frames sensible again"
http://patchwork.freedesktop.org/api/1.0/series/12148/revisions/1/mbox/

Test gem_exec_gttfill:
        Subgroup basic:
                pass       -> SKIP       (fi-snb-2600)

fi-bdw-5557u     total:252  pass:233  dwarn:2   dfail:1   fail:1   skip:15 
fi-bsw-n3050     total:252  pass:203  dwarn:1   dfail:1   fail:1   skip:46 
fi-hsw-4770k     total:252  pass:226  dwarn:2   dfail:1   fail:1   skip:22 
fi-hsw-4770r     total:252  pass:222  dwarn:2   dfail:1   fail:1   skip:26 
fi-ilk-650       total:252  pass:179  dwarn:2   dfail:1   fail:3   skip:67 
fi-ivb-3520m     total:252  pass:217  dwarn:2   dfail:1   fail:1   skip:31 
fi-ivb-3770      total:252  pass:217  dwarn:2   dfail:1   fail:1   skip:31 
fi-skl-6260u     total:252  pass:234  dwarn:2   dfail:1   fail:1   skip:14 
fi-skl-6700k     total:252  pass:219  dwarn:3   dfail:1   fail:1   skip:28 
fi-snb-2520m     total:252  pass:204  dwarn:1   dfail:1   fail:1   skip:45 
fi-snb-2600      total:252  pass:202  dwarn:2   dfail:1   fail:1   skip:46 
fi-byt-n2820 failed to collect. IGT log at Patchwork_2489/fi-byt-n2820/igt.log

Results at /archive/results/CI_IGT_test/Patchwork_2489/

a6f2d1b98141ef44ea5d6c480c7fb2ce821189d2 drm-intel-nightly: 2016y-09m-07d-23h-08m-05s UTC integration manifest
c04448b Revert "drm/i915/psr: Make idle_frames sensible again"

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"
  2016-09-08  0:42 [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" Rodrigo Vivi
  2016-09-08  1:53 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-09-08  8:01 ` Ville Syrjälä
  2016-09-08  8:53   ` Jani Nikula
  1 sibling, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2016-09-08  8:01 UTC (permalink / raw)
  To: Rodrigo Vivi; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, Dominik Brodowski

On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote:
> This reverts commit 1c80c25fb622973dd135878e98d172be20859049.
> 
> There are panels that needs 4 idle frames before entering PSR,
> but VBT is unproperly set.
> 
> Also lately it was identified that idle frame count calculated at HW
> can be off by 1, what makes the minimum of 2, at least.
> 
> Without the current vbt+1 we are with the risk of having HW calculating
> 0 idle frames and entering PSR when it shouldn't. Regardless the lack
> of link training.
> 
> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 59a21c9..108ba1e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	uint32_t max_sleep_time = 0x1f;
> -	/* Lately it was identified that depending on panel idle frame count
> -	 * calculated at HW can be off by 1. So let's use what came
> -	 * from VBT + 1.
> -	 * There are also other cases where panel demands at least 4
> -	 * but VBT is not being set. To cover these 2 cases lets use
> -	 * at least 5 when VBT isn't set to be on the safest side.
> +	/*
> +	 * Let's respect VBT in case VBT asks a higher idle_frame value.
> +	 * Let's use 6 as the minimum to cover all known cases including
> +	 * the off-by-one issue that HW has in some cases. Also there are
> +	 * cases where sink should be able to train
> +	 * with the 5 or 6 idle patterns.
>  	 */
> -	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
> +	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);

I don't really understand this logic compared to the explanations.

Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'?

>  	uint32_t val = EDP_PSR_ENABLE;
>  
>  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> -- 
> 1.9.1
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"
  2016-09-08  8:01 ` [PATCH] " Ville Syrjälä
@ 2016-09-08  8:53   ` Jani Nikula
  2016-09-13  9:05     ` Jani Nikula
  0 siblings, 1 reply; 5+ messages in thread
From: Jani Nikula @ 2016-09-08  8:53 UTC (permalink / raw)
  To: Ville Syrjälä, Rodrigo Vivi
  Cc: Daniel Vetter, intel-gfx, Dominik Brodowski

On Thu, 08 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote:
>> This reverts commit 1c80c25fb622973dd135878e98d172be20859049.
>> 
>> There are panels that needs 4 idle frames before entering PSR,
>> but VBT is unproperly set.
>> 
>> Also lately it was identified that idle frame count calculated at HW
>> can be off by 1, what makes the minimum of 2, at least.
>> 
>> Without the current vbt+1 we are with the risk of having HW calculating
>> 0 idle frames and entering PSR when it shouldn't. Regardless the lack
>> of link training.
>> 
>> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_psr.c | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index 59a21c9..108ba1e 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>  
>>  	uint32_t max_sleep_time = 0x1f;
>> -	/* Lately it was identified that depending on panel idle frame count
>> -	 * calculated at HW can be off by 1. So let's use what came
>> -	 * from VBT + 1.
>> -	 * There are also other cases where panel demands at least 4
>> -	 * but VBT is not being set. To cover these 2 cases lets use
>> -	 * at least 5 when VBT isn't set to be on the safest side.
>> +	/*
>> +	 * Let's respect VBT in case VBT asks a higher idle_frame value.
>> +	 * Let's use 6 as the minimum to cover all known cases including
>> +	 * the off-by-one issue that HW has in some cases. Also there are
>> +	 * cases where sink should be able to train
>> +	 * with the 5 or 6 idle patterns.
>>  	 */
>> -	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
>> +	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>
> I don't really understand this logic compared to the explanations.
>
> Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'?

We're at rc5, smells like revert, not trial and error.

Side note, looking at the VBT spec on tp1, tp2/tp3 wakeup times, there
seems to be some confusion about what the values mean. Has there perhaps
been a change in the spec? What's the VBT version where the problems
happen?

BR,
Jani.

>
>>  	uint32_t val = EDP_PSR_ENABLE;
>>  
>>  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
>> -- 
>> 1.9.1
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again"
  2016-09-08  8:53   ` Jani Nikula
@ 2016-09-13  9:05     ` Jani Nikula
  0 siblings, 0 replies; 5+ messages in thread
From: Jani Nikula @ 2016-09-13  9:05 UTC (permalink / raw)
  To: Ville Syrjälä, Rodrigo Vivi
  Cc: Daniel Vetter, intel-gfx, Dominik Brodowski

On Thu, 08 Sep 2016, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 08 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> On Wed, Sep 07, 2016 at 05:42:31PM -0700, Rodrigo Vivi wrote:
>>> This reverts commit 1c80c25fb622973dd135878e98d172be20859049.
>>> 
>>> There are panels that needs 4 idle frames before entering PSR,
>>> but VBT is unproperly set.
>>> 
>>> Also lately it was identified that idle frame count calculated at HW
>>> can be off by 1, what makes the minimum of 2, at least.
>>> 
>>> Without the current vbt+1 we are with the risk of having HW calculating
>>> 0 idle frames and entering PSR when it shouldn't. Regardless the lack
>>> of link training.
>>> 
>>> Cc: Dominik Brodowski <linux@dominikbrodowski.net>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_psr.c | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>>> index 59a21c9..108ba1e 100644
>>> --- a/drivers/gpu/drm/i915/intel_psr.c
>>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>>> @@ -255,14 +255,14 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
>>>  	struct drm_i915_private *dev_priv = to_i915(dev);
>>>  
>>>  	uint32_t max_sleep_time = 0x1f;
>>> -	/* Lately it was identified that depending on panel idle frame count
>>> -	 * calculated at HW can be off by 1. So let's use what came
>>> -	 * from VBT + 1.
>>> -	 * There are also other cases where panel demands at least 4
>>> -	 * but VBT is not being set. To cover these 2 cases lets use
>>> -	 * at least 5 when VBT isn't set to be on the safest side.
>>> +	/*
>>> +	 * Let's respect VBT in case VBT asks a higher idle_frame value.
>>> +	 * Let's use 6 as the minimum to cover all known cases including
>>> +	 * the off-by-one issue that HW has in some cases. Also there are
>>> +	 * cases where sink should be able to train
>>> +	 * with the 5 or 6 idle patterns.
>>>  	 */
>>> -	uint32_t idle_frames = dev_priv->vbt.psr.idle_frames + 1;
>>> +	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>>
>> I don't really understand this logic compared to the explanations.
>>
>> Shouldn't we do something like 'idle_frames = max(4, psr.idle_frames) + 1;'?
>
> We're at rc5, smells like revert, not trial and error.

Pushed.

BR,
Jani.

>
> Side note, looking at the VBT spec on tp1, tp2/tp3 wakeup times, there
> seems to be some confusion about what the values mean. Has there perhaps
> been a change in the spec? What's the VBT version where the problems
> happen?
>
> BR,
> Jani.
>
>>
>>>  	uint32_t val = EDP_PSR_ENABLE;
>>>  
>>>  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
>>> -- 
>>> 1.9.1
>>> 
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-09-13  9:05 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-08  0:42 [PATCH] Revert "drm/i915/psr: Make idle_frames sensible again" Rodrigo Vivi
2016-09-08  1:53 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-09-08  8:01 ` [PATCH] " Ville Syrjälä
2016-09-08  8:53   ` Jani Nikula
2016-09-13  9:05     ` Jani Nikula

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.