On Thu, Aug 25, 2016 at 02:49:59PM +0300, Joonas Lahtinen wrote:
> 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.
Hmm, this was meant to be the accessor function - as nowhere else was
meant to be pulling out the fn from the mask. But that can be split out
again.
> 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?
Several thousand for async init. Tens of thousands per second for i915,
though we only except a few hundred in flight at any time. Ditto for
kms. Ditto for async work. And places I haven't looked at yet.
That's more than there are mutex_init!
> > +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...
True being the error case.
kfence_err_if_after?
kfence_abort_if_after?
> > +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?
Correctly it should be
int pending = await_dma_fence();
if (pending < 0) {
ret = pending;
goto out;
}
ret |= pending;
I've been using the ternary result as a shortcut (if the fence is
passed, we can dispose of it for example). The code becomes much simpler
if we didn't pass that information back - but I think it is worth it.
> > +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?
signal is fence_complete() (corresponding to complete(struct
completion))
commit is a convenience function for complete + kref_put. I do like it
as it gives a clear finish to a
kfence_init(); .. setup ...; kfence_commit()
sequence. But the name is only so-so.
> > +static inline bool i915_sw_fence_done(const struct i915_sw_fence *fence)
>
> _is_done() or _is_completed()
As above, instructions were to use _done, so that is matched struct
completion as closely as possible. They have already chosen their
interface, we have to try and fit in as close as possible.
I do agree, I much prefer having _is_ in my boolean functions - but
matching struct completion is a must for upstream proposal (imo).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
