[PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Paulo Zanoni]

With the previous code we were only recomputing the DDB partitioning
for the CRTCs included in the atomic commit, so any other active CRTCs
would end up having their DDB registers zeroed. In this patch we make
sure that the computed state starts as a copy of the current
partitioning, and then we only zero the DDBs that we're actually
going to recompute.

How to reproduce the bug:
  1 - Enable the primary plane on pipe A
  2 - Enable the primary plane on pipe B
  3 - Enable the sprite plane on pipe A

Step 3 will zero the DDB partitioning for pipe B since it's not
included in the commit that enabled the sprite for pipe A.

I expect this to fix many FIFO underrun problems on gen9+.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596
Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-Skylake-Multi-Screen-Woes
Cc: stable@vger.kernel.org
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

I still have to confirm whether this closes the above bugs, but it
certainly fixes the problem I described.

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 425544b..0c2e252 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
 	int num_active;
 	int id, i;
 
+	/* Clear the partitioning for disabled planes. */
+	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
+	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
+
 	if (WARN_ON(!state))
 		return 0;
 
 	if (!cstate->base.active) {
 		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
-		memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
-		memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
 		return 0;
 	}
 
@@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state *state)
 		intel_state->wm_results.dirty_pipes = ~0;
 	}
 
+	/*
+	 * We're not recomputing for the pipes not included in the commit, so
+	 * make sure we start with the current state.
+	 */
+	memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
+
 	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
 		struct intel_crtc_state *cstate;
 
-- 
2.7.4

Re: [PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Paulo Zanoni]

Em Ter, 2016-10-04 às 14:37 -0300, Paulo Zanoni escreveu:
> With the previous code we were only recomputing the DDB partitioning
> for the CRTCs included in the atomic commit, so any other active
> CRTCs
> would end up having their DDB registers zeroed. In this patch we make
> sure that the computed state starts as a copy of the current
> partitioning, and then we only zero the DDBs that we're actually
> going to recompute.
> 
> How to reproduce the bug:
>   1 - Enable the primary plane on pipe A
>   2 - Enable the primary plane on pipe B
>   3 - Enable the sprite plane on pipe A

Forgot to mention: sprite or cursor.

> 
> Step 3 will zero the DDB partitioning for pipe B since it's not
> included in the commit that enabled the sprite for pipe A.
> 
> I expect this to fix many FIFO underrun problems on gen9+.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596
> Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-S
> kylake-Multi-Screen-Woes
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> I still have to confirm whether this closes the above bugs, but it
> certainly fixes the problem I described.
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 425544b..0c2e252 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	int num_active;
>  	int id, i;
>  
> +	/* Clear the partitioning for disabled planes. */
> +	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> +	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> +
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	if (!cstate->base.active) {
>  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> -		memset(ddb->plane[pipe], 0, sizeof(ddb-
> >plane[pipe]));
> -		memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> >y_plane[pipe]));
>  		return 0;
>  	}
>  
> @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state
> *state)
>  		intel_state->wm_results.dirty_pipes = ~0;
>  	}
>  
> +	/*
> +	 * We're not recomputing for the pipes not included in the
> commit, so
> +	 * make sure we start with the current state.
> +	 */
> +	memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
> +
>  	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
>  		struct intel_crtc_state *cstate;
>  

Re: [PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Paulo Zanoni]

Em Ter, 2016-10-04 às 14:37 -0300, Paulo Zanoni escreveu:
> With the previous code we were only recomputing the DDB partitioning
> for the CRTCs included in the atomic commit, so any other active
> CRTCs
> would end up having their DDB registers zeroed. In this patch we make
> sure that the computed state starts as a copy of the current
> partitioning, and then we only zero the DDBs that we're actually
> going to recompute.
> 
> How to reproduce the bug:
>   1 - Enable the primary plane on pipe A
>   2 - Enable the primary plane on pipe B
>   3 - Enable the sprite plane on pipe A

Forgot to mention: sprite or cursor.

