On Tue, Jan 27, 2015 at 12:13:14PM +0000, Tvrtko Ursulin wrote:
> On 01/27/2015 11:40 AM, Chris Wilson wrote:
> >On Tue, Jan 27, 2015 at 11:29:36AM +0000, Tvrtko Ursulin wrote:
> >>+static void i915_gem_request_unreference_worker(struct work_struct *work)
> >>+{
> >>+ struct drm_i915_gem_request *req =
> >>+ container_of(work, struct drm_i915_gem_request, unref_work);
> >>+ struct drm_device *dev = req->ring->dev;
> >>+
> >>+ mutex_lock(&dev->struct_mutex);
> >>+ i915_gem_request_unreference(req);
> >>+ mutex_unlock(&dev->struct_mutex);
> >>+}
> >>+
> >>+static int
> >>+i915_fence_ring_check(wait_queue_t *wait, unsigned mode, int flags, void *key)
> >>+{
> >>+ struct drm_i915_gem_request *req = wait->private;
> >>+ struct intel_engine_cs *ring = req->ring;
> >>+
> >>+ if (!i915_gem_request_completed(req, false))
> >>+ return 0;
> >>+
> >>+ fence_signal_locked(&req->fence);
> >>+
> >>+ __remove_wait_queue(&ring->irq_queue, wait);
> >>+ ring->irq_put(ring);
> >>+
> >>+ INIT_WORK(&req->unref_work, i915_gem_request_unreference_worker);
> >>+ schedule_work(&req->unref_work);
> >>+
> >>+ return 0;
> >>+}
> >>+
> >>+static bool i915_fence_ring_enable_signaling(struct fence *fence)
> >>+{
> >>+ struct drm_i915_gem_request *req = to_i915_request(fence);
> >>+ struct intel_engine_cs *ring = req->ring;
> >>+ struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >>+ wait_queue_t *wait = &req->wait;
> >>+
> >>+ /* queue fence wait queue on irq queue and get fence */
> >>+ if (i915_gem_request_completed(req, false) ||
> >>+ i915_terminally_wedged(&dev_priv->gpu_error))
> >>+ return false;
> >>+
> >>+ if (!ring->irq_get(ring))
> >>+ return false;
> >>+
> >>+ if (i915_gem_request_completed(req, false)) {
> >>+ ring->irq_put(ring);
> >>+ return true;
> >>+ }
> >>+
> >>+ wait->flags = 0;
> >>+ wait->private = req;
> >>+ wait->func = i915_fence_ring_check;
> >>+
> >>+ i915_gem_request_reference(req);
> >>+
> >>+ __add_wait_queue(&ring->irq_queue, wait);
> >>+
> >>+ return true;
> >>+}
> >
> >Explain how fence_release interacts with enable_signalling. Presumably
> >either the core fence routines cleanup the outstanding signalling, or we
> >are expected to. Doing so will remove the requirement for the extra
> >ref/unref (including the worker).
>
> I think normally we would be expected to use fence_get/put on the
> signaling side of things but that is not possible here since struct
> fence is embedded in the request.
>
> So as it is, sync_fence_release will tear everything down with our
> callback still on the irq_queue which is what taking a request
> reference sorts out.
So you are saying that we have a callback for when the fence is
discarded by userspace and we ignore that opportunity for disabling
interrupt generation, and remove the extra request references?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
