Hi Lucas,
2017-08-22 10:39 GMT+02:00 Lucas Stach <l.stach@xxxxxxxxxxxxxx>:
> 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@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.
>
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
