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

Jesse Barnes jbarnes at virtuousgeek.org
Thu May 14 22:33:24 CEST 2009


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:
>  	struct drm_i915_fence_reg fence_regs[16]; /* assume 965 */
>  	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet
> */ int num_fence_regs; /* 8 on pre-965, 16 otherwise */
>  
> -	/* Register state */
> -	u8 saveLBB;
> -	u32 saveDSPACNTR;
> -	u32 saveDSPBCNTR;
> -	u32 saveDSPARB;
> -	u32 saveRENDERSTANDBY;
> -	u32 saveHWS;
> -	u32 savePIPEACONF;
> -	u32 savePIPEBCONF;
> -	u32 savePIPEASRC;
> -	u32 savePIPEBSRC;
> -	u32 saveFPA0;
> -	u32 saveFPA1;
> -	u32 saveDPLL_A;
> -	u32 saveDPLL_A_MD;
> -	u32 saveHTOTAL_A;
> -	u32 saveHBLANK_A;
> -	u32 saveHSYNC_A;
> -	u32 saveVTOTAL_A;
> -	u32 saveVBLANK_A;
> -	u32 saveVSYNC_A;
> -	u32 saveBCLRPAT_A;
> -	u32 savePIPEASTAT;
> -	u32 saveDSPASTRIDE;
> -	u32 saveDSPASIZE;
> -	u32 saveDSPAPOS;
> -	u32 saveDSPAADDR;
> -	u32 saveDSPASURF;
> -	u32 saveDSPATILEOFF;
> -	u32 savePFIT_PGM_RATIOS;
> -	u32 saveBLC_PWM_CTL;
> -	u32 saveBLC_PWM_CTL2;
> -	u32 saveFPB0;
> -	u32 saveFPB1;
> -	u32 saveDPLL_B;
> -	u32 saveDPLL_B_MD;
> -	u32 saveHTOTAL_B;
> -	u32 saveHBLANK_B;
> -	u32 saveHSYNC_B;
> -	u32 saveVTOTAL_B;
> -	u32 saveVBLANK_B;
> -	u32 saveVSYNC_B;
> -	u32 saveBCLRPAT_B;
> -	u32 savePIPEBSTAT;
> -	u32 saveDSPBSTRIDE;
> -	u32 saveDSPBSIZE;
> -	u32 saveDSPBPOS;
> -	u32 saveDSPBADDR;
> -	u32 saveDSPBSURF;
> -	u32 saveDSPBTILEOFF;
> -	u32 saveVGA0;
> -	u32 saveVGA1;
> -	u32 saveVGA_PD;
> -	u32 saveVGACNTRL;
> -	u32 saveADPA;
> -	u32 saveLVDS;
> -	u32 savePP_ON_DELAYS;
> -	u32 savePP_OFF_DELAYS;
> -	u32 saveDVOA;
> -	u32 saveDVOB;
> -	u32 saveDVOC;
> -	u32 savePP_ON;
> -	u32 savePP_OFF;
> -	u32 savePP_CONTROL;
> -	u32 savePP_DIVISOR;
> -	u32 savePFIT_CONTROL;
> -	u32 save_palette_a[256];
> -	u32 save_palette_b[256];
> -	u32 saveFBC_CFB_BASE;
> -	u32 saveFBC_LL_BASE;
> -	u32 saveFBC_CONTROL;
> -	u32 saveFBC_CONTROL2;
> -	u32 saveIER;
> -	u32 saveIIR;
> -	u32 saveIMR;
> -	u32 saveCACHE_MODE_0;
> -	u32 saveD_STATE;
> -	u32 saveCG_2D_DIS;
> -	u32 saveMI_ARB_STATE;
> -	u32 saveSWF0[16];
> -	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.

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

-- 
Jesse Barnes, Intel Open Source Technology Center



More information about the Intel-gfx mailing list