Re: [PATCH 1/2] drm/i915: Update plane flip count registers

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 22, 2013 at 11:12:13AM -0700, Ben Widawsky wrote:
> On Thu, Aug 22, 2013 at 08:18:44PM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 21, 2013 at 08:15:52PM -0700, Ben Widawsky wrote:
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 53d0e70..d1079db 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3277,7 +3277,6 @@
> > >  #define   PIPE_PIXEL_SHIFT        0
> > >  /* GM45+ just has to be different */
> > >  #define _PIPEA_FRMCOUNT_GM45	0x70040
> > > -#define _PIPEA_FLIPCOUNT_GM45	0x70044
> > >  #define PIPE_FRMCOUNT_GM45(pipe) _PIPE(pipe, _PIPEA_FRMCOUNT_GM45, _PIPEB_FRMCOUNT_GM45)
> > >  
> > >  /* Cursor A & B regs */
> > > @@ -3361,6 +3360,7 @@
> > >  #define   DISPPLANE_STEREO_POLARITY_SECOND	(1<<18)
> > >  #define   DISPPLANE_TRICKLE_FEED_DISABLE	(1<<14) /* Ironlake */
> > >  #define   DISPPLANE_TILED			(1<<10)
> > > +#define _DSPAFLIPCNT		(dev_priv->info->display_mmio_offset + 0x70044)
> > 
> > Hmm. I don't quite get it. Why rename and move it? Sure it should really
> > be called DSPFLIPCNT since it applies to the primary plane, but BSpec
> > doesn't actually call it that, never has AFAICS.
> 
> The rename and move was just to keep everything in a nice place.
> _PIPEA_FLIPCOUNT_GM45 was never actually used AFAICT. Since it seemed I
> needed to define a PIPEB anyway, I figured I'd try to adhere to the
> other register convention.
> 
> I don't know what naming scheme we typically use to define these
> registers, as I infrequently touch them. What is your recommendation,
> I'll gladly update. I just used what the code around me used (and I
> personally dislike that we call the stuff DSP[AB] anyway).

I guess generally the name has more or less come from the earliest BSpec
that had the register. I probably need to break that rule soon tough as
I want to unify all the sprite registers. We have too many copies which
basically just differ by some constant offset.

> 
> > 
> > Also I was first sceptical about the mmio_offset, since I remembered
> > seeing the pre-CTG two part frame counter registers in VLV specs. But after
> > re-checking, the TOC only has the old regs, while the actual text has only
> > the CTG style regs. So I guess VLV does indeed have the CTG style registers.
> > I don't have a VLV board on me to verify though.
> > 
> 
> As a broader question, is that how we denote whether or not the register
> exists on VLV, by not use dev_priv->info->display_mmio_offset + ? I was
> sort of curious about this since I too was unsure about VLV.

Yes, more or less.

The basic rule has been that register which exists only on VLV gets a
hardcoded +VLV_DISPLAY_BASE, register which exists on both VLV and
non-VLV gets a +display_mmio_offset, and registers that don't exist on
VLV get nothing.

Sometimes we have VLV prefix/suffix in the register name as well if the
register is VLV specific.

There's also some things like the ADPA register where we could have
used display_mmio_offset for VLV, but since we already had a pre-PCH
and PCH versions of it, we also added a specific VLV version. Also the
GMBUS registers already had offset handling through gmbus_mmio_offset,
so we didn't add display_mmio_offset to them.

-- 
Ville Syrjälä
Intel OTC