| Message ID | 1426768264-16996-52-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 19, 2015 at 12:30:56PM +0000, John.C.Harrison@Intel.com wrote: > 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) > { > - /* 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); This feels a bit convoluted tbh, and would fall aparat if we start adding sanity checks to _begin/advance functions. Can't we instead directly call ring_wait_for_space? This forgoes the intel_wrap_ring_buffer call, but otoh we just need to factor that into our estimates. Wrapping the ring for the entire reservartion right away is a) way too much - we only wrap individual ring_being calls anyway b) not doing any good since all the intermediate intel_ring_emit calls might very-well push us into a wrap anyway. In the end we just need to increase our reservation with the biggest intel_ring_begin we have in the add_request code - that's the worst-case of ring space we might waste due to wrapping. Same for the lrc path ofc. -Daniel
On Fri, Mar 20, 2015 at 04:23:45PM +0100, Daniel Vetter wrote: > On Thu, Mar 19, 2015 at 12:30:56PM +0000, John.C.Harrison@Intel.com wrote: > > 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) > > { > > - /* 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); > > This feels a bit convoluted tbh, and would fall aparat if we start adding > sanity checks to _begin/advance functions. Can't we instead directly call > ring_wait_for_space? This forgoes the intel_wrap_ring_buffer call, but > otoh we just need to factor that into our estimates. Wrapping the ring for > the entire reservartion right away is > a) way too much - we only wrap individual ring_being calls anyway > b) not doing any good since all the intermediate intel_ring_emit calls > might very-well push us into a wrap anyway. > > In the end we just need to increase our reservation with the biggest > intel_ring_begin we have in the add_request code - that's the worst-case > of ring space we might waste due to wrapping. Sorry to tune in here but what? What happened to the transactional request model? -Chris
On 20/03/2015 15:30, Chris Wilson wrote: > On Fri, Mar 20, 2015 at 04:23:45PM +0100, Daniel Vetter wrote: >> On Thu, Mar 19, 2015 at 12:30:56PM +0000, John.C.Harrison@Intel.com wrote: >>> 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) >>> { >>> - /* 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); >> This feels a bit convoluted tbh, and would fall aparat if we start adding >> sanity checks to _begin/advance functions. Can't we instead directly call >> ring_wait_for_space? This forgoes the intel_wrap_ring_buffer call, but >> otoh we just need to factor that into our estimates. Wrapping the ring for >> the entire reservartion right away is >> a) way too much - we only wrap individual ring_being calls anyway >> b) not doing any good since all the intermediate intel_ring_emit calls >> might very-well push us into a wrap anyway. >> >> In the end we just need to increase our reservation with the biggest >> intel_ring_begin we have in the add_request code - that's the worst-case >> of ring space we might waste due to wrapping. I don't see why sanity checks in begin/advance would be a problem. This _begin() call is only required in the situation where no-one actually adds any commands to the ring before calling i915_add_request() and then is only necessary because i915_add_request() is not allowed to call _begin() any more because it is not allowed to fail. There is no magic going on behind the scenes. It is exactly the same as calling '_begin(RESERVED_SIZE)'. All subsequent code must still do it's own _begin() call before adding commands to the ring. Any sanity checks would ignore the reserved size and complain if a _begin() is too small for the commands actually written. It should all just work :). Plus it is nice and simple and easy to maintain. Also, I don't see why it is better to do the wrap as late as possible. If a wrap is going to be required at some point then it doesn't really matter when it is done - early or late, the (small) hit still has to be taken. Given that the buffer size is orders of magnitude larger than the size of an individual request, there is no possibility of ever having to wrap twice. So I would say that keeping the code nice and simple outweighs any advantage to wrapping a few calls later. > Sorry to tune in here but what? What happened to the transactional > request model? > -Chris Er, what transactional request model? There were comments a while ago about a scheme of not updating the hardware until the add_request() call such that the entire request could be discarded by simply resetting the ring pointers to how they were at the point of the alloc_request() call. I don't know if anything ever came out of that discussion though. I certainly didn't see it as a 'we must implement this before we do anything else'. And I'm just trying to do the simplest/quickest route to getting the GPU scheduler upstreamed! John.
On Fri, Mar 20, 2015 at 04:09:55PM +0000, John Harrison wrote: > On 20/03/2015 15:30, Chris Wilson wrote: > >On Fri, Mar 20, 2015 at 04:23:45PM +0100, Daniel Vetter wrote: > >>On Thu, Mar 19, 2015 at 12:30:56PM +0000, John.C.Harrison@Intel.com wrote: > >>>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) > >>> { > >>>- /* 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); > >>This feels a bit convoluted tbh, and would fall aparat if we start adding > >>sanity checks to _begin/advance functions. Can't we instead directly call > >>ring_wait_for_space? This forgoes the intel_wrap_ring_buffer call, but > >>otoh we just need to factor that into our estimates. Wrapping the ring for > >>the entire reservartion right away is > >>a) way too much - we only wrap individual ring_being calls anyway > >>b) not doing any good since all the intermediate intel_ring_emit calls > >>might very-well push us into a wrap anyway. > >> > >>In the end we just need to increase our reservation with the biggest > >>intel_ring_begin we have in the add_request code - that's the worst-case > >>of ring space we might waste due to wrapping. > > I don't see why sanity checks in begin/advance would be a problem. This > _begin() call is only required in the situation where no-one actually adds > any commands to the ring before calling i915_add_request() and then is only > necessary because i915_add_request() is not allowed to call _begin() any > more because it is not allowed to fail. There is no magic going on behind > the scenes. It is exactly the same as calling '_begin(RESERVED_SIZE)'. All > subsequent code must still do it's own _begin() call before adding commands > to the ring. Any sanity checks would ignore the reserved size and complain > if a _begin() is too small for the commands actually written. It should all > just work :). Plus it is nice and simple and easy to maintain. > > Also, I don't see why it is better to do the wrap as late as possible. If a > wrap is going to be required at some point then it doesn't really matter > when it is done - early or late, the (small) hit still has to be taken. > Given that the buffer size is orders of magnitude larger than the size of an > individual request, there is no possibility of ever having to wrap twice. So > I would say that keeping the code nice and simple outweighs any advantage to > wrapping a few calls later. Ok two examples: 1. We have 100 bytes add request tail that we reserve, but only 80 bytes left before wrapping. Your reservation code will wrap. Let's also assume we have 160 bytes of execbuf stuff in 20bytes block. If we would not have wrapped right away we'd save those 80 bytes of wrapping. Eager wrapping just wasted space. 2. Again 100 bytes of reservation, 160 bytes of execbuf. But this time around assume we have 200 bytes left before wrap. Your reservation doesn't wrap since there's still 100 bytes left. But after the execbuf there's only 40 bytes left, requiring a wrap. Eager wrapping didn't help at all for correct reservation. Which means we _have_ to include the worst-case loss for wrapping into our reservation, and more important the reservation check _must_ handle wrapping correctly. Since that's the case where things will fall appart if the reservation is just a bit too small. Hence my recommendation to only reserve the space and not bother with wrapping (it's a net loss) and also make sure we catch any kind of overflow due to wrapping. We could instead try to keep wrapping into account for the reserved space, but I think that's more fragile. It's also more wasteful since generally we don't need a 100 byte contiguous block but just a few dwords for each individual command. So effective wrapping overhead is a lot less than 100-4. Finally imo shrugging the double-wrap off isn't a good idea since emitting a lot of crap by accident is a very likely bug. And with the guc scheduling mode I've heard that the rings will be just 4k ... -Daniel
On 19/03/2015 12:30, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@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@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); > >
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) { - /* 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);