On Wed, Sep 25, 2019 at 03:35:28PM -0700, Summers, Stuart wrote:
> On Wed, 2019-09-25 at 08:35 -0700, James Ausmus wrote:
> > On Wed, Sep 25, 2019 at 07:33:38AM -0700, Summers, Stuart wrote:
> > > On Tue, 2019-09-24 at 15:28 -0700, James Ausmus wrote:
> > > > The memory type values have changed in TGL, so we need to
> > > > translate
> > > > them
> > > > differently than ICL. While we're moving it, fix up the ICL
> > > > translation
> > > > for LPDDR4.
> > > >
> > > > BSpec: 53998
> > > >
> > > > v2: Fix up ICL LPDDR4 entry (Ville); Drop unused values from TGL
> > > > (Ville)
> > > >
> > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > > > Signed-off-by: James Ausmus <james.ausmus@xxxxxxxxx>
> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/display/intel_bw.c | 55 ++++++++++++++++++-
> > > > ----
> > > > --
> > > > 1 file changed, 39 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > index cd58e47ab7b2..22e83f857de8 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > > > @@ -35,22 +35,45 @@ static int
> > > > icl_pcode_read_mem_global_info(struct
> > > > drm_i915_private *dev_priv,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - switch (val & 0xf) {
> > > > - case 0:
> > > > - qi->dram_type = INTEL_DRAM_DDR4;
> > > > - break;
> > > > - case 1:
> > > > - qi->dram_type = INTEL_DRAM_DDR3;
> > > > - break;
> > > > - case 2:
> > > > - qi->dram_type = INTEL_DRAM_LPDDR3;
> > > > - break;
> > > > - case 3:
> > > > - qi->dram_type = INTEL_DRAM_LPDDR3;
> > > > - break;
> > > > - default:
> > > > - MISSING_CASE(val & 0xf);
> > > > - break;
> > > > + if (IS_GEN(dev_priv, 12)) {
> > > > + switch (val & 0xf) {
> > > > + case 0:
> > > > + qi->dram_type = INTEL_DRAM_DDR4;
> > > > + break;
> > > > + case 3:
> > > > + qi->dram_type = INTEL_DRAM_LPDDR4;
> > > > + break;
> > > > + case 4:
> > > > + qi->dram_type = INTEL_DRAM_DDR3;
> > > > + break;
> > > > + case 5:
> > > > + qi->dram_type = INTEL_DRAM_LPDDR3;
> > > > + break;
> > > > + default:
> > > > + MISSING_CASE(val & 0xf);
> > > > + break;
> > > > + }
> > > > + } else if (IS_GEN(dev_priv, 11)) {
> > > > + switch (val & 0xf) {
> > > > + case 0:
> > > > + qi->dram_type = INTEL_DRAM_DDR4;
> > > > + break;
> > > > + case 1:
> > > > + qi->dram_type = INTEL_DRAM_DDR3;
> > > > + break;
> > > > + case 2:
> > > > + qi->dram_type = INTEL_DRAM_LPDDR3;
> > > > + break;
> > > > + case 3:
> > > > + qi->dram_type = INTEL_DRAM_LPDDR4;
> > > > + break;
> > > > + default:
> > > > + MISSING_CASE(val & 0xf);
> > > > + break;
> > >
> > > James, is there a reason we can't just combine these two conditions
> > > into one switch statement? At initial glance it looks like the
> > > cases
> > > are the same for the common ones and the only real difference is
> > > the
> > > supported bits.
> >
> > The info I got from the HW guys indicates that the same values are
> > very
> > likely to have different meanings for different gens, and likely to
> > even
> > have different values for variants of a single gen, so as more
> > platforms
> > are added in the future, a single switch would get very messy. Even
> > now,
> > I think it would be fairly ugly, as it would look something like:
> >
> > switch (val) {
> > case 0:
> > DDR4;
> > case 1:
> > if (GEN == 11)
> > DDR3;
> > else
> > MISSING_CASE(val);
> > case 2:
> > if (GEN == 11)
> > LPDDR3;
> > else
> > MISSING_CASE(val);
> > case 3:
> > LPDDR4;
> > case 4:
> > if (GEN == 12)
> > DDR3;
> > else
> > MISSING_CASE(val);
> > case 5:
> > if (GEN == 12)
> > LPDDR3;
> > else
> > MISSING_CASE(val);
> > }
> >
> > And then start adding special cases for variants within a gen, as
> > well
> > as additional gen checks, and I think it starts looking fairly
> > spaghetti. :)
>
> I understand and thanks for the explanation. Your reasoning makes sense
> to me.
>
> I checked bspec and confirmed the TGL entries look right. We also spoke
> in IRC and I agree with the changes you're making for ICL. With that:
>
> Reviewed-by: Stuart Summers <stuart.summers@xxxxxxxxx>
Thanks for the review!
-James
>
> >
> > -James
> >
> > >
> > > Thanks,
> > > Stuart
> > >
> > > > + }
> > > > + } else {
> > > > + MISSING_CASE(INTEL_GEN(dev_priv));
> > > > + qi->dram_type = INTEL_DRAM_LPDDR3; /* Conservative
> > > > default */
> > > > }
> > > >
> > > > qi->num_channels = (val & 0xf0) >> 4;
> >
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
