Re: [Intel-gfx] [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Ville Syrjälä]

On Mon, Nov 20, 2017 at 12:12:40PM +0100, Maarten Lankhorst wrote:
> Op 17-11-17 om 16:55 schreef Ville Syrjälä:
> > On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst 
> >> ---
> >>  drivers/gpu/drm/i915/intel_display.c | 13 ++---
> >>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 8283e80597bd..38a1cdb3dbb2 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct 
> >> intel_crtc_state *old_crtc_state)
> >>intel_fbc_post_update(crtc);
> >>  
> >>if (primary_state->base.visible &&
> >> -  (needs_modeset(_config->base) ||
> >> +  (pipe_config->disable_cxsr ||
> >> !old_primary_state->base.visible))
> >>intel_post_enable_primary(>base, pipe_config);
> >>}
> >> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct 
> >> intel_crtc_state *old_crtc_state,
> >>struct intel_atomic_state *old_intel_state =
> >>to_intel_atomic_state(old_state);
> >>  
> >> -  if (needs_modeset(_config->base) || !pipe_config->ips_enabled)
> >> +  if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
> > What does IPS have to do with cxsr?
> Nothing, laziness. disable cxsr is set when planes get disabled.
> >>hsw_disable_ips(old_crtc_state);
> >>  
> >>if (old_pri_state) {
> >> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct 
> >> drm_i915_private *dev_priv,
> >>visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
> >>  
> >>/*
> >> -   * FIXME IPS should be fine as long as one plane is
> >> -   * enabled, but in practice it seems to have problems
> >> -   * when going from primary only to sprite only and vice
> >> -   * versa.
> >> +   * IPS should be fine as long as one plane is enabled, but
> >> +   * temporarily disable it when when going from primary only
> >> +   * to sprite only and vice versa.
> >> */
> >> -  if (visible_planes != BIT(PLANE_PRIMARY))
> >> +  if (hweight32(visible_planes) != 1)
> >>return false;
> > That should just be
> > if (active_planes == 0)
> > return false;
> >
> > assuming we have no problems with the toggling between
> > primary only and sprite only.
> >
> > I can't recall how the cursor affecrs IPS. But I think IPS should 
> > work as long as any plane (including the cursor) is enabled.
> But cursor visibility can change without the full atomic commit, so it's too 
> unreliable to take into
> account with the calculations.

Hmm. I guess we can ignore the cursor then. It's generally pretty
small, so probably no huge benefit from using IPS with cursor only
anyway.

> 
> What happens when it doesn't work well? Would it be caught by any tests?

I think it should be caught by CRCs, if we have any that capture
CRCs across plane enable/disable.

Except, there is a problem on ilk-ivb at least, maybe it went all
the way to bdw. IIRC when I last tried a test like that the CRC for
a black primary plane didn't match the CRC for no planes. And I think I
tried to eliminate gamma/csc as the source of the discrepancy without
success. Also IIRC there was always a single frame with an even stranger
CRC just after turning off the last plane.

-- 
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/3] drm/i915: Enable IPS for sprite plane

[Maarten Lankhorst]

Op 17-11-17 om 16:55 schreef Ville Syrjälä:
> On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 13 ++---
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c 
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 8283e80597bd..38a1cdb3dbb2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct 
>> intel_crtc_state *old_crtc_state)
>>  intel_fbc_post_update(crtc);
>>  
>>  if (primary_state->base.visible &&
>> -(needs_modeset(_config->base) ||
>> +(pipe_config->disable_cxsr ||
>>   !old_primary_state->base.visible))
>>  intel_post_enable_primary(>base, pipe_config);
>>  }
>> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct 
>> intel_crtc_state *old_crtc_state,
>>  struct intel_atomic_state *old_intel_state =
>>  to_intel_atomic_state(old_state);
>>  
>> -if (needs_modeset(_config->base) || !pipe_config->ips_enabled)
>> +if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
> What does IPS have to do with cxsr?
Nothing, laziness. disable cxsr is set when planes get disabled.
>>  hsw_disable_ips(old_crtc_state);
>>  
>>  if (old_pri_state) {
>> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct 
>> drm_i915_private *dev_priv,
>>  visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
>>  
>>  /*
>> - * FIXME IPS should be fine as long as one plane is
>> - * enabled, but in practice it seems to have problems
>> - * when going from primary only to sprite only and vice
>> - * versa.
>> + * IPS should be fine as long as one plane is enabled, but
>> + * temporarily disable it when when going from primary only
>> + * to sprite only and vice versa.
>>   */
>> -if (visible_planes != BIT(PLANE_PRIMARY))
>> +if (hweight32(visible_planes) != 1)
>>  return false;
> That should just be
> if (active_planes == 0)
>   return false;
>
> assuming we have no problems with the toggling between
> primary only and sprite only.
>
> I can't recall how the cursor affecrs IPS. But I think IPS should 
> work as long as any plane (including the cursor) is enabled.
But cursor visibility can change without the full atomic commit, so it's too 
unreliable to take into
account with the calculations.

What happens when it doesn't work well? Would it be caught by any tests?

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

Re: [Intel-gfx] [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Ville Syrjälä]

On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 8283e80597bd..38a1cdb3dbb2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct 
> intel_crtc_state *old_crtc_state)
>   intel_fbc_post_update(crtc);
>  
>   if (primary_state->base.visible &&
> - (needs_modeset(_config->base) ||
> + (pipe_config->disable_cxsr ||
>!old_primary_state->base.visible))
>   intel_post_enable_primary(>base, pipe_config);
>   }
> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct 
> intel_crtc_state *old_crtc_state,
>   struct intel_atomic_state *old_intel_state =
>   to_intel_atomic_state(old_state);
>  
> - if (needs_modeset(_config->base) || !pipe_config->ips_enabled)
> + if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)

What does IPS have to do with cxsr?

>   hsw_disable_ips(old_crtc_state);
>  
>   if (old_pri_state) {
> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct 
> drm_i915_private *dev_priv,
>   visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
>  
>   /*
> -  * FIXME IPS should be fine as long as one plane is
> -  * enabled, but in practice it seems to have problems
> -  * when going from primary only to sprite only and vice
> -  * versa.
> +  * IPS should be fine as long as one plane is enabled, but
> +  * temporarily disable it when when going from primary only
> +  * to sprite only and vice versa.
>*/
> - if (visible_planes != BIT(PLANE_PRIMARY))
> + if (hweight32(visible_planes) != 1)
>   return false;

That should just be
if (active_planes == 0)
return false;

assuming we have no problems with the toggling between
primary only and sprite only.

I can't recall how the cursor affecrs IPS. But I think IPS should 
work as long as any plane (including the cursor) is enabled.

>  
>   /* HSW can handle pixel rate up to cdclk? */
> -- 
> 2.15.0
> 
> ___
> 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

[Intel-gfx] [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Maarten Lankhorst]

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/intel_display.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 8283e80597bd..38a1cdb3dbb2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct 
intel_crtc_state *old_crtc_state)
intel_fbc_post_update(crtc);
 
if (primary_state->base.visible &&
-   (needs_modeset(_config->base) ||
+   (pipe_config->disable_cxsr ||
 !old_primary_state->base.visible))
intel_post_enable_primary(>base, pipe_config);
}
@@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct 
intel_crtc_state *old_crtc_state,
struct intel_atomic_state *old_intel_state =
to_intel_atomic_state(old_state);
 
-   if (needs_modeset(_config->base) || !pipe_config->ips_enabled)
+   if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
hsw_disable_ips(old_crtc_state);
 
if (old_pri_state) {
@@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct 
drm_i915_private *dev_priv,
visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
 
/*
-* FIXME IPS should be fine as long as one plane is
-* enabled, but in practice it seems to have problems
-* when going from primary only to sprite only and vice
-* versa.
+* IPS should be fine as long as one plane is enabled, but
+* temporarily disable it when when going from primary only
+* to sprite only and vice versa.
 */
-   if (visible_planes != BIT(PLANE_PRIMARY))
+   if (hweight32(visible_planes) != 1)
return false;
 
/* HSW can handle pixel rate up to cdclk? */
-- 
2.15.0

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