[Intel-gfx] [RFC][PATCH 43/43] WIP: drm/i915: Type safe register read/write

[Ville Syrjälä]

On Fri, Sep 18, 2015 at 07:12:30PM +0100, Chris Wilson wrote:
> On Fri, Sep 18, 2015 at 08:43:27PM +0300, Ville Syrjälä wrote:
> > On Fri, Sep 18, 2015 at 06:33:38PM +0100, Chris Wilson wrote:
> > > On Fri, Sep 18, 2015 at 08:03:56PM +0300, ville.syrjala at linux.intel.com wrote:
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index 140076d..0589aba 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -25,16 +25,28 @@
> > > >  #ifndef _I915_REG_H_
> > > >  #define _I915_REG_H_
> > > >  
> > > > +struct i915_reg {
> > > > +	uint32_t reg;
> > > > +};
> > > 
> > > Fundamental objection averted. Maybe even typedef it to hide the struct,
> > > ignornace is bliss.  i915_reg_t reg; looks reasonable in this instance.
> > 
> > Yeah typedef would make it less ugly. Maybe i915_mmio_reg_t or something,
> > so we can add other register types?
> 
> Yes. Separating out the display/mmio/gpio registers would be nice, that
> should spawn a few functions that can check we don't add the wrong base
> (if any such remain!). It would mean we end up with a construct like
> union i915_reg {
> 	i915_gt_reg_t gt;
> 	i915_mmio_reg_t mmio;
> 	i915_gpio_reg_t gpio;
> 	i915_display_reg_t display;
> };
> and typecasting when we try to write a register. Or maybe we just have
> more typesafe helpers that call the private vfuncs. It may even be a net
> reduction in code size, since we move all the addition of display
> offsets into a central location. However, it looks like it may just be
> an inconvenient mess.

I don't think I'll go as far as having different classes of mmio regs.

I was thinking more about sideband stuff. We had at least one case on
CHV where we read the register from a totally wrong sideband port.

I was somewhat thinking of VGA registers too since those are a bit
special (either mmio or port io depending on the gen). Though we have so
little interaction with them that I'm not sure it's worth the effort.

> 
> > > The downside with this patch is that we have a lot of places that cares
> > > about the reg.reg,
> > 
> > That is a bit annoying, yes. I think most of those are in the ring code
> > where we emit LRIs. I suppose we could add a special version of ring_emit
> > that takes the struct and thus hides the reg.reg part from the calling
> > code?
> 
> Right, even just adding an inline can be down in a preceding patch and
> so reduce churn here and extends the typesafety argument for LRI as
> well (and SRM).
> 
> I half considered a function to return the raw offset from a struct i915_reg,
> that is only marginally better than adding .reg (but only in the case
> where we end up with a few very similar i915_reg structs). But it
> doesn't actually reduce the churn... unless you add a stub in a previous
> patch such that any location that wants to use the register offset as a
> value is converted first.
>  
> > > and even more where we make up reg on the fly, so it
> > > looks like we are just doing I915_WRITE(_REG(old_u32), x) i.e. more or
> > > less subverting the extra safety, and we would still want something like
> > > 
> > > 	i915_write16(reg) { WARN_ON(reg & 1); }
> > > 	i915_write32(reg) { WARN_ON(reg & 3); }
> > > 
> > > to catch the class of bug where we create an invalid reg address.
> > 
> > I've killed off almost all of the "make up regs on the fly" in the prep
> > work. I think what was left were vgpu and the 2x32 register read ioctl.
> > I don't recall any other significant places off the top of my head.
> 
> Never underestimate the cunning of the lazy programmer. Not that I want
> to add any more instructions to register access. Going forward, I guess
> the rule is to scrutinize _REG() etc very careful and limit them to headers?

Yeah I wanted to undef _REG() at the end of the header so it can't be
abused, but someone had gone and added register defines to other files
too so I left out the undef for now. And I guess I even abused it myself
in one place myself. So there are still some things to clean up.

-- 
Ville Syrjälä
Intel OTC