Re: [PATCH 1/2] drm/i915: Android sync points for i915 v2

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 03, 2014 at 08:09:01AM +0100, Chris Wilson wrote:
> On Tue, Sep 02, 2014 at 02:32:40PM -0700, Jesse Barnes wrote:
> > +static int i915_fence_check(wait_queue_t *wait, unsigned mode, int flags,
> > +			    void *key)
> > +{
> > +	struct i915_fence *intel_fence = wait->private;
> > +	struct intel_engine_cs *ring = intel_fence->ring;
> > +
> > +	if (!i915_seqno_passed(ring->get_seqno(ring, false),
> > +			       intel_fence->seqno))
> > +		return 0;
> > +
> > +	fence_signal_locked(&intel_fence->base);
> > +
> > +	__remove_wait_queue(&ring->irq_queue, wait);
> > +	fence_put(&intel_fence->base);
> > +	ring->irq_put(ring);
> > +
> > +	return 0;
> > +}
> > +
> > +static bool i915_fence_enable_signaling(struct fence *fence)
> > +{
> > +	struct i915_fence *intel_fence = to_intel_fence(fence);
> > +	struct intel_engine_cs *ring = intel_fence->ring;
> > +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > +	wait_queue_t *wait = &intel_fence->wait;
> > +
> > +	/* queue fence wait queue on irq queue and get fence */
> > +	if (i915_seqno_passed(ring->get_seqno(ring, false),
> > +			      intel_fence->seqno) ||
> > +	    i915_terminally_wedged(&dev_priv->gpu_error))
> > +		return false;
> > +
> > +	if (!ring->irq_get(ring))
> > +		return false;
> > +
> > +	wait->flags = 0;
> > +	wait->private = intel_fence;
> > +	wait->func = i915_fence_check;
> > +
> > +	__add_wait_queue(&ring->irq_queue, wait);
> > +	fence_get(fence);
> > +
> > +	return true;
> > +}
> 
> This looks like it implements poll(). 
> 
> You should recheck i915_request_complete() after setting up the irq
> waiter. Or does struct fence_ops handle that?

Also looks quite a bit like my ring notify doohicky from:
http://lists.freedesktop.org/archives/intel-gfx/2014-June/047623.html

Except I kept the list in the driver so you would need to do only one
get_seqno() per irq. Also if the list would be sorted (which it wasn't
in my patch) it would prevent signalling the fences out of order. But
maybe that's not really a problem for anyone.

Hmm, so if the out of order thing isn't a problem maybe use the wait
queue still but replace the wake_up() with __wake_up() so that the seqno
can be passed in as the key. That's assuming people care about
optimizing the seqno reads.

-- 
Ville Syrjälä
Intel OTC