Re: [Intel-gfx] [PATCH] drm/i915: Block enabling FBC until flips have been completed

[Maarten Lankhorst]

Op 12-04-18 om 21:41 schreef Souza, Jose:
> On Thu, 2018-04-12 at 18:07 +0200, Maarten Lankhorst wrote:
>> There is a small race window in which FBC can be enabled after
>> pre_plane_update is called, but before the page flip has been
>> queued or completed.
> I don't think there is such window, intel_fbc_deactivate() that is
> called from intel_fbc_pre_update() will set fbc->work.scheduled =
> false; so the FBC will not be enabled in intel_fbc_work_fn()
Yeah, intel_fbc_pre_update deactivates it, but intel_fbc_flush() can re-enable 
it. :)
>> Signed-off-by: Maarten Lankhorst 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>>  drivers/gpu/drm/i915/intel_fbc.c | 35 +++---
>> -
>>  2 files changed, 12 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index a0b8db3db141..2e2f24c2db9e 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -492,6 +492,7 @@ struct intel_fbc {
>>  
>>  bool enabled;
>>  bool active;
>> +bool flip_pending;
>>  
>>  bool underrun_detected;
>>  struct work_struct underrun_work;
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
>> b/drivers/gpu/drm/i915/intel_fbc.c
>> index b431b6733cc1..4770dd7dad5c 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -924,13 +924,6 @@ static void intel_fbc_get_reg_params(struct
>> intel_crtc *crtc,
>>  32 * fbc->threshold) 
>> * 8;
>>  }
>>  
>> -static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params
>> *params1,
>> -   struct intel_fbc_reg_params
>> *params2)
>> -{
>> -/* We can use this since intel_fbc_get_reg_params() does a
>> memset. */
>> -return memcmp(params1, params2, sizeof(*params1)) == 0;
>> -}
>> -
>>  void intel_fbc_pre_update(struct intel_crtc *crtc,
>>struct intel_crtc_state *crtc_state,
>>struct intel_plane_state *plane_state)
>> @@ -952,6 +945,7 @@ void intel_fbc_pre_update(struct intel_crtc
>> *crtc,
>>  if (!fbc->enabled || fbc->crtc != crtc)
>>  goto unlock;
>>  
>> +fbc->flip_pending = true;
> Also this is not a good name, other actions can cause this function to
> be executed other than a flip.
Well, for FBC purposes it's a flip, but I am also ok with renaming it to 
update_pending? :)
>>  intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
>>  
>>  deactivate:
>> @@ -988,13 +982,15 @@ static void __intel_fbc_post_update(struct
>> intel_crtc *crtc)
>>  {
>>  struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>  struct intel_fbc *fbc = _priv->fbc;
>> -struct intel_fbc_reg_params old_params;
>>  
>>  WARN_ON(!mutex_is_locked(>lock));
>>  
>>  if (!fbc->enabled || fbc->crtc != crtc)
>>  return;
>>  
>> +fbc->flip_pending = false;
>> +WARN_ON(fbc->active);
>> +
>>  if (!i915_modparams.enable_fbc) {
>>  intel_fbc_deactivate(dev_priv, "disabled at runtime
>> per module param");
>>  __intel_fbc_disable(dev_priv);
>> @@ -1002,25 +998,16 @@ static void __intel_fbc_post_update(struct
>> intel_crtc *crtc)
>>  return;
>>  }
>>  
>> -if (!intel_fbc_can_activate(crtc)) {
>> -WARN_ON(fbc->active);
>> +if (!intel_fbc_can_activate(crtc))
>>  return;
>> -}
>>  
>> -old_params = fbc->params;
>>  intel_fbc_get_reg_params(crtc, >params);
>>  
>> -/* If the scanout has not changed, don't modify the FBC
>> settings.
>> - * Note that we make the fundamental assumption that the fb-
>>> obj
>> - * cannot be unpinned (and have its GTT offset and fence
>> revoked)
>> - * without first being decoupled from the scanout and FBC
>> disabled.
>> - */
>> -if (fbc->active &&
>> -intel_fbc_reg_params_equal(_params, >params))
>> -return;
>> -
>> -intel_fbc_deactivate(dev_priv, "FBC enabled (active or
>> scheduled)");
>> -intel_fbc_schedule_activation(crtc);
>> +if (!fbc->busy_bits) {
> I guess this 'if' the line that is fixing the issue.

I think that's not necessarily the case for these tests. I don't know if this 
fixes
the bug, as the dirtyfb is called after the atomic update completed. I just 
noted that
after pre_plane_update, you could sneak in a dirtyfb and then fbc could be 
activated
too early.

That's the hole I've been trying to close. But I closed it the other way around 
too
just in case. :) 

>> +intel_fbc_deactivate(dev_priv, "FBC enabled (active
>> or scheduled)");
>> +intel_fbc_schedule_activation(crtc);
>> +} else
>> +intel_fbc_deactivate(dev_priv, "frontbuffer write");
>>  }
>>  
>>  void intel_fbc_post_update(struct intel_crtc *crtc)
>> 

Re: [Intel-gfx] [PATCH] drm/i915: Block enabling FBC until flips have been completed

[Souza, Jose]

On Thu, 2018-04-12 at 18:07 +0200, Maarten Lankhorst wrote:
> There is a small race window in which FBC can be enabled after
> pre_plane_update is called, but before the page flip has been
> queued or completed.

I don't think there is such window, intel_fbc_deactivate() that is
called from intel_fbc_pre_update() will set fbc->work.scheduled =
false; so the FBC will not be enabled in intel_fbc_work_fn()

> 
> Signed-off-by: Maarten Lankhorst 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_fbc.c | 35 +++---
> -
>  2 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index a0b8db3db141..2e2f24c2db9e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -492,6 +492,7 @@ struct intel_fbc {
>  
>   bool enabled;
>   bool active;
> + bool flip_pending;
>  
>   bool underrun_detected;
>   struct work_struct underrun_work;
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c
> b/drivers/gpu/drm/i915/intel_fbc.c
> index b431b6733cc1..4770dd7dad5c 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -924,13 +924,6 @@ static void intel_fbc_get_reg_params(struct
> intel_crtc *crtc,
>   32 * fbc->threshold) 
> * 8;
>  }
>  
> -static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params
> *params1,
> -struct intel_fbc_reg_params
> *params2)
> -{
> - /* We can use this since intel_fbc_get_reg_params() does a
> memset. */
> - return memcmp(params1, params2, sizeof(*params1)) == 0;
> -}
> -
>  void intel_fbc_pre_update(struct intel_crtc *crtc,
> struct intel_crtc_state *crtc_state,
> struct intel_plane_state *plane_state)
> @@ -952,6 +945,7 @@ void intel_fbc_pre_update(struct intel_crtc
> *crtc,
>   if (!fbc->enabled || fbc->crtc != crtc)
>   goto unlock;
>  
> + fbc->flip_pending = true;

Also this is not a good name, other actions can cause this function to
be executed other than a flip.

>   intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
>  
>  deactivate:
> @@ -988,13 +982,15 @@ static void __intel_fbc_post_update(struct
> intel_crtc *crtc)
>  {
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   struct intel_fbc *fbc = _priv->fbc;
> - struct intel_fbc_reg_params old_params;
>  
>   WARN_ON(!mutex_is_locked(>lock));
>  
>   if (!fbc->enabled || fbc->crtc != crtc)
>   return;
>  
> + fbc->flip_pending = false;
> + WARN_ON(fbc->active);
> +
>   if (!i915_modparams.enable_fbc) {
>   intel_fbc_deactivate(dev_priv, "disabled at runtime
> per module param");
>   __intel_fbc_disable(dev_priv);
> @@ -1002,25 +998,16 @@ static void __intel_fbc_post_update(struct
> intel_crtc *crtc)
>   return;
>   }
>  
> - if (!intel_fbc_can_activate(crtc)) {
> - WARN_ON(fbc->active);
> + if (!intel_fbc_can_activate(crtc))
>   return;
> - }
>  
> - old_params = fbc->params;
>   intel_fbc_get_reg_params(crtc, >params);
>  
> - /* If the scanout has not changed, don't modify the FBC
> settings.
> -  * Note that we make the fundamental assumption that the fb-
> >obj
> -  * cannot be unpinned (and have its GTT offset and fence
> revoked)
> -  * without first being decoupled from the scanout and FBC
> disabled.
> -  */
> - if (fbc->active &&
> - intel_fbc_reg_params_equal(_params, >params))
> - return;
> -
> - intel_fbc_deactivate(dev_priv, "FBC enabled (active or
> scheduled)");
> - intel_fbc_schedule_activation(crtc);
> + if (!fbc->busy_bits) {

I guess this 'if' the line that is fixing the issue.

> + intel_fbc_deactivate(dev_priv, "FBC enabled (active
> or scheduled)");
> + intel_fbc_schedule_activation(crtc);
> + } else
> + intel_fbc_deactivate(dev_priv, "frontbuffer write");
>  }
>  
>  void intel_fbc_post_update(struct intel_crtc *crtc)
> @@ -1085,7 +1072,7 @@ void intel_fbc_flush(struct drm_i915_private
> *dev_priv,
>   (frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc)))
> {
>   if (fbc->active)
>   intel_fbc_recompress(dev_priv);
> - else
> + else if (!fbc->flip_pending)
>   __intel_fbc_post_update(fbc->crtc);
>   }
>  
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Block enabling FBC until flips have been completed

[Maarten Lankhorst]

There is a small race window in which FBC can be enabled after
pre_plane_update is called, but before the page flip has been
queued or completed.

Signed-off-by: Maarten Lankhorst 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103167
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_fbc.c | 35 +++
 2 files changed, 12 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a0b8db3db141..2e2f24c2db9e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -492,6 +492,7 @@ struct intel_fbc {
 
bool enabled;
bool active;
+   bool flip_pending;
 
bool underrun_detected;
struct work_struct underrun_work;
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index b431b6733cc1..4770dd7dad5c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -924,13 +924,6 @@ static void intel_fbc_get_reg_params(struct intel_crtc 
*crtc,
32 * fbc->threshold) * 8;
 }
 
-static bool intel_fbc_reg_params_equal(struct intel_fbc_reg_params *params1,
-  struct intel_fbc_reg_params *params2)
-{
-   /* We can use this since intel_fbc_get_reg_params() does a memset. */
-   return memcmp(params1, params2, sizeof(*params1)) == 0;
-}
-
 void intel_fbc_pre_update(struct intel_crtc *crtc,
  struct intel_crtc_state *crtc_state,
  struct intel_plane_state *plane_state)
@@ -952,6 +945,7 @@ void intel_fbc_pre_update(struct intel_crtc *crtc,
if (!fbc->enabled || fbc->crtc != crtc)
goto unlock;
 
+   fbc->flip_pending = true;
intel_fbc_update_state_cache(crtc, crtc_state, plane_state);
 
 deactivate:
@@ -988,13 +982,15 @@ static void __intel_fbc_post_update(struct intel_crtc 
*crtc)
 {
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
struct intel_fbc *fbc = _priv->fbc;
-   struct intel_fbc_reg_params old_params;
 
WARN_ON(!mutex_is_locked(>lock));
 
if (!fbc->enabled || fbc->crtc != crtc)
return;
 
+   fbc->flip_pending = false;
+   WARN_ON(fbc->active);
+
if (!i915_modparams.enable_fbc) {
intel_fbc_deactivate(dev_priv, "disabled at runtime per module 
param");
__intel_fbc_disable(dev_priv);
@@ -1002,25 +998,16 @@ static void __intel_fbc_post_update(struct intel_crtc 
*crtc)
return;
}
 
-   if (!intel_fbc_can_activate(crtc)) {
-   WARN_ON(fbc->active);
+   if (!intel_fbc_can_activate(crtc))
return;
-   }
 
-   old_params = fbc->params;
intel_fbc_get_reg_params(crtc, >params);
 
-   /* If the scanout has not changed, don't modify the FBC settings.
-* Note that we make the fundamental assumption that the fb->obj
-* cannot be unpinned (and have its GTT offset and fence revoked)
-* without first being decoupled from the scanout and FBC disabled.
-*/
-   if (fbc->active &&
-   intel_fbc_reg_params_equal(_params, >params))
-   return;
-
-   intel_fbc_deactivate(dev_priv, "FBC enabled (active or scheduled)");
-   intel_fbc_schedule_activation(crtc);
+   if (!fbc->busy_bits) {
+   intel_fbc_deactivate(dev_priv, "FBC enabled (active or 
scheduled)");
+   intel_fbc_schedule_activation(crtc);
+   } else
+   intel_fbc_deactivate(dev_priv, "frontbuffer write");
 }
 
 void intel_fbc_post_update(struct intel_crtc *crtc)
@@ -1085,7 +1072,7 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
(frontbuffer_bits & intel_fbc_get_frontbuffer_bit(fbc))) {
if (fbc->active)
intel_fbc_recompress(dev_priv);
-   else
+   else if (!fbc->flip_pending)
__intel_fbc_post_update(fbc->crtc);
}
 
-- 
2.16.3

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