| Message ID | 20210120154019.5146-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Allow importing of shmemfs objects into any device | expand |
On Wed, 20 Jan 2021 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > If we import a shmemfs object between devices, for example from > Tigerlake to DG1, we can simply reuse the native object and its backing > store. Hmmm interesting, so does that include re-using the actual sg mapping for the backing pages? Does that work out-of-the-box between different devices assuming we have iommu enabled? > > Suggested-by: Imre Deak <imre.deak@intel.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > index 04e9c04545ad..4816f08c4009 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > @@ -242,6 +242,13 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, > */ > return &i915_gem_object_get(obj)->base; > } > + > + /* > + * If the object is in plain system memory, we can reuse the > + * same backing store in any device. > + */ > + if (i915_gem_object_is_shmem(obj)) > + return &i915_gem_object_get(obj)->base; > } > > /* need to attach */ > -- > 2.20.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi Chris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-tip/drm-tip v5.11-rc4 next-20210120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-gem-Allow-importing-of-shmemfs-objects-into-any-device/20210120-234237
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: i386-randconfig-a013-20210120 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/ce377384dec1c18f1af3558d5d4624d300d160c4
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chris-Wilson/drm-i915-gem-Allow-importing-of-shmemfs-objects-into-any-device/20210120-234237
git checkout ce377384dec1c18f1af3558d5d4624d300d160c4
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c: In function 'i915_gem_prime_import':
>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c:250:7: error: implicit declaration of function 'i915_gem_object_is_shmem'; did you mean 'i915_gem_object_is_tiled'? [-Werror=implicit-function-declaration]
250 | if (i915_gem_object_is_shmem(obj))
| ^~~~~~~~~~~~~~~~~~~~~~~~
| i915_gem_object_is_tiled
cc1: some warnings being treated as errors
vim +250 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
225
226 struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
227 struct dma_buf *dma_buf)
228 {
229 static struct lock_class_key lock_class;
230 struct dma_buf_attachment *attach;
231 struct drm_i915_gem_object *obj;
232 int ret;
233
234 /* is this one of own objects? */
235 if (dma_buf->ops == &i915_dmabuf_ops) {
236 obj = dma_buf_to_obj(dma_buf);
237 /* is it from our device? */
238 if (obj->base.dev == dev) {
239 /*
240 * Importing dmabuf exported from out own gem increases
241 * refcount on gem itself instead of f_count of dmabuf.
242 */
243 return &i915_gem_object_get(obj)->base;
244 }
245
246 /*
247 * If the object is in plain system memory, we can reuse the
248 * same backing store in any device.
249 */
> 250 if (i915_gem_object_is_shmem(obj))
251 return &i915_gem_object_get(obj)->base;
252 }
253
254 /* need to attach */
255 attach = dma_buf_attach(dma_buf, dev->dev);
256 if (IS_ERR(attach))
257 return ERR_CAST(attach);
258
259 get_dma_buf(dma_buf);
260
261 obj = i915_gem_object_alloc();
262 if (obj == NULL) {
263 ret = -ENOMEM;
264 goto fail_detach;
265 }
266
267 drm_gem_private_object_init(dev, &obj->base, dma_buf->size);
268 i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops, &lock_class);
269 obj->base.import_attach = attach;
270 obj->base.resv = dma_buf->resv;
271
272 /* We use GTT as shorthand for a coherent domain, one that is
273 * neither in the GPU cache nor in the CPU cache, where all
274 * writes are immediately visible in memory. (That's not strictly
275 * true, but it's close! There are internal buffers such as the
276 * write-combined buffer or a delay through the chipset for GTT
277 * writes that do require us to treat GTT as a separate cache domain.)
278 */
279 obj->read_domains = I915_GEM_DOMAIN_GTT;
280 obj->write_domain = 0;
281
282 return &obj->base;
283
284 fail_detach:
285 dma_buf_detach(dma_buf, attach);
286 dma_buf_put(dma_buf);
287
288 return ERR_PTR(ret);
289 }
290
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Quoting Chris Wilson (2021-01-20 18:06:08) > Quoting Matthew Auld (2021-01-20 17:46:10) > > On Wed, 20 Jan 2021 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > If we import a shmemfs object between devices, for example from > > > Tigerlake to DG1, we can simply reuse the native object and its backing > > > store. > > > > Hmmm interesting, so does that include re-using the actual sg mapping > > for the backing pages? Does that work out-of-the-box between different > > devices assuming we have iommu enabled? > > Indeed interesting; the dma_addr_t are supposed to be local to a device. On reflection, we are expected to use cross-device dma_addr_t with dma-buf. It's the exporter who assigns the dma_addr_t for the importer to use, and they are always given from the original device. Maybe not so bad. Definitely needs testing to see what happens in practice. -Chris
On 20/01/2021 20:59, Chris Wilson wrote: > Quoting Chris Wilson (2021-01-20 18:06:08) >> Quoting Matthew Auld (2021-01-20 17:46:10) >>> On Wed, 20 Jan 2021 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>> >>>> If we import a shmemfs object between devices, for example from >>>> Tigerlake to DG1, we can simply reuse the native object and its backing >>>> store. >>> >>> Hmmm interesting, so does that include re-using the actual sg mapping >>> for the backing pages? Does that work out-of-the-box between different >>> devices assuming we have iommu enabled? >> >> Indeed interesting; the dma_addr_t are supposed to be local to a device. > > On reflection, we are expected to use cross-device dma_addr_t with > dma-buf. It's the exporter who assigns the dma_addr_t for the importer > to use, and they are always given from the original device. > > Maybe not so bad. Definitely needs testing to see what happens in > practice. What about object migration? I did not spot anything preventing it once object was exported like this so owning device could move it to device memory afterwards which would probably be bad. Regards, Tvrtko
Quoting Tvrtko Ursulin (2021-01-21 11:00:25) > > On 20/01/2021 20:59, Chris Wilson wrote: > > Quoting Chris Wilson (2021-01-20 18:06:08) > >> Quoting Matthew Auld (2021-01-20 17:46:10) > >>> On Wed, 20 Jan 2021 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >>>> > >>>> If we import a shmemfs object between devices, for example from > >>>> Tigerlake to DG1, we can simply reuse the native object and its backing > >>>> store. > >>> > >>> Hmmm interesting, so does that include re-using the actual sg mapping > >>> for the backing pages? Does that work out-of-the-box between different > >>> devices assuming we have iommu enabled? > >> > >> Indeed interesting; the dma_addr_t are supposed to be local to a device. > > > > On reflection, we are expected to use cross-device dma_addr_t with > > dma-buf. It's the exporter who assigns the dma_addr_t for the importer > > to use, and they are always given from the original device. > > > > Maybe not so bad. Definitely needs testing to see what happens in > > practice. > > What about object migration? I did not spot anything preventing it once > object was exported like this so owning device could move it to device > memory afterwards which would probably be bad. Depends on how you do your migration, but your migration should be checking that it is allowed to migrate the object first. -Chris
On 21/01/2021 11:03, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2021-01-21 11:00:25) >> >> On 20/01/2021 20:59, Chris Wilson wrote: >>> Quoting Chris Wilson (2021-01-20 18:06:08) >>>> Quoting Matthew Auld (2021-01-20 17:46:10) >>>>> On Wed, 20 Jan 2021 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>>>> >>>>>> If we import a shmemfs object between devices, for example from >>>>>> Tigerlake to DG1, we can simply reuse the native object and its backing >>>>>> store. >>>>> >>>>> Hmmm interesting, so does that include re-using the actual sg mapping >>>>> for the backing pages? Does that work out-of-the-box between different >>>>> devices assuming we have iommu enabled? >>>> >>>> Indeed interesting; the dma_addr_t are supposed to be local to a device. >>> >>> On reflection, we are expected to use cross-device dma_addr_t with >>> dma-buf. It's the exporter who assigns the dma_addr_t for the importer >>> to use, and they are always given from the original device. >>> >>> Maybe not so bad. Definitely needs testing to see what happens in >>> practice. >> >> What about object migration? I did not spot anything preventing it once >> object was exported like this so owning device could move it to device >> memory afterwards which would probably be bad. > > Depends on how you do your migration, but your migration should be > checking that it is allowed to migrate the object first. Okay agreed, meaning to be handled once migration gets to upstream. Regards, Tvrtko
>-----Original Message----- >From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of >Matthew Auld >Sent: Wednesday, January 20, 2021 12:46 PM >To: Chris Wilson <chris@chris-wilson.co.uk> >Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>; Auld, >Matthew <matthew.auld@intel.com> >Subject: Re: [Intel-gfx] [PATCH] drm/i915/gem: Allow importing of shmemfs >objects into any device > >On Wed, 20 Jan 2021 at 15:40, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> >> If we import a shmemfs object between devices, for example from >> Tigerlake to DG1, we can simply reuse the native object and its backing >> store. > >Hmmm interesting, so does that include re-using the actual sg mapping >for the backing pages? Does that work out-of-the-box between different >devices assuming we have iommu enabled? > >> >> Suggested-by: Imre Deak <imre.deak@intel.com> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> index 04e9c04545ad..4816f08c4009 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c >> @@ -242,6 +242,13 @@ struct drm_gem_object >*i915_gem_prime_import(struct drm_device *dev, >> */ >> return &i915_gem_object_get(obj)->base; >> } >> + >> + /* >> + * If the object is in plain system memory, we can reuse the >> + * same backing store in any device. >> + */ >> + if (i915_gem_object_is_shmem(obj)) >> + return &i915_gem_object_get(obj)->base; >> } So this would mean sharing all of the object attributes between devices (pin count, etc)? I.e. vma list etc? Would that work? Mike >> /* need to attach */ >> -- >> 2.20.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 04e9c04545ad..4816f08c4009 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -242,6 +242,13 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev, */ return &i915_gem_object_get(obj)->base; } + + /* + * If the object is in plain system memory, we can reuse the + * same backing store in any device. + */ + if (i915_gem_object_is_shmem(obj)) + return &i915_gem_object_get(obj)->base; } /* need to attach */
If we import a shmemfs object between devices, for example from Tigerlake to DG1, we can simply reuse the native object and its backing store. Suggested-by: Imre Deak <imre.deak@intel.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 7 +++++++ 1 file changed, 7 insertions(+)