[Intel-gfx] [PATCH] Antigcc bitfield bikeshed
Jani Nikula
jani.nikula at linux.intel.com
Wed Jun 17 07:28:20 PDT 2015
On Wed, 17 Jun 2015, Chris Wilson <chris at chris-wilson.co.uk> 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.
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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list