Re: [Intel-gfx] [PATCH 06/11] drm/i915/pvc: Reduce stack usage in reset selftest with extra blitter engine

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 02/05/2022 17:34, Matt Roper wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> PVC adds extra blitter engines (in the following patch). The reset
> selftest has a local array on the stack which is sized by the number
> of engines. The increase pushes the size of this array to the point
> where it trips the 'stack too large' compile warning. This patch takes
> the allocation of the stack and makes it dynamic instead.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> index 83ff4c2e57c5..3b9d82276db2 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c
> @@ -979,6 +979,7 @@ static int __igt_reset_engines(struct intel_gt *gt,
>   	enum intel_engine_id id, tmp;
>   	struct hang h;
>   	int err = 0;
> +	struct active_engine *threads;

Drive by nits - sticks out like a sore thumb a bit here - I'd put it 
above "id, tmp" so it's all nicely sorted by width.
>   
>   	/* Check that issuing a reset on one engine does not interfere
>   	 * with any other engine.
> @@ -996,8 +997,11 @@ static int __igt_reset_engines(struct intel_gt *gt,
>   			h.ctx->sched.priority = 1024;
>   	}
>   
> +	threads = kzalloc(sizeof(*threads) * I915_NUM_ENGINES, GFP_KERNEL);

And this could be kcalloc (or kmalloc_array since zeroing is not needed) 
if that's any better. Seems selftests use that pattern anyway.

Both comments are optional really.

Regards,

Tvrtko

> +	if (!threads)
> +		return -ENOMEM;
> +
>   	for_each_engine(engine, gt, id) {
> -		struct active_engine threads[I915_NUM_ENGINES] = {};
>   		unsigned long device = i915_reset_count(global);
>   		unsigned long count = 0, reported;
>   		bool using_guc = intel_engine_uses_guc(engine);
> @@ -1016,7 +1020,7 @@ static int __igt_reset_engines(struct intel_gt *gt,
>   			break;
>   		}
>   
> -		memset(threads, 0, sizeof(threads));
> +		memset(threads, 0, sizeof(*threads) * I915_NUM_ENGINES);
>   		for_each_engine(other, gt, tmp) {
>   			struct task_struct *tsk;
>   
> @@ -1236,6 +1240,7 @@ static int __igt_reset_engines(struct intel_gt *gt,
>   			break;
>   		}
>   	}
> +	kfree(threads);
>   
>   	if (intel_gt_is_wedged(gt))
>   		err = -EIO;