Re: [PATCH v2 11/15] drm/i915: Exercise request cancellation using a mock selftest

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

On 22/02/2017 11:46, Chris Wilson wrote:
> Add a mock selftest to preempt a request and check that we cancel it,
> requeue the request and then complete its execution.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/selftests/i915_gem_request.c | 59 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/selftests/mock_request.c     | 19 ++++++++
>  drivers/gpu/drm/i915/selftests/mock_request.h     |  2 +
>  3 files changed, 80 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c
> index 9d056a86723d..c803ef60a127 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c
> @@ -26,6 +26,7 @@
>
>  #include "../i915_selftest.h"
>
> +#include "mock_context.h"
>  #include "mock_gem_device.h"
>
>  static int igt_add_request(void *arg)
> @@ -181,12 +182,70 @@ static int igt_fence_wait(void *arg)
>  	return err;
>  }
>
> +static int igt_request_rewind(void *arg)
> +{
> +	struct drm_i915_private *i915 = arg;
> +	struct drm_i915_gem_request *request, *vip;
> +	struct i915_gem_context *ctx[2];
> +	int err = -EINVAL;
> +
> +	mutex_lock(&i915->drm.struct_mutex);
> +	ctx[0] = mock_context(i915, "A");
> +	request = mock_request(i915->engine[RCS], ctx[0], 2 * HZ);
> +	if (!request) {
> +		err = -ENOMEM;
> +		goto err_device;

Leaks ctx[0].

> +	}
> +	i915_add_request(request);
> +
> +	ctx[1] = mock_context(i915, "B");
> +	vip = mock_request(i915->engine[RCS], ctx[1], 0);
> +	if (!vip) {
> +		err = -ENOMEM;
> +		goto err_locked;

Leaks the request.

> +	}
> +
> +	/* Simulate preemption by manual reordering */
> +	if (!mock_cancel_request(request)) {
> +		pr_err("failed to cancel request (already executed)!\n");
> +		i915_add_request(vip);
> +		goto err_locked;
> +	}
> +	i915_add_request(vip);
> +	request->engine->submit_request(request);
> +
> +	mutex_unlock(&i915->drm.struct_mutex);
> +
> +	if (i915_wait_request(vip, 0, HZ) == -ETIME) {
> +		pr_err("timed out waiting for high priority request, vip.seqno=%d, current seqno=%d\n",
> +		       vip->global_seqno, intel_engine_get_seqno(i915->engine[RCS]));
> +		goto err;
> +	}
> +
> +	if (i915_gem_request_completed(request)) {
> +		pr_err("low priority request already completed\n");
> +		goto err;
> +	}
> +
> +	err = 0;
> +err:
> +	mutex_lock(&i915->drm.struct_mutex);
> +err_locked:
> +	mock_context_close(ctx[1]);
> +	mock_context_close(ctx[0]);
> +err_device:
> +	mock_device_flush(i915);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +	return err;
> +}
> +
>  int i915_gem_request_mock_selftests(void)
>  {
>  	static const struct i915_subtest tests[] = {
>  		SUBTEST(igt_add_request),
>  		SUBTEST(igt_wait_request),
>  		SUBTEST(igt_fence_wait),
> +		SUBTEST(igt_request_rewind),
>  	};
>  	struct drm_i915_private *i915;
>  	int err;
> diff --git a/drivers/gpu/drm/i915/selftests/mock_request.c b/drivers/gpu/drm/i915/selftests/mock_request.c
> index e23242d1b88a..0e8d2e7f8c70 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_request.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_request.c
> @@ -22,6 +22,7 @@
>   *
>   */
>
> +#include "mock_engine.h"
>  #include "mock_request.h"
>
>  struct drm_i915_gem_request *
> @@ -42,3 +43,21 @@ mock_request(struct intel_engine_cs *engine,
>
>  	return &mock->base;
>  }
> +
> +bool mock_cancel_request(struct drm_i915_gem_request *request)
> +{
> +	struct mock_request *mock = container_of(request, typeof(*mock), base);
> +	struct mock_engine *engine =
> +		container_of(request->engine, typeof(*engine), base);
> +	bool was_queued;
> +
> +	spin_lock_irq(&engine->hw_lock);
> +	was_queued = !list_empty(&mock->link);

I suggest a better name for the mock request. Even just req would be 
better IMO.

> +	list_del_init(&mock->link);
> +	spin_unlock_irq(&engine->hw_lock);
> +
> +	if (was_queued)
> +		i915_gem_request_unsubmit(request);
> +
> +	return was_queued;
> +}
> diff --git a/drivers/gpu/drm/i915/selftests/mock_request.h b/drivers/gpu/drm/i915/selftests/mock_request.h
> index cc76d4f4eb4e..4dea74c8e96d 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_request.h
> +++ b/drivers/gpu/drm/i915/selftests/mock_request.h
> @@ -41,4 +41,6 @@ mock_request(struct intel_engine_cs *engine,
>  	     struct i915_gem_context *context,
>  	     unsigned long delay);
>
> +bool mock_cancel_request(struct drm_i915_gem_request *request);
> +
>  #endif /* !__MOCK_REQUEST__ */
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx