Re: [RFC][PATCH 43/43] WIP: drm/i915: Type safe register read/write

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [RFC][PATCH 43/43] WIP: drm/i915: Type safe register read/write
Date: Fri, 18 Sep 2015 20:43:27 +0300	[thread overview]
Message-ID: <20150918174327.GU26517@intel.com> (raw)
In-Reply-To: <20150918173338.GT6442@nuc-i3427.alporthouse.com>

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@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?

> For sanity's sake could you disassemble a simple function and include the
> diff in the changelog. It will be reasuring to know that gcc handles the
> struct-in-register just fine.

Oh right. I did actually mean to look at the generated code, but it then
slipped my mind. I'll definitely have to do that before we consider
actually merging this.

> 
> Once upon a time the plan was to use sparse for detecting type
> mismatches, hence enum plane and enum pipe, but I know I don't run make
> C=1 regularly at all, so using gcc itself is a win.
> 
> 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?

> 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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx