On Thu, 31 Oct 2013, Antti Koskipää <antti.koskipaa@xxxxxxxxxxxxxxx> wrote:
> On 10/31/13 09:32, Jani Nikula wrote:
>> 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.
>
> 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.
Because the names in the specs are as volatile as everything else from
generation to generation... And when you need to add a new #define,
possibly from a spec with the new cool name for the thing, I like to
check if it's already there, hiding. :)
Also, it's just slower to check which exact register any read or write
ends up using if you have to go through the table. Maybe it's just me
running code in my head in pedantic mode that this affects. *shrug*.
> 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.
All uses of the macros in i915_reg.h.
>> 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]
Yeah, not that great I guess. But that's probably about as good as it
gets with the approach of passing all alternatives to _PIPE() and
_TRANSCODER().
Anyone else have other alternatives?
BR,
Jani.
>
>>
>> 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
