| Message ID | 1467070964-14864-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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
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; /*
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(-)