| Message ID | 20190924222829.13142-1-james.ausmus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915/tgl: Add memory type decoding for bandwidth checking | expand |
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@linux.intel.com> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > Signed-off-by: James Ausmus <james.ausmus@intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > 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. Thanks, Stuart > + } > + } else { > + MISSING_CASE(INTEL_GEN(dev_priv)); > + qi->dram_type = INTEL_DRAM_LPDDR3; /* Conservative > default */ > } > > qi->num_channels = (val & 0xf0) >> 4;
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@linux.intel.com> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > 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;
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@linux.intel.com> > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > 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@intel.com> > > -James > > > > > Thanks, > > Stuart > > > > > + } > > > + } else { > > > + MISSING_CASE(INTEL_GEN(dev_priv)); > > > + qi->dram_type = INTEL_DRAM_LPDDR3; /* Conservative > > > default */ > > > } > > > > > > qi->num_channels = (val & 0xf0) >> 4; > >
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@linux.intel.com> > > > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > Signed-off-by: James Ausmus <james.ausmus@intel.com> > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > 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@intel.com> 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; > > > >
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; + } + } else { + MISSING_CASE(INTEL_GEN(dev_priv)); + qi->dram_type = INTEL_DRAM_LPDDR3; /* Conservative default */ } qi->num_channels = (val & 0xf0) >> 4;