[Intel-gfx] [PATCH 1/6] drm/i915: Split up struct drm_i915_private

[Richard Purdie]

On Thu, 2009-05-14 at 13:33 -0700, Jesse Barnes wrote:
> Only a couple of bits really jumped out at me...  Overall the split
> looks pretty good, it'll be nice to have this stuff abstracted.
> 
> On Wed, 13 May 2009 15:02:50 +0100
> Richard Purdie <rpurdie at linux.intel.com> wrote:
[lots of save registers]
> > -	u32 saveSWF1[16];
> > -	u32 saveSWF2[3];
> > -	u8 saveMSR;
> > -	u8 saveSR[8];
> > -	u8 saveGR[25];
> > -	u8 saveAR_INDEX;
> > -	u8 saveAR[21];
> > -	u8 saveDACMASK;
> > -	u8 saveCR[37];
> > -	uint64_t saveFENCE[16];
> > -
> >  	struct {
> >  		struct drm_mm gtt_space;
> 
> Seems like only some of these regs will likely be shared with other
> devices.  I guess we don't have to figure out which now though, so
> pulling them all out is ok.

I tried this different ways and in the end decided it was better to move
all the registers. If we don't use all of them on a given platform, so
be it. The alternative gets really ugly...

I think its something we can revisit in later patches.

> > -#define I915_READ(reg)          readl(dev_priv->regs + (reg))
> > -#define I915_WRITE(reg, val)     writel(val, dev_priv->regs + (reg))
> > -#define I915_READ16(reg)	readw(dev_priv->regs + (reg))
> > -#define I915_WRITE16(reg, val)	writel(val, dev_priv->regs +
> > (reg)) -#define I915_READ8(reg)		readb(dev_priv->regs +
> > (reg)) -#define I915_WRITE8(reg, val)	writeb(val,
> > dev_priv->regs + (reg)) -#define I915_WRITE64(reg, val)
> > writeq(val, dev_priv->regs + (reg)) -#define I915_READ64(reg)
> > readq(dev_priv->regs + (reg)) +#define I915_READ(reg)
> > readl(vdc->regs + (reg)) +#define I915_WRITE(reg, val)
> > writel(val, vdc->regs + (reg)) +#define I915_READ16(reg)
> > readw(vdc->regs + (reg)) +#define I915_WRITE16(reg, val)
> > writel(val, vdc->regs + (reg)) +#define
> > I915_READ8(reg)		readb(vdc->regs + (reg)) +#define
> > I915_WRITE8(reg, val)	writeb(val, vdc->regs + (reg)) +#define
> > I915_WRITE64(reg, val)	writeq(val, vdc->regs + (reg)) +#define
> > I915_READ64(reg)	readq(vdc->regs + (reg)) #define
> > POSTING_READ(reg)	(void)I915_READ(reg) 
> >  #define I915_VERBOSE 0
> 
> Having all register accesses go through the vdc register mapping is
> also a bit counterintuitive, since many regs will be chipset specific.
> I guess the best way to avoid this would be to create a separate set of
> VDC_READ/WRITE macros and make them use a copy of the register iomap
> pointer in the vdc struct, but that's a bit more invasive

I know what you mean. It makes sense in that this iomap pointer is
always available easily from struct drm. Having two pointers and two
sets of access macros is certainly possible too though. Is that what we
really want or would renaming I915_* to VDC_* actually be ok? It has the
advantage of keeping things simpler in a way.

If we do want two sets of things, do we want this all as one huge patch?
I guess I can find a way to do it in two stages...

Just thinking out loud, another option would be ugliness in the I915_*
macro definitions which turn them into VDC_* accesses so only one
pointer would be needed. Two pointers is probably neater though.

-- 
Richard Purdie
Intel Open Source Technology Centre