From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [RFC] drm/i915: Clean up display pipe register accesses Date: Thu, 31 Oct 2013 15:12:20 +0200 Message-ID: <20131031131220.GD13047@intel.com> References: <1383141129-3119-1-git-send-email-antti.koskipaa@linux.intel.com> <8738nhevz2.fsf@intel.com> <527236AE.5020604@linux.intel.com> <87zjpppqqd.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id A925FEE803 for ; Thu, 31 Oct 2013 06:28:41 -0700 (PDT) Content-Disposition: inline In-Reply-To: <87zjpppqqd.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Oct 31, 2013 at 02:30:34PM +0200, Jani Nikula wrote: > On Thu, 31 Oct 2013, Antti Koskip=E4=E4 = wrote: > > On 10/31/13 09:32, Jani Nikula wrote: > >> On Wed, 30 Oct 2013, Antti Koskipaa w= rote: > >>> 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 > >>> --- > >>> 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 macro= s. > > > >> 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=E4l=E4 Intel OTC