[Intel-gfx] [PATCH] drm/i915: Assert that we don't overflow frontbuffer tracking bits

[Ville Syrjälä]

On Wed, Jan 24, 2018 at 06:50:45PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2018-01-24 18:36:42)
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > 
> > Add some compile time assrts to the frontbuffer tracking to make sure
> > that we have enough bits per pipe to cover all the planes, and that we
> > have enough total bits to cover all the planes across all pipes.
> > 
> > We'll ignore any potential clash between the overlay bit and the
> > plane bits because that will allow us to keep using a total of 32
> > bits for the foreseeable future.
> > 
> > While at it change the macros to use BIT() and GENMASK(). The latter
> > gets rid of the hardcoded 0xff and thus means we can change the
> > number of bits per pipe by just changing
> > INTEL_FRONTBUFFER_BITS_PER_PIPE.
> > 
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 454d8f937fae..98f346c7b0b3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2411,12 +2411,16 @@ enum hdmi_force_audio {
> >   * We have one bit per pipe and per scanout plane type.
> >   */
> >  #define INTEL_FRONTBUFFER_BITS_PER_PIPE 8
> > -#define INTEL_FRONTBUFFER(pipe, plane_id) \
> > -       (1 << ((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > +#define INTEL_FRONTBUFFER(pipe, plane_id) ({ \
> > +       BUILD_BUG_ON(INTEL_FRONTBUFFER_BITS_PER_PIPE * I915_MAX_PIPES > 32); \
> > +       BUILD_BUG_ON(I915_MAX_PLANES > INTEL_FRONTBUFFER_BITS_PER_PIPE); \
> > +       BIT((plane_id) + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)); \
> > +})
> 
> Next endeavour will be to convert to unsigned long just so we can use
> BITS_PER_LONG here ; (These masks are now defined as ul, seems a natural
> progression to use unsigned long throughout.

longs always worry me because they can lead to funky bugs on 32bit
systems. Also we still depend on manual verification to make the
same type is used consistently.

> Alternatively typedef
> intel_frontbuffer_mask_t and 8*sizeof().)

typedef might be decent. Still no help from the compiler though.
I guess the only way to get such help would be to start using
macros/functions to manipulate bitmasks.

> 
> >  #define INTEL_FRONTBUFFER_OVERLAY(pipe) \
> > -       (1 << (INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > +       BIT(INTEL_FRONTBUFFER_BITS_PER_PIPE - 1 + INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))
> >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> > -       (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> > +       GENMASK(INTEL_FRONTBUFFER_BITS_PER_PIPE * ((pipe) + 1) - 1, \
> > +               INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe))
> 
> Inclusive ends, check.
> 
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris

-- 
Ville Syrjälä
Intel OTC