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.
> >> @@ -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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
