[Intel-gfx] [PATCH 07/11] drm/i915/skl+: Fail the flip if ddb min requirement exceeds pipe allocation

[Mahesh Kumar]

On Friday 12 May 2017 05:53 AM, Matt Roper wrote:
> On Mon, May 08, 2017 at 05:18:58PM +0530, Mahesh Kumar wrote:
>> DDB minimum requirement may exceed the allocated DDB for CRTC/Pipe. This
>> patch make changes to fail the flip if minimum requirement for pipe
>> exceeds the total ddb allocated to the pipe.
>> Previously it succeeded but making alloc_size a negative value. Which
>> will make later calculations for plane ddb allocation bogus & may lead
>> to screen corruption or system hang.
>>
> I think originally the bspec defined the initial minimum DDB allocation
> for each plane to just be '8' in all cases.  So even with 4 planes on a
> pipe, your initial pipe minimum would only be 32, so the code here never
> had a risk of overflowing (note that the initial minimum is different
> than the WM0 minimum requirement we ultimately wind up calculating).
> But then the bspec was updated to suggest a higher initial minimum for
> y-tiled buffers (calculated based on the width of the buffer), so our
> initial assumptions here don't really hold anymore.  We should still be
> safe if we're not using y-tile, but even a single 4k y-tiled plane would
> have an initial minimum of something like 250 blocks iirc., so we can't
> assume we're always safe anymore.
>
>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at 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 2a4e9d89cd6f..0ace94d67432 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3591,6 +3591,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]));
>> @@ -3618,10 +3619,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)) {
>> +		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;
> I think you're missing a space here?
>
> With that fixed,
I think I already fixed this in newer version of patch. :),
anyway resubmitting complete series again with other comments addressed.
thanks,
-Mahesh
>
> Reviewed-by: Matt Roper <matthew.d.roper at intel.com>
>
>> +	}
>> +
>> +	alloc_size -= total_min_blocks;
>>   	ddb->plane[pipe][PLANE_CURSOR].start = alloc->end - minimum[PLANE_CURSOR];
>>   	ddb->plane[pipe][PLANE_CURSOR].end = alloc->end;
>>   
>> -- 
>> 2.11.0
>>