On Fri, May 29, 2015 at 05:44:09PM +0100, John.C.Harrison@xxxxxxxxx wrote: > From: John Harrison <John.C.Harrison@xxxxxxxxx> > > 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. > > v2: Renamed functions according to review feedback (Tomas Elf). > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx> Still has my question open from the previos round: http://mid.gmane.org/20150323091030.GL1349@phenom.ffwll.local Note that this isn't all that unlikely with GuC mode since there the ringbuffer is substantially smaller (due to firmware limitations) than what we allocate ourselves right now. -Daniel > --- > drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++++++------------ > drivers/gpu/drm/i915/intel_lrc.c | 15 +++++++++++++++ > drivers/gpu/drm/i915/intel_lrc.h | 1 + > drivers/gpu/drm/i915/intel_ringbuffer.c | 29 ++++++++++++++++------------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 5 files changed, 46 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 9f3e0717..1261792 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2680,19 +2680,20 @@ 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. > */ > - intel_ring_reserved_space_reserve(req->ringbuf, MIN_SPACE_FOR_ADD_REQUEST); > + if (i915.enable_execlists) > + ret = intel_logical_ring_reserve_space(req); > + else > + ret = intel_ring_reserve_space(req); > + if (ret) { > + /* > + * At this point, the request is fully allocated even if not > + * fully prepared. Thus it can be cleaned up using the proper > + * free code. > + */ > + i915_gem_request_cancel(req); > + return ret; > + } > > *req_out = ring->outstanding_lazy_request = req; > return 0; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 548c53d..e164ac0 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -823,6 +823,21 @@ static int intel_logical_ring_begin(struct drm_i915_gem_request *req, > return 0; > } > > +int intel_logical_ring_reserve_space(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..f59940a 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 intel_logical_ring_reserve_space(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 bb10fc2..0ba5787 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2192,24 +2192,27 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request) > return 0; > } > > +int intel_ring_reserve_space(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_ring_begin(request, 0); > +} > + > void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size) > { > - /* 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);*/ > + 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! This will be fixed later but for now just > - * return and hope for the best. Note that there is only a real > - * problem if the create of the request never actually calls _begin() > - * but if they are not submitting any work then why did they create > - * the request in the first place? > - */ > } > > void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf) > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 16fd9ba..f4633ca 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -450,6 +450,7 @@ intel_ring_get_request(struct intel_engine_cs *ring) > > #define MIN_SPACE_FOR_ADD_REQUEST 128 > > +int intel_ring_reserve_space(struct drm_i915_gem_request *request); > void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int size); > void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf); > void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf); > -- > 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 http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
