Re: [PATCH 11/15] drm/i915: Implementation of GuC client

Chris Wilson
2015-06-15T21:55:43Z

On Mon, Jun 15, 2015 at 07:36:29PM +0100, Dave Gordon wrote:
> +/* Get valid workqueue item and return it back to offset */
> +static int guc_get_workqueue_space(struct i915_guc_client *gc, u32 *offset)
> +{
> + struct guc_process_desc *desc;
> + void *base;
> + u32 size = sizeof(struct guc_wq_item);
> + int ret = 0, timeout_counter = 200;
> + unsigned long flags;
> +
> + base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> + desc = base + gc->proc_desc_offset;
> +
> + while (timeout_counter-- > 0) {
> + spin_lock_irqsave(&gc->wq_lock, flags);
> +
> + ret = wait_for_atomic(CIRC_SPACE(gc->wq_tail, desc->head,
> + gc->wq_size) >= size, 1);

What is the point of this loop? Drop the spinlock 200 times? You already
have a timeout, the loop extends that by a factor or 200. You merely
allow gazzumping, however I haven't looked at the locking to see what
you intend to lock (since it is not described at all).
-Chris

--
Chris Wilson, Intel Open Source Technology Centre