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

[Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>]

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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxxxx>
> -Chris

Thanks a lot,


-Lionel

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