[PATCH] drm/i915/gen9: Re-allocate DDB only for changed pipes

[PATCH] drm/i915/gen9: Re-allocate DDB only for changed pipes

[Matt Roper]

When a display update triggers a DDB re-allocation, we should start by
assuming that only the updated pipes need to be re-allocated (we have
logic later that may add additional pipes if, e.g., a modeset triggers a
change to the global allocation).

We were erroneously using the _active_ pipes as our starting point
rather than the changed pipes.  This causes us to grab CRTC locks that
we didn't actually need, reducing parallelism.  Given the recent
non-blocking atomic changes, it also causes legacy pageflips against one
CRTC to return -EBUSY if there's an outstanding pageflip against a
different CRTC (a situation easily triggered via compositors like
Weston).

Fixes: 98d39494d3 ("drm/i915/gen9: Compute DDB allocation at atomic check time (v4)")
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c94521cc..d7f8ba8 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3879,6 +3879,19 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
 	return 0;
 }
 
+static uint32_t
+pipes_modified(struct drm_atomic_state *state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *cstate;
+	uint32_t i, ret = 0;
+
+	for_each_crtc_in_state(state, crtc, cstate, i)
+		ret |= drm_crtc_mask(crtc);
+
+	return ret;
+}
+
 static int
 skl_compute_ddb(struct drm_atomic_state *state)
 {
@@ -3887,7 +3900,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
 	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct intel_crtc *intel_crtc;
 	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
-	unsigned realloc_pipes = dev_priv->active_crtcs;
+	uint32_t realloc_pipes = pipes_modified(state);
 	int ret;
 
 	/*
-- 
2.1.4

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

✓ Ro.CI.BAT: success for drm/i915/gen9: Re-allocate DDB only for changed pipes

[Patchwork]

== Series Details ==

Series: drm/i915/gen9: Re-allocate DDB only for changed pipes
URL   : https://patchwork.freedesktop.org/series/9211/
State : success

== Summary ==

Series 9211v1 drm/i915/gen9: Re-allocate DDB only for changed pipes
http://patchwork.freedesktop.org/api/1.0/series/9211/revisions/1/mbox

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-kbl-qkkr      total:229  pass:161  dwarn:29  dfail:0   fail:0   skip:39 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:3   dfail:1   fail:2   skip:21 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1316/

13bc84e drm-intel-nightly: 2016y-06m-27d-21h-27m-04s UTC integration manifest
8c121b7 drm/i915/gen9: Re-allocate DDB only for changed pipes

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

Re: [PATCH] drm/i915/gen9: Re-allocate DDB only for changed pipes

[Maarten Lankhorst]

Op 28-06-16 om 01:42 schreef Matt Roper:
> When a display update triggers a DDB re-allocation, we should start by
> assuming that only the updated pipes need to be re-allocated (we have
> logic later that may add additional pipes if, e.g., a modeset triggers a
> change to the global allocation).
>
> We were erroneously using the _active_ pipes as our starting point
> rather than the changed pipes.  This causes us to grab CRTC locks that
> we didn't actually need, reducing parallelism.  Given the recent
> non-blocking atomic changes, it also causes legacy pageflips against one
> CRTC to return -EBUSY if there's an outstanding pageflip against a
> different CRTC (a situation easily triggered via compositors like
> Weston).
>
> Fixes: 98d39494d3 ("drm/i915/gen9: Compute DDB allocation at atomic check time (v4)")
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c94521cc..d7f8ba8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3879,6 +3879,19 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
>  	return 0;
>  }
>  
> +static uint32_t
> +pipes_modified(struct drm_atomic_state *state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
> +	uint32_t i, ret = 0;
> +
> +	for_each_crtc_in_state(state, crtc, cstate, i)
> +		ret |= drm_crtc_mask(crtc);
> +
> +	return ret;
> +}
> +
>  static int
>  skl_compute_ddb(struct drm_atomic_state *state)
>  {
> @@ -3887,7 +3900,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
>  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct intel_crtc *intel_crtc;
>  	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
> -	unsigned realloc_pipes = dev_priv->active_crtcs;
> +	uint32_t realloc_pipes = pipes_modified(state);
>  	int ret;
>  
>  	/*

Hey,

Thanks, this is the original bug that triggered the need for "[PATCH] drm/atomic: Make drm_atomic_legacy_backoff reset crtc->acquire_ctx".
Fixing this should remove unnecessary stalls in atomic commit. :-)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>


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

Re: [PATCH] drm/i915/gen9: Re-allocate DDB only for changed pipes

[Matt Roper]

On Tue, Jun 28, 2016 at 10:50:20AM +0200, Maarten Lankhorst wrote:
> Op 28-06-16 om 01:42 schreef Matt Roper:
> > When a display update triggers a DDB re-allocation, we should start by
> > assuming that only the updated pipes need to be re-allocated (we have
> > logic later that may add additional pipes if, e.g., a modeset triggers a
> > change to the global allocation).
> >
> > We were erroneously using the _active_ pipes as our starting point
> > rather than the changed pipes.  This causes us to grab CRTC locks that
> > we didn't actually need, reducing parallelism.  Given the recent
> > non-blocking atomic changes, it also causes legacy pageflips against one
> > CRTC to return -EBUSY if there's an outstanding pageflip against a
> > different CRTC (a situation easily triggered via compositors like
> > Weston).
> >
> > Fixes: 98d39494d3 ("drm/i915/gen9: Compute DDB allocation at atomic check time (v4)")
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index c94521cc..d7f8ba8 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3879,6 +3879,19 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> >  	return 0;
> >  }
> >  
> > +static uint32_t
> > +pipes_modified(struct drm_atomic_state *state)
> > +{
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *cstate;
> > +	uint32_t i, ret = 0;
> > +
> > +	for_each_crtc_in_state(state, crtc, cstate, i)
> > +		ret |= drm_crtc_mask(crtc);
> > +
> > +	return ret;
> > +}
> > +
> >  static int
> >  skl_compute_ddb(struct drm_atomic_state *state)
> >  {
> > @@ -3887,7 +3900,7 @@ skl_compute_ddb(struct drm_atomic_state *state)
> >  	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >  	struct intel_crtc *intel_crtc;
> >  	struct skl_ddb_allocation *ddb = &intel_state->wm_results.ddb;
> > -	unsigned realloc_pipes = dev_priv->active_crtcs;
> > +	uint32_t realloc_pipes = pipes_modified(state);
> >  	int ret;
> >  
> >  	/*
> 
> Hey,
> 
> Thanks, this is the original bug that triggered the need for "[PATCH] drm/atomic: Make drm_atomic_legacy_backoff reset crtc->acquire_ctx".
> Fixing this should remove unnecessary stalls in atomic commit. :-)
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 

Merged to dinq; thanks for the review.


Matt

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx