On Wed, Apr 27, 2016 at 12:21:52PM +0300, Joonas Lahtinen wrote:
> On ti, 2016-04-26 at 21:06 +0100, Chris Wilson wrote:
> > - ret = intel_logical_ring_begin(req, 2 + 2 * GEN9_NUM_MOCS_ENTRIES);
> > - if (ret) {
> > - DRM_DEBUG("intel_logical_ring_begin failed %d\n", ret);
>
> These debugs disappear in silence. Could be worthy a note in the commit
> message.
"Remove useless debugging following WARN in case of real error." ?
> > void intel_ring_reserved_space_cancel(struct intel_ringbuffer *ringbuf)
> > {
> > - WARN_ON(ringbuf->reserved_in_use);
> > -
> > + GEM_BUG_ON(!ringbuf->reserved_size);
> > ringbuf->reserved_size = 0;
> > - ringbuf->reserved_in_use = false;
> > }
> >
> > void intel_ring_reserved_space_use(struct intel_ringbuffer *ringbuf)
> > {
> > - WARN_ON(ringbuf->reserved_in_use);
> > -
> > - ringbuf->reserved_in_use = true;
> > - ringbuf->reserved_tail = ringbuf->tail;
> > + GEM_BUG_ON(!ringbuf->reserved_size);
> > + ringbuf->reserved_size = 0;
> > }
> >
>
> Above two functions are now the same, I'd remove the _cancel variant.
I thought semantically they were different until the next patch. Moving
reserved onto the request, makes it moot as the cancellation is just
deleting the incomplete request.
> > void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf)
> > {
> > - WARN_ON(!ringbuf->reserved_in_use);
> > - if (ringbuf->tail > ringbuf->reserved_tail) {
> > - WARN(ringbuf->tail > ringbuf->reserved_tail + ringbuf->reserved_size,
> > - "request reserved size too small: %d vs %d!\n",
> > - ringbuf->tail - ringbuf->reserved_tail, ringbuf->reserved_size);
> > - } else {
> > + GEM_BUG_ON(ringbuf->reserved_size);
> > +}
> > +
> > +static int wait_for_space(struct drm_i915_gem_request *req, int bytes)
> > +{
> > + struct intel_ringbuffer *ringbuf = req->ringbuf;
> > + struct intel_engine_cs *engine = req->engine;
> > + struct drm_i915_gem_request *target;
> > +
> > + intel_ring_update_space(ringbuf);
> > + if (ringbuf->space >= bytes)
> > + return 0;
> > +
> > + /* The whole point of reserving space is to not wait! */
> > + GEM_BUG_ON(!ringbuf->reserved_size);
>
> reserved_size = 0 would indicate we're at __i915_add_request, but the
> comment and test are not clearest. reserved_size being zero does not
> directly indicate "aha, reserved bytes are being used", it could very
> well be no reserved_size was requested. But right.
/* The whole point of reserving space before the request was not to wait! */
doesn't fit :|
/* Space is reserved in the ringbuffer for finalising the request,
* as that cannot be allowed to fail. During request finalisation,
* reserved_space is set to 0 to stop the overallocation and the
* assumption is that then we never need to wait (and so risk
* EINTR).
*/
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
