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
