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

[Christian Gmeiner]

Hi Lucas,

2017-08-22 10:39 GMT+02:00 Lucas Stach <l.stach at 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 at 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 at 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.
>

Yeah :)

>> >> @@ -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;

ok

>>     }
>>
>>     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.
>

Oops.. totally missed that part - time for a coffee.

greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info