Re: [PATCH v6 06/11] drm/i915: introduce a mechanism to extend execbuf2

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx