On Wed, 30 Oct 2013, Antti Koskipaa <antti.koskipaa@xxxxxxxxxxxxxxx> wrote:
> Upcoming hardware will not have the various display pipe register
> ranges evenly spaced in memory. Change register address calculations
> into array lookups.
>
> Tested on SandyBridge.
>
> I left the UMS cruft untouched.
>
> Signed-off-by: Antti Koskipaa <antti.koskipaa@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/dvo_ns2501.c | 6 ++--
> drivers/gpu/drm/i915/i915_dma.c | 16 +++++++++++
> drivers/gpu/drm/i915/i915_drv.h | 10 ++++++-
> drivers/gpu/drm/i915/i915_reg.h | 59 +++++++++++++++++++++++++++------------
> 4 files changed, 69 insertions(+), 22 deletions(-)
[snip]
> +/* Pipe timing regs */
> +#define HTOTAL(trans) (dev_priv->trans_offsets[trans])
> +#define HBLANK(trans) (dev_priv->trans_offsets[trans] + 0x04)
> +#define HSYNC(trans) (dev_priv->trans_offsets[trans] + 0x08)
> +#define VTOTAL(trans) (dev_priv->trans_offsets[trans] + 0x0c)
> +#define VBLANK(trans) (dev_priv->trans_offsets[trans] + 0x10)
> +#define VSYNC(trans) (dev_priv->trans_offsets[trans] + 0x14)
> +#define BCLRPAT(trans) (dev_priv->trans_offsets[trans] + 0x20)
> +#define VSYNCSHIFT(trans) (dev_priv->trans_offsets[trans] + 0x28)
> +#define PIPESRC(pipe) (dev_priv->trans_offsets[pipe] + 0x1c)
This is the part that gives me the creeps about this approach, in many
different ways. First, I don't know if we have guarantees that the
offsets between registers remain the same; maybe they do, maybe they
don't. Second, I find myself searching for the register addresses in
this file quite often. With this the "reverse lookup" becomes
hard. Third, an unsubstanciated claim, it just *feels* like too many
levels of indirection to be comfortable.
Here's an alternative approach. How about we change the defines for
_PIPE(), _TRANSCODER(), etc. to require the the register addresses for
*every* pipe, transcoder, etc. as parameters. It may be a bit tedious to
change all uses of the macros, but at least it's straightforward, with,
I think, fewer chances for bugs.
Here's an idea how to do it neatly:
#define _OFFSET(index, ...) (((int[]){ __VA_ARGS__ })[index])
#define _PIPE(pipe, a, b, c) _OFFSET(pipe, a, b, c)
#define _TRANSCODER(trans, a, b, c, edp) _OFFSET(trans, a, b, c, edp)
Note that you'd still need to change TRANSCODER_EDP enum value to be in
sequence (but then it's 0xF just to work for the current macros).
I did not look at the assembly produced by that vs. the table lookup.
BR,
Jani.
PS. And don't just go ahead and do what I suggested; more bikeshedding
might be required! ;)
> +
> +
> /* Pipe A timing regs */
> #define _HTOTAL_A (dev_priv->info->display_mmio_offset + 0x60000)
> #define _HBLANK_A (dev_priv->info->display_mmio_offset + 0x60004)
> @@ -1941,15 +1969,6 @@
> #define _BCLRPAT_B (dev_priv->info->display_mmio_offset + 0x61020)
> #define _VSYNCSHIFT_B (dev_priv->info->display_mmio_offset + 0x61028)
>
> -#define HTOTAL(trans) _TRANSCODER(trans, _HTOTAL_A, _HTOTAL_B)
> -#define HBLANK(trans) _TRANSCODER(trans, _HBLANK_A, _HBLANK_B)
> -#define HSYNC(trans) _TRANSCODER(trans, _HSYNC_A, _HSYNC_B)
> -#define VTOTAL(trans) _TRANSCODER(trans, _VTOTAL_A, _VTOTAL_B)
> -#define VBLANK(trans) _TRANSCODER(trans, _VBLANK_A, _VBLANK_B)
> -#define VSYNC(trans) _TRANSCODER(trans, _VSYNC_A, _VSYNC_B)
> -#define BCLRPAT(pipe) _PIPE(pipe, _BCLRPAT_A, _BCLRPAT_B)
> -#define VSYNCSHIFT(trans) _TRANSCODER(trans, _VSYNCSHIFT_A, _VSYNCSHIFT_B)
> -
> /* HSW eDP PSR registers */
> #define EDP_PSR_BASE(dev) 0x64800
> #define EDP_PSR_CTL(dev) (EDP_PSR_BASE(dev) + 0)
> @@ -3211,12 +3230,16 @@
> #define PIPE_VBLANK_INTERRUPT_STATUS (1UL<<1)
> #define PIPE_OVERLAY_UPDATED_STATUS (1UL<<0)
>
> -#define PIPESRC(pipe) _PIPE(pipe, _PIPEASRC, _PIPEBSRC)
> -#define PIPECONF(tran) _TRANSCODER(tran, _PIPEACONF, _PIPEBCONF)
> -#define PIPEDSL(pipe) _PIPE(pipe, _PIPEADSL, _PIPEBDSL)
> -#define PIPEFRAME(pipe) _PIPE(pipe, _PIPEAFRAMEHIGH, _PIPEBFRAMEHIGH)
> -#define PIPEFRAMEPIXEL(pipe) _PIPE(pipe, _PIPEAFRAMEPIXEL, _PIPEBFRAMEPIXEL)
> -#define PIPESTAT(pipe) _PIPE(pipe, _PIPEASTAT, _PIPEBSTAT)
> +#define PIPE_A_OFFSET 0x70000
> +#define PIPE_B_OFFSET 0x71000
> +#define PIPE_EDP_OFFSET 0x7f000
> +
> +#define PIPEDSL(pipe) (dev_priv->pipe_offsets[pipe])
> +#define PIPECONF(tran) ((int)(tran) == TRANSCODER_EDP ? PIPE_EDP_OFFSET : \
> + dev_priv->pipe_offsets[tran] + 0x08)
> +#define PIPESTAT(pipe) (dev_priv->pipe_offsets[pipe] + 0x24)
> +#define PIPEFRAME(pipe) (dev_priv->pipe_offsets[pipe] + 0x40)
> +#define PIPEFRAMEPIXEL(pipe) (dev_priv->pipe_offsets[pipe] + 0x44)
>
> #define VLV_DPFLIPSTAT (VLV_DISPLAY_BASE + 0x70028)
> #define PIPEB_LINE_COMPARE_INT_EN (1<<29)
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
