Re: [PATCH 30/59] drm/i915: Update queue_flip() to take a request structure

[Tomas Elf <tomas.elf@intel.com>]

On 19/03/2015 12:30, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
>
> 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.
>
> Note that every single flip function used to finish with
> '__intel_ring_advance(ring);'. However, immediately after they return there is
> now an add request call which will do the advance anyway. Thus the many
> duplicate advance calls have been removed.
>
> v2: Updated commit message with comment about advance removal.
>
> v3: The request can now be allocated by the _sync() code earlier on. Thus the
> page flip path does not necessarily need to allocate a new request, it may be
> able to re-use one.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         |    2 +-
>   drivers/gpu/drm/i915/intel_display.c    |   33 ++++++++++++++++++-------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c |    2 +-
>   drivers/gpu/drm/i915/intel_ringbuffer.h |    1 -
>   4 files changed, 21 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 55d82a7..1fd59bc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -574,7 +574,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 c71c523..c39cd34 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9418,9 +9418,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;
> @@ -9445,7 +9446,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;
>   }
>
> @@ -9453,9 +9453,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;
> @@ -9477,7 +9478,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;
>   }
>
> @@ -9485,9 +9485,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;
> @@ -9516,7 +9517,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;
>   }
>
> @@ -9524,9 +9524,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;
> @@ -9552,7 +9553,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;
>   }
>
> @@ -9560,9 +9560,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;
> @@ -9647,7 +9648,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;
>   }
>
> @@ -9803,7 +9803,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;
> @@ -9997,13 +9997,18 @@ 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,
> +		if (!request) {
> +			ret = i915_gem_request_alloc(ring, ring->default_context, &request);
> +			if (ret)
> +				goto cleanup_unpin;
> +		}
> +
> +		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
>   						   page_flip_flags);
>   		if (ret)
>   			goto cleanup_unpin;
>
> -		i915_gem_request_assign(&work->flip_queued_req,
> -					intel_ring_get_request(ring));
> +		i915_gem_request_assign(&work->flip_queued_req, request);
>   	}
>
>   	if (request)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 291920e..8a55032 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 2dc111c..5fac101 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -412,7 +412,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);
>

Reviewed-by: Tomas Elf <tomas.elf@intel.com>

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