On Mon, 19 Sep 2016, Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> 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.lahtinen@xxxxxxxxxxxxxxx>
>
> Regards, Joonas
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
