Re: [PATCH 15/18] drm/i915: Reserve space in the global seqno during request allocation

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

On ke, 2016-09-14 at 07:52 +0100, Chris Wilson wrote:
> +static int reserve_global_seqno(struct drm_i915_private *i915)
>  {
> -	struct i915_gem_timeline *tl = &dev_priv->gt.global_timeline;
> +	struct i915_gem_timeline *tl = &i915->gt.global_timeline;
> +	u32 next_seqno = atomic_read(&tl->next_seqno);
>  
> -	/* reserve 0 for non-seqno */
> -	if (unlikely(tl->next_seqno == 0)) {

Meh, do not hide the ++i915->gt.active_requests in if (unlikely(...)).

> +	if (unlikely(next_seqno + ++i915->gt.active_requests <= next_seqno)) {
>  		int ret;
>  

Why not if (likely(next_seqno + active_requests > next_seqno))
		return 0;

> -		ret = i915_gem_init_global_seqno(dev_priv, 0);
> -		if (ret)
> +		ret = i915_gem_init_global_seqno(i915, 0);
> +		if (ret) {
> +			i915->gt.active_requests--;
>  			return ret;

With above change the built-in teardown becomes OK. Otherwise split.

> -
> -		tl->next_seqno = 1;
> +		}
>  	}
>  
> -	*seqno = tl->next_seqno++;
>  	return 0;
>  }
>  
> +static u32 timeline_get_seqno(struct i915_gem_timeline *tl)
> +{
> +	return atomic_inc_return(&tl->next_seqno);
> +}
> +

How about u32 timeline_peek_seqno(), as you have quite a few
atomic_reads on it. Also, lockdep_assert_held would be useful sprinkled
ni the functions.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
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