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

[Tvrtko Ursulin]

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...

Regards,

Tvrtko