On Wed, Apr 28, 2021 at 9:26 AM Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > >>>> --- > >>>> 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@xxxxxxxx> > >> > >> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