> 
> Step 3 will zero the DDB partitioning for pipe B since it's not
> included in the commit that enabled the sprite for pipe A.
> 
> I expect this to fix many FIFO underrun problems on gen9+.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596
> Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-S
> kylake-Multi-Screen-Woes
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> I still have to confirm whether this closes the above bugs, but it
> certainly fixes the problem I described.
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 425544b..0c2e252 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	int num_active;
>  	int id, i;
>  
> +	/* Clear the partitioning for disabled planes. */
> +	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> +	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> +
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	if (!cstate->base.active) {
>  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> -		memset(ddb->plane[pipe], 0, sizeof(ddb-
> >plane[pipe]));
> -		memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> >y_plane[pipe]));
>  		return 0;
>  	}
>  
> @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state
> *state)
>  		intel_state->wm_results.dirty_pipes = ~0;
>  	}
>  
> +	/*
> +	 * We're not recomputing for the pipes not included in the
> commit, so
> +	 * make sure we start with the current state.
> +	 */
> +	memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
> +
>  	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
>  		struct intel_crtc_state *cstate;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Patchwork]

== Series Details ==

Series: drm/i915/gen9: fix DDB partitioning for multi-screen cases
URL   : https://patchwork.freedesktop.org/series/13293/
State : success

== Summary ==

Series 13293v1 drm/i915/gen9: fix DDB partitioning for multi-screen cases
https://patchwork.freedesktop.org/api/1.0/series/13293/revisions/1/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                dmesg-warn -> PASS       (fi-byt-n2820)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (fi-byt-j1900)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-byt-j1900)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:244  pass:214  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:244  pass:212  dwarn:0   dfail:0   fail:1   skip:31 
fi-byt-n2820     total:244  pass:208  dwarn:0   dfail:0   fail:1   skip:35 
fi-hsw-4770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6700k     total:244  pass:219  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:244  pass:228  dwarn:1   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:244  pass:208  dwarn:0   dfail:0   fail:0   skip:36 
fi-snb-2600      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_2625/

2e47fbed60d05480cfbd097c1912e6823c0310ee drm-intel-nightly: 2016y-10m-04d-14h-08m-17s UTC integration manifest
128e9a1 drm/i915/gen9: fix DDB partitioning for multi-screen cases

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

Re: [Intel-gfx] [PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Maarten Lankhorst]

Op 04-10-16 om 19:44 schreef Paulo Zanoni:
> Em Ter, 2016-10-04 às 14:37 -0300, Paulo Zanoni escreveu:
>> With the previous code we were only recomputing the DDB partitioning
>> for the CRTCs included in the atomic commit, so any other active
>> CRTCs
>> would end up having their DDB registers zeroed. In this patch we make
>> sure that the computed state starts as a copy of the current
>> partitioning, and then we only zero the DDBs that we're actually
>> going to recompute.
>>
>> How to reproduce the bug:
>>   1 - Enable the primary plane on pipe A
>>   2 - Enable the primary plane on pipe B
>>   3 - Enable the sprite plane on pipe A
> Forgot to mention: sprite or cursor.
Testcase: kms_cursor_legacy.cursorA-vs-flipB-atomic-transitions

Fails with:
(kms_cursor_legacy:1962) igt-kms-CRITICAL: Last errno: 22, Invalid argument
(kms_cursor_legacy:1962) igt-kms-CRITICAL: error: -22 != 0

The test succeeds with this patch applied, but it still has FIFO underruns. Oh well it's progress..

Re: [PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Maarten Lankhorst]

Op 04-10-16 om 19:44 schreef Paulo Zanoni:
> Em Ter, 2016-10-04 às 14:37 -0300, Paulo Zanoni escreveu:
>> With the previous code we were only recomputing the DDB partitioning
>> for the CRTCs included in the atomic commit, so any other active
>> CRTCs
>> would end up having their DDB registers zeroed. In this patch we make
>> sure that the computed state starts as a copy of the current
>> partitioning, and then we only zero the DDBs that we're actually
>> going to recompute.
>>
>> How to reproduce the bug:
>>   1 - Enable the primary plane on pipe A
>>   2 - Enable the primary plane on pipe B
>>   3 - Enable the sprite plane on pipe A
> Forgot to mention: sprite or cursor.
Testcase: kms_cursor_legacy.cursorA-vs-flipB-atomic-transitions

Fails with:
(kms_cursor_legacy:1962) igt-kms-CRITICAL: Last errno: 22, Invalid argument
(kms_cursor_legacy:1962) igt-kms-CRITICAL: error: -22 != 0

The test succeeds with this patch applied, but it still has FIFO underruns. Oh well it's progress..

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

Re: [Intel-gfx] [PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Lyude]

Sorry about how long this took! Anyway, finally got to testing it

Reviewed-by: Lyude <cpaul@redhat.com>

On Tue, 2016-10-04 at 14:37 -0300, Paulo Zanoni wrote:
> With the previous code we were only recomputing the DDB partitioning
> for the CRTCs included in the atomic commit, so any other active
> CRTCs
> would end up having their DDB registers zeroed. In this patch we make
> sure that the computed state starts as a copy of the current
> partitioning, and then we only zero the DDBs that we're actually
> going to recompute.
> 
> How to reproduce the bug:
>   1 - Enable the primary plane on pipe A
>   2 - Enable the primary plane on pipe B
>   3 - Enable the sprite plane on pipe A
> 
> Step 3 will zero the DDB partitioning for pipe B since it's not
> included in the commit that enabled the sprite for pipe A.
> 
> I expect this to fix many FIFO underrun problems on gen9+.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596
> Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-S
> kylake-Multi-Screen-Woes
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> I still have to confirm whether this closes the above bugs, but it
> certainly fixes the problem I described.
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 425544b..0c2e252 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	int num_active;
>  	int id, i;
>  
> +	/* Clear the partitioning for disabled planes. */
> +	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> +	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> +
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	if (!cstate->base.active) {
>  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> -		memset(ddb->plane[pipe], 0, sizeof(ddb-
> >plane[pipe]));
> -		memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> >y_plane[pipe]));
>  		return 0;
>  	}
>  
> @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state
> *state)
>  		intel_state->wm_results.dirty_pipes = ~0;
>  	}
>  
> +	/*
> +	 * We're not recomputing for the pipes not included in the
> commit, so
> +	 * make sure we start with the current state.
> +	 */
> +	memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
> +
>  	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
>  		struct intel_crtc_state *cstate;
>  
-- 
Cheers,
	Lyude

Re: [PATCH] drm/i915/gen9: fix DDB partitioning for multi-screen cases

[Lyude]

Sorry about how long this took! Anyway, finally got to testing it

Reviewed-by: Lyude <cpaul@redhat.com>

On Tue, 2016-10-04 at 14:37 -0300, Paulo Zanoni wrote:
> With the previous code we were only recomputing the DDB partitioning
> for the CRTCs included in the atomic commit, so any other active
> CRTCs
> would end up having their DDB registers zeroed. In this patch we make
> sure that the computed state starts as a copy of the current
> partitioning, and then we only zero the DDBs that we're actually
> going to recompute.
> 
> How to reproduce the bug:
>   1 - Enable the primary plane on pipe A
>   2 - Enable the primary plane on pipe B
>   3 - Enable the sprite plane on pipe A
> 
> Step 3 will zero the DDB partitioning for pipe B since it's not
> included in the commit that enabled the sprite for pipe A.
> 
> I expect this to fix many FIFO underrun problems on gen9+.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96226
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96828
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97450
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97596
> Bugzilla: https://www.phoronix.com/scan.php?page=news_item&px=Intel-S
> kylake-Multi-Screen-Woes
> Cc: stable@vger.kernel.org
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> I still have to confirm whether this closes the above bugs, but it
> certainly fixes the problem I described.
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 425544b..0c2e252 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3362,13 +3362,15 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	int num_active;
>  	int id, i;
>  
> +	/* Clear the partitioning for disabled planes. */
> +	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> +	memset(ddb->y_plane[pipe], 0, sizeof(ddb->y_plane[pipe]));
> +
>  	if (WARN_ON(!state))
>  		return 0;
>  
>  	if (!cstate->base.active) {
>  		ddb->pipe[pipe].start = ddb->pipe[pipe].end = 0;
> -		memset(ddb->plane[pipe], 0, sizeof(ddb-
> >plane[pipe]));
> -		memset(ddb->y_plane[pipe], 0, sizeof(ddb-
> >y_plane[pipe]));
>  		return 0;
>  	}
>  
> @@ -4054,6 +4056,12 @@ skl_compute_ddb(struct drm_atomic_state
> *state)
>  		intel_state->wm_results.dirty_pipes = ~0;
>  	}
>  
> +	/*
> +	 * We're not recomputing for the pipes not included in the
> commit, so
> +	 * make sure we start with the current state.
> +	 */
> +	memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
> +
>  	for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) {
>  		struct intel_crtc_state *cstate;
>  
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx