Re: [PATCH 10/12] drm/i915: extract some common olr+wedge code

[Ben Widawsky <ben@bwidawsk.net>]

On Fri, 27 Apr 2012 09:46:31 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Thu, 26 Apr 2012 16:03:07 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > +/*
> > + * Compare seqno against outstanding lazy request. Emit a request
> > if they are
> > + * equal. Seqno is updated with the new value if a request was
> > emitted.
> > + */
> > +static int
> > +i915_gem_check_olr(struct intel_ring_buffer *ring, u32 *seqno)
> > +{
> > +	int ret = 0;
> > +
> > +	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> > +
> > +	if (*seqno == ring->outstanding_lazy_request) {
> > +		struct drm_i915_gem_request *request;
> > +
> > +		request = kzalloc(sizeof(*request), GFP_KERNEL);
> > +		if (request == NULL)
> > +			return -ENOMEM;
> > +
> > +		ret = i915_add_request(ring, NULL, request);
> > +		if (ret) {
> > +			kfree(request);
> > +			return ret;
> > +		}
> > +
> > +		*seqno = request->seqno;
> I'd love for this to be BUG_ON(seqno != request->seqno) and so drop
> the out parameter, as it would tidy all the callers up.
> > +	}
> > +
> > +	return ret;
> -Chris
> 

Got it, thanks.

I hesitated to do this initially because of the logic in
i915_gem_object_sync. In that function, the updated seqno is used by the
ring sync code, and without the outparam, there was no way to not modify
the behavior. However after inspecting the code a bit deeper, I
understand why it's not a functional change, and the BUG_ON asserts
that.

The one bit which is still not clear to me, even after IRC, is why we
need to add the olr request at this point at all. Even if the mbox update
is delayed for a bit, I don't understand why things won't work
correctly; though I've confirmed I do get a hangcheck if I remove the
bit of code.

Comments?