@Jani @Manasi Bump. On Mon, 2021-11-08 at 15:52 -0800, Navare, Manasi wrote: > On Mon, Nov 01, 2021 at 12:25:21PM +0200, Jani Nikula wrote: > > On Mon, 28 Jun 2021, Madhumitha Tolakanahalli Pradeep > > <madhumitha.tolakanahalli.pradeep@xxxxxxxxx> wrote: > > > PCH display HPD IRQ is not detected with default filter value. > > > So, PP_CONTROL is manually reprogrammed. > > > > Returning to this workaround. > > > > You're not supposed to enable the workaround when there's eDP > > connected. This is also crucial in avoiding issues with eDP PPS. > > > > The workaround is specific to Tiger Lake PCH, so you need to check > > against the PCH, not the GPU. > > > > Also see comments inline. > > > > > > > > Signed-off-by: Madhumitha Tolakanahalli Pradeep > > > <madhumitha.tolakanahalli.pradeep@xxxxxxxxx> > > > --- > > > .../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); > > > + } > > > > As José said, how do you enable the workaround after resume if > > external > > displays are still connected? > > > > > 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); > > > + } > > > + } > > > > First off, usually if you have a clean, generic, high level > > function, > > it's a hint you shouldn't stick low level register access there. > > > > If you plug in two external displays and then unplug one of them, > > you > > end up disabling the workaround, while it's supposed to remain > > enabled > > if there's an external display connected. This is likely the most > > annoying part about the workaround. > > > > This does not seem like a trivial workaround to implement. > > > > Yes I agree, not a trivial W/A to implement. I think few main things > to figure out: > - Right place to enable/disable the W/A at connect/disconnect and for > the connectors already connected on boot - Probably in > hdmi_init_connector() and dp_init_connector() for the connected conns > on boot and then intel_encoder_hptplug() at the time of ext display > hotplug/unplug > > @Jani having this W/A in above 2 places you think is good? > - The other thing like Jani pointed out is that we should > enable/disable only if !edp - so add this check for init_connector > functions before setting/ clearing the W/A > > - Third thing is the wrapper function to be defined in intel_pps.c > something like below: > > intel_pps_wa_enable(struct intel_dp *intel_dp) > { > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > intel_wakeref_t wakeref; > > with_intel_pps_lock(intel_dp, wakeref) { > i915_reg_t pp_ctrl_reg = _pp_ctrl_reg(intel_dp); > u32 pp; > > pp = intel_de_read(dev_priv, _pp_ctrl_reg(intel_dp)); > pp |= PANEL_POWER_ON; > intel_de_write(dev_priv, pp_ctrl_reg, pp); > intel_de_posting_read(dev_priv, pp_ctrl_reg); > } > } > And similar for disabling the wa > > Then this function can be called in the appropriate places identified > above. > > @Jani this pps helper function for enable/disable W/A makes sense? > > > Regards > Manasi > } > > > > BR, > > Jani. > > > > > > > return INTEL_HOTPLUG_CHANGED; > > > } > > > return INTEL_HOTPLUG_UNCHANGED; > > > > -- > > Jani Nikula, Intel Open Source Graphics Center - Madhumitha