[PATCH 0/7] Minor DP aux transaction fixes

[PATCH 0/7] Minor DP aux transaction fixes

[Lyude]

While I was investigating an unrelated bug on the radeon driver, I noticed that
it's become rather difficult to actually read through dmesg with drm.debug
turned on, on account of the huge number of messages we end up printing from
failed DP aux transactions that happen every time we reprobe each connector.

Timed out transactions are relatively normal, and as well there's a lot of
places in radeon/amdgpu where we're printing redundant debugging information
dozens of times each time we attempt a DP aux transactions.

Additionally, I've removed some of the retry loops in amdgpu/radeon. These were
definitely useful at one point, but since we now retry any failed aux
transaction unconditionally in DRM's dp helpers they don't serve much purpose
other then to make failing aux transactions take a lot more time then they need
to.

Lyude (7):
  drm/dp_helper: Print first error received on failure in
    drm_dp_dpcd_access()
  drm/radeon: Don't print error on aux transaction timeouts
  drm/radeon: Don't retry 7 times in radeon_dp_dpcd()
  drm/amdgpu: Don't print error on aux transaction timeouts
  drm/amdgpu: Don't retry 7 times in amdgpu_atombios_dp_get_dpcd()
  drm: Add ratelimited versions of the DRM_DEBUG* macros
  drm/dp_helper: Rate limit timeout errors from drm_dp_i2c_do_msg()

 drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 22 ++++++++++------------
 drivers/gpu/drm/drm_dp_helper.c          | 14 ++++++++++++--
 drivers/gpu/drm/radeon/atombios_dp.c     | 21 ++++++++++-----------
 drivers/gpu/drm/radeon/radeon_dp_auxch.c |  1 -
 include/drm/drmP.h                       | 30 ++++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 26 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/7] drm/amdgpu: Don't print error on aux transaction timeouts

[Lyude]

Since it's normal for DRM to retry our aux transaction helpers multiple
times in a row, up to 32 times for each attempted transaction, we're
making a lot of noise that is no longer necessary now that DRM will just
print the return code we give it.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
index 7f85c2c..166dc7b 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
@@ -88,7 +88,6 @@ static int amdgpu_atombios_dp_process_aux_ch(struct amdgpu_i2c_chan *chan,
 
 	/* timeout */
 	if (args.v2.ucReplyStatus == 1) {
-		DRM_DEBUG_KMS("dp_aux_ch timeout\n");
 		r = -ETIMEDOUT;
 		goto done;
 	}
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/7] drm/amdgpu: Don't retry 7 times in amdgpu_atombios_dp_get_dpcd()

[Lyude]

When this code was written, we didn't retry DP aux transactions on any
error, which required retrying important transactions like this in
individual drivers. Since that's no longer the case, retrying here is
not necessary. As well, we retry any aux transaction on any error 32
times. 7 * 32 = 224, which means this loop causes us to retry grabbing
the dpcd 224 times. This is definitely far more then we actually need to
do.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
index 166dc7b..f81068b 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_dp.c
@@ -338,22 +338,21 @@ int amdgpu_atombios_dp_get_dpcd(struct amdgpu_connector *amdgpu_connector)
 {
 	struct amdgpu_connector_atom_dig *dig_connector = amdgpu_connector->con_priv;
 	u8 msg[DP_DPCD_SIZE];
-	int ret, i;
+	int ret;
 
-	for (i = 0; i < 7; i++) {
-		ret = drm_dp_dpcd_read(&amdgpu_connector->ddc_bus->aux, DP_DPCD_REV, msg,
-				       DP_DPCD_SIZE);
-		if (ret == DP_DPCD_SIZE) {
-			memcpy(dig_connector->dpcd, msg, DP_DPCD_SIZE);
+	ret = drm_dp_dpcd_read(&amdgpu_connector->ddc_bus->aux, DP_DPCD_REV,
+			       msg, DP_DPCD_SIZE);
+	if (ret == DP_DPCD_SIZE) {
+		memcpy(dig_connector->dpcd, msg, DP_DPCD_SIZE);
 
-			DRM_DEBUG_KMS("DPCD: %*ph\n", (int)sizeof(dig_connector->dpcd),
-				      dig_connector->dpcd);
+		DRM_DEBUG_KMS("DPCD: %*ph\n", (int)sizeof(dig_connector->dpcd),
+			      dig_connector->dpcd);
 
-			amdgpu_atombios_dp_probe_oui(amdgpu_connector);
+		amdgpu_atombios_dp_probe_oui(amdgpu_connector);
 
-			return 0;
-		}
+		return 0;
 	}
+
 	dig_connector->dpcd[0] = 0;
 	return -EINVAL;
 }
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/7] drm/dp_helper: Print first error received on failure in drm_dp_dpcd_access()

[Lyude]

Since we always retry in drm_dp_dpcd_access() regardless of the error,
we're going to make a lot of noise if the aux->transfer function prints
it's own errors (as is the case with radeon). If we can print the error
code here, this reduces the need for drivers to do this. So instead of
having to print "dp_aux_ch timed out" over 32 times we can just print
once.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 8f11b87..43be189 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -223,7 +223,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 			err = ret;
 	}
 
-	DRM_DEBUG_KMS("too many retries, giving up\n");
+	DRM_DEBUG_KMS("Too many retries, giving up. First error: %d\n", err);
 	ret = err;
 
 unlock:
-- 
2.7.4

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

[PATCH 2/7] drm/radeon: Don't print error on aux transaction timeouts

[Lyude]

Since it's normal for DRM to retry our aux transaction helpers multiple
times in a row, up to 32 times for each attempted transaction, we're
making a lot of noise that is no longer necessary now that DRM will just
print the return code we give it.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/radeon/radeon_dp_auxch.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_dp_auxch.c b/drivers/gpu/drm/radeon/radeon_dp_auxch.c
index db64e00..2d46564 100644
--- a/drivers/gpu/drm/radeon/radeon_dp_auxch.c
+++ b/drivers/gpu/drm/radeon/radeon_dp_auxch.c
@@ -164,7 +164,6 @@ radeon_dp_aux_transfer_native(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg
 	}
 
 	if (tmp & AUX_SW_RX_TIMEOUT) {
-		DRM_DEBUG_KMS("dp_aux_ch timed out\n");
 		ret = -ETIMEDOUT;
 		goto done;
 	}
-- 
2.7.4

_______________________________________________
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
https://lists.x.org/mailman/listinfo/xorg-driver-ati

[PATCH 3/7] drm/radeon: Don't retry 7 times in radeon_dp_dpcd()

[Lyude]

When this code was written, we didn't retry DP aux transactions on any
error, which required retrying important transactions like this in
individual drivers. Since that's no longer the case, retrying here is
not necessary. As well, we retry any aux transaction on any error 32
times. 7 * 32 = 224, which means this loop causes us to retry grabbing
the dpcd 224 times. This is definitely far more then we actually need to
do.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/radeon/atombios_dp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
index cead089a..432cb46 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -389,22 +389,21 @@ bool radeon_dp_getdpcd(struct radeon_connector *radeon_connector)
 {
 	struct radeon_connector_atom_dig *dig_connector = radeon_connector->con_priv;
 	u8 msg[DP_DPCD_SIZE];
-	int ret, i;
+	int ret;
 
-	for (i = 0; i < 7; i++) {
-		ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_DPCD_REV, msg,
-				       DP_DPCD_SIZE);
-		if (ret == DP_DPCD_SIZE) {
-			memcpy(dig_connector->dpcd, msg, DP_DPCD_SIZE);
+	ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_DPCD_REV, msg,
+			       DP_DPCD_SIZE);
+	if (ret == DP_DPCD_SIZE) {
+		memcpy(dig_connector->dpcd, msg, DP_DPCD_SIZE);
 
-			DRM_DEBUG_KMS("DPCD: %*ph\n", (int)sizeof(dig_connector->dpcd),
-				      dig_connector->dpcd);
+		DRM_DEBUG_KMS("DPCD: %*ph\n", (int)sizeof(dig_connector->dpcd),
+			      dig_connector->dpcd);
 
-			radeon_dp_probe_oui(radeon_connector);
+		radeon_dp_probe_oui(radeon_connector);
 
-			return true;
-		}
+		return true;
 	}
+
 	dig_connector->dpcd[0] = 0;
 	return false;
 }
-- 
2.7.4

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

[PATCH 6/7] drm: Add ratelimited versions of the DRM_DEBUG* macros

[Lyude]

There's a couple of places where this would be useful for drivers (such
as reporting DP aux transaction timeouts).

Signed-off-by: Lyude <cpaul@redhat.com>
---
 include/drm/drmP.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index d377865..1c4d91b 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -231,6 +231,36 @@ void drm_err(const char *format, ...);
 			drm_ut_debug_printk(__func__, fmt, ##args);	\
 	} while (0)
 
+#define _DRM_DEFINE_DEBUG_RATELIMITED(level, fmt, args...)		\
+	do {								\
+		if (unlikely(drm_debug & DRM_UT_ ## level)) {		\
+			static DEFINE_RATELIMIT_STATE(			\
+				_rs,					\
+				DEFAULT_RATELIMIT_INTERVAL,		\
+				DEFAULT_RATELIMIT_BURST);		\
+									\
+			if (__ratelimit(&_rs)) {			\
+				drm_ut_debug_printk(__func__, fmt,	\
+						    ##args);		\
+			}						\
+		}							\
+	} while (0)
+
+/**
+ * Rate limited debug output. Like DRM_DEBUG() but won't flood the log.
+ *
+ * \param fmt printf() like format string.
+ * \param arg arguments
+ */
+#define DRM_DEBUG_RATELIMITED(fmt, args...)				\
+	_DRM_DEFINE_DEBUG_RATELIMITED(CORE, fmt, ##args)
+#define DRM_DEBUG_DRIVER_RATELIMITED(fmt, args...)			\
+	_DRM_DEFINE_DEBUG_RATELIMITED(DRIVER, fmt, ##args)
+#define DRM_DEBUG_KMS_RATELIMITED(fmt, args...)				\
+	_DRM_DEFINE_DEBUG_RATELIMITED(KMS, fmt, ##args)
+#define DRM_DEBUG_PRIME_RATELIMITED(fmt, args...)			\
+	_DRM_DEFINE_DEBUG_RATELIMITED(PRIME, fmt, ##args)
+
 /*@}*/
 
 /***********************************************************************/
-- 
2.7.4

_______________________________________________
xorg-driver-ati mailing list
xorg-driver-ati@lists.x.org
https://lists.x.org/mailman/listinfo/xorg-driver-ati

Re: [PATCH 6/7] drm: Add ratelimited versions of the DRM_DEBUG* macros

[Joe Perches]

On Fri, 2016-08-05 at 20:30 -0400, Lyude wrote:
> There's a couple of places where this would be useful for drivers (such
> as reporting DP aux transaction timeouts).

Maybe a single static _rs or one for each type would
be better than an individual _rs per callsite.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/7] drm/dp_helper: Rate limit timeout errors from drm_dp_i2c_do_msg()

[Lyude]

Timeouts can be errors, but timeouts are also usually normal behavior
and happen a lot. Since the kernel already lets us know when we're
suppressing messages due to rate limiting, rate limit timeout errors so
we don't make too much noise in the kernel log.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 43be189..5ca72d25 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -574,7 +574,17 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			if (ret == -EBUSY)
 				continue;
 
-			DRM_DEBUG_KMS("transaction failed: %d\n", ret);
+			/*
+			 * While timeouts can be errors, they're usually normal
+			 * behavior (for instance, when a driver tries to
+			 * communicate with a non-existant DisplayPort device).
+			 * Avoid spamming the kernel log with timeout errors.
+			 */
+			if (ret == -ETIMEDOUT)
+				DRM_DEBUG_KMS_RATELIMITED("transaction timed out\n");
+			else
+				DRM_DEBUG_KMS("transaction failed: %d\n", ret);
+
 			return ret;
 		}
 
-- 
2.7.4

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

Re: [PATCH 0/7] Minor DP aux transaction fixes

[Christian König]

Am 06.08.2016 um 02:30 schrieb Lyude:
> While I was investigating an unrelated bug on the radeon driver, I noticed that
> it's become rather difficult to actually read through dmesg with drm.debug
> turned on, on account of the huge number of messages we end up printing from
> failed DP aux transactions that happen every time we reprobe each connector.
>
> Timed out transactions are relatively normal, and as well there's a lot of
> places in radeon/amdgpu where we're printing redundant debugging information
> dozens of times each time we attempt a DP aux transactions.
>
> Additionally, I've removed some of the retry loops in amdgpu/radeon. These were
> definitely useful at one point, but since we now retry any failed aux
> transaction unconditionally in DRM's dp helpers they don't serve much purpose
> other then to make failing aux transactions take a lot more time then they need
> to.

The whole set is Reviewed-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

>
> Lyude (7):
>    drm/dp_helper: Print first error received on failure in
>      drm_dp_dpcd_access()
>    drm/radeon: Don't print error on aux transaction timeouts
>    drm/radeon: Don't retry 7 times in radeon_dp_dpcd()
>    drm/amdgpu: Don't print error on aux transaction timeouts
>    drm/amdgpu: Don't retry 7 times in amdgpu_atombios_dp_get_dpcd()
>    drm: Add ratelimited versions of the DRM_DEBUG* macros
>    drm/dp_helper: Rate limit timeout errors from drm_dp_i2c_do_msg()
>
>   drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 22 ++++++++++------------
>   drivers/gpu/drm/drm_dp_helper.c          | 14 ++++++++++++--
>   drivers/gpu/drm/radeon/atombios_dp.c     | 21 ++++++++++-----------
>   drivers/gpu/drm/radeon/radeon_dp_auxch.c |  1 -
>   include/drm/drmP.h                       | 30 ++++++++++++++++++++++++++++++
>   5 files changed, 62 insertions(+), 26 deletions(-)
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] Minor DP aux transaction fixes

[Alex Deucher]

On Fri, Aug 5, 2016 at 8:30 PM, Lyude <cpaul@redhat.com> wrote:
> While I was investigating an unrelated bug on the radeon driver, I noticed that
> it's become rather difficult to actually read through dmesg with drm.debug
> turned on, on account of the huge number of messages we end up printing from
> failed DP aux transactions that happen every time we reprobe each connector.
>
> Timed out transactions are relatively normal, and as well there's a lot of
> places in radeon/amdgpu where we're printing redundant debugging information
> dozens of times each time we attempt a DP aux transactions.
>
> Additionally, I've removed some of the retry loops in amdgpu/radeon. These were
> definitely useful at one point, but since we now retry any failed aux
> transaction unconditionally in DRM's dp helpers they don't serve much purpose
> other then to make failing aux transactions take a lot more time then they need
> to.

I've applied the amdgpu and radeon patches.  For the drm patches, I
can either take them through my tree or via drm-misc.

Alex

>
> Lyude (7):
>   drm/dp_helper: Print first error received on failure in
>     drm_dp_dpcd_access()
>   drm/radeon: Don't print error on aux transaction timeouts
>   drm/radeon: Don't retry 7 times in radeon_dp_dpcd()
>   drm/amdgpu: Don't print error on aux transaction timeouts
>   drm/amdgpu: Don't retry 7 times in amdgpu_atombios_dp_get_dpcd()
>   drm: Add ratelimited versions of the DRM_DEBUG* macros
>   drm/dp_helper: Rate limit timeout errors from drm_dp_i2c_do_msg()
>
>  drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 22 ++++++++++------------
>  drivers/gpu/drm/drm_dp_helper.c          | 14 ++++++++++++--
>  drivers/gpu/drm/radeon/atombios_dp.c     | 21 ++++++++++-----------
>  drivers/gpu/drm/radeon/radeon_dp_auxch.c |  1 -
>  include/drm/drmP.h                       | 30 ++++++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+), 26 deletions(-)
>
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/7] Minor DP aux transaction fixes

[Daniel Vetter]

On Mon, Aug 08, 2016 at 01:30:37PM -0400, Alex Deucher wrote:
> On Fri, Aug 5, 2016 at 8:30 PM, Lyude <cpaul@redhat.com> wrote:
> > While I was investigating an unrelated bug on the radeon driver, I noticed that
> > it's become rather difficult to actually read through dmesg with drm.debug
> > turned on, on account of the huge number of messages we end up printing from
> > failed DP aux transactions that happen every time we reprobe each connector.
> >
> > Timed out transactions are relatively normal, and as well there's a lot of
> > places in radeon/amdgpu where we're printing redundant debugging information
> > dozens of times each time we attempt a DP aux transactions.
> >
> > Additionally, I've removed some of the retry loops in amdgpu/radeon. These were
> > definitely useful at one point, but since we now retry any failed aux
> > transaction unconditionally in DRM's dp helpers they don't serve much purpose
> > other then to make failing aux transactions take a lot more time then they need
> > to.
> 
> I've applied the amdgpu and radeon patches.  For the drm patches, I
> can either take them through my tree or via drm-misc.

I applied the 2 core patches to drm-misc, thanks.
-Daniel

> 
> Alex
> 
> >
> > Lyude (7):
> >   drm/dp_helper: Print first error received on failure in
> >     drm_dp_dpcd_access()
> >   drm/radeon: Don't print error on aux transaction timeouts
> >   drm/radeon: Don't retry 7 times in radeon_dp_dpcd()
> >   drm/amdgpu: Don't print error on aux transaction timeouts
> >   drm/amdgpu: Don't retry 7 times in amdgpu_atombios_dp_get_dpcd()
> >   drm: Add ratelimited versions of the DRM_DEBUG* macros
> >   drm/dp_helper: Rate limit timeout errors from drm_dp_i2c_do_msg()
> >
> >  drivers/gpu/drm/amd/amdgpu/atombios_dp.c | 22 ++++++++++------------
> >  drivers/gpu/drm/drm_dp_helper.c          | 14 ++++++++++++--
> >  drivers/gpu/drm/radeon/atombios_dp.c     | 21 ++++++++++-----------
> >  drivers/gpu/drm/radeon/radeon_dp_auxch.c |  1 -
> >  include/drm/drmP.h                       | 30 ++++++++++++++++++++++++++++++
> >  5 files changed, 62 insertions(+), 26 deletions(-)
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel