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

[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 <maarten.lankhorst@linux.intel.com>
---
 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(&fbc->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 = &dev_priv->fbc;
+	struct intel_crtc *crtc = fbc->crtc;
+
+	WARN_ON(!mutex_is_locked(&fbc->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(&fbc->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 = &dev_priv->fbc;
-	struct intel_crtc *crtc = fbc->crtc;
-
-	WARN_ON(!mutex_is_locked(&fbc->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(&fbc->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(&fbc->lock);
-	if (fbc->enabled)
+	if (fbc->enabled) {
+		WARN_ON(fbc->crtc->active);
 		__intel_fbc_disable(dev_priv);
+	}
 	mutex_unlock(&fbc->lock);
 
 	cancel_work_sync(&fbc->work.work);
-- 
2.16.2

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

✓ Fi.CI.BAT: success for drm/i915: Handle changing enable_fbc parameter at runtime better.

[Patchwork]

== Series Details ==

Series: drm/i915: Handle changing enable_fbc parameter at runtime better.
URL   : https://patchwork.freedesktop.org/series/39375/
State : success

== Summary ==

Series 39375v1 drm/i915: Handle changing enable_fbc parameter at runtime better.
https://patchwork.freedesktop.org/api/1.0/series/39375/revisions/1/mbox/

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:416s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:370s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:484s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:277s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:484s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:463s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:452s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:391s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:570s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:288s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:506s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:388s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:408s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:456s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:408s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:451s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:489s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:449s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:492s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:584s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:428s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:520s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:489s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:466s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:403s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:245  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:391s
fi-cnl-y3 failed to collect. IGT log at Patchwork_8229/fi-cnl-y3/run0.log

276a88800a08604e3f617f084f59aeef75d5a01a drm-tip: 2018y-03m-05d-12h-15m-50s UTC integration manifest
021431fba377 drm/i915: Handle changing enable_fbc parameter at runtime better.

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8229/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Handle changing enable_fbc parameter at runtime better.

[Patchwork]

== Series Details ==

Series: drm/i915: Handle changing enable_fbc parameter at runtime better.
URL   : https://patchwork.freedesktop.org/series/39375/
State : success

== Summary ==

---- Possible new issues:

Test drv_suspend:
        Subgroup forcewake:
                skip       -> PASS       (shard-snb)

---- Known issues:

Test gem_eio:
        Subgroup in-flight-external:
                incomplete -> PASS       (shard-apl) fdo#104945
Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-top-edge:
                pass       -> DMESG-WARN (shard-snb) fdo#105185 +2
        Subgroup pipe-c-128x128-bottom-edge:
                pass       -> FAIL       (shard-apl) fdo#104671
Test kms_frontbuffer_tracking:
        Subgroup basic:
                pass       -> FAIL       (shard-apl) fdo#103167 +1
Test kms_plane_multiple:
        Subgroup atomic-pipe-b-tiling-yf:
                fail       -> PASS       (shard-apl) fdo#103166
Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252

fdo#104945 https://bugs.freedesktop.org/show_bug.cgi?id=104945
fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#104671 https://bugs.freedesktop.org/show_bug.cgi?id=104671
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-apl        total:3468 pass:1824 dwarn:1   dfail:0   fail:9   skip:1633 time:12250s
shard-hsw        total:3468 pass:1774 dwarn:1   dfail:0   fail:1   skip:1691 time:11739s
shard-snb        total:3468 pass:1362 dwarn:3   dfail:0   fail:2   skip:2101 time:7058s
Blacklisted hosts:
shard-kbl        total:3326 pass:1868 dwarn:4   dfail:0   fail:7   skip:1444 time:8320s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8229/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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 <maarten.lankhorst@linux.intel.com>
> ---
>  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(&fbc->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 = &dev_priv->fbc;
> +	struct intel_crtc *crtc = fbc->crtc;
> +
> +	WARN_ON(!mutex_is_locked(&fbc->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(&fbc->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 = &dev_priv->fbc;
> -	struct intel_crtc *crtc = fbc->crtc;
> -
> -	WARN_ON(!mutex_is_locked(&fbc->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(&fbc->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(&fbc->lock);
> -	if (fbc->enabled)
> +	if (fbc->enabled) {
> +		WARN_ON(fbc->crtc->active);
>  		__intel_fbc_disable(dev_priv);
> +	}
>  	mutex_unlock(&fbc->lock);
>  
>  	cancel_work_sync(&fbc->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

Re: [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: [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: [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: [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 <rodrigo.vivi@intel.com>

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