| Message ID | 20190701113437.4043-7-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Vulkan performance query support | expand |
Quoting Lionel Landwerlin (2019-07-01 12:34:32) > We're planning to use this for a couple of new feature where we need > to provide additional parameters to execbuf. > > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Looks ok, are you convinced by I915_EXEC_EXT? It doesn't roll off the tongue too well for me, but I guess EXT is a bit more ingrained in your cerebral cortex. > --- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 32 ++++++++++++++++++- > include/uapi/drm/i915_drm.h | 25 +++++++++++++-- > 2 files changed, 53 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > index 1c5dfbfad71b..9887fa9e3ac8 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c > @@ -23,6 +23,7 @@ > #include "i915_gem_clflush.h" > #include "i915_gem_context.h" > #include "i915_trace.h" > +#include "i915_user_extensions.h" > #include "intel_drv.h" > > enum { > @@ -271,6 +272,10 @@ struct i915_execbuffer { > */ > int lut_size; > struct hlist_head *buckets; /** ht for relocation handles */ > + > + struct { > + u64 flags; /** Available extensions parameters */ > + } extensions; > }; > > #define exec_entry(EB, VMA) (&(EB)->exec[(VMA)->exec_flags - (EB)->flags]) > @@ -1969,7 +1974,7 @@ static bool i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec) > return false; > > /* Kernel clipping was a DRI1 misfeature */ > - if (!(exec->flags & I915_EXEC_FENCE_ARRAY)) { > + if (!(exec->flags & (I915_EXEC_FENCE_ARRAY | I915_EXEC_EXT))) { > if (exec->num_cliprects || exec->cliprects_ptr) > return false; > } > @@ -2347,6 +2352,27 @@ signal_fence_array(struct i915_execbuffer *eb, > } > } > > +static const i915_user_extension_fn execbuf_extensions[] = { > +}; > + > +static int > +parse_execbuf2_extensions(struct drm_i915_gem_execbuffer2 *args, > + struct i915_execbuffer *eb) > +{ > + eb->extensions.flags = 0; > + > + if (!(args->flags & I915_EXEC_EXT)) > + return 0; > + > + if (args->num_cliprects != 0) > + return -EINVAL; > + > + return i915_user_extensions(u64_to_user_ptr(args->cliprects_ptr), > + execbuf_extensions, > + ARRAY_SIZE(execbuf_extensions), > + eb); > +} > + > static int > i915_gem_do_execbuffer(struct drm_device *dev, > struct drm_file *file, > @@ -2393,6 +2419,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, > if (args->flags & I915_EXEC_IS_PINNED) > eb.batch_flags |= I915_DISPATCH_PINNED; > > + err = parse_execbuf2_extensions(args, &eb); > + if (err) > + return err; > + > if (args->flags & I915_EXEC_FENCE_IN) { > in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2)); > if (!in_fence) > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index e27a8eda9121..efa195d6994e 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -1013,6 +1013,10 @@ struct drm_i915_gem_exec_fence { > __u32 flags; > }; > > +enum drm_i915_gem_execbuffer_ext { > + DRM_I915_GEM_EXECBUFFER_EXT_MAX /* non-ABI */ We have a weird mix of trying to avoid drm_i915_gem and yet it's plastered all over the structs. Sigh. > +}; enums next to uABI make me nervous :) Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 01/07/2019 18:17, Chris Wilson wrote: > Quoting Lionel Landwerlin (2019-07-01 12:34:32) >> We're planning to use this for a couple of new feature where we need >> to provide additional parameters to execbuf. >> >> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Looks ok, are you convinced by I915_EXEC_EXT? It doesn't roll off the > tongue too well for me, but I guess EXT is a bit more ingrained in > your cerebral cortex. I'm open to any suggestion for the name :) > >> --- >> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 32 ++++++++++++++++++- >> include/uapi/drm/i915_drm.h | 25 +++++++++++++-- >> 2 files changed, 53 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> index 1c5dfbfad71b..9887fa9e3ac8 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c >> @@ -23,6 +23,7 @@ >> #include "i915_gem_clflush.h" >> #include "i915_gem_context.h" >> #include "i915_trace.h" >> +#include "i915_user_extensions.h" >> #include "intel_drv.h" >> >> enum { >> @@ -271,6 +272,10 @@ struct i915_execbuffer { >> */ >> int lut_size; >> struct hlist_head *buckets; /** ht for relocation handles */ >> + >> + struct { >> + u64 flags; /** Available extensions parameters */ >> + } extensions; >> }; >> >> #define exec_entry(EB, VMA) (&(EB)->exec[(VMA)->exec_flags - (EB)->flags]) >> @@ -1969,7 +1974,7 @@ static bool i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec) >> return false; >> >> /* Kernel clipping was a DRI1 misfeature */ >> - if (!(exec->flags & I915_EXEC_FENCE_ARRAY)) { >> + if (!(exec->flags & (I915_EXEC_FENCE_ARRAY | I915_EXEC_EXT))) { >> if (exec->num_cliprects || exec->cliprects_ptr) >> return false; >> } >> @@ -2347,6 +2352,27 @@ signal_fence_array(struct i915_execbuffer *eb, >> } >> } >> >> +static const i915_user_extension_fn execbuf_extensions[] = { >> +}; >> + >> +static int >> +parse_execbuf2_extensions(struct drm_i915_gem_execbuffer2 *args, >> + struct i915_execbuffer *eb) >> +{ >> + eb->extensions.flags = 0; >> + >> + if (!(args->flags & I915_EXEC_EXT)) >> + return 0; >> + >> + if (args->num_cliprects != 0) >> + return -EINVAL; >> + >> + return i915_user_extensions(u64_to_user_ptr(args->cliprects_ptr), >> + execbuf_extensions, >> + ARRAY_SIZE(execbuf_extensions), >> + eb); >> +} >> + >> static int >> i915_gem_do_execbuffer(struct drm_device *dev, >> struct drm_file *file, >> @@ -2393,6 +2419,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, >> if (args->flags & I915_EXEC_IS_PINNED) >> eb.batch_flags |= I915_DISPATCH_PINNED; >> >> + err = parse_execbuf2_extensions(args, &eb); >> + if (err) >> + return err; >> + >> if (args->flags & I915_EXEC_FENCE_IN) { >> in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2)); >> if (!in_fence) >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index e27a8eda9121..efa195d6994e 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -1013,6 +1013,10 @@ struct drm_i915_gem_exec_fence { >> __u32 flags; >> }; >> >> +enum drm_i915_gem_execbuffer_ext { >> + DRM_I915_GEM_EXECBUFFER_EXT_MAX /* non-ABI */ > We have a weird mix of trying to avoid drm_i915_gem and yet it's > plastered all over the structs. Sigh. Yeah, I couldn't figure out what is desired. Happy to change it if you have a naming scheme. > >> +}; > enums next to uABI make me nervous :) > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris > Thanks a lot, -Lionel
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 1c5dfbfad71b..9887fa9e3ac8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -23,6 +23,7 @@ #include "i915_gem_clflush.h" #include "i915_gem_context.h" #include "i915_trace.h" +#include "i915_user_extensions.h" #include "intel_drv.h" enum { @@ -271,6 +272,10 @@ struct i915_execbuffer { */ int lut_size; struct hlist_head *buckets; /** ht for relocation handles */ + + struct { + u64 flags; /** Available extensions parameters */ + } extensions; }; #define exec_entry(EB, VMA) (&(EB)->exec[(VMA)->exec_flags - (EB)->flags]) @@ -1969,7 +1974,7 @@ static bool i915_gem_check_execbuffer(struct drm_i915_gem_execbuffer2 *exec) return false; /* Kernel clipping was a DRI1 misfeature */ - if (!(exec->flags & I915_EXEC_FENCE_ARRAY)) { + if (!(exec->flags & (I915_EXEC_FENCE_ARRAY | I915_EXEC_EXT))) { if (exec->num_cliprects || exec->cliprects_ptr) return false; } @@ -2347,6 +2352,27 @@ signal_fence_array(struct i915_execbuffer *eb, } } +static const i915_user_extension_fn execbuf_extensions[] = { +}; + +static int +parse_execbuf2_extensions(struct drm_i915_gem_execbuffer2 *args, + struct i915_execbuffer *eb) +{ + eb->extensions.flags = 0; + + if (!(args->flags & I915_EXEC_EXT)) + return 0; + + if (args->num_cliprects != 0) + return -EINVAL; + + return i915_user_extensions(u64_to_user_ptr(args->cliprects_ptr), + execbuf_extensions, + ARRAY_SIZE(execbuf_extensions), + eb); +} + static int i915_gem_do_execbuffer(struct drm_device *dev, struct drm_file *file, @@ -2393,6 +2419,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, if (args->flags & I915_EXEC_IS_PINNED) eb.batch_flags |= I915_DISPATCH_PINNED; + err = parse_execbuf2_extensions(args, &eb); + if (err) + return err; + if (args->flags & I915_EXEC_FENCE_IN) { in_fence = sync_file_get_fence(lower_32_bits(args->rsvd2)); if (!in_fence) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index e27a8eda9121..efa195d6994e 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -1013,6 +1013,10 @@ struct drm_i915_gem_exec_fence { __u32 flags; }; +enum drm_i915_gem_execbuffer_ext { + DRM_I915_GEM_EXECBUFFER_EXT_MAX /* non-ABI */ +}; + struct drm_i915_gem_execbuffer2 { /** * List of gem_exec_object2 structs @@ -1029,8 +1033,14 @@ struct drm_i915_gem_execbuffer2 { __u32 num_cliprects; /** * This is a struct drm_clip_rect *cliprects if I915_EXEC_FENCE_ARRAY - * is not set. If I915_EXEC_FENCE_ARRAY is set, then this is a - * struct drm_i915_gem_exec_fence *fences. + * & I915_EXEC_EXT are not set. + * + * If I915_EXEC_FENCE_ARRAY is set, then this is a pointer to an array + * of struct drm_i915_gem_exec_fence and num_cliprects is the length + * of the array. + * + * If I915_EXEC_EXT is set, then this is a pointer to a single struct + * drm_i915_gem_base_execbuffer_ext and num_cliprects is 0. */ __u64 cliprects_ptr; #define I915_EXEC_RING_MASK (0x3f) @@ -1148,7 +1158,16 @@ struct drm_i915_gem_execbuffer2 { */ #define I915_EXEC_FENCE_SUBMIT (1 << 20) -#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_FENCE_SUBMIT << 1)) +/* + * Setting I915_EXEC_EXT implies that drm_i915_gem_execbuffer2.cliprects_ptr + * is treated as a pointer to an linked list of i915_user_extension. Each + * i915_user_extension node is the base of a larger structure. The list of + * supported structures are listed in the drm_i915_gem_execbuffer_ext + * enum. + */ +#define I915_EXEC_EXT (1 << 21) + +#define __I915_EXEC_UNKNOWN_FLAGS (-(I915_EXEC_EXT<<1)) #define I915_EXEC_CONTEXT_ID_MASK (0xffffffff) #define i915_execbuffer2_set_context_id(eb2, context) \
We're planning to use this for a couple of new feature where we need to provide additional parameters to execbuf. Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 32 ++++++++++++++++++- include/uapi/drm/i915_drm.h | 25 +++++++++++++-- 2 files changed, 53 insertions(+), 4 deletions(-)