Re: [PATCH v6 12/25] drm/i915: Unify intel_ring_begin()

Chris Wilson
2016-04-27T09:37:51Z

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