On Tue, Oct 05, 2021 at 02:44:50PM +0200, Christian König wrote:
> Am 05.10.21 um 14:40 schrieb Tvrtko Ursulin:
> >
> > On 05/10/2021 12:37, Christian König wrote:
> > > This makes the function much simpler since the complex
> > > retry logic is now handled else where.
> > >
> > > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> > Reminder - r-b was retracted until at least more text is added to commit
> > message about pros and cons. But really some discussion had inside the
> > i915 team on the topic.
>
> Sure, going to move those to a different branch.
>
> But I really only see the following options:
> 1. Grab the lock.
> 2. Use the _unlocked variant with get/put.
> 3. Add another _rcu iterator just for this case.
>
> I'm fine with either, but Daniel pretty much already rejected #3 and #2/#1
> has more overhead then the original one.
Anything that removes open-code rcu/lockless magic from i915 gets my ack,
there's way too much of this everywhere. So on this:
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
I've asked Maarten to review the i915 ones for you, please pester him if
it's not happening :-)
-Daniel
>
> Regards,
> Christian.
>
> >
> > Regards,
> >
> > Tvrtko
> >
> > > ---
> > > drivers/gpu/drm/i915/gem/i915_gem_busy.c | 35 ++++++++++--------------
> > > 1 file changed, 14 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
> > > b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
> > > index 6234e17259c1..dc72b36dae54 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
> > > @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void
> > > *data,
> > > {
> > > struct drm_i915_gem_busy *args = data;
> > > struct drm_i915_gem_object *obj;
> > > - struct dma_resv_list *list;
> > > - unsigned int seq;
> > > + struct dma_resv_iter cursor;
> > > + struct dma_fence *fence;
> > > int err;
> > > err = -ENOENT;
> > > @@ -109,27 +109,20 @@ i915_gem_busy_ioctl(struct drm_device *dev,
> > > void *data,
> > > * to report the overall busyness. This is what the wait-ioctl
> > > does.
> > > *
> > > */
> > > -retry:
> > > - seq = raw_read_seqcount(&obj->base.resv->seq);
> > > -
> > > - /* Translate the exclusive fence to the READ *and* WRITE engine */
> > > - args->busy =
> > > busy_check_writer(dma_resv_excl_fence(obj->base.resv));
> > > -
> > > - /* Translate shared fences to READ set of engines */
> > > - list = dma_resv_shared_list(obj->base.resv);
> > > - if (list) {
> > > - unsigned int shared_count = list->shared_count, i;
> > > -
> > > - for (i = 0; i < shared_count; ++i) {
> > > - struct dma_fence *fence =
> > > - rcu_dereference(list->shared[i]);
> > > -
> > > + args->busy = 0;
> > > + dma_resv_iter_begin(&cursor, obj->base.resv, true);
> > > + dma_resv_for_each_fence_unlocked(&cursor, fence) {
> > > + if (dma_resv_iter_is_restarted(&cursor))
> > > + args->busy = 0;
> > > +
> > > + if (dma_resv_iter_is_exclusive(&cursor))
> > > + /* Translate the exclusive fence to the READ *and*
> > > WRITE engine */
> > > + args->busy |= busy_check_writer(fence);
> > > + else
> > > + /* Translate shared fences to READ set of engines */
> > > args->busy |= busy_check_reader(fence);
> > > - }
> > > }
> > > -
> > > - if (args->busy && read_seqcount_retry(&obj->base.resv->seq, seq))
> > > - goto retry;
> > > + dma_resv_iter_end(&cursor);
> > > err = 0;
> > > out:
> > >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
