| Message ID | 20191023222226.9064-1-robh@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | drm/prime: Fix mmap fake offset for drm_gem_object_funcs.mmap | expand |
On Wed, Oct 23, 2019 at 05:22:26PM -0500, Rob Herring wrote: > Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > introduced a GEM object mmap() hook which is expected to subtract the > fake offset from vm_pgoff. Long-term it is probably a good idea to just remove the fake offset handling from drivers. But that'll only work once all drivers switched away from custom fops->mmap handlers so we can handle the offset -> obj lookup in the drm core for everybody. So let's go this way for now. Acked-by: Gerd Hoffmann <kraxel@redhat.com> > However, for mmap() on dmabufs, there is not > a fake offset. To fix this, we need to add the fake offset just like the > driver->fops->mmap() code path. > > Fixes: c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > I ran into this while working on converting vgem to shmem helpers and > the IGT vgem_basic dmabuf-mmap test failed. This fixes shmem, but I > have checked any other users of the new mmap hook. > Rob > > drivers/gpu/drm/drm_prime.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 0814211b0f3f..5d06690a2e9d 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -713,6 +713,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > struct file *fil; > int ret; > > + vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > + > if (obj->funcs && obj->funcs->mmap) { > ret = obj->funcs->mmap(obj, vma); > if (ret) > @@ -737,8 +739,6 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > if (ret) > goto out; > > - vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > - > ret = obj->dev->driver->fops->mmap(fil, vma); > > drm_vma_node_revoke(&obj->vma_node, priv); > -- > 2.20.1 >
On Thu, Oct 24, 2019 at 11:02:40AM +0200, Gerd Hoffmann wrote: > On Wed, Oct 23, 2019 at 05:22:26PM -0500, Rob Herring wrote: > > Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > > introduced a GEM object mmap() hook which is expected to subtract the > > fake offset from vm_pgoff. > > Long-term it is probably a good idea to just remove the fake offset > handling from drivers. But that'll only work once all drivers switched > away from custom fops->mmap handlers so we can handle the offset -> obj > lookup in the drm core for everybody. > > So let's go this way for now. > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> Uh this sounds like doubling down on rather horrible semantics. Can we at least stop the mess instead of baking it in for real? The hook is very very new after all. I.e. - Document that obj->funcs->mmap will have 0 offset in the kerneldoc. - Remove the subtracting from the shmem helper - In ttm_bo_mmap_obj re-add the offset with a huge FIXME comment. - Adjust drm_gem_mmap_obj to do that same for obj->funcs->mmap and also document the expectation there too. This feels like very much going the wrong direction ... Also I guess Gerd didn't really test this prime mmap support? Thanks, Daniel > > > However, for mmap() on dmabufs, there is not > > a fake offset. To fix this, we need to add the fake offset just like the > > driver->fops->mmap() code path. > > > > Fixes: c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Signed-off-by: Rob Herring <robh@kernel.org> > > --- > > I ran into this while working on converting vgem to shmem helpers and > > the IGT vgem_basic dmabuf-mmap test failed. This fixes shmem, but I > > have checked any other users of the new mmap hook. > > Rob > > > > drivers/gpu/drm/drm_prime.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > > index 0814211b0f3f..5d06690a2e9d 100644 > > --- a/drivers/gpu/drm/drm_prime.c > > +++ b/drivers/gpu/drm/drm_prime.c > > @@ -713,6 +713,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > struct file *fil; > > int ret; > > > > + vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > > + > > if (obj->funcs && obj->funcs->mmap) { > > ret = obj->funcs->mmap(obj, vma); > > if (ret) > > @@ -737,8 +739,6 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > if (ret) > > goto out; > > > > - vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); > > - > > ret = obj->dev->driver->fops->mmap(fil, vma); > > > > drm_vma_node_revoke(&obj->vma_node, priv); > > -- > > 2.20.1 > > >
On Thu, Oct 24, 2019 at 7:32 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Oct 24, 2019 at 11:02:40AM +0200, Gerd Hoffmann wrote: > > On Wed, Oct 23, 2019 at 05:22:26PM -0500, Rob Herring wrote: > > > Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > > > introduced a GEM object mmap() hook which is expected to subtract the > > > fake offset from vm_pgoff. > > > > Long-term it is probably a good idea to just remove the fake offset > > handling from drivers. But that'll only work once all drivers switched > > away from custom fops->mmap handlers so we can handle the offset -> obj > > lookup in the drm core for everybody. > > > > So let's go this way for now. > > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > Uh this sounds like doubling down on rather horrible semantics. Can we at > least stop the mess instead of baking it in for real? The hook is very > very new after all. I.e. > - Document that obj->funcs->mmap will have 0 offset in the kerneldoc. > - Remove the subtracting from the shmem helper > - In ttm_bo_mmap_obj re-add the offset with a huge FIXME comment. > - Adjust drm_gem_mmap_obj to do that same for obj->funcs->mmap and also > document the expectation there too. Okay. > This feels like very much going the wrong direction ... > > Also I guess Gerd didn't really test this prime mmap support? Perhaps because at least parts of the IGT "vgem" tests really have nothing specific for "vgem" and there doesn't seem to be another test case that does run doing the same thing. And none of the IGT prime tests run without an Intel driver. Looking at IGT always makes me sad, and then I move on to other things... </rant> BTW, are there IGT test results for vgem/vkms somewhere? I didn't have any luck finding anything. Rob
On Thu, Oct 24, 2019 at 4:39 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Oct 24, 2019 at 7:32 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > > > On Thu, Oct 24, 2019 at 11:02:40AM +0200, Gerd Hoffmann wrote: > > > On Wed, Oct 23, 2019 at 05:22:26PM -0500, Rob Herring wrote: > > > > Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > > > > introduced a GEM object mmap() hook which is expected to subtract the > > > > fake offset from vm_pgoff. > > > > > > Long-term it is probably a good idea to just remove the fake offset > > > handling from drivers. But that'll only work once all drivers switched > > > away from custom fops->mmap handlers so we can handle the offset -> obj > > > lookup in the drm core for everybody. > > > > > > So let's go this way for now. > > > > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > > > Uh this sounds like doubling down on rather horrible semantics. Can we at > > least stop the mess instead of baking it in for real? The hook is very > > very new after all. I.e. > > - Document that obj->funcs->mmap will have 0 offset in the kerneldoc. > > - Remove the subtracting from the shmem helper > > - In ttm_bo_mmap_obj re-add the offset with a huge FIXME comment. > > - Adjust drm_gem_mmap_obj to do that same for obj->funcs->mmap and also > > document the expectation there too. > > Okay. > > > This feels like very much going the wrong direction ... > > > > Also I guess Gerd didn't really test this prime mmap support? > > Perhaps because at least parts of the IGT "vgem" tests really have > nothing specific for "vgem" and there doesn't seem to be another test > case that does run doing the same thing. And none of the IGT prime > tests run without an Intel driver. Looking at IGT always makes me sad, > and then I move on to other things... </rant> The only prime test that could be made generic is the kms one, which still requires crc to make sure the stuff actually works. Everything else is kinda device specific (and vgem is just the test vehicle to make writing a testcase possible without actually needing 2 gpus). Maybe we could do a dumb buffer export, then mmap test, but that doesn't exist. I think for the prime+kms one there's actually patches floating around to make it generic. For the others I really don't think you can make them much generic. > BTW, are there IGT test results for vgem/vkms somewhere? I didn't have > any luck finding anything. Unfortunately not (yet). I have this dream that once we have the kernels on gitlab I'll make igt runs on a qemu run with vkms+vgem only happen, as the first baseline. But we're some ways off from that still :-/ -Daniel
On Thu, Oct 24, 2019 at 02:32:14PM +0200, Daniel Vetter wrote: > On Thu, Oct 24, 2019 at 11:02:40AM +0200, Gerd Hoffmann wrote: > > On Wed, Oct 23, 2019 at 05:22:26PM -0500, Rob Herring wrote: > > > Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") > > > introduced a GEM object mmap() hook which is expected to subtract the > > > fake offset from vm_pgoff. > > > > Long-term it is probably a good idea to just remove the fake offset > > handling from drivers. But that'll only work once all drivers switched > > away from custom fops->mmap handlers so we can handle the offset -> obj > > lookup in the drm core for everybody. > > > > So let's go this way for now. > > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > Uh this sounds like doubling down on rather horrible semantics. Can we at > least stop the mess instead of baking it in for real? The hook is very > very new after all. I.e. > - Document that obj->funcs->mmap will have 0 offset in the kerneldoc. > - Remove the subtracting from the shmem helper > - In ttm_bo_mmap_obj re-add the offset with a huge FIXME comment. Ah, right, we can also tweak the drivers which need it instead of doing it for everybody in drm_gem_mmap_obj(). > Also I guess Gerd didn't really test this prime mmap support? I did, but I think the test[1] is too simple. It'll only try to mmap() the dma-buf, but doesn't touch the pages, so it doesn't see the shmem fault callback throwing SIGBUS. cheers, Gerd [1] https://git.kraxel.org/cgit/drminfo/tree/prime.c
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0814211b0f3f..5d06690a2e9d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -713,6 +713,8 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) struct file *fil; int ret; + vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); + if (obj->funcs && obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) @@ -737,8 +739,6 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) if (ret) goto out; - vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); - ret = obj->dev->driver->fops->mmap(fil, vma); drm_vma_node_revoke(&obj->vma_node, priv);
Commit c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") introduced a GEM object mmap() hook which is expected to subtract the fake offset from vm_pgoff. However, for mmap() on dmabufs, there is not a fake offset. To fix this, we need to add the fake offset just like the driver->fops->mmap() code path. Fixes: c40069cb7bd6 ("drm: add mmap() to drm_gem_object_funcs") Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Rob Herring <robh@kernel.org> --- I ran into this while working on converting vgem to shmem helpers and the IGT vgem_basic dmabuf-mmap test failed. This fixes shmem, but I have checked any other users of the new mmap hook. Rob drivers/gpu/drm/drm_prime.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)