On Fri, Nov 23, 2018 at 02:03:18PM +0200, Jani Nikula wrote:
> On Mon, 19 Nov 2018, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > Depending on the transcoder enum values to translate from transcoder to the
> > corresponding CHICKEN_TRANS register can easily break if we add a new
> > transcoder. Add an explicit mapping instead, by using helpers to look up the
> > register instance either by transcoder or port (since unconveniently the
> > registers have both port and transcoder specific bits).
> >
> > While at it also check for the correctness of GEN, port, transcoder. I wasn't
> > sure if psr2_enabled can only be set for GEN9+, but that seems to be the case
> > indeed (see setting of sink_psr2_support in intel_psr_init_dpcd()).
> >
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 7 +++---
> > drivers/gpu/drm/i915/intel_ddi.c | 53 +++++++++++++++++++++++++++++++---------
> > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_psr.c | 6 +++--
> > 4 files changed, 51 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 25b069175c2a..1beed39de303 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7399,9 +7399,10 @@ enum {
> > #define BDW_DPRS_MASK_VBLANK_SRD (1 << 0)
> > #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
> >
> > -#define CHICKEN_TRANS_A 0x420c0
> > -#define CHICKEN_TRANS_B 0x420c4
> > -#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
> > +#define CHICKEN_TRANS_A _MMIO(0x420c0)
> > +#define CHICKEN_TRANS_B _MMIO(0x420c4)
> > +#define CHICKEN_TRANS_C _MMIO(0x420c8)
> > +#define CHICKEN_TRANS_EDP _MMIO(0x420cc)
> > #define VSC_DATA_SEL_SOFTWARE_CONTROL (1 << 25) /* GLK and CNL+ */
> > #define DDI_TRAINING_OVERRIDE_ENABLE (1 << 19)
> > #define DDI_TRAINING_OVERRIDE_VALUE (1 << 18)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 040483c96029..126e6aac335d 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3380,6 +3380,42 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
> > intel_audio_codec_enable(encoder, crtc_state, conn_state);
> > }
> >
> > +i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
> > + enum transcoder trans)
> > +{
> > + static const i915_reg_t regs[] = {
> > + [TRANSCODER_A] = CHICKEN_TRANS_A,
> > + [TRANSCODER_B] = CHICKEN_TRANS_B,
> > + [TRANSCODER_C] = CHICKEN_TRANS_C,
> > + [TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
> > + };
> > +
> > + WARN_ON(INTEL_GEN(dev_priv) < 9);
> > +
> > + if (WARN_ON(trans >= ARRAY_SIZE(regs) || !regs[trans].reg))
> > + trans = TRANSCODER_A;
> > +
> > + return regs[trans];
> > +}
>
> I'm late to the party, but it kind of makes me sad that we're now
> introducing yet another way to choose registers based on transcoder (or
> some other index). And this one is hand-rolled, with local functions
> instead of putting it to i915_reg.h.
Didn't occur to me that _PICK could be used with named initializers.. We
would still miss the checks which I added here for incorrect indices,
but that's a separate issue from what I wanted to solve in this patch.
So the following would work too:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 47baf2fe8f71..dfa547832dae 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7399,10 +7399,31 @@ enum {
#define BDW_DPRS_MASK_VBLANK_SRD (1 << 0)
#define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
-#define CHICKEN_TRANS_A _MMIO(0x420c0)
-#define CHICKEN_TRANS_B _MMIO(0x420c4)
-#define CHICKEN_TRANS_C _MMIO(0x420c8)
-#define CHICKEN_TRANS_EDP _MMIO(0x420cc)
+#define _CHICKEN_TRANS_A 0x420c0
+#define _CHICKEN_TRANS_B 0x420c4
+#define _CHICKEN_TRANS_C 0x420c8
+#define _CHICKEN_TRANS_EDP 0x420cc
+#define CHICKEN_TRANS(trans) _MMIO(_PICK(trans, \
+ [TRANSCODER_A] = \
+ _CHICKEN_TRANS_A, \
+ [TRANSCODER_B] = \
+ _CHICKEN_TRANS_B, \
+ [TRANSCODER_C] = \
+ _CHICKEN_TRANS_C, \
+ [TRANSCODER_EDP] = \
+ _CHICKEN_TRANS_EDP))
+
+#define CHICKEN_TRANS_BYPORT(port) _MMIO(_PICK(port, \
+ [PORT_A] = \
+ _CHICKEN_TRANS_EDP, \
+ [PORT_B] = \
+ _CHICKEN_TRANS_A, \
+ [PORT_C] = \
+ _CHICKEN_TRANS_B, \
+ [PORT_D] = \
+ _CHICKEN_TRANS_C, \
+ [PORT_E] = \
+ _CHICKEN_TRANS_A))
#define VSC_DATA_SEL_SOFTWARE_CONTROL (1 << 25) /* GLK and CNL+ */
#define DDI_TRAINING_OVERRIDE_ENABLE (1 << 19)
#define DDI_TRAINING_OVERRIDE_VALUE (1 << 18)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ad11540ac436..c261320330b6 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3380,26 +3380,6 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
intel_audio_codec_enable(encoder, crtc_state, conn_state);
}
-static i915_reg_t
-gen9_chicken_trans_reg_by_port(struct drm_i915_private *dev_priv,
- enum port port)
-{
- static const i915_reg_t regs[] = {
- [PORT_A] = CHICKEN_TRANS_EDP,
- [PORT_B] = CHICKEN_TRANS_A,
- [PORT_C] = CHICKEN_TRANS_B,
- [PORT_D] = CHICKEN_TRANS_C,
- [PORT_E] = CHICKEN_TRANS_A,
- };
-
- WARN_ON(INTEL_GEN(dev_priv) < 9);
-
- if (WARN_ON(port < PORT_A || port > PORT_E))
- port = PORT_A;
-
- return regs[port];
-}
-
static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
const struct intel_crtc_state *crtc_state,
const struct drm_connector_state *conn_state)
@@ -3423,10 +3403,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
* the bits affect a specific DDI port rather than
* a specific transcoder.
*/
- i915_reg_t reg = gen9_chicken_trans_reg_by_port(dev_priv, port);
- u32 val;
-
- val = I915_READ(reg);
+ u32 val = I915_READ(CHICKEN_TRANS_BYPORT(port));
if (port == PORT_E)
val |= DDIE_TRAINING_OVERRIDE_ENABLE |
@@ -3435,8 +3412,8 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
val |= DDI_TRAINING_OVERRIDE_ENABLE |
DDI_TRAINING_OVERRIDE_VALUE;
- I915_WRITE(reg, val);
- POSTING_READ(reg);
+ I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
+ POSTING_READ(CHICKEN_TRANS_BYPORT(port));
udelay(1);
@@ -3447,7 +3424,7 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
val &= ~(DDI_TRAINING_OVERRIDE_ENABLE |
DDI_TRAINING_OVERRIDE_VALUE);
- I915_WRITE(reg, val);
+ I915_WRITE(CHICKEN_TRANS_BYPORT(port), val);
}
/* In HDMI/DVI mode, the port width, and swing/emphasis values
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 54fa17a5596a..76667c5d7800 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -577,25 +577,6 @@ static void intel_psr_activate(struct intel_dp *intel_dp)
dev_priv->psr.active = true;
}
-static i915_reg_t gen9_chicken_trans_reg(struct drm_i915_private *dev_priv,
- enum transcoder cpu_transcoder)
-{
- static const i915_reg_t regs[] = {
- [TRANSCODER_A] = CHICKEN_TRANS_A,
- [TRANSCODER_B] = CHICKEN_TRANS_B,
- [TRANSCODER_C] = CHICKEN_TRANS_C,
- [TRANSCODER_EDP] = CHICKEN_TRANS_EDP,
- };
-
- WARN_ON(INTEL_GEN(dev_priv) < 9);
-
- if (WARN_ON(cpu_transcoder >= ARRAY_SIZE(regs) ||
- !regs[cpu_transcoder].reg))
- cpu_transcoder = TRANSCODER_A;
-
- return regs[cpu_transcoder];
-}
-
static void intel_psr_enable_source(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state)
{
@@ -610,9 +591,10 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
hsw_psr_setup_aux(intel_dp);
if (dev_priv->psr.psr2_enabled) {
- i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
- cpu_transcoder);
- u32 chicken = I915_READ(reg);
+ u32 chicken;
+
+ WARN_ON(INTEL_GEN(dev_priv) < 9);
+ chicken = I915_READ(CHICKEN_TRANS(cpu_transcoder));
if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
@@ -620,7 +602,8 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
else
chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
- I915_WRITE(reg, chicken);
+
+ I915_WRITE(CHICKEN_TRANS(cpu_transcoder), chicken);
}
/*
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
