On Fri, Oct 01, 2021 at 09:40:19AM +0100, Tvrtko Ursulin wrote:
>
> + Daniel as reviewer and maybe merge, avoid falling through cracks at least.
>
Ty, working on push rights myself.
> On 22/09/2021 20:43, Matthew Brost wrote:
> > Set number of engines before attempting to create contexts so the
> > function free_engines can clean up properly. Also check return of
> > alloc_engines for NULL.
> >
> > v2:
> > (Tvrtko)
> > - Send as stand alone patch
> > (John Harrison)
> > - Check for alloc_engines returning NULL
> >
> > Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> > Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)")
> > Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_context.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index c2ab0e22db0a..9627c7aac6a3 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -898,6 +898,11 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx,
> > unsigned int n;
> > e = alloc_engines(num_engines);
> > + if (!e) {
> > + return ERR_PTR(-ENOMEM);
> > + }
>
> Ideally remove the braces and respin.
>
Yep, checkpatch didn't like this. Will respin.
> > + e->num_engines = num_engines;
>
> Theoretically you could have put it next to "e->engines[n] = ce" assignment
> so the pattern is the same as in default_engines(). Kind of makes more sense
> that the number is not set before anything is created, but as it doesn't
> really matter since free_engines handles sparse arrays so there is argument
> to have a simpler single assignment as well.
>
I like a single assignment, let's not overthink this.
> > +
> > for (n = 0; n < num_engines; n++) {
> > struct intel_context *ce;
> > int ret;
> > @@ -931,7 +936,6 @@ static struct i915_gem_engines *user_engines(struct i915_gem_context *ctx,
> > goto free_engines;
> > }
> > }
> > - e->num_engines = num_engines;
> > return e;
> >
>
> Fix looks good to me. I did not want to butt in but since more than a week
> has passed without it getting noticed:
>
Again, ty.
Matt
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Regards,
>
> Tvrtko
