Re: [Intel-gfx] [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Rodrigo Vivi]

On Tue, Mar 06, 2018 at 09:12:02PM +0100, Maarten Lankhorst wrote:
> Op 06-03-18 om 21:02 schreef Rodrigo Vivi:
> > On Tue, Mar 06, 2018 at 11:22:16AM +0100, Maarten Lankhorst wrote:
> >> Op 05-03-18 om 19:50 schreef Rodrigo Vivi:
> >>> On Mon, Mar 05, 2018 at 01:36:08PM +0100, Maarten Lankhorst wrote:
>  If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
>  then we don't disable FBC until the next time the crtc is disabled.
> 
>  Make sure that if the module param is changed, we disable FBC in
>  intel_fbc_post_update so we never have to worry about disabling.
> >>> What about switching this from a parameter to debugfs toggle like drrs?
> >> There are still places where people recommend booting with 
> >> i915.enable_fbc=0, this would break if we moved it to debugfs.
> > I think this was exactly what Chris and Joonas were trying to avoid when 
> > removing enable_rc6.
> > To avoid random recommendations of unvalidated combinations.
> >
> >> Having a small fix is enough.
> >>
> > Well... it would be fine for me.
> >
> > But I went to look the patch and I don't know what exactly we are trying to 
> > fix here.
> >
> > I don't know if it is safe to disable it like this when crtc is already 
> > enabled
> > and mostly: why?! why just not wait until next modeset?
> >
> > Cc: Paulo
> I'm trying to get rid of all modesets in kms_frontbuffer_tracking, they add 
> up on glk where it's 50% of the total runtime for kms tests. :)

oh! this is a very good reason indeed. ;)

Patch looks correct to me. my only concern is with the reliability of toggling 
this on/off without
the modeset. But if there is problem with this I believe the test cases on CI 
will get this.

Besides I just remembered that I did some toggle on/off on sysfs at somepoint 
when playing with
that fbc false color and I never saw a problem... so:

Reviewed-by: Rodrigo Vivi 

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

Re: [Intel-gfx] [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Maarten Lankhorst]

Op 06-03-18 om 21:02 schreef Rodrigo Vivi:
> On Tue, Mar 06, 2018 at 11:22:16AM +0100, Maarten Lankhorst wrote:
>> Op 05-03-18 om 19:50 schreef Rodrigo Vivi:
>>> On Mon, Mar 05, 2018 at 01:36:08PM +0100, Maarten Lankhorst wrote:
 If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
 then we don't disable FBC until the next time the crtc is disabled.

 Make sure that if the module param is changed, we disable FBC in
 intel_fbc_post_update so we never have to worry about disabling.
>>> What about switching this from a parameter to debugfs toggle like drrs?
>> There are still places where people recommend booting with 
>> i915.enable_fbc=0, this would break if we moved it to debugfs.
> I think this was exactly what Chris and Joonas were trying to avoid when 
> removing enable_rc6.
> To avoid random recommendations of unvalidated combinations.
>
>> Having a small fix is enough.
>>
> Well... it would be fine for me.
>
> But I went to look the patch and I don't know what exactly we are trying to 
> fix here.
>
> I don't know if it is safe to disable it like this when crtc is already 
> enabled
> and mostly: why?! why just not wait until next modeset?
>
> Cc: Paulo
I'm trying to get rid of all modesets in kms_frontbuffer_tracking, they add up 
on glk where it's 50% of the total runtime for kms tests. :)

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

Re: [Intel-gfx] [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Rodrigo Vivi]

On Tue, Mar 06, 2018 at 11:22:16AM +0100, Maarten Lankhorst wrote:
> Op 05-03-18 om 19:50 schreef Rodrigo Vivi:
> > On Mon, Mar 05, 2018 at 01:36:08PM +0100, Maarten Lankhorst wrote:
> >> If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
> >> then we don't disable FBC until the next time the crtc is disabled.
> >>
> >> Make sure that if the module param is changed, we disable FBC in
> >> intel_fbc_post_update so we never have to worry about disabling.
> > What about switching this from a parameter to debugfs toggle like drrs?
> There are still places where people recommend booting with i915.enable_fbc=0, 
> this would break if we moved it to debugfs.

I think this was exactly what Chris and Joonas were trying to avoid when 
removing enable_rc6.
To avoid random recommendations of unvalidated combinations.

> 
> Having a small fix is enough.
>

Well... it would be fine for me.

But I went to look the patch and I don't know what exactly we are trying to fix 
here.

I don't know if it is safe to disable it like this when crtc is already enabled
and mostly: why?! why just not wait until next modeset?

Cc: Paulo

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

Re: [Intel-gfx] [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Maarten Lankhorst]

Op 05-03-18 om 19:50 schreef Rodrigo Vivi:
> On Mon, Mar 05, 2018 at 01:36:08PM +0100, Maarten Lankhorst wrote:
>> If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
>> then we don't disable FBC until the next time the crtc is disabled.
>>
>> Make sure that if the module param is changed, we disable FBC in
>> intel_fbc_post_update so we never have to worry about disabling.
> What about switching this from a parameter to debugfs toggle like drrs?
There are still places where people recommend booting with i915.enable_fbc=0, 
this would break if we moved it to debugfs.

Having a small fix is enough.

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

Re: [Intel-gfx] [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Rodrigo Vivi]

On Mon, Mar 05, 2018 at 01:36:08PM +0100, Maarten Lankhorst wrote:
> If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
> then we don't disable FBC until the next time the crtc is disabled.
> 
> Make sure that if the module param is changed, we disable FBC in
> intel_fbc_post_update so we never have to worry about disabling.

What about switching this from a parameter to debugfs toggle like drrs?

> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_fbc.c | 62 
> +++-
>  1 file changed, 36 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c 
> b/drivers/gpu/drm/i915/intel_fbc.c
> index eaaa59b45707..5325b59c2f9c 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -949,6 +949,30 @@ void intel_fbc_pre_update(struct intel_crtc *crtc,
>   mutex_unlock(>lock);
>  }
>  
> +/**
> + * __intel_fbc_disable - disable FBC
> + * @dev_priv: i915 device instance
> + *
> + * This is the low level function that actually disables FBC. Callers should
> + * grab the FBC lock.
> + */
> +static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
> +{
> + struct intel_fbc *fbc = _priv->fbc;
> + struct intel_crtc *crtc = fbc->crtc;
> +
> + WARN_ON(!mutex_is_locked(>lock));
> + WARN_ON(!fbc->enabled);
> + WARN_ON(fbc->active);
> +
> + DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
> +
> + __intel_fbc_cleanup_cfb(dev_priv);
> +
> + fbc->enabled = false;
> + fbc->crtc = NULL;
> +}
> +
>  static void __intel_fbc_post_update(struct intel_crtc *crtc)
>  {
>   struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -960,6 +984,13 @@ static void __intel_fbc_post_update(struct intel_crtc 
> *crtc)
>   if (!fbc->enabled || fbc->crtc != crtc)
>   return;
>  
> + if (!i915_modparams.enable_fbc) {
> + intel_fbc_deactivate(dev_priv, "disabled at runtime per module 
> param");
> + __intel_fbc_disable(dev_priv);
> +
> + return;
> + }
> +
>   if (!intel_fbc_can_activate(crtc)) {
>   WARN_ON(fbc->active);
>   return;
> @@ -1163,31 +1194,6 @@ void intel_fbc_enable(struct intel_crtc *crtc,
>   mutex_unlock(>lock);
>  }
>  
> -/**
> - * __intel_fbc_disable - disable FBC
> - * @dev_priv: i915 device instance
> - *
> - * This is the low level function that actually disables FBC. Callers should
> - * grab the FBC lock.
> - */
> -static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
> -{
> - struct intel_fbc *fbc = _priv->fbc;
> - struct intel_crtc *crtc = fbc->crtc;
> -
> - WARN_ON(!mutex_is_locked(>lock));
> - WARN_ON(!fbc->enabled);
> - WARN_ON(fbc->active);
> - WARN_ON(crtc->active);
> -
> - DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
> -
> - __intel_fbc_cleanup_cfb(dev_priv);
> -
> - fbc->enabled = false;
> - fbc->crtc = NULL;
> -}
> -
>  /**
>   * intel_fbc_disable - disable FBC if it's associated with crtc
>   * @crtc: the CRTC
> @@ -1202,6 +1208,8 @@ void intel_fbc_disable(struct intel_crtc *crtc)
>   if (!fbc_supported(dev_priv))
>   return;
>  
> + WARN_ON(crtc->active);
> +
>   mutex_lock(>lock);
>   if (fbc->crtc == crtc)
>   __intel_fbc_disable(dev_priv);
> @@ -1224,8 +1232,10 @@ void intel_fbc_global_disable(struct drm_i915_private 
> *dev_priv)
>   return;
>  
>   mutex_lock(>lock);
> - if (fbc->enabled)
> + if (fbc->enabled) {
> + WARN_ON(fbc->crtc->active);
>   __intel_fbc_disable(dev_priv);
> + }
>   mutex_unlock(>lock);
>  
>   cancel_work_sync(>work.work);
> -- 
> 2.16.2
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Handle changing enable_fbc parameter at runtime better.

[Maarten Lankhorst]

If i915.enable_fbc is cleared at runtime, but FBC was previously enabled
then we don't disable FBC until the next time the crtc is disabled.

Make sure that if the module param is changed, we disable FBC in
intel_fbc_post_update so we never have to worry about disabling.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/intel_fbc.c | 62 +++-
 1 file changed, 36 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index eaaa59b45707..5325b59c2f9c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -949,6 +949,30 @@ void intel_fbc_pre_update(struct intel_crtc *crtc,
mutex_unlock(>lock);
 }
 
+/**
+ * __intel_fbc_disable - disable FBC
+ * @dev_priv: i915 device instance
+ *
+ * This is the low level function that actually disables FBC. Callers should
+ * grab the FBC lock.
+ */
+static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
+{
+   struct intel_fbc *fbc = _priv->fbc;
+   struct intel_crtc *crtc = fbc->crtc;
+
+   WARN_ON(!mutex_is_locked(>lock));
+   WARN_ON(!fbc->enabled);
+   WARN_ON(fbc->active);
+
+   DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
+
+   __intel_fbc_cleanup_cfb(dev_priv);
+
+   fbc->enabled = false;
+   fbc->crtc = NULL;
+}
+
 static void __intel_fbc_post_update(struct intel_crtc *crtc)
 {
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -960,6 +984,13 @@ static void __intel_fbc_post_update(struct intel_crtc 
*crtc)
if (!fbc->enabled || fbc->crtc != crtc)
return;
 
+   if (!i915_modparams.enable_fbc) {
+   intel_fbc_deactivate(dev_priv, "disabled at runtime per module 
param");
+   __intel_fbc_disable(dev_priv);
+
+   return;
+   }
+
if (!intel_fbc_can_activate(crtc)) {
WARN_ON(fbc->active);
return;
@@ -1163,31 +1194,6 @@ void intel_fbc_enable(struct intel_crtc *crtc,
mutex_unlock(>lock);
 }
 
-/**
- * __intel_fbc_disable - disable FBC
- * @dev_priv: i915 device instance
- *
- * This is the low level function that actually disables FBC. Callers should
- * grab the FBC lock.
- */
-static void __intel_fbc_disable(struct drm_i915_private *dev_priv)
-{
-   struct intel_fbc *fbc = _priv->fbc;
-   struct intel_crtc *crtc = fbc->crtc;
-
-   WARN_ON(!mutex_is_locked(>lock));
-   WARN_ON(!fbc->enabled);
-   WARN_ON(fbc->active);
-   WARN_ON(crtc->active);
-
-   DRM_DEBUG_KMS("Disabling FBC on pipe %c\n", pipe_name(crtc->pipe));
-
-   __intel_fbc_cleanup_cfb(dev_priv);
-
-   fbc->enabled = false;
-   fbc->crtc = NULL;
-}
-
 /**
  * intel_fbc_disable - disable FBC if it's associated with crtc
  * @crtc: the CRTC
@@ -1202,6 +1208,8 @@ void intel_fbc_disable(struct intel_crtc *crtc)
if (!fbc_supported(dev_priv))
return;
 
+   WARN_ON(crtc->active);
+
mutex_lock(>lock);
if (fbc->crtc == crtc)
__intel_fbc_disable(dev_priv);
@@ -1224,8 +1232,10 @@ void intel_fbc_global_disable(struct drm_i915_private 
*dev_priv)
return;
 
mutex_lock(>lock);
-   if (fbc->enabled)
+   if (fbc->enabled) {
+   WARN_ON(fbc->crtc->active);
__intel_fbc_disable(dev_priv);
+   }
mutex_unlock(>lock);
 
cancel_work_sync(>work.work);
-- 
2.16.2

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