On Wed, 2021-10-27 at 17:55 +0300, Jani Nikula wrote:
> On Wed, 27 Oct 2021, "Tolakanahalli Pradeep, Madhumitha" <
> madhumitha.tolakanahalli.pradeep@xxxxxxxxx> wrote:
> > 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 ?
>
> Basically don't access any of the PPS registers outside of
> intel_pps.c. Any access like that is probably going to get the
> locking
> and timeout rules wrong, as well as make the software and hardware
> states go out of sync. Things like these need to be abstracted
> better. Bottom line, you can't just go poke at the registers in
> random
> places, no matter what the W/A says, and expect it to work out fine.
>
> The commit message also doesn't properly explain what is going on,
> and
> *why* this change is needed. Especially when you're adding special
> cases, you need to take extra care to explain the rationale. People
> are
> going to look at git log and git blame literally years from now, and
> wonder what this is about.
>
> BR,
> Jani.
>
>
Wa_14013120569 requires PP_CONTROL bit #0 to be set to 1 when external
display is plugged or resume after sleep. Bit #0 is to be cleared when
external display is unplugged or before going to sleep. W/A isnt
enabled when eDP is connected.
I shall add these details in v2, thank you for pointing that out.
As this W/A is required in a non-eDP scenario, I wouldn't be able to
use abstractions like intel_pps_on and intel_pps_off.
So would I need to create new wrapper functions in intel_pps.c for
setting and clearing the bits in PP_CONTROL with proper locks held,
and call these wrapper functions in intel_display.c/intel_dp.c?
> PS. Please try to ensure your mail client handles thread replies
> properly. This should have been in reply to:
>
> https://lore.kernel.org/r/54fada5eea99c1b5d7af300bcd6697711c3c5705.camel@xxxxxxxxx
>
Sorry about that, fixed it.
- Madhumitha
>
> > - 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(connect
> > > > > or-
> > > > > > 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
