Re: [Intel-gfx] [PATCH] drm/i915/display/tgl: Implement Wa_14013120569

["Tolakanahalli Pradeep, Madhumitha" <madhumitha.tolakanahalli.pradeep@intel.com>]

On Tue, 2021-06-29 at 22:25 +0000, Souza, Jose wrote:
> On Mon, 2021-06-28 at 16:50 -0700, Madhumitha Tolakanahalli Pradeep
> wrote:
> > PCH display HPD IRQ is not detected with default filter value.
> > So, PP_CONTROL is manually reprogrammed.
> > 
> > Signed-off-by: Madhumitha Tolakanahalli Pradeep <
> > madhumitha.tolakanahalli.pradeep@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_power.c   |  8 ++++++++
> >  drivers/gpu/drm/i915/display/intel_hotplug.c     | 16
> > ++++++++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 285380079aab..e44323cc76f5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -6385,8 +6385,16 @@ static void
> > intel_power_domains_verify_state(struct drm_i915_private *i915)
> > 
> >  void intel_display_power_suspend_late(struct drm_i915_private
> > *i915)
> >  {
> > +    struct drm_i915_private *dev_priv = i915;
> > +    u32 val;
> >  if (DISPLAY_VER(i915) >= 11 || IS_GEMINILAKE(i915) ||
> >      IS_BROXTON(i915)) {
> > +val = intel_de_read(dev_priv, PP_CONTROL(0));
> > +/* Wa_14013120569:tgl */
> > +if (IS_TIGERLAKE(i915)) {
> > +val &= ~PANEL_POWER_ON;
> > +intel_de_write(dev_priv, PP_CONTROL(0), val);
> > +}
> 
> Code style is all wrong, please fix it and run "dim checkpatch" to
> validate it before sending patches.

Thanks for pointing that out, I will fix it in the next version.

> Also PP_CONTROL(0) don't point to the same register that the
> workaround is talking about, between generations register address
> change that might be
> the case for this one.

Could you point me to the right register that I need to be
programming for this WA?

> This satisfy the "before going into sleep to allow CS entry" but it
> do not restore the workaround after waking up from suspend.
> do not restore the workaround after waking up from suspend.

Ah, I missed that point, will fix it in v2.

> Also you could improve the code, you are reading the register even
> for platforms that don't need the wa, also check intel_de_rmw() it is
> better suited
> to this case.

You're right, I will move that code under IS_TIGERLAKE().
> 
> >  bxt_enable_dc9(i915);
> >  /* Tweaked Wa_14010685332:icp,jsp,mcc */
> >  if (INTEL_PCH_TYPE(i915) >= PCH_ICP && INTEL_PCH_TYPE(i915) <=
> > PCH_MCC)
> > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > index 47c85ac97c87..8e3f84100daf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> > @@ -26,6 +26,7 @@
> >  #include "i915_drv.h"
> >  #include "intel_display_types.h"
> >  #include "intel_hotplug.h"
> > +#include "intel_de.h"
> > 
> >  /**
> >   * DOC: Hotplug
> > @@ -266,7 +267,9 @@ intel_encoder_hotplug(struct intel_encoder
> > *encoder,
> >        struct intel_connector *connector)
> >  {
> >  struct drm_device *dev = connector->base.dev;
> > +struct drm_i915_private *dev_priv = to_i915(dev);
> >  enum drm_connector_status old_status;
> > +u32 val;
> >  u64 old_epoch_counter;
> >  bool ret = false;
> > 
> > @@ -288,6 +291,19 @@ intel_encoder_hotplug(struct intel_encoder
> > *encoder,
> >        drm_get_connector_status_name(connector->base.status),
> >        old_epoch_counter,
> >        connector->base.epoch_counter);
> > +
> > +/* Wa_14013120569:tgl */
> > +if (IS_TIGERLAKE(dev_priv)) {
> > +val = intel_de_read(dev_priv, PP_CONTROL(0));
> > +if (connector->base.status == connector_status_connected) {
> > +val |= PANEL_POWER_ON;
> > +intel_de_write(dev_priv, PP_CONTROL(0), val);
> > +}
> > +else if (connector->base.status == connector_status_disconnected)
> > {
> > +val &= ~PANEL_POWER_ON;
> > +intel_de_write(dev_priv, PP_CONTROL(0), val);
> > +}
> > +}
> 
> Not sure if this is the best place but anyways it is missing handle
> the case were tigerlake boots with the external display connected.
> No hotplug will happen and workaround will never be enabled.

Could you suggest a better place to add this WA?

I will add the check for TGL booting with external display connected
in v2.

> 
> >  return INTEL_HOTPLUG_CHANGED;
> >  }
> >  return INTEL_HOTPLUG_UNCHANGED;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx