[PATCH] sna: avoid negative timeouts

[Felipe Contreras]

On Thu, Oct 3, 2013 at 3:17 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Thu, Oct 03, 2013 at 03:06:03AM -0500, Felipe Contreras wrote:
>> On Thu, Oct 3, 2013 at 2:36 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
>> > On Thu, Oct 03, 2013 at 01:29:29AM -0500, Felipe Contreras wrote:
>> >> It's nice to avoid X server crashes (by not passing negative values to
>> >> select(3)).
>> >>
>> >> For more information:
>> >> http://article.gmane.org/gmane.comp.freedesktop.xorg.devel/37388
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras at gmail.com>
>> >
>> > Thanks for the patch, I pushed a slightly different version to consume
>> > the expired flush timeout immediately.
>> >
>> > However, in order for this to happen something within the BlockHandler
>> > must have run for at least 4ms. Which is itself very worrying - the two
>> > candidates are the throttle or lock contention. (The throttle is also
>> > prone to being influenced by a third party.) Such stalls are likely to
>> > be noticeable as jitter or judder, so if you can spot their source
>> > hopefully we can tackle the root cause.
>>
>> What exactly do you mean by third party? I wouldn't notice if there
>> was jitter because it's a loading screen, and there's absolutely no
>> updates on the screen; it's static.
>
> Other users of /dev/dri/card0. The most likely cause for the delay here
> is waiting for the GPU to complete an operation or for waiting for
> access to the GPU.
>
>> Do you mean you would like me to add debugging in
>> sna_accel_block_handler() to figure exactly which block is taking too
>> long to complete?
>
> Right, if you do something like:
>
> diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
> index 21cd342..31ea1a3 100644
> @@ -15931,6 +15931,8 @@ void sna_accel_close(struct sna *sna)
>
>  void sna_accel_block_handler(struct sna *sna, struct timeval **tv)
>  {
> +       uint32_t duration[NUM_TIMERS] = { 0 };
> +
>         sigtrap_assert();
>
>         if (sna->kgem.need_retire)
> @@ -15947,19 +15949,28 @@ void sna_accel_block_handler(struct sna *sna, struct timeval **tv)
>         }
>
>  restart:
> -       if (sna_accel_do_flush(sna))
> +       if (sna_accel_do_flush(sna)) {
> +               duration[FLUSH_TIMER] = TIME;
>                 sna_accel_flush(sna);
> +               duration[FLUSH_TIMER] = TIME - duration[FLUSH_TIMER];
> +       }
>         assert(sna_accel_scanout(sna) == NULL ||
>                sna_accel_scanout(sna)->gpu_bo->exec == NULL ||
>                sna->timer_active & (1<<(FLUSH_TIMER)));
>
> -       if (sna_accel_do_throttle(sna))
> +       if (sna_accel_do_throttle(sna)) {
> +               duration[THROTTLE_TIMER] = TIME;
>                 sna_accel_throttle(sna);
> +               duration[THROTTLE_TIMER] = TIME - duration[THROTTLE_TIMER];
> +       }
>         assert(!sna->kgem.need_retire ||
>                sna->timer_active & (1<<(THROTTLE_TIMER)));
>
> -       if (sna_accel_do_expire(sna))
> +       if (sna_accel_do_expire(sna)) {
> +               duration[EXPIRE_TIMER] = TIME;
>                 sna_accel_expire(sna);
> +               duration[EXPIRE_TIMER] = TIME - duration[EXPIRE_TIMER];
> +       }
>         assert(!sna->kgem.need_expire ||
>                sna->timer_active & (1<<(EXPIRE_TIMER)));
>
> @@ -15981,8 +15992,14 @@ restart:
>                 timeout = sna->timer_expire[FLUSH_TIMER] - TIME;
>                 DBG(("%s: flush timer expires in %d [%d]\n",
>                      __FUNCTION__, timeout, sna->timer_expire[FLUSH_TIMER]));
> -               if (timeout < 3)
> +               if (timeout < 3) {
> +                       ErrorF("restarting timeout[%d] (%d, %d, %d)\n",
> +                              timeout,
> +                              duration[FLUSH_TIMER],
> +                              duration[THROTTLE_TIMER],
> +                              duration[EXPIRE_TIMER]);
>                         goto restart;
> +               }
>
>                 if (*tv == NULL) {
>                         *tv = &sna->timer_tv;
>
> That will hopefully catch which path is consuming too much time

This is what I got:

restarting timeout[2] (14, 0, 0)
restarting timeout[0] (16, 0, 0)
restarting timeout[0] (16, 0, 0)
restarting timeout[2] (14, 0, 0)
restarting timeout[1] (15, 0, 0)
restarting timeout[0] (16, 0, 0)
restarting timeout[0] (16, 0, 0)
restarting timeout[1] (15, 0, 0)
restarting timeout[1] (15, 0, 0)
restarting timeout[-1] (17, 0, 0)

-- 
Felipe Contreras