On Fri, Nov 08, 2019 at 05:55:28PM +0100, Daniel Vetter wrote:
On Fri, Nov 08, 2019 at 05:27:59PM +0100, Daniel Vetter wrote:
On Fri, Oct 25, 2019 at 09:30:42AM +0200, Daniel Vetter wrote:
On Thu, Oct 24, 2019 at 02:18:59PM -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. However, for mmap() on dmabufs, there is not a fake offset.
To fix this, let's always call mmap() object callback with an offset of 0, and leave it up to drm_gem_mmap_obj() to remove the fake offset.
TTM still needs the fake offset, so we have to add it back until that's fixed.
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
v2:
- Move subtracting the fake offset out of mmap() obj callbacks.
I've tested shmem, but not ttm. Hopefully, I understood what's needed for TTM.
So unfortunately I'm already having regrets on this. We might even have broken some of the ttm drivers here.
Trouble is, if you need to shoot down userspace ptes of a bo (because it's getting moved or whatever), then we do that with unmap_mapping_range. Which means each bo needs to be mapping with a unique (offset, adress_space), or it won't work. By remapping all the bo to 0 we've broken this. We've also had this broken this for a while for the simplistic dma-buf mmap, since without any further action we'll reuse the address space of the dma-buf inode, not of the drm_device.
Strangely both etnaviv and msm have a comment to that effect - grep for unmap_mapping_range. But neither actually uses it.
Not exactly sure what's the best course of action here now.
Also adding Thomas Zimmermann, who's worked on all the vram helpers.
Correction, I missed the unmap_mapping_range in the vma node manager header, so didn't spot the drivers using drm_vma_node_unmap. We did break all the ttm stuff :-/
ttm still uses the offset, now added in ttm_bo_mmap_obj(). So, for normal mmap behavior did not change I think. The simplistic dma-buf mmap did change, it now uses the offset because it goes through ttm_bo_mmap_obj() too.
Not fully sure which address space is used for dma-bufs though. As far I can see neither the old nor the new dma-buf mmap code path touch vma->vm_file (unless the driver does explicitly care, like msm does in msm_gem_mmap_obj).
Plus panfrost, which is using drm_gem_shmem_purge_locked.
Hmm, looking at drm_gem_shmem_purge_locked I'm wondering why it uses a mix of dev->anon_inode->i_mapping and file_inode(obj->filp)->i_mapping.
Also shmem helpers used a zero vm_pgoff before, only difference is the change is applied in drm_gem_mmap_obj() now instead of somewhere in the shmem helper code.
slightly confused, Gerd