| Message ID | 1476278901-15750-3-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote: > It's only used in one function, and can be calculated without caching it > in the global struct by using drm_atomic_crtc_state_for_each_plane_state. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 4 ---- > drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++--------------------- > 2 files changed, 21 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index bb468c974e14..888054518f3c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -502,10 +502,6 @@ struct intel_crtc_wm_state { > struct skl_pipe_wm optimal; > struct skl_ddb_entry ddb; > > - /* cached plane data rate */ > - unsigned plane_data_rate[I915_MAX_PLANES]; > - unsigned plane_y_data_rate[I915_MAX_PLANES]; > - > /* minimum block allocation */ > uint16_t minimum_blocks[I915_MAX_PLANES]; > uint16_t minimum_y_blocks[I915_MAX_PLANES]; > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index b96a899c899d..97b6202c4097 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, > * 3 * 4096 * 8192 * 4 < 2^32 > */ > static unsigned int > -skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) > +skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate, > + unsigned *plane_data_rate, > + unsigned *plane_y_data_rate) > { > struct drm_crtc_state *cstate = &intel_cstate->base; > struct drm_atomic_state *state = cstate->state; > struct drm_crtc *crtc = cstate->crtc; > - struct drm_device *dev = crtc->dev; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct drm_plane *plane; > const struct intel_plane *intel_plane; > @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) > /* packed/uv */ > rate = skl_plane_relative_data_rate(intel_cstate, > pstate, 0); > - intel_cstate->wm.skl.plane_data_rate[id] = rate; > + plane_data_rate[id] = rate; > + > + total_data_rate += rate; > > /* y-plane */ > rate = skl_plane_relative_data_rate(intel_cstate, > pstate, 1); > - intel_cstate->wm.skl.plane_y_data_rate[id] = rate; > - } > - > - /* Calculate CRTC's total data rate from cached values */ > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > - int id = skl_wm_plane_id(intel_plane); > + plane_y_data_rate[id] = rate; > > - /* packed/uv */ > - total_data_rate += intel_cstate->wm.skl.plane_data_rate[id]; > - total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id]; > + total_data_rate += rate; > } > > return total_data_rate; > @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > int num_active; > int id, i; > > + unsigned data_rate[I915_MAX_PLANES] = {}; > + unsigned y_data_rate[I915_MAX_PLANES] = {}; > + Minor nitpick; if you picked a different names here (e.g., plane_data_rate[]) then you could leave the local variables farther down named 'data_rate' and 'y_data_rate' which would reduce the diff changes and result in a slightly smaller patch. Whether or not you feel like making that change, killing the caching is good so, Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > /* 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])); > @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > * > * FIXME: we may not allocate every single block here. > */ > - total_data_rate = skl_get_total_relative_data_rate(cstate); > + total_data_rate = skl_get_total_relative_data_rate(cstate, data_rate, y_data_rate); > if (total_data_rate == 0) > return 0; > > start = alloc->start; > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > - unsigned int data_rate, y_data_rate; > + for (id = 0; id < I915_MAX_PLANES; id++) { > + unsigned rate; > uint16_t plane_blocks, y_plane_blocks = 0; > - int id = skl_wm_plane_id(intel_plane); > > - data_rate = cstate->wm.skl.plane_data_rate[id]; > + rate = data_rate[id]; > > /* > * allocation for (packed formats) or (uv-plane part of planar format): > * promote the expression to 64 bits to avoid overflowing, the > - * result is < available as data_rate / total_data_rate < 1 > + * result is < available as rate / total_data_rate < 1 > */ > plane_blocks = minimum[id]; > - plane_blocks += div_u64((uint64_t)alloc_size * data_rate, > + plane_blocks += div_u64((uint64_t)alloc_size * rate, > total_data_rate); > > /* Leave disabled planes at (0,0) */ > - if (data_rate) { > + if (rate) { > ddb->plane[pipe][id].start = start; > ddb->plane[pipe][id].end = start + plane_blocks; > } > @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, > /* > * allocation for y_plane part of planar format: > */ > - y_data_rate = cstate->wm.skl.plane_y_data_rate[id]; > + rate = y_data_rate[id]; > > y_plane_blocks = y_minimum[id]; > - y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate, > + y_plane_blocks += div_u64((uint64_t)alloc_size * rate, > total_data_rate); > > - if (y_data_rate) { > + if (rate) { > ddb->y_plane[pipe][id].start = start; > ddb->y_plane[pipe][id].end = start + y_plane_blocks; > } > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu: > On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote: > > > > It's only used in one function, and can be calculated without > > caching it > > in the global struct by using > > drm_atomic_crtc_state_for_each_plane_state. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com > > > > > --- > > drivers/gpu/drm/i915/intel_drv.h | 4 ---- > > drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++--------- > > ------------ > > 2 files changed, 21 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index bb468c974e14..888054518f3c 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -502,10 +502,6 @@ struct intel_crtc_wm_state { > > struct skl_pipe_wm optimal; > > struct skl_ddb_entry ddb; > > > > - /* cached plane data rate */ > > - unsigned plane_data_rate[I915_MAX_PLANES]; > > - unsigned > > plane_y_data_rate[I915_MAX_PLANES]; > > - > > /* minimum block allocation */ > > uint16_t minimum_blocks[I915_MAX_PLANES]; > > uint16_t > > minimum_y_blocks[I915_MAX_PLANES]; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > b/drivers/gpu/drm/i915/intel_pm.c > > index b96a899c899d..97b6202c4097 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct > > intel_crtc_state *cstate, > > * 3 * 4096 * 8192 * 4 < 2^32 > > */ > > static unsigned int > > -skl_get_total_relative_data_rate(struct intel_crtc_state > > *intel_cstate) > > +skl_get_total_relative_data_rate(struct intel_crtc_state > > *intel_cstate, > > + unsigned *plane_data_rate, > > + unsigned *plane_y_data_rate) > > { > > struct drm_crtc_state *cstate = &intel_cstate->base; > > struct drm_atomic_state *state = cstate->state; > > struct drm_crtc *crtc = cstate->crtc; > > - struct drm_device *dev = crtc->dev; > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > struct drm_plane *plane; > > const struct intel_plane *intel_plane; > > @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct > > intel_crtc_state *intel_cstate) > > /* packed/uv */ > > rate = skl_plane_relative_data_rate(intel_cstate, > > pstate, 0); > > - intel_cstate->wm.skl.plane_data_rate[id] = rate; > > + plane_data_rate[id] = rate; > > + > > + total_data_rate += rate; > > > > /* y-plane */ > > rate = skl_plane_relative_data_rate(intel_cstate, > > pstate, 1); > > - intel_cstate->wm.skl.plane_y_data_rate[id] = rate; > > - } > > - > > - /* Calculate CRTC's total data rate from cached values */ > > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) > > { > > - int id = skl_wm_plane_id(intel_plane); > > + plane_y_data_rate[id] = rate; > > > > - /* packed/uv */ > > - total_data_rate += intel_cstate- > > >wm.skl.plane_data_rate[id]; > > - total_data_rate += intel_cstate- > > >wm.skl.plane_y_data_rate[id]; > > + total_data_rate += rate; > > } > > > > return total_data_rate; > > @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state > > *cstate, > > int num_active; > > int id, i; > > > > + unsigned data_rate[I915_MAX_PLANES] = {}; > > + unsigned y_data_rate[I915_MAX_PLANES] = {}; > > + > > Minor nitpick; if you picked a different names here (e.g., > plane_data_rate[]) then you could leave the local variables farther > down > named 'data_rate' and 'y_data_rate' which would reduce the diff > changes > and result in a slightly smaller patch. > > Whether or not you feel like making that change, killing the caching > is > good so, > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > /* 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])); > > @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct > > intel_crtc_state *cstate, > > * > > * FIXME: we may not allocate every single block here. > > */ > > - total_data_rate = > > skl_get_total_relative_data_rate(cstate); > > + total_data_rate = skl_get_total_relative_data_rate(cstate, > > data_rate, y_data_rate); > > if (total_data_rate == 0) > > return 0; > > > > start = alloc->start; > > - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) > > { > > - unsigned int data_rate, y_data_rate; > > + for (id = 0; id < I915_MAX_PLANES; id++) { Can we please use a different kind of iteration? Although this is correct today, history shows that the number of planes increases over time and the code may suddenly break when if we ever introduce PLANE_D. Perhaps: for_each_intel_plane_on_crtc(...) { id = skl_wm_plane_id(intel_plane); ... } With that fixed (and, in case you want, Matt's suggestion): Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > + unsigned rate; > > uint16_t plane_blocks, y_plane_blocks = 0; > > - int id = skl_wm_plane_id(intel_plane); > > > > - data_rate = cstate->wm.skl.plane_data_rate[id]; > > + rate = data_rate[id]; > > > > /* > > * allocation for (packed formats) or (uv-plane > > part of planar format): > > * promote the expression to 64 bits to avoid > > overflowing, the > > - * result is < available as data_rate / > > total_data_rate < 1 > > + * result is < available as rate / total_data_rate > > < 1 > > */ > > plane_blocks = minimum[id]; > > - plane_blocks += div_u64((uint64_t)alloc_size * > > data_rate, > > + plane_blocks += div_u64((uint64_t)alloc_size * > > rate, > > total_data_rate); > > > > /* Leave disabled planes at (0,0) */ > > - if (data_rate) { > > + if (rate) { > > ddb->plane[pipe][id].start = start; > > ddb->plane[pipe][id].end = start + > > plane_blocks; > > } > > @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct > > intel_crtc_state *cstate, > > /* > > * allocation for y_plane part of planar format: > > */ > > - y_data_rate = cstate- > > >wm.skl.plane_y_data_rate[id]; > > + rate = y_data_rate[id]; > > > > y_plane_blocks = y_minimum[id]; > > - y_plane_blocks += div_u64((uint64_t)alloc_size * > > y_data_rate, > > + y_plane_blocks += div_u64((uint64_t)alloc_size * > > rate, > > total_data_rate); > > > > - if (y_data_rate) { > > + if (rate) { > > ddb->y_plane[pipe][id].start = start; > > ddb->y_plane[pipe][id].end = start + > > y_plane_blocks; > > } > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Em Qui, 2016-10-20 às 15:18 -0200, Paulo Zanoni escreveu: > Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu: > > > > On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote: > > > > > > > > > It's only used in one function, and can be calculated without > > > caching it > > > in the global struct by using > > > drm_atomic_crtc_state_for_each_plane_state. > > > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.c > > > om > > > > > > > > > > > --- > > > drivers/gpu/drm/i915/intel_drv.h | 4 ---- > > > drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++------- > > > -- > > > ------------ > > > 2 files changed, 21 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index bb468c974e14..888054518f3c 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -502,10 +502,6 @@ struct intel_crtc_wm_state { > > > struct skl_pipe_wm optimal; > > > struct skl_ddb_entry ddb; > > > > > > - /* cached plane data rate */ > > > - unsigned > > > plane_data_rate[I915_MAX_PLANES]; > > > - unsigned > > > plane_y_data_rate[I915_MAX_PLANES]; > > > - > > > /* minimum block allocation */ > > > uint16_t > > > minimum_blocks[I915_MAX_PLANES]; > > > uint16_t > > > minimum_y_blocks[I915_MAX_PLANES]; > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > > > b/drivers/gpu/drm/i915/intel_pm.c > > > index b96a899c899d..97b6202c4097 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct > > > intel_crtc_state *cstate, > > > * 3 * 4096 * 8192 * 4 < 2^32 > > > */ > > > static unsigned int > > > -skl_get_total_relative_data_rate(struct intel_crtc_state > > > *intel_cstate) > > > +skl_get_total_relative_data_rate(struct intel_crtc_state > > > *intel_cstate, > > > + unsigned *plane_data_rate, > > > + unsigned *plane_y_data_rate) > > > { > > > struct drm_crtc_state *cstate = &intel_cstate->base; > > > struct drm_atomic_state *state = cstate->state; > > > struct drm_crtc *crtc = cstate->crtc; > > > - struct drm_device *dev = crtc->dev; > > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > struct drm_plane *plane; > > > const struct intel_plane *intel_plane; > > > @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct > > > intel_crtc_state *intel_cstate) > > > /* packed/uv */ > > > rate = > > > skl_plane_relative_data_rate(intel_cstate, > > > pstate, 0); > > > - intel_cstate->wm.skl.plane_data_rate[id] = rate; > > > + plane_data_rate[id] = rate; > > > + > > > + total_data_rate += rate; > > > > > > /* y-plane */ > > > rate = > > > skl_plane_relative_data_rate(intel_cstate, > > > pstate, 1); > > > - intel_cstate->wm.skl.plane_y_data_rate[id] = > > > rate; > > > - } > > > - > > > - /* Calculate CRTC's total data rate from cached values > > > */ > > > - for_each_intel_plane_on_crtc(dev, intel_crtc, > > > intel_plane) > > > { > > > - int id = skl_wm_plane_id(intel_plane); > > > + plane_y_data_rate[id] = rate; > > > > > > - /* packed/uv */ > > > - total_data_rate += intel_cstate- > > > > > > > > wm.skl.plane_data_rate[id]; > > > - total_data_rate += intel_cstate- > > > > > > > > wm.skl.plane_y_data_rate[id]; > > > + total_data_rate += rate; > > > } > > > > > > return total_data_rate; > > > @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct > > > intel_crtc_state > > > *cstate, > > > int num_active; > > > int id, i; > > > Also obligatory bikeshed to remove the ugly blank line above :) > > > + unsigned data_rate[I915_MAX_PLANES] = {}; > > > + unsigned y_data_rate[I915_MAX_PLANES] = {}; > > > + > > > > Minor nitpick; if you picked a different names here (e.g., > > plane_data_rate[]) then you could leave the local variables farther > > down > > named 'data_rate' and 'y_data_rate' which would reduce the diff > > changes > > and result in a slightly smaller patch. > > > > Whether or not you feel like making that change, killing the > > caching > > is > > good so, > > > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > > > > > > > > > /* 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])); > > > @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct > > > intel_crtc_state *cstate, > > > * > > > * FIXME: we may not allocate every single block here. > > > */ > > > - total_data_rate = > > > skl_get_total_relative_data_rate(cstate); > > > + total_data_rate = > > > skl_get_total_relative_data_rate(cstate, > > > data_rate, y_data_rate); > > > if (total_data_rate == 0) > > > return 0; > > > > > > start = alloc->start; > > > - for_each_intel_plane_on_crtc(dev, intel_crtc, > > > intel_plane) > > > { > > > - unsigned int data_rate, y_data_rate; > > > + for (id = 0; id < I915_MAX_PLANES; id++) { > > Can we please use a different kind of iteration? Although this is > correct today, history shows that the number of planes increases over > time and the code may suddenly break when if we ever introduce > PLANE_D. > > Perhaps: > for_each_intel_plane_on_crtc(...) { > id = skl_wm_plane_id(intel_plane); > ... > } > > With that fixed (and, in case you want, Matt's suggestion): > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > > > + unsigned rate; > > > uint16_t plane_blocks, y_plane_blocks = 0; > > > - int id = skl_wm_plane_id(intel_plane); > > > > > > - data_rate = cstate->wm.skl.plane_data_rate[id]; > > > + rate = data_rate[id]; > > > > > > /* > > > * allocation for (packed formats) or (uv-plane > > > part of planar format): > > > * promote the expression to 64 bits to avoid > > > overflowing, the > > > - * result is < available as data_rate / > > > total_data_rate < 1 > > > + * result is < available as rate / > > > total_data_rate > > > < 1 > > > */ > > > plane_blocks = minimum[id]; > > > - plane_blocks += div_u64((uint64_t)alloc_size * > > > data_rate, > > > + plane_blocks += div_u64((uint64_t)alloc_size * > > > rate, > > > total_data_rate); > > > > > > /* Leave disabled planes at (0,0) */ > > > - if (data_rate) { > > > + if (rate) { > > > ddb->plane[pipe][id].start = start; > > > ddb->plane[pipe][id].end = start + > > > plane_blocks; > > > } > > > @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct > > > intel_crtc_state *cstate, > > > /* > > > * allocation for y_plane part of planar format: > > > */ > > > - y_data_rate = cstate- > > > > > > > > wm.skl.plane_y_data_rate[id]; > > > + rate = y_data_rate[id]; > > > > > > y_plane_blocks = y_minimum[id]; > > > - y_plane_blocks += div_u64((uint64_t)alloc_size * > > > y_data_rate, > > > + y_plane_blocks += div_u64((uint64_t)alloc_size * > > > rate, > > > total_data_rate); > > > > > > - if (y_data_rate) { > > > + if (rate) { > > > ddb->y_plane[pipe][id].start = start; > > > ddb->y_plane[pipe][id].end = start + > > > y_plane_blocks; > > > } > > > -- > > > 2.7.4 > > > > > > _______________________________________________ > > > 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
Op 20-10-16 om 19:18 schreef Paulo Zanoni: > Em Qua, 2016-10-19 às 15:13 -0700, Matt Roper escreveu: >> On Wed, Oct 12, 2016 at 03:28:15PM +0200, Maarten Lankhorst wrote: >>> It's only used in one function, and can be calculated without >>> caching it >>> in the global struct by using >>> drm_atomic_crtc_state_for_each_plane_state. >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com >>> --- >>> drivers/gpu/drm/i915/intel_drv.h | 4 ---- >>> drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++--------- >>> ------------ >>> 2 files changed, 21 insertions(+), 27 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>> b/drivers/gpu/drm/i915/intel_drv.h >>> index bb468c974e14..888054518f3c 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -502,10 +502,6 @@ struct intel_crtc_wm_state { >>> struct skl_pipe_wm optimal; >>> struct skl_ddb_entry ddb; >>> >>> - /* cached plane data rate */ >>> - unsigned plane_data_rate[I915_MAX_PLANES]; >>> - unsigned >>> plane_y_data_rate[I915_MAX_PLANES]; >>> - >>> /* minimum block allocation */ >>> uint16_t minimum_blocks[I915_MAX_PLANES]; >>> uint16_t >>> minimum_y_blocks[I915_MAX_PLANES]; >>> diff --git a/drivers/gpu/drm/i915/intel_pm.c >>> b/drivers/gpu/drm/i915/intel_pm.c >>> index b96a899c899d..97b6202c4097 100644 >>> --- a/drivers/gpu/drm/i915/intel_pm.c >>> +++ b/drivers/gpu/drm/i915/intel_pm.c >>> @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct >>> intel_crtc_state *cstate, >>> * 3 * 4096 * 8192 * 4 < 2^32 >>> */ >>> static unsigned int >>> -skl_get_total_relative_data_rate(struct intel_crtc_state >>> *intel_cstate) >>> +skl_get_total_relative_data_rate(struct intel_crtc_state >>> *intel_cstate, >>> + unsigned *plane_data_rate, >>> + unsigned *plane_y_data_rate) >>> { >>> struct drm_crtc_state *cstate = &intel_cstate->base; >>> struct drm_atomic_state *state = cstate->state; >>> struct drm_crtc *crtc = cstate->crtc; >>> - struct drm_device *dev = crtc->dev; >>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>> struct drm_plane *plane; >>> const struct intel_plane *intel_plane; >>> @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct >>> intel_crtc_state *intel_cstate) >>> /* packed/uv */ >>> rate = skl_plane_relative_data_rate(intel_cstate, >>> pstate, 0); >>> - intel_cstate->wm.skl.plane_data_rate[id] = rate; >>> + plane_data_rate[id] = rate; >>> + >>> + total_data_rate += rate; >>> >>> /* y-plane */ >>> rate = skl_plane_relative_data_rate(intel_cstate, >>> pstate, 1); >>> - intel_cstate->wm.skl.plane_y_data_rate[id] = rate; >>> - } >>> - >>> - /* Calculate CRTC's total data rate from cached values */ >>> - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) >>> { >>> - int id = skl_wm_plane_id(intel_plane); >>> + plane_y_data_rate[id] = rate; >>> >>> - /* packed/uv */ >>> - total_data_rate += intel_cstate- >>>> wm.skl.plane_data_rate[id]; >>> - total_data_rate += intel_cstate- >>>> wm.skl.plane_y_data_rate[id]; >>> + total_data_rate += rate; >>> } >>> >>> return total_data_rate; >>> @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state >>> *cstate, >>> int num_active; >>> int id, i; >>> >>> + unsigned data_rate[I915_MAX_PLANES] = {}; >>> + unsigned y_data_rate[I915_MAX_PLANES] = {}; >>> + >> Minor nitpick; if you picked a different names here (e.g., >> plane_data_rate[]) then you could leave the local variables farther >> down >> named 'data_rate' and 'y_data_rate' which would reduce the diff >> changes >> and result in a slightly smaller patch. >> >> Whether or not you feel like making that change, killing the caching >> is >> good so, >> >> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >> >> >>> /* 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])); >>> @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct >>> intel_crtc_state *cstate, >>> * >>> * FIXME: we may not allocate every single block here. >>> */ >>> - total_data_rate = >>> skl_get_total_relative_data_rate(cstate); >>> + total_data_rate = skl_get_total_relative_data_rate(cstate, >>> data_rate, y_data_rate); >>> if (total_data_rate == 0) >>> return 0; >>> >>> start = alloc->start; >>> - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) >>> { >>> - unsigned int data_rate, y_data_rate; >>> + for (id = 0; id < I915_MAX_PLANES; id++) { > Can we please use a different kind of iteration? Although this is > correct today, history shows that the number of planes increases over > time and the code may suddenly break when if we ever introduce PLANE_D. > > Perhaps: > for_each_intel_plane_on_crtc(...) { > id = skl_wm_plane_id(intel_plane); > ... > } > > With that fixed (and, in case you want, Matt's suggestion): > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Why would that break with PLANE_D? in that case rate = 0 and nothing happens. The hooks for it will never be called anyway, since it only updates up to max_planes. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index bb468c974e14..888054518f3c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -502,10 +502,6 @@ struct intel_crtc_wm_state { struct skl_pipe_wm optimal; struct skl_ddb_entry ddb; - /* cached plane data rate */ - unsigned plane_data_rate[I915_MAX_PLANES]; - unsigned plane_y_data_rate[I915_MAX_PLANES]; - /* minimum block allocation */ uint16_t minimum_blocks[I915_MAX_PLANES]; uint16_t minimum_y_blocks[I915_MAX_PLANES]; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index b96a899c899d..97b6202c4097 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3236,12 +3236,13 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, * 3 * 4096 * 8192 * 4 < 2^32 */ static unsigned int -skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) +skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate, + unsigned *plane_data_rate, + unsigned *plane_y_data_rate) { struct drm_crtc_state *cstate = &intel_cstate->base; struct drm_atomic_state *state = cstate->state; struct drm_crtc *crtc = cstate->crtc; - struct drm_device *dev = crtc->dev; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_plane *plane; const struct intel_plane *intel_plane; @@ -3263,21 +3264,16 @@ skl_get_total_relative_data_rate(struct intel_crtc_state *intel_cstate) /* packed/uv */ rate = skl_plane_relative_data_rate(intel_cstate, pstate, 0); - intel_cstate->wm.skl.plane_data_rate[id] = rate; + plane_data_rate[id] = rate; + + total_data_rate += rate; /* y-plane */ rate = skl_plane_relative_data_rate(intel_cstate, pstate, 1); - intel_cstate->wm.skl.plane_y_data_rate[id] = rate; - } - - /* Calculate CRTC's total data rate from cached values */ - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { - int id = skl_wm_plane_id(intel_plane); + plane_y_data_rate[id] = rate; - /* packed/uv */ - total_data_rate += intel_cstate->wm.skl.plane_data_rate[id]; - total_data_rate += intel_cstate->wm.skl.plane_y_data_rate[id]; + total_data_rate += rate; } return total_data_rate; @@ -3366,6 +3362,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, int num_active; int id, i; + unsigned data_rate[I915_MAX_PLANES] = {}; + unsigned y_data_rate[I915_MAX_PLANES] = {}; + /* 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])); @@ -3425,29 +3424,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, * * FIXME: we may not allocate every single block here. */ - total_data_rate = skl_get_total_relative_data_rate(cstate); + total_data_rate = skl_get_total_relative_data_rate(cstate, data_rate, y_data_rate); if (total_data_rate == 0) return 0; start = alloc->start; - for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { - unsigned int data_rate, y_data_rate; + for (id = 0; id < I915_MAX_PLANES; id++) { + unsigned rate; uint16_t plane_blocks, y_plane_blocks = 0; - int id = skl_wm_plane_id(intel_plane); - data_rate = cstate->wm.skl.plane_data_rate[id]; + rate = data_rate[id]; /* * allocation for (packed formats) or (uv-plane part of planar format): * promote the expression to 64 bits to avoid overflowing, the - * result is < available as data_rate / total_data_rate < 1 + * result is < available as rate / total_data_rate < 1 */ plane_blocks = minimum[id]; - plane_blocks += div_u64((uint64_t)alloc_size * data_rate, + plane_blocks += div_u64((uint64_t)alloc_size * rate, total_data_rate); /* Leave disabled planes at (0,0) */ - if (data_rate) { + if (rate) { ddb->plane[pipe][id].start = start; ddb->plane[pipe][id].end = start + plane_blocks; } @@ -3457,13 +3455,13 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate, /* * allocation for y_plane part of planar format: */ - y_data_rate = cstate->wm.skl.plane_y_data_rate[id]; + rate = y_data_rate[id]; y_plane_blocks = y_minimum[id]; - y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate, + y_plane_blocks += div_u64((uint64_t)alloc_size * rate, total_data_rate); - if (y_data_rate) { + if (rate) { ddb->y_plane[pipe][id].start = start; ddb->y_plane[pipe][id].end = start + y_plane_blocks; }
It's only used in one function, and can be calculated without caching it in the global struct by using drm_atomic_crtc_state_for_each_plane_state. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 4 ---- drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++++++++++++--------------------- 2 files changed, 21 insertions(+), 27 deletions(-)