Re: [RFC][PATCH 0/2] dma-buf: add importer private data forreimporting

[Daniel Vetter]

On Tue, Jun 04, 2013 at 07:42:22PM +0900, êìì wrote:
> 
> 
> On 2013ë 06ì 01ì 00:29, Daniel Vetter wrote:
> > On Fri, May 31, 2013 at 07:22:24PM +0900, êìì wrote:
> >> Hello Daniel,
> >>
> >> Thanks for your comment.
> >>
> >> On 2013ë 05ì 31ì 18:14, Daniel Vetter wrote:
> >>> On Fri, May 31, 2013 at 10:54 AM, Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx> wrote:
> >>>> importer private data in dma-buf attachment can be used by importer to
> >>>> reimport same dma-buf.
> >>>>
> >>>> Seung-Woo Kim (2):
> >>>>   dma-buf: add importer private data to attachment
> >>>>   drm/prime: find gem object from the reimported dma-buf
> >>>
> >>> Self-import should already work (at least with the latest refcount
> >>> fixes merged). At least the tests to check both re-import on the same
> >>> drm fd and on a different all work as expected now.
> >>
> >> Currently, prime works well for all case including self-importing,
> >> importing, and reimporting as you describe. Just, importing dma-buf from
> >> other driver twice with different drm_fd, each import create its own gem
> >> object even two import is done for same buffer because prime_priv is in
> >> struct drm_file. This means mapping to the device is done also twice.
> >> IMHO, these duplicated creations and maps are not necessary if drm can
> >> find previous import in different prime_priv.
> > 
> > Well, that's imo a bug with the other driver. If it doesn't export
> > something really simple (e.g. contiguous memory which doesn't require any
> > mmio resources at all) it should have a cache of exported dma_buf fds so
> > that it hands out the same dma_buf every time.
> 
> Hm, all existing dma-buf exporter including i915 driver implements its
> map_dma_buf callback as allocating scatter-gather table with pages in
> its buffer and calling dma_map_sg() with the sgt. With different
> drm_fds, importing one dma-buf *twice*, then importer calls
> dma_buf_attach() and dma_buf_map_attachment() twice at least in drm
> importer because re-importing case can only checked with prime_priv in
> drm_file as I described.

Well, but thanks to all the self-import and re-import checks, it's
_impossible_ to import the same dma_buf twice without noticing (presuming
both importer and exporter are drm devices).
> 
> > 
> > Or it needs to be more clever in it's dma_buf_attachment_map functions and
> > lookup up a pre-existing iommu mapping.
> > 
> > But dealing with this in the importer is just broken.
> > 
> >>> Second, the dma_buf_attachment is _definitely_ the wrong place to do
> >>> this. If you need iommu mapping caching, that should happen at a lower
> >>> level (i.e. in the map_attachment callback somewhere of the exporter,
> >>> that's what the priv field in the attachment is for). Snatching away
> >>> the attachement from some random other import is certainly not the way
> >>> to go - attachements are _not_ refcounted!
> >>
> >> Yes, attachments do not have refcount, so importer should handle and drm
> >> case in my patch, importer private data is gem object and it has, of
> >> course, refcount.
> >>
> >> And at current, exporter can not classify map_dma_buf requests of same
> >> importer to same buffer with different attachment because dma_buf_attach
> >> always makes new attachments. To resolve this exporter should search all
> >> different attachment from same importer of dma-buf and it seems more
> >> complex than importer private data to me.
> >>
> >> If I misunderstood something, please let me know.
> > 
> > Like I've said above, just fix this in the exporter. If an importer sees
> > two different dma_bufs it can very well presume that it those two indeed
> > point to different backing storage.
> 
> Yes, my patch does not break this concept. I just fixed case importing
> _one_ dma-buf twice with different drm_fds.

See above, if you have two different struct file * for the same underlying
buffer object something is wrong already.

> > This will be even more important if we attach fences two dma_bufs. If your
> > broken exporter creates multiple dma_bufs each one of them will have their
> > own fences attached, leading to a complete disasters. Ok, strictly
> > speaking if you keep the same reservation pointer for each dma_buf it'll
> > work, but that's just a detail of how you solve this in the exporter.
> 
> I can not understand about broken exporter you addressed. I don't mean
> exporter makes dma-bufs from one backing storage.
> While, my patch prevents not to create drm gem objects from one back
> storage by importing one dma-buf with different drm-fds.

Well, we also have code in drm prime for that case - if the same dma_buf
object shows up multiple times, we'll only import it once. For the second
import we'll return the already created drm_gem object from the first
import, but with the refcount incremented.

> I do not believe the fix of importer is the best way, but at this
> moment, I have no idea how I can fix the exporter for this issue.

I think if you have drm prime drivers both as importers and exporters, it
is already fixed. It is correct though that both importer and exporter
need a bit of code to take care and not accidentally duplicate a shared
object somehow.

But since you've proposed your rfc as part of the drm subsystem I've
figured that we don't need to discuss the duplicate import handling code.

Yours, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/