[Intel-gfx] [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.
Paulo Zanoni
paulo.r.zanoni at intel.com
Thu Oct 20 17:18:08 UTC 2016
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 at 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 at 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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list