[RFC][PATCH] drm/i915: Get rid of IS_DISPLAYREG()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jan 22, 2013 at 09:03:20PM +0100, Daniel Vetter wrote:
> On Tue, Jan 22, 2013 at 09:13:04PM +0200, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> > 
> > IS_DISPLAYREG() is rather messy, and hard to maintain. Furthermore it
> > can't cope with situations where there's a register both at offset X,
> > and at offset 0x180000+X.
> > 
> > So just kill it, and instead include an optional offset in the
> > register definitions themselves. The offset is a part of the
> > intel_device_info structure.
> > 
> > Add the offset to most display registers, leaving out some clearly
> > PCH/ILK+ specific things, TV encoder registers (unlikely those would
> > be used on anything new), and the GPIO registers. The GPIO registers
> > are left out beacuse the current i2c init code expects them to be
> > constants, and said code already has offset handling. In my zeal I
> > may have converted some registers that will never be used on a
> > platform requiring an offset.
> > 
> > A few switch statements must be manually converted to if-else
> > ladders because the silly C standard demands integer constants
> > for case labels.
> > 
> > VLV_IIR & co. get also thrown out since those can also be
> > handled through the offset mechanism.
> > 
> > Signed-off-by: Ville Syrj?l? <ville.syrjala at linux.intel.com>
> 
> Infinitely much more in favour of this than IS_DISPLAYREG!
> 
> Imo we should fasttrack this to dinq, now that we don't have any big piles
> of register #define patches hanging around ...
> 
> A few bikesheds/comments inline below. Imo as soon as we have some
> agreement on what the patch should roughly look like, Jesse needs to test
> it asap.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |   6 +-
> >  drivers/gpu/drm/i915/i915_drv.c     | 105 +------
> >  drivers/gpu/drm/i915/i915_drv.h     |   1 +
> >  drivers/gpu/drm/i915/i915_irq.c     |  45 ++-
> >  drivers/gpu/drm/i915/i915_reg.h     | 588 ++++++++++++++++++------------------
> >  drivers/gpu/drm/i915/intel_crt.c    |   2 -
> >  drivers/gpu/drm/i915/intel_dp.c     |  13 +-
> >  drivers/gpu/drm/i915/intel_hdmi.c   |  32 +-
> >  drivers/gpu/drm/i915/intel_i2c.c    |   2 +-
> >  drivers/gpu/drm/i915/intel_sdvo.c   |   9 +-
> >  10 files changed, 338 insertions(+), 465 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 18971f5..76c5b2f 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -430,13 +430,13 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
> >  
> >  	if (IS_VALLEYVIEW(dev)) {
> >  		seq_printf(m, "Display IER:\t%08x\n",
> > -			   I915_READ(VLV_IER));
> > +			   I915_READ(IER));
> >  		seq_printf(m, "Display IIR:\t%08x\n",
> > -			   I915_READ(VLV_IIR));
> > +			   I915_READ(IIR));
> >  		seq_printf(m, "Display IIR_RW:\t%08x\n",
> >  			   I915_READ(VLV_IIR_RW));
> >  		seq_printf(m, "Display IMR:\t%08x\n",
> > -			   I915_READ(VLV_IMR));
> > +			   I915_READ(IMR));
> 
> All the VLV_irq_reg #defines you kill here are within IS_VLV blocks or
> functions even already. So I think it makes much more sense to keep all
> the VLV_ #defines around here - less potential for confusion.

I'm not a fan of special cases. But if you prefer I can leave these
alone.

> 
> >  		for_each_pipe(pipe)
> >  			seq_printf(m, "Pipe %c stat:\t%08x\n",
> >  				   pipe_name(pipe),
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 3ff8e73..a9352b20 100644
> 
> [cut out awesome part of the patch]
> 
> One bikeshed though: Since essentially the display block moved about, I'm
> voting for a s/info->mmio_offset/info->display_mmio_offset/ sed job
> accross the entire patch.

Sure. I was also wondering whether the (dev_priv... + x) is a bit heavy
on the eyes. We could hide that in another macro eg. DSPREG(x) or
something. But if you anyway want some registers to be converted to
(VLV_DISPLAY_BASE + x) instead, then it doesn't make much sense to
add such a macro.

> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 9a9dda1..89a338c 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -552,7 +552,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  	atomic_inc(&dev_priv->irq_received);
> >  
> >  	while (true) {
> > -		iir = I915_READ(VLV_IIR);
> > +		iir = I915_READ(IIR);
> >  		gt_iir = I915_READ(GTIIR);
> >  		pm_iir = I915_READ(GEN6_PMIIR);
> >  
> > @@ -612,13 +612,12 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  
> >  		I915_WRITE(GTIIR, gt_iir);
> >  		I915_WRITE(GEN6_PMIIR, pm_iir);
> > -		I915_WRITE(VLV_IIR, iir);
> > +		I915_WRITE(IIR, iir);
> 
> Same thing about interrupt registers, imo those are ok as-is. The only
> exception/share reg I've seen thus far is the PORT_HOTPLUG stuff. Since
> that's much more display related than all the other irq fun, I guess we
> could just leave it as-is.

I can leave the VLV_IIR & co. alone. But now that I think about I
probably want to convert them to the (VLV_DISPLAY_BASE + x) format
as well. Much easier to grep/seach.

> [cut out irq conversion.]
> 
> >  static void ironlake_irq_uninstall(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index aea2d12..238e40a 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -133,25 +133,25 @@
> >  
> >  /* VGA stuff */
> >  
> > -#define VGA_ST01_MDA 0x3ba
> > -#define VGA_ST01_CGA 0x3da
> > +#define VGA_ST01_MDA (dev_priv->info->mmio_offset + 0x3ba)
> > +#define VGA_ST01_CGA (dev_priv->info->mmio_offset + 0x3da)
> >  
> > -#define VGA_MSR_WRITE 0x3c2
> > -#define VGA_MSR_READ 0x3cc
> > +#define VGA_MSR_WRITE (dev_priv->info->mmio_offset + 0x3c2)
> > +#define VGA_MSR_READ (dev_priv->info->mmio_offset + 0x3cc)
> >  #define   VGA_MSR_MEM_EN (1<<1)
> >  #define   VGA_MSR_CGA_MODE (1<<0)
> >  
> > -#define VGA_SR_INDEX 0x3c4
> > +#define VGA_SR_INDEX (dev_priv->info->mmio_offset + 0x3c4)
> >  #define SR01			1
> > -#define VGA_SR_DATA 0x3c5
> > +#define VGA_SR_DATA (dev_priv->info->mmio_offset + 0x3c5)
> >  
> > -#define VGA_AR_INDEX 0x3c0
> > +#define VGA_AR_INDEX (dev_priv->info->mmio_offset + 0x3c0)
> >  #define   VGA_AR_VID_EN (1<<5)
> > -#define VGA_AR_DATA_WRITE 0x3c0
> > -#define VGA_AR_DATA_READ 0x3c1
> > +#define VGA_AR_DATA_WRITE (dev_priv->info->mmio_offset + 0x3c0)
> > +#define VGA_AR_DATA_READ (dev_priv->info->mmio_offset + 0x3c1)
> >  
> > -#define VGA_GR_INDEX 0x3ce
> > -#define VGA_GR_DATA 0x3cf
> > +#define VGA_GR_INDEX (dev_priv->info->mmio_offset + 0x3ce)
> > +#define VGA_GR_DATA (dev_priv->info->mmio_offset + 0x3cf)
> >  /* GR05 */
> >  #define   VGA_GR_MEM_READ_MODE_SHIFT 3
> >  #define     VGA_GR_MEM_READ_MODE_PLANE 1
> > @@ -163,15 +163,15 @@
> >  #define   VGA_GR_MEM_B0000_B7FFF 2
> >  #define   VGA_GR_MEM_B0000_BFFFF 3
> >  
> > -#define VGA_DACMASK 0x3c6
> > -#define VGA_DACRX 0x3c7
> > -#define VGA_DACWX 0x3c8
> > -#define VGA_DACDATA 0x3c9
> > +#define VGA_DACMASK (dev_priv->info->mmio_offset + 0x3c6)
> > +#define VGA_DACRX (dev_priv->info->mmio_offset + 0x3c7)
> > +#define VGA_DACWX (dev_priv->info->mmio_offset + 0x3c8)
> > +#define VGA_DACDATA (dev_priv->info->mmio_offset + 0x3c9)
> >  
> > -#define VGA_CR_INDEX_MDA 0x3b4
> > -#define VGA_CR_DATA_MDA 0x3b5
> > -#define VGA_CR_INDEX_CGA 0x3d4
> > -#define VGA_CR_DATA_CGA 0x3d5
> > +#define VGA_CR_INDEX_MDA (dev_priv->info->mmio_offset + 0x3b4)
> > +#define VGA_CR_DATA_MDA (dev_priv->info->mmio_offset + 0x3b5)
> > +#define VGA_CR_INDEX_CGA (dev_priv->info->mmio_offset + 0x3d4)
> > +#define VGA_CR_DATA_CGA (dev_priv->info->mmio_offset + 0x3d5)
> >  
> >  /*
> >   * Memory interface instructions used by the kernel
> > @@ -337,16 +337,16 @@
> >   *  0x8048/68: low pass filter coefficients
> >   *  0x8100: fast clock controls
> >   */
> > -#define DPIO_PKT			0x2100
> > +#define DPIO_PKT			(dev_priv->info->mmio_offset + 0x2100)
> >  #define  DPIO_RID			(0<<24)
> >  #define  DPIO_OP_WRITE			(1<<16)
> >  #define  DPIO_OP_READ			(0<<16)
> >  #define  DPIO_PORTID			(0x12<<8)
> >  #define  DPIO_BYTE			(0xf<<4)
> >  #define  DPIO_BUSY			(1<<0) /* status only */
> > -#define DPIO_DATA			0x2104
> > -#define DPIO_REG			0x2108
> > -#define DPIO_CTL			0x2110
> > +#define DPIO_DATA			(dev_priv->info->mmio_offset + 0x2104)
> > +#define DPIO_REG			(dev_priv->info->mmio_offset + 0x2108)
> > +#define DPIO_CTL			(dev_priv->info->mmio_offset + 0x2110)
> 
> dpio is vlv-only,

Ah, didn't realize that as there's no comment to that effect.
I'll add one.

> so I'm wondering whether it's not just better to use the
> real register offset - that way there's less confusion when poking bits
> directly with intel_reg_read/write.
> 
> >  #define  DPIO_MODSEL1			(1<<3) /* if ref clk b == 27 */
> >  #define  DPIO_MODSEL0			(1<<2) /* if ref clk a == 27 */
> >  #define  DPIO_SFR_BYPASS		(1<<1)
> > @@ -553,17 +553,13 @@
> >  #define VLV_DISPLAY_BASE 0x180000
> >  
> >  #define SCPD0		0x0209c /* 915+ only */
> > -#define IER		0x020a0
> > -#define IIR		0x020a4
> > -#define IMR		0x020a8
> > -#define ISR		0x020ac
> > -#define VLV_GUNIT_CLOCK_GATE	0x182060
> > +#define IER		(dev_priv->info->mmio_offset + 0x20a0)
> > +#define IIR		(dev_priv->info->mmio_offset + 0x20a4)
> > +#define IMR		(dev_priv->info->mmio_offset + 0x20a8)
> > +#define ISR		(dev_priv->info->mmio_offset + 0x20ac)
> > +#define VLV_GUNIT_CLOCK_GATE	(dev_priv->info->mmio_offset + 0x2060)
> >  #define   GCFG_DIS		(1<<8)
> > -#define VLV_IIR_RW	0x182084
> > -#define VLV_IER		0x1820a0
> > -#define VLV_IIR		0x1820a4
> > -#define VLV_IMR		0x1820a8
> > -#define VLV_ISR		0x1820ac
> > +#define VLV_IIR_RW	(dev_priv->info->mmio_offset + 0x2084)
> >  #define   I915_PIPE_CONTROL_NOTIFY_INTERRUPT		(1<<18)
> >  #define   I915_DISPLAY_PORT_INTERRUPT			(1<<17)
> >  #define   I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT	(1<<15)
> > @@ -861,7 +857,7 @@
> >  # define GPIO_DATA_VAL_IN		(1 << 12)
> >  # define GPIO_DATA_PULLUP_DISABLE	(1 << 13)
> >  
> > -#define GMBUS0			0x5100 /* clock/port select */
> > +#define GMBUS0			(dev_priv->info->mmio_offset + 0x5100) /* clock/port select */
> >  #define   GMBUS_RATE_100KHZ	(0<<8)
> >  #define   GMBUS_RATE_50KHZ	(1<<8)
> >  #define   GMBUS_RATE_400KHZ	(2<<8) /* reserved on Pineview */
> > @@ -876,7 +872,7 @@
> >  #define   GMBUS_PORT_DPD	6 /* HDMID */
> >  #define   GMBUS_PORT_RESERVED	7 /* 7 reserved */
> >  #define   GMBUS_NUM_PORTS	(GMBUS_PORT_DPD - GMBUS_PORT_SSC + 1)
> > -#define GMBUS1			0x5104 /* command/status */
> > +#define GMBUS1			(dev_priv->info->mmio_offset + 0x5104) /* command/status */
> >  #define   GMBUS_SW_CLR_INT	(1<<31)
> >  #define   GMBUS_SW_RDY		(1<<30)
> >  #define   GMBUS_ENT		(1<<29) /* enable timeout */
> > @@ -889,7 +885,7 @@
> >  #define   GMBUS_SLAVE_ADDR_SHIFT 1
> >  #define   GMBUS_SLAVE_READ	(1<<0)
> >  #define   GMBUS_SLAVE_WRITE	(0<<0)
> > -#define GMBUS2			0x5108 /* status */
> > +#define GMBUS2			(dev_priv->info->mmio_offset + 0x5108) /* status */
> >  #define   GMBUS_INUSE		(1<<15)
> >  #define   GMBUS_HW_WAIT_PHASE	(1<<14)
> >  #define   GMBUS_STALL_TIMEOUT	(1<<13)
> > @@ -897,23 +893,23 @@
> >  #define   GMBUS_HW_RDY		(1<<11)
> >  #define   GMBUS_SATOER		(1<<10)
> >  #define   GMBUS_ACTIVE		(1<<9)
> > -#define GMBUS3			0x510c /* data buffer bytes 3-0 */
> > -#define GMBUS4			0x5110 /* interrupt mask (Pineview+) */
> > +#define GMBUS3			(dev_priv->info->mmio_offset + 0x510c) /* data buffer bytes 3-0 */
> > +#define GMBUS4			(dev_priv->info->mmio_offset + 0x5110) /* interrupt mask (Pineview+) */
> >  #define   GMBUS_SLAVE_TIMEOUT_EN (1<<4)
> >  #define   GMBUS_NAK_EN		(1<<3)
> >  #define   GMBUS_IDLE_EN		(1<<2)
> >  #define   GMBUS_HW_WAIT_EN	(1<<1)
> >  #define   GMBUS_HW_RDY_EN	(1<<0)
> > -#define GMBUS5			0x5120 /* byte index */
> > +#define GMBUS5			(dev_priv->info->mmio_offset + 0x5120) /* byte index */
> >  #define   GMBUS_2BYTE_INDEX_EN	(1<<31)
> 
> gmbus has a mmio_base handling already, and I think it would be easier to
> simply extend that (just in case someone moves things around). I've
> written a patch a while back, but it seems to have been lost. This could
> even be done in a small prep patch upfront (since all the infrastructure
> is there already).

Right. I accidentally looked at the gma500 code. I really need to kill
those files from my cscope list...

So yeah GMBUS also uses gpio_mmio_base, which means the patch was simply
broken. I'll drop the offset from GMBUS regs.

> >  /*
> >   * Clock control & power management
> >   */
> >  
> > -#define VGA0	0x6000
> > -#define VGA1	0x6004
> > -#define VGA_PD	0x6010
> > +#define VGA0	(dev_priv->info->mmio_offset + 0x6000)
> > +#define VGA1	(dev_priv->info->mmio_offset + 0x6004)
> > +#define VGA_PD	(dev_priv->info->mmio_offset + 0x6010)
> >  #define   VGA0_PD_P2_DIV_4	(1 << 7)
> >  #define   VGA0_PD_P1_DIV_2	(1 << 5)
> >  #define   VGA0_PD_P1_SHIFT	0
> > @@ -922,8 +918,8 @@
> >  #define   VGA1_PD_P1_DIV_2	(1 << 13)
> >  #define   VGA1_PD_P1_SHIFT	8
> >  #define   VGA1_PD_P1_MASK	(0x1f << 8)
> > -#define _DPLL_A	0x06014
> > -#define _DPLL_B	0x06018
> > +#define _DPLL_A	(dev_priv->info->mmio_offset + 0x6014)
> > +#define _DPLL_B	(dev_priv->info->mmio_offset + 0x6018)
> >  #define DPLL(pipe) _PIPE(pipe, _DPLL_A, _DPLL_B)
> >  #define   DPLL_VCO_ENABLE		(1 << 31)
> >  #define   DPLL_DVO_HIGH_SPEED		(1 << 30)
> > @@ -982,7 +978,7 @@
> >  #define   SDVO_MULTIPLIER_MASK			0x000000ff
> >  #define   SDVO_MULTIPLIER_SHIFT_HIRES		4
> >  #define   SDVO_MULTIPLIER_SHIFT_VGA		0
> > -#define _DPLL_A_MD 0x0601c /* 965+ only */
> > +#define _DPLL_A_MD (dev_priv->info->mmio_offset + 0x601c) /* 965+ only */
> >  /*
> >   * UDI pixel divider, controlling how many pixels are stuffed into a packet.
> >   *
> > @@ -1019,13 +1015,13 @@
> >   */
> >  #define   DPLL_MD_VGA_UDI_MULTIPLIER_MASK	0x0000003f
> >  #define   DPLL_MD_VGA_UDI_MULTIPLIER_SHIFT	0
> > -#define _DPLL_B_MD 0x06020 /* 965+ only */
> > +#define _DPLL_B_MD (dev_priv->info->mmio_offset + 0x6020) /* 965+ only */
> >  #define DPLL_MD(pipe) _PIPE(pipe, _DPLL_A_MD, _DPLL_B_MD)
> >  
> > -#define _FPA0	0x06040
> > -#define _FPA1	0x06044
> > -#define _FPB0	0x06048
> > -#define _FPB1	0x0604c
> > +#define _FPA0	(dev_priv->info->mmio_offset + 0x6040)
> > +#define _FPA1	(dev_priv->info->mmio_offset + 0x6044)
> > +#define _FPB0	(dev_priv->info->mmio_offset + 0x6048)
> > +#define _FPB1	(dev_priv->info->mmio_offset + 0x604c)
> >  #define FP0(pipe) _PIPE(pipe, _FPA0, _FPB0)
> >  #define FP1(pipe) _PIPE(pipe, _FPA1, _FPB1)
> >  #define   FP_N_DIV_MASK		0x003f0000
> > @@ -1036,7 +1032,7 @@
> >  #define   FP_M2_DIV_MASK	0x0000003f
> >  #define   FP_M2_PINEVIEW_DIV_MASK	0x000000ff
> >  #define   FP_M2_DIV_SHIFT		 0
> > -#define DPLL_TEST	0x606c
> > +#define DPLL_TEST	(dev_priv->info->mmio_offset + 0x606c)
> >  #define   DPLLB_TEST_SDVO_DIV_1		(0 << 22)
> >  #define   DPLLB_TEST_SDVO_DIV_2		(1 << 22)
> >  #define   DPLLB_TEST_SDVO_DIV_4		(2 << 22)
> > @@ -1047,12 +1043,12 @@
> >  #define   DPLLA_TEST_N_BYPASS		(1 << 3)
> >  #define   DPLLA_TEST_M_BYPASS		(1 << 2)
> >  #define   DPLLA_INPUT_BUFFER_ENABLE	(1 << 0)
> > -#define D_STATE		0x6104
> > +#define D_STATE		(dev_priv->info->mmio_offset + 0x6104)
> >  #define  DSTATE_GFX_RESET_I830			(1<<6)
> >  #define  DSTATE_PLL_D3_OFF			(1<<3)
> >  #define  DSTATE_GFX_CLOCK_GATING		(1<<1)
> >  #define  DSTATE_DOT_CLOCK_GATING		(1<<0)
> > -#define DSPCLK_GATE_D		0x6200
> > +#define DSPCLK_GATE_D		(dev_priv->info->mmio_offset + 0x6200)
> >  # define DPUNIT_B_CLOCK_GATE_DISABLE		(1 << 30) /* 965 */
> >  # define VSUNIT_CLOCK_GATE_DISABLE		(1 << 29) /* 965 */
> >  # define VRHUNIT_CLOCK_GATE_DISABLE		(1 << 28) /* 965 */
> > @@ -1091,7 +1087,7 @@
> >  # define ZVUNIT_CLOCK_GATE_DISABLE		(1 << 0) /* 830 */
> >  # define OVLUNIT_CLOCK_GATE_DISABLE		(1 << 0) /* 845,865 */
> >  
> > -#define RENCLK_GATE_D1		0x6204
> > +#define RENCLK_GATE_D1		(dev_priv->info->mmio_offset + 0x6204)
> >  # define BLITTER_CLOCK_GATE_DISABLE		(1 << 13) /* 945GM only */
> >  # define MPEG_CLOCK_GATE_DISABLE		(1 << 12) /* 945GM only */
> >  # define PC_FE_CLOCK_GATE_DISABLE		(1 << 11)
> > @@ -1155,22 +1151,22 @@
> >  # define I965_FT_CLOCK_GATE_DISABLE		(1 << 1)
> >  # define I965_DM_CLOCK_GATE_DISABLE		(1 << 0)
> >  
> > -#define RENCLK_GATE_D2		0x6208
> > +#define RENCLK_GATE_D2		(dev_priv->info->mmio_offset + 0x6208)
> >  #define VF_UNIT_CLOCK_GATE_DISABLE		(1 << 9)
> >  #define GS_UNIT_CLOCK_GATE_DISABLE		(1 << 7)
> >  #define CL_UNIT_CLOCK_GATE_DISABLE		(1 << 6)
> > -#define RAMCLK_GATE_D		0x6210		/* CRL only */
> > -#define DEUC			0x6214          /* CRL only */
> > +#define RAMCLK_GATE_D		(dev_priv->info->mmio_offset + 0x6210)		/* CRL only */
> > +#define DEUC			(dev_priv->info->mmio_offset + 0x6214)          /* CRL only */
> >  
> > -#define FW_BLC_SELF_VLV		0x6500
> > +#define FW_BLC_SELF_VLV		(dev_priv->info->mmio_offset + 0x6500)
> >  #define  FW_CSPWRDWNEN		(1<<15)
> >  
> >  /*
> >   * Palette regs
> >   */
> >  
> > -#define _PALETTE_A		0x0a000
> > -#define _PALETTE_B		0x0a800
> > +#define _PALETTE_A		(dev_priv->info->mmio_offset + 0xa000)
> > +#define _PALETTE_B		(dev_priv->info->mmio_offset + 0xa800)
> >  #define PALETTE(pipe) _PIPE(pipe, _PALETTE_A, _PALETTE_B)
> >  
> >  /* MCH MMIO space */
> > @@ -1535,26 +1531,26 @@
> >   */
> >  
> >  /* Pipe A timing regs */
> > -#define _HTOTAL_A	0x60000
> > -#define _HBLANK_A	0x60004
> > -#define _HSYNC_A		0x60008
> > -#define _VTOTAL_A	0x6000c
> > -#define _VBLANK_A	0x60010
> > -#define _VSYNC_A		0x60014
> > -#define _PIPEASRC	0x6001c
> > -#define _BCLRPAT_A	0x60020
> > -#define _VSYNCSHIFT_A	0x60028
> > +#define _HTOTAL_A	(dev_priv->info->mmio_offset + 0x60000)
> > +#define _HBLANK_A	(dev_priv->info->mmio_offset + 0x60004)
> > +#define _HSYNC_A		(dev_priv->info->mmio_offset + 0x60008)
> > +#define _VTOTAL_A	(dev_priv->info->mmio_offset + 0x6000c)
> > +#define _VBLANK_A	(dev_priv->info->mmio_offset + 0x60010)
> > +#define _VSYNC_A		(dev_priv->info->mmio_offset + 0x60014)
> > +#define _PIPEASRC	(dev_priv->info->mmio_offset + 0x6001c)
> > +#define _BCLRPAT_A	(dev_priv->info->mmio_offset + 0x60020)
> > +#define _VSYNCSHIFT_A	(dev_priv->info->mmio_offset + 0x60028)
> >  
> >  /* Pipe B timing regs */
> > -#define _HTOTAL_B	0x61000
> > -#define _HBLANK_B	0x61004
> > -#define _HSYNC_B		0x61008
> > -#define _VTOTAL_B	0x6100c
> > -#define _VBLANK_B	0x61010
> > -#define _VSYNC_B		0x61014
> > -#define _PIPEBSRC	0x6101c
> > -#define _BCLRPAT_B	0x61020
> > -#define _VSYNCSHIFT_B	0x61028
> > +#define _HTOTAL_B	(dev_priv->info->mmio_offset + 0x61000)
> > +#define _HBLANK_B	(dev_priv->info->mmio_offset + 0x61004)
> > +#define _HSYNC_B		(dev_priv->info->mmio_offset + 0x61008)
> > +#define _VTOTAL_B	(dev_priv->info->mmio_offset + 0x6100c)
> > +#define _VBLANK_B	(dev_priv->info->mmio_offset + 0x61010)
> > +#define _VSYNC_B		(dev_priv->info->mmio_offset + 0x61014)
> > +#define _PIPEBSRC	(dev_priv->info->mmio_offset + 0x6101c)
> > +#define _BCLRPAT_B	(dev_priv->info->mmio_offset + 0x61020)
> > +#define _VSYNCSHIFT_B	(dev_priv->info->mmio_offset + 0x61028)
> >  
> >  
> >  #define HTOTAL(trans) _TRANSCODER(trans, _HTOTAL_A, _HTOTAL_B)
> > @@ -1567,9 +1563,8 @@
> >  #define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
> >  
> >  /* VGA port control */
> > -#define ADPA			0x61100
> > +#define ADPA			(dev_priv->info->mmio_offset + 0x61100)
> >  #define PCH_ADPA                0xe1100
> > -#define VLV_ADPA		(VLV_DISPLAY_BASE + ADPA)
> 
> The crt encoder should be fully converted to use platform regs already,
> and has a case for VLV (using the above #define). I guess we could just
> leave things as-is for now. If that looks strange, we can always change
> things. But for the inital IS_DISPLAYREG kill patch I'd like to make it as
> small as possible.

Sure. I can split it out (and try to split other things a bit
better too). Then it's easier to decide which bits to take.

> >  #define   ADPA_DAC_ENABLE	(1<<31)
> >  #define   ADPA_DAC_DISABLE	0
> > @@ -1615,7 +1610,7 @@
> >  
> >  
> >  /* Hotplug control (945+ only) */
> > -#define PORT_HOTPLUG_EN		0x61110
> > +#define PORT_HOTPLUG_EN		(dev_priv->info->mmio_offset + 0x61110)
> >  #define   HDMIB_HOTPLUG_INT_EN			(1 << 29)
> >  #define   DPB_HOTPLUG_INT_EN			(1 << 29)
> >  #define   HDMIC_HOTPLUG_INT_EN			(1 << 28)
> > @@ -1642,7 +1637,7 @@
> >  #define CRT_HOTPLUG_DETECT_VOLTAGE_325MV	(0 << 2)
> >  #define CRT_HOTPLUG_DETECT_VOLTAGE_475MV	(1 << 2)
> >  
> > -#define PORT_HOTPLUG_STAT	0x61114
> > +#define PORT_HOTPLUG_STAT	(dev_priv->info->mmio_offset + 0x61114)
> >  /* HDMI/DP bits are gen4+ */
> >  #define   DPB_HOTPLUG_LIVE_STATUS               (1 << 29)
> >  #define   DPC_HOTPLUG_LIVE_STATUS               (1 << 28)
> > @@ -1673,8 +1668,8 @@
> >  #define   SDVOB_HOTPLUG_INT_STATUS_I915		(1 << 6)
> >  
> >  /* SDVO port control */
> > -#define SDVOB			0x61140
> > -#define SDVOC			0x61160
> > +#define SDVOB			(dev_priv->info->mmio_offset + 0x61140)
> > +#define SDVOC			(dev_priv->info->mmio_offset + 0x61160)
> 
> Not sure about this one here, since we shouldn't depend upon the reg
> address anymore in the hdmi encoder code - everything should be using the
> port enum now. Maybe this could again be fixed in a prep patch first if
> there's anything to do?

Yeah, I'll drop intel_sdvo, and fix intel_hdmi to use the port stuff.

> >  #define   SDVO_ENABLE		(1 << 31)
> >  #define   SDVO_PIPE_B_SELECT	(1 << 30)
> >  #define   SDVO_STALL_SELECT	(1 << 29)
> > @@ -1794,12 +1789,12 @@
> >  #define   LVDS_B0B3_POWER_UP		(3 << 2)
> >  
> >  /* Video Data Island Packet control */
> > -#define VIDEO_DIP_DATA		0x61178
> > +#define VIDEO_DIP_DATA		(dev_priv->info->mmio_offset + 0x61178)
> >  /* Read the description of VIDEO_DIP_DATA (before Haswel) or VIDEO_DIP_ECC
> >   * (Haswell and newer) to see which VIDEO_DIP_DATA byte corresponds to each byte
> >   * of the infoframe structure specified by CEA-861. */
> >  #define   VIDEO_DIP_DATA_SIZE	32
> > -#define VIDEO_DIP_CTL		0x61170
> > +#define VIDEO_DIP_CTL		(dev_priv->info->mmio_offset + 0x61170)
> >  /* Pre HSW: */
> >  #define   VIDEO_DIP_ENABLE		(1 << 31)
> >  #define   VIDEO_DIP_PORT_B		(1 << 29)
> > @@ -1828,7 +1823,7 @@
> >  #define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
> >  
> >  /* Panel power sequencing */
> > -#define PP_STATUS	0x61200
> > +#define PP_STATUS	(dev_priv->info->mmio_offset + 0x61200)
> >  #define   PP_ON		(1 << 31)
> >  /*
> >   * Indicates that all dependencies of the panel are on:
> > @@ -1854,14 +1849,14 @@
> >  #define   PP_SEQUENCE_STATE_ON_S1_2	(0xa << 0)
> >  #define   PP_SEQUENCE_STATE_ON_S1_3	(0xb << 0)
> >  #define   PP_SEQUENCE_STATE_RESET	(0xf << 0)
> > -#define PP_CONTROL	0x61204
> > +#define PP_CONTROL	(dev_priv->info->mmio_offset + 0x61204)
> >  #define   POWER_TARGET_ON	(1 << 0)
> > -#define PP_ON_DELAYS	0x61208
> > -#define PP_OFF_DELAYS	0x6120c
> > -#define PP_DIVISOR	0x61210
> > +#define PP_ON_DELAYS	(dev_priv->info->mmio_offset + 0x61208)
> > +#define PP_OFF_DELAYS	(dev_priv->info->mmio_offset + 0x6120c)
> > +#define PP_DIVISOR	(dev_priv->info->mmio_offset + 0x61210)
> 
> We have some if-ladders already in the code, so dunno what we should do
> with the panel power sequencer. But the code is a mess, and vlv adds a 2nd
> panel sequencer making it worse. So on 2nd thought I don't care ;-)
> 
> >  /* Panel fitting */
> > -#define PFIT_CONTROL	0x61230
> > +#define PFIT_CONTROL	(dev_priv->info->mmio_offset + 0x61230)
> >  #define   PFIT_ENABLE		(1 << 31)
> >  #define   PFIT_PIPE_MASK	(3 << 29)
> >  #define   PFIT_PIPE_SHIFT	29
> > @@ -1879,7 +1874,7 @@
> >  #define   PFIT_SCALING_PROGRAMMED (1 << 26)
> >  #define   PFIT_SCALING_PILLAR	(2 << 26)
> >  #define   PFIT_SCALING_LETTER	(3 << 26)
> > -#define PFIT_PGM_RATIOS	0x61234
> > +#define PFIT_PGM_RATIOS	(dev_priv->info->mmio_offset + 0x61234)
> >  /* Pre-965 */
> >  #define		PFIT_VERT_SCALE_SHIFT		20
> >  #define		PFIT_VERT_SCALE_MASK		0xfff00000
> > @@ -1891,10 +1886,10 @@
> >  #define		PFIT_HORIZ_SCALE_SHIFT_965	0
> >  #define		PFIT_HORIZ_SCALE_MASK_965	0x00001fff
> >  
> > -#define PFIT_AUTO_RATIOS 0x61238
> > +#define PFIT_AUTO_RATIOS (dev_priv->info->mmio_offset + 0x61238)
> >  
> >  /* Backlight control */
> > -#define BLC_PWM_CTL2		0x61250 /* 965+ only */
> > +#define BLC_PWM_CTL2		(dev_priv->info->mmio_offset + 0x61250) /* 965+ only */
> >  #define   BLM_PWM_ENABLE		(1 << 31)
> >  #define   BLM_COMBINATION_MODE		(1 << 30) /* gen4 only */
> >  #define   BLM_PIPE_SELECT		(1 << 29)
> > @@ -1913,7 +1908,7 @@
> >  #define   BLM_PHASE_IN_COUNT_MASK	(0xff << 8)
> >  #define   BLM_PHASE_IN_INCR_SHIFT	(0)
> >  #define   BLM_PHASE_IN_INCR_MASK	(0xff << 0)
> > -#define BLC_PWM_CTL		0x61254
> > +#define BLC_PWM_CTL		(dev_priv->info->mmio_offset + 0x61254)
> >  /*
> >   * This is the most significant 15 bits of the number of backlight cycles in a
> >   * complete cycle of the modulated backlight control.
> > @@ -1935,7 +1930,7 @@
> >  #define   BACKLIGHT_DUTY_CYCLE_MASK_PNV		(0xfffe)
> >  #define   BLM_POLARITY_PNV			(1 << 0) /* pnv only */
> >  
> > -#define BLC_HIST_CTL		0x61260
> > +#define BLC_HIST_CTL		(dev_priv->info->mmio_offset + 0x61260)
> 
> Backlight is similar to PP imo, but similarly messy. So going with this
> approach sounds sane.
> 
> >  
> >  /* New registers for PCH-split platforms. Safe where new bits show up, the
> >   * register layout machtes with gen4 BLC_PWM_CTL[12]. */
> > @@ -2431,10 +2426,10 @@
> >  #define TV_V_CHROMA_42		0x684a8
> >  
> >  /* Display Port */
> > -#define DP_A				0x64000 /* eDP */
> > -#define DP_B				0x64100
> > -#define DP_C				0x64200
> > -#define DP_D				0x64300
> > +#define DP_A				(dev_priv->info->mmio_offset + 0x64000) /* eDP */
> > +#define DP_B				(dev_priv->info->mmio_offset + 0x64100)
> > +#define DP_C				(dev_priv->info->mmio_offset + 0x64200)
> > +#define DP_D				(dev_priv->info->mmio_offset + 0x64300)
> >  
> >  #define   DP_PORT_EN			(1 << 31)
> >  #define   DP_PIPEB_SELECT		(1 << 30)
> > @@ -2518,33 +2513,33 @@
> >   * is 20 bytes in each direction, hence the 5 fixed
> >   * data registers
> >   */
> > -#define DPA_AUX_CH_CTL			0x64010
> > -#define DPA_AUX_CH_DATA1		0x64014
> > -#define DPA_AUX_CH_DATA2		0x64018
> > -#define DPA_AUX_CH_DATA3		0x6401c
> > -#define DPA_AUX_CH_DATA4		0x64020
> > -#define DPA_AUX_CH_DATA5		0x64024
> > +#define DPA_AUX_CH_CTL			(dev_priv->info->mmio_offset + 0x64010)
> > +#define DPA_AUX_CH_DATA1		(dev_priv->info->mmio_offset + 0x64014)
> > +#define DPA_AUX_CH_DATA2		(dev_priv->info->mmio_offset + 0x64018)
> > +#define DPA_AUX_CH_DATA3		(dev_priv->info->mmio_offset + 0x6401c)
> > +#define DPA_AUX_CH_DATA4		(dev_priv->info->mmio_offset + 0x64020)
> > +#define DPA_AUX_CH_DATA5		(dev_priv->info->mmio_offset + 0x64024)
> >  
> > -#define DPB_AUX_CH_CTL			0x64110
> > -#define DPB_AUX_CH_DATA1		0x64114
> > -#define DPB_AUX_CH_DATA2		0x64118
> > -#define DPB_AUX_CH_DATA3		0x6411c
> > -#define DPB_AUX_CH_DATA4		0x64120
> > -#define DPB_AUX_CH_DATA5		0x64124
> > +#define DPB_AUX_CH_CTL			(dev_priv->info->mmio_offset + 0x64110)
> > +#define DPB_AUX_CH_DATA1		(dev_priv->info->mmio_offset + 0x64114)
> > +#define DPB_AUX_CH_DATA2		(dev_priv->info->mmio_offset + 0x64118)
> > +#define DPB_AUX_CH_DATA3		(dev_priv->info->mmio_offset + 0x6411c)
> > +#define DPB_AUX_CH_DATA4		(dev_priv->info->mmio_offset + 0x64120)
> > +#define DPB_AUX_CH_DATA5		(dev_priv->info->mmio_offset + 0x64124)
> >  
> > -#define DPC_AUX_CH_CTL			0x64210
> > -#define DPC_AUX_CH_DATA1		0x64214
> > -#define DPC_AUX_CH_DATA2		0x64218
> > -#define DPC_AUX_CH_DATA3		0x6421c
> > -#define DPC_AUX_CH_DATA4		0x64220
> > -#define DPC_AUX_CH_DATA5		0x64224
> > +#define DPC_AUX_CH_CTL			(dev_priv->info->mmio_offset + 0x64210)
> > +#define DPC_AUX_CH_DATA1		(dev_priv->info->mmio_offset + 0x64214)
> > +#define DPC_AUX_CH_DATA2		(dev_priv->info->mmio_offset + 0x64218)
> > +#define DPC_AUX_CH_DATA3		(dev_priv->info->mmio_offset + 0x6421c)
> > +#define DPC_AUX_CH_DATA4		(dev_priv->info->mmio_offset + 0x64220)
> > +#define DPC_AUX_CH_DATA5		(dev_priv->info->mmio_offset + 0x64224)
> >  
> > -#define DPD_AUX_CH_CTL			0x64310
> > -#define DPD_AUX_CH_DATA1		0x64314
> > -#define DPD_AUX_CH_DATA2		0x64318
> > -#define DPD_AUX_CH_DATA3		0x6431c
> > -#define DPD_AUX_CH_DATA4		0x64320
> > -#define DPD_AUX_CH_DATA5		0x64324
> > +#define DPD_AUX_CH_CTL			(dev_priv->info->mmio_offset + 0x64310)
> > +#define DPD_AUX_CH_DATA1		(dev_priv->info->mmio_offset + 0x64314)
> > +#define DPD_AUX_CH_DATA2		(dev_priv->info->mmio_offset + 0x64318)
> > +#define DPD_AUX_CH_DATA3		(dev_priv->info->mmio_offset + 0x6431c)
> > +#define DPD_AUX_CH_DATA4		(dev_priv->info->mmio_offset + 0x64320)
> > +#define DPD_AUX_CH_DATA5		(dev_priv->info->mmio_offset + 0x64324)
> 
> DP is similar to hdmi, we (mostly) shouldn't rely the register address,
> but instead use the port enum. To avoid tons of duplicate #defines maybe
> just pass a DP_A + DISPLAYREG_BASE_VLV instead of a DP_A to intel_dp_init
> in intel_setup_outputs. Also, we don't use most of the DPx_AUX #defines
> (instead computing its address from the base reg address).

Can do. I need to fix that one switch statement to use the port enum.

> >  #define   DP_AUX_CH_CTL_SEND_BUSY	    (1 << 31)
> >  #define   DP_AUX_CH_CTL_DONE		    (1 << 30)
> > @@ -2581,8 +2576,8 @@
> >   * which is after the LUTs, so we want the bytes for our color format.
> >   * For our current usage, this is always 3, one byte for R, G and B.
> >   */
> > -#define _PIPEA_GMCH_DATA_M			0x70050
> > -#define _PIPEB_GMCH_DATA_M			0x71050
> > +#define _PIPEA_GMCH_DATA_M			(dev_priv->info->mmio_offset + 0x70050)
> > +#define _PIPEB_GMCH_DATA_M			(dev_priv->info->mmio_offset + 0x71050)
> >  
> >  /* Transfer unit size for display port - 1, default is 0x3f (for TU size 64) */
> >  #define   PIPE_GMCH_DATA_M_TU_SIZE_MASK		(0x3f << 25)
> > @@ -2590,8 +2585,8 @@
> >  
> >  #define   PIPE_GMCH_DATA_M_MASK			(0xffffff)
> >  
> > -#define _PIPEA_GMCH_DATA_N			0x70054
> > -#define _PIPEB_GMCH_DATA_N			0x71054
> > +#define _PIPEA_GMCH_DATA_N			(dev_priv->info->mmio_offset + 0x70054)
> > +#define _PIPEB_GMCH_DATA_N			(dev_priv->info->mmio_offset + 0x71054)
> >  #define   PIPE_GMCH_DATA_N_MASK			(0xffffff)
> >  
> >  /*
> > @@ -2605,12 +2600,12 @@
> >   * Attributes and VB-ID.
> >   */
> >  
> > -#define _PIPEA_DP_LINK_M				0x70060
> > -#define _PIPEB_DP_LINK_M				0x71060
> > +#define _PIPEA_DP_LINK_M				(dev_priv->info->mmio_offset + 0x70060)
> > +#define _PIPEB_DP_LINK_M				(dev_priv->info->mmio_offset + 0x71060)
> >  #define   PIPEA_DP_LINK_M_MASK			(0xffffff)
> >  
> > -#define _PIPEA_DP_LINK_N				0x70064
> > -#define _PIPEB_DP_LINK_N				0x71064
> > +#define _PIPEA_DP_LINK_N				(dev_priv->info->mmio_offset + 0x70064)
> > +#define _PIPEB_DP_LINK_N				(dev_priv->info->mmio_offset + 0x71064)
> >  #define   PIPEA_DP_LINK_N_MASK			(0xffffff)
> 
> Afaict the above pipe/data m/n register are only used on gen4, so no need
> to convert them over (check out the big if-ladder in intel_dp.c).

Yeah, looks like you're right. I'll undo the conversion for these guys.

> >  
> >  #define PIPE_GMCH_DATA_M(pipe) _PIPE(pipe, _PIPEA_GMCH_DATA_M, _PIPEB_GMCH_DATA_M)
> > @@ -2621,10 +2616,10 @@
> >  /* Display & cursor control */
> >  
> >  /* Pipe A */
> > -#define _PIPEADSL		0x70000
> > +#define _PIPEADSL		(dev_priv->info->mmio_offset + 0x70000)
> >  #define   DSL_LINEMASK_GEN2	0x00000fff
> >  #define   DSL_LINEMASK_GEN3	0x00001fff
> > -#define _PIPEACONF		0x70008
> > +#define _PIPEACONF		(dev_priv->info->mmio_offset + 0x70008)
> >  #define   PIPECONF_ENABLE	(1<<31)
> >  #define   PIPECONF_DISABLE	0
> >  #define   PIPECONF_DOUBLE_WIDE	(1<<30)
> > @@ -2665,7 +2660,7 @@
> >  #define   PIPECONF_DITHER_TYPE_ST1 (1<<2)
> >  #define   PIPECONF_DITHER_TYPE_ST2 (2<<2)
> >  #define   PIPECONF_DITHER_TYPE_TEMP (3<<2)
> > -#define _PIPEASTAT		0x70024
> > +#define _PIPEASTAT		(dev_priv->info->mmio_offset + 0x70024)
> >  #define   PIPE_FIFO_UNDERRUN_STATUS		(1UL<<31)
> >  #define   SPRITE1_FLIPDONE_INT_EN_VLV		(1UL<<30)
> >  #define   PIPE_CRC_ERROR_ENABLE			(1UL<<29)
> > @@ -2710,7 +2705,7 @@
> >  #define PIPEFRAMEPIXEL(pipe)  _PIPE(pipe, _PIPEAFRAMEPIXEL, _PIPEBFRAMEPIXEL)
> >  #define PIPESTAT(pipe) _PIPE(pipe, _PIPEASTAT, _PIPEBSTAT)
> >  
> > -#define VLV_DPFLIPSTAT				0x70028
> > +#define VLV_DPFLIPSTAT				(dev_priv->info->mmio_offset + 0x70028)
> 
> Imo registers with a VLV pre/postfix should just use the address with the
> right offset (maybe make it explicit with (VLV_DISPLAY_BASE + 0xfoo) so
> that it's easier to grep in the docs).

Can do.
 
> >  #define   PIPEB_LINE_COMPARE_INT_EN		(1<<29)
> >  #define   PIPEB_HLINE_INT_EN			(1<<28)
> >  #define   PIPEB_VBLANK_INT_EN			(1<<27)
> > @@ -2724,7 +2719,7 @@
> >  #define   SPRITEA_FLIPDONE_INT_EN		(1<<17)
> >  #define   PLANEA_FLIPDONE_INT_EN		(1<<16)
> >  
> > -#define DPINVGTT				0x7002c /* VLV only */
> 
> Not a VLV prefix, but says "vlv only" in the comment. It's also only used
> in the irq code, so that would keep in line with my bikeshed to keep the
> VLV prefix for (most) irq regs. Maybe split this out into a prep patch ...

Sure.

> > +#define DPINVGTT				(dev_priv->info->mmio_offset + 0x7002c) /* VLV only */
> >  #define   CURSORB_INVALID_GTT_INT_EN		(1<<23)
> >  #define   CURSORA_INVALID_GTT_INT_EN		(1<<22)
> >  #define   SPRITED_INVALID_GTT_INT_EN		(1<<21)
> > @@ -2744,7 +2739,7 @@
> >  #define   PLANEA_INVALID_GTT_STATUS		(1<<0)
> >  #define   DPINVGTT_STATUS_MASK			0xff
> >  
> > -#define DSPARB			0x70030
> > +#define DSPARB			(dev_priv->info->mmio_offset + 0x70030)
> 
> DSPARB is gen3/4 iirc, so no need to adjust.

According to my docs VLV has it too.

> >  #define   DSPARB_CSTART_MASK	(0x7f << 7)
> >  #define   DSPARB_CSTART_SHIFT	7
> >  #define   DSPARB_BSTART_MASK	(0x7f)
> > @@ -2752,7 +2747,7 @@
> >  #define   DSPARB_BEND_SHIFT	9 /* on 855 */
> >  #define   DSPARB_AEND_SHIFT	0
> >  
> > -#define DSPFW1			0x70034
> > +#define DSPFW1			(dev_priv->info->mmio_offset + 0x70034)
> >  #define   DSPFW_SR_SHIFT	23
> >  #define   DSPFW_SR_MASK		(0x1ff<<23)
> >  #define   DSPFW_CURSORB_SHIFT	16
> > @@ -2760,11 +2755,11 @@
> >  #define   DSPFW_PLANEB_SHIFT	8
> >  #define   DSPFW_PLANEB_MASK	(0x7f<<8)
> >  #define   DSPFW_PLANEA_MASK	(0x7f)
> > -#define DSPFW2			0x70038
> > +#define DSPFW2			(dev_priv->info->mmio_offset + 0x70038)
> >  #define   DSPFW_CURSORA_MASK	0x00003f00
> >  #define   DSPFW_CURSORA_SHIFT	8
> >  #define   DSPFW_PLANEC_MASK	(0x7f)
> > -#define DSPFW3			0x7003c
> > +#define DSPFW3			(dev_priv->info->mmio_offset + 0x7003c)
> >  #define   DSPFW_HPLL_SR_EN	(1<<31)
> >  #define   DSPFW_CURSOR_SR_SHIFT	24
> >  #define   PINEVIEW_SELF_REFRESH_EN	(1<<30)
> 
> Above FW regs are shared, so need this.
> 
> > @@ -2776,13 +2771,13 @@
> >  /* drain latency register values*/
> >  #define DRAIN_LATENCY_PRECISION_32	32
> >  #define DRAIN_LATENCY_PRECISION_16	16
> > -#define VLV_DDL1			0x70050
> > +#define VLV_DDL1			(dev_priv->info->mmio_offset + 0x70050)
> >  #define DDL_CURSORA_PRECISION_32	(1<<31)
> >  #define DDL_CURSORA_PRECISION_16	(0<<31)
> >  #define DDL_CURSORA_SHIFT		24
> >  #define DDL_PLANEA_PRECISION_32		(1<<7)
> >  #define DDL_PLANEA_PRECISION_16		(0<<7)
> > -#define VLV_DDL2			0x70054
> > +#define VLV_DDL2			(dev_priv->info->mmio_offset + 0x70054)
> 
> Again, I wonder whether it's not better to bake the offset into the reg
> #define here ...
> 
> >  #define DDL_CURSORB_PRECISION_32	(1<<31)
> >  #define DDL_CURSORB_PRECISION_16	(0<<31)
> >  #define DDL_CURSORB_SHIFT		24
> > @@ -2926,21 +2921,21 @@
> >   *  } while (high1 != high2);
> >   *  frame = (high1 << 8) | low1;
> >   */
> > -#define _PIPEAFRAMEHIGH          0x70040
> > +#define _PIPEAFRAMEHIGH          (dev_priv->info->mmio_offset + 0x70040)
> >  #define   PIPE_FRAME_HIGH_MASK    0x0000ffff
> >  #define   PIPE_FRAME_HIGH_SHIFT   0
> > -#define _PIPEAFRAMEPIXEL         0x70044
> > +#define _PIPEAFRAMEPIXEL         (dev_priv->info->mmio_offset + 0x70044)
> >  #define   PIPE_FRAME_LOW_MASK     0xff000000
> >  #define   PIPE_FRAME_LOW_SHIFT    24
> >  #define   PIPE_PIXEL_MASK         0x00ffffff
> >  #define   PIPE_PIXEL_SHIFT        0
> >  /* GM45+ just has to be different */
> > -#define _PIPEA_FRMCOUNT_GM45	0x70040
> > -#define _PIPEA_FLIPCOUNT_GM45	0x70044
> > +#define _PIPEA_FRMCOUNT_GM45	(dev_priv->info->mmio_offset + 0x70040)
> > +#define _PIPEA_FLIPCOUNT_GM45	(dev_priv->info->mmio_offset + 0x70044)
> >  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FRMCOUNT_GM45, _PIPEB_FRMCOUNT_GM45)
> >  
> >  /* Cursor A & B regs */
> > -#define _CURACNTR		0x70080
> > +#define _CURACNTR		(dev_priv->info->mmio_offset + 0x70080)
> >  /* Old style CUR*CNTR flags (desktop 8xx) */
> >  #define   CURSOR_ENABLE		0x80000000
> >  #define   CURSOR_GAMMA_ENABLE	0x40000000
> > @@ -2961,20 +2956,20 @@
> >  #define   MCURSOR_PIPE_A	0x00
> >  #define   MCURSOR_PIPE_B	(1 << 28)
> >  #define   MCURSOR_GAMMA_ENABLE  (1 << 26)
> > -#define _CURABASE		0x70084
> > -#define _CURAPOS			0x70088
> > +#define _CURABASE		(dev_priv->info->mmio_offset + 0x70084)
> > +#define _CURAPOS		(dev_priv->info->mmio_offset + 0x70088)
> >  #define   CURSOR_POS_MASK       0x007FF
> >  #define   CURSOR_POS_SIGN       0x8000
> >  #define   CURSOR_X_SHIFT        0
> >  #define   CURSOR_Y_SHIFT        16
> > -#define CURSIZE			0x700a0
> > -#define _CURBCNTR		0x700c0
> > -#define _CURBBASE		0x700c4
> > -#define _CURBPOS			0x700c8
> > +#define CURSIZE			(dev_priv->info->mmio_offset + 0x700a0)
> > +#define _CURBCNTR		(dev_priv->info->mmio_offset + 0x700c0)
> > +#define _CURBBASE		(dev_priv->info->mmio_offset + 0x700c4)
> > +#define _CURBPOS		(dev_priv->info->mmio_offset + 0x700c8)
> >  
> > -#define _CURBCNTR_IVB		0x71080
> > -#define _CURBBASE_IVB		0x71084
> > -#define _CURBPOS_IVB		0x71088
> > +#define _CURBCNTR_IVB		(dev_priv->info->mmio_offset + 0x71080)
> > +#define _CURBBASE_IVB		(dev_priv->info->mmio_offset + 0x71084)
> > +#define _CURBPOS_IVB		(dev_priv->info->mmio_offset + 0x71088)
> >  
> >  #define CURCNTR(pipe) _PIPE(pipe, _CURACNTR, _CURBCNTR)
> >  #define CURBASE(pipe) _PIPE(pipe, _CURABASE, _CURBBASE)
> > @@ -2985,7 +2980,7 @@
> >  #define CURPOS_IVB(pipe) _PIPE(pipe, _CURAPOS, _CURBPOS_IVB)
> >  
> >  /* Display A control */
> > -#define _DSPACNTR                0x70180
> > +#define _DSPACNTR                (dev_priv->info->mmio_offset + 0x70180)
> >  #define   DISPLAY_PLANE_ENABLE			(1<<31)
> >  #define   DISPLAY_PLANE_DISABLE			0
> >  #define   DISPPLANE_GAMMA_ENABLE		(1<<30)
> > @@ -3018,14 +3013,14 @@
> >  #define   DISPPLANE_STEREO_POLARITY_SECOND	(1<<18)
> >  #define   DISPPLANE_TRICKLE_FEED_DISABLE	(1<<14) /* Ironlake */
> >  #define   DISPPLANE_TILED			(1<<10)
> > -#define _DSPAADDR		0x70184
> > -#define _DSPASTRIDE		0x70188
> > -#define _DSPAPOS			0x7018C /* reserved */
> > -#define _DSPASIZE		0x70190
> > -#define _DSPASURF		0x7019C /* 965+ only */
> > -#define _DSPATILEOFF		0x701A4 /* 965+ only */
> > -#define _DSPAOFFSET		0x701A4 /* HSW */
> > -#define _DSPASURFLIVE		0x701AC
> > +#define _DSPAADDR		(dev_priv->info->mmio_offset + 0x70184)
> > +#define _DSPASTRIDE		(dev_priv->info->mmio_offset + 0x70188)
> > +#define _DSPAPOS		(dev_priv->info->mmio_offset + 0x7018C) /* reserved */
> > +#define _DSPASIZE		(dev_priv->info->mmio_offset + 0x70190)
> > +#define _DSPASURF		(dev_priv->info->mmio_offset + 0x7019C) /* 965+ only */
> > +#define _DSPATILEOFF		(dev_priv->info->mmio_offset + 0x701A4) /* 965+ only */
> > +#define _DSPAOFFSET		(dev_priv->info->mmio_offset + 0x701A4) /* HSW */
> > +#define _DSPASURFLIVE		(dev_priv->info->mmio_offset + 0x701AC)
> >  
> >  #define DSPCNTR(plane) _PIPE(plane, _DSPACNTR, _DSPBCNTR)
> >  #define DSPADDR(plane) _PIPE(plane, _DSPAADDR, _DSPBADDR)
> > @@ -3046,47 +3041,46 @@
> >  		(I915_WRITE((reg), (gfx_addr) | I915_LO_DISPBASE(I915_READ(reg))))
> >  
> >  /* VBIOS flags */
> > -#define SWF00			0x71410
> > -#define SWF01			0x71414
> > -#define SWF02			0x71418
> > -#define SWF03			0x7141c
> > -#define SWF04			0x71420
> > -#define SWF05			0x71424
> > -#define SWF06			0x71428
> > -#define SWF10			0x70410
> > -#define SWF11			0x70414
> > -#define SWF14			0x71420
> > -#define SWF30			0x72414
> > -#define SWF31			0x72418
> > -#define SWF32			0x7241c
> > +#define SWF00			(dev_priv->info->mmio_offset + 0x71410)
> > +#define SWF01			(dev_priv->info->mmio_offset + 0x71414)
> > +#define SWF02			(dev_priv->info->mmio_offset + 0x71418)
> > +#define SWF03			(dev_priv->info->mmio_offset + 0x7141c)
> > +#define SWF04			(dev_priv->info->mmio_offset + 0x71420)
> > +#define SWF05			(dev_priv->info->mmio_offset + 0x71424)
> > +#define SWF06			(dev_priv->info->mmio_offset + 0x71428)
> > +#define SWF10			(dev_priv->info->mmio_offset + 0x70410)
> > +#define SWF11			(dev_priv->info->mmio_offset + 0x70414)
> > +#define SWF14			(dev_priv->info->mmio_offset + 0x71420)
> > +#define SWF30			(dev_priv->info->mmio_offset + 0x72414)
> > +#define SWF31			(dev_priv->info->mmio_offset + 0x72418)
> > +#define SWF32			(dev_priv->info->mmio_offset + 0x7241c)
> 
> This block of registers seems to be completely unused in our code, so imo
> better drop this hunk.

I can drop them. They're just scratch regs anyways.

> >  /* Pipe B */
> > -#define _PIPEBDSL		0x71000
> > -#define _PIPEBCONF		0x71008
> > -#define _PIPEBSTAT		0x71024
> > -#define _PIPEBFRAMEHIGH		0x71040
> > -#define _PIPEBFRAMEPIXEL		0x71044
> > -#define _PIPEB_FRMCOUNT_GM45	0x71040
> > -#define _PIPEB_FLIPCOUNT_GM45	0x71044
> > -
> > +#define _PIPEBDSL		(dev_priv->info->mmio_offset + 0x71000)
> > +#define _PIPEBCONF		(dev_priv->info->mmio_offset + 0x71008)
> > +#define _PIPEBSTAT		(dev_priv->info->mmio_offset + 0x71024)
> > +#define _PIPEBFRAMEHIGH		(dev_priv->info->mmio_offset + 0x71040)
> > +#define _PIPEBFRAMEPIXEL		(dev_priv->info->mmio_offset + 0x71044)
> > +#define _PIPEB_FRMCOUNT_GM45	(dev_priv->info->mmio_offset + 0x71040)
> > +#define _PIPEB_FLIPCOUNT_GM45	(dev_priv->info->mmio_offset + 0x71044)
> >  
> >  /* Display B control */
> > -#define _DSPBCNTR		0x71180
> > +#define _DSPBCNTR		(dev_priv->info->mmio_offset + 0x71180)
> >  #define   DISPPLANE_ALPHA_TRANS_ENABLE		(1<<15)
> >  #define   DISPPLANE_ALPHA_TRANS_DISABLE		0
> >  #define   DISPPLANE_SPRITE_ABOVE_DISPLAY	0
> >  #define   DISPPLANE_SPRITE_ABOVE_OVERLAY	(1)
> > -#define _DSPBADDR		0x71184
> > -#define _DSPBSTRIDE		0x71188
> > -#define _DSPBPOS			0x7118C
> > -#define _DSPBSIZE		0x71190
> > -#define _DSPBSURF		0x7119C
> > -#define _DSPBTILEOFF		0x711A4
> > -#define _DSPBOFFSET		0x711A4
> > -#define _DSPBSURFLIVE		0x711AC
> > +#define _DSPBADDR		(dev_priv->info->mmio_offset + 0x71184)
> > +#define _DSPBSTRIDE		(dev_priv->info->mmio_offset + 0x71188)
> > +#define _DSPBPOS			(dev_priv->info->mmio_offset + 0x7118C)
> > +#define _DSPBSIZE		(dev_priv->info->mmio_offset + 0x71190)
> > +#define _DSPBSURF		(dev_priv->info->mmio_offset + 0x7119C)
> > +#define _DSPBTILEOFF		(dev_priv->info->mmio_offset + 0x711A4)
> > +#define _DSPBOFFSET		(dev_priv->info->mmio_offset + 0x711A4)
> > +#define _DSPBSURFLIVE		(dev_priv->info->mmio_offset + 0x711AC)
> >  
> >  /* Sprite A control */
> > -#define _DVSACNTR		0x72180
> > +#define _DVSACNTR		(dev_priv->info->mmio_offset + 0x72180)
> >  #define   DVS_ENABLE		(1<<31)
> >  #define   DVS_GAMMA_ENABLE	(1<<30)
> >  #define   DVS_PIXFORMAT_MASK	(3<<25)
> > @@ -3104,17 +3098,17 @@
> >  #define   DVS_DEST_KEY		(1<<2)
> >  #define   DVS_TRICKLE_FEED_DISABLE (1<<14)
> >  #define   DVS_TILED		(1<<10)
> > -#define _DVSALINOFF		0x72184
> > -#define _DVSASTRIDE		0x72188
> > -#define _DVSAPOS		0x7218c
> > -#define _DVSASIZE		0x72190
> > -#define _DVSAKEYVAL		0x72194
> > -#define _DVSAKEYMSK		0x72198
> > -#define _DVSASURF		0x7219c
> > -#define _DVSAKEYMAXVAL		0x721a0
> > -#define _DVSATILEOFF		0x721a4
> > -#define _DVSASURFLIVE		0x721ac
> > -#define _DVSASCALE		0x72204
> > +#define _DVSALINOFF		(dev_priv->info->mmio_offset + 0x72184)
> > +#define _DVSASTRIDE		(dev_priv->info->mmio_offset + 0x72188)
> > +#define _DVSAPOS		(dev_priv->info->mmio_offset + 0x7218c)
> > +#define _DVSASIZE		(dev_priv->info->mmio_offset + 0x72190)
> > +#define _DVSAKEYVAL		(dev_priv->info->mmio_offset + 0x72194)
> > +#define _DVSAKEYMSK		(dev_priv->info->mmio_offset + 0x72198)
> > +#define _DVSASURF		(dev_priv->info->mmio_offset + 0x7219c)
> > +#define _DVSAKEYMAXVAL		(dev_priv->info->mmio_offset + 0x721a0)
> > +#define _DVSATILEOFF		(dev_priv->info->mmio_offset + 0x721a4)
> > +#define _DVSASURFLIVE		(dev_priv->info->mmio_offset + 0x721ac)
> > +#define _DVSASCALE		(dev_priv->info->mmio_offset + 0x72204)
> >  #define   DVS_SCALE_ENABLE	(1<<31)
> >  #define   DVS_FILTER_MASK	(3<<29)
> >  #define   DVS_FILTER_MEDIUM	(0<<29)
> > @@ -3122,21 +3116,21 @@
> >  #define   DVS_FILTER_SOFTENING	(2<<29)
> >  #define   DVS_VERTICAL_OFFSET_HALF (1<<28) /* must be enabled below */
> >  #define   DVS_VERTICAL_OFFSET_ENABLE (1<<27)
> > -#define _DVSAGAMC		0x72300
> > +#define _DVSAGAMC		(dev_priv->info->mmio_offset + 0x72300)
> >  
> > -#define _DVSBCNTR		0x73180
> > -#define _DVSBLINOFF		0x73184
> > -#define _DVSBSTRIDE		0x73188
> > -#define _DVSBPOS		0x7318c
> > -#define _DVSBSIZE		0x73190
> > -#define _DVSBKEYVAL		0x73194
> > -#define _DVSBKEYMSK		0x73198
> > -#define _DVSBSURF		0x7319c
> > -#define _DVSBKEYMAXVAL		0x731a0
> > -#define _DVSBTILEOFF		0x731a4
> > -#define _DVSBSURFLIVE		0x731ac
> > -#define _DVSBSCALE		0x73204
> > -#define _DVSBGAMC		0x73300
> > +#define _DVSBCNTR		(dev_priv->info->mmio_offset + 0x73180)
> > +#define _DVSBLINOFF		(dev_priv->info->mmio_offset + 0x73184)
> > +#define _DVSBSTRIDE		(dev_priv->info->mmio_offset + 0x73188)
> > +#define _DVSBPOS		(dev_priv->info->mmio_offset + 0x7318c)
> > +#define _DVSBSIZE		(dev_priv->info->mmio_offset + 0x73190)
> > +#define _DVSBKEYVAL		(dev_priv->info->mmio_offset + 0x73194)
> > +#define _DVSBKEYMSK		(dev_priv->info->mmio_offset + 0x73198)
> > +#define _DVSBSURF		(dev_priv->info->mmio_offset + 0x7319c)
> > +#define _DVSBKEYMAXVAL		(dev_priv->info->mmio_offset + 0x731a0)
> > +#define _DVSBTILEOFF		(dev_priv->info->mmio_offset + 0x731a4)
> > +#define _DVSBSURFLIVE		(dev_priv->info->mmio_offset + 0x731ac)
> > +#define _DVSBSCALE		(dev_priv->info->mmio_offset + 0x73204)
> > +#define _DVSBGAMC		(dev_priv->info->mmio_offset + 0x73300)
> >  
> >  #define DVSCNTR(pipe) _PIPE(pipe, _DVSACNTR, _DVSBCNTR)
> >  #define DVSLINOFF(pipe) _PIPE(pipe, _DVSALINOFF, _DVSBLINOFF)
> > @@ -3151,7 +3145,7 @@
> >  #define DVSKEYMSK(pipe) _PIPE(pipe, _DVSAKEYMSK, _DVSBKEYMSK)
> >  #define DVSSURFLIVE(pipe) _PIPE(pipe, _DVSASURFLIVE, _DVSBSURFLIVE)
> 
> Iirc the above is the ilk/snb sprite block, but vlv has a ivb/hsw-like
> sprite block, which follows below. So again imo better to drop the above
> hunk.

I dislike out the duplication in the sprite regs. All the regs seem
pretty much identical even if their name has changed from DVS to
SPR. It might make sense to have just some kind of sprite_offset
handling. In fact all planes in general share quite a bit, so some
kind of bigger refactoring may be warranted, especially when we get
around to exposing the primary plane as a drm_plane.

But I'll drop the DVS register conversion for now.

> > -#define _SPRA_CTL		0x70280
> > +#define _SPRA_CTL		(dev_priv->info->mmio_offset + 0x70280)
> >  #define   SPRITE_ENABLE			(1<<31)
> >  #define   SPRITE_GAMMA_ENABLE		(1<<30)
> >  #define   SPRITE_PIXFORMAT_MASK		(7<<25)
> > @@ -3175,18 +3169,18 @@
> >  #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> >  #define   SPRITE_TILED			(1<<10)
> >  #define   SPRITE_DEST_KEY		(1<<2)
> > -#define _SPRA_LINOFF		0x70284
> > -#define _SPRA_STRIDE		0x70288
> > -#define _SPRA_POS		0x7028c
> > -#define _SPRA_SIZE		0x70290
> > -#define _SPRA_KEYVAL		0x70294
> > -#define _SPRA_KEYMSK		0x70298
> > -#define _SPRA_SURF		0x7029c
> > -#define _SPRA_KEYMAX		0x702a0
> > -#define _SPRA_TILEOFF		0x702a4
> > -#define _SPRA_OFFSET		0x702a4
> > -#define _SPRA_SURFLIVE		0x702ac
> > -#define _SPRA_SCALE		0x70304
> > +#define _SPRA_LINOFF		(dev_priv->info->mmio_offset + 0x70284)
> > +#define _SPRA_STRIDE		(dev_priv->info->mmio_offset + 0x70288)
> > +#define _SPRA_POS		(dev_priv->info->mmio_offset + 0x7028c)
> > +#define _SPRA_SIZE		(dev_priv->info->mmio_offset + 0x70290)
> > +#define _SPRA_KEYVAL		(dev_priv->info->mmio_offset + 0x70294)
> > +#define _SPRA_KEYMSK		(dev_priv->info->mmio_offset + 0x70298)
> > +#define _SPRA_SURF		(dev_priv->info->mmio_offset + 0x7029c)
> > +#define _SPRA_KEYMAX		(dev_priv->info->mmio_offset + 0x702a0)
> > +#define _SPRA_TILEOFF		(dev_priv->info->mmio_offset + 0x702a4)
> > +#define _SPRA_OFFSET		(dev_priv->info->mmio_offset + 0x702a4)
> > +#define _SPRA_SURFLIVE		(dev_priv->info->mmio_offset + 0x702ac)
> > +#define _SPRA_SCALE		(dev_priv->info->mmio_offset + 0x70304)
> >  #define   SPRITE_SCALE_ENABLE	(1<<31)
> >  #define   SPRITE_FILTER_MASK	(3<<29)
> >  #define   SPRITE_FILTER_MEDIUM	(0<<29)
> > @@ -3194,22 +3188,22 @@
> >  #define   SPRITE_FILTER_SOFTENING	(2<<29)
> >  #define   SPRITE_VERTICAL_OFFSET_HALF	(1<<28) /* must be enabled below */
> >  #define   SPRITE_VERTICAL_OFFSET_ENABLE	(1<<27)
> > -#define _SPRA_GAMC		0x70400
> > +#define _SPRA_GAMC		(dev_priv->info->mmio_offset + 0x70400)
> >  
> > -#define _SPRB_CTL		0x71280
> > -#define _SPRB_LINOFF		0x71284
> > -#define _SPRB_STRIDE		0x71288
> > -#define _SPRB_POS		0x7128c
> > -#define _SPRB_SIZE		0x71290
> > -#define _SPRB_KEYVAL		0x71294
> > -#define _SPRB_KEYMSK		0x71298
> > -#define _SPRB_SURF		0x7129c
> > -#define _SPRB_KEYMAX		0x712a0
> > -#define _SPRB_TILEOFF		0x712a4
> > -#define _SPRB_OFFSET		0x712a4
> > -#define _SPRB_SURFLIVE		0x712ac
> > -#define _SPRB_SCALE		0x71304
> > -#define _SPRB_GAMC		0x71400
> > +#define _SPRB_CTL		(dev_priv->info->mmio_offset + 0x71280)
> > +#define _SPRB_LINOFF		(dev_priv->info->mmio_offset + 0x71284)
> > +#define _SPRB_STRIDE		(dev_priv->info->mmio_offset + 0x71288)
> > +#define _SPRB_POS		(dev_priv->info->mmio_offset + 0x7128c)
> > +#define _SPRB_SIZE		(dev_priv->info->mmio_offset + 0x71290)
> > +#define _SPRB_KEYVAL		(dev_priv->info->mmio_offset + 0x71294)
> > +#define _SPRB_KEYMSK		(dev_priv->info->mmio_offset + 0x71298)
> > +#define _SPRB_SURF		(dev_priv->info->mmio_offset + 0x7129c)
> > +#define _SPRB_KEYMAX		(dev_priv->info->mmio_offset + 0x712a0)
> > +#define _SPRB_TILEOFF		(dev_priv->info->mmio_offset + 0x712a4)
> > +#define _SPRB_OFFSET		(dev_priv->info->mmio_offset + 0x712a4)
> > +#define _SPRB_SURFLIVE		(dev_priv->info->mmio_offset + 0x712ac)
> > +#define _SPRB_SCALE		(dev_priv->info->mmio_offset + 0x71304)
> > +#define _SPRB_GAMC		(dev_priv->info->mmio_offset + 0x71400)
> >  
> >  #define SPRCTL(pipe) _PIPE(pipe, _SPRA_CTL, _SPRB_CTL)
> >  #define SPRLINOFF(pipe) _PIPE(pipe, _SPRA_LINOFF, _SPRB_LINOFF)
> > @@ -3227,7 +3221,7 @@
> >  #define SPRSURFLIVE(pipe) _PIPE(pipe, _SPRA_SURFLIVE, _SPRB_SURFLIVE)
> >  
> >  /* VBIOS regs */
> > -#define VGACNTRL		0x71400
> > +#define VGACNTRL		(dev_priv->info->mmio_offset + 0x71400)
> >  # define VGA_DISP_DISABLE			(1 << 31)
> >  # define VGA_2X_MODE				(1 << 30)
> >  # define VGA_PIPE_B_SELECT			(1 << 29)
> > @@ -3272,41 +3266,41 @@
> >  #define  FDI_PLL_FREQ_DISABLE_COUNT_LIMIT_MASK  0xff
> >  
> >  
> > -#define _PIPEA_DATA_M1           0x60030
> > +#define _PIPEA_DATA_M1           (dev_priv->info->mmio_offset + 0x60030)
> >  #define  TU_SIZE(x)             (((x)-1) << 25) /* default size 64 */
> >  #define  TU_SIZE_MASK           0x7e000000
> >  #define  PIPE_DATA_M1_OFFSET    0
> > -#define _PIPEA_DATA_N1           0x60034
> > +#define _PIPEA_DATA_N1           (dev_priv->info->mmio_offset + 0x60034)
> >  #define  PIPE_DATA_N1_OFFSET    0
> >  
> > -#define _PIPEA_DATA_M2           0x60038
> > +#define _PIPEA_DATA_M2           (dev_priv->info->mmio_offset + 0x60038)
> >  #define  PIPE_DATA_M2_OFFSET    0
> > -#define _PIPEA_DATA_N2           0x6003c
> > +#define _PIPEA_DATA_N2           (dev_priv->info->mmio_offset + 0x6003c)
> >  #define  PIPE_DATA_N2_OFFSET    0
> >  
> > -#define _PIPEA_LINK_M1           0x60040
> > +#define _PIPEA_LINK_M1           (dev_priv->info->mmio_offset + 0x60040)
> >  #define  PIPE_LINK_M1_OFFSET    0
> > -#define _PIPEA_LINK_N1           0x60044
> > +#define _PIPEA_LINK_N1           (dev_priv->info->mmio_offset + 0x60044)
> >  #define  PIPE_LINK_N1_OFFSET    0
> >  
> > -#define _PIPEA_LINK_M2           0x60048
> > +#define _PIPEA_LINK_M2           (dev_priv->info->mmio_offset + 0x60048)
> >  #define  PIPE_LINK_M2_OFFSET    0
> > -#define _PIPEA_LINK_N2           0x6004c
> > +#define _PIPEA_LINK_N2           (dev_priv->info->mmio_offset + 0x6004c)
> >  #define  PIPE_LINK_N2_OFFSET    0
> >  
> >  /* PIPEB timing regs are same start from 0x61000 */
> >  
> > -#define _PIPEB_DATA_M1           0x61030
> > -#define _PIPEB_DATA_N1           0x61034
> > +#define _PIPEB_DATA_M1           (dev_priv->info->mmio_offset + 0x61030)
> > +#define _PIPEB_DATA_N1           (dev_priv->info->mmio_offset + 0x61034)
> >  
> > -#define _PIPEB_DATA_M2           0x61038
> > -#define _PIPEB_DATA_N2           0x6103c
> > +#define _PIPEB_DATA_M2           (dev_priv->info->mmio_offset + 0x61038)
> > +#define _PIPEB_DATA_N2           (dev_priv->info->mmio_offset + 0x6103c)
> >  
> > -#define _PIPEB_LINK_M1           0x61040
> > -#define _PIPEB_LINK_N1           0x61044
> > +#define _PIPEB_LINK_M1           (dev_priv->info->mmio_offset + 0x61040)
> > +#define _PIPEB_LINK_N1           (dev_priv->info->mmio_offset + 0x61044)
> >  
> > -#define _PIPEB_LINK_M2           0x61048
> > -#define _PIPEB_LINK_N2           0x6104c
> > +#define _PIPEB_LINK_M2           (dev_priv->info->mmio_offset + 0x61048)
> > +#define _PIPEB_LINK_N2           (dev_priv->info->mmio_offset + 0x6104c)
> >  
> >  #define PIPE_DATA_M1(tran) _TRANSCODER(tran, _PIPEA_DATA_M1, _PIPEB_DATA_M1)
> >  #define PIPE_DATA_N1(tran) _TRANSCODER(tran, _PIPEA_DATA_N1, _PIPEB_DATA_N1)
> > @@ -3319,8 +3313,8 @@
> >  
> >  /* CPU panel fitter */
> >  /* IVB+ has 3 fitters, 0 is 7x5 capable, the other two only 3x3 */
> > -#define _PFA_CTL_1               0x68080
> > -#define _PFB_CTL_1               0x68880
> > +#define _PFA_CTL_1               (dev_priv->info->mmio_offset + 0x68080)
> > +#define _PFB_CTL_1               (dev_priv->info->mmio_offset + 0x68880)
> >  #define  PF_ENABLE              (1<<31)
> >  #define  PF_PIPE_SEL_MASK_IVB	(3<<29)
> >  #define  PF_PIPE_SEL_IVB(pipe)	((pipe)<<29)
> > @@ -3329,14 +3323,14 @@
> >  #define  PF_FILTER_MED_3x3	(1<<23)
> >  #define  PF_FILTER_EDGE_ENHANCE	(2<<23)
> >  #define  PF_FILTER_EDGE_SOFTEN	(3<<23)
> > -#define _PFA_WIN_SZ		0x68074
> > -#define _PFB_WIN_SZ		0x68874
> > -#define _PFA_WIN_POS		0x68070
> > -#define _PFB_WIN_POS		0x68870
> > -#define _PFA_VSCALE		0x68084
> > -#define _PFB_VSCALE		0x68884
> > -#define _PFA_HSCALE		0x68090
> > -#define _PFB_HSCALE		0x68890
> > +#define _PFA_WIN_SZ		(dev_priv->info->mmio_offset + 0x68074)
> > +#define _PFB_WIN_SZ		(dev_priv->info->mmio_offset + 0x68874)
> > +#define _PFA_WIN_POS		(dev_priv->info->mmio_offset + 0x68070)
> > +#define _PFB_WIN_POS		(dev_priv->info->mmio_offset + 0x68870)
> > +#define _PFA_VSCALE		(dev_priv->info->mmio_offset + 0x68084)
> > +#define _PFB_VSCALE		(dev_priv->info->mmio_offset + 0x68884)
> > +#define _PFA_HSCALE		(dev_priv->info->mmio_offset + 0x68090)
> > +#define _PFB_HSCALE		(dev_priv->info->mmio_offset + 0x68890)
> 
> The new per-pipe panel fitters are only available on pch-split platforms,
> so not required to convert I think.

Right. I'll drop these.

> >  #define PF_CTL(pipe)		_PIPE(pipe, _PFA_CTL_1, _PFB_CTL_1)
> >  #define PF_WIN_SZ(pipe)		_PIPE(pipe, _PFA_WIN_SZ, _PFB_WIN_SZ)
> > @@ -3703,13 +3697,13 @@
> >  #define TVIDEO_DIP_DATA(pipe) _PIPE(pipe, _VIDEO_DIP_DATA_A, _VIDEO_DIP_DATA_B)
> >  #define TVIDEO_DIP_GCP(pipe) _PIPE(pipe, _VIDEO_DIP_GCP_A, _VIDEO_DIP_GCP_B)
> >  
> > -#define VLV_VIDEO_DIP_CTL_A		0x60200
> > -#define VLV_VIDEO_DIP_DATA_A		0x60208
> > -#define VLV_VIDEO_DIP_GDCP_PAYLOAD_A	0x60210
> > +#define VLV_VIDEO_DIP_CTL_A		(dev_priv->info->mmio_offset + 0x60200)
> > +#define VLV_VIDEO_DIP_DATA_A		(dev_priv->info->mmio_offset + 0x60208)
> > +#define VLV_VIDEO_DIP_GDCP_PAYLOAD_A	(dev_priv->info->mmio_offset + 0x60210)
> >  
> > -#define VLV_VIDEO_DIP_CTL_B		0x61170
> > -#define VLV_VIDEO_DIP_DATA_B		0x61174
> > -#define VLV_VIDEO_DIP_GDCP_PAYLOAD_B	0x61178
> > +#define VLV_VIDEO_DIP_CTL_B		(dev_priv->info->mmio_offset + 0x61170)
> > +#define VLV_VIDEO_DIP_DATA_B		(dev_priv->info->mmio_offset + 0x61174)
> > +#define VLV_VIDEO_DIP_GDCP_PAYLOAD_B	(dev_priv->info->mmio_offset + 0x61178)
> 
> I'm starting to be on the fence wrt. going with a fixed offset for these
> ...
> 
> >  
> >  #define VLV_TVIDEO_DIP_CTL(pipe) \
> >  	 _PIPE(pipe, VLV_VIDEO_DIP_CTL_A, VLV_VIDEO_DIP_CTL_B)
> > @@ -3999,17 +3993,17 @@
> >  #define  LVDS_DETECTED	(1 << 1)
> >  
> >  /* vlv has 2 sets of panel control regs. */
> > -#define PIPEA_PP_STATUS         0x61200
> > -#define PIPEA_PP_CONTROL        0x61204
> > -#define PIPEA_PP_ON_DELAYS      0x61208
> > -#define PIPEA_PP_OFF_DELAYS     0x6120c
> > -#define PIPEA_PP_DIVISOR        0x61210
> > +#define PIPEA_PP_STATUS         (dev_priv->info->mmio_offset + 0x61200)
> > +#define PIPEA_PP_CONTROL        (dev_priv->info->mmio_offset + 0x61204)
> > +#define PIPEA_PP_ON_DELAYS      (dev_priv->info->mmio_offset + 0x61208)
> > +#define PIPEA_PP_OFF_DELAYS     (dev_priv->info->mmio_offset + 0x6120c)
> > +#define PIPEA_PP_DIVISOR        (dev_priv->info->mmio_offset + 0x61210)
> >  
> > -#define PIPEB_PP_STATUS         0x61300
> > -#define PIPEB_PP_CONTROL        0x61304
> > -#define PIPEB_PP_ON_DELAYS      0x61308
> > -#define PIPEB_PP_OFF_DELAYS     0x6130c
> > -#define PIPEB_PP_DIVISOR        0x61310
> > +#define PIPEB_PP_STATUS         (dev_priv->info->mmio_offset + 0x61300)
> > +#define PIPEB_PP_CONTROL        (dev_priv->info->mmio_offset + 0x61304)
> > +#define PIPEB_PP_ON_DELAYS      (dev_priv->info->mmio_offset + 0x61308)
> > +#define PIPEB_PP_OFF_DELAYS     (dev_priv->info->mmio_offset + 0x6130c)
> > +#define PIPEB_PP_DIVISOR        (dev_priv->info->mmio_offset + 0x61310)
> 
> Blargh, another set of PP registers ... I'm confused.

The per pipe PP regs are for VLV in fact. Not sure they're used anywhere
else. I suppose not (based  on the comment). I can drop the non-pipe PP
regs from the conversion.

Although in fact PIPEA_PP == PP, so we could remove one of those sets
completely. Not sure if it would cause confusion in the code using them.
For now I'll leave the non-pipe PP regs be since this is going to need
more work for VLV anyway.

> >  #define PCH_PP_STATUS		0xc7200
> >  #define PCH_PP_CONTROL		0xc7204
> > @@ -4301,17 +4295,17 @@
> >  #define GEN7_ROW_CHICKEN2_GT2		0xf4f4
> >  #define   DOP_CLOCK_GATING_DISABLE	(1<<0)
> >  
> > -#define G4X_AUD_VID_DID			0x62020
> > +#define G4X_AUD_VID_DID			(dev_priv->info->mmio_offset + 0x62020)
> >  #define INTEL_AUDIO_DEVCL		0x808629FB
> >  #define INTEL_AUDIO_DEVBLC		0x80862801
> >  #define INTEL_AUDIO_DEVCTG		0x80862802
> >  
> > -#define G4X_AUD_CNTL_ST			0x620B4
> > +#define G4X_AUD_CNTL_ST			(dev_priv->info->mmio_offset + 0x620B4)
> >  #define G4X_ELDV_DEVCL_DEVBLC		(1 << 13)
> >  #define G4X_ELDV_DEVCTG			(1 << 14)
> >  #define G4X_ELD_ADDR			(0xf << 5)
> >  #define G4X_ELD_ACK			(1 << 4)
> > -#define G4X_HDMIW_HDMIEDID		0x6210C
> > +#define G4X_HDMIW_HDMIEDID		(dev_priv->info->mmio_offset + 0x6210C)
> >  
> >  #define IBX_HDMIW_HDMIEDID_A		0xE2050
> >  #define IBX_HDMIW_HDMIEDID_B		0xE2150
> 
> I'm not too sure about those here, but it looks like g4x_write_eld is
> getting reused on vlv. A quick check with docs whether the registers
> really match would be good I think.

While this change is basically correct, I think this stuff needs
more work for VLV. I'll drop these for now.

> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 71a5eba..21bd64f 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -769,8 +769,6 @@ void intel_crt_init(struct drm_device *dev)
> >  
> >  	if (HAS_PCH_SPLIT(dev))
> >  		crt->adpa_reg = PCH_ADPA;
> > -	else if (IS_VALLEYVIEW(dev))
> > -		crt->adpa_reg = VLV_ADPA;
> >  	else
> >  		crt->adpa_reg = ADPA;
> 
> Like I've said I think it's better to keep this, since different ways to
> switch register offsets are confusing imo ...
> 
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index f05364a..d696855 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2295,19 +2295,14 @@ g4x_dp_detect(struct intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t bit;
> >  
> > -	switch (intel_dp->output_reg) {
> > -	case DP_B:
> > +	if (intel_dp->output_reg == DP_B)
> >  		bit = DPB_HOTPLUG_LIVE_STATUS;
> > -		break;
> > -	case DP_C:
> > +	else if (intel_dp->output_reg == DP_C)
> >  		bit = DPC_HOTPLUG_LIVE_STATUS;
> > -		break;
> > -	case DP_D:
> > +	else if (intel_dp->output_reg == DP_D)
> >  		bit = DPD_HOTPLUG_LIVE_STATUS;
> > -		break;
> > -	default:
> > +	else
> 
> This ladder here should instead switch on intel_dig_port->port.

Right.

> >  		return connector_status_unknown;
> > -	}
> >  
> >  	if ((I915_READ(PORT_HOTPLUG_STAT) & bit) == 0)
> >  		return connector_status_disconnected;
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index d53b731..d5aaf3e 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -399,14 +399,11 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
> >  		return;
> >  	}
> >  
> > -	switch (intel_hdmi->sdvox_reg) {
> > -	case SDVOB:
> > +	if (intel_hdmi->sdvox_reg == SDVOB)
> >  		port = VIDEO_DIP_PORT_B;
> > -		break;
> > -	case SDVOC:
> > +	else if (intel_hdmi->sdvox_reg == SDVOC)
> >  		port = VIDEO_DIP_PORT_C;
> > -		break;
> > -	default:
> > +	else {
> >  		BUG();
> >  		return;
> >  	}
> > @@ -455,17 +452,13 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> >  		return;
> >  	}
> >  
> > -	switch (intel_hdmi->sdvox_reg) {
> > -	case HDMIB:
> > +	if (intel_hdmi->sdvox_reg == HDMIB)
> >  		port = VIDEO_DIP_PORT_B;
> > -		break;
> > -	case HDMIC:
> > +	else if (intel_hdmi->sdvox_reg == HDMIC)
> >  		port = VIDEO_DIP_PORT_C;
> > -		break;
> > -	case HDMID:
> > +	else if (intel_hdmi->sdvox_reg == HDMID)
> >  		port = VIDEO_DIP_PORT_D;
> > -		break;
> > -	default:
> > +	else {
> >  		BUG();
> >  		return;
> >  	}
> > @@ -797,17 +790,12 @@ static bool g4x_hdmi_connected(struct intel_hdmi *intel_hdmi)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint32_t bit;
> >  
> > -	switch (intel_hdmi->sdvox_reg) {
> > -	case SDVOB:
> > +	if (intel_hdmi->sdvox_reg == SDVOB)
> >  		bit = HDMIB_HOTPLUG_LIVE_STATUS;
> > -		break;
> > -	case SDVOC:
> > +	else if (intel_hdmi->sdvox_reg == SDVOC)
> >  		bit = HDMIC_HOTPLUG_LIVE_STATUS;
> > -		break;
> > -	default:
> > +	else
> >  		bit = 0;
> > -		break;
> > -	}
> >  
> >  	return I915_READ(PORT_HOTPLUG_STAT) & bit;
> >  }
> 
> Same for the HDMI ones ...

Yes

> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index 7f09041..599a1b3 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -516,7 +516,7 @@ int intel_setup_gmbus(struct drm_device *dev)
> >  	if (HAS_PCH_SPLIT(dev))
> >  		dev_priv->gpio_mmio_base = PCH_GPIOA - GPIOA;
> >  	else
> > -		dev_priv->gpio_mmio_base = 0;
> > +		dev_priv->gpio_mmio_base = dev_priv->info->mmio_offset;
> 
> Well, that works, too ;-) But I'd vote for an explicit IS_VLV case
> here, since that stuff moved around with the PCH split already. And having
> different magic ways to select the register offset doesn't look too good
> imo.

If you want uniformity, we should just kill all these special offset
handling thingys and convert everything over to the intel_device_info
offset (even PCH vs. non-PCH if possible) ;)

> >  	mutex_init(&dev_priv->gmbus_mutex);
> >  	init_waitqueue_head(&dev_priv->gmbus_wait_queue);
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index f01063a..8504d00 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -1181,14 +1181,11 @@ static void intel_sdvo_mode_set(struct drm_encoder *encoder,
> >  			sdvox |= SDVO_BORDER_ENABLE;
> >  	} else {
> >  		sdvox = I915_READ(intel_sdvo->sdvo_reg);
> > -		switch (intel_sdvo->sdvo_reg) {
> > -		case SDVOB:
> > +
> > +		if (intel_sdvo->sdvo_reg == SDVOB)
> >  			sdvox &= SDVOB_PRESERVE_MASK;
> > -			break;
> > -		case SDVOC:
> > +		else if (intel_sdvo->sdvo_reg == SDVOC)
> >  			sdvox &= SDVOC_PRESERVE_MASK;
> > -			break;
> > -		}
> >  		sdvox |= (9 << 19) | SDVO_BORDER_ENABLE;
> 
> I truly hope SDVO is gone on vlv. And at least intel_setup_outputs seems
> to agree.

Yeah should be AFAIK. And the register spec seems to agree
as it states that only SDVO_ENCODING_TMDS is valid.

Although intel_setup_outputs() seems to disagree with that
assesment. Hopefully that's just due to copy-paste coding.

> Monster patch bikeshedded, time for a beeer!
> 
> Cheers, Daniel
> 
> >  	}
> >  
> > -- 
> > 1.7.12.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrj?l?
Intel OTC


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux