Re: [PATCH 01/25] drm/i915: Add ring_notify mechanism

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 18, 2014 at 08:58:34PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Proveide a ring notify mechanism where you can ask for a callback when a
> specific seqno has been passed.
> 
> Can be used for FBC and mmio flips at least.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hm, most of the callbacks I can image need some process context anyway due
to at least grabbing a mutex or something similar. And we support lockless
seqno waits. So with that I'd lean towards offloading this into work items
and just doing a lockless wait in the overall sequence since doing full
callback-based programming is hard. But I haven't looked at the users at
all here tbh.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c         | 10 ++++
>  drivers/gpu/drm/i915/i915_irq.c         |  4 ++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 89 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 17 +++++++
>  4 files changed, 120 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d857f58..a5e62cb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2521,6 +2521,8 @@ static void i915_gem_reset_ring_status(struct drm_i915_private *dev_priv,
>  static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
>  					struct intel_engine_cs *ring)
>  {
> +	struct intel_ring_notify *notify, *next;
> +
>  	while (!list_empty(&ring->active_list)) {
>  		struct drm_i915_gem_object *obj;
>  
> @@ -2552,6 +2554,14 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv,
>  	kfree(ring->preallocated_lazy_request);
>  	ring->preallocated_lazy_request = NULL;
>  	ring->outstanding_lazy_seqno = 0;
> +
> +	spin_lock_irq(&ring->lock);
> +	list_for_each_entry_safe(notify, next, &ring->notify_list, list) {
> +		intel_ring_notify_complete(notify);
> +		/* FIXME should we notify at reset? */
> +		notify->notify(notify);
> +	}
> +	spin_unlock_irq(&ring->lock);
>  }
>  
>  void i915_gem_restore_fences(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1c1ec22..218f011 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1221,6 +1221,10 @@ static void notify_ring(struct drm_device *dev,
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		intel_notify_mmio_flip(ring);
>  
> +	spin_lock(&ring->lock);
> +	intel_ring_notify_check(ring);
> +	spin_unlock(&ring->lock);
> +
>  	wake_up_all(&ring->irq_queue);
>  	i915_queue_hangcheck(dev);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index b96edaf..31321ae 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1447,6 +1447,9 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  	init_waitqueue_head(&ring->irq_queue);
>  
> +	INIT_LIST_HEAD(&ring->notify_list);
> +	spin_lock_init(&ring->lock);
> +
>  	if (I915_NEED_GFX_HWS(dev)) {
>  		ret = init_status_page(ring);
>  		if (ret)
> @@ -2407,3 +2410,89 @@ intel_stop_ring_buffer(struct intel_engine_cs *ring)
>  
>  	stop_ring(ring);
>  }
> +
> +void intel_ring_notify_complete(struct intel_ring_notify *notify)
> +{
> +	struct intel_engine_cs *ring = notify->ring;
> +
> +	ring->irq_put(ring);
> +	list_del(&notify->list);
> +	notify->ring = NULL;
> +}
> +
> +void intel_ring_notify_check(struct intel_engine_cs *ring)
> +{
> +	struct intel_ring_notify *notify, *next;
> +	u32 seqno;
> +
> +	assert_spin_locked(&ring->lock);
> +
> +	if (list_empty(&ring->notify_list))
> +		return;
> +
> +	seqno = ring->get_seqno(ring, false);
> +
> +	list_for_each_entry_safe(notify, next, &ring->notify_list, list) {
> +		if (i915_seqno_passed(seqno, notify->seqno)) {
> +			intel_ring_notify_complete(notify);
> +			notify->notify(notify);
> +		}
> +	}
> +}
> +
> +int intel_ring_notify_add(struct intel_engine_cs *ring,
> +			  struct intel_ring_notify *notify)
> +{
> +	unsigned long irqflags;
> +	int ret;
> +
> +	lockdep_assert_held(&ring->dev->struct_mutex);
> +
> +	if (WARN_ON(notify->ring != NULL || notify->seqno == 0))
> +		return -EINVAL;
> +
> +	if (i915_seqno_passed(ring->get_seqno(ring, true), notify->seqno))
> +		goto notify_immediately;
> +
> +	ret = i915_gem_check_olr(ring, notify->seqno);
> +	if (ret)
> +		return ret;
> +
> +	if (WARN_ON(!ring->irq_get(ring)))
> +		goto notify_immediately;
> +
> +	spin_lock_irqsave(&ring->lock, irqflags);
> +	notify->ring = ring;
> +	list_add_tail(&notify->list, &ring->notify_list);
> +	/* check again in case we just missed it */
> +	intel_ring_notify_check(ring);
> +	spin_unlock_irqrestore(&ring->lock, irqflags);
> +
> +	return 0;
> +
> + notify_immediately:
> +	spin_lock_irqsave(&ring->lock, irqflags);
> +	notify->notify(notify);
> +	spin_unlock_irqrestore(&ring->lock, irqflags);
> +
> +	return 0;
> +}
> +
> +bool intel_ring_notify_pending(struct intel_ring_notify *notify)
> +{
> +	return notify->ring != NULL;
> +}
> +
> +void intel_ring_notify_cancel(struct intel_ring_notify *notify)
> +{
> +	struct intel_engine_cs *ring = ACCESS_ONCE(notify->ring);
> +	unsigned long irqflags;
> +
> +	if (!ring)
> +		return;
> +
> +	spin_lock_irqsave(&ring->lock, irqflags);
> +	if (notify->ring)
> +		intel_ring_notify_complete(notify);
> +	spin_unlock_irqrestore(&ring->lock, irqflags);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index e72017b..273abf3 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -79,6 +79,13 @@ struct intel_ringbuffer {
>  	u32 last_retired_head;
>  };
>  
> +struct intel_ring_notify {
> +	void (*notify)(struct intel_ring_notify *notify);
> +	struct intel_engine_cs *ring;
> +	struct list_head list;
> +	u32 seqno;
> +};
> +
>  struct  intel_engine_cs {
>  	const char	*name;
>  	enum intel_ring_id {
> @@ -217,6 +224,9 @@ struct  intel_engine_cs {
>  	 * to encode the command length in the header).
>  	 */
>  	u32 (*get_cmd_length_mask)(u32 cmd_header);
> +
> +	struct list_head notify_list;
> +	spinlock_t lock;
>  };
>  
>  static inline bool
> @@ -335,6 +345,13 @@ static inline void i915_trace_irq_get(struct intel_engine_cs *ring, u32 seqno)
>  		ring->trace_irq_seqno = seqno;
>  }
>  
> +void intel_ring_notify_complete(struct intel_ring_notify *notify);
> +void intel_ring_notify_check(struct intel_engine_cs *ring);
> +int __must_check intel_ring_notify_add(struct intel_engine_cs *ring,
> +				       struct intel_ring_notify *notify);
> +bool intel_ring_notify_pending(struct intel_ring_notify *notify);
> +void intel_ring_notify_cancel(struct intel_ring_notify *notify);
> +
>  /* DRI warts */
>  int intel_render_ring_init_dri(struct drm_device *dev, u64 start, u32 size);
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch