Re: [PATCH V2 02/23] drm/etnaviv: make it possible to allocate multiple events

[Lucas Stach <l.stach@pengutronix.de>]

Hi Christian,

Am Dienstag, den 22.08.2017, 10:27 +0200 schrieb Christian Gmeiner:
> Hi Lucas.
> 
> Thanks for your review - hopefully there will be only one last v3
> series of that patches.

Yep, I would really like to merge this series. It's been in the making
for long enough. :)

> 2017-08-08 12:00 GMT+02:00 Lucas Stach <l.stach@pengutronix.de>:
> > Am Samstag, den 22.07.2017, 11:53 +0200 schrieb Christian Gmeiner:
> >> This makes it possible to allocate multiple events under the event
> >> spinlock. This change is needed to support 'sync'-points.
> >>
> >> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> >> ---
> >>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 31 ++++++++++++++++++++-----------
> >>  1 file changed, 20 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> >> index fa9c7bd98e9c..ab108b0ed573 100644
> >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> >> @@ -1137,10 +1137,12 @@ int etnaviv_gpu_fence_sync_obj(struct etnaviv_gem_object *etnaviv_obj,
> >>   * event management:
> >>   */
> >>
> >> -static unsigned int event_alloc(struct etnaviv_gpu *gpu)
> >> +static int event_alloc(struct etnaviv_gpu *gpu, unsigned nr_events,
> >> +     unsigned int *events)
> >>  {
> >>       unsigned long ret, flags;
> >> -     unsigned int event;
> >> +     unsigned used, i;
> >> +     int err = 0;
> >>
> >>       ret = wait_for_completion_timeout(&gpu->event_free,
> >>                                         msecs_to_jiffies(10 * 10000));
> >
> > This isn't obvious from the current code, but there are exactly as much
> > completions in the queue, as there are events. See initialization of the
> > completions in etnaviv_gpu_init(). This means the event allocation under
> > spinlock always succeeds if the wait hasn't timed out. To keep this
> > working you need to wait for the completion nr_event times, probably
> > changing this to an absolute timeout, so we don't lengthen the timeout
> > with the number of events.
> >
> 
> Makes sense but I not really sure how to best implement an absolute
> timeout. We could wait
> absolute timeout / nr_events in each wait_for_completion_timeout(..) call.

I think we should just keep the 10sec timeout regardless of the number
of events. If we don't acquire the needed events after 10secs, it's
probably fine to give up.

> >> @@ -1149,16 +1151,24 @@ static unsigned int event_alloc(struct etnaviv_gpu *gpu)
> >>
> >>       spin_lock_irqsave(&gpu->event_spinlock, flags);
> >>
> >> -     /* find first free event */
> >> -     event = find_first_zero_bit(gpu->event_bitmap, ETNA_NR_EVENTS);
> >> -     if (event < ETNA_NR_EVENTS)
> >> +     /* are there enough free events? */
> >> +     used = bitmap_weight(gpu->event_bitmap, ETNA_NR_EVENTS);
> >> +     if (used + nr_events > ETNA_NR_EVENTS) {
> >> +             err = -EBUSY;
> >> +             goto out;
> >> +     }
> >
> > This isn't necessary if you waited successfully for the completion to
> > signal nr_event times. The allocation is guaranteed to succeed in that
> > case. We just want an early return before even locking the spinlock,
> > giving back the already collected completions if one of the waits timed
> > out.
> >
> 
> Yes - feels more elegant that way.
> 
> >> +
> >> +     for (i = 0; i < nr_events; i++) {
> >> +             int event = find_first_zero_bit(gpu->event_bitmap, ETNA_NR_EVENTS);
> >> +
> >> +             events[i] = event;
> >>               set_bit(event, gpu->event_bitmap);
> >> -     else
> >> -             event = ~0U;
> >> +     }
> >>
> >> +out:
> >>       spin_unlock_irqrestore(&gpu->event_spinlock, flags);
> >>
> >> -     return event;
> >> +     return err;
> >>  }
> >>
> >>  static void event_free(struct etnaviv_gpu *gpu, unsigned int event)
> >> @@ -1327,10 +1337,9 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
> >>        *
> >>        */
> >>
> >> -     event = event_alloc(gpu);
> >> -     if (unlikely(event == ~0U)) {
> >> +     ret = event_alloc(gpu, 1, &event);
> >> +     if (!ret) {
> >>               DRM_ERROR("no free event\n");
> >> -             ret = -EBUSY;
> >>               goto out_pm_put;
> >>       }
> >>
> >
> >
> 
> What about something like this:

A few notes inline.

> 
> static int event_alloc(struct etnaviv_gpu *gpu, unsigned nr_events,
>     unsigned int *events)
> {
>     unsigned long flags;
>     unsigned i;
>     int err = 0;
> 
>     for (i = 0; i < nr_events; i++) {
>         unsigned long ret;
	  timeout = msecs_to_jiffies(10 * 10000);
> 
>         ret = wait_for_completion_timeout(&gpu->event_free,
>                         msecs_to_jiffies(10 * 10000));
                          ^^ use timeout here
>         if (!ret) {
>             dev_err(gpu->dev, "wait_for_completion_timeout failed");
>             err = -EBUSY;
>             goto out;
>         }

If we waited successfully, wait_for_completion_timeout will return the
remaining jiffies, so we can re-initialize the timeout with the return
value to shorten the next wait.

          timeout = ret;
>     }
> 
>     spin_lock_irqsave(&gpu->event_spinlock, flags);
> 
>     for (i = 0; i < nr_events; i++) {
>         int event = find_first_zero_bit(gpu->event_bitmap, ETNA_NR_EVENTS);
> 
>         events[i] = event;
>         set_bit(event, gpu->event_bitmap);
>     }
> 
>     spin_unlock_irqrestore(&gpu->event_spinlock, flags);
> 
> out:

If you end up here you need to return the already acquired completions,
by rolling back your for loop above and do a complete(&gpu->event_free)
for each acquired completion to avoid depleting the completion queue.

>     return err;
> }

Regards,
Lucas

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel