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
