All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: "Christian König" <christian.koenig@amd.com>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Gustavo Padovan" <gustavo@padovan.org>,
	"Tomasz Figa" <tfiga@chromium.org>,
	"Ricardo Ribalda" <ribalda@chromium.org>,
	"Christoph Hellwig" <hch@infradead.org>,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org
Subject: Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock
Date: Wed, 1 Jun 2022 16:38:51 +0200	[thread overview]
Message-ID: <a009c207-a5fa-af1e-b961-8083b48360bf@gmail.com> (raw)
In-Reply-To: <Ypd3Us3a93aLonqT@google.com>

Am 01.06.22 um 16:27 schrieb Sergey Senozhatsky:
> On (22/06/01 14:45), Christian König wrote:
>> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
>>> On (22/05/30 16:55), Christian König wrote:
>>>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>>>> [SNIP]
>>>>> So the `lock` should have at least same lifespan as the DMA fence
>>>>> that borrows it, which is impossible to guarantee in our case.
>>>> Nope, that's not correct. The lock should have at least same lifespan as the
>>>> context of the DMA fence.
>>> How does one know when it's safe to release the context? DMA fence
>>> objects are still transparently refcount-ed and "live their own lives",
>>> how does one synchronize lifespans?
>> Well, you don't.
>>
>> If you have a dynamic context structure you need to reference count that as
>> well. In other words every time you create a fence in your context you need
>> to increment the reference count and every time a fence is release you
>> decrement it.
> OK then fence release should be able to point back to its "context"
> structure. Either a "private" data in dma fence or we need to "embed"
> fence into another object (refcounted) that owns the lock and provide
> dma fence ops->release callback, which can container_of() to the object
> that dma fence is embedded into.
>
> I think you are suggesting the latter. Thanks for clarifications.

Daniel might hurt me for this, but if you really only need a pointer to 
your context then we could say that using a pointer value for the 
context field is ok as well.

That should be fine as well as long as you can guarantee that it will be 
unique during the lifetime of all it's fences.

We would just have to adjust the documentation a bit.

> The limiting factor of this approach is that now our ops->release() is
> under the same "pressure" as dma_fence_put()->dma_fence_release() are.
> dma_fence_put() and dma_fence_release() can be called from any context,
> as far as I understand, e.g. IRQ, however our normal object ->release
> can schedule, we do things like synchronize_rcu() and so on. Nothing is
> impossible, just saying that even this approach is not 100% perfect and
> may need additional workarounds.

Well just use a work item for release.

Regards,
Christian.

>> If you have a static context structure like most drivers have then you must
>> make sure that all fences at least signal before you unload your driver. We
>> still somewhat have a race when you try to unload a driver and the fence_ops
>> structure suddenly disappear, but we currently live with that.
> Hmm, indeed... I didn't consider fence_ops case.
>
>> Apart from that you are right, fences can live forever and we need to deal
>> with that.
> OK. I see.


WARNING: multiple messages have this Message-ID (diff)
From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: linaro-mm-sig@lists.linaro.org,
	"Gustavo Padovan" <gustavo@padovan.org>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
	"Tomasz Figa" <tfiga@chromium.org>,
	"Christoph Hellwig" <hch@infradead.org>,
	"Ricardo Ribalda" <ribalda@chromium.org>,
	"Sumit Semwal" <sumit.semwal@linaro.org>,
	"Christian König" <christian.koenig@amd.com>,
	linux-media@vger.kernel.org
Subject: Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have their own lock
Date: Wed, 1 Jun 2022 16:38:51 +0200	[thread overview]
Message-ID: <a009c207-a5fa-af1e-b961-8083b48360bf@gmail.com> (raw)
In-Reply-To: <Ypd3Us3a93aLonqT@google.com>

