Re: [PATCH 2/4] drm/i915: fixup seqno allocation logic for lazy_request

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 25 Jan 2012 14:03:58 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Currently we reserve seqnos only when we emit the request to the ring
> (by bumping dev_priv->next_seqno), but start using it much earlier for
> ring->oustanding_lazy_request. When 2 threads compete for the gpu and
> run on two different rings (e.g. ddx on blitter vs. compositor)
> hilarity ensued, especially when we get constantly interrupted while
> reserving buffers.
> 
> Breakage seems to have been introduced in
> 
> commit 6f392d548658a17600da7faaf8a5df25ee5f01f6
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Sat Aug 7 11:01:22 2010 +0100
> 
>     drm/i915: Use a common seqno for all rings.
> 
> This patch fixes up the seqno reservation logic by moving it into
> i915_gem_next_request_seqno. The ring->add_request functions now
> superflously still return the new seqno through a pointer, that will
> be refactored in the next patch.
> 
> v2: Keep i915_gem_get_seqno (but move it to i915_gem.c) to make it
> clear that we only have one seqno counter for all rings. Suggested by
> Chris Wilson.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=45181
> Tested-by: Nicolas Kalkhof nkalkhof()at()web.de
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  static int
>  render_ring_flush(struct intel_ring_buffer *ring,
>  		  u32	invalidate_domains,
> @@ -467,7 +453,7 @@ gen6_add_request(struct intel_ring_buffer *ring,
>  	mbox1_reg = ring->signal_mbox[0];
>  	mbox2_reg = ring->signal_mbox[1];
>  
> -	*seqno = i915_gem_get_seqno(ring->dev);
> +	*seqno = ring->outstanding_lazy_request;

In discussing this patch with Daniel, I made the mistake of reading that
as i915_gem_get_next_request_seqno() instead of get_seqno(). I'd suggest
the patch makes that change and hide the ugly ring->o_l_r. Then since we
do i915_gem_get_next_request_seqno() both here and in the caller, it
becomes much clearer that we are able to remove it.

Daniel, apologies for the confusion!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre