[Intel-gfx] [PATCH v2] drm/i915/tgl: Add memory type decoding for bandwidth checking

[Summers, Stuart]

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 at linux.intel.com>
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Signed-off-by: James Ausmus <james.ausmus at intel.com>
> > > Reviewed-by: Ville Syrjälä <ville.syrjala at 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 at 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;
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3270 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20190925/801d992e/attachment.bin>