Hi Adrian, Sorry for the delayed response, this got buried in my inbox... >-----Original Message----- >From: Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx> >Sent: Monday, June 6, 2022 4:20 PM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx> >Cc: daniel@xxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Subject: Re: [RFC PATCH v2 1/1] drm/i915: Replace shmem memory >region and object backend with TTM > >On 01.06.2022 19:43, Ruhl, Michael J wrote: >>>-----Original Message----- >>>From: Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx> >>>Sent: Friday, May 27, 2022 12:08 PM >>>To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx> >>>Cc: daniel@xxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>Subject: Re: [RFC PATCH v2 1/1] drm/i915: Replace shmem >memory >>>region and object backend with TTM >>> >>>On 17.05.2022 21:39, Ruhl, Michael J wrote: >>>> >-----Original Message----- >>>> >From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of >>>> >Adrian Larumbe >>>> >Sent: Tuesday, May 17, 2022 4:45 PM >>>> >To: daniel@xxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>>> >Cc: adrian.larumbe@xxxxxxxxxxxxx >>>> >Subject: [RFC PATCH v2 1/1] drm/i915: Replace shmem >memory >>>> >region and object backend with TTM >>>> > >>>> >Remove shmem region and object backend code as they are now >>>> >unnecessary. >>>> >In the case of objects placed in SMEM and backed by kernel shmem >files, >>>the >>>> >file pointer has to be retrieved from the ttm_tt structure, so change this >>>> >as well. This means accessing an shmem-backed BO's file pointer >requires >>>> >getting its pages beforehand, unlike in the old shmem backend. >>>> > >>>> >Expand TTM BO creation by handling devices with no LLC so that their >>>> >caching and coherency properties are set accordingly. >>>> > >>>> >Adapt phys backend to put pages of original shmem object in a 'TTM >way', >>>> >also making sure its pages are put when a TTM shmem object has no >struct >>>> >page. >>>> > >>>> >Signed-off-by: Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx> >>>> >--- >>>> > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 12 +- >>>> > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 32 +- >>>> > drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 +- >>>> > drivers/gpu/drm/i915/gem/i915_gem_phys.c | 32 +- >>>> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 390 +------------------ >>>> > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 267 ++++++++++++- >>>> > drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 3 + >>>> > drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 9 +- >>>> > drivers/gpu/drm/i915/gt/shmem_utils.c | 64 ++- >>>> > drivers/gpu/drm/i915/intel_memory_region.c | 7 +- >>>> > 10 files changed, 398 insertions(+), 422 deletions(-) >>>> > >>>> >diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>>> >b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>>> >index f5062d0c6333..de04ce4210b3 100644 >>>> >--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>>> >+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>>> >@@ -12,6 +12,7 @@ >>>> > #include <asm/smp.h> >>>> > >>>> > #include "gem/i915_gem_dmabuf.h" >>>> >+#include "gem/i915_gem_ttm.h" >>>> > #include "i915_drv.h" >>>> > #include "i915_gem_object.h" >>>> > #include "i915_scatterlist.h" >>>> >@@ -94,22 +95,25 @@ static int i915_gem_dmabuf_mmap(struct >dma_buf >>>> >*dma_buf, struct vm_area_struct * >>>> > { >>>> > struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); >>>> > struct drm_i915_private *i915 = to_i915(obj->base.dev); >>>> >+ struct file *filp = i915_gem_ttm_get_filep(obj); >>>> > int ret; >>>> > >>>> >+ GEM_BUG_ON(obj->base.import_attach != NULL); >>>> >+ >>>> > if (obj->base.size < vma->vm_end - vma->vm_start) >>>> > return -EINVAL; >>>> > >>>> > if (HAS_LMEM(i915)) >>>> > return drm_gem_prime_mmap(&obj->base, vma); >>>> >>>> Since all of the devices that will have device memory will be true for >>>HAS_LMEM, >>>> won't your code path always go to drm_gem_prime_mmap()? >>> >>>This makes me wonder, how was mapping of a dmabuf BO working before, >in >>>the case >>>of DG2 and DG1, when an object is smem-bound, and therefore backed by >>>shmem? >> >>LMEM is a new thing. DG2 and DG1 have it available, but the initial patch >>sets did not support access via dma-buf. >> >>With the TTM being used for the LMEM objects, I think that the drm_gem >code >>was able to be used. >> >>> >>>I guess in this case it might be more sensible to control for the case that it's >>>an lmem-only object on a discrete platform as follows: >>> >>>static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct >>>vm_area_struct *vma) >>>{ >>> struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf); >>> struct drm_i915_private *i915 = to_i915(obj->base.dev); >>> struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); >>> struct file *filp = i915_gem_ttm_get_filep(obj); >>> int ret; >>> >>> if (obj->base.size < vma->vm_end - vma->vm_start) >>> return -EINVAL; >>> >>> if (IS_DGFX(i915) && i915_ttm_cpu_maps_iomem(bo->resource)) >>> return drm_gem_prime_mmap(&obj->base, vma); >> >>HAS_LMEM is roughly IS_DGFX... >> >>As for the second part, (_maps_iomem), hmm... >> >>If I am following this correctly drm_gem_prime_mmap() will call >i915_gem_mmap, >>so I think that just the call (without the iomem check) is correct. >> >>This is a newer mmap code path than the older integrated cards had, so I >think that >>the context is HAS_LMEM (new and discrete), otherwise the old path. >> >>> if (IS_ERR(filp)) >>> return PTR_ERR(filp); >>> >>> ret = call_mmap(filp, vma); >>> if (ret) >>> return ret; >>> >>> vma_set_file(vma, filp); >>> >>> return 0; >>>} >>> >>>> >- if (!obj->base.filp) >>>> >+ if (!filp) >>>> > return -ENODEV; >>>> > >>>> >- ret = call_mmap(obj->base.filp, vma); >>>> >+ ret = call_mmap(filp, vma); >>>> > if (ret) >>>> > return ret; >>>> > >>>> >- vma_set_file(vma, obj->base.filp); >>>> >+ vma_set_file(vma, filp); >>>> > >>>> > return 0; >>>> > } >>>> >@@ -224,6 +228,8 @@ struct dma_buf *i915_gem_prime_export(struct >>>> >drm_gem_object *gem_obj, int flags) >>>> > exp_info.priv = gem_obj; >>>> > exp_info.resv = obj->base.resv; >>>> > >>>> >+ GEM_BUG_ON(obj->base.import_attach != NULL); >>>> >+ >>>> > if (obj->ops->dmabuf_export) { >>>> > int ret = obj->ops->dmabuf_export(obj); >>>> > if (ret) >>>> >diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> >b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> >index 0c5c43852e24..d963cf35fdc9 100644 >>>> >--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> >+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >>>> >@@ -64,7 +64,9 @@ i915_gem_mmap_ioctl(struct drm_device *dev, >void >>>> >*data, >>>> > struct drm_i915_private *i915 = to_i915(dev); >>>> > struct drm_i915_gem_mmap *args = data; >>>> > struct drm_i915_gem_object *obj; >>>> >+ struct file *filp; >>>> > unsigned long addr; >>>> >+ int ret; >>>> > >>>> > /* >>>> > * mmap ioctl is disallowed for all discrete platforms, >>>> >@@ -83,12 +85,20 @@ i915_gem_mmap_ioctl(struct drm_device *dev, >>>void >>>> >*data, >>>> > if (!obj) >>>> > return -ENOENT; >>>> > >>>> >- /* prime objects have no backing filp to GEM mmap >>>> >- * pages from. >>>> >- */ >>>> >- if (!obj->base.filp) { >>>> >- addr = -ENXIO; >>>> >- goto err; >>>> >+ if (obj->base.import_attach) >>>> >+ filp = obj->base.filp; >>>> >>>> Why is this now ok? This is the imported object. And it used to give an >error. >>>> >>>> If you are importing from a different device, (i.e. an amd device is >exporting >>>> the object, and you are i915 here), can you even do this? >>>> >>>> My understanding was that mmaping was only for the exported object. >>>> >>>> Has this changed? >>> >>>You're right here, I completely misunderstood how this function is meant to >>>deal >>>with imported objects. This arose as a consequence of the file pointer >being >>>moved into the ttm_tt structure from the base DRM object. >> >>Hmm, one other thing that I am seeing here for i915_gem_mmap_ioctl: >> >> /* >> * mmap ioctl is disallowed for all discrete platforms, >> * and for all platforms with GRAPHICS_VER > 12. >> */ >> if (IS_DGFX(i915) || GRAPHICS_VER_FULL(i915) > IP_VER(12, 0)) >> return -EOPNOTSUPP; >> >>You need to use the i915_gem_mmap_offset_ioctl() to do mmap with the >discrete >>(i.e. devices with LMEM). >> >>So are we looking at the right code path? >> >>>The way I now check for the case that it's an imported object and therefore >>>this >>>function should throw back an error is as follows: >>> >>>/* prime objects have no backing filp to GEM mmap >>> * pages from. >>> */ >>>if (obj->base.import_attach) { >>> GEM_WARN_ON(obj->base.filp != NULL); >>> addr = -ENXIO; >>> goto err; >>>} >>> >>>I believe the import_attach member has to be set for all imported >>>members. However, this just made me think, what would happen in the >case >>>we want >>>to mat a BO that we have exported for another driver to use? The >>>import_attach >>>field would be set so my code would return with an error, even though we >>>should >>>be in full control of mmaping it. >> >>I am not following your comment. >> >>import_attach is the BO pointer to the dmabuf attachment. >> >>If I export a BO, I cannot set this pointer on the BO because it is for the >>import side only. >> >>>Maybe by allowing the function to go forward in case the base GEM >object's >>>dma-buf >>>operations are the same as our driver's: >>> >>>i915_gem_dmabuf.c: >>> >>>const struct dma_buf_ops *i915_get_dmabuf_ops(void) { >>> return &i915_dmabuf_ops; >>>} >> >>These will only get set on the exported BO. >> >>>i915_gem_mman.c: >>> >>>/* prime objects have no backing filp to GEM mmap >>> * pages from. >>> */ >>>if (obj->base.import_attach && >>> obj->base.dma_buf->ops != i915_get_dmabuf_ops()) { >>> GEM_WARN_ON(obj->base.filp != NULL); >>> addr = -ENXIO; >>> goto err; >>>} >> >>So you would never have import_attach and i915_dmabuf_ops. >> >>Can you restate what you are trying to solve here? Maybe that >>will make things more clear? > >Sorry, this is all because of a misunderstanding on my part. I thought the >import_attach field would be set even for buffers that we've PRIME-exported >to other drivers, but that's not the case. > >Anyway, the bottom question was sourcing the file pointer for the underlying >shmem file from its new location inside the i915_ttm_tt structure bound to a >TTM bo, rather than obj->base.filp. The latter will now always be unset, but that >doesn't mean the object is a PRIME import and therefore the legacy mmap >ioctl should fail, so instead I should check the base.import_attach field instead, >which would tell me that the buffer was imported, and so the ioctl must >return an error. > >Hope this clarifies it. I think so. So, with the new TTM memory handling there is a potential hole in mmap processing path. I am not yet as familiar with the TTM code paths. Will have to spend some time there (in my spare time...). 😊 Thanks, Mike >>Thanks, >> >>M >> >> >>>> Thanks, >>>> >>>> Mike >>> >>>Cheers, >>>Adrian