drm/i915/skl: Bypass debug message if scalers are not requested

Commit Message

Tvrtko Ursulin April 24, 2015, 4:08 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

Signed-off-by: Tvrtko Ursulin <[e-mail]>
Cc: Chandra Konduru <[e-mail]>
---
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(+)

Comments

Chandra Konduru April 24, 2015, 4:30 p.m. UTC | #1

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:[e-mail]]
> Sent: Friday, April 24, 2015 9:08 AM
> To: [e-mail]
> Cc: Ursulin, Tvrtko; Konduru, Chandra
> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
> requested
> 
> From: Tvrtko Ursulin <[e-mail]>
> 
> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> Cc: Chandra Konduru <[e-mail]>
> ---
> 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.

> +
>  	num_scalers_need = hweight32(scaler_state->scaler_users);
>  	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users =
> 0x%x\n",
>  		crtc_state, num_scalers_need, intel_crtc->num_scalers,
> --
> 2.3.5

Tvrtko Ursulin April 24, 2015, 4:33 p.m. UTC | #2

On 04/24/2015 05:30 PM, Konduru, Chandra wrote:
>
>
>> -----Original Message-----
>> From: Tvrtko Ursulin [mailto:[e-mail]]
>> Sent: Friday, April 24, 2015 9:08 AM
>> To: [e-mail]
>> Cc: Ursulin, Tvrtko; Konduru, Chandra
>> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
>> requested
>>
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>> Cc: Chandra Konduru <[e-mail]>
>> ---
>> 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 ?

Regards,

Tvrtko

Chandra Konduru April 24, 2015, 5:52 p.m. UTC | #3

> -----Original Message-----
> From: Tvrtko Ursulin [mailto:[e-mail]]
> Sent: Friday, April 24, 2015 9:34 AM
> To: Konduru, Chandra; [e-mail]
> 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:[e-mail]]
> >> Sent: Friday, April 24, 2015 9:08 AM
> >> To: [e-mail]
> >> Cc: Ursulin, Tvrtko; Konduru, Chandra
> >> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
> >> not requested
> >>
> >> From: Tvrtko Ursulin <[e-mail]>
> >>
> >> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >> Cc: Chandra Konduru <[e-mail]>
> >> ---
> >> 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

> 
> Regards,
> 
> Tvrtko

Chandra Konduru April 24, 2015, 6:07 p.m. UTC | #4

> -----Original Message-----
> From: Konduru, Chandra
> Sent: Friday, April 24, 2015 10:53 AM
> To: 'Tvrtko Ursulin'; [e-mail]
> Cc: Ursulin, Tvrtko
> Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
> requested
> 
> 
> 
> > -----Original Message-----
> > From: Tvrtko Ursulin [mailto:[e-mail]]
> > Sent: Friday, April 24, 2015 9:34 AM
> > To: Konduru, Chandra; [e-mail]
> > 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:[e-mail]]
> > >> Sent: Friday, April 24, 2015 9:08 AM
> > >> To: [e-mail]
> > >> Cc: Ursulin, Tvrtko; Konduru, Chandra
> > >> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
> > >> not requested
> > >>
> > >> From: Tvrtko Ursulin <[e-mail]>
> > >>
> > >> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> > >> Cc: Chandra Konduru <[e-mail]>
> > >> ---
> > >> 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.

> >
> > Regards,
> >
> > Tvrtko

Shuang He April 28, 2015, 8:24 a.m. UTC | #5

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: [e-mail])
Task id: 6262
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  301/301              301/301
SNB                                  318/318              318/318
IVB                                  345/345              345/345
BYT                 -2              285/285              283/285
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_ctx_basic      PASS(3)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
*BYT  igt@kms_setmode@invalid-clone-exclusive-crtc      PASS(6)      DMESG_WARN(1)PASS(1)
(dmesg patch applied)drm:check_crtc_state[i915]]*ERROR*mismatch_in_has_infoframe(expected#,found#)@mismatch in has_infoframe .* found
WARNING:at_drivers/gpu/drm/i915/intel_display.c:#check_crtc_state[i915]()@WARNING:.* at .* check_crtc_state+0x
Note: You need to pay more attention to line start with '*'

Daniel Vetter May 4, 2015, 2:29 p.m. UTC | #6

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'; [e-mail]
> > Cc: Ursulin, Tvrtko
> > Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
> > requested
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Tvrtko Ursulin [mailto:[e-mail]]
> > > Sent: Friday, April 24, 2015 9:34 AM
> > > To: Konduru, Chandra; [e-mail]
> > > 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:[e-mail]]
> > > >> Sent: Friday, April 24, 2015 9:08 AM
> > > >> To: [e-mail]
> > > >> Cc: Ursulin, Tvrtko; Konduru, Chandra
> > > >> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
> > > >> not requested
> > > >>
> > > >> From: Tvrtko Ursulin <[e-mail]>
> > > >>
> > > >> Signed-off-by: Tvrtko Ursulin <[e-mail]>
> > > >> Cc: Chandra Konduru <[e-mail]>
> > > >> ---
> > > >> 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.
-Daniel

Tvrtko Ursulin Aug. 5, 2015, 1:20 p.m. UTC | #7

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'; [e-mail]
>>> Cc: Ursulin, Tvrtko
>>> Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
>>> requested
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Tvrtko Ursulin [mailto:[e-mail]]
>>>> Sent: Friday, April 24, 2015 9:34 AM
>>>> To: Konduru, Chandra; [e-mail]
>>>> 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:[e-mail]]
>>>>>> Sent: Friday, April 24, 2015 9:08 AM
>>>>>> To: [e-mail]
>>>>>> Cc: Ursulin, Tvrtko; Konduru, Chandra
>>>>>> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
>>>>>> not requested
>>>>>>
>>>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>>>>> Cc: Chandra Konduru <[e-mail]>
>>>>>> ---
>>>>>> 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

Daniel Vetter Aug. 5, 2015, 1:58 p.m. UTC | #8

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'; [e-mail]
> >>>Cc: Ursulin, Tvrtko
> >>>Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
> >>>requested
> >>>
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: Tvrtko Ursulin [mailto:[e-mail]]
> >>>>Sent: Friday, April 24, 2015 9:34 AM
> >>>>To: Konduru, Chandra; [e-mail]
> >>>>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:[e-mail]]
> >>>>>>Sent: Friday, April 24, 2015 9:08 AM
> >>>>>>To: [e-mail]
> >>>>>>Cc: Ursulin, Tvrtko; Konduru, Chandra
> >>>>>>Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
> >>>>>>not requested
> >>>>>>
> >>>>>>From: Tvrtko Ursulin <[e-mail]>
> >>>>>>
> >>>>>>Signed-off-by: Tvrtko Ursulin <[e-mail]>
> >>>>>>Cc: Chandra Konduru <[e-mail]>
> >>>>>>---
> >>>>>>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:(
-Daniel

> 
> Regards,
> 
> Tvrtko

Tvrtko Ursulin Aug. 5, 2015, 2:12 p.m. UTC | #9

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'; [e-mail]
>>>>> Cc: Ursulin, Tvrtko
>>>>> Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers are not
>>>>> requested
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Tvrtko Ursulin [mailto:[e-mail]]
>>>>>> Sent: Friday, April 24, 2015 9:34 AM
>>>>>> To: Konduru, Chandra; [e-mail]
>>>>>> 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:[e-mail]]
>>>>>>>> Sent: Friday, April 24, 2015 9:08 AM
>>>>>>>> To: [e-mail]
>>>>>>>> Cc: Ursulin, Tvrtko; Konduru, Chandra
>>>>>>>> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
>>>>>>>> not requested
>>>>>>>>
>>>>>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>>>>>
>>>>>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>>>>>>> Cc: Chandra Konduru <[e-mail]>
>>>>>>>> ---
>>>>>>>> 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

Tvrtko Ursulin Aug. 5, 2015, 2:19 p.m. UTC | #10

On 08/05/2015 03:12 PM, Tvrtko Ursulin wrote:
>
> 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'; [e-mail]
>>>>>> Cc: Ursulin, Tvrtko
>>>>>> Subject: RE: [PATCH] drm/i915/skl: Bypass debug message if scalers
>>>>>> are not
>>>>>> requested
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Tvrtko Ursulin [mailto:[e-mail]]
>>>>>>> Sent: Friday, April 24, 2015 9:34 AM
>>>>>>> To: Konduru, Chandra; [e-mail]
>>>>>>> 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:[e-mail]]
>>>>>>>>> Sent: Friday, April 24, 2015 9:08 AM
>>>>>>>>> To: [e-mail]
>>>>>>>>> Cc: Ursulin, Tvrtko; Konduru, Chandra
>>>>>>>>> Subject: [PATCH] drm/i915/skl: Bypass debug message if scalers are
>>>>>>>>> not requested
>>>>>>>>>
>>>>>>>>> From: Tvrtko Ursulin <[e-mail]>
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tvrtko Ursulin <[e-mail]>
>>>>>>>>> Cc: Chandra Konduru <[e-mail]>
>>>>>>>>> ---
>>>>>>>>> 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.

Sent it as a new thread but forgot v2 in the subject.

Other source of spam is probably intel_begin_crtc_commit -> 
skl_detach_scalers.

Don't know what the right fix would be there. Looks like it is not 
tracking transitions in scaler use so it would be able to act and log 
when something interesting happens, but rather does it every time. I 
defer to Chandra. :)

Regards,

Tvrtko

Tvrtko Ursulin Aug. 5, 2015, 3:02 p.m. UTC | #11

On 08/05/2015 03:19 PM, Tvrtko Ursulin wrote:
[snip]
>>>>> 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.
>
> Sent it as a new thread but forgot v2 in the subject.
>
> Other source of spam is probably intel_begin_crtc_commit ->
> skl_detach_scalers.
>
> Don't know what the right fix would be there. Looks like it is not
> tracking transitions in scaler use so it would be able to act and log
> when something interesting happens, but rather does it every time. I
> defer to Chandra. :)

Ok I've sent something named "[PATCH] drm/i915/skl: Only disable scalers 
once".

It was only compile tested due my current lack of suitable hardware for 
testing. And I don't pretend to be confident in the world of atomic 
states etc. so it may well be completely wrong.

So either scrutinize it a lot please, or let it be a motivator to come 
up with a proper fix of your own. :)

Regards,

Tvrtko

Patch

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;
+
 	num_scalers_need = hweight32(scaler_state->scaler_users);
 	DRM_DEBUG_KMS("crtc_state = %p need = %d avail = %d scaler_users = 0x%x\n",
 		crtc_state, num_scalers_need, intel_crtc->num_scalers,