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

["Antti Koskipää" <antti.koskipaa@linux.intel.com>]

On 10/31/13 09:32, Jani Nikula wrote:
> 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.

Probably they do, since it's usually the same IP block that's just
replicated n times.

> Second, I find myself searching for the register addresses in
> this file quite often. With this the "reverse lookup" becomes
> hard.

Why do you search by address? The registers have names. And because the
UMS stuff had to be left in, the original definitions w/addresses are
still there.

> Third, an unsubstanciated claim, it just *feels* like too many
> levels of indirection to be comfortable.

See below for stack usage of your approach.

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

What do you mean "change all the uses of the macros"? Nobody uses _PIPE
and _TRANSCODER directly. If they did, we'd have to change them *every
time* we added a display pipe, thus negating the whole purpose of this
patch.

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

This helps with your grepping for addresses, but it just hides the array
on the local variable storage area on the stack, replicated in the
machine code as many times as there are functions that call these macros.

> I did not look at the assembly produced by that vs. the table lookup.

I did, for _TRANSCODER() above:

    mov     DWORD PTR [rsp-24], 0x60000
    mov     DWORD PTR [rsp-20], 0x61000
    mov     DWORD PTR [rsp-16], 0x63000
    mov     DWORD PTR [rsp-12], 0x6f000
    mov     eax, DWORD PTR [rsp-24+rdi*4]

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