| Message ID | 1383141129-3119-1-git-send-email-antti.koskipaa@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
On Thu, 31 Oct 2013, Antti Koskipää <antti.koskipaa@linux.intel.com> wrote: > 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. 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@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >
On Thu, Oct 31, 2013 at 02:30:34PM +0200, Jani Nikula wrote: > On Thu, 31 Oct 2013, Antti Koskipää <antti.koskipaa@linux.intel.com> wrote: > > 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. > > 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)
diff --git a/drivers/gpu/drm/i915/dvo_ns2501.c b/drivers/gpu/drm/i915/dvo_ns2501.c index c4a255b..ce6a11b 100644 --- a/drivers/gpu/drm/i915/dvo_ns2501.c +++ b/drivers/gpu/drm/i915/dvo_ns2501.c @@ -99,12 +99,12 @@ static void enable_dvo(struct intel_dvo_device *dvo) DRM_DEBUG_KMS("%s: Trying to re-enable the DVO\n", __FUNCTION__); ns->dvoc = I915_READ(DVO_C); - ns->pll_a = I915_READ(_DPLL_A); + ns->pll_a = I915_READ(DPLL(0)); ns->srcdim = I915_READ(DVOC_SRCDIM); ns->fw_blc = I915_READ(FW_BLC); I915_WRITE(DVOC, 0x10004084); - I915_WRITE(_DPLL_A, 0xd0820000); + I915_WRITE(DPLL(0), 0xd0820000); I915_WRITE(DVOC_SRCDIM, 0x400300); // 1024x768 I915_WRITE(FW_BLC, 0x1080304); @@ -125,7 +125,7 @@ static void restore_dvo(struct intel_dvo_device *dvo) struct ns2501_priv *ns = (struct ns2501_priv *)(dvo->dev_priv); I915_WRITE(DVOC, ns->dvoc); - I915_WRITE(_DPLL_A, ns->pll_a); + I915_WRITE(DPLL(0), ns->pll_a); I915_WRITE(DVOC_SRCDIM, ns->srcdim); I915_WRITE(FW_BLC, ns->fw_blc); } diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 0cab2d0..64fbfbb1 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1465,6 +1465,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) struct drm_i915_private *dev_priv; struct intel_device_info *info; int ret = 0, mmio_bar, mmio_size; + u32 mmio; uint32_t aperture_size; info = (struct intel_device_info *) flags; @@ -1484,6 +1485,21 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags) dev_priv->dev = dev; dev_priv->info = info; + /* Set addresses for various pipe registers */ + mmio = dev_priv->info->display_mmio_offset; + dev_priv->pipe_offsets[0] = mmio + PIPE_A_OFFSET; + dev_priv->pipe_offsets[1] = mmio + PIPE_B_OFFSET; + dev_priv->trans_offsets[0] = mmio + TRANSCODER_A_OFFSET; + dev_priv->trans_offsets[1] = mmio + TRANSCODER_B_OFFSET; + dev_priv->trans_offsets[2] = mmio + TRANSCODER_C_OFFSET; + dev_priv->trans_offsets[3] = mmio + TRANSCODER_EDP_OFFSET; + dev_priv->dpll_offsets[0] = mmio + DPLL_A_OFFSET; + dev_priv->dpll_offsets[1] = mmio + DPLL_B_OFFSET; + dev_priv->dpll_md_offsets[0] = mmio + DPLL_A_MD_OFFSET; + dev_priv->dpll_md_offsets[1] = mmio + DPLL_B_MD_OFFSET; + dev_priv->palette_offsets[0] = mmio + PALETTE_A_OFFSET; + dev_priv->palette_offsets[1] = mmio + PALETTE_B_OFFSET; + spin_lock_init(&dev_priv->irq_lock); spin_lock_init(&dev_priv->gpu_error.lock); spin_lock_init(&dev_priv->backlight.lock); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2c1921d..7ea469a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -65,7 +65,8 @@ enum transcoder { TRANSCODER_A = 0, TRANSCODER_B, TRANSCODER_C, - TRANSCODER_EDP = 0xF, + TRANSCODER_EDP, + I915_MAX_TRANSCODERS }; #define transcoder_name(t) ((t) + 'A') @@ -1473,6 +1474,13 @@ typedef struct drm_i915_private { struct i915_package_c8 pc8; + /* Register offsets for the various display pipes */ + int pipe_offsets[I915_MAX_PIPES]; + int trans_offsets[I915_MAX_TRANSCODERS]; + int dpll_offsets[I915_MAX_PIPES]; + int dpll_md_offsets[I915_MAX_PIPES]; + int palette_offsets[I915_MAX_PIPES]; + /* Old dri1 support infrastructure, beware the dragons ya fools entering * here! */ struct i915_dri1_state dri1; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 4d2db59..0afc9fa 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1187,6 +1187,10 @@ * Clock control & power management */ +#define DPLL_A_OFFSET 0x6014 +#define DPLL_B_OFFSET 0x6018 +#define DPLL(pipe) (dev_priv->dpll_offsets[pipe]) + #define VGA0 0x6000 #define VGA1 0x6004 #define VGA_PD 0x6010 @@ -1200,7 +1204,6 @@ #define VGA1_PD_P1_MASK (0x1f << 8) #define _DPLL_A (dev_priv->info->display_mmio_offset + 0x6014) #define _DPLL_B (dev_priv->info->display_mmio_offset + 0x6018) -#define DPLL(pipe) _PIPE(pipe, _DPLL_A, _DPLL_B) #define DPLL_VCO_ENABLE (1 << 31) #define DPLL_SDVO_HIGH_SPEED (1 << 30) #define DPLL_DVO_2X_MODE (1 << 30) @@ -1263,6 +1266,11 @@ #define SDVO_MULTIPLIER_SHIFT_HIRES 4 #define SDVO_MULTIPLIER_SHIFT_VGA 0 #define _DPLL_A_MD (dev_priv->info->display_mmio_offset + 0x601c) /* 965+ only */ + +#define DPLL_A_MD_OFFSET 0x601c /* 965+ only */ +#define DPLL_B_MD_OFFSET 0x6020 /* 965+ only */ +#define DPLL_MD(pipe) (dev_priv->dpll_md_offsets[pipe]) + /* * UDI pixel divider, controlling how many pixels are stuffed into a packet. * @@ -1300,7 +1308,6 @@ #define DPLL_MD_VGA_UDI_MULTIPLIER_MASK 0x0000003f #define DPLL_MD_VGA_UDI_MULTIPLIER_SHIFT 0 #define _DPLL_B_MD (dev_priv->info->display_mmio_offset + 0x6020) /* 965+ only */ -#define DPLL_MD(pipe) _PIPE(pipe, _DPLL_A_MD, _DPLL_B_MD) #define _FPA0 0x06040 #define _FPA1 0x06044 @@ -1457,9 +1464,13 @@ * Palette regs */ +#define PALETTE_A_OFFSET 0xa000 +#define PALETTE_B_OFFSET 0xa800 + #define _PALETTE_A (dev_priv->info->display_mmio_offset + 0xa000) #define _PALETTE_B (dev_priv->info->display_mmio_offset + 0xa800) -#define PALETTE(pipe) _PIPE(pipe, _PALETTE_A, _PALETTE_B) + +#define PALETTE(pipe) (dev_priv->palette_offsets[pipe]) /* MCH MMIO space */ @@ -1919,6 +1930,23 @@ #define PIPE_CRC_RES_RES2_G4X(pipe) \ _PIPE_INC(pipe, _PIPE_CRC_RES_RES2_A_G4X, 0x01000) +#define TRANSCODER_A_OFFSET 0x60000 +#define TRANSCODER_B_OFFSET 0x61000 +#define TRANSCODER_C_OFFSET 0x62000 +#define TRANSCODER_EDP_OFFSET 0x6f000 + +/* 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) + + /* 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)
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(-)