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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Feb 14, 2022 at 10:03:25PM -0800, Matt Roper wrote:
> 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.

Sounds good.

> 
> > 
> > >  * 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.

ILK_GDSR is not just about the GT but it can reset the 
display engine too. IIRC GEN6_GDRST can't do that.

> 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?

Not sure. Sadly there is no clear theme to some of the
MCHBAR registers so I'm not sure where else we'd place them
(assuming the plan is basically to hollow out i915_regs.h).

I guess we could start with what you have here and if it turns
out annoying we can always come up with something different later.

-- 
Ville Syrjälä
Intel