| Message ID | 1353671876-13852-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Some devices may respond very slowly and only flag that the reply is > pending within the first 15us response window. Be kind to such devices > and wait a further 15ms, before checking for the pending reply. This > moves the existing special case delay of 30ms down from the detection > routine into the common path and pretends to explain it... > > v2: Simplify the loop constructs as suggested by Jani Nikula. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_sdvo.c | 31 +++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > index d85ebb0..cff3c0b 100644 > --- a/drivers/gpu/drm/i915/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > @@ -509,7 +509,7 @@ out: > static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > void *response, int response_len) > { > - u8 retry = 5; > + u8 retry = 15; /* 5 quick checks, followed by 10 long checks */ > u8 status; > int i; > > @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > * command to be complete. > * > * Check 5 times in case the hardware failed to read the docs. > + * > + * Also beware that the first response by many devices is to > + * reply PENDING and stall for time. TVs are notorious for > + * requiring longer than specified to complete their replies. > + * Originally (in the DDX long ago), the delay was only ever 15ms > + * with an additional delay of 30ms applied for TVs added later after > + * many experiments. To accommodate both sets of delays, we do a > + * sequence of slow checks if the device is falling behind and fails > + * to reply within 5*15µs. > */ > if (!intel_sdvo_read_byte(intel_sdvo, > SDVO_I2C_CMD_STATUS, > &status)) > goto log_fail; > > - while (status == SDVO_CMD_STATUS_PENDING && retry--) { > - udelay(15); > + while (status == SDVO_CMD_STATUS_PENDING && --retry) { Hey, why did you switch from post to pre decrement? It will now retry only retry-1 times. Or is this about the semantics of retries vs. tries? ;) Otherwise, Reviewed-by: Jani Nikula <jani.nikula@intel.com> > + if (retry < 10) > + msleep(15); > + else > + udelay(15); > + > if (!intel_sdvo_read_byte(intel_sdvo, > SDVO_I2C_CMD_STATUS, > &status)) > @@ -1535,15 +1548,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force) > struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector); > enum drm_connector_status ret; > > - if (!intel_sdvo_write_cmd(intel_sdvo, > - SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL, 0)) > - return connector_status_unknown; > - > - /* add 30ms delay when the output type might be TV */ > - if (intel_sdvo->caps.output_flags & SDVO_TV_MASK) > - msleep(30); > - > - if (!intel_sdvo_read_response(intel_sdvo, &response, 2)) > + if (!intel_sdvo_get_value(intel_sdvo, > + SDVO_CMD_GET_ATTACHED_DISPLAYS, > + &response, 2)) > return connector_status_unknown; > > DRM_DEBUG_KMS("SDVO response %d %d [%x]\n", > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 23 Nov 2012 14:21:58 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Some devices may respond very slowly and only flag that the reply is > > pending within the first 15us response window. Be kind to such devices > > and wait a further 15ms, before checking for the pending reply. This > > moves the existing special case delay of 30ms down from the detection > > routine into the common path and pretends to explain it... > > > > v2: Simplify the loop constructs as suggested by Jani Nikula. > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_sdvo.c | 31 +++++++++++++++++++------------ > > 1 file changed, 19 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c > > index d85ebb0..cff3c0b 100644 > > --- a/drivers/gpu/drm/i915/intel_sdvo.c > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c > > @@ -509,7 +509,7 @@ out: > > static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > > void *response, int response_len) > > { > > - u8 retry = 5; > > + u8 retry = 15; /* 5 quick checks, followed by 10 long checks */ > > u8 status; > > int i; > > > > @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, > > * command to be complete. > > * > > * Check 5 times in case the hardware failed to read the docs. > > + * > > + * Also beware that the first response by many devices is to > > + * reply PENDING and stall for time. TVs are notorious for > > + * requiring longer than specified to complete their replies. > > + * Originally (in the DDX long ago), the delay was only ever 15ms > > + * with an additional delay of 30ms applied for TVs added later after > > + * many experiments. To accommodate both sets of delays, we do a > > + * sequence of slow checks if the device is falling behind and fails > > + * to reply within 5*15µs. > > */ > > if (!intel_sdvo_read_byte(intel_sdvo, > > SDVO_I2C_CMD_STATUS, > > &status)) > > goto log_fail; > > > > - while (status == SDVO_CMD_STATUS_PENDING && retry--) { > > - udelay(15); > > + while (status == SDVO_CMD_STATUS_PENDING && --retry) { > > Hey, why did you switch from post to pre decrement? It will now retry > only retry-1 times. Or is this about the semantics of retries vs. tries? > ;) Because on the last go through, inside the loop retry would be 255 and we would not get the final 15ms sleep. -Chris
On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, 23 Nov 2012 14:21:58 +0200, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> On Fri, 23 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > Some devices may respond very slowly and only flag that the reply is >> > pending within the first 15us response window. Be kind to such devices >> > and wait a further 15ms, before checking for the pending reply. This >> > moves the existing special case delay of 30ms down from the detection >> > routine into the common path and pretends to explain it... >> > >> > v2: Simplify the loop constructs as suggested by Jani Nikula. >> > >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997 >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> > --- >> > drivers/gpu/drm/i915/intel_sdvo.c | 31 +++++++++++++++++++------------ >> > 1 file changed, 19 insertions(+), 12 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c >> > index d85ebb0..cff3c0b 100644 >> > --- a/drivers/gpu/drm/i915/intel_sdvo.c >> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c >> > @@ -509,7 +509,7 @@ out: >> > static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, >> > void *response, int response_len) >> > { >> > - u8 retry = 5; >> > + u8 retry = 15; /* 5 quick checks, followed by 10 long checks */ >> > u8 status; >> > int i; >> > >> > @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, >> > * command to be complete. >> > * >> > * Check 5 times in case the hardware failed to read the docs. >> > + * >> > + * Also beware that the first response by many devices is to >> > + * reply PENDING and stall for time. TVs are notorious for >> > + * requiring longer than specified to complete their replies. >> > + * Originally (in the DDX long ago), the delay was only ever 15ms >> > + * with an additional delay of 30ms applied for TVs added later after >> > + * many experiments. To accommodate both sets of delays, we do a >> > + * sequence of slow checks if the device is falling behind and fails >> > + * to reply within 5*15µs. >> > */ >> > if (!intel_sdvo_read_byte(intel_sdvo, >> > SDVO_I2C_CMD_STATUS, >> > &status)) >> > goto log_fail; >> > >> > - while (status == SDVO_CMD_STATUS_PENDING && retry--) { >> > - udelay(15); >> > + while (status == SDVO_CMD_STATUS_PENDING && --retry) { >> >> Hey, why did you switch from post to pre decrement? It will now retry >> only retry-1 times. Or is this about the semantics of retries vs. tries? >> ;) > > Because on the last go through, inside the loop retry would be 255 and > we would not get the final 15ms sleep. Right. The r-b applies. Jani.
On Fri, Nov 23, 2012 at 11:57:56AM +0000, Chris Wilson wrote: > Some devices may respond very slowly and only flag that the reply is > pending within the first 15us response window. Be kind to such devices > and wait a further 15ms, before checking for the pending reply. This > moves the existing special case delay of 30ms down from the detection > routine into the common path and pretends to explain it... > > v2: Simplify the loop constructs as suggested by Jani Nikula. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=36997 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index d85ebb0..cff3c0b 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -509,7 +509,7 @@ out: static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, void *response, int response_len) { - u8 retry = 5; + u8 retry = 15; /* 5 quick checks, followed by 10 long checks */ u8 status; int i; @@ -522,14 +522,27 @@ static bool intel_sdvo_read_response(struct intel_sdvo *intel_sdvo, * command to be complete. * * Check 5 times in case the hardware failed to read the docs. + * + * Also beware that the first response by many devices is to + * reply PENDING and stall for time. TVs are notorious for + * requiring longer than specified to complete their replies. + * Originally (in the DDX long ago), the delay was only ever 15ms + * with an additional delay of 30ms applied for TVs added later after + * many experiments. To accommodate both sets of delays, we do a + * sequence of slow checks if the device is falling behind and fails + * to reply within 5*15µs. */ if (!intel_sdvo_read_byte(intel_sdvo, SDVO_I2C_CMD_STATUS, &status)) goto log_fail; - while (status == SDVO_CMD_STATUS_PENDING && retry--) { - udelay(15); + while (status == SDVO_CMD_STATUS_PENDING && --retry) { + if (retry < 10) + msleep(15); + else + udelay(15); + if (!intel_sdvo_read_byte(intel_sdvo, SDVO_I2C_CMD_STATUS, &status)) @@ -1535,15 +1548,9 @@ intel_sdvo_detect(struct drm_connector *connector, bool force) struct intel_sdvo_connector *intel_sdvo_connector = to_intel_sdvo_connector(connector); enum drm_connector_status ret; - if (!intel_sdvo_write_cmd(intel_sdvo, - SDVO_CMD_GET_ATTACHED_DISPLAYS, NULL, 0)) - return connector_status_unknown; - - /* add 30ms delay when the output type might be TV */ - if (intel_sdvo->caps.output_flags & SDVO_TV_MASK) - msleep(30); - - if (!intel_sdvo_read_response(intel_sdvo, &response, 2)) + if (!intel_sdvo_get_value(intel_sdvo, + SDVO_CMD_GET_ATTACHED_DISPLAYS, + &response, 2)) return connector_status_unknown; DRM_DEBUG_KMS("SDVO response %d %d [%x]\n",
Some devices may respond very slowly and only flag that the reply is pending within the first 15us response window. Be kind to such devices and wait a further 15ms, before checking for the pending reply. This moves the existing special case delay of 30ms down from the detection routine into the common path and pretends to explain it... v2: Simplify the loop constructs as suggested by Jani Nikula. References: https://bugs.freedesktop.org/show_bug.cgi?id=36997 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/intel_sdvo.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)