On Fri, Jan 26, 2018 at 06:24:32PM -0200, Paulo Zanoni wrote:
> Em Ter, 2018-01-23 às 16:32 -0800, James Ausmus escreveu:
> > On Tue, Jan 23, 2018 at 05:05:21PM -0200, Paulo Zanoni wrote:
> > > On ICL we have two sets of registers: one for port A and another
> > > for
> > > port B. The set of port A registers is the same as the CNL
> > > registers.
> > >
> > > Since the procmon table on ICL is the same we want to reuse the CNL
> > > function. To do that we add a port argument and make CNL always
> > > call
> > > the function passing port A. This way, we'll be able to easily
> > > reuse
> > > the function on ICL when we add icl_display_core_init().
> > >
> > > v2: Don't use _PICK() when you can use a ternary operator.
> > >
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_reg.h | 26
> > > ++++++++++++++++++++++++++
> > > drivers/gpu/drm/i915/intel_runtime_pm.c | 21 ++++++++++++++-------
> > > 2 files changed, 40 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index d72e206b2b9f..ebf6261d30fd 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2102,6 +2102,32 @@ enum i915_power_well_id {
> > > #define CNL_PORT_COMP_DW9 _MMIO(0x162124)
> > > #define CNL_PORT_COMP_DW10 _MMIO(0x162128)
> > >
> > > +#define _ICL_PORT_COMP_DW0_A 0x162100
> > > +#define _ICL_PORT_COMP_DW0_B 0x6C100
> > > +#define ICL_PORT_COMP_DW0(port) _MMIO((port ==
> > > PORT_A) ? \
> > > + _ICL_PORT_COMP_DW0_A
> > > : \
> > > + _ICL_PORT_COMP_DW0_B
> > > )
> > > +#define _ICL_PORT_COMP_DW1_A 0x162104
> > > +#define _ICL_PORT_COMP_DW1_B 0x6C104
> > > +#define ICL_PORT_COMP_DW1(port) _MMIO((port ==
> > > PORT_A) ? \
> > > + _ICL_PORT_COMP_DW1_A
> > > : \
> > > + _ICL_PORT_COMP_DW1_B
> > > )
> > > +#define _ICL_PORT_COMP_DW3_A 0x16210C
> > > +#define _ICL_PORT_COMP_DW3_B 0x6C10C
> > > +#define ICL_PORT_COMP_DW3(port) _MMIO((port ==
> > > PORT_A) ? \
> > > + _ICL_PORT_COMP_DW3_A
> > > : \
> > > + _ICL_PORT_COMP_DW3_B
> > > )
> > > +#define _ICL_PORT_COMP_DW9_A 0x162124
> > > +#define _ICL_PORT_COMP_DW9_B 0x6C124
> > > +#define ICL_PORT_COMP_DW9(port) _MMIO((port ==
> > > PORT_A) ? \
> > > + _ICL_PORT_COMP_DW9_A
> > > : \
> > > + _ICL_PORT_COMP_DW9_B
> > > )
> > > +#define _ICL_PORT_COMP_DW10_A 0x162128
> > > +#define _ICL_PORT_COMP_DW10_B 0x6C128
> > > +#define ICL_PORT_COMP_DW10(port) _MMIO((port == PORT_A) ?
> > > \
> > > + _ICL_PORT_COMP_DW10_
> > > A : \
> > > + _ICL_PORT_COMP_DW10_
> > > B)
> > > +
> > > /* BXT PHY Ref registers */
> > > #define _PORT_REF_DW3_A 0x16218C
> > > #define _PORT_REF_DW3_BC 0x6C18C
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 5b1aa4b9c72c..73dd525d241a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -2758,12 +2758,19 @@ static const struct cnl_procmon {
> > > { .dw1 = 0x00440000, .dw9 = 0x9A00AB25, .dw10 =
> > > 0x8AE38FF1, },
> > > };
> > >
> > > -static void cnl_set_procmon_ref_values(struct drm_i915_private
> > > *dev_priv)
> > > +/*
> > > + * CNL has just one set of registers, while ICL has two sets: one
> > > for port A and
> > > + * the other for port B. The CNL registers are equivalent to the
> > > ICL port A
> > > + * registers, that's why we call the ICL macros even though the
> > > function has CNL
> > > + * on its name.
> > > + */
>
> My reply below refers to this comment here ^.
>
>
>
>
> > > +static void cnl_set_procmon_ref_values(struct drm_i915_private
> > > *dev_priv,
> > > + enum port port)
> > > {
> > > const struct cnl_procmon *procmon;
> > > u32 val;
> > >
> > > - val = I915_READ(CNL_PORT_COMP_DW3);
> > > + val = I915_READ(ICL_PORT_COMP_DW3(port));
> > > switch (val & (PROCESS_INFO_MASK | VOLTAGE_INFO_MASK)) {
> > > default:
> > > MISSING_CASE(val);
> > > @@ -2784,13 +2791,13 @@ static void
> > > cnl_set_procmon_ref_values(struct drm_i915_private *dev_priv)
> > > break;
> > > }
> > >
> > > - val = I915_READ(CNL_PORT_COMP_DW1);
> > > + val = I915_READ(ICL_PORT_COMP_DW1(port));
> > > val &= ~((0xff << 16) | 0xff);
> > > val |= procmon->dw1;
> > > - I915_WRITE(CNL_PORT_COMP_DW1, val);
> > > + I915_WRITE(ICL_PORT_COMP_DW1(port), val);
> > >
> > > - I915_WRITE(CNL_PORT_COMP_DW9, procmon->dw9);
> > > - I915_WRITE(CNL_PORT_COMP_DW10, procmon->dw10);
> > > + I915_WRITE(ICL_PORT_COMP_DW9(port), procmon->dw9);
> > > + I915_WRITE(ICL_PORT_COMP_DW10(port), procmon->dw10);
> > > }
> > >
> > > static void cnl_display_core_init(struct drm_i915_private
> > > *dev_priv, bool resume)
> > > @@ -2811,7 +2818,7 @@ static void cnl_display_core_init(struct
> > > drm_i915_private *dev_priv, bool resume
> > > val &= ~CNL_COMP_PWR_DOWN;
> > > I915_WRITE(CHICKEN_MISC_2, val);
> > >
> > > - cnl_set_procmon_ref_values(dev_priv);
> > > + cnl_set_procmon_ref_values(dev_priv, PORT_A);
> >
> > Maybe worth a one-line comment here about why we're passing PORT_A so
> > drive-by readings don't get confused on why we're only setting
> > PORT_A?
> >
> > Maybe something like
> >
> > /* Dummy PORT_A to get the correct CNL register from the ICL macro */
>
> I put such comment at the beginning of the function. See above. Is that
> enough or you think we also need this extra comment?
Yeah, I saw the above comment - just figured that a quick one-liner here
would prevent confusion of anyone that was skimming through this code,
without diving in to each individual call. A marginal improvement at
best, considering all it takes to grok is to not be lazy and look at the
comment you already provided above. :D
>
> >
> > Either way:
> >
> > Reviewed-by: James Ausmus <james.ausmus@xxxxxxxxx>
>
> Thanks!
>
> >
> > >
> > > val = I915_READ(CNL_PORT_COMP_DW0);
> > > val |= COMP_INIT;
> > > --
> > > 2.14.3
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
