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@jlekstrand.net Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org --- 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); + } + e->num_engines = num_engines; + 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;
+ Daniel as reviewer and maybe merge, avoid falling through cracks at least.
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@jlekstrand.net Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
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.
- 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.
- 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:
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@intel.com
Regards,
Tvrtko
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@jlekstrand.net Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
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@intel.com
Regards,
Tvrtko
On 01/10/2021 16:48, Matthew Brost wrote:
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.
I ended up pushing it myself to avoid having a potential crash in the driver for too long. Hope people will not mind.
Regards,
Tvrtko
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@jlekstrand.net Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create parameters (v5)") Signed-off-by: Matthew Brost matthew.brost@intel.com Cc: stable@vger.kernel.org
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@intel.com
Regards,
Tvrtko
dri-devel@lists.freedesktop.org
-
Matthew Brost -
Tvrtko Ursulin