Re: [PATCH 01/13] drm/i915: Add a sw fence for collecting up dma fences

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2016-08-25 at 10:08 +0100, Chris Wilson wrote:
> +++ b/drivers/gpu/drm/i915/i915_sw_fence.c
> @@ -0,0 +1,329 @@
> +/*
> + * (C) Copyright 2016 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */

Copyright notice is rather brief, copy from i915_gem_execbuffer.c

> +static int __i915_sw_fence_notify(struct i915_sw_fence *fence)
> +{
> +	i915_sw_fence_notify_t fn;
> +
> +	fn = (i915_sw_fence_notify_t)(fence->flags & I915_SW_FENCE_MASK);

I'd define an accessor functions in scatterlist.h fashion.

Although I'm not convinced of packing the flags with the function
pointer. How many fences do we expect to be allocated simultaneously on
a real usage scenario?

> +static void __i915_sw_fence_wake_up_all(struct i915_sw_fence *fence,
> +				 struct list_head *continuation)
> +{
> +	wait_queue_head_t *x = &fence->wait;
> +	unsigned long flags;
> +
> +	atomic_dec(&fence->pending);
> +
> +	/*
> +	 * To prevent unbounded recursion as we traverse the graph of i915_sw_fences,

Long line due to s/kfence/.../

> +	 * we move the task_list from this the next ready fence to the tail of
> +	 * the original fence's task_list (and so added to the list to be
> +	 * woken).
> +	 */
> +	smp_mb__before_spinlock();
> +	if (!list_empty_careful(&x->task_list)) {

if (list_empty_careful()
	return;

> +static bool __i915_sw_fence_check_if_after(struct i915_sw_fence *fence,
> +				    const struct i915_sw_fence * const signaler)
> +{
> +	wait_queue_t *wq;
> +
> +	if (__test_and_set_bit(I915_SW_FENCE_CHECKED_BIT, &fence->flags))
> +		return false;
> +
> +	if (fence == signaler)
> +		return true;

Not sure if I would expect _if_after to return true on an equal
object...

> +int i915_sw_fence_await_reservation(struct i915_sw_fence *fence,
> +				    struct reservation_object *resv,
> +				    const struct fence_ops *exclude,
> +				    bool write,
> +				    gfp_t gfp)
> +{
> +	struct fence *excl, **shared;
> +	unsigned int count, i;
> +	int ret;
> +
> +	ret = reservation_object_get_fences_rcu(resv, &excl, &count, &shared);
> +	if (ret)
> +		return ret;
> +
> +	if (write) {
> +		for (i = 0; i < count; i++) {
> +			if (shared[i]->ops == exclude)
> +				continue;
> +
> +			ret |= i915_sw_fence_await_dma_fence(fence, shared[i], gfp);

Do we really want to bitwise here?

> +			if (ret < 0)
> +				goto out;
> +		}
> +	}
> +
> +	if (excl && excl->ops != exclude)
> +		ret |= i915_sw_fence_await_dma_fence(fence, excl, gfp);

Ditto.

> +
> +out:
> +	fence_put(excl);
> +	for (i = 0; i < count; i++)
> +		fence_put(shared[i]);
> +	kfree(shared);
> +
> +	return ret;
> +}

<SNIP>

> +struct i915_sw_fence {
> +	wait_queue_head_t wait;
> +	unsigned long flags;

Name is rather deceiving to contain a callback function.

> +#define __i915_sw_fence_call __aligned(4)

Not packing would get rid of this too...

> +
> +void i915_sw_fence_init(struct i915_sw_fence *fence, i915_sw_fence_notify_t fn);
> +void i915_sw_fence_commit(struct i915_sw_fence *fence);

Not _signal?

> +static inline bool i915_sw_fence_done(const struct i915_sw_fence *fence)

_is_done() or _is_completed()

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx