Re: [PATCH v2 3/5] drm/i915/guc: don't spinwait if the GuC's workqueue is full

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

On 27/04/16 19:03, Dave Gordon wrote:
> Rather than wait to see whether more space becomes available in the GuC
> submission workqueue, we can just return -EAGAIN and let the caller try
> again in a little while. This gets rid of an uninterruptable sleep in
> the polling code :)
>
> We'll also add a counter to the GuC client statistics, to see how often
> we find the WQ full.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        |  1 +
>   drivers/gpu/drm/i915/i915_guc_submission.c | 16 +++++-----------
>   drivers/gpu/drm/i915/intel_guc.h           |  8 ++++----
>   3 files changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8b8d6f0..1024947 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2509,6 +2509,7 @@ static void i915_guc_client_info(struct seq_file *m,
>   	seq_printf(m, "\tWQ size %d, offset: 0x%x, tail %d\n",
>   		client->wq_size, client->wq_offset, client->wq_tail);
>
> +	seq_printf(m, "\tWork queue full: %u\n", client->no_wq_space);
>   	seq_printf(m, "\tFailed to queue: %u\n", client->q_fail);
>   	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
>   	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 66af5ce..6626eff 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -453,27 +453,21 @@ static void guc_fini_ctx_desc(struct intel_guc *guc,
>
>   int i915_guc_wq_check_space(struct drm_i915_gem_request *request)
>   {
> -	const size_t size = sizeof(struct guc_wq_item);
> +	const size_t wqi_size = sizeof(struct guc_wq_item);
>   	struct i915_guc_client *gc = request->i915->guc.execbuf_client;
>   	struct guc_process_desc *desc;
> -	int ret = -ETIMEDOUT, timeout_counter = 200;
>
>   	if (!gc)
>   		return 0;

Not part of this patch but spotted it - can this really happen, and if 
so, does it warrant a WARN_ONCE, GEM_BUG_ON or something?

>
>   	desc = gc->client_base + gc->proc_desc_offset;
>
> -	while (timeout_counter-- > 0) {
> -		if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
> -			ret = 0;
> -			break;
> -		}
> +	if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= wqi_size)
> +		return 0;

Could stick a likely here to lay out the code for the expected case but 
it could be just OCD. :)

>
> -		if (timeout_counter)
> -			usleep_range(1000, 2000);
> -	};
> +	gc->no_wq_space += 1;
>
> -	return ret;
> +	return -EAGAIN;

I suppose it is OK to make userspace pay the cost since this should be 
extremely rare, correct?

>   }
>
>   static int guc_add_workqueue_item(struct i915_guc_client *gc,
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index b37c731..436f2d6 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -73,10 +73,10 @@ struct i915_guc_client {
>
>   	/* GuC submission statistics & status */
>   	uint64_t submissions[GUC_MAX_ENGINES_NUM];
> -	uint32_t q_fail;
> -	uint32_t b_fail;
> -	int retcode;
> -	int spare;			/* pad to 32 DWords		*/
> +	uint32_t no_wq_space;		/* Space pre-check failed	*/
> +	uint32_t q_fail;		/* Failed to queue (MBZ)	*/
> +	uint32_t b_fail;		/* Doorbell failure (MBZ)	*/
> +	int retcode;			/* Result of last guc_submit()	*/
>   };
>
>   enum intel_guc_fw_status {
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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