[Intel-gfx] [PATCH] drm/i915/skl: Bypass debug message if scalers are not requested

[Tvrtko Ursulin]

On 08/05/2015 02:58 PM, Daniel Vetter wrote:
> On Wed, Aug 05, 2015 at 02:20:17PM +0100, Tvrtko Ursulin wrote:
>>
>> On 05/04/2015 03:29 PM, Daniel Vetter wrote:
>>> On Fri, Apr 24, 2015 at 06:07:05PM +0000, Konduru, Chandra wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Konduru, Chandra
>>>>> Sent: Friday, April 24, 2015 10:53 AM
>>>>> To: 'Tvrtko Ursulin'; Intel-gfx at lists.freedesktop.org
>>>>> Cc: Ursulin, Tvrtko
>>>>> Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
>>>>> requested
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
>>>>>> Sent: Friday, April 24, 2015 9:34 AM
>>>>>> To: Konduru, Chandra; Intel-gfx at lists.freedesktop.org
>>>>>> Cc: Ursulin, Tvrtko
>>>>>> Subject: Re: [PATCH] drm/i915/skl: Bypass debug message if scalers are
>>>>>> not requested
>>>>>>
>>>>>>
>>>>>> On 04/24/2015 05:30 PM, Konduru, Chandra wrote:
>>>>>>>
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Tvrtko Ursulin [mailto:tvrtko.ursulin at linux.intel.com]
>>>>>>>> Sent: Friday, April 24, 2015 9:08 AM
>>>>>>>> To: Intel-gfx at lists.freedesktop.org
>>>>>>>> Cc: Ursulin, Tvrtko; Konduru, Chandra
>>>>>>>> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
>>>>>>>> not requested
>>>>>>>>
>>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>>>
>>>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>>>>> Cc: Chandra Konduru <chandra.konduru at intel.com>
>>>>>>>> ---
>>>>>>>> Up for discussion I suppose, but like it is, with typical drm.debug
>>>>>>>> = 0xe, it logs one line per cursor movement while the log would
>>>>>>>> otherwise be
>>>>>> quiet.
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/i915/intel_atomic.c | 3 +++
>>>>>>>>    1 file changed, 3 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>>>>>>>> b/drivers/gpu/drm/i915/intel_atomic.c
>>>>>>>> index 3c4b7cd..7284c6d 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>>>>>>>> @@ -302,6 +302,9 @@ int intel_atomic_setup_scalers(struct
>>>>>>>> drm_device
>>>>>> *dev,
>>>>>>>>    	scaler_state = &crtc_state->scaler_state;
>>>>>>>>    	drm_state = crtc_state->base.state;
>>>>>>>>
>>>>>>>> +	if (!scaler_state->scaler_users)
>>>>>>>> +		return 0;
>>>>>>>
>>>>>>> This will cause issue because scalers will never get freed if they
>>>>>>> are in use
>>>>>> before and no more required now.
>>>>>>> I put the debug print to help debug variety of state related issues
>>>>>>> while we are in development, but perhaps debug print can be deleted.
>>>>>>
>>>>>> Doesn't the loop below skip everything anyway when no bits are set in
>>>>>> scaler_state->scaler_users ?
>>>>> Oh, that's right, ignore my prev comment (with updated scaler design above
>>>>> shouldn't cause issues).
>>>>> By the way, can you pls run kms_panel_fit and kms_plane_scaling to make sure
>>>>> they pass and there are no related warnings or errors in kernel log?
>>>>> For kms_plane_scaling, pls apply two pending patches that aren't merged:
>>>>> v5 [PATCH 13/14] drm/i915: skylake primary plane scaling using shared scalers
>>>>> v5 [PATCH 14/14] drm/i915: skylake sprite plane scaling using shared scalers
>>>>>
>>>> One more: This change address logs when no scaler is required,
>>>> but might comeback when a scaler is active (panel fitting or plane scaling is enabled).
>>>> I don't know general policy in these kind of situations, but perhaps debug print
>>>> can be deleted.
>>>
>>> Atomic is really complicated, but doing fully diagnostics for each frame
>>> is also way too noisy. For that reason we've add a DRM_DEBUG_ATOMIC which
>>> can be used for all these state tracking debug lines.
>>
>> We didn't do anything here and I just noticed kernel is still too spammy
>> with regards to this issue.
>>
>> Should we just merge my patch? Still looks completely safe to me...
>
> doesn't seem to apply any more:(

Yeah only some months have passed, who would have thought. :)

But I realized that would be only one of the three log lines per cursor 
update - there is a code path calling skl_detach_scaler two times as 
well. Looks like this overall, per update:

[drm:intel_atomic_setup_scalers] crtc_state = ffff880074b55c00 need = 0 
avail = 2 scaler_users = 0x0
[drm:skl_detach_scaler] CRTC:21 Disabled scaler id 0.0
[drm:skl_detach_scaler] CRTC:21 Disabled scaler id 0.1

I'll rebase this patch for a start.

Regards,

Tvrtko