| Message ID | 1309558978-4704-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Especially after a hotplug or power status change, the sink may not > reply immediately to a link status query. So retry 3 times per the spec > to really make sure nothing is there. There's no 'false' return path here. I think you want: > > --- > drivers/gpu/drm/i915/intel_dp.c | 17 +++++++++++++---- > 1 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 391b55f..1829ecc 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1019,13 +1019,22 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode) > static bool > intel_dp_get_link_status(struct intel_dp *intel_dp) > { > - int ret; > + int ret, i; > + > + /* Must try AUX reads for this at least 3 times */ > + for (i = 0; i < 3; i++) { > + ret = intel_dp_aux_native_read(intel_dp, > + DP_LANE0_1_STATUS, > + intel_dp->link_status, > + DP_LINK_STATUS_SIZE); > + if (ret == DP_LINK_STATUS_SIZE) > + break; return true; > + msleep(1); > + } > > - ret = intel_dp_aux_native_read(intel_dp, > - DP_LANE0_1_STATUS, > - intel_dp->link_status, DP_LINK_STATUS_SIZE); > if (ret != DP_LINK_STATUS_SIZE) > return false; > + > return true; return false; > }
On Fri, 01 Jul 2011 16:41:06 -0700 Keith Packard <keithp@keithp.com> wrote: > On Fri, 1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > Especially after a hotplug or power status change, the sink may not > > reply immediately to a link status query. So retry 3 times per the spec > > to really make sure nothing is there. > > There's no 'false' return path here. I think you want: > > > > --- > > drivers/gpu/drm/i915/intel_dp.c | 17 +++++++++++++---- > > 1 files changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 391b55f..1829ecc 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1019,13 +1019,22 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode) > > static bool > > intel_dp_get_link_status(struct intel_dp *intel_dp) > > { > > - int ret; > > + int ret, i; > > + > > + /* Must try AUX reads for this at least 3 times */ > > + for (i = 0; i < 3; i++) { > > + ret = intel_dp_aux_native_read(intel_dp, > > + DP_LANE0_1_STATUS, > > + intel_dp->link_status, > > + DP_LINK_STATUS_SIZE); > > + if (ret == DP_LINK_STATUS_SIZE) > > + break; > > return true; > > + msleep(1); > > + } > > > > - ret = intel_dp_aux_native_read(intel_dp, > > - DP_LANE0_1_STATUS, > > - intel_dp->link_status, DP_LINK_STATUS_SIZE); > > if (ret != DP_LINK_STATUS_SIZE) > > return false; I think we'd hit the "return false" above if the loop failed. But simply returning true from the loop and false otherwise is clearer and fewer lines. Will fix.
On Fri, 1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > Especially after a hotplug or power status change, the sink may not > reply immediately to a link status query. So retry 3 times per the spec > to really make sure nothing is there. One thing we've been trying to do increasingly on the 3D side is cite chapter and verse in comments when we're doing something "per the spec" (which spec here?). Clarifies whether it was some crazy workaround copying what the BIOS did at one point vs. something that we really are supposed to do.
On Tue, 05 Jul 2011 21:27:35 -0700 Eric Anholt <eric@anholt.net> wrote: > On Fri, 1 Jul 2011 15:22:51 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > Especially after a hotplug or power status change, the sink may not > > reply immediately to a link status query. So retry 3 times per the spec > > to really make sure nothing is there. > > One thing we've been trying to do increasingly on the 3D side is cite > chapter and verse in comments when we're doing something "per the spec" > (which spec here?). Clarifies whether it was some crazy workaround > copying what the BIOS did at one point vs. something that we really are > supposed to do. Yeah good point, I'll point at the DP spec section stuff here when I re-post. Thanks,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 391b55f..1829ecc 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1019,13 +1019,22 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode) static bool intel_dp_get_link_status(struct intel_dp *intel_dp) { - int ret; + int ret, i; + + /* Must try AUX reads for this at least 3 times */ + for (i = 0; i < 3; i++) { + ret = intel_dp_aux_native_read(intel_dp, + DP_LANE0_1_STATUS, + intel_dp->link_status, + DP_LINK_STATUS_SIZE); + if (ret == DP_LINK_STATUS_SIZE) + break; + msleep(1); + } - ret = intel_dp_aux_native_read(intel_dp, - DP_LANE0_1_STATUS, - intel_dp->link_status, DP_LINK_STATUS_SIZE); if (ret != DP_LINK_STATUS_SIZE) return false; + return true; }
Especially after a hotplug or power status change, the sink may not reply immediately to a link status query. So retry 3 times per the spec to really make sure nothing is there. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_dp.c | 17 +++++++++++++---- 1 files changed, 13 insertions(+), 4 deletions(-)