On Fri, 2023-04-28 at 14:03 +0300, Ville Syrjälä wrote:
> On Fri, Apr 28, 2023 at 10:18:34AM +0000, Hogander, Jouni wrote:
> > Hello,
> >
> > Please check my inline comments below.
> >
> > On Fri, 2023-04-21 at 15:02 +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > Reintroduce the special PSR AUX CH setup for hsw/bdw. Not all
> > > of it was even removed (BDW AUX data registers were left behind).
> > > Update the code to use REG_BIT() & co. while at it.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_dp_aux.c | 2 +-
> > > drivers/gpu/drm/i915/display/intel_dp_aux.h | 4 ++
> > > drivers/gpu/drm/i915/display/intel_psr.c | 61
> > > +++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_psr_regs.h | 11 ++++
> > > 4 files changed, 77 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > > index abf77ba76972..847fd6bfa7e4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.c
> > > @@ -14,7 +14,7 @@
> > > #include "intel_pps.h"
> > > #include "intel_tc.h"
> > >
> > > -static u32 intel_dp_aux_pack(const u8 *src, int src_bytes)
> > > +u32 intel_dp_aux_pack(const u8 *src, int src_bytes)
> > > {
> > > int i;
> > > u32 v = 0;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux.h
> > > b/drivers/gpu/drm/i915/display/intel_dp_aux.h
> > > index 138e340f94ee..3bc529a23dd6 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_aux.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux.h
> > > @@ -6,6 +6,8 @@
> > > #ifndef __INTEL_DP_AUX_H__
> > > #define __INTEL_DP_AUX_H__
> > >
> > > +#include <linux/types.h>
> > > +
> > > enum aux_ch;
> > > struct intel_dp;
> > > struct intel_encoder;
> > > @@ -15,4 +17,6 @@ void intel_dp_aux_init(struct intel_dp
> > > *intel_dp);
> > >
> > > enum aux_ch intel_dp_aux_ch(struct intel_encoder *encoder);
> > >
> > > +u32 intel_dp_aux_pack(const u8 *src, int src_bytes);
> > > +
> > > #endif /* __INTEL_DP_AUX_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > index 7f748c7a71f3..2ff6f75c2bee 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > @@ -288,6 +288,24 @@ static i915_reg_t psr_iir_reg(struct
> > > drm_i915_private *dev_priv,
> > > return EDP_PSR_IIR;
> > > }
> > >
> > > +static i915_reg_t psr_aux_ctl_reg(struct drm_i915_private
> > > *dev_priv,
> > > + enum transcoder cpu_transcoder)
> > > +{
> > > + if (DISPLAY_VER(dev_priv) >= 8)
> > > + return EDP_PSR_AUX_CTL(cpu_transcoder);
> > > + else
> > > + return HSW_SRD_AUX_CTL;
> > > +}
> > > +
> > > +static i915_reg_t psr_aux_data_reg(struct drm_i915_private
> > > *dev_priv,
> > > + enum transcoder
> > > cpu_transcoder,
> > > int i)
> > > +{
> > > + if (DISPLAY_VER(dev_priv) >= 8)
> > > + return EDP_PSR_AUX_DATA(cpu_transcoder, i);
> > > + else
> > > + return HSW_SRD_AUX_DATA(i);
> > > +}
> > > +
> > > static void psr_irq_control(struct intel_dp *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > @@ -512,6 +530,42 @@ void intel_psr_init_dpcd(struct intel_dp
> > > *intel_dp)
> > > }
> > > }
> > >
> > > +static void hsw_psr_setup_aux(struct intel_dp *intel_dp)
> > > +{
> > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > + enum transcoder cpu_transcoder = intel_dp-
> > > >psr.transcoder;
> > > + u32 aux_clock_divider, aux_ctl;
> > > + static const u8 aux_msg[] = {
> > > + [0] = (DP_AUX_NATIVE_WRITE << 4) | ((DP_SET_POWER
> > > >>
> > > 16) & 0xf),
> > > + [1] = (DP_SET_POWER >> 8) & 0xff,
> > > + [2] = DP_SET_POWER & 0xff,
> > > + [3] = 1 - 1,
> > > + [4] = DP_SET_POWER_D0,
> > > + };
> >
> > Could you please add some description or provide some pointer which
> > would help to parse what you are doing here?
>
> It's just crafting a DP_SET_POWER=D0 DPCD write by hand.
>
> I was thinking of refactoring the AUX msg packing code
> to make thig something like
> struct drm_dp_aux_msg {
> ...
> };
> intel_dp_aux_pack_msg(msg)
> but that would require some actual though so not something
> I want to do in this patch. So for now I just restored
> this to (more or less) what we had before.
Ok, that is fine.
>
> >
> > > + int i;
> > > +
> > > + BUILD_BUG_ON(sizeof(aux_msg) > 20);
> > > + for (i = 0; i < sizeof(aux_msg); i += 4)
> > > + intel_de_write(dev_priv,
> > > + psr_aux_data_reg(dev_priv,
> > > cpu_transcoder, i >> 2),
> > > + intel_dp_aux_pack(&aux_msg[i],
> > > sizeof(aux_msg) - i));
> > > +
> > > + aux_clock_divider = intel_dp-
> > > >get_aux_clock_divider(intel_dp,
> > > 0);
> > > +
> > > + /* Start with bits set for DDI_AUX_CTL register */
> > > + aux_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> > > sizeof(aux_msg),
> > > + aux_clock_divider);
> > > +
> > > + /* Select only valid bits for SRD_AUX_CTL */
> > > + aux_ctl &= EDP_PSR_AUX_CTL_TIME_OUT_MASK |
> > > + EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK |
> > > + EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK |
> > > + EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK;
> >
> > How about using definitions from
> > drivers/gpu/drm/i915/display/intel_dp_aux_regs.h?
>
> I suppose we could define the bits as
> #define EPD_PSR_FOO DP_AUX_CH_CTL_FOO
> instead of defining them with REG_BIT() & co. direclty,
> to make it clear they are identical.
>
> > Or just refer these
> > being identical definitions to aux_send_ctl bits?
Either way is fine for me.
> >
> > > +
> > > + intel_de_write(dev_priv, psr_aux_ctl_reg(dev_priv,
> > > cpu_transcoder),
> > > + aux_ctl);
> > > +}
> > > +
> > > static void intel_psr_enable_sink(struct intel_dp *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > @@ -1318,6 +1372,13 @@ static void intel_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > > enum transcoder cpu_transcoder = intel_dp-
> > > >psr.transcoder;
> > > u32 mask;
> > >
> > > + /*
> > > + * Only HSW and BDW have PSR AUX registers that need to
> > > be
> > > setup.
> > > + * SKL+ use hardcoded values PSR AUX transactions
> > > + */
> > > + if (DISPLAY_VER(dev_priv) < 9)
> > > + hsw_psr_setup_aux(intel_dp);
> > > +
> > > /*
> > > * Per Spec: Avoid continuous PSR exit by masking MEMUP
> > > and
> > > HPD also
> > > * mask LPSP to avoid dependency on other drivers that
> > > might
> > > block
> > > diff --git a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > > b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > > index 998f638ee182..5e54817b6a0f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_psr_regs.h
> > > @@ -80,6 +80,17 @@
> > > #define EDP_PSR_PRE_ENTRY(trans) (TGL_PSR_PRE_ENTRY
> > > << \
> > >
> > > _EDP_PSR_TRANS_SHIFT(trans))
> > >
> > > +#define
> > > HSW_SRD_AUX_CTL _MMIO(0x64810)
> > > +#define _SRD_AUX_CTL_A 0x60810
> > > +#define _SRD_AUX_CTL_EDP 0x6f810
> > > +#define
> > > EDP_PSR_AUX_CTL(tran) _MMIO_TRANS2(tran,
> > > _SRD_AUX_CTL_A)
> > > +#define
> > > EDP_PSR_AUX_CTL_TIME_OUT_MASK REG_GENMASK(27, 26)
> > > +#define EDP_PSR_AUX_CTL_MESSAGE_SIZE_MASK REG_GENMASK(24,
> > > 20)
> > > +#define EDP_PSR_AUX_CTL_PRECHARGE_2US_MASK REG_GENMASK(19,
> > > 16)
> > > +#define EDP_PSR_AUX_CTL_ERROR_INTERRUPT REG_BIT(11)
> > > +#define EDP_PSR_AUX_CTL_BIT_CLOCK_2X_MASK REG_GENMASK(10,
> > > 0)
> > > +
> > > +#define HSW_SRD_AUX_DATA(i) _MMIO(0x64814 +
> > > (i) *
> > > 4) /* 5 registers */
> > > #define _SRD_AUX_DATA_A 0x60814
> > > #define _SRD_AUX_DATA_EDP 0x6f814
> > > #define EDP_PSR_AUX_DATA(tran,
> > > i) _MMIO_TRANS2(tran,
> > > _SRD_AUX_DATA_A + (i) * 4) /* 5 registers */
> >
> > BR,
> >
> > Jouni Högander
> >
>
