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

[Mahesh Kumar]

Hi Ander,

Thanks for review

On Wednesday 12 April 2017 02:47 PM, Ander Conselvan De Oliveira wrote:
> 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.
Ok, will make changes,
Not only modeset, any flip whose requirement is exceeding available ddb 
allocation, we should fail that ioctl.
>
> 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.
Yes, patch 3,4 as well are independent of the series & applicable for 
old algorithm too.
>
>> 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 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.
will do :)
>
>> +		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.
will update.

-Mahesh
>
> 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;
>>