[Intel-gfx] [PATCH 11/12] drm/i915/skl: New ddb allocation algorithm

[Mahesh Kumar]

Hi,


On Thursday 18 May 2017 02:44 AM, Matt Roper wrote:
> On Wed, May 17, 2017 at 05:28:30PM +0530, Mahesh Kumar wrote:
>> From: "Kumar, Mahesh" <mahesh1.kumar at intel.com>
>>
>> This patch implements new DDB allocation algorithm as per HW team
>> recommendation. This algo takecare of scenario where we allocate less DDB
>> for the planes with lower relative pixel rate, but they require more DDB
>> to work.
>> It also takes care of enabling same watermark level for each
>> plane in crtc, for efficient power saving.
>>
>> Changes since v1:
>>   - Rebase on top of Paulo's patch series
>>
>> Changes since v2:
>>   - Fix the for loop condition to enable WM
>>
>> Changes since v3:
>>   - Fix crash in cursor i-g-t reported by Maarten
>>   - Rebase after addressing Paulo's comments
>>   - Few other ULT fixes
>> Changes since v4:
>>   - Rebase on drm-tip
>>   - Added separate function to enable WM levels
>> Changes since v5:
>>   - Fix a crash identified in skl-6770HQ system
>> Changes since v6:
>>   - Address review comments from Matt
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
> ...
>> @@ -4096,10 +4126,48 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *cstate,
> ...
>> +
>> +		/*
>> +		 * If This this level can successfully be enabled with the
> Typo; repeated word ("This this").
>
> ...
>> @@ -4381,64 +4452,41 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>>   		}
>>   	}
>>   
>> -	if (res_blocks >= ddb_allocation || res_lines > 31) {
>> -		*enabled = false;
>> +	if (res_lines >= 31 && level == 0) {
>> +		struct drm_plane *plane = pstate->plane;
>>   
>> -		/*
>> -		 * If there are no valid level 0 watermarks, then we can't
>> -		 * support this display configuration.
>> -		 */
>> -		if (level) {
>> -			return 0;
>> -		} else {
>> -			struct drm_plane *plane = pstate->plane;
>> -
>> -			DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
>> -			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks required = %u/%u, lines required = %u/31\n",
>> -				      plane->base.id, plane->name,
>> -				      res_blocks, ddb_allocation, res_lines);
>> -			return -EINVAL;
>> -		}
>> +		DRM_DEBUG_KMS("Requested display configuration exceeds system watermark limitations\n");
>> +		DRM_DEBUG_KMS("[PLANE:%d:%s] lines required = %u/31\n",
>> +				plane->base.id, plane->name, res_lines);
>>   	}
> I'm still confused why we don't need to return an error in this case?
> We print the debug message here, but the function still returns 0
> (success).  When we run skl_enable_plane_wm_levels(), it will disable
> level 0 for that plane, which means we should be rejecting the whole
> atomic flip (since this plane fails at all levels), but I don't see
> anywhere that we actually check that again and trigger the failure?
I really missed the point you were trying to make previously, Indeed 
this should return -EINVAL,
I missed that while recreating the patch. thanks alot for pointing that out.
submitting new version of patch.
-Mahesh
>
> Matt
>
>>   
>>   	*out_blocks = res_blocks;
>>   	*out_lines = res_lines;
>> -	*enabled = true;
>>   
>>   	return 0;
>>   }
>>   
> ...
>