Re: [PATCH 5/8] drm/i915/skl+: ddb min requirement may exceed allocation

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Tue, 2017-02-28 at 17:01 +0530, Mahesh Kumar wrote:
> DDB minimum requirement may also exceed the allocated DDB for CRTC.
> Instead of directly deducting from alloc_size, check against
> total_min_ddb requirement. if exceeding fail the flip.

Instead of doing a low level description of the code change, including variable
names and whatnot, use this space to describe in a high level what the patch
does and why it is necessary. For instance, in this particular case the patch
changes the modeset to fail when there isn't enough ddb for the given
configuration. Previously it succeeded, but the plane ddb allocations would be
bogus because of the negative value of alloc_size, leading to screen corruption
and system hangs.

Do I understand correctly that this patch is independent from the rest of the
series? Might make sense to merge it earlier, since the bug is there with the
old ddb allocation algorithm too.

> 
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 76c986166664..24e9e5d69833 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3380,6 +3380,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	int num_active;
>  	unsigned plane_data_rate[I915_MAX_PLANES] = {};
>  	unsigned plane_y_data_rate[I915_MAX_PLANES] = {};
> +	uint16_t total_min_blocks = 0;
>  
>  	/* Clear the partitioning for disabled planes. */
>  	memset(ddb->plane[pipe], 0, sizeof(ddb->plane[pipe]));
> @@ -3407,10 +3408,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
>  	 */
>  
>  	for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> -		alloc_size -= minimum[plane_id];
> -		alloc_size -= y_minimum[plane_id];
> +		total_min_blocks += minimum[plane_id];
> +		total_min_blocks += y_minimum[plane_id];
>  	}
>  
> +	if ((total_min_blocks > alloc_size)) {

One '(' is enough.

> +		DRM_DEBUG_KMS("Requested display configuration exceeds system DDB limitations");
> +		DRM_DEBUG_KMS("minimum required %d/%d\n", total_min_blocks,
> +							alloc_size);
> +		return-EINVAL;

Space between return and -EINVAL please.

Ander

> +	}
> +
> +	alloc_size -= total_min_blocks;
>  	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
>  	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx