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

[Christian Gmeiner <christian.gmeiner@xxxxxxxxx>]

Hi Lucas.

Thanks for your review - hopefully there will be only one last v3
series of that patches.

2017-08-08 12:00 GMT+02:00 Lucas Stach <l.stach@xxxxxxxxxxxxxx>:
> 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@xxxxxxxxx>
>> ---
>>  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.

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

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;

        ret = wait_for_completion_timeout(&gpu->event_free,
                        msecs_to_jiffies(10 * 10000));
        if (!ret) {
            dev_err(gpu->dev, "wait_for_completion_timeout failed");
            err = -EBUSY;
            goto out;
        }
    }

    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:
    return err;
}

greets
--
Christian Gmeiner, MSc

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