radeon_dp_aux_transfer_native: 74 callbacks suppressed

radeon_dp_aux_transfer_native: 74 callbacks suppressed

[Jean Delvare]

Hi all,

commit 92c177b7947d9c889ea7b024871445015ea74221
Author: Lyude
Date:   Wed Feb 22 16:34:53 2017 -0500

    drm/radeon/dp_auxch: Ratelimit aux transfer debug messages

does more harm than good in my opinion. Since this commit, I see
several occurrences of the following message in my kernel log daily:

radeon_dp_aux_transfer_native: 74 callbacks suppressed

I never got to see the "callback" in question though, not even once, as
this is a debug message which is off by default. Before the change, I
would not get any such message in the kernel log (as I would expect
when everything works as intended.)

Does this debug message really have any practical value? If not, the
easiest solution would be to simply drop it:

--- a/drivers/gpu/drm/radeon/radeon_dp_auxch.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_auxch.c
@@ -168,8 +168,10 @@ radeon_dp_aux_transfer_native(struct drm
 		goto done;
 	}
 	if (tmp & AUX_RX_ERROR_FLAGS) {
-		DRM_DEBUG_KMS_RATELIMITED("dp_aux_ch flags not zero: %08x\n",
-					  tmp);
+		/*
+		 * aux transfers always fail with non-zero status flags when
+		 * there's nothing connected on the port
+		 */
 		ret = -EIO;
 		goto done;
 	}

I can resend this as a formal patch if you agree with this solution.

The actual cause of the problem is that ___ratelimit() prints its
message at KERN_WARNING level regardless of the level of the message
being suppressed. This makes ratelimiting debug, info or notice
messages awkward. Looks like a design overlook to me, maybe it should
be fixed, but that's a much bigger and intrusive change than the
proposal above.

-- 
Jean Delvare
SUSE L3 Support
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: radeon_dp_aux_transfer_native: 74 callbacks suppressed

[Lyude Paul]

The main reason I added this was because the radeon driver's hotplugging paths
for DP do a ton of unnessecary probing, and because the driver usually also
checks all connectors every time there's a hotplug (there isn't much of a good
reason for this, it's just an old driver) it's not at all difficult to get
well more then 70 callbacks from this that end up filling the kernel log with
useless information (all the messages mean is that for some reason or another,
a DP aux transaction fails which usually just means the port is disconnected).

Ideally: We should be printing the first error code that the i2c handlers for
radeon return in addition to the suppressed messages (and this itself should
not be suppressed iirc), since that's almost always the only piece of
information that matters.

I wouldn't drop the message entirely though unless we're printing something
from DRM. Additionally, we should also just fix this ratelimit macro anyway
since it's intended purpose is not to print anything when debugging isn't
enabled. What do you think Alex?

On Mon, 2017-11-06 at 10:21 +0100, Jean Delvare wrote:
> Hi all,
> 
> commit 92c177b7947d9c889ea7b024871445015ea74221
> Author: Lyude
> Date:   Wed Feb 22 16:34:53 2017 -0500
> 
>     drm/radeon/dp_auxch: Ratelimit aux transfer debug messages
> 
> does more harm than good in my opinion. Since this commit, I see
> several occurrences of the following message in my kernel log daily:
> 
> radeon_dp_aux_transfer_native: 74 callbacks suppressed
> 
> I never got to see the "callback" in question though, not even once, as
> this is a debug message which is off by default. Before the change, I
> would not get any such message in the kernel log (as I would expect
> when everything works as intended.)
> 
> Does this debug message really have any practical value? If not, the
> easiest solution would be to simply drop it:
> 
> --- a/drivers/gpu/drm/radeon/radeon_dp_auxch.c
> +++ b/drivers/gpu/drm/radeon/radeon_dp_auxch.c
> @@ -168,8 +168,10 @@ radeon_dp_aux_transfer_native(struct drm
>  		goto done;
>  	}
>  	if (tmp & AUX_RX_ERROR_FLAGS) {
> -		DRM_DEBUG_KMS_RATELIMITED("dp_aux_ch flags not zero:
> %08x\n",
> -					  tmp);
> +		/*
> +		 * aux transfers always fail with non-zero status flags
> when
> +		 * there's nothing connected on the port
> +		 */
>  		ret = -EIO;
>  		goto done;
>  	}
> 
> I can resend this as a formal patch if you agree with this solution.
> 
> The actual cause of the problem is that ___ratelimit() prints its
> message at KERN_WARNING level regardless of the level of the message
> being suppressed. This makes ratelimiting debug, info or notice
> messages awkward. Looks like a design overlook to me, maybe it should
> be fixed, but that's a much bigger and intrusive change than the
> proposal above.
> 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: radeon_dp_aux_transfer_native: 74 callbacks suppressed

[Christian König]

> Additionally, we should also just fix this ratelimit macro anyway
> since it's intended purpose is not to print anything when debugging isn't
> enabled. What do you think Alex?
Well, at least I think that the described behavior of the macro is a bug 
which should be fixed.

But I think that is independent of the problem that radeon is a bit 
flaky about DP aux transfers.

Regards,
Christian.

Am 06.11.2017 um 17:17 schrieb Lyude Paul:
> The main reason I added this was because the radeon driver's hotplugging paths
> for DP do a ton of unnessecary probing, and because the driver usually also
> checks all connectors every time there's a hotplug (there isn't much of a good
> reason for this, it's just an old driver) it's not at all difficult to get
> well more then 70 callbacks from this that end up filling the kernel log with
> useless information (all the messages mean is that for some reason or another,
> a DP aux transaction fails which usually just means the port is disconnected).
>
> Ideally: We should be printing the first error code that the i2c handlers for
> radeon return in addition to the suppressed messages (and this itself should
> not be suppressed iirc), since that's almost always the only piece of
> information that matters.
>
> I wouldn't drop the message entirely though unless we're printing something
> from DRM. Additionally, we should also just fix this ratelimit macro anyway
> since it's intended purpose is not to print anything when debugging isn't
> enabled. What do you think Alex?
>
> On Mon, 2017-11-06 at 10:21 +0100, Jean Delvare wrote:
>> Hi all,
>>
>> commit 92c177b7947d9c889ea7b024871445015ea74221
>> Author: Lyude
>> Date:   Wed Feb 22 16:34:53 2017 -0500
>>
>>      drm/radeon/dp_auxch: Ratelimit aux transfer debug messages
>>
>> does more harm than good in my opinion. Since this commit, I see
>> several occurrences of the following message in my kernel log daily:
>>
>> radeon_dp_aux_transfer_native: 74 callbacks suppressed
>>
>> I never got to see the "callback" in question though, not even once, as
>> this is a debug message which is off by default. Before the change, I
>> would not get any such message in the kernel log (as I would expect
>> when everything works as intended.)
>>
>> Does this debug message really have any practical value? If not, the
>> easiest solution would be to simply drop it:
>>
>> --- a/drivers/gpu/drm/radeon/radeon_dp_auxch.c
>> +++ b/drivers/gpu/drm/radeon/radeon_dp_auxch.c
>> @@ -168,8 +168,10 @@ radeon_dp_aux_transfer_native(struct drm
>>   		goto done;
>>   	}
>>   	if (tmp & AUX_RX_ERROR_FLAGS) {
>> -		DRM_DEBUG_KMS_RATELIMITED("dp_aux_ch flags not zero:
>> %08x\n",
>> -					  tmp);
>> +		/*
>> +		 * aux transfers always fail with non-zero status flags
>> when
>> +		 * there's nothing connected on the port
>> +		 */
>>   		ret = -EIO;
>>   		goto done;
>>   	}
>>
>> I can resend this as a formal patch if you agree with this solution.
>>
>> The actual cause of the problem is that ___ratelimit() prints its
>> message at KERN_WARNING level regardless of the level of the message
>> being suppressed. This makes ratelimiting debug, info or notice
>> messages awkward. Looks like a design overlook to me, maybe it should
>> be fixed, but that's a much bigger and intrusive change than the
>> proposal above.
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx