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

From: Matt Roper <matthew.d.roper@intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/8] drm/i915/skl+: Remove data_rate from watermark struct.
Date: Wed, 19 Oct 2016 15:13:38 -0700	[thread overview]
Message-ID: <20161019221338.GX19156@intel.com> (raw)
In-Reply-To: <1476278901-15750-3-git-send-email-maarten.lankhorst@linux.intel.com>

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

-- 
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