From mboxrd@z Thu Jan 1 00:00:00 1970 From: Todd Previte Subject: Re: [PATCH] drm/i915: Retry reading DPCD when bogus values are read Date: Wed, 22 Jan 2014 17:59:18 -0700 Message-ID: <52E06966.2040006@gmail.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pb0-f42.google.com (mail-pb0-f42.google.com [209.85.160.42]) by gabe.freedesktop.org (Postfix) with ESMTP id E3441FA2C3 for ; Wed, 22 Jan 2014 16:59:21 -0800 (PST) Received: by mail-pb0-f42.google.com with SMTP id jt11so1146686pbb.1 for ; Wed, 22 Jan 2014 16:59:21 -0800 (PST) Received: from [192.168.1.120] (ip70-162-72-129.ph.ph.cox.net. [70.162.72.129]) by mx.google.com with ESMTPSA id qq5sm28876735pbb.24.2014.01.22.16.59.19 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 22 Jan 2014 16:59:20 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 11/22/13 1:36 AM, Takashi Iwai wrote: > I got kernel WARNINGs frequently on Haswell laptops complaining about > invalid max DP link bw. With drm.debug=0x0e, it turned out that the > obtained DPCD is utterly bogus when it happens: > [drm:intel_dp_get_dpcd], DPCD: 4d 4d 4d 4d 4d 4d 4d 4d 4d 4d 4d 4d 4d 4d 4d > > This seems happening intermittently without plug state changes and > even after the first few reads succeeded. As a workaround, add a > sanity check for such bogus values and retry reading if it hits. > > Signed-off-by: Takashi Iwai > --- > > drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index eb8139da9763..f596353da557 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2792,13 +2792,26 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_device *dev = dig_port->base.base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - > + int retry = 0; > char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3]; > > + again: > if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, > sizeof(intel_dp->dpcd)) == 0) > return false; /* aux transfer failed */ > > + /* sanity check: sometimes DPCD contains a series of same bogus value > + * even though the above returned success. Retry reading in that case. > + */ > + if (intel_dp->dpcd[0]) { > + int i; > + for (i = 1; i < sizeof(intel_dp->dpcd); i++) > + if (intel_dp->dpcd[i] != intel_dp->dpcd[0]) > + break; > + if (i >= sizeof(intel_dp->dpcd) && retry++ < 2) > + goto again; > + } > + > hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd), > 32, 1, dpcd_hex_dump, sizeof(dpcd_hex_dump), false); > DRM_DEBUG_KMS("DPCD: %s\n", dpcd_hex_dump); I'm in agreement with Jani's assessment. While it does address the particular problem at hand, it just looks for duplicate data and issues a retry if that specific condition is detected. It seems more appropriate that instead of checking for specific failure modes, we check to ensure that we have useable values for the basic, critical fields required for Displayport. I would suggest that we validate the first 3 bytes of the DPCD. Those fields are as follows: 0x00 - DPCD revision level Currently can only be 0x10, 0x11 or 0x12 0x01 - Link rate Also only 3 values currently - 0x06, 0x0A and 0x14 0x02 - Max lane count For validation purposes, we just need to look at 4:0 and verify it's 0x01, 0x02 or 0x04 While bits 6 and 7 are used, they're just feature bits and we check them later anyways. This is kind of what I was thinking: #define MAX_DPCD_REVISION 0x12 u8 lane_count = DPCD[2] & 0x1F; // Verify DPCD revision is in a valid range. MAX_DPCD_REVISION should be updated as necessasry // to reflect the current supported DPCD version. if (DPCD[0] < 0x10 || DPCD > MAX_DPCD_REVISION) goto retry; // These are the only valid values for link rate. More will surely be added as Displayport moves forward. if !(DPCD[1] == 0x06 || DPCD[1] == 0x0A || DPCD[1] == 0x14) goto retry; // Lane count must be 1, 2 or 4. 3 shalt thou not count, and 5 is right out... if !(lane_count == 0x01 || lane_count == 0x02 || lane_count == 0x04) goto retry; retry: retry_count++; if retry_count > max_retries goto error else goto read_dpcd -T