[Intel-gfx] [PATCH 51/59] drm/i915: Add *_ring_begin() to request allocation
Tomas Elf
tomas.elf at intel.com
Tue Mar 31 10:17:50 PDT 2015
On 19/03/2015 12:30, John.C.Harrison at Intel.com wrote:
> From: John Harrison <John.C.Harrison at Intel.com>
>
> Now that the *_ring_begin() functions no longer call the request allocation
> code, it is finally safe for the request allocation code to call *_ring_begin().
> This is important to guarantee that the space reserved for the subsequent
> i915_add_request() call does actually get reserved.
This commit message could go into more detail regarding the fact that
we've split the reserved_space_reserve function into two specific
functions depending on submission mode.
>
> For: VIZ-5115
> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 16 ++++------------
> drivers/gpu/drm/i915/intel_lrc.c | 15 +++++++++++++++
> drivers/gpu/drm/i915/intel_lrc.h | 1 +
> drivers/gpu/drm/i915/intel_ringbuffer.c | 28 +++++++++++++++++-----------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ++-
> 5 files changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b047693..fe2de21 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2561,19 +2561,11 @@ int i915_gem_request_alloc(struct intel_engine_cs *ring,
> * i915_add_request() call can't fail. Note that the reserve may need
> * to be redone if the request is not actually submitted straight
> * away, e.g. because a GPU scheduler has deferred it.
> - *
> - * Note further that this call merely notes the reserve request. A
> - * subsequent call to *_ring_begin() is required to actually ensure
> - * that the reservation is available. Without the begin, if the
> - * request creator immediately submitted the request without adding
> - * any commands to it then there might not actually be sufficient
> - * room for the submission commands. Unfortunately, the current
> - * *_ring_begin() implementations potentially call back here to
> - * i915_gem_request_alloc(). Thus calling _begin() here would lead to
> - * infinite recursion! Until that back call path is removed, it is
> - * necessary to do a manual _begin() outside.
> */
> - ret = intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> + if (i915.enable_execlists)
> + ret = logical_ring_reserved_space_reserve(request);
> + else
> + ret = legacy_ring_reserved_space_reserve(request);
> if (ret) {
> /*
> * At this point, the request is fully allocated even if not
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c16d726..8cb34c6 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -829,6 +829,21 @@ static int intel_logical_ring_begin(struct drm_i915_gem_request *req,
> return 0;
> }
>
> +int logical_ring_reserved_space_reserve(struct drm_i915_gem_request *request)
> +{
> + /*
> + * The first call merely notes the reserve request and is common for
> + * all back ends. The subsequent localised _begin() call actually
> + * ensures that the reservation is available. Without the begin, if
> + * the request creator immediately submitted the request without
> + * adding any commands to it then there might not actually be
> + * sufficient room for the submission commands.
> + */
> + intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> +
> + return intel_logical_ring_begin(request, 0);
> +}
> +
> /**
> * execlists_submission() - submit a batchbuffer for execution, Execlists style
> * @dev: DRM device.
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 044c0e5..905a83e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -37,6 +37,7 @@
>
> /* Logical Rings */
> int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request);
> +int logical_ring_reserved_space_reserve(struct drm_i915_gem_request *request);
> void intel_logical_ring_stop(struct intel_engine_cs *ring);
> void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
> int intel_logical_rings_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6f198df..c7dcabd 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2205,21 +2205,27 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request)
> return 0;
> }
>
> -int intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> +int legacy_ring_reserved_space_reserve(struct drm_i915_gem_request *request)
I'm not completely convinced of this naming convention. In the past when
we've had to separate implementations based on legacy submission from
implementations based on execlist submission we've followed the
convention of intel_ring_* and intel_logical_ring_*. I realise that it
might not have been very pretty to make this split and I realise that
the reason we ended up with that convention was simply because we
already had a bunch of (to be) legacy functions and we added the
_logical_ring_* ones on top of it so that the legacy was implied and not
explicitly stated in their respective names. However, now we have that
convention so it would be nice if we could follow it at least until the
day comes when we can remove all the legacy/execlists duplication or
whatever we do in the end.
Could someone more familiar with i915 history tell me how this has been
dealt with in the past when there's been legacy functions that have had
to be separated from other flavors of the same functions? I cannot find
a whole lot of "*_legacy_*" functions in the driver so I assume this is
not a previously used naming convention. I'm open for other opinions on
this matter that perhaps take i915 history into account. If there's
historical precedence I'm willing to opt for it but personally I just
feel like it's bad form to name something legacy_* since you might up
end up with a situation where you have a lot of functions called
legacy_* that have nothing to do with each other aside from the fact
that they are legacy in relation to something else. The fact that they
are named legacy_* could imply that they are somehow related (like
intel_execlists_* functions are all related in the way that they are
all part of the execlist implementation).
Thanks,
Tomas
> {
> - /* NB: Until request management is fully tidied up and the OLR is
> - * removed, there are too many ways for get false hits on this
> - * anti-recursion check! */
> - /*WARN_ON(ringbuf->reserved_size);*/
> + /*
> + * The first call merely notes the reserve request and is common for
> + * all back ends. The subsequent localised _begin() call actually
> + * ensures that the reservation is available. Without the begin, if
> + * the request creator immediately submitted the request without
> + * adding any commands to it then there might not actually be
> + * sufficient room for the submission commands.
> + */
> + intel_ring_reserved_space_reserve(request->ringbuf, MIN_SPACE_FOR_ADD_REQUEST);
> +
> + return intel_ring_begin(request, 0);
> +}
> +
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size)
> +{
> + WARN_ON(ringbuf->reserved_size);
> WARN_ON(ringbuf->reserved_in_use);
>
> ringbuf->reserved_size = size;
> -
> - /*
> - * Really need to call _begin() here but that currently leads to
> - * recursion problems! So just return zero and hope for the best...
> - */
> - return 0;
> }
>
> void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf, int size)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index f6ab6bb..365b98d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -442,7 +442,8 @@ intel_ring_get_request(struct intel_engine_cs *ring)
>
> #define MIN_SPACE_FOR_ADD_REQUEST 128
>
> -int intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
> +int legacy_ring_reserved_space_reserve(struct drm_i915_gem_request *request);
> +void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size);
> void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf, int size);
> void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
>
>
More information about the Intel-gfx
mailing list