[Intel-gfx] [PATCH v6 12/25] drm/i915: Unify intel_ring_begin()

[Chris Wilson]

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