>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> >Sent: Friday, June 25, 2021 12:18 PM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel- >gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx >Cc: Auld, Matthew <matthew.auld@xxxxxxxxx> >Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map >time > >Hi, Michael, > >thanks for looking at this. > >On 6/25/21 6:02 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of >>> Thomas Hellström >>> Sent: Thursday, June 24, 2021 2:31 PM >>> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx >>> Cc: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>; Auld, >Matthew >>> <matthew.auld@xxxxxxxxx> >>> Subject: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map >time >>> >>> Until we support p2p dma or as a complement to that, migrate data >>> to system memory at dma-buf map time if possible. >>> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> >>> --- >>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> index 616c3a2f1baf..a52f885bc09a 100644 >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >>> @@ -25,7 +25,14 @@ static struct sg_table >*i915_gem_map_dma_buf(struct >>> dma_buf_attachment *attachme >>> struct scatterlist *src, *dst; >>> int ret, i; >>> >>> - ret = i915_gem_object_pin_pages_unlocked(obj); >>> + ret = i915_gem_object_lock_interruptible(obj, NULL); >> Hmm, I believe in most cases that the caller should be holding the >> lock (object dma-resv) on this object already. > >Yes, I agree, In particular for other instances of our own driver, at >least since the dma_resv introduction. > >But I also think that's a pre-existing bug, since >i915_gem_object_pin_pages_unlocked() will also take the lock. Ouch yes. Missed that. >I Think we need to initially make the exporter dynamic-capable to >resolve this, and drop the locking here completely, as dma-buf docs says >that we're then guaranteed to get called with the object lock held. > >I figure if we make the exporter dynamic, we need to migrate already at >dma_buf_pin time so we don't pin the object in the wrong location. The exporter as dynamic (ops->pin is available) is optional, but importer dynamic (ops->move_notify) is required. With that in mind, it would seem that there are three possible combinations for the migrate to be attempted: 1) in the ops->pin function (export_dynamic != import_dynamic, during attach) 2) in the ops->pin function (export_dynamic and !CONFIG_DMABUF_MOVE_NOTIFY) during mapping 3) and possibly in ops->map_dma_buf (exort_dynamic iand CONFIG_DMABUF_MOVE_NOTIFY) Since one possibility has to be in the mapping function, it seems that if we can figure out the locking, that the migrate should probably be available here. Mike >/Thomas > > >> >> I know for the dynamic version of dma-buf, there is a check to make >> sure that the lock is held when called. >> >> I think you will run into some issues if you try to get it here as well. >> >> Mike >> >>> + if (ret) >>> + return ERR_PTR(ret); >>> + >>> + ret = i915_gem_object_migrate(obj, NULL, INTEL_REGION_SMEM); >>> + if (!ret) >>> + ret = i915_gem_object_pin_pages(obj); >>> + i915_gem_object_unlock(obj); >>> if (ret) >>> goto err; >>> >>> -- >>> 2.31.1
