Re: [RFC] drm/i915: Clean up display pipe register accesses

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 30 Oct 2013, Antti Koskipaa <antti.koskipaa@linux.intel.com> 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@linux.intel.com>
> ---
>  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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center