Re: [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags

From: Daniel Vetter <daniel@ffwll.ch>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
Date: Wed, 13 Jul 2016 14:35:47 +0200	[thread overview]
Message-ID: <20160713123547.GL23520@phenom.ffwll.local> (raw)
In-Reply-To: <1467299569-10920-1-git-send-email-david.s.gordon@intel.com>

On Thu, Jun 30, 2016 at 04:12:48PM +0100, Dave Gordon wrote:
> Two different sets of flag bits are stored in the 'flags' member of a
> 'struct drm_i915_gem_exec_object2', and they're defined in two different
> source files, increasing the risk of an accidental clash.
> 
> Some flags in this field are supplied by the user; these are defined in
> i915_drm.h, and they start from the LSB and work up.
> 
> Other flags are defined in i915_gem_execbuffer, for internal use within
> that file only; they start from the MSB and work down.
> 
> So here we add a compile-time check that the two sets of flags do not
> overlap, which would cause all sorts of confusion.
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++++++++----
>  include/uapi/drm/i915_drm.h                | 11 ++++++-----
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7941f1f..608fdc4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -34,10 +34,11 @@
>  #include <linux/dma_remapping.h>
>  #include <linux/uaccess.h>
>  
> -#define  __EXEC_OBJECT_HAS_PIN (1<<31)
> -#define  __EXEC_OBJECT_HAS_FENCE (1<<30)
> -#define  __EXEC_OBJECT_NEEDS_MAP (1<<29)
> -#define  __EXEC_OBJECT_NEEDS_BIAS (1<<28)
> +#define  __EXEC_OBJECT_HAS_PIN		(1<<31)
> +#define  __EXEC_OBJECT_HAS_FENCE	(1<<30)
> +#define  __EXEC_OBJECT_NEEDS_MAP	(1<<29)
> +#define  __EXEC_OBJECT_NEEDS_BIAS	(1<<28)
> +#define  __EXEC_OBJECT_INTERNAL_FLAGS (0xf<<28) /* all of the above */

s/(0xf<<28)/(~(__EXEC_OBJECT_NEEDS_BIAS-1))/ if you feel like, with the
comment retained. Either way:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
>  #define BATCH_OFFSET_BIAS (256*1024)
>  
> @@ -1007,6 +1008,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
>  	unsigned invalid_flags;
>  	int i;
>  
> +	/* INTERNAL flags must not overlap with external ones */
> +	BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS & ~__EXEC_OBJECT_UNKNOWN_FLAGS);
> +
>  	invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
>  	if (USES_FULL_PPGTT(dev))
>  		invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index c17d63d..079d274 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -696,12 +696,13 @@ struct drm_i915_gem_exec_object2 {
>  	 */
>  	__u64 offset;
>  
> -#define EXEC_OBJECT_NEEDS_FENCE (1<<0)
> -#define EXEC_OBJECT_NEEDS_GTT	(1<<1)
> -#define EXEC_OBJECT_WRITE	(1<<2)
> +#define EXEC_OBJECT_NEEDS_FENCE		 (1<<0)
> +#define EXEC_OBJECT_NEEDS_GTT		 (1<<1)
> +#define EXEC_OBJECT_WRITE		 (1<<2)
>  #define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
> -#define EXEC_OBJECT_PINNED	(1<<4)
> -#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_PINNED<<1)
> +#define EXEC_OBJECT_PINNED		 (1<<4)
> +/* All remaining bits are MBZ and RESERVED FOR FUTURE USE */
> +#define __EXEC_OBJECT_UNKNOWN_FLAGS	(-(EXEC_OBJECT_PINNED<<1))
>  	__u64 flags;
>  
>  	__u64 rsvd1;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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