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.syrj...@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. > > 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? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx