Re: [PATCH] Antigcc bitfield bikeshed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jun 17, 2015 at 05:28:20PM +0300, Jani Nikula wrote:
> On Wed, 17 Jun 2015, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> > Here's an idea I want to float to see if anyone has a better idea.
> 
> I'll give it some thought, but it pains me that things like this make it
> harder for source code cross referencers and even grep to find what you
> you're looking for.

The minimal thing we've tossed around on irc (and we only need minimal
since there's just a few places that need the raw flags field) is to
hardcode the offsets and check them at runtime ...
-Daniel

> 
> BR,
> Jani.
> 
> 
> > Daniel is very keen on using READ_ONCE/WRITE_ONCE/ACCESS_ONCE to
> > document where we play games with memory barriers instead outside of the
> > usual locks. However, that falls down given that we have a lot of
> > bitfields and the macros to ensure no compiler trickery cannot handle a
> > bitfield. One solution is to switch over to using unsigned long flags
> > and manual bit twiddling. Another is to mix and match between
> > readibility and speed, using a bitfield for convenience and flags for
> > when gcc is not helpful. Using flags requires a lot more manual
> > involvement in the bit layout, and obviously duplicates using a
> > bitfield. So to try and keep it maintainable, we only want one
> > definition that is as painless as possible. This is my attempt.
> > -Chris
> >
> > P.S. You should see what happens with i915_vma!
> > http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=b93fd1fbdd7f82a7a045ff7e081907f3ac7ee806
> >
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h        | 140 +++++++++++++++++++--------------
> >  drivers/gpu/drm/i915/i915_gem.c        |   2 +-
> >  drivers/gpu/drm/i915/i915_gem_tiling.c |   3 +-
> >  3 files changed, 85 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 937f8fe385f5..a47ec76591db 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1987,6 +1987,82 @@ struct drm_i915_gem_object_ops {
> >  #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
> >  	(0xf << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
> >  
> > +#define I915_BO_BIT(T, name, prev, width) \
> > +	T name:width;
> > +#define I915_BO_ENUM(T, name, prev, width) \
> > +	I915_BO_FLAG_SHIFT_##name = I915_BO_FLAG_SHIFT_##prev + I915_BO_FLAG_WIDTH_##prev, \
> > +	I915_BO_FLAG_WIDTH_##name = width,
> > +
> > +#define I915_BO_FLAG_SHIFT___first__ 0
> > +#define I915_BO_FLAG_WIDTH___first__ 0
> > +
> > +#define I915_BO_FLAGS(func) \
> > +	/** \
> > +	 * This is set if the object is on the active lists (has pending \
> > +	 * rendering and so a non-zero seqno), and is not set if it i s on \
> > +	 * inactive (ready to be unbound) list. \
> > +	 */ \
> > +	func(unsigned int, active, __first__, I915_NUM_RINGS) \
> > +	func(unsigned int, active_reference, active, 1) \
> > +\
> > +	/** \
> > +	 * This is set if the object has been written to since last bound \
> > +	 * to the GTT \
> > +	 */ \
> > +	func(unsigned int, dirty, active_reference, 1) \
> > +\
> > +	/** \
> > +	 * Fence register bits (if any) for this object.  Will be set \
> > +	 * as needed when mapped into the GTT. \
> > +	 * Protected by dev->struct_mutex. \
> > +	 */ \
> > +	func(signed int, fence_reg, dirty, I915_MAX_NUM_FENCE_BITS) \
> > +\
> > +	/** \
> > +	 * Advice: are the backing pages purgeable? \
> > +	 */ \
> > +	func(unsigned int, madv, fence_reg, 2) \
> > +\
> > +	/** \
> > +	 * Current tiling mode for the object. \
> > +	 */ \
> > +	func(unsigned int, tiling_mode, madv, 2) \
> > +	/** \
> > +	 * Whether the tiling parameters for the currently associated fence \
> > +	 * register have changed. Note that for the purposes of tracking \
> > +	 * tiling changes we also treat the unfenced register, the register \
> > +	 * slot that the object occupies whilst it executes a fenced \
> > +	 * command (such as BLT on gen2/3), as a "fence". \
> > +	 */ \
> > +	func(unsigned int, fence_dirty, tiling_mode, 1) \
> > +\
> > +	/** \
> > +	 * Whether the current gtt mapping needs to be mappable (and isn't just \
> > +	 * mappable by accident). Track pin and fault separate for a more \
> > +	 * accurate mappable working set. \
> > +	 */ \
> > +	func(unsigned int, fault_mappable, fence_dirty, 1) \
> > +\
> > +	/* \
> > +	 * Is the object to be mapped as read-only to the GPU \
> > +	 * Only honoured if hardware has relevant pte bit \
> > +	 */ \
> > +	func(unsigned int, gt_ro, fault_mappable, 1) \
> > +	func(unsigned int, cache_level, gt_ro, 3) \
> > +	func(unsigned int, cache_dirty, cache_level, 1) \
> > +\
> > +	func(unsigned int, has_dma_mapping, cache_dirty, 1) \
> > +\
> > +	func(unsigned int, frontbuffer_bits, has_dma_mapping, INTEL_FRONTBUFFER_BITS) \
> > +	func(unsigned int, vma_ht_bits, frontbuffer_bits, 5)
> > +
> > +#define I915_BO_FLAG_MASK(name) (((I915_BO_FLAG_WIDTH_##name<<1) - 1) << I915_BO_FLAG_SHIFT_##name)
> > +#define I915_BO_FLAG_VALUE(flags, name) (((flags) >> I915_BO_FLAG_SHIFT_##name) & ((I915_BO_FLAG_WIDTH_##name<<1) - 1))
> > +
> > +enum {
> > +	I915_BO_FLAGS(I915_BO_ENUM)
> > +};
> > +
> >  struct drm_i915_gem_object {
> >  	struct drm_gem_object base;
> >  
> > @@ -2004,64 +2080,12 @@ struct drm_i915_gem_object {
> >  	struct list_head batch_pool_link;
> >  	struct list_head tmp_link;
> >  
> > -	/**
> > -	 * This is set if the object is on the active lists (has pending
> > -	 * rendering and so a non-zero seqno), and is not set if it i s on
> > -	 * inactive (ready to be unbound) list.
> > -	 */
> > -	unsigned int active:I915_NUM_RINGS;
> > -	unsigned int active_reference:1;
> > -
> > -	/**
> > -	 * This is set if the object has been written to since last bound
> > -	 * to the GTT
> > -	 */
> > -	unsigned int dirty:1;
> > -
> > -	/**
> > -	 * Fence register bits (if any) for this object.  Will be set
> > -	 * as needed when mapped into the GTT.
> > -	 * Protected by dev->struct_mutex.
> > -	 */
> > -	signed int fence_reg:I915_MAX_NUM_FENCE_BITS;
> > -
> > -	/**
> > -	 * Advice: are the backing pages purgeable?
> > -	 */
> > -	unsigned int madv:2;
> > -
> > -	/**
> > -	 * Current tiling mode for the object.
> > -	 */
> > -	unsigned int tiling_mode:2;
> > -	/**
> > -	 * Whether the tiling parameters for the currently associated fence
> > -	 * register have changed. Note that for the purposes of tracking
> > -	 * tiling changes we also treat the unfenced register, the register
> > -	 * slot that the object occupies whilst it executes a fenced
> > -	 * command (such as BLT on gen2/3), as a "fence".
> > -	 */
> > -	unsigned int fence_dirty:1;
> > -
> > -	/**
> > -	 * Whether the current gtt mapping needs to be mappable (and isn't just
> > -	 * mappable by accident). Track pin and fault separate for a more
> > -	 * accurate mappable working set.
> > -	 */
> > -	unsigned int fault_mappable:1;
> > -
> > -	/*
> > -	 * Is the object to be mapped as read-only to the GPU
> > -	 * Only honoured if hardware has relevant pte bit
> > -	 */
> > -	unsigned long gt_ro:1;
> > -	unsigned int cache_level:3;
> > -	unsigned int cache_dirty:1;
> > -
> > -	unsigned int has_dma_mapping:1;
> > -
> > -	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> > -	unsigned int vma_ht_bits:5;
> > +	union {
> > +		struct {
> > +			I915_BO_FLAGS(I915_BO_BIT);
> > +		};
> > +		unsigned long flags;
> > +	};
> >  
> >  	unsigned int bind_count;
> >  	unsigned int pin_display;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index b2fc997e8f63..36c99757c3d2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4452,7 +4452,7 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> >  	if (&obj->base == NULL)
> >  		return -ENOENT;
> >  
> > -	if (obj->active) {
> > +	if (READ_ONCE(obj->flags) & I915_BO_FLAG_MASK(active)) {
> >  		ret = i915_mutex_lock_interruptible(dev);
> >  		if (ret)
> >  			goto unref;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > index 8a2325c1101e..b7c9e6ea3e34 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> > @@ -454,7 +454,8 @@ i915_gem_get_tiling(struct drm_device *dev, void *data,
> >  	if (&obj->base == NULL)
> >  		return -ENOENT;
> >  
> > -	args->tiling_mode = obj->tiling_mode;
> > +	args->tiling_mode =
> > +		I915_BO_FLAG_VALUE(READ_ONCE(obj->flags), tiling_mode);
> >  	switch (args->tiling_mode) {
> >  	case I915_TILING_X:
> >  		args->swizzle_mode = dev_priv->mm.bit_6_swizzle_x;
> > -- 
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux