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
