[PATCH] drm/i915: HSW/BDW PSR Set idle_frames=2 since sometimes it can be off by 1.

[PATCH] drm/i915: HSW/BDW PSR Set idle_frames=2 since sometimes it can be off by 1.

[Rodrigo Vivi]

It was identified that in some cases when moving cursor Hardware can do
mistake with idle_frame count. So Spec is being updated to use
2 as minimum idle_frames.

Reference: https://hsdhsw.intel.com/hsd/haswell_platform/default.aspx#sighting/default.aspx?sighting_id=4394433
Cc: Arthur Runyan <arthur.j.runyan@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 47e9d71..8cfbbc2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2178,7 +2178,7 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t max_sleep_time = 0x1f;
-	uint32_t idle_frames = 1;
+	uint32_t idle_frames = 2; /* 2 is the minimum allowed */
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 	bool only_standby = false;
-- 
1.9.3

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

Re: [PATCH] drm/i915: HSW/BDW PSR Set idle_frames=2 since sometimes it can be off by 1.

[Daniel Vetter]

On Thu, Oct 30, 2014 at 05:35:55AM -0700, Rodrigo Vivi wrote:
> It was identified that in some cases when moving cursor Hardware can do
> mistake with idle_frame count. So Spec is being updated to use
> 2 as minimum idle_frames.
> 
> Reference: https://hsdhsw.intel.com/hsd/haswell_platform/default.aspx#sighting/default.aspx?sighting_id=4394433
> Cc: Arthur Runyan <arthur.j.runyan@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Since we have full-blown sw frontbuffer tracking: Do we care?

I.e. is the effect of idle_frames = 1 that the hw will go bananas (bad, we
want this patch) or that it might miss a cursor movement (no problem, the
kernel will catch it anyway)?

Cheers, Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 47e9d71..8cfbbc2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2178,7 +2178,7 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t max_sleep_time = 0x1f;
> -	uint32_t idle_frames = 1;
> +	uint32_t idle_frames = 2; /* 2 is the minimum allowed */
>  	uint32_t val = 0x0;
>  	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>  	bool only_standby = false;
> -- 
> 1.9.3
> 
> _______________________________________________
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: HSW/BDW PSR Set idle_frames=2 since sometimes it can be off by 1.

[Runyan, Arthur J]

>From: Daniel Vetter 
>
>On Thu, Oct 30, 2014 at 05:35:55AM -0700, Rodrigo Vivi wrote:
>> It was identified that in some cases when moving cursor Hardware can do
>> mistake with idle_frame count. So Spec is being updated to use
>> 2 as minimum idle_frames.
>>
>> Reference:
>https://hsdhsw.intel.com/hsd/haswell_platform/default.aspx#sighting/default.as
>px?sighting_id=4394433
>> Cc: Arthur Runyan <arthur.j.runyan@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
>Since we have full-blown sw frontbuffer tracking: Do we care?
>
>I.e. is the effect of idle_frames = 1 that the hw will go bananas (bad, we
>want this patch) or that it might miss a cursor movement (no problem, the
>kernel will catch it anyway)?
>

The behavior will depend on the panel.  Going bananas is a possibility.
It also looks like VBT is being used to indicate that some panels have minimum idle frame requirements, so you really should be using VBT + 1 to ensure the hardware always outputs at least the VBT specified number of frames.


>Cheers, Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 47e9d71..8cfbbc2 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2178,7 +2178,7 @@ static void intel_edp_psr_enable_source(struct
>intel_dp *intel_dp)
>>  	struct drm_device *dev = dig_port->base.base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	uint32_t max_sleep_time = 0x1f;
>> -	uint32_t idle_frames = 1;
>> +	uint32_t idle_frames = 2; /* 2 is the minimum allowed */
>>  	uint32_t val = 0x0;
>>  	const uint32_t link_entry_time =
>EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>>  	bool only_standby = false;
>> --
>> 1.9.3
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: HSW/BDW PSR Set idle_frames=2 since sometimes it can be off by 1.

[Rodrigo Vivi]

On Mon, Nov 3, 2014 at 9:25 AM, Runyan, Arthur J
<arthur.j.runyan@intel.com> wrote:
>>From: Daniel Vetter
>>
>>On Thu, Oct 30, 2014 at 05:35:55AM -0700, Rodrigo Vivi wrote:
>>> It was identified that in some cases when moving cursor Hardware can do
>>> mistake with idle_frame count. So Spec is being updated to use
>>> 2 as minimum idle_frames.
>>>
>>> Reference:
>>https://hsdhsw.intel.com/hsd/haswell_platform/default.aspx#sighting/default.as
>>px?sighting_id=4394433
>>> Cc: Arthur Runyan <arthur.j.runyan@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>>Since we have full-blown sw frontbuffer tracking: Do we care?
>>
>>I.e. is the effect of idle_frames = 1 that the hw will go bananas (bad, we
>>want this patch) or that it might miss a cursor movement (no problem, the
>>kernel will catch it anyway)?

Even with the frontbuffer tracking HW still controls when to really
entry PSR based on this count. So I prefer to be on the safe side and
do the propper setup. If we
I had faced some cases here where even with propper frontbuffer
tracking idle_frames=2 let it more stable. I had also sent this same
patch in the past. But at that point I didn't know we had this miss
calculation issue on HW.

>>
>
> The behavior will depend on the panel.  Going bananas is a possibility.
> It also looks like VBT is being used to indicate that some panels have minimum idle frame requirements, so you really should be using VBT + 1 to ensure the hardware always outputs at least the VBT specified number of frames.

After think a bit I still think this is the patch to go to -fixes.
On the reorg series with 4 patches I just sent I add VBT parse and get
this idle_frame from there. But I'd prefer that regorg going to dinq
and this patch here to -fixes.

>
>
>>Cheers, Daniel
>>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 47e9d71..8cfbbc2 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -2178,7 +2178,7 @@ static void intel_edp_psr_enable_source(struct
>>intel_dp *intel_dp)
>>>      struct drm_device *dev = dig_port->base.base.dev;
>>>      struct drm_i915_private *dev_priv = dev->dev_private;
>>>      uint32_t max_sleep_time = 0x1f;
>>> -    uint32_t idle_frames = 1;
>>> +    uint32_t idle_frames = 2; /* 2 is the minimum allowed */
>>>      uint32_t val = 0x0;
>>>      const uint32_t link_entry_time =
>>EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>>>      bool only_standby = false;
>>> --
>>> 1.9.3
>>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Thanks,
Rodrigo.

-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: HSW/BDW PSR Set idle_frames=2 since sometimes it can be off by 1.

[Daniel Vetter]

On Fri, Nov 07, 2014 at 04:01:29PM -0800, Rodrigo Vivi wrote:
> After think a bit I still think this is the patch to go to -fixes.
> On the reorg series with 4 patches I just sent I add VBT parse and get
> this idle_frame from there. But I'd prefer that regorg going to dinq
> and this patch here to -fixes.

psr is disabled in 3.18 (and dinq fwiw) so I don't think this is -fixes
material.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx