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
