Re: [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx