From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E1FEDC433EF for ; Thu, 3 Mar 2022 13:13:21 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4AA9110E272; Thu, 3 Mar 2022 13:13:20 +0000 (UTC) Received: from mail-ej1-x62d.google.com (mail-ej1-x62d.google.com [IPv6:2a00:1450:4864:20::62d]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8A3E210E272 for ; Thu, 3 Mar 2022 13:13:18 +0000 (UTC) Received: by mail-ej1-x62d.google.com with SMTP id bg10so10591075ejb.4 for ; Thu, 03 Mar 2022 05:13:18 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to; bh=z83ai6YJkmKUW5jJF+issOQYawb8DTmSR4isI7/tX8o=; b=Aw6EcItCCacH6Q/KGW4/qLnNgYEcCT996Pt8p07zKW4Y05TE3Kd7eLXkAPShtgOJp+ nTFvXZOe4ON3z9uo1jorAVBkB2lD6nlfD/wCJS3Ic1h7/I/XLpp25moYQgEFXci1yBmk an13Oh+jOH9/tOiwEFFt5dwEGMyYEl7z39MfYCnJ64kDrF2ChYL64SKKCn2QLC1YS+pq XIYupm8ObM7CjViMMN6Xaugqhs0EF6PpdgHhTdZkqwVvBMZGpZinDK5zyuYYLMw72Dih SDTPPayBAXfbnCWeBz2qqQynVmb8zDt2qZmVop2XDkwfIocQhqV52rjiiZCVWkzzJSqM jmiw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to; bh=z83ai6YJkmKUW5jJF+issOQYawb8DTmSR4isI7/tX8o=; b=IFXlTe2MZWh3LZS2eMEqLAt65g4QNc26y5N9ZTvZkXuKcKnLom2H42ObnpzIyxHy/p QrR3+qQBi5tAC3/G9VERV3A6zjQNRM11P1KNDctM2IGd/hA5+KY3m0BleicYWf3AuSxN CalTGifVjL9iLjEVWQL6qjBKLqKyK9eC7WQ39g1tBdLuUWGleUF/1R70ZDtj0LQ3cv2y O/zSBAaqpfRiAS9sX+ozyXguPlknmxsQekZsjrsXLFvfu+KIdeXhCtPgyShqWiuuxFCy VhHAcaXXKzqSDDcT9wdi46m1TUfbkDENK4nFQ0mhhO1evMb1h5WZq6fYYwpMYIt1ImZ/ Bapg== X-Gm-Message-State: AOAM530V1Pd/5dAi9Ctw8A2fcOqhZkTtqBKi0o+FcytYJ6J16Ziv/Kns Xa+w6pN9Cer6STSAHvWuZfU= X-Google-Smtp-Source: ABdhPJzWWn1KJC5Y5z2KGWrGiUQWe8T31Z0Bv0oHFd1su4SZywv7m3muqLYjkGZaczowONPst9v0Gw== X-Received: by 2002:a17:906:974e:b0:6bb:4f90:a6ae with SMTP id o14-20020a170906974e00b006bb4f90a6aemr27697906ejy.452.1646313196785; Thu, 03 Mar 2022 05:13:16 -0800 (PST) Received: from [192.168.178.21] (p5b0eab60.dip0.t-ipconnect.de. [91.14.171.96]) by smtp.gmail.com with ESMTPSA id ce7-20020a170906b24700b006cf095c2f5bsm694935ejb.83.2022.03.03.05.13.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Mar 2022 05:13:16 -0800 (PST) Content-Type: multipart/alternative; boundary="------------YwDqmQ8S0wtpPBImV3BrUKIL" Message-ID: Date: Thu, 3 Mar 2022 14:13:15 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: [PATCH 18/24] dma-buf: add enum dma_resv_usage v3 Content-Language: en-US To: Jason Ekstrand , Daniel Vetter References: <20211207123411.167006-1-christian.koenig@amd.com> <20211207123411.167006-19-christian.koenig@amd.com> From: =?UTF-8?Q?Christian_K=c3=b6nig?= In-Reply-To: X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "moderated list:DMA BUFFER SHARING FRAMEWORK" , Maling list - DRI developers , "open list:DMA BUFFER SHARING FRAMEWORK" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" This is a multi-part message in MIME format. --------------YwDqmQ8S0wtpPBImV3BrUKIL Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Am 02.03.22 um 18:55 schrieb Jason Ekstrand: > > On Wed, Dec 22, 2021 at 4:00 PM Daniel Vetter wrote: > > On Tue, Dec 07, 2021 at 01:34:05PM +0100, Christian König wrote: > > This change adds the dma_resv_usage enum and allows us to > specify why a > > dma_resv object is queried for its containing fences. > > > > Additional to that a dma_resv_usage_rw() helper function is > added to aid > > retrieving the fences for a read or write userspace submission. > > > > This is then deployed to the different query functions of the > dma_resv > > object and all of their users. When the write paratermer was > previously > > true we now use DMA_RESV_USAGE_WRITE and DMA_RESV_USAGE_READ > otherwise. > > > > v2: add KERNEL/OTHER in separate patch > > v3: some kerneldoc suggestions by Daniel > > > > Signed-off-by: Christian König > > Just commenting on the kerneldoc here. > > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > > index ecb2ff606bac..33a17db89fb4 100644 > > --- a/drivers/dma-buf/dma-resv.c > > +++ b/drivers/dma-buf/dma-resv.c > > @@ -408,7 +408,7 @@ static void > dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) > >         cursor->seq = read_seqcount_begin(&cursor->obj->seq); > >         cursor->index = -1; > >         cursor->shared_count = 0; > > -       if (cursor->all_fences) { > > +       if (cursor->usage >= DMA_RESV_USAGE_READ) { > > > If we're going to do this.... > > >                 cursor->fences = dma_resv_shared_list(cursor->obj); > >                 if (cursor->fences) > >                         cursor->shared_count = > cursor->fences->shared_count; > > > > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > > index 40ac9d486f8f..d96d8ca9af56 100644 > > --- a/include/linux/dma-resv.h > > +++ b/include/linux/dma-resv.h > > @@ -49,6 +49,49 @@ extern struct ww_class reservation_ww_class; > > > >  struct dma_resv_list; > > > > +/** > > + * enum dma_resv_usage - how the fences from a dma_resv obj are > used > > + * > > > We probably want a note in here about the ordering of this enum.  I'm > not even sure that comparing enum values is good or that all values > will have a strict ordering that can be useful.  It would definitely > make me nervous if anything outside dma-resv.c is doing comparisons on > these values. Exactly that is added in the follow up patch which adds DMA_RESV_USAGE_KERNEL. I've just had everything in one single patch originally. Christian. > > --Jason > > > + * This enum describes the different use cases for a dma_resv > object and > > + * controls which fences are returned when queried. > > We need to link here to both dma_buf.resv and from there to here. > > Also we had a fair amount of text in the old dma_resv fields which > should > probably be included here. > > > + */ > > +enum dma_resv_usage { > > +     /** > > +      * @DMA_RESV_USAGE_WRITE: Implicit write synchronization. > > +      * > > +      * This should only be used for userspace command > submissions which add > > +      * an implicit write dependency. > > +      */ > > +     DMA_RESV_USAGE_WRITE, > > + > > +     /** > > +      * @DMA_RESV_USAGE_READ: Implicit read synchronization. > > +      * > > +      * This should only be used for userspace command > submissions which add > > +      * an implicit read dependency. > > I think the above would benefit from at least a link each to > &dma_buf.resv > for further discusion. > > Plus the READ flag needs a huge warning that in general it does _not_ > guarantee that neither there's no writes possible, nor that the > writes can > be assumed mistakes and dropped (on buffer moves e.g.). > > Drivers can only make further assumptions for driver-internal dma_resv > objects (e.g. on vm/pagetables) or when the fences are all fences > of the > same driver (e.g. the special sync rules amd has that takes the fence > owner into account). > > We have this documented in the dma_buf.resv rules, but since it > came up > again in a discussion with Thomas H. somewhere, it's better to > hammer this > in a few more time. Specically in generally ignoring READ fences for > buffer moves (well the copy job, memory freeing still has to wait > for all > of them) is a correctness bug. > > Maybe include a big warning that really the difference between > READ and > WRITE should only matter for implicit sync, and _not_ for anything > else > the kernel does. > > I'm assuming the actual replacement is all mechanical, so I > skipped that > one for now, that's for next year :-) > -Daniel > > > +      */ > > +     DMA_RESV_USAGE_READ, > > +}; > > + > > +/** > > + * dma_resv_usage_rw - helper for implicit sync > > + * @write: true if we create a new implicit sync write > > + * > > + * This returns the implicit synchronization usage for write or > read accesses, > > + * see enum dma_resv_usage. > > + */ > > +static inline enum dma_resv_usage dma_resv_usage_rw(bool write) > > +{ > > +     /* This looks confusing at first sight, but is indeed correct. > > +      * > > +      * The rational is that new write operations needs to wait > for the > > +      * existing read and write operations to finish. > > +      * But a new read operation only needs to wait for the > existing write > > +      * operations to finish. > > +      */ > > +     return write ? DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE; > > +} > > + > >  /** > >   * struct dma_resv - a reservation object manages fences for a > buffer > >   * > > @@ -147,8 +190,8 @@ struct dma_resv_iter { > >       /** @obj: The dma_resv object we iterate over */ > >       struct dma_resv *obj; > > > > -     /** @all_fences: If all fences should be returned */ > > -     bool all_fences; > > +     /** @usage: Controls which fences are returned */ > > +     enum dma_resv_usage usage; > > > >       /** @fence: the currently handled fence */ > >       struct dma_fence *fence; > > @@ -178,14 +221,14 @@ struct dma_fence > *dma_resv_iter_next(struct dma_resv_iter *cursor); > >   * dma_resv_iter_begin - initialize a dma_resv_iter object > >   * @cursor: The dma_resv_iter object to initialize > >   * @obj: The dma_resv object which we want to iterate over > > - * @all_fences: If all fences should be returned or just the > exclusive one > > + * @usage: controls which fences to include, see enum > dma_resv_usage. > >   */ > >  static inline void dma_resv_iter_begin(struct dma_resv_iter > *cursor, > >                                      struct dma_resv *obj, > > -                                    bool all_fences) > > +                                    enum dma_resv_usage usage) > >  { > >       cursor->obj = obj; > > -     cursor->all_fences = all_fences; > > +     cursor->usage = usage; > >       cursor->fence = NULL; > >  } > > > > @@ -242,7 +285,7 @@ static inline bool > dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) > >   * dma_resv_for_each_fence - fence iterator > >   * @cursor: a struct dma_resv_iter pointer > >   * @obj: a dma_resv object pointer > > - * @all_fences: true if all fences should be returned > > + * @usage: controls which fences to return > >   * @fence: the current fence > >   * > >   * Iterate over the fences in a struct dma_resv object while > holding the > > @@ -251,8 +294,8 @@ static inline bool > dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) > >   * valid as long as the lock is held and so no extra reference > to the fence is > >   * taken. > >   */ > > -#define dma_resv_for_each_fence(cursor, obj, all_fences, > fence)      \ > > -     for (dma_resv_iter_begin(cursor, obj, all_fences),      \ > > +#define dma_resv_for_each_fence(cursor, obj, usage, fence)   \ > > +     for (dma_resv_iter_begin(cursor, obj, usage),   \ > >            fence = dma_resv_iter_first(cursor); fence;       \ > >            fence = dma_resv_iter_next(cursor)) > > > > @@ -419,14 +462,14 @@ void dma_resv_add_shared_fence(struct > dma_resv *obj, struct dma_fence *fence); > >  void dma_resv_replace_fences(struct dma_resv *obj, uint64_t > context, > >                            struct dma_fence *fence); > >  void dma_resv_add_excl_fence(struct dma_resv *obj, struct > dma_fence *fence); > > -int dma_resv_get_fences(struct dma_resv *obj, bool write, > > +int dma_resv_get_fences(struct dma_resv *obj, enum > dma_resv_usage usage, > >                       unsigned int *num_fences, struct dma_fence > ***fences); > > -int dma_resv_get_singleton(struct dma_resv *obj, bool write, > > +int dma_resv_get_singleton(struct dma_resv *obj, enum > dma_resv_usage usage, > >                          struct dma_fence **fence); > >  int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv > *src); > > -long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, > bool intr, > > -                        unsigned long timeout); > > -bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all); > > +long dma_resv_wait_timeout(struct dma_resv *obj, enum > dma_resv_usage usage, > > +                        bool intr, unsigned long timeout); > > +bool dma_resv_test_signaled(struct dma_resv *obj, enum > dma_resv_usage usage); > >  void dma_resv_describe(struct dma_resv *obj, struct seq_file *seq); > > > >  #endif /* _LINUX_RESERVATION_H */ > > -- > > 2.25.1 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > --------------YwDqmQ8S0wtpPBImV3BrUKIL Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit Am 02.03.22 um 18:55 schrieb Jason Ekstrand:

On Wed, Dec 22, 2021 at 4:00 PM Daniel Vetter <daniel@ffwll.ch> wrote:
On Tue, Dec 07, 2021 at 01:34:05PM +0100, Christian König wrote:
> This change adds the dma_resv_usage enum and allows us to specify why a
> dma_resv object is queried for its containing fences.
>
> Additional to that a dma_resv_usage_rw() helper function is added to aid
> retrieving the fences for a read or write userspace submission.
>
> This is then deployed to the different query functions of the dma_resv
> object and all of their users. When the write paratermer was previously
> true we now use DMA_RESV_USAGE_WRITE and DMA_RESV_USAGE_READ otherwise.
>
> v2: add KERNEL/OTHER in separate patch
> v3: some kerneldoc suggestions by Daniel
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Just commenting on the kerneldoc here.

> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index ecb2ff606bac..33a17db89fb4 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -408,7 +408,7 @@ static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor)
>         cursor->seq = read_seqcount_begin(&cursor->obj->seq);
>         cursor->index = -1;
>         cursor->shared_count = 0;
> -       if (cursor->all_fences) {
> +       if (cursor->usage >= DMA_RESV_USAGE_READ) {

If we're going to do this....
 
>                 cursor->fences = dma_resv_shared_list(cursor->obj);
>                 if (cursor->fences)
>                         cursor->shared_count = cursor->fences->shared_count; 

> diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h
> index 40ac9d486f8f..d96d8ca9af56 100644
> --- a/include/linux/dma-resv.h
> +++ b/include/linux/dma-resv.h
> @@ -49,6 +49,49 @@ extern struct ww_class reservation_ww_class;
> 
>  struct dma_resv_list;
> 
> +/**
> + * enum dma_resv_usage - how the fences from a dma_resv obj are used
> + *

We probably want a note in here about the ordering of this enum.  I'm not even sure that comparing enum values is good or that all values will have a strict ordering that can be useful.  It would definitely make me nervous if anything outside dma-resv.c is doing comparisons on these values.

Exactly that is added in the follow up patch which adds DMA_RESV_USAGE_KERNEL.

I've just had everything in one single patch originally.

Christian.


--Jason
 
> + * This enum describes the different use cases for a dma_resv object and
> + * controls which fences are returned when queried.

We need to link here to both dma_buf.resv and from there to here.

Also we had a fair amount of text in the old dma_resv fields which should
probably be included here.

> + */
> +enum dma_resv_usage {
> +     /**
> +      * @DMA_RESV_USAGE_WRITE: Implicit write synchronization.
> +      *
> +      * This should only be used for userspace command submissions which add
> +      * an implicit write dependency.
> +      */
> +     DMA_RESV_USAGE_WRITE,
> +
> +     /**
> +      * @DMA_RESV_USAGE_READ: Implicit read synchronization.
> +      *
> +      * This should only be used for userspace command submissions which add
> +      * an implicit read dependency.

I think the above would benefit from at least a link each to &dma_buf.resv
for further discusion.

Plus the READ flag needs a huge warning that in general it does _not_
guarantee that neither there's no writes possible, nor that the writes can
be assumed mistakes and dropped (on buffer moves e.g.).

Drivers can only make further assumptions for driver-internal dma_resv
objects (e.g. on vm/pagetables) or when the fences are all fences of the
same driver (e.g. the special sync rules amd has that takes the fence
owner into account).

We have this documented in the dma_buf.resv rules, but since it came up
again in a discussion with Thomas H. somewhere, it's better to hammer this
in a few more time. Specically in generally ignoring READ fences for
buffer moves (well the copy job, memory freeing still has to wait for all
of them) is a correctness bug.

Maybe include a big warning that really the difference between READ and
WRITE should only matter for implicit sync, and _not_ for anything else
the kernel does.

I'm assuming the actual replacement is all mechanical, so I skipped that
one for now, that's for next year :-)
-Daniel

> +      */
> +     DMA_RESV_USAGE_READ,
> +};
> +
> +/**
> + * dma_resv_usage_rw - helper for implicit sync
> + * @write: true if we create a new implicit sync write
> + *
> + * This returns the implicit synchronization usage for write or read accesses,
> + * see enum dma_resv_usage.
> + */
> +static inline enum dma_resv_usage dma_resv_usage_rw(bool write)
> +{
> +     /* This looks confusing at first sight, but is indeed correct.
> +      *
> +      * The rational is that new write operations needs to wait for the
> +      * existing read and write operations to finish.
> +      * But a new read operation only needs to wait for the existing write
> +      * operations to finish.
> +      */
> +     return write ? DMA_RESV_USAGE_READ : DMA_RESV_USAGE_WRITE;
> +}
> +
>  /**
>   * struct dma_resv - a reservation object manages fences for a buffer
>   *
> @@ -147,8 +190,8 @@ struct dma_resv_iter {
>       /** @obj: The dma_resv object we iterate over */
>       struct dma_resv *obj;
> 
> -     /** @all_fences: If all fences should be returned */
> -     bool all_fences;
> +     /** @usage: Controls which fences are returned */
> +     enum dma_resv_usage usage;
> 
>       /** @fence: the currently handled fence */
>       struct dma_fence *fence;
> @@ -178,14 +221,14 @@ struct dma_fence *dma_resv_iter_next(struct dma_resv_iter *cursor);
>   * dma_resv_iter_begin - initialize a dma_resv_iter object
>   * @cursor: The dma_resv_iter object to initialize
>   * @obj: The dma_resv object which we want to iterate over
> - * @all_fences: If all fences should be returned or just the exclusive one
> + * @usage: controls which fences to include, see enum dma_resv_usage.
>   */
>  static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor,
>                                      struct dma_resv *obj,
> -                                    bool all_fences)
> +                                    enum dma_resv_usage usage)
>  {
>       cursor->obj = obj;
> -     cursor->all_fences = all_fences;
> +     cursor->usage = usage;
>       cursor->fence = NULL;
>  }
> 
> @@ -242,7 +285,7 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor)
>   * dma_resv_for_each_fence - fence iterator
>   * @cursor: a struct dma_resv_iter pointer
>   * @obj: a dma_resv object pointer
> - * @all_fences: true if all fences should be returned
> + * @usage: controls which fences to return
>   * @fence: the current fence
>   *
>   * Iterate over the fences in a struct dma_resv object while holding the
> @@ -251,8 +294,8 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor)
>   * valid as long as the lock is held and so no extra reference to the fence is
>   * taken.
>   */
> -#define dma_resv_for_each_fence(cursor, obj, all_fences, fence)      \
> -     for (dma_resv_iter_begin(cursor, obj, all_fences),      \
> +#define dma_resv_for_each_fence(cursor, obj, usage, fence)   \
> +     for (dma_resv_iter_begin(cursor, obj, usage),   \
>            fence = dma_resv_iter_first(cursor); fence;        \
>            fence = dma_resv_iter_next(cursor))
> 
> @@ -419,14 +462,14 @@ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence);
>  void dma_resv_replace_fences(struct dma_resv *obj, uint64_t context,
>                            struct dma_fence *fence);
>  void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence);
> -int dma_resv_get_fences(struct dma_resv *obj, bool write,
> +int dma_resv_get_fences(struct dma_resv *obj, enum dma_resv_usage usage,
>                       unsigned int *num_fences, struct dma_fence ***fences);
> -int dma_resv_get_singleton(struct dma_resv *obj, bool write,
> +int dma_resv_get_singleton(struct dma_resv *obj, enum dma_resv_usage usage,
>                          struct dma_fence **fence);
>  int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
> -long dma_resv_wait_timeout(struct dma_resv *obj, bool wait_all, bool intr,
> -                        unsigned long timeout);
> -bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all);
> +long dma_resv_wait_timeout(struct dma_resv *obj, enum dma_resv_usage usage,
> +                        bool intr, unsigned long timeout);
> +bool dma_resv_test_signaled(struct dma_resv *obj, enum dma_resv_usage usage);
>  void dma_resv_describe(struct dma_resv *obj, struct seq_file *seq);
> 
>  #endif /* _LINUX_RESERVATION_H */
> --
> 2.25.1
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

--------------YwDqmQ8S0wtpPBImV3BrUKIL--