Re: [PATCH 48/89] drm/i915/skl: Allocate DDB portions for display planes

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 04, 2014 at 12:27:14PM +0100, Damien Lespiau wrote:
> v2: Fix the 3rd plane/cursor logic (Pradeep Bhat)
> v3: Fix one-by-one error in the DDB allocation code
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 150 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4ee90b6..0ddcbad 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2937,6 +2937,155 @@ static bool ilk_disable_lp_wm(struct drm_device *dev)
>  	return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL);
>  }
>  
> +/*
> + * On gen9, we need to allocate Display Data Buffer (DDB) portions to the
> + * different active planes.
> + */
> +
> +#define SKL_DDB_SIZE		896	/* in blocks */
> +
> +static void
> +skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> +				   struct drm_crtc *for_crtc,
> +				   struct intel_wm_config *config,
> +				   struct skl_pipe_wm_parameters *params,

const

> +				   uint16_t *available,
> +				   uint16_t *start,
> +				   uint16_t *end)
> +{
> +	struct drm_crtc *crtc;
> +	unsigned int ddb_size;
> +	int nth_active_pipe;
> +
> +	if (!params->active) {
> +		*available = 0;
> +		*start = 0;
> +		*end = 0;
> +		return;
> +	}
> +
> +	ddb_size = SKL_DDB_SIZE;
> +
> +	ddb_size -= 4; /* 4 blocks for bypass path allocation */
> +
> +	nth_active_pipe = 1;

Why does this start from 1 and not 0? Are we pascal coders now? :P

> +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +		if (!intel_crtc_active(crtc))
> +			continue;
> +
> +		if (crtc == for_crtc)
> +			break;
> +
> +		nth_active_pipe++;
> +	}

One thing that has been bugging me for a long time is this kind of
loops for just counting stuff. I think we want to start using bitmasks
to track active pipes/planes/etc. But that's not really related to this
patch itself, just a more generic remark. And in this case it wouldn't
even help :)

> +
> +	*available = ddb_size / config->num_pipes_active;

This seems like a pointless thing to return separately. end-start is
the same thing (apart from the +/-1 stuff which should be killed again
by making end exclusive).

> +	*start = (nth_active_pipe - 1) * ddb_size / config->num_pipes_active;
> +	*end = *start + *available - 1;

Didn't we have that new struct with start+end in it? Perhaps it should
be used here?

As stated before this is going to complicate the application of the DDB
registers since we now have to consider the vblanks of potentially
multiple active pipes. But it should provide more optiomal DDB usage
than the max/num_pipes that the automagic maxfifo mode on most other
platforms provides, so I guess we really do want to do it this way. And
it seems to me that having two pipes enabled woud more much more common
than three pipes.

> +}
> +
> +static unsigned int skl_cursor_allocation(struct intel_wm_config *config)

const

> +{
> +	if (config->num_pipes_active == 1)
> +		return 32;
> +
> +	return 8;
> +}
> +
> +static unsigned int
> +skl_plane_relative_data_rate(struct intel_plane_wm_parameters *p)

const

> +{
> +	return p->horiz_pixels * p->vert_pixels * p->bytes_per_pixel;
> +}
> +
> +/*
> + * We don't overflow 32 bits. Worst case is 3 planes enabled, each fetching
> + * a 8192x4096@32bpp framebuffer:
> + *   3 * 4096 * 8192  * 4 < 2^32
> + */
> +static unsigned int
> +skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
> +				 struct skl_pipe_wm_parameters *params)

const

> +{
> +	unsigned int total_data_rate = 0;
> +	int plane;
> +
> +	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
> +		struct intel_plane_wm_parameters *p = &params->plane[plane];
> +
> +		if (!p->enabled)
> +			continue;
> +
> +		total_data_rate += skl_plane_relative_data_rate(p);
> +	}
> +
> +	return total_data_rate;
> +}
> +
> +static void
> +skl_allocate_pipe_ddb(struct drm_crtc *crtc,
> +		      struct intel_wm_config *config,
> +		      struct skl_pipe_wm_parameters *params,

const

> +		      struct skl_ddb_allocation *ddb /* out */)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum pipe pipe = intel_crtc->pipe;
> +	uint16_t available, start, end, cursor_blocks;
> +	unsigned int total_data_rate;
> +	int plane;
> +
> +	skl_ddb_get_pipe_allocation_limits(dev, crtc, config, params,
> +					   &available, &start, &end);
> +	if (available == 0) {
> +		const size_t size = ARRAY_SIZE(ddb->plane[pipe]) *
> +				    sizeof(struct skl_ddb_entry);
> +
> +		memset(ddb->plane[pipe], 0, size);

sizeof(ddb->plane[pipe]) 

> +		memset(&ddb->cursor[pipe], 0, sizeof(struct skl_ddb_entry));

sizeof(ddb->cursor[pipe]) 

> +		return;
> +	}
> +
> +	cursor_blocks = skl_cursor_allocation(config);
> +	ddb->cursor[pipe].start = end - cursor_blocks + 1;
> +	ddb->cursor[pipe].end = end;
> +
> +	available -= cursor_blocks;
> +	end -= cursor_blocks;

I got the impression bspec recommened putting all the cursor allocations
into one lump. But I can't really think of any good reason for doing
that so this seems fine to me.

> +
> +	/*
> +	 * Each active plane get a portion of the remaining space, in
> +	 * proportion to the amount of data they need to fetch from memory.
> +	 *
> +	 * FIXME: we don't always allocate every single block here.
> +	 */
> +	total_data_rate = skl_get_total_relative_data_rate(intel_crtc, params);
> +
> +	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
> +		struct intel_plane_wm_parameters *p = &params->plane[plane];
> +		unsigned int data_rate;
> +		uint16_t plane_blocks;
> +
> +		if (!p->enabled)
> +			continue;
> +
> +		data_rate = skl_plane_relative_data_rate(p);
> +
> +		/*
> +		 * promote the expression to 64 bits to avoid overflowing, the
> +		 * result is < available as data_rate / total_data_rate < 1
> +		 */
> +		plane_blocks = div_u64((uint64_t)available * data_rate,
> +				       total_data_rate);
> +
> +		ddb->plane[pipe][plane].start = start;
> +		ddb->plane[pipe][plane].end = start + plane_blocks - 1;
> +
> +		start += plane_blocks;
> +	}
> +
> +}
> +
>  static uint32_t skl_pipe_pixel_rate(struct drm_device *dev,
>  				    struct drm_crtc *crtc)
>  {
> @@ -3259,6 +3408,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  	skl_compute_wm_pipe_parameters(crtc, params);
> +	skl_allocate_pipe_ddb(crtc, config, params, ddb);
>  	skl_compute_pipe_wm(crtc, ddb, params, pipe_wm);
>  
>  	if (!memcmp(&intel_crtc->wm.skl_active, pipe_wm, sizeof(*pipe_wm)))
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC