On Fri, Oct 10, 2014 at 12:41:33PM +0100, John.C.Harrison@xxxxxxxxx wrote:
> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>
> For: VIZ-4377
> Signed-off-by: John.C.Harrison@xxxxxxxxx
Now I'm confused ... patch 3 made it sound like having the request and the
seqno allocated at different points is a really fragile idea? Or is this
now all save with everyone using struct request? Please elaborate.
I think the idea is solid, since with the scheduler we'll probably want to
allocate the seqno even later (to avoid having to deal with out-of-order
seqnos).
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 5 ++++-
> drivers/gpu/drm/i915/i915_gem.c | 28 +++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_lrc.c | 10 ++++------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++------
> 4 files changed, 39 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e46c78c..d797975 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1979,6 +1979,9 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
> if (req->complete)
> return true;
>
> + if (req->seqno == 0)
> + return false;
> +
> i915_gem_complete_requests_ring(req->ring, lazy_coherency);
>
> return req->complete;
> @@ -2482,7 +2485,7 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
> return (int32_t)(seq1 - seq2) >= 0;
> }
>
> -int __must_check i915_gem_get_seqno(struct drm_device *dev, u32 *seqno);
> +int __must_check i915_gem_prepare_next_seqno(struct drm_device *dev);
> int __must_check i915_gem_set_seqno(struct drm_device *dev, u32 seqno);
> int __must_check i915_gem_object_get_fence(struct drm_i915_gem_object *obj);
> int __must_check i915_gem_object_put_fence(struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 260ef47..7db84b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2310,12 +2310,15 @@ int i915_gem_set_seqno(struct drm_device *dev, u32 seqno)
> }
>
> int
> -i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> +i915_gem_prepare_next_seqno(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> /* reserve 0 for non-seqno */
> if (dev_priv->next_seqno == 0) {
> + /* Why is the full re-initialisation required? Is it only for
> + * hardware semaphores? If so, could skip it in the case where
> + * semaphores are disabled? */
> int ret = i915_gem_init_seqno(dev, 0);
> if (ret)
> return ret;
> @@ -2323,6 +2326,24 @@ i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> dev_priv->next_seqno = 1;
> }
>
> + return 0;
> +}
> +
> +static int
> +i915_gem_get_seqno(struct drm_device *dev, u32 *seqno)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + /* reserve 0 for non-seqno */
> + if (dev_priv->next_seqno == 0) {
> + /* Should never get here! Must always call 'prepare_next' in
> + * advance. This code is called during request submission.
> + * Trying to wrap the seqno and the implicit idle() calls that
> + * the wrap code makes are a bad idea at this point! */
> + DRM_ERROR("Need to wrap seqno at inopportune moment!\n");
> + return -EBUSY;
> + }
> +
> *seqno = dev_priv->last_seqno = dev_priv->next_seqno++;
> return 0;
> }
> @@ -2366,6 +2387,11 @@ int __i915_add_request(struct intel_engine_cs *ring,
> return ret;
> }
>
> + /* Assign an identifier to track this request through the hardware: */
> + ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> + if (ret)
> + return ret;
> +
> /* Record the position of the start of the request so that
> * should we detect the updated seqno part-way through the
> * GPU processing the request, we never over-estimate the
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5a75eb5..e7d4d20 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -802,6 +802,10 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
> if (ring->outstanding_lazy_request)
> return 0;
>
> + ret = i915_gem_prepare_next_seqno(ring->dev);
> + if (ret)
> + return ret;
> +
> request = kzalloc(sizeof(*request), GFP_KERNEL);
> if (request == NULL)
> return -ENOMEM;
> @@ -809,12 +813,6 @@ static int logical_ring_alloc_request(struct intel_engine_cs *ring,
> kref_init(&request->ref);
> request->ring = ring;
>
> - ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> - if (ret) {
> - kfree(request);
> - return ret;
> - }
> -
> /* Hold a reference to the context this request belongs to
> * (we will need it when the time comes to emit/retire the
> * request).
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0f2719d..6a2f25d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2017,6 +2017,10 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
> if (ring->outstanding_lazy_request)
> return 0;
>
> + ret = i915_gem_prepare_next_seqno(ring->dev);
> + if (ret)
> + return ret;
> +
> request = kzalloc(sizeof(*request), GFP_KERNEL);
> if (request == NULL)
> return -ENOMEM;
> @@ -2024,12 +2028,6 @@ intel_ring_alloc_request(struct intel_engine_cs *ring)
> kref_init(&request->ref);
> request->ring = ring;
>
> - ret = i915_gem_get_seqno(ring->dev, &request->seqno);
> - if (ret) {
> - kfree(request);
> - return ret;
> - }
> -
> ring->outstanding_lazy_request = request;
> return 0;
> }
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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
