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

[Daniel Vetter]

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".
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch