Re: [PATCH v3] drm/i915 : Added Programming of the MOCS

From: "chris@chris-wilson.co.uk" <chris@chris-wilson.co.uk>
To: "Antoine, Peter" <peter.antoine@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v3] drm/i915 : Added Programming of the MOCS
Date: Wed, 17 Jun 2015 16:32:18 +0100	[thread overview]
Message-ID: <20150617153218.GK24012@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1434553347.15415.21.camel@peterant-linux>

On Wed, Jun 17, 2015 at 03:02:31PM +0000, Antoine, Peter wrote:
> On Wed, 2015-06-10 at 11:37 +0100, Chris Wilson wrote:
> > > +/* Defines for the tables (XXX_MOCS_0 - XXX_MOCS_63) */
> > > +#define	MOCS_CACHEABILITY(value)	((value & 0x03) << 0)
> > 
> > value & mask? These macros should only be feed enums so the masking of
> > the input is superfluous and indicative of a programming bug.
> Superfluous yes, but it lets the coder know the layout of the field, but
> may hide a programming bug but does not indicate one (other coding
> standards (for safe systems) that I have used require this as it reduces
> the impact of coding errors).

I'm wary. Maybe not so much for the MOCS table, but it means that the
value being programmed to hw does not match what the programmer
intended. It's a bug either way, the hardware is being programmed to an
invalid value - and that may be catastrophic to use either the original
value of the transformed value.

> I have removed them.

I kind of liked it. With a bit of work we could make it catch
programming bugs at compile time. I think we have idly discussed such in
the past, something like a

#define SET_FIELD(x, shift, width) ({
  if (__builtin_constant_p(x)) {
  	BUILD_BUG_ON(x & -(1<<width));
  }
  x << shift;
 })

#define MOCS_CACHEABILITY(value) SET_FIELD(value, 0, 2)

may do the trick.
-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