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

[Tvrtko Ursulin]

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.

Regards,

Tvrtko

>> +	if (num_engines > I915_EXEC_RING_MASK + 1)
>> +		return -EINVAL;
>> +
>>   	set.engines = alloc_engines(num_engines);
>>   	if (!set.engines)
>>   		return -ENOMEM;
>> -- 
>> 2.31.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>