Re: [PATCH 28/53] drm/i915: Update queue_flip() to do explicit request management

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

 



On Thu, Mar 05, 2015 at 07:29:27PM +0000, Tomas Elf wrote:
> On 19/02/2015 17:17, John.C.Harrison@xxxxxxxxx wrote:
> >From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> >Updated the display page flip code to do explicit request creation and
> >submission rather than relying on the OLR and just hoping that the request
> >actually gets submitted at some random point.
> >
> >The sequence is now to create a request, queue the work to the ring, assign the
> >known request to the flip queue work item then actually submit the work and post
> >the request.
> >
> >For: VIZ-5115
> >Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> >---
> >  drivers/gpu/drm/i915/i915_drv.h         |    2 +-
> >  drivers/gpu/drm/i915/intel_display.c    |   43 ++++++++++++++++++++-----------
> >  drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |    1 -
> >  4 files changed, 30 insertions(+), 18 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >index e9cc343..34fd338 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -560,7 +560,7 @@ struct drm_i915_display_funcs {
> >  	int (*queue_flip)(struct drm_device *dev, struct drm_crtc *crtc,
> >  			  struct drm_framebuffer *fb,
> >  			  struct drm_i915_gem_object *obj,
> >-			  struct intel_engine_cs *ring,
> >+			  struct drm_i915_gem_request *req,
> >  			  uint32_t flags);
> >  	void (*update_primary_plane)(struct drm_crtc *crtc,
> >  				     struct drm_framebuffer *fb,
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index 3b0fe9f..c32bc0c 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -9251,9 +9251,10 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> >  				 struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb,
> >  				 struct drm_i915_gem_object *obj,
> >-				 struct intel_engine_cs *ring,
> >+				 struct drm_i915_gem_request *req,
> >  				 uint32_t flags)
> >  {
> >+	struct intel_engine_cs *ring = req->ring;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	u32 flip_mask;
> >  	int ret;
> >@@ -9278,7 +9279,6 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> >  	intel_ring_emit(ring, 0); /* aux display base address, unused */
> >
> >  	intel_mark_page_flip_active(intel_crtc);
> >-	__intel_ring_advance(ring);
> >  	return 0;
> >  }
> >
> >@@ -9286,9 +9286,10 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
> >  				 struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb,
> >  				 struct drm_i915_gem_object *obj,
> >-				 struct intel_engine_cs *ring,
> >+				 struct drm_i915_gem_request *req,
> >  				 uint32_t flags)
> >  {
> >+	struct intel_engine_cs *ring = req->ring;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	u32 flip_mask;
> >  	int ret;
> >@@ -9310,7 +9311,6 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
> >  	intel_ring_emit(ring, MI_NOOP);
> >
> >  	intel_mark_page_flip_active(intel_crtc);
> >-	__intel_ring_advance(ring);
> >  	return 0;
> >  }
> >
> >@@ -9318,9 +9318,10 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> >  				 struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb,
> >  				 struct drm_i915_gem_object *obj,
> >-				 struct intel_engine_cs *ring,
> >+				 struct drm_i915_gem_request *req,
> >  				 uint32_t flags)
> >  {
> >+	struct intel_engine_cs *ring = req->ring;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	uint32_t pf, pipesrc;
> >@@ -9349,7 +9350,6 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> >  	intel_ring_emit(ring, pf | pipesrc);
> >
> >  	intel_mark_page_flip_active(intel_crtc);
> >-	__intel_ring_advance(ring);
> >  	return 0;
> >  }
> >
> >@@ -9357,9 +9357,10 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> >  				 struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb,
> >  				 struct drm_i915_gem_object *obj,
> >-				 struct intel_engine_cs *ring,
> >+				 struct drm_i915_gem_request *req,
> >  				 uint32_t flags)
> >  {
> >+	struct intel_engine_cs *ring = req->ring;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	uint32_t pf, pipesrc;
> >@@ -9385,7 +9386,6 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> >  	intel_ring_emit(ring, pf | pipesrc);
> >
> >  	intel_mark_page_flip_active(intel_crtc);
> >-	__intel_ring_advance(ring);
> >  	return 0;
> >  }
> >
> >@@ -9393,9 +9393,10 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  				 struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb,
> >  				 struct drm_i915_gem_object *obj,
> >-				 struct intel_engine_cs *ring,
> >+				 struct drm_i915_gem_request *req,
> >  				 uint32_t flags)
> >  {
> >+	struct intel_engine_cs *ring = req->ring;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >  	uint32_t plane_bit = 0;
> >  	int len, ret;
> >@@ -9480,7 +9481,6 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  	intel_ring_emit(ring, (MI_NOOP));
> >
> >  	intel_mark_page_flip_active(intel_crtc);
> >-	__intel_ring_advance(ring);
> >  	return 0;
> >  }
> >
> >@@ -9636,7 +9636,7 @@ static int intel_default_queue_flip(struct drm_device *dev,
> >  				    struct drm_crtc *crtc,
> >  				    struct drm_framebuffer *fb,
> >  				    struct drm_i915_gem_object *obj,
> >-				    struct intel_engine_cs *ring,
> >+				    struct drm_i915_gem_request *req,
> >  				    uint32_t flags)
> >  {
> >  	return -ENODEV;
> >@@ -9715,6 +9715,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	enum pipe pipe = intel_crtc->pipe;
> >  	struct intel_unpin_work *work;
> >  	struct intel_engine_cs *ring;
> >+	struct drm_i915_gem_request *request;
> >  	int ret;
> >
> >  	/*
> >@@ -9828,13 +9829,20 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  		i915_gem_request_assign(&work->flip_queued_req,
> >  					obj->last_write_req);
> >  	} else {
> >-		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
> >-						   page_flip_flags);
> >+		ret = dev_priv->gt.alloc_request(ring, ring->default_context, &request);
> >  		if (ret)
> >  			goto cleanup_unpin;
> >
> >-		i915_gem_request_assign(&work->flip_queued_req,
> >-					intel_ring_get_request(ring));
> >+		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
> >+						   page_flip_flags);
> >+		if (ret)
> >+			goto cleanup_request;
> >+
> >+		i915_gem_request_assign(&work->flip_queued_req, request);
> >+
> >+		ret = i915_add_request_no_flush(request->ring);
> >+		if (ret)
> >+			goto cleanup_request_assign;
> >  	}
> >
> >  	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
> >@@ -9851,6 +9859,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >
> >  	return 0;
> >
> >+cleanup_request_assign:
> >+	i915_gem_request_assign(&work->flip_queued_req, NULL);
> >+cleanup_request:
> >+	i915_gem_request_unreference(request);
> >+
> >  cleanup_unpin:
> >  	intel_unpin_fb_obj(obj);
> >  cleanup_pending:
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >index efb1729..18d12a5 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >@@ -81,7 +81,7 @@ bool intel_ring_stopped(struct intel_engine_cs *ring)
> >  	return dev_priv->gpu_error.stop_rings & intel_ring_flag(ring);
> >  }
> >
> >-void __intel_ring_advance(struct intel_engine_cs *ring)
> >+static void __intel_ring_advance(struct intel_engine_cs *ring)
> >  {
> >  	struct intel_ringbuffer *ringbuf = ring->buffer;
> >  	ringbuf->tail &= ringbuf->size - 1;
> >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >index c32f5a1..25d5ede 100644
> >--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >@@ -410,7 +410,6 @@ int __intel_ring_space(int head, int tail, int size);
> >  void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
> >  int intel_ring_space(struct intel_ringbuffer *ringbuf);
> >  bool intel_ring_stopped(struct intel_engine_cs *ring);
> >-void __intel_ring_advance(struct intel_engine_cs *ring);
> >
> >  int __must_check intel_ring_idle(struct intel_engine_cs *ring);
> >  void intel_ring_init_seqno(struct intel_engine_cs *ring, u32 seqno);
> >
> 
> Could you perhaps make a small note in the commit message to clarify why you
> remove __intel_ring_advance everywhere? After discussing it with you I
> understood that it's because we already do the corresponding operation in
> i915_add_request_no_flush that is added to intel_crtc_page_flip but since
> these pieces of code are so far from each other it's not entirely obvious
> from the start that they have something to do with each other.
> 
> Or am I the only one who thinks this is not trivial?

I concur, the commit message should explain this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
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