Am 16.09.20 um 17:24 schrieb Daniel Vetter:
> On Wed, Sep 16, 2020 at 4:14 PM Christian König
> <[email protected]> wrote:
>> Am 16.09.20 um 16:07 schrieb Jason Gunthorpe:
>>> On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
>>>
>>>> But within the driver, we generally need thousands of these, and that
>>>> tends to bring fd exhaustion problems with it. That's why all the private
>>>> buffer objects which aren't shared with other process or other drivers are
>>>> handles only valid for a specific fd instance of the drm chardev (each
>>>> open gets their own namespace), and only for ioctls done on that chardev.
>>>> And for mmap we assign fake (but unique across all open fd on it) offsets
>>>> within the overall chardev. Hence all the pgoff mangling and re-mangling.
>>> Are they still unique struct files? Just without a fdno?
>> Yes, exactly.
> Not entirely, since dma-buf happened after drm chardev, so for that
> historical reason the underlying struct file is shared, since it's the
> drm chardev. But since that's per-device we don't have a problem in
> practice with different vm_ops, since those are also per-device. But
> yeah we could fish out some entirely hidden per-object struct file if
> that's required for some mm internal reasons.
Hui? Ok that is just the handling in i915, isn't it?
As far as I know we create an unique struct file for each DMA-buf.
Regards,
Christian.
> -Daniel
>
>>>> Hence why we'd like to be able to forward aliasing mappings and adjust the
>>>> file and pgoff, while hopefully everything keeps working. I thought this
>>>> would work, but Christian noticed it doesn't really.
>>> It seems reasonable to me that the dma buf should be the owner of the
>>> VMA, otherwise like you say, there is a big mess attaching the custom
>>> vma ops and what not to the proper dma buf.
>>>
>>> I don't see anything obviously against this in mmap_region() - why did
>>> Chritian notice it doesn't really work?
>> To clarify I think this might work.
>>
>> I just had the same "Is that legal?", "What about security?", etc..
>> questions you raised as well.
>>
>> It seems like a source of trouble so I thought better ask somebody more
>> familiar with that.
>>
>> Christian.
>>
>>> Jason
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
On Wed, Sep 16, 2020 at 5:31 PM Christian König
<[email protected]> wrote:
>
> Am 16.09.20 um 17:24 schrieb Daniel Vetter:
> > On Wed, Sep 16, 2020 at 4:14 PM Christian König
> > <[email protected]> wrote:
> >> Am 16.09.20 um 16:07 schrieb Jason Gunthorpe:
> >>> On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
> >>>
> >>>> But within the driver, we generally need thousands of these, and that
> >>>> tends to bring fd exhaustion problems with it. That's why all the private
> >>>> buffer objects which aren't shared with other process or other drivers are
> >>>> handles only valid for a specific fd instance of the drm chardev (each
> >>>> open gets their own namespace), and only for ioctls done on that chardev.
> >>>> And for mmap we assign fake (but unique across all open fd on it) offsets
> >>>> within the overall chardev. Hence all the pgoff mangling and re-mangling.
> >>> Are they still unique struct files? Just without a fdno?
> >> Yes, exactly.
> > Not entirely, since dma-buf happened after drm chardev, so for that
> > historical reason the underlying struct file is shared, since it's the
> > drm chardev. But since that's per-device we don't have a problem in
> > practice with different vm_ops, since those are also per-device. But
> > yeah we could fish out some entirely hidden per-object struct file if
> > that's required for some mm internal reasons.
>
> Hui? Ok that is just the handling in i915, isn't it?
>
> As far as I know we create an unique struct file for each DMA-buf.
Yes dma-buf, but that gets forwarded to the original drm chardev which
originally exported the buffer. It's only there where the forwarding
chain stops. The other thing is that iirc we have a singleton
anon_inode behind all the dma-buf, so they'd share all the same
address_space and so would all alias for unmap_mapping_range (I think
at least).
-Daniel
>
> Regards,
> Christian.
>
>
> > -Daniel
> >
> >>>> Hence why we'd like to be able to forward aliasing mappings and adjust the
> >>>> file and pgoff, while hopefully everything keeps working. I thought this
> >>>> would work, but Christian noticed it doesn't really.
> >>> It seems reasonable to me that the dma buf should be the owner of the
> >>> VMA, otherwise like you say, there is a big mess attaching the custom
> >>> vma ops and what not to the proper dma buf.
> >>>
> >>> I don't see anything obviously against this in mmap_region() - why did
> >>> Chritian notice it doesn't really work?
> >> To clarify I think this might work.
> >>
> >> I just had the same "Is that legal?", "What about security?", etc..
> >> questions you raised as well.
> >>
> >> It seems like a source of trouble so I thought better ask somebody more
> >> familiar with that.
> >>
> >> Christian.
> >>
> >>> Jason
> >> _______________________________________________
> >> dri-devel mailing list
> >> [email protected]
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Am 17.09.20 um 08:23 schrieb Daniel Vetter:
> On Wed, Sep 16, 2020 at 5:31 PM Christian König
> <[email protected]> wrote:
>> Am 16.09.20 um 17:24 schrieb Daniel Vetter:
>>> On Wed, Sep 16, 2020 at 4:14 PM Christian König
>>> <[email protected]> wrote:
>>>> Am 16.09.20 um 16:07 schrieb Jason Gunthorpe:
>>>>> On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
>>>>>
>>>>>> But within the driver, we generally need thousands of these, and that
>>>>>> tends to bring fd exhaustion problems with it. That's why all the private
>>>>>> buffer objects which aren't shared with other process or other drivers are
>>>>>> handles only valid for a specific fd instance of the drm chardev (each
>>>>>> open gets their own namespace), and only for ioctls done on that chardev.
>>>>>> And for mmap we assign fake (but unique across all open fd on it) offsets
>>>>>> within the overall chardev. Hence all the pgoff mangling and re-mangling.
>>>>> Are they still unique struct files? Just without a fdno?
>>>> Yes, exactly.
>>> Not entirely, since dma-buf happened after drm chardev, so for that
>>> historical reason the underlying struct file is shared, since it's the
>>> drm chardev. But since that's per-device we don't have a problem in
>>> practice with different vm_ops, since those are also per-device. But
>>> yeah we could fish out some entirely hidden per-object struct file if
>>> that's required for some mm internal reasons.
>> Hui? Ok that is just the handling in i915, isn't it?
>>
>> As far as I know we create an unique struct file for each DMA-buf.
> Yes dma-buf, but that gets forwarded to the original drm chardev which
> originally exported the buffer. It's only there where the forwarding
> chain stops. The other thing is that iirc we have a singleton
> anon_inode behind all the dma-buf, so they'd share all the same
> address_space and so would all alias for unmap_mapping_range (I think
> at least).
Amdgpu works by using the address_space of the drm chardev into the
struct file of DMA-buf instead.
I think that this is cleaner, but only by a little bit :)
Anyway I'm a bit concerned that we have so many different approaches for
the same problem.
Christian.
> -Daniel
>
>> Regards,
>> Christian.
>>
>>
>>> -Daniel
>>>
>>>>>> Hence why we'd like to be able to forward aliasing mappings and adjust the
>>>>>> file and pgoff, while hopefully everything keeps working. I thought this
>>>>>> would work, but Christian noticed it doesn't really.
>>>>> It seems reasonable to me that the dma buf should be the owner of the
>>>>> VMA, otherwise like you say, there is a big mess attaching the custom
>>>>> vma ops and what not to the proper dma buf.
>>>>>
>>>>> I don't see anything obviously against this in mmap_region() - why did
>>>>> Chritian notice it doesn't really work?
>>>> To clarify I think this might work.
>>>>
>>>> I just had the same "Is that legal?", "What about security?", etc..
>>>> questions you raised as well.
>>>>
>>>> It seems like a source of trouble so I thought better ask somebody more
>>>> familiar with that.
>>>>
>>>> Christian.
>>>>
>>>>> Jason
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> [email protected]
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7Cf725d2eb6a5a49bd533f08d85ad23308%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637359206142262941&sdata=qcLsl9R1gP%2FGY39ctsQkIzI99Bn%2F840YS17F4xudrAE%3D&reserved=0
>>>
>
On Thu, Sep 17, 2020 at 9:11 AM Christian König
<[email protected]> wrote:
>
> Am 17.09.20 um 08:23 schrieb Daniel Vetter:
> > On Wed, Sep 16, 2020 at 5:31 PM Christian König
> > <[email protected]> wrote:
> >> Am 16.09.20 um 17:24 schrieb Daniel Vetter:
> >>> On Wed, Sep 16, 2020 at 4:14 PM Christian König
> >>> <[email protected]> wrote:
> >>>> Am 16.09.20 um 16:07 schrieb Jason Gunthorpe:
> >>>>> On Wed, Sep 16, 2020 at 11:53:59AM +0200, Daniel Vetter wrote:
> >>>>>
> >>>>>> But within the driver, we generally need thousands of these, and that
> >>>>>> tends to bring fd exhaustion problems with it. That's why all the private
> >>>>>> buffer objects which aren't shared with other process or other drivers are
> >>>>>> handles only valid for a specific fd instance of the drm chardev (each
> >>>>>> open gets their own namespace), and only for ioctls done on that chardev.
> >>>>>> And for mmap we assign fake (but unique across all open fd on it) offsets
> >>>>>> within the overall chardev. Hence all the pgoff mangling and re-mangling.
> >>>>> Are they still unique struct files? Just without a fdno?
> >>>> Yes, exactly.
> >>> Not entirely, since dma-buf happened after drm chardev, so for that
> >>> historical reason the underlying struct file is shared, since it's the
> >>> drm chardev. But since that's per-device we don't have a problem in
> >>> practice with different vm_ops, since those are also per-device. But
> >>> yeah we could fish out some entirely hidden per-object struct file if
> >>> that's required for some mm internal reasons.
> >> Hui? Ok that is just the handling in i915, isn't it?
> >>
> >> As far as I know we create an unique struct file for each DMA-buf.
> > Yes dma-buf, but that gets forwarded to the original drm chardev which
> > originally exported the buffer. It's only there where the forwarding
> > chain stops. The other thing is that iirc we have a singleton
> > anon_inode behind all the dma-buf, so they'd share all the same
> > address_space and so would all alias for unmap_mapping_range (I think
> > at least).
>
> Amdgpu works by using the address_space of the drm chardev into the
> struct file of DMA-buf instead.
>
> I think that this is cleaner, but only by a little bit :)
Yeah, but it doesn't work when forwarding from the drm chardev to the
dma-buf on the importer side, since you'd need a ton of different
address spaces. And you still rely on the core code picking up your
pgoff mangling, which feels about as risky to me as the vma file
pointer wrangling - if it's not consistently applied the reverse map
is toast and unmap_mapping_range doesn't work correctly for our needs.
> Anyway I'm a bit concerned that we have so many different approaches for
> the same problem.
Yeah, I think if we can standardize this then that would be really good.
-Daniel
>
> Christian.
>
> > -Daniel
> >
> >> Regards,
> >> Christian.
> >>
> >>
> >>> -Daniel
> >>>
> >>>>>> Hence why we'd like to be able to forward aliasing mappings and adjust the
> >>>>>> file and pgoff, while hopefully everything keeps working. I thought this
> >>>>>> would work, but Christian noticed it doesn't really.
> >>>>> It seems reasonable to me that the dma buf should be the owner of the
> >>>>> VMA, otherwise like you say, there is a big mess attaching the custom
> >>>>> vma ops and what not to the proper dma buf.
> >>>>>
> >>>>> I don't see anything obviously against this in mmap_region() - why did
> >>>>> Chritian notice it doesn't really work?
> >>>> To clarify I think this might work.
> >>>>
> >>>> I just had the same "Is that legal?", "What about security?", etc..
> >>>> questions you raised as well.
> >>>>
> >>>> It seems like a source of trouble so I thought better ask somebody more
> >>>> familiar with that.
> >>>>
> >>>> Christian.
> >>>>
> >>>>> Jason
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> [email protected]
> >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=02%7C01%7Cchristian.koenig%40amd.com%7Cf725d2eb6a5a49bd533f08d85ad23308%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637359206142262941&sdata=qcLsl9R1gP%2FGY39ctsQkIzI99Bn%2F840YS17F4xudrAE%3D&reserved=0
> >>>
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
> Yeah, but it doesn't work when forwarding from the drm chardev to the
> dma-buf on the importer side, since you'd need a ton of different
> address spaces. And you still rely on the core code picking up your
> pgoff mangling, which feels about as risky to me as the vma file
> pointer wrangling - if it's not consistently applied the reverse map
> is toast and unmap_mapping_range doesn't work correctly for our needs.
I would think the pgoff has to be translated at the same time the
vm->vm_file is changed?
The owner of the dma_buf should have one virtual address space and FD,
all its dma bufs should be linked to it, and all pgoffs translated to
that space.
Jason
Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
> On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
>
>> Yeah, but it doesn't work when forwarding from the drm chardev to the
>> dma-buf on the importer side, since you'd need a ton of different
>> address spaces. And you still rely on the core code picking up your
>> pgoff mangling, which feels about as risky to me as the vma file
>> pointer wrangling - if it's not consistently applied the reverse map
>> is toast and unmap_mapping_range doesn't work correctly for our needs.
> I would think the pgoff has to be translated at the same time the
> vm->vm_file is changed?
>
> The owner of the dma_buf should have one virtual address space and FD,
> all its dma bufs should be linked to it, and all pgoffs translated to
> that space.
Yeah, that is exactly like amdgpu is doing it.
Going to document that somehow when I'm done with TTM cleanups.
Christian.
>
> Jason
> _______________________________________________
> Linaro-mm-sig mailing list
> [email protected]
> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
> Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
> > On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
> >
> > > Yeah, but it doesn't work when forwarding from the drm chardev to the
> > > dma-buf on the importer side, since you'd need a ton of different
> > > address spaces. And you still rely on the core code picking up your
> > > pgoff mangling, which feels about as risky to me as the vma file
> > > pointer wrangling - if it's not consistently applied the reverse map
> > > is toast and unmap_mapping_range doesn't work correctly for our needs.
> > I would think the pgoff has to be translated at the same time the
> > vm->vm_file is changed?
> >
> > The owner of the dma_buf should have one virtual address space and FD,
> > all its dma bufs should be linked to it, and all pgoffs translated to
> > that space.
>
> Yeah, that is exactly like amdgpu is doing it.
>
> Going to document that somehow when I'm done with TTM cleanups.
BTW, while people are looking at this, is there a way to go from a VMA
to a dma_buf that owns it?
Jason
Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
> On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
>> Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
>>> On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
>>>
>>>> Yeah, but it doesn't work when forwarding from the drm chardev to the
>>>> dma-buf on the importer side, since you'd need a ton of different
>>>> address spaces. And you still rely on the core code picking up your
>>>> pgoff mangling, which feels about as risky to me as the vma file
>>>> pointer wrangling - if it's not consistently applied the reverse map
>>>> is toast and unmap_mapping_range doesn't work correctly for our needs.
>>> I would think the pgoff has to be translated at the same time the
>>> vm->vm_file is changed?
>>>
>>> The owner of the dma_buf should have one virtual address space and FD,
>>> all its dma bufs should be linked to it, and all pgoffs translated to
>>> that space.
>> Yeah, that is exactly like amdgpu is doing it.
>>
>> Going to document that somehow when I'm done with TTM cleanups.
> BTW, while people are looking at this, is there a way to go from a VMA
> to a dma_buf that owns it?
Only a driver specific one.
For TTM drivers vma->vm_private_data points to the buffer object. Not
sure about the drivers using GEM only.
Why are you asking?
Regards,
Christian.
>
> Jason
> _______________________________________________
> Linaro-mm-sig mailing list
> [email protected]
> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
On Thu, Sep 17, 2020 at 02:24:29PM +0200, Christian K?nig wrote:
> Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
> > On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian K?nig wrote:
> > > Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
> > > > On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
> > > >
> > > > > Yeah, but it doesn't work when forwarding from the drm chardev to the
> > > > > dma-buf on the importer side, since you'd need a ton of different
> > > > > address spaces. And you still rely on the core code picking up your
> > > > > pgoff mangling, which feels about as risky to me as the vma file
> > > > > pointer wrangling - if it's not consistently applied the reverse map
> > > > > is toast and unmap_mapping_range doesn't work correctly for our needs.
> > > > I would think the pgoff has to be translated at the same time the
> > > > vm->vm_file is changed?
> > > >
> > > > The owner of the dma_buf should have one virtual address space and FD,
> > > > all its dma bufs should be linked to it, and all pgoffs translated to
> > > > that space.
> > > Yeah, that is exactly like amdgpu is doing it.
> > >
> > > Going to document that somehow when I'm done with TTM cleanups.
> > BTW, while people are looking at this, is there a way to go from a VMA
> > to a dma_buf that owns it?
>
> Only a driver specific one.
>
> For TTM drivers vma->vm_private_data points to the buffer object. Not sure
> about the drivers using GEM only.
For gem drivers (which includes the ones using vram helpers, which uses
ttm internally) that points at the drm_gem_object pointer. I guess that
might be something we can unify a bit on the ttm mmap paths of the
remaining driver, now that there's a drm_gem_object embedded anyway.
-Daniel
>
> Why are you asking?
>
> Regards,
> Christian.
>
> >
> > Jason
> > _______________________________________________
> > Linaro-mm-sig mailing list
> > [email protected]
> > https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Thu, Sep 17, 2020 at 02:24:29PM +0200, Christian König wrote:
> Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
> > On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
> > > Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
> > > > On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
> > > >
> > > > > Yeah, but it doesn't work when forwarding from the drm chardev to the
> > > > > dma-buf on the importer side, since you'd need a ton of different
> > > > > address spaces. And you still rely on the core code picking up your
> > > > > pgoff mangling, which feels about as risky to me as the vma file
> > > > > pointer wrangling - if it's not consistently applied the reverse map
> > > > > is toast and unmap_mapping_range doesn't work correctly for our needs.
> > > > I would think the pgoff has to be translated at the same time the
> > > > vm->vm_file is changed?
> > > >
> > > > The owner of the dma_buf should have one virtual address space and FD,
> > > > all its dma bufs should be linked to it, and all pgoffs translated to
> > > > that space.
> > > Yeah, that is exactly like amdgpu is doing it.
> > >
> > > Going to document that somehow when I'm done with TTM cleanups.
> > BTW, while people are looking at this, is there a way to go from a VMA
> > to a dma_buf that owns it?
>
> Only a driver specific one.
Sounds OK
> For TTM drivers vma->vm_private_data points to the buffer object. Not sure
> about the drivers using GEM only.
Why are drivers in control of the vma? I would think dma_buf should be
the vma owner. IIRC module lifetime correctness essentially hings on
the module owner of the struct file
> Why are you asking?
I'm thinking about using find_vma on something that is not
get_user_pages()'able to go to the underlying object, in this case dma
buf.
So, user VA -> find_vma -> dma_buf object -> dma_buf operations on the
memory it represents
Jason
Am 17.09.20 um 16:35 schrieb Jason Gunthorpe:
> On Thu, Sep 17, 2020 at 02:24:29PM +0200, Christian König wrote:
>> Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
>>> On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
>>>> Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
>>>>> On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
>>>>>
>>>>>> Yeah, but it doesn't work when forwarding from the drm chardev to the
>>>>>> dma-buf on the importer side, since you'd need a ton of different
>>>>>> address spaces. And you still rely on the core code picking up your
>>>>>> pgoff mangling, which feels about as risky to me as the vma file
>>>>>> pointer wrangling - if it's not consistently applied the reverse map
>>>>>> is toast and unmap_mapping_range doesn't work correctly for our needs.
>>>>> I would think the pgoff has to be translated at the same time the
>>>>> vm->vm_file is changed?
>>>>>
>>>>> The owner of the dma_buf should have one virtual address space and FD,
>>>>> all its dma bufs should be linked to it, and all pgoffs translated to
>>>>> that space.
>>>> Yeah, that is exactly like amdgpu is doing it.
>>>>
>>>> Going to document that somehow when I'm done with TTM cleanups.
>>> BTW, while people are looking at this, is there a way to go from a VMA
>>> to a dma_buf that owns it?
>> Only a driver specific one.
> Sounds OK
>
>> For TTM drivers vma->vm_private_data points to the buffer object. Not sure
>> about the drivers using GEM only.
> Why are drivers in control of the vma? I would think dma_buf should be
> the vma owner. IIRC module lifetime correctness essentially hings on
> the module owner of the struct file
Because the page fault handling is completely driver specific.
We could install some DMA-buf vmops, but that would just be another
layer of redirection.
>> Why are you asking?
> I'm thinking about using find_vma on something that is not
> get_user_pages()'able to go to the underlying object, in this case dma
> buf.
>
> So, user VA -> find_vma -> dma_buf object -> dma_buf operations on the
> memory it represents
Ah, yes we are already doing this in amdgpu as well. But only for
DMA-bufs or more generally buffers which are mmaped by this driver instance.
Some applications are braindead enough to mmap() a buffer and then give
us back the CPU pointer and request to make it a handle (userptr) again.
That is clearly forbidden by OpenGL, OpenCL and Vulkan, but they use it
anyway.
Christian.
>
> Jason
> _______________________________________________
> Linaro-mm-sig mailing list
> [email protected]
> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig
On Thu, Sep 17, 2020 at 04:54:44PM +0200, Christian König wrote:
> Am 17.09.20 um 16:35 schrieb Jason Gunthorpe:
> > On Thu, Sep 17, 2020 at 02:24:29PM +0200, Christian König wrote:
> > > Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
> > > > On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
> > > > > Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
> > > > > > On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
> > > > > >
> > > > > > > Yeah, but it doesn't work when forwarding from the drm chardev to the
> > > > > > > dma-buf on the importer side, since you'd need a ton of different
> > > > > > > address spaces. And you still rely on the core code picking up your
> > > > > > > pgoff mangling, which feels about as risky to me as the vma file
> > > > > > > pointer wrangling - if it's not consistently applied the reverse map
> > > > > > > is toast and unmap_mapping_range doesn't work correctly for our needs.
> > > > > > I would think the pgoff has to be translated at the same time the
> > > > > > vm->vm_file is changed?
> > > > > >
> > > > > > The owner of the dma_buf should have one virtual address space and FD,
> > > > > > all its dma bufs should be linked to it, and all pgoffs translated to
> > > > > > that space.
> > > > > Yeah, that is exactly like amdgpu is doing it.
> > > > >
> > > > > Going to document that somehow when I'm done with TTM cleanups.
> > > > BTW, while people are looking at this, is there a way to go from a VMA
> > > > to a dma_buf that owns it?
> > > Only a driver specific one.
> > Sounds OK
> >
> > > For TTM drivers vma->vm_private_data points to the buffer object. Not sure
> > > about the drivers using GEM only.
> > Why are drivers in control of the vma? I would think dma_buf should be
> > the vma owner. IIRC module lifetime correctness essentially hings on
> > the module owner of the struct file
>
> Because the page fault handling is completely driver specific.
>
> We could install some DMA-buf vmops, but that would just be another layer of
> redirection.
If it is already taking a page fault I'm not sure the extra function
call indirection is going to be a big deal. Having a uniform VMA
sounds saner than every driver custom rolling something.
When I unwound a similar mess in RDMA all the custom VMA stuff in the
drivers turned out to be generally buggy, at least.
Is vma->vm_file->private_data universally a dma_buf pointer at least?
> > So, user VA -> find_vma -> dma_buf object -> dma_buf operations on the
> > memory it represents
>
> Ah, yes we are already doing this in amdgpu as well. But only for DMA-bufs
> or more generally buffers which are mmaped by this driver instance.
So there is no general dma_buf service? That is a real bummer
Jason
On Thu, Sep 17, 2020 at 5:24 PM Jason Gunthorpe <[email protected]> wrote:
>
> On Thu, Sep 17, 2020 at 04:54:44PM +0200, Christian König wrote:
> > Am 17.09.20 um 16:35 schrieb Jason Gunthorpe:
> > > On Thu, Sep 17, 2020 at 02:24:29PM +0200, Christian König wrote:
> > > > Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
> > > > > On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
> > > > > > Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
> > > > > > > On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
> > > > > > >
> > > > > > > > Yeah, but it doesn't work when forwarding from the drm chardev to the
> > > > > > > > dma-buf on the importer side, since you'd need a ton of different
> > > > > > > > address spaces. And you still rely on the core code picking up your
> > > > > > > > pgoff mangling, which feels about as risky to me as the vma file
> > > > > > > > pointer wrangling - if it's not consistently applied the reverse map
> > > > > > > > is toast and unmap_mapping_range doesn't work correctly for our needs.
> > > > > > > I would think the pgoff has to be translated at the same time the
> > > > > > > vm->vm_file is changed?
> > > > > > >
> > > > > > > The owner of the dma_buf should have one virtual address space and FD,
> > > > > > > all its dma bufs should be linked to it, and all pgoffs translated to
> > > > > > > that space.
> > > > > > Yeah, that is exactly like amdgpu is doing it.
> > > > > >
> > > > > > Going to document that somehow when I'm done with TTM cleanups.
> > > > > BTW, while people are looking at this, is there a way to go from a VMA
> > > > > to a dma_buf that owns it?
> > > > Only a driver specific one.
> > > Sounds OK
> > >
> > > > For TTM drivers vma->vm_private_data points to the buffer object. Not sure
> > > > about the drivers using GEM only.
> > > Why are drivers in control of the vma? I would think dma_buf should be
> > > the vma owner. IIRC module lifetime correctness essentially hings on
> > > the module owner of the struct file
> >
> > Because the page fault handling is completely driver specific.
> >
> > We could install some DMA-buf vmops, but that would just be another layer of
> > redirection.
Uh geez I didn't know amdgpu was doing that :-/
Since this is on, I guess the inverse of trying to convert a userptr
into a dma-buf is properly rejected?
> If it is already taking a page fault I'm not sure the extra function
> call indirection is going to be a big deal. Having a uniform VMA
> sounds saner than every driver custom rolling something.
>
> When I unwound a similar mess in RDMA all the custom VMA stuff in the
> drivers turned out to be generally buggy, at least.
>
> Is vma->vm_file->private_data universally a dma_buf pointer at least?
Nope. I think if you want this without some large scale rewrite of a
lot of code we'd need a vmops->get_dmabuf or similar. Not pretty, but
would get the job done.
> > > So, user VA -> find_vma -> dma_buf object -> dma_buf operations on the
> > > memory it represents
> >
> > Ah, yes we are already doing this in amdgpu as well. But only for DMA-bufs
> > or more generally buffers which are mmaped by this driver instance.
>
> So there is no general dma_buf service? That is a real bummer
Mostly historical reasons and "it's complicated". One problem is that
dma-buf isn't a powerful enough interface that drivers could use it
for all their native objects, e.g. userptr doesn't pass through it,
and clever cache flushing tricks aren't allowed and a bunch of other
things. So there's some serious roadblocks before we could have a
common allocator (or set of allocators) behind dma-buf.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Am 17.09.20 um 17:37 schrieb Daniel Vetter:
> On Thu, Sep 17, 2020 at 5:24 PM Jason Gunthorpe <[email protected]> wrote:
>> On Thu, Sep 17, 2020 at 04:54:44PM +0200, Christian König wrote:
>>> Am 17.09.20 um 16:35 schrieb Jason Gunthorpe:
>>>> On Thu, Sep 17, 2020 at 02:24:29PM +0200, Christian König wrote:
>>>>> Am 17.09.20 um 14:18 schrieb Jason Gunthorpe:
>>>>>> On Thu, Sep 17, 2020 at 02:03:48PM +0200, Christian König wrote:
>>>>>>> Am 17.09.20 um 13:31 schrieb Jason Gunthorpe:
>>>>>>>> On Thu, Sep 17, 2020 at 10:09:12AM +0200, Daniel Vetter wrote:
>>>>>>>>
>>>>>>>>> Yeah, but it doesn't work when forwarding from the drm chardev to the
>>>>>>>>> dma-buf on the importer side, since you'd need a ton of different
>>>>>>>>> address spaces. And you still rely on the core code picking up your
>>>>>>>>> pgoff mangling, which feels about as risky to me as the vma file
>>>>>>>>> pointer wrangling - if it's not consistently applied the reverse map
>>>>>>>>> is toast and unmap_mapping_range doesn't work correctly for our needs.
>>>>>>>> I would think the pgoff has to be translated at the same time the
>>>>>>>> vm->vm_file is changed?
>>>>>>>>
>>>>>>>> The owner of the dma_buf should have one virtual address space and FD,
>>>>>>>> all its dma bufs should be linked to it, and all pgoffs translated to
>>>>>>>> that space.
>>>>>>> Yeah, that is exactly like amdgpu is doing it.
>>>>>>>
>>>>>>> Going to document that somehow when I'm done with TTM cleanups.
>>>>>> BTW, while people are looking at this, is there a way to go from a VMA
>>>>>> to a dma_buf that owns it?
>>>>> Only a driver specific one.
>>>> Sounds OK
>>>>
>>>>> For TTM drivers vma->vm_private_data points to the buffer object. Not sure
>>>>> about the drivers using GEM only.
>>>> Why are drivers in control of the vma? I would think dma_buf should be
>>>> the vma owner. IIRC module lifetime correctness essentially hings on
>>>> the module owner of the struct file
>>> Because the page fault handling is completely driver specific.
>>>
>>> We could install some DMA-buf vmops, but that would just be another layer of
>>> redirection.
> Uh geez I didn't know amdgpu was doing that :-/
>
> Since this is on, I guess the inverse of trying to convert a userptr
> into a dma-buf is properly rejected?
My fault, I wasn't specific enough in my description :)
Amdgpu is NOT doing this with mmaped DMA-bufs, but rather with it's own
mmaped BOs.
In other words when userspace call the userptr IOCTL and we get an error
because we can't make an userptr from some random device memory we
instead check all CPU mappings if the application was brain dead enough
to provide us our own pointer back.
IIRC this is even done in userspace and not the kernel. But we talked
about doing it in the kernel with the private_data as well.
>
>> If it is already taking a page fault I'm not sure the extra function
>> call indirection is going to be a big deal. Having a uniform VMA
>> sounds saner than every driver custom rolling something.
>>
>> When I unwound a similar mess in RDMA all the custom VMA stuff in the
>> drivers turned out to be generally buggy, at least.
>>
>> Is vma->vm_file->private_data universally a dma_buf pointer at least?
> Nope. I think if you want this without some large scale rewrite of a
> lot of code we'd need a vmops->get_dmabuf or similar. Not pretty, but
> would get the job done.
Yeah, agree that sounds like the simplest approach.
Regards,
Christian.
>
>>>> So, user VA -> find_vma -> dma_buf object -> dma_buf operations on the
>>>> memory it represents
>>> Ah, yes we are already doing this in amdgpu as well. But only for DMA-bufs
>>> or more generally buffers which are mmaped by this driver instance.
>> So there is no general dma_buf service? That is a real bummer
> Mostly historical reasons and "it's complicated". One problem is that
> dma-buf isn't a powerful enough interface that drivers could use it
> for all their native objects, e.g. userptr doesn't pass through it,
> and clever cache flushing tricks aren't allowed and a bunch of other
> things. So there's some serious roadblocks before we could have a
> common allocator (or set of allocators) behind dma-buf.
> -Daniel
On Thu, Sep 17, 2020 at 06:06:14PM +0200, Christian König wrote:
> > > If it is already taking a page fault I'm not sure the extra function
> > > call indirection is going to be a big deal. Having a uniform VMA
> > > sounds saner than every driver custom rolling something.
> > >
> > > When I unwound a similar mess in RDMA all the custom VMA stuff in the
> > > drivers turned out to be generally buggy, at least.
> > >
> > > Is vma->vm_file->private_data universally a dma_buf pointer at least?
> > Nope. I think if you want this without some large scale rewrite of a
> > lot of code we'd need a vmops->get_dmabuf or similar. Not pretty, but
> > would get the job done.
>
> Yeah, agree that sounds like the simplest approach.
I don't think that will fly, it is clearly only papering over a mess
inside DRM/dma buf :\
Jason
On Thu, Sep 17, 2020 at 6:39 PM Jason Gunthorpe <[email protected]> wrote:
>
> On Thu, Sep 17, 2020 at 06:06:14PM +0200, Christian König wrote:
> > > > If it is already taking a page fault I'm not sure the extra function
> > > > call indirection is going to be a big deal. Having a uniform VMA
> > > > sounds saner than every driver custom rolling something.
> > > >
> > > > When I unwound a similar mess in RDMA all the custom VMA stuff in the
> > > > drivers turned out to be generally buggy, at least.
> > > >
> > > > Is vma->vm_file->private_data universally a dma_buf pointer at least?
> > > Nope. I think if you want this without some large scale rewrite of a
> > > lot of code we'd need a vmops->get_dmabuf or similar. Not pretty, but
> > > would get the job done.
> >
> > Yeah, agree that sounds like the simplest approach.
>
> I don't think that will fly, it is clearly only papering over a mess
> inside DRM/dma buf :\
dma-buf started out as something to paper over the disjoint mess of
allocators, since it was pretty clear to anyone involved we're not
going to unify them anytime soon, if ever. So the mess pretty much is
bound to stay.
I think most reasonable thing would be to throw a common vmops in
there somehow, but even that means some large scale surgery across
every driver/subsystem involved in dma-buf. It wouldn't unify
anything, all it would give you is a constant vma->vm_ops to do some
kind of upcasting. And that would still only give you a slightly less
opaque pointer with a callback to upcast to a dma-buf in some
driver/subsystem specific way.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch