Re: [Intel-gfx] [PATCH 09/14] drm/i915: Clean up vlv/chv sprite plane registers

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

On Fri, Jan 14, 2022 at 04:34:14PM +0000, Souza, Jose wrote:
> On Wed, 2021-12-01 at 17:25 +0200, Ville Syrjala wrote:
> > @@ -7238,28 +7257,36 @@ enum {
> >  #define SPCSCYGOFF(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d900)
> >  #define SPCSCCBOFF(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d904)
> >  #define SPCSCCROFF(plane_id)	_MMIO_CHV_SPCSC(plane_id, 0x6d908)
> > -#define  SPCSC_OOFF(x)		(((x) & 0x7ff) << 16) /* s11 */
> > -#define  SPCSC_IOFF(x)		(((x) & 0x7ff) << 0) /* s11 */
> > +#define  SPCSC_OOFF_MASK	REG_GENMASK(26, 16)
> > +#define  SPCSC_OOFF(x)		REG_FIELD_PREP(SPCSC_OOFF_MASK, (x) & 0x7ff) /* s11 */
> 
> With REG_FIELD_PREP you don't need to do (x) & 0x7ff.

Actually we do. These are two's complemnt so if we pass in a wider
negative value we need to mask off a bunch of the the sign bits.
Yes, REG_FIELD_PREP() does that in the end but it also BUILD_BUG()s
if you pass in a constant value that exceeds the bitmask. And for
these registers we do pass in negative constants.

I'm not entirely sure how much magic we should have in these macros
tbh, vs. just forcing the caller to handle it. If we had readout for
these then the caller would anyway have take care to sign extend the
result. So by that argument maybe these macros shouldn't have anything
like this. Not sure.

I've also occasioanlly pondered about extending that BUILD_BUG_ON()
behaviour to do runtime checks as well, hidden behind a suitable
debug kconfig knob. But haven't actually written the patch for it.

-- 
Ville Syrjälä
Intel