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

[Chris Wilson]

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