[Intel-gfx] [PATCH] drm/i915/guc: Clear terminated attribute bit on GuC preemption context

[Michel Thierry]

On 01/11/17 15:16, jeff.mcgee at intel.com wrote:
> From: Jeff McGee <jeff.mcgee at intel.com>
> 
> If GuC firmware performs an engine reset while that engine had a
> preemption pending, it will set the terminated attribute bit on our
> preemption stage descriptor. GuC firmware retains all pending work
> items for a high-priority GuC client, unlike the normal-priority GuC
> client where work items are dropped. It wants to make sure the preempt-
> to-idle work doesn't run when scheduling resumes, and uses this bit to
> inform its scheduler and presumably us as well. Our job is to clear it
> for the next preemption after reset, otherwise that and future
> preemptions will never complete. We'll just clear it every time.
> 
> Signed-off-by: Jeff McGee <jeff.mcgee at intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 3049a0781b88..d14c1342f09d 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -590,6 +590,7 @@ static void inject_preempt_context(struct work_struct *work)
>          struct intel_guc *guc = container_of(preempt_work, typeof(*guc),
>                                               preempt_work[engine->id]);
>          struct i915_guc_client *client = guc->preempt_client;
> +       struct guc_stage_desc *stage_desc = __get_stage_desc(client);
>          struct intel_ring *ring = client->owner->engine[engine->id].ring;
>          u32 ctx_desc = lower_32_bits(intel_lr_context_descriptor(client->owner,
>                                                                   engine));
> @@ -623,6 +624,20 @@ static void inject_preempt_context(struct work_struct *work)
>                             ring->tail / sizeof(u64), 0);
>          spin_unlock_irq(&client->wq_lock);
> 
> +       /*
> +        * If GuC firmware performs an engine reset while that engine had
> +        * a preemption pending, it will set the terminated attribute bit
> +        * on our preemption stage descriptor. GuC firmware retains all
> +        * pending work items for a high-priority GuC client, unlike the
> +        * normal-priority GuC client where work items are dropped. It
> +        * wants to make sure the preempt-to-idle work doesn't run when
> +        * scheduling resumes, and uses this bit to inform its scheduler
> +        * and presumably us as well. Our job is to clear it for the next
> +        * preemption after reset, otherwise that and future preemptions
> +        * will never complete. We'll just clear it every time.
> +        */
> +       stage_desc->attribute &= ~GUC_STAGE_DESC_ATTR_TERMINATED;
> +

I looked around and yes, the firmware will set the terminated bit in the 
stage_desc of the preempt client if it had a pending preemption request. 
No harm in clearing it unconditionally.

>          data[0] = INTEL_GUC_ACTION_REQUEST_PREEMPTION;
>          data[1] = client->stage_id;
>          data[2] = INTEL_GUC_PREEMPT_OPTION_DROP_WORK_Q |
> --
> 2.14.2

Since Michał is not around,

Reviewed-by: Michel Thierry <michel.thierry at intel.com>