Re: [PATCH v2 04/11] drm/i915/skl+: Remove data_rate from watermark struct, v2.

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

Em Qua, 2016-10-26 às 15:41 +0200, Maarten Lankhorst escreveu:
> 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.
> 
> There are loops over all planes, including planes that don't exist.
> This is harmless, because data_rate will always be 0 for them and we
> never program them when updating watermarks.
> 
> Changes since v1:
> - Rename rate back to data_rate, and change array name to
>   plane_data_rate. (Matt)
> - Remove whitespace. (Paulo)
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

I'd still prefer if you had used a safer way to loop through the
planes, not something that may just break when someone decides refactor
this code for the Nth time in the future and forget that it only works
because we zero-initialize things and then rely on multiplication by
zero. Since I'm not going to be able to convince you, and the code is
correct today:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_drv.h |  4 ----
>  drivers/gpu/drm/i915/intel_pm.c  | 35 ++++++++++++++++------------
> -------
>  2 files changed, 16 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 7dda79df55d0..d0485457b335 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 c1520afb2360..0c199c51dad9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3263,13 +3263,12 @@ 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;
>  	const struct drm_plane_state *pstate;
> @@ -3287,21 +3286,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;
> -	}
> +		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);
> -
> -		/* 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;
> @@ -3389,6 +3383,8 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	unsigned int total_data_rate;
>  	int num_active;
>  	int id, i;
> +	unsigned plane_data_rate[I915_MAX_PLANES] = {};
> +	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
>  
>  	/* Clear the partitioning for disabled planes. */
>  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3446,17 +3442,18 @@ 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,
> +							   plane_dat
> a_rate,
> +							   plane_y_d
> ata_rate);
>  	if (total_data_rate == 0)
>  		return 0;
>  
>  	start = alloc->start;
> -	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> +	for (id = 0; id < I915_MAX_PLANES; id++) {
>  		unsigned int data_rate, y_data_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];
> +		data_rate = plane_data_rate[id];
>  
>  		/*
>  		 * allocation for (packed formats) or (uv-plane part
> of planar format):
> @@ -3478,7 +3475,7 @@ 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];
> +		y_data_rate = plane_y_data_rate[id];
>  
>  		y_plane_blocks = y_minimum[id];
>  		y_plane_blocks += div_u64((uint64_t)alloc_size *
> y_data_rate,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx