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.
>Thanks,
>
>M
>
>
>>> Thanks,
>>>
>>> Mike
>>
>>Cheers,
>>Adrian
