Re: [PATCH] dma-buf: refcount the attachment for cache_sgt_mapping

[Koenig, Christian]

Am 12.06.19 um 10:02 schrieb Nicolin Chen:
> Hi Christian,
>
> Thanks for the quick reply.
>
> On Wed, Jun 12, 2019 at 07:45:38AM +0000, Koenig, Christian wrote:
>> Am 12.06.19 um 03:22 schrieb Nicolin Chen:
>>> Commit f13e143e7444 ("dma-buf: start caching of sg_table objects v2")
>>> added a support of caching the sgt pointer into an attach pointer to
>>> let users reuse the sgt pointer without another mapping. However, it
>>> might not totally work as most of dma-buf callers are doing attach()
>>> and map_attachment() back-to-back, using drm_prime.c for example:
>>>       drm_gem_prime_import_dev() {
>>>           attach = dma_buf_attach() {
>>>               /* Allocating a new attach */
>>>               attach = kzalloc();
>>>               /* .... */
>>>               return attach;
>>>           }
>>>           dma_buf_map_attachment(attach, direction) {
>>>               /* attach->sgt would be always empty as attach is new */
>>>               if (attach->sgt) {
>>>                   /* Reuse attach->sgt */
>>>               }
>>>               /* Otherwise, map it */
>>>               attach->sgt = map();
>>>           }
>>>       }
>>>
>>> So, for a cache_sgt_mapping use case, it would need to get the same
>>> attachment pointer in order to reuse its sgt pointer. So this patch
>>> adds a refcount to the attach() function and lets it search for the
>>> existing attach pointer by matching the dev pointer.
>> I don't think that this is a good idea.
>>
>> We use sgt caching as workaround for locking order problems and want to
>> remove it again in the long term.
> Oh. I thought it was for a performance improving purpose. It may
> be a misunderstanding then.
>
>> So what is the actual use case of this?
> We have some similar downstream changes at dma_buf to reduce the
> overhead from multiple clients of the same device doing attach()
> and map_attachment() calls for the same dma_buf.

I don't think that this is a good idea over all. A driver calling attach 
for the same buffer is doing something wrong in the first place and we 
should not work around this in the DMA-buf handling.

> We haven't used DRM/GRM_PRIME yet but I am also curious would it
> benefit DRM also if we reduce this overhead in the dma_buf?

No, not at all.

Regards,
Christian.

>
> Thanks
> Nicolin