[Intel-gfx] [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Mahesh Kumar]

Hi Maarten,

Thanks for review :)


On Tuesday 30 May 2017 03:30 PM, Maarten Lankhorst wrote:
> Op 26-05-17 om 17:15 schreef Mahesh Kumar:
>> Don't trust cached DDB values. Recalculate the ddb value if cached value
>> is zero.
>>
>> If i915 is build as a module, there may be a race condition when
>> cursor_disable call comes even before intel_fbdev_initial_config.
>> Which may lead to cached value being 0. And further commit will fail
>> until a modeset.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 936eef1634c7..b67be1355e39 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>   	struct drm_i915_private *dev_priv = to_i915(dev);
>>   	struct drm_crtc *for_crtc = cstate->base.crtc;
>>   	unsigned int pipe_size, ddb_size;
>> +	unsigned int active_crtcs;
>>   	int nth_active_pipe;
>>   
>>   	if (WARN_ON(!state) || !cstate->base.active) {
>> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>   	}
>>   
>>   	if (intel_state->active_pipe_changes)
>> -		*num_active = hweight32(intel_state->active_crtcs);
>> +		active_crtcs = intel_state->active_crtcs;
>>   	else
>> -		*num_active = hweight32(dev_priv->active_crtcs);
>> +		active_crtcs = dev_priv->active_crtcs;
>>   
>> +	*num_active = hweight32(active_crtcs);
>>   	ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>>   	WARN_ON(ddb_size == 0);
>>   
>> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>   		 * copy from old state to be sure
>>   		 */
>>   		*alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
>> -		return;
>> +		if (!skl_ddb_entry_size(alloc))
>> +			DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
>> +		else
>> +			return;
>>   	}
>>   
>> -	nth_active_pipe = hweight32(intel_state->active_crtcs &
>> +	nth_active_pipe = hweight32(active_crtcs &
>>   				    (drm_crtc_mask(for_crtc) - 1));
>> -	pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
>> +	pipe_size = ddb_size / hweight32(active_crtcs);
>>   	alloc->start = nth_active_pipe * ddb_size / *num_active;
>>   	alloc->end = alloc->start + pipe_size;
>>   }
> I'd love it if you also add a warning somewhere for active pipe with no ddb allocation to prevent this from reoccuring in the future. :)
> But with the root cause being a invalid ddb allocation at boot, won't the below one liner should fix it too, but better?
Above DEBUG message is actually warning. That will hit only if pipe is 
active & DDB allocated for that pipe is "0" (we do return from caller if 
!cstate->active).
I can reword the msg to be WARN & make it something like "Pipe is active 
with No DDB allocated, recompute ddb now". This will fix the issue as 
well as add warning msg.

>
> I'l ltest it when f3-kbl becomes available again.
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a488e068c3d6..63a47909f618 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state *state)
>   	 */
>   	for_each_new_crtc_in_state(state, crtc, cstate, i)
>   		changed = true;
> -	if (!changed)
> +	if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm)
>   		return 0;
>   
This change should also solve the issue during boot.

-Mahesh
>   	/* Clear all dirty flags */
>