On Mon, 19 Sep 2016, Joonas Lahtinen <joonas.lahti...@linux.intel.com> wrote:
> 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(...)).

*shudder* it doesn't get any better by removing unlikely!

BR,
Jani.

>
>> +    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.lahti...@linux.intel.com>
>
> Regards, Joonas

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to