On ke, 2016-04-27 at 10:37 +0100, Chris Wilson wrote:
> 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." ?
Sounds ok.
>
> >
> > >
> > > 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).
> */
Looks good too.
Regards, Joonas
> -Chris
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
