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ä <[email protected]>
> > Cc: Stanislav Lisovskiy <[email protected]>
> > Signed-off-by: James Ausmus <[email protected]>
> > Reviewed-by: Ville Syrjälä <[email protected]>
> > ---
> > 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. :)
-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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
