[Intel-gfx] [RFC] drm/i915: Clean up display pipe register accesses

[Ville Syrjälä]

On Thu, Oct 31, 2013 at 02:30:34PM +0200, Jani Nikula wrote:
> On Thu, 31 Oct 2013, Antti Koskipää <antti.koskipaa at linux.intel.com> wrote:
> > On 10/31/13 09:32, Jani Nikula wrote:
> >> On Wed, 30 Oct 2013, Antti Koskipaa <antti.koskipaa at 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 at 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.
> 
> 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?

I was toying about with a similar idea for planes at some point.

What I had was this:

struct intel_hw_plane {
	uint32_t mmio_base;
};

#define _PLANE_REG(hw, a, b) ((hw)->mmio_base+(b)-(a))

#define DSPCNTR(hw) _PLANE_REG((hw), _DSPACNTR, _DSPACNTR)
#define DSPADDR(hw) _PLANE_REG((hw), _DSPAADDR, _DSPACNTR)
#define DSPSTRIDE(hw) _PLANE_REG((hw), _DSPASTRIDE, _DSPACNTR)
...

struct intel_hw_plane was there just to make the primary vs. sprite
stuff work the same way w/o actually converting primary planes to
drm_planes.

So I kept the full offsets for pipe A registers, but killed off all the
pipe B,C... register defines.

But it still felt somehow crappy so I never posted it, and I couldn't
come up with anything better at the time, so I just left it alone for
the time being.

But yeah, I agree that having the full register offset around would be
nice for looking up the same register even if the hardware folks went and 
totally changed the name, which they seem to like to do every few years.
In fact, now that I think about it, I almost never look up unfamiliar
registers by name in BSpec. Instead I tend to use the address since it's
a more reliable identifier.

So if we were to keep just the pipe/trans/etc. A defines around, maybe
we could have something like this?
#define _PIPE(pipe, a) _PIPE(dev_priv->pipe_offset[(pipe)] - dev_priv->pipe_offset[PIPE_A] + (a))
#define PIPEFOO(pipe) _PIPE(pipe, _PIPEAFOO)

-- 
Ville Syrjälä
Intel OTC