[Intel-gfx] [PATCH] drm/i915: extract object unplug code from busy/wait_timeout ioctl

[Daniel Vetter]

On Wed, May 30, 2012 at 01:41:28PM -0700, Ben Widawsky wrote:
> On Wed, 30 May 2012 20:21:33 +0200
> Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> 
> > Both need to do the same dance (or at least should). Some slight
> > changes:
> > - busy_ioctl now unconditionally checks for olr. Before emitting a
> >   require flush would have prevent the olr check and hence required a
> >   second call to the busy ioctl to really emit the request.
> > - the timeout wait now also retires request. Not really required for
> >   abi-reasons, but makes a notch more sense imo.
> > 
> 
> The one thing I dislike about the retiring behavior is it is still not
> guaranteed. After some coffee, I've remembered a bit about this.  Since
> we do an non-blocking wait, it's possible for userspace to race against
> itself and rebusy the object. Therefore, the client should never assume
> the call to wait_ioctl is the same as busy. (Busy has a similar, shorter
> race; but also has a little smarter semantics). Therefore, I don't think
> this buys us much, but I also don't think it hurts. So I'd vote on
> letting Eric decide if he wants this or not.

Well, as I've said in the commit message, we don't need the retire_request
in the wait_ioctl for abi reasons - it doesn't change anything.

But if you use the wait_ioctl as a poll (with timeout=0) we will never
transition the object out of the active state (well, as long as the retire
work doesn't run at least), so we'll always go to through the __wait_seqno
call to notice that the seqno passed already. Purely academic optimization
because such a usage-pattern would be nuts, but it nicely unifies how we
handle things in the busy ioctl (which I like as a cleanup).

> > I've tested this by pimping the i-g-t test some more and also checking
> > the polling behviour of the wait_rendering_timeout ioctl versus what
> > busy_ioctl returns.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |   61 ++++++++++++++++++--------------------
> >  1 files changed, 29 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d2eaa00..521e294 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2000,6 +2000,31 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj)
> >  }
> >  
> >  /**
> > + * Ensures that an object will eventually get non-busy by flushing any required
> > + * write domains, emitting any outstanding lazy request and retiring and
> > + * completed requests. The unplug moniker is stolen from the linux block layer.
> > + */
> I'd prefer something like, "unbusy" but whatever.

Considered and I've thought that's not a proper word. And unbusy isn't
quite correct either, because this only ensures that the object will get
unbusy eventually, if you keep on calling this function (due to the
retire_request in there). And _eventually_unbusy_object sounds horrible to
me. I admit that unplug is a misdenomer, too, but I lack good ideas.

> > +static int
> > +i915_gem_unplug_object(struct drm_i915_gem_object *obj)
> > +{
> > +	int ret;
> > +
> > +	if (obj->active) {
> > +		ret = i915_gem_object_flush_gpu_write_domain(obj);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = i915_gem_check_olr(obj->ring,
> > +					 obj->last_rendering_seqno);
> > +		if (ret)
> > +			return ret;
> > +		i915_gem_retire_requests_ring(obj->ring);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> >   * i915_gem_wait_ioctl - implements DRM_IOCTL_I915_GEM_WAIT
> >   * @DRM_IOCTL_ARGS: standard ioctl arguments
> >   *
> > @@ -2043,11 +2068,8 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		return -ENOENT;
> >  	}
> >  
> > -	/* Need to make sure the object is flushed first. This non-obvious
> > -	 * flush is required to enforce that (active && !olr) == no wait
> > -	 * necessary.
> > -	 */
> > -	ret = i915_gem_object_flush_gpu_write_domain(obj);
> > +	/* Need to make sure the object gets un-active eventually. */
> > +	ret = i915_gem_unplug_object(obj);
> >  	if (ret)
> >  		goto out;
> 
> I can't remember the reason offhand, but I vaguely recall Chris demanded
> we flush the write domain even if the object isn't active. Perhaps you
> can look up the email, or he can chime in. I forget the exact reason.

If an object has non-zero write_domain, we keep it on the flushing_list
and also keep the object active. If that's not the case somewhere, we have
a bug. So the flush is required to eventually unbusy the object.

> > @@ -2059,10 +2081,6 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  	if (seqno == 0)
> >  		 goto out;
> >  
> > -	ret = i915_gem_check_olr(ring, seqno);
> > -	if (ret)
> > -		goto out;
> > -
> >  	/* Do this after OLR check to make sure we make forward progress polling
> >  	 * on this IOCTL with a 0 timeout (like busy ioctl)
> >  	 */
> > @@ -3302,30 +3320,9 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
> >  	 * become non-busy without any further actions, therefore emit any
> >  	 * necessary flushes here.
> >  	 */
> > -	args->busy = obj->active;
> > -	if (args->busy) {
> > -		/* Unconditionally flush objects, even when the gpu still uses this
> > -		 * object. Userspace calling this function indicates that it wants to
> > -		 * use this buffer rather sooner than later, so issuing the required
> > -		 * flush earlier is beneficial.
> > -		 */
> > -		if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
> > -			ret = i915_gem_flush_ring(obj->ring,
> > -						  0, obj->base.write_domain);
> > -		} else {
> > -			ret = i915_gem_check_olr(obj->ring,
> > -						 obj->last_rendering_seqno);
> > -		}
> > +	ret = i915_gem_unplug_object(obj);
> >  
> > -		/* Update the active list for the hardware's current position.
> > -		 * Otherwise this only updates on a delayed timer or when irqs
> > -		 * are actually unmasked, and our working set ends up being
> > -		 * larger than required.
> > -		 */
> > -		i915_gem_retire_requests_ring(obj->ring);
> > -
> > -		args->busy = obj->active;
> > -	}
> > +	args->busy = obj->active;
> >  
> >  	drm_gem_object_unreference(&obj->base);
> >  unlock:
> 
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48