| Message ID | 20210624183110.22582-5-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Introduce a migrate interface | expand |
>-----Original Message----- >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >Thomas Hellström >Sent: Thursday, June 24, 2021 2:31 PM >To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, Matthew ><matthew.auld@intel.com> >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@linux.intel.com> >--- > 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. 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
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@lists.freedesktop.org> On Behalf Of >> Thomas Hellström >> Sent: Thursday, June 24, 2021 2:31 PM >> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, Matthew >> <matthew.auld@intel.com> >> 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@linux.intel.com> >> --- >> 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. 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. /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
>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Sent: Friday, June 25, 2021 12:18 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >Cc: Auld, Matthew <matthew.auld@intel.com> >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@lists.freedesktop.org> On Behalf Of >>> Thomas Hellström >>> Sent: Thursday, June 24, 2021 2:31 PM >>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >Matthew >>> <matthew.auld@intel.com> >>> 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@linux.intel.com> >>> --- >>> 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
On 6/25/21 7:38 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Sent: Friday, June 25, 2021 12:18 PM >> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Auld, Matthew <matthew.auld@intel.com> >> 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@lists.freedesktop.org> On Behalf Of >>>> Thomas Hellström >>>> Sent: Thursday, June 24, 2021 2:31 PM >>>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >> Matthew >>>> <matthew.auld@intel.com> >>>> 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@linux.intel.com> >>>> --- >>>> 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(); /Thomas
>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Sent: Friday, June 25, 2021 1:52 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >Cc: Auld, Matthew <matthew.auld@intel.com> >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@linux.intel.com> >>> Sent: Friday, June 25, 2021 12:18 PM >>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>> Cc: Auld, Matthew <matthew.auld@intel.com> >>> 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@lists.freedesktop.org> On Behalf >Of >>>>> Thomas Hellström >>>>> Sent: Thursday, June 24, 2021 2:31 PM >>>>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >>> Matthew >>>>> <matthew.auld@intel.com> >>>>> 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@linux.intel.com> >>>>> --- >>>>> 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. 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 >
Hi, Mike, On 6/25/21 7:57 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Sent: Friday, June 25, 2021 1:52 PM >> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Auld, Matthew <matthew.auld@intel.com> >> 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@linux.intel.com> >>>> Sent: Friday, June 25, 2021 12:18 PM >>>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>> Cc: Auld, Matthew <matthew.auld@intel.com> >>>> 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@lists.freedesktop.org> On Behalf >> Of >>>>>> Thomas Hellström >>>>>> Sent: Thursday, June 24, 2021 2:31 PM >>>>>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >>>> Matthew >>>>>> <matthew.auld@intel.com> >>>>>> 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@linux.intel.com> >>>>>> --- >>>>>> 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. /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 >>
>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Sent: Friday, June 25, 2021 2:50 PM >To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >Cc: Auld, Matthew <matthew.auld@intel.com> >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@linux.intel.com> >>> Sent: Friday, June 25, 2021 1:52 PM >>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>> Cc: Auld, Matthew <matthew.auld@intel.com> >>> 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@linux.intel.com> >>>>> Sent: Friday, June 25, 2021 12:18 PM >>>>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>> Cc: Auld, Matthew <matthew.auld@intel.com> >>>>> 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@lists.freedesktop.org> On >Behalf >>> Of >>>>>>> Thomas Hellström >>>>>>> Sent: Thursday, June 24, 2021 2:31 PM >>>>>>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >>>>> Matthew >>>>>>> <matthew.auld@intel.com> >>>>>>> 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@linux.intel.com> >>>>>>> --- >>>>>>> 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.
On 6/25/21 9:07 PM, Ruhl, Michael J wrote: >> -----Original Message----- >> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Sent: Friday, June 25, 2021 2:50 PM >> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Auld, Matthew <matthew.auld@intel.com> >> 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@linux.intel.com> >>>> Sent: Friday, June 25, 2021 1:52 PM >>>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>> Cc: Auld, Matthew <matthew.auld@intel.com> >>>> 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@linux.intel.com> >>>>>> Sent: Friday, June 25, 2021 12:18 PM >>>>>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>>>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>>> Cc: Auld, Matthew <matthew.auld@intel.com> >>>>>> 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@lists.freedesktop.org> On >> Behalf >>>> Of >>>>>>>> Thomas Hellström >>>>>>>> Sent: Thursday, June 24, 2021 2:31 PM >>>>>>>> To: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >>>>>> Matthew >>>>>>>> <matthew.auld@intel.com> >>>>>>>> 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@linux.intel.com> >>>>>>>> --- >>>>>>>> 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.
>-----Original Message----- >From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Sent: Friday, June 25, 2021 3:10 PM>To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >Cc: Auld, Matthew <matthew.auld@intel.com> >Subject: Re: [PATCH 4/4] drm/i915/gem: Migrate to system at dma-buf map >time > > >On 6/25/21 9:07 PM, Ruhl, Michael J wrote: >>> -----Original Message----- >>> From: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> Sent: Friday, June 25, 2021 2:50 PM >>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>> Cc: Auld, Matthew <matthew.auld@intel.com> >>> 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@linux.intel.com> >>>>> Sent: Friday, June 25, 2021 1:52 PM >>>>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>> Cc: Auld, Matthew <matthew.auld@intel.com> >>>>> 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@linux.intel.com> >>>>>>> Sent: Friday, June 25, 2021 12:18 PM >>>>>>> To: Ruhl, Michael J <michael.j.ruhl@intel.com>; intel- >>>>>>> gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >>>>>>> Cc: Auld, Matthew <matthew.auld@intel.com> >>>>>>> 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@lists.freedesktop.org> On >>> Behalf >>>>> Of >>>>>>>>> Thomas Hellström >>>>>>>>> Sent: Thursday, June 24, 2021 2:31 PM >>>>>>>>> To: intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org >>>>>>>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; Auld, >>>>>>> Matthew >>>>>>>>> <matthew.auld@intel.com> >>>>>>>>> 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@linux.intel.com> >>>>>>>>> --- >>>>>>>>> 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.
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); + 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;
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@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)