| Message ID | 20180416155309.11100-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/16/2018 08:53 AM, Imre Deak wrote: > LSPCON adapters in low-power state may ignore the first I2C write during > TMDS output buffer enabling, resulting in a blank screen even with an > otherwise enabled pipe. Fix this by reading back and validating the > written value a few times. > > The problem was noticed on GLK machines with an onboard LSPCON adapter > after entering/exiting DC5 power state. Doing an I2C read of the adapter > ID as the first transaction - instead of the I2C write to enable the > TMDS buffers - returns the correct value. Based on this we assume that > the transaction itself is sent properly, it's only the adapter that is > not ready for some reason to accept this first write after waking from > low-power state. In my case the second I2C write attempt always > succeeded. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105854 > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/drm_dp_dual_mode_helper.c | 39 +++++++++++++++++++++++++------ > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c > index 02a50929af67..e7f4fe2848a5 100644 > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c > @@ -350,19 +350,44 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type, > { > uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE; > ssize_t ret; > + int retry; > > if (type < DRM_DP_DUAL_MODE_TYPE2_DVI) > return 0; > > - ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, > - &tmds_oen, sizeof(tmds_oen)); > - if (ret) { > - DRM_DEBUG_KMS("Failed to %s TMDS output buffers\n", > - enable ? "enable" : "disable"); > - return ret; > + /* > + * LSPCON adapters in low-power state may ignore the first write, so > + * read back and verify the written value a few times. > + */ > + for (retry = 0; retry < 3; retry++) { > + uint8_t tmp; > + > + ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, > + &tmds_oen, sizeof(tmds_oen)); > + if (ret) { > + DRM_DEBUG_KMS("Failed to %s TMDS output buffers (%d attempts)\n", > + enable ? "enable" : "disable", > + retry + 1); > + return ret; > + } > + > + ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN, > + &tmp, sizeof(tmp)); > + if (ret) { > + DRM_DEBUG_KMS("I2C read failed during TMDS output buffer %s (%d attempts)\n", > + enable ? "enabling" : "disabling", > + retry + 1); > + return ret; > + } > + > + if (tmp == tmds_oen) > + return 0; > } > > - return 0; > + DRM_DEBUG_KMS("I2C write value mismatch during TMDS output buffer %s\n", > + enable ? "enabling" : "disabling"); > + > + return -EIO; > } > EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output); > Appears to fix the issue seen on GLK with LSPCON and DMC firmware loaded. Customer was concerned about the fix being in DRM instead of i915. However, there are no other SOCs that use this DRM function. Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com> Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com> -Clint
On Mon, Apr 16, 2018 at 06:53:09PM +0300, Imre Deak wrote: > LSPCON adapters in low-power state may ignore the first I2C write during > TMDS output buffer enabling, resulting in a blank screen even with an > otherwise enabled pipe. Fix this by reading back and validating the > written value a few times. > > The problem was noticed on GLK machines with an onboard LSPCON adapter > after entering/exiting DC5 power state. Doing an I2C read of the adapter > ID as the first transaction - instead of the I2C write to enable the > TMDS buffers - returns the correct value. Based on this we assume that > the transaction itself is sent properly, it's only the adapter that is > not ready for some reason to accept this first write after waking from > low-power state. In my case the second I2C write attempt always > succeeded. Yeah, I guess this is an OK approach. Probably not much point in optimizing the non-LSPCON case since this is modeset stuff, although we do have the adaptor type passed in so we could if we wanted to. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105854 > Cc: Clinton Taylor <clinton.a.taylor@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/drm_dp_dual_mode_helper.c | 39 +++++++++++++++++++++++++------ > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c > index 02a50929af67..e7f4fe2848a5 100644 > --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c > +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c > @@ -350,19 +350,44 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type, > { > uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE; > ssize_t ret; > + int retry; > > if (type < DRM_DP_DUAL_MODE_TYPE2_DVI) > return 0; > > - ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, > - &tmds_oen, sizeof(tmds_oen)); > - if (ret) { > - DRM_DEBUG_KMS("Failed to %s TMDS output buffers\n", > - enable ? "enable" : "disable"); > - return ret; > + /* > + * LSPCON adapters in low-power state may ignore the first write, so > + * read back and verify the written value a few times. > + */ > + for (retry = 0; retry < 3; retry++) { > + uint8_t tmp; > + > + ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, > + &tmds_oen, sizeof(tmds_oen)); > + if (ret) { > + DRM_DEBUG_KMS("Failed to %s TMDS output buffers (%d attempts)\n", > + enable ? "enable" : "disable", > + retry + 1); > + return ret; > + } > + > + ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN, > + &tmp, sizeof(tmp)); > + if (ret) { > + DRM_DEBUG_KMS("I2C read failed during TMDS output buffer %s (%d attempts)\n", > + enable ? "enabling" : "disabling", > + retry + 1); > + return ret; > + } > + > + if (tmp == tmds_oen) > + return 0; > } > > - return 0; > + DRM_DEBUG_KMS("I2C write value mismatch during TMDS output buffer %s\n", > + enable ? "enabling" : "disabling"); > + > + return -EIO; > } > EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output); > > -- > 2.13.2
On Mon, 16 Apr 2018, Clint Taylor <clinton.a.taylor@intel.com> wrote: > On 04/16/2018 08:53 AM, Imre Deak wrote: >> LSPCON adapters in low-power state may ignore the first I2C write during >> TMDS output buffer enabling, resulting in a blank screen even with an >> otherwise enabled pipe. Fix this by reading back and validating the >> written value a few times. >> >> The problem was noticed on GLK machines with an onboard LSPCON adapter >> after entering/exiting DC5 power state. Doing an I2C read of the adapter >> ID as the first transaction - instead of the I2C write to enable the >> TMDS buffers - returns the correct value. Based on this we assume that >> the transaction itself is sent properly, it's only the adapter that is >> not ready for some reason to accept this first write after waking from >> low-power state. In my case the second I2C write attempt always >> succeeded. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105854 >> Cc: Clinton Taylor <clinton.a.taylor@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Imre Deak <imre.deak@intel.com> >> --- >> drivers/gpu/drm/drm_dp_dual_mode_helper.c | 39 +++++++++++++++++++++++++------ >> 1 file changed, 32 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c >> index 02a50929af67..e7f4fe2848a5 100644 >> --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c >> +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c >> @@ -350,19 +350,44 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type, >> { >> uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE; >> ssize_t ret; >> + int retry; >> >> if (type < DRM_DP_DUAL_MODE_TYPE2_DVI) >> return 0; >> >> - ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, >> - &tmds_oen, sizeof(tmds_oen)); >> - if (ret) { >> - DRM_DEBUG_KMS("Failed to %s TMDS output buffers\n", >> - enable ? "enable" : "disable"); >> - return ret; >> + /* >> + * LSPCON adapters in low-power state may ignore the first write, so >> + * read back and verify the written value a few times. >> + */ >> + for (retry = 0; retry < 3; retry++) { >> + uint8_t tmp; >> + >> + ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, >> + &tmds_oen, sizeof(tmds_oen)); >> + if (ret) { >> + DRM_DEBUG_KMS("Failed to %s TMDS output buffers (%d attempts)\n", >> + enable ? "enable" : "disable", >> + retry + 1); >> + return ret; >> + } >> + >> + ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN, >> + &tmp, sizeof(tmp)); >> + if (ret) { >> + DRM_DEBUG_KMS("I2C read failed during TMDS output buffer %s (%d attempts)\n", >> + enable ? "enabling" : "disabling", >> + retry + 1); >> + return ret; >> + } >> + >> + if (tmp == tmds_oen) >> + return 0; >> } >> >> - return 0; >> + DRM_DEBUG_KMS("I2C write value mismatch during TMDS output buffer %s\n", >> + enable ? "enabling" : "disabling"); >> + >> + return -EIO; >> } >> EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output); >> > > Appears to fix the issue seen on GLK with LSPCON and DMC firmware > loaded. Customer was concerned about the fix being in DRM instead of > i915. However, there are no other SOCs that use this DRM function. > > Reviewed-by: Clint Taylor <Clinton.A.Taylor@intel.com> > Tested-by: Clint Taylor <Clinton.A.Taylor@intel.com> Pushed to drm-misc-fixes, thanks for the patch. Clint and Ville, many thanks for your testing and review, alas I screwed up and pushed this without the tags added to the commit. I'm sorry. I expect better from myself. :( BR, Jani. > > -Clint > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/drm_dp_dual_mode_helper.c b/drivers/gpu/drm/drm_dp_dual_mode_helper.c index 02a50929af67..e7f4fe2848a5 100644 --- a/drivers/gpu/drm/drm_dp_dual_mode_helper.c +++ b/drivers/gpu/drm/drm_dp_dual_mode_helper.c @@ -350,19 +350,44 @@ int drm_dp_dual_mode_set_tmds_output(enum drm_dp_dual_mode_type type, { uint8_t tmds_oen = enable ? 0 : DP_DUAL_MODE_TMDS_DISABLE; ssize_t ret; + int retry; if (type < DRM_DP_DUAL_MODE_TYPE2_DVI) return 0; - ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, - &tmds_oen, sizeof(tmds_oen)); - if (ret) { - DRM_DEBUG_KMS("Failed to %s TMDS output buffers\n", - enable ? "enable" : "disable"); - return ret; + /* + * LSPCON adapters in low-power state may ignore the first write, so + * read back and verify the written value a few times. + */ + for (retry = 0; retry < 3; retry++) { + uint8_t tmp; + + ret = drm_dp_dual_mode_write(adapter, DP_DUAL_MODE_TMDS_OEN, + &tmds_oen, sizeof(tmds_oen)); + if (ret) { + DRM_DEBUG_KMS("Failed to %s TMDS output buffers (%d attempts)\n", + enable ? "enable" : "disable", + retry + 1); + return ret; + } + + ret = drm_dp_dual_mode_read(adapter, DP_DUAL_MODE_TMDS_OEN, + &tmp, sizeof(tmp)); + if (ret) { + DRM_DEBUG_KMS("I2C read failed during TMDS output buffer %s (%d attempts)\n", + enable ? "enabling" : "disabling", + retry + 1); + return ret; + } + + if (tmp == tmds_oen) + return 0; } - return 0; + DRM_DEBUG_KMS("I2C write value mismatch during TMDS output buffer %s\n", + enable ? "enabling" : "disabling"); + + return -EIO; } EXPORT_SYMBOL(drm_dp_dual_mode_set_tmds_output);
LSPCON adapters in low-power state may ignore the first I2C write during TMDS output buffer enabling, resulting in a blank screen even with an otherwise enabled pipe. Fix this by reading back and validating the written value a few times. The problem was noticed on GLK machines with an onboard LSPCON adapter after entering/exiting DC5 power state. Doing an I2C read of the adapter ID as the first transaction - instead of the I2C write to enable the TMDS buffers - returns the correct value. Based on this we assume that the transaction itself is sent properly, it's only the adapter that is not ready for some reason to accept this first write after waking from low-power state. In my case the second I2C write attempt always succeeded. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105854 Cc: Clinton Taylor <clinton.a.taylor@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/drm_dp_dual_mode_helper.c | 39 +++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 7 deletions(-)