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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx