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

[Nicolin Chen]

Hi Christian,

On Wed, Jun 12, 2019 at 08:05:53AM +0000, Koenig, Christian wrote:
> 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.

>From you replies, in a summary, does it means that there won't be a case
of DRM having a dma_buf attaching to the same device, i.e. multiple calls
of drm_gem_prime_import() function with same parameters of dev + dma_buf?

If so, we can just ignore/drop this patch. Sorry for the misunderstanding.

Thanks
Nicolin