| Message ID | 1485509961-9010-4-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Regards Shashank On 1/27/2017 3:09 PM, Imre Deak wrote: > This effectively reverts > commit 489375c866c111f16cea93b2467ebe59c9022cc7 > Author: Imre Deak <imre.deak@intel.com> > Date: Mon Oct 24 19:33:31 2016 +0300 > > drm/i915/lspcon: Add workaround for resuming in PCON mode > > The workaround was added without considering that HPD is low during > the failed AUX transfers the WA fixed. Since the previous patch we > wait for HPD to get asserted. My tests also show that this happens > _after_ the DPCD reads start to return correct values. This > suggests that we don't need this WA any more, let's try to remove > it to reduce the clutter. > > Cc: Shashank Sharma <shashank.sharma@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 1 - > drivers/gpu/drm/i915/intel_lspcon.c | 17 ++--------------- > 2 files changed, 2 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index b9cde11..b2882ff 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -989,7 +989,6 @@ struct intel_dp { > struct intel_lspcon { > bool active; > enum drm_lspcon_mode mode; > - bool desc_valid; > }; > > struct intel_digital_port { > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > index c300647..71cbe9c 100644 > --- a/drivers/gpu/drm/i915/intel_lspcon.c > +++ b/drivers/gpu/drm/i915/intel_lspcon.c > @@ -162,21 +162,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon) > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > unsigned long start = jiffies; > > - if (!lspcon->desc_valid) > - return; > - > while (1) { > - struct intel_dp_desc desc; > - > - /* > - * The w/a only applies in PCON mode and we don't expect any > - * AUX errors. > - */ > - if (!__intel_dp_read_desc(intel_dp, &desc)) > - return; > - > - if (intel_digital_port_connected(dev_priv, dig_port) && > - !memcmp(&intel_dp->desc, &desc, sizeof(desc))) { > + if (intel_digital_port_connected(dev_priv, dig_port)) { Again, does it matter, as in PCON mode live_status will be always true ? - Shashank > DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n", > jiffies_to_msecs(jiffies - start)); > return; > @@ -253,7 +240,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > return false; > } > > - lspcon->desc_valid = intel_dp_read_desc(dp); > + intel_dp_read_desc(dp); > > DRM_DEBUG_KMS("Success: LSPCON init\n"); > return true;
On Sat, Jan 28, 2017 at 10:36:16AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 1/27/2017 3:09 PM, Imre Deak wrote: > >This effectively reverts > >commit 489375c866c111f16cea93b2467ebe59c9022cc7 > >Author: Imre Deak <imre.deak@intel.com> > >Date: Mon Oct 24 19:33:31 2016 +0300 > > > > drm/i915/lspcon: Add workaround for resuming in PCON mode > > > >The workaround was added without considering that HPD is low during > >the failed AUX transfers the WA fixed. Since the previous patch we > >wait for HPD to get asserted. My tests also show that this happens > >_after_ the DPCD reads start to return correct values. This > >suggests that we don't need this WA any more, let's try to remove > >it to reduce the clutter. > > > >Cc: Shashank Sharma <shashank.sharma@intel.com> > >Cc: Jani Nikula <jani.nikula@linux.intel.com> > >Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >Signed-off-by: Imre Deak <imre.deak@intel.com> > >--- > > drivers/gpu/drm/i915/intel_drv.h | 1 - > > drivers/gpu/drm/i915/intel_lspcon.c | 17 ++--------------- > > 2 files changed, 2 insertions(+), 16 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >index b9cde11..b2882ff 100644 > >--- a/drivers/gpu/drm/i915/intel_drv.h > >+++ b/drivers/gpu/drm/i915/intel_drv.h > >@@ -989,7 +989,6 @@ struct intel_dp { > > struct intel_lspcon { > > bool active; > > enum drm_lspcon_mode mode; > >- bool desc_valid; > > }; > > struct intel_digital_port { > >diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c > >index c300647..71cbe9c 100644 > >--- a/drivers/gpu/drm/i915/intel_lspcon.c > >+++ b/drivers/gpu/drm/i915/intel_lspcon.c > >@@ -162,21 +162,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon) > > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > unsigned long start = jiffies; > >- if (!lspcon->desc_valid) > >- return; > >- > > while (1) { > >- struct intel_dp_desc desc; > >- > >- /* > >- * The w/a only applies in PCON mode and we don't expect any > >- * AUX errors. > >- */ > >- if (!__intel_dp_read_desc(intel_dp, &desc)) > >- return; > >- > >- if (intel_digital_port_connected(dev_priv, dig_port) && > >- !memcmp(&intel_dp->desc, &desc, sizeof(desc))) { > >+ if (intel_digital_port_connected(dev_priv, dig_port)) { > Again, does it matter, as in PCON mode live_status will be always true ? See my answer for the previous patch. > - Shashank > > DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n", > > jiffies_to_msecs(jiffies - start)); > > return; > >@@ -253,7 +240,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) > > return false; > > } > >- lspcon->desc_valid = intel_dp_read_desc(dp); > >+ intel_dp_read_desc(dp); > > DRM_DEBUG_KMS("Success: LSPCON init\n"); > > return true; >
Reviewed-by: Shashank Sharma Regards Shashank On 1/28/2017 1:49 PM, Imre Deak wrote: > On Sat, Jan 28, 2017 at 10:36:16AM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 1/27/2017 3:09 PM, Imre Deak wrote: >>> This effectively reverts >>> commit 489375c866c111f16cea93b2467ebe59c9022cc7 >>> Author: Imre Deak <imre.deak@intel.com> >>> Date: Mon Oct 24 19:33:31 2016 +0300 >>> >>> drm/i915/lspcon: Add workaround for resuming in PCON mode >>> >>> The workaround was added without considering that HPD is low during >>> the failed AUX transfers the WA fixed. Since the previous patch we >>> wait for HPD to get asserted. My tests also show that this happens >>> _after_ the DPCD reads start to return correct values. This >>> suggests that we don't need this WA any more, let's try to remove >>> it to reduce the clutter. >>> >>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Imre Deak <imre.deak@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_drv.h | 1 - >>> drivers/gpu/drm/i915/intel_lspcon.c | 17 ++--------------- >>> 2 files changed, 2 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >>> index b9cde11..b2882ff 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -989,7 +989,6 @@ struct intel_dp { >>> struct intel_lspcon { >>> bool active; >>> enum drm_lspcon_mode mode; >>> - bool desc_valid; >>> }; >>> struct intel_digital_port { >>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c >>> index c300647..71cbe9c 100644 >>> --- a/drivers/gpu/drm/i915/intel_lspcon.c >>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c >>> @@ -162,21 +162,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon) >>> struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); >>> unsigned long start = jiffies; >>> - if (!lspcon->desc_valid) >>> - return; >>> - >>> while (1) { >>> - struct intel_dp_desc desc; >>> - >>> - /* >>> - * The w/a only applies in PCON mode and we don't expect any >>> - * AUX errors. >>> - */ >>> - if (!__intel_dp_read_desc(intel_dp, &desc)) >>> - return; >>> - >>> - if (intel_digital_port_connected(dev_priv, dig_port) && >>> - !memcmp(&intel_dp->desc, &desc, sizeof(desc))) { >>> + if (intel_digital_port_connected(dev_priv, dig_port)) { >> Again, does it matter, as in PCON mode live_status will be always true ? > See my answer for the previous patch. > >> - Shashank >>> DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n", >>> jiffies_to_msecs(jiffies - start)); >>> return; >>> @@ -253,7 +240,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) >>> return false; >>> } >>> - lspcon->desc_valid = intel_dp_read_desc(dp); >>> + intel_dp_read_desc(dp); >>> DRM_DEBUG_KMS("Success: LSPCON init\n"); >>> return true;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index b9cde11..b2882ff 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -989,7 +989,6 @@ struct intel_dp { struct intel_lspcon { bool active; enum drm_lspcon_mode mode; - bool desc_valid; }; struct intel_digital_port { diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c index c300647..71cbe9c 100644 --- a/drivers/gpu/drm/i915/intel_lspcon.c +++ b/drivers/gpu/drm/i915/intel_lspcon.c @@ -162,21 +162,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon) struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); unsigned long start = jiffies; - if (!lspcon->desc_valid) - return; - while (1) { - struct intel_dp_desc desc; - - /* - * The w/a only applies in PCON mode and we don't expect any - * AUX errors. - */ - if (!__intel_dp_read_desc(intel_dp, &desc)) - return; - - if (intel_digital_port_connected(dev_priv, dig_port) && - !memcmp(&intel_dp->desc, &desc, sizeof(desc))) { + if (intel_digital_port_connected(dev_priv, dig_port)) { DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n", jiffies_to_msecs(jiffies - start)); return; @@ -253,7 +240,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port) return false; } - lspcon->desc_valid = intel_dp_read_desc(dp); + intel_dp_read_desc(dp); DRM_DEBUG_KMS("Success: LSPCON init\n"); return true;
This effectively reverts commit 489375c866c111f16cea93b2467ebe59c9022cc7 Author: Imre Deak <imre.deak@intel.com> Date: Mon Oct 24 19:33:31 2016 +0300 drm/i915/lspcon: Add workaround for resuming in PCON mode The workaround was added without considering that HPD is low during the failed AUX transfers the WA fixed. Since the previous patch we wait for HPD to get asserted. My tests also show that this happens _after_ the DPCD reads start to return correct values. This suggests that we don't need this WA any more, let's try to remove it to reduce the clutter. Cc: Shashank Sharma <shashank.sharma@intel.com> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_lspcon.c | 17 ++--------------- 2 files changed, 2 insertions(+), 16 deletions(-)