Re: [PATCH 4/8] drm/i915/skl+: Clean up minimum allocations.

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

Em Qua, 2016-10-12 às 15:28 +0200, Maarten Lankhorst escreveu:
> Move calculating minimum allocations to a helper, which cleans up the
> code some more. The cursor is still allocated in advance because it
> doesn't count towards data rate and should always be reserved.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 66 ++++++++++++++++++++++++-------
> ----------
>  1 file changed, 39 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 83c1b0acef38..45fb8275abea 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3342,6 +3342,32 @@ skl_ddb_min_alloc(const struct drm_plane_state
> *pstate,
>  	return DIV_ROUND_UP((4 * src_w * plane_bpp), 512) *
> min_scanlines/4 + 3;
>  }
>  
> +static void
> +skl_ddb_calc_min(const struct intel_crtc_state *cstate, int
> num_active,
> +		 uint16_t *minimum, uint16_t *y_minimum)
> +{
> +	const struct drm_plane_state *pstate;
> +	struct drm_plane *plane;
> +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> +
> +	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> &cstate->base) {
> +		struct intel_plane *intel_plane =
> to_intel_plane(plane);
> +		int id = skl_wm_plane_id(intel_plane);
> +
> +		if (intel_plane->pipe != pipe ||
> +		    id == PLANE_CURSOR)

You can also remove the check for pipe here.


> +			continue;
> +
> +		if (!pstate->visible)
> +			continue;
> +
> +		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> +		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> +	}
> +
> +	minimum[PLANE_CURSOR] = skl_cursor_allocation(num_active);
> +}
> +
>  static int
>  skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  		      struct skl_ddb_allocation *ddb /* out */)
> @@ -3350,12 +3376,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  	struct drm_crtc *crtc = cstate->base.crtc;
>  	struct drm_device *dev = crtc->dev;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	struct intel_plane *intel_plane;
> -	struct drm_plane *plane;
> -	const struct drm_plane_state *pstate;
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct skl_ddb_entry *alloc = &cstate->wm.skl.ddb;
> -	uint16_t alloc_size, start, cursor_blocks;
> +	uint16_t alloc_size, start;
>  	uint16_t minimum[I915_MAX_PLANES] = {};
>  	uint16_t y_minimum[I915_MAX_PLANES] = {};
>  	unsigned int total_data_rate;
> @@ -3384,35 +3407,21 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		return 0;
>  	}
>  
> -	cursor_blocks = skl_cursor_allocation(num_active);
> -	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> cursor_blocks;
> -	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> -
> -	alloc_size -= cursor_blocks;
> -
> -	/* 1. Allocate the mininum required blocks for each active
> plane */
> -	drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
> &cstate->base) {
> -		intel_plane = to_intel_plane(plane);
> -		id = skl_wm_plane_id(intel_plane);
> -
> -		if (intel_plane->pipe != pipe)
> -			continue;
> -
> -		if (!pstate->visible)
> -			continue;
> +	skl_ddb_calc_min(cstate, num_active, minimum, y_minimum);
>  
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -			continue;
> -
> -		minimum[id] = skl_ddb_min_alloc(pstate, 0);
> -		y_minimum[id] = skl_ddb_min_alloc(pstate, 1);
> -	}
> +	/* 1. Allocate the mininum required blocks for each active
> plane
> +	 * and allocate the cursor, it doesn't require extra
> allocation
> +	 * proportional to the data rate.
> +	 */
>  
> -	for (i = 0; i < PLANE_CURSOR; i++) {
> +	for (i = 0; i < I915_MAX_PLANES; i++) {

As I mentioned earlier, this is also an unsafe loop. I know you didn't
introduce it, so we can fix this in a next patch.

With the pipe check removed (and Matt's requests addressed):
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>  		alloc_size -= minimum[i];
>  		alloc_size -= y_minimum[i];
>  	}
>  
> +	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end -
> minimum[PLANE_CURSOR];
> +	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
> +
>  	/*
>  	 * 2. Distribute the remaining space in proportion to the
> amount of
>  	 * data each plane needs to fetch from memory.
> @@ -3428,6 +3437,9 @@ skl_allocate_pipe_ddb(struct intel_crtc_state
> *cstate,
>  		unsigned rate;
>  		uint16_t plane_blocks, y_plane_blocks = 0;
>  
> +		if (id == PLANE_CURSOR)
> +			continue;
> +
>  		rate = data_rate[id];
>  
>  		/*
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx