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

[Tolakanahalli Pradeep, Madhumitha]

On Mon, 2021-07-05 at 13:28 +0300, Jani Nikula wrote:
> On Tue, 29 Jun 2021, "Souza, Jose" <jose.souza at intel.com> 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 at 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.
> > 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.
> 
> In general, I've put a bunch of effort into moving most PPS stuff and
> PP_CONTROL reg access into intel_pps.c, not least because you must
> hold
> appropriate locks and power domain references to poke at this. You
> can't
> just mess with it nilly willy. I don't want these abstractions
> bypassed.
> 
> BR,
> Jani.

I see that intel_pps_get_registers(),  populates the regs-
>pp_ctrl  correctly. That is what I would want to access and set the
bits for this W/A. However as is I cannot call pps_get_registers() in
intel_dp or intel_display.c for the external connector  at
connect/disconnect time. Do you recommend making this function non
static and calling it for this W/A or is there a way I can access the
populated i915_reg_t pp_ctrl  to set the W/A?

Or are you wanting to  define another helper for enable/disable of this
W/A in intel_pps.c that would then call pps_init_registers or similar
function ?

- Madhumitha

> 
> > This satisfy the "before going into sleep to allow CS entry" but it
> > do not restore the workaround after waking up from suspend.
> > 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.
> > 
> > >  		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.
> > 
> > >  		return INTEL_HOTPLUG_CHANGED;
> > >  	}
> > >  	return INTEL_HOTPLUG_UNCHANGED;
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx