Re: [PATCH v2 11/28] drm/i915: Add IRQ friendly request deference facility

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 18/11/2014 09:34, Daniel, Thomas wrote:
-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf
Of John.C.Harrison@xxxxxxxxx
Sent: Friday, November 14, 2014 12:19 PM
To: Intel-GFX@xxxxxxxxxxxxxxxxxxxxx
Subject:  [PATCH v2 11/28] drm/i915: Add IRQ friendly request
deference facility

From: John Harrison <John.C.Harrison@xxxxxxxxx>

The next patches in the series convert some display related seqno usage to
request structure usage. However, the request dereference introduced
must be done from interrupt context. As the dereference potentially
involves freeing the request structure and thus calling lots of non-interrupt
friendly code, this poses a problem.

The solution is to add an IRQ friendly version of the dereference function. All
this does is to add the request structure to a 'delayed free' list and return.
The retire code, which is run periodically, then processes this list and does
the actual dereferencing of the request structures.

v2: Added a count to allow for multiple IRQ dereferences of the same
request at a time.

For: VIZ-4377
Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.h         |    7 +++++++
  drivers/gpu/drm/i915/i915_gem.c         |   33
+++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_lrc.c        |    2 ++
  drivers/gpu/drm/i915/intel_ringbuffer.c |    3 +++
  drivers/gpu/drm/i915/intel_ringbuffer.h |    2 ++
  5 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h index 180b674..87cb355 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2019,6 +2019,10 @@ struct drm_i915_gem_request {
  	struct drm_i915_file_private *file_priv;
  	/** file_priv list entry for this request */
  	struct list_head client_list;
+
+	/** deferred free list for dereferencing from IRQ context */
+	struct list_head delay_free_list;
+	uint32_t delay_free_count;
  };

  void i915_gem_request_free(struct kref *req_ref); @@ -2044,9 +2048,12
@@ i915_gem_request_reference(struct drm_i915_gem_request *req)
static inline void  i915_gem_request_unreference(struct
drm_i915_gem_request *req)  {
+	WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
  	kref_put(&req->ref, i915_gem_request_free);  }

+void i915_gem_request_unreference_irq(struct drm_i915_gem_request
+*req);
+
  static inline void i915_gem_request_assign(struct drm_i915_gem_request
**pdst,
  					   struct drm_i915_gem_request *src)
{ diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c index 60e5eec..8453bbd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2722,6 +2722,19 @@ void i915_gem_reset(struct drm_device *dev)
  	i915_gem_restore_fences(dev);
  }

+void i915_gem_request_unreference_irq(struct drm_i915_gem_request
*req)
+{
+	struct intel_engine_cs *ring = req->ring;
+	unsigned long flags;
+
+	/* Need to add the req to a deferred dereference list to be
processed
+	 * outside of interrupt time */
+	spin_lock_irqsave(&ring->reqlist_lock, flags);
+	if (req->delay_free_count++ == 0)
+		list_add_tail(&req->delay_free_list, &ring-
delayed_free_list);
+	spin_unlock_irqrestore(&ring->reqlist_lock, flags); }
+
  /**
   * This function clears the request list as sequence numbers are passed.
   */
@@ -2796,6 +2809,25 @@ i915_gem_retire_requests_ring(struct
intel_engine_cs *ring)
  		ring->trace_irq_seqno = 0;
  	}

+	while (!list_empty(&ring->delayed_free_list)) {
+		struct drm_i915_gem_request *request;
+		unsigned long flags;
+		uint32_t count;
+
+		request = list_first_entry(&ring->delayed_free_list,
+					   struct drm_i915_gem_request,
+					   delay_free_list);
+
+		spin_lock_irqsave(&request->ring->reqlist_lock, flags);
+		list_del(&request->delay_free_list);
+		count = request->delay_free_count;
+		request->delay_free_count = 0;
+		spin_unlock_irqrestore(&request->ring->reqlist_lock, flags);
+
+		while (count-- > 0)
+			i915_gem_request_unreference(request);
+	}
+
  	WARN_ON(i915_verify_lists(ring->dev));
  }

@@ -5055,6 +5087,7 @@ init_ring_lists(struct intel_engine_cs *ring)  {
  	INIT_LIST_HEAD(&ring->active_list);
  	INIT_LIST_HEAD(&ring->request_list);
+	INIT_LIST_HEAD(&ring->delayed_free_list);
Same comment as before - multiple init points for this list.

Thomas.
Same reply as before: the function already exists and is already initialising other lists therefore it seems sensible to initialise the new list as well. Whether the function as a whole is redundant or not is unclear. The same duplicated initialisation also happens in intel_render_ring_init_dri(). If someone thinks these can all be simplified and wants to only do the initialisation once in one place then that should be another patch.

  }

  void i915_init_vm(struct drm_i915_private *dev_priv, diff --git
a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index eba0acd..db8efaa 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1287,7 +1287,9 @@ static int logical_ring_init(struct drm_device *dev,
struct intel_engine_cs *rin
  	ring->dev = dev;
  	INIT_LIST_HEAD(&ring->active_list);
  	INIT_LIST_HEAD(&ring->request_list);
+	spin_lock_init(&ring->reqlist_lock);
  	init_waitqueue_head(&ring->irq_queue);
+	INIT_LIST_HEAD(&ring->delayed_free_list);

  	INIT_LIST_HEAD(&ring->execlist_queue);
  	spin_lock_init(&ring->execlist_lock);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 74c48ed..4338132 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1808,6 +1808,8 @@ static int intel_init_ring_buffer(struct drm_device
*dev,
  	ring->dev = dev;
  	INIT_LIST_HEAD(&ring->active_list);
  	INIT_LIST_HEAD(&ring->request_list);
+	spin_lock_init(&ring->reqlist_lock);
+	INIT_LIST_HEAD(&ring->delayed_free_list);
  	INIT_LIST_HEAD(&ring->execlist_queue);
  	ringbuf->size = 32 * PAGE_SIZE;
  	ringbuf->ring = ring;
@@ -2510,6 +2512,7 @@ int intel_render_ring_init_dri(struct drm_device
*dev, u64 start, u32 size)
  	ring->dev = dev;
  	INIT_LIST_HEAD(&ring->active_list);
  	INIT_LIST_HEAD(&ring->request_list);
+	INIT_LIST_HEAD(&ring->delayed_free_list);

  	ringbuf->size = size;
  	ringbuf->effective_size = ringbuf->size; diff --git
a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 824f5884..3af7b7c 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -262,6 +262,8 @@ struct  intel_engine_cs {
  	 * outstanding.
  	 */
  	struct list_head request_list;
+	spinlock_t reqlist_lock;
+	struct list_head delayed_free_list;

  	/**
  	 * Do we have some not yet emitted requests outstanding?
--
1.7.9.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux