Re: [Intel-gfx] [PATCH] drm/i915: Move MCHBAR registers to their own header

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, Feb 11, 2022 at 10:41:53AM +0200, Ville Syrjälä wrote:
> On Thu, Feb 10, 2022 at 03:12:17PM -0800, Matt Roper wrote:
> > Registers that exist within the MCH BAR and are mirrored into the GPU's
> > MMIO space are a good candidate to separate out into their own header.
> > 
> > For reference, the mirror of the MCH BAR lives at the following
> > locations in the graphics MMIO space:
> > 
> >  * Pre-gen6:           0x10000 -  0x13000
> 
> Should go up to 0x14000 according to some docs I have.

I think I was looking at a gm45 PRM for this.  Given the spotty
documentation on the older platforms and the number of different end
points there seem to be, maybe it's a better idea to just give the
starting offset in the commit message and say that the upper bound
varies.

> 
> >  * Gen6-Gen11 + RKL:  0x140000 - 0x14FFFF
> 
> Some docs say this goes up to 0x180000, other docs have different
> numbers. I suppose it doesn't matter all that much really. And
> BXT+ clearly can't go past 0x160000 since IIRC that's where some
> of the PHY/PLL stuff lives.
> 
> >  * TGL, ADL:          0x140000 - 0x15FFFF
> > 
> > Bspec: 134, 51771
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> <snip>
> > -#define GEN6_GT_PERF_STATUS	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5948)
> > -#define BXT_GT_PERF_STATUS      _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x7070)
> > -#define GEN6_RP_STATE_LIMITS	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5994)
> > -#define GEN6_RP_STATE_CAP	_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5998)
> > -#define   RP0_CAP_MASK		REG_GENMASK(7, 0)
> > -#define   RP1_CAP_MASK		REG_GENMASK(15, 8)
> > -#define   RPN_CAP_MASK		REG_GENMASK(23, 16)
> >  #define BXT_RP_STATE_CAP        _MMIO(0x138170)
> >  #define GEN9_RP_STATE_LIMITS	_MMIO(0x138148)
> >  #define XEHPSDV_RP_STATE_CAP	_MMIO(0x250014)
> 
> :( This is a bit unfortunate. I wonder if we should make an exception
> for these and keep them all together somewhere?

I don't really see a problem with having them in separate headers.  We
have other stuff like ILK_GDSR / GEN6_GDRST that also used to be in the
MCHBAR and then moved to the GT proper.  I believe the MCHBAR mirror is
going away completely a platform or two down the road and all the
important registers are migrating to non-MCHBAR offsets, so if we try to
keep them all together, that defeats most of the purpose of having a
separate MCHBAR header?

> 
> <snip>
> > -/* Memory latency timer register */
> > -#define MLTR_ILK		_MMIO(0x11222)
> > +#define MLTR_ILK				_MMIO(MCHBAR_MIRROR_BASE + 0x1222)
> 
> I'd prefer to see a separate patch for the s/number/MCHBAR_MIRROR_BASE/ 
> stuff. Very hard to review those in this form.

Good point; I'll fix that.


Matt

> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795