Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <30c96646-bb16-a876-57f5-155d46b8d805@gmail.com>
Date:   Wed, 1 Jun 2022 17:06:18 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [Linaro-mm-sig] Re: [PATCH] dma-fence: allow dma fence to have
 their own lock
Content-Language: en-US
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     =?UTF-8?Q?Christian_K=c3=b6nig?= <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
References: <20220530142232.2871634-1-senozhatsky@chromium.org>
 <7eee4274-bd69-df8d-9067-771366217804@amd.com> <YpWCvniLzJfcp684@google.com>
 <33aba213-b6ad-4a15-9272-c62f5dfb1fb7@gmail.com>
 <Ypd3Us3a93aLonqT@google.com>
 <a009c207-a5fa-af1e-b961-8083b48360bf@gmail.com>
 <Ypd9OSqMtGMVKYZ0@google.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <ckoenig.leichtzumerken@gmail.com>
In-Reply-To: <Ypd9OSqMtGMVKYZ0@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Am 01.06.22 um 16:52 schrieb Sergey Senozhatsky:
> On (22/06/01 16:38), Christian König wrote:
>>>> 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.
> I think we can guarantee that. Object that creates fence is kmalloc-ed and
> it sticks around until dma_fence_release() calls ops->release() and kfree-s
> it. We *probably* can even do something like it now, by re-purposing dma_fence
> context member:
>
>          dma_fence_init(obj->fence,
>                         &fence_ops,
>                         &obj->fence_lock,
>                         (u64)obj,                             <<   :/
>                         atomic64_inc_return(&obj->seqno));
>
> I'd certainly refrain from being creative here and doing things that
> are not documented/common. DMA fence embedding should work for us.

Yeah, exactly that's the idea. But if you are fine to create a subclass 
of the dma_fence than that would indeed be cleaner.

Christian.

>
>>> 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.
> Yup, that's the plan.