On Thu, May 18, 2017 at 11:22:57AM -0700, Michel Thierry wrote:
> And store the active request so that we only search for it once; this
> applies for reset-engine and full reset.
>
> v2: Check for request completion inside _prepare_engine, don't use
> ECANCELED, remove unnecessary null checks (Chris).
>
> v3: Capture active requests during reset_prepare and store it the
> engine hangcheck obj.
>
> v4: Rename commit, change i915_gem_reset_request to just confirm the
> active_request is still incomplete, instead of engine_stalled (Chris).
>
> Suggested-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
>
> fixes
>
> Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 11 ++++++-----
> drivers/gpu/drm/i915/i915_drv.h | 3 ++-
> drivers/gpu/drm/i915/i915_gem.c | 34 +++++++++++++++++++++------------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 +
> 4 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index d62793805794..ec719376fc24 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1900,15 +1900,16 @@ int i915_reset_engine(struct intel_engine_cs *engine)
>
> DRM_DEBUG_DRIVER("resetting %s\n", engine->name);
>
> - ret = i915_gem_reset_prepare_engine(engine);
> - if (ret) {
> - DRM_ERROR("Previous reset failed - promote to full reset\n");
> + engine->hangcheck.active_request = i915_gem_reset_prepare_engine(engine);
Whilst this is not wrong (since we are serialising the per-engine and
global resets), I would suggest we avoid storing the request in the
hangcheck here and just pass the request along to
i915_gem_request_engine.
No strong reason, just less magic state passing between functions.
> + if (IS_ERR(engine->hangcheck.active_request)) {
> + DRM_DEBUG_DRIVER("Previous reset failed, promote to full reset\n");
> + ret = PTR_ERR(engine->hangcheck.active_request);
> goto out;
> }
>
> index b5dc073a5ddc..c9f139b322d2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2793,12 +2793,14 @@ static bool engine_stalled(struct intel_engine_cs *engine)
> return true;
> }
>
> -/* Ensure irq handler finishes, and not run again. */
> -int i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
> +/*
> + * Ensure irq handler finishes, and not run again.
> + * Also store the active request so that we only search for it once.
> + */
> +struct drm_i915_gem_request *
> +i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
> {
> - struct drm_i915_gem_request *request;
> - int err = 0;
> -
> + struct drm_i915_gem_request *request = NULL;
>
> /* Prevent the signaler thread from updating the request
> * state (by calling dma_fence_signal) as we are processing
> @@ -2827,21 +2829,30 @@ int i915_gem_reset_prepare_engine(struct intel_engine_cs *engine)
>
> if (engine_stalled(engine)) {
> request = i915_gem_find_active_request(engine);
> +
If we neuter the return beneath the if, this blank line can also go.
> if (request && request->fence.error == -EIO)
> - err = -EIO; /* Previous reset failed! */
> + return ERR_PTR(-EIO); /* Previous reset failed! */
request = ERR_PTR(-EIO); and then keep the single return.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
