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.
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 messagesdoes 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 flagswhen
* there's nothing connected on the port*/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.
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 messagesdoes 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 flagswhen
* there's nothing connected on the port*/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
dri-devel@lists.freedesktop.org
-
Christian König -
Jean Delvare -
Lyude Paul