Am 01.06.22 um 16:27 schrieb Sergey Senozhatsky:
> On (22/06/01 14:45), Christian König wrote:
>> Am 31.05.22 um 04:51 schrieb Sergey Senozhatsky:
>>> On (22/05/30 16:55), Christian König wrote:
>>>> Am 30.05.22 um 16:22 schrieb Sergey Senozhatsky:
>>>>> [SNIP]
>>>>> So the `lock` should have at least same lifespan as the DMA fence
>>>>> that borrows it, which is impossible to guarantee in our case.
>>>> Nope, that's not correct. The lock should have at least same lifespan as the
>>>> context of the DMA fence.
>>> How does one know when it's safe to release the context? DMA fence
>>> objects are still transparently refcount-ed and "live their own lives",
>>> how does one synchronize lifespans?
>> Well, you don't.
>>
>> If you have a dynamic context structure you need to reference count that as
>> well. In other words every time you create a fence in your context you need
>> to increment the reference count and every time a fence is release you
>> decrement it.
> OK then fence release should be able to point back to its "context"
> structure. Either a "private" data in dma fence or we need to "embed"
> fence into another object (refcounted) that owns the lock and provide
> dma fence ops->release callback, which can container_of() to the object
> that dma fence is embedded into.
>
> I think you are suggesting the latter. Thanks for clarifications.

Daniel might hurt me for this, but if you really only need a pointer to 
your context then we could say that using a pointer value for the 
context field is ok as well.

That should be fine as well as long as you can guarantee that it will be 
unique during the lifetime of all it's fences.

We would just have to adjust the documentation a bit.

> The limiting factor of this approach is that now our ops->release() is
> under the same "pressure" as dma_fence_put()->dma_fence_release() are.
> dma_fence_put() and dma_fence_release() can be called from any context,
> as far as I understand, e.g. IRQ, however our normal object ->release
> can schedule, we do things like synchronize_rcu() and so on. Nothing is
> impossible, just saying that even this approach is not 100% perfect and
> may need additional workarounds.

Well just use a work item for release.

Regards,
Christian.

>> If you have a static context structure like most drivers have then you must
>> make sure that all fences at least signal before you unload your driver. We
>> still somewhat have a race when you try to unload a driver and the fence_ops
>> structure suddenly disappear, but we currently live with that.
> Hmm, indeed... I didn't consider fence_ops case.
>
>> Apart from that you are right, fences can live forever and we need to deal
>> with that.
> OK. I see.


  reply	other threads:[~2022-06-01 14:39 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-30 14:22 [PATCH] dma-fence: allow dma fence to have their own lock Sergey Senozhatsky
2022-05-30 14:22 ` Sergey Senozhatsky
2022-05-30 14:55 ` Christian König
2022-05-30 14:55   ` Christian König
2022-05-30 15:09   ` Sergey Senozhatsky
2022-05-30 15:09     ` Sergey Senozhatsky
2022-05-30 15:45   ` Sergey Senozhatsky
2022-05-30 15:45     ` Sergey Senozhatsky
2022-06-01 13:50     ` [Linaro-mm-sig] " Christian König
2022-06-01 13:50       ` Christian König
2022-05-31  2:51   ` Sergey Senozhatsky
2022-05-31  2:51     ` Sergey Senozhatsky
2022-06-01 12:45     ` [Linaro-mm-sig] " Christian König
2022-06-01 12:45       ` Christian König
2022-06-01 13:22       ` Daniel Vetter
2022-06-01 13:22         ` Daniel Vetter
2022-06-01 13:52         ` Christian König
2022-06-01 14:27       ` Sergey Senozhatsky
2022-06-01 14:27         ` Sergey Senozhatsky
2022-06-01 14:38         ` Christian König [this message]
2022-06-01 14:38           ` Christian König
2022-06-01 14:52           ` Sergey Senozhatsky
2022-06-01 14:52             ` Sergey Senozhatsky
2022-06-01 15:06             ` Christian König
2022-06-01 15:06               ` Christian König

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a009c207-a5fa-af1e-b961-8083b48360bf@gmail.com \
    --to=ckoenig.leichtzumerken@gmail.com \
    --cc=christian.koenig@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gustavo@padovan.org \
    --cc=hch@infradead.org \
    --cc=linaro-mm-sig@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=ribalda@chromium.org \
    --cc=senozhatsky@chromium.org \
    --cc=sumit.semwal@linaro.org \
    --cc=tfiga@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.