>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> >Sent: Friday, June 25, 2021 2:50 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, Mike, > >On 6/25/21 7:57 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx> >>> Sent: Friday, June 25, 2021 1:52 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 >>> >>> >>> On 6/25/21 7:38 PM, Ruhl, Michael J wrote: >>>>> -----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 >>> So perhaps just to initially fix the bug, we could just implement NOP >>> pin() and unpin() callbacks and drop the locking in map_attach() and >>> replace it with an assert_object_held(); >> That is the sticky part of the move notify API. >> >> If you do the attach_dynamic you have to have an ops with move_notify. >> >> (https://elixir.bootlin.com/linux/v5.13-rc7/source/drivers/dma-buf/dma- >buf.c#L730) >> >> If you don't have that, i.e. just the pin interface, the attach will be >> rejected, and you will not get the callbacks. > >I understood that as the requirement for move_notify is only if the >*importer* declares dynamic. A dynamic exporter could choose whether to >call move_notify() on eviction or to pin and never evict. If the >importer is non-dynamic, the core calls pin() and the only choice is to >pin and never evict. > >So if we temporarily choose to pin and never evict for *everything*, (as >the current code does now), I think we should be good for now, and then >we can implement all fancy p2p and move_notify stuff on top of that. /sigh. You are correct. I was mistakenly placing the pin API (dma_buf_ops) in the attach_ops. 😐 Must be Friday. Upon further reflection, I think that your path will work. However, is doing a pin (with no locking) from the dma_buf_mapping any different from using the pin API + export_dynamic? M >/Thomas > > >> >> So I think that the only thing we can do for now is to dop the locking and add >the >> >> assert_object_held(); >> >> M > > > >> >>> /Thomas >>>
