[Intel-gfx] [PATCH 09/21] drm/i915/gem: Disallow creating contexts with too many engines

[Jason Ekstrand]

On Wed, Apr 28, 2021 at 9:26 AM Tvrtko Ursulin
<tvrtko.ursulin at linux.intel.com> wrote:
> On 28/04/2021 15:02, Daniel Vetter wrote:
> > On Wed, Apr 28, 2021 at 11:42:31AM +0100, Tvrtko Ursulin wrote:
> >>
> >> On 28/04/2021 11:16, Daniel Vetter wrote:
> >>> On Fri, Apr 23, 2021 at 05:31:19PM -0500, Jason Ekstrand wrote:
> >>>> There's no sense in allowing userspace to create more engines than it
> >>>> can possibly access via execbuf.
> >>>>
> >>>> Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/gem/i915_gem_context.c | 7 +++----
> >>>>    1 file changed, 3 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>>> index 5f8d0faf783aa..ecb3bf5369857 100644
> >>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> >>>> @@ -1640,11 +1640,10 @@ set_engines(struct i915_gem_context *ctx,
> >>>>                    return -EINVAL;
> >>>>            }
> >>>> -  /*
> >>>> -   * Note that I915_EXEC_RING_MASK limits execbuf to only using the
> >>>> -   * first 64 engines defined here.
> >>>> -   */
> >>>>            num_engines = (args->size - sizeof(*user)) / sizeof(*user->engines);
> >>>
> >>> Maybe add a comment like /* RING_MASK has not shift, so can be used
> >>> directly here */ since I had to check that :-)
> >>>
> >>> Same story about igt testcases needed, just to be sure.
> >>>
> >>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> >>
> >> I am not sure about the churn vs benefit ratio here. There are also patches
> >> which extend the engine selection field in execbuf2 over the unused
> >> constants bits (with an explicit flag). So churn upstream and churn in
> >> internal (if interesting) for not much benefit.
> >
> > This isn't churn.
> >
> > This is "lock done uapi properly".

Pretty much.

> IMO it is a "meh" patch. Doesn't fix any problems and will create work
> for other people and man hours spent which no one will ever properly
> account against.
>
> Number of contexts in the engine map should not really be tied to
> execbuf2. As is demonstrated by the incoming work to address more than
> 63 engines, either as an extension to execbuf2 or future execbuf3.

Which userspace driver has requested more than 64 engines in a single context?

Also, for execbuf3, I'd like to get rid of contexts entirely and have
engines be their own userspace-visible object.  If we go this
direction, you can have UINT32_MAX of them.  Problem solved.

--Jason