| Message ID | 20170329002720.11445-5-raof@ubuntu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/03/17 09:27 AM, raof@ubuntu.com wrote: > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > Attempting to migrate the bo will break the sharing of the buffer. > > Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > CC: amd-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > index 3826d5aea0a6..3c84ec5c6ac8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > @@ -74,6 +74,17 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, > if (ret) > return ERR_PTR(ret); > > + /* Imported bo must be pinned to GTT, as moving it breaks sharing */ > + ret = amdgpu_bo_reserve(bo, false); > + if (ret) > + return ERR_PTR(ret); > + > + ret = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); > + if (ret) > + return ERR_PTR(ret); > + > + amdgpu_bo_unreserve(bo); > + > bo->prime_shared_count = 1; > return &bo->gem_base; > } > Thanks for beating me to this! :) This patch and patch 6 are Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
Am 29.03.2017 um 02:27 schrieb raof@ubuntu.com: > From: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > > Attempting to migrate the bo will break the sharing of the buffer. > > Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com> > CC: amd-gfx@lists.freedesktop.org NAK on this one and the radeon version. We can't migrate the buffers to VRAM, but we shouldn't pin it either cause that will lock down the GTT space used for it. Instead you should modify amdgpu_bo_pin() and fail if anybody tries to migrate a BO which has prime_shared_count != 0 to VRAM (you migth need to add this for radeon). Regards, Christian. > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > index 3826d5aea0a6..3c84ec5c6ac8 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > @@ -74,6 +74,17 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, > if (ret) > return ERR_PTR(ret); > > + /* Imported bo must be pinned to GTT, as moving it breaks sharing */ > + ret = amdgpu_bo_reserve(bo, false); > + if (ret) > + return ERR_PTR(ret); > + > + ret = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); > + if (ret) > + return ERR_PTR(ret); > + > + amdgpu_bo_unreserve(bo); > + > bo->prime_shared_count = 1; > return &bo->gem_base; > }
On 29/03/17 04:14 PM, Christian König wrote: > Am 29.03.2017 um 02:27 schrieb raof@ubuntu.com: >> From: Christopher James Halse Rogers >> <christopher.halse.rogers@canonical.com> >> >> Attempting to migrate the bo will break the sharing of the buffer. >> >> Signed-off-by: Christopher James Halse Rogers >> <christopher.halse.rogers@canonical.com> >> CC: amd-gfx@lists.freedesktop.org > > NAK on this one and the radeon version. > > We can't migrate the buffers to VRAM, but we shouldn't pin it either > cause that will lock down the GTT space used for it. Ah, good point, didn't think of that. > Instead you should modify amdgpu_bo_pin() and fail if anybody tries to > migrate a BO which has prime_shared_count != 0 to VRAM (you migth need > to add this for radeon). This would also need to be checked in the AMDGPU_GEM_OP_SET_PLACEMENT case of amdgpu_gem_op_ioctl, otherwise userspace can change prefered_domains to AMDGPU_GEM_DOMAIN_VRAM, and using the BO for GPU operations may move it to VRAM. Or maybe there's a central place which can catch both of these cases (and any others).
Am 29.03.2017 um 11:07 schrieb Michel Dänzer: > On 29/03/17 04:14 PM, Christian König wrote: >> Am 29.03.2017 um 02:27 schrieb raof@ubuntu.com: >>> From: Christopher James Halse Rogers >>> <christopher.halse.rogers@canonical.com> >>> >>> Attempting to migrate the bo will break the sharing of the buffer. >>> >>> Signed-off-by: Christopher James Halse Rogers >>> <christopher.halse.rogers@canonical.com> >>> CC: amd-gfx@lists.freedesktop.org >> NAK on this one and the radeon version. >> >> We can't migrate the buffers to VRAM, but we shouldn't pin it either >> cause that will lock down the GTT space used for it. > Ah, good point, didn't think of that. > > >> Instead you should modify amdgpu_bo_pin() and fail if anybody tries to >> migrate a BO which has prime_shared_count != 0 to VRAM (you migth need >> to add this for radeon). > This would also need to be checked in the AMDGPU_GEM_OP_SET_PLACEMENT > case of amdgpu_gem_op_ioctl, otherwise userspace can change > prefered_domains to AMDGPU_GEM_DOMAIN_VRAM, and using the BO for GPU > operations may move it to VRAM. Good point as well, that indeed also needs to be forbidden. > Or maybe there's a central place which can catch both of these cases > (and any others). At least of hand I can't think of any. But catching those two cases should be sufficient as far as I can see. Christian.
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c index 3826d5aea0a6..3c84ec5c6ac8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c @@ -74,6 +74,17 @@ amdgpu_gem_prime_import_sg_table(struct drm_device *dev, if (ret) return ERR_PTR(ret); + /* Imported bo must be pinned to GTT, as moving it breaks sharing */ + ret = amdgpu_bo_reserve(bo, false); + if (ret) + return ERR_PTR(ret); + + ret = amdgpu_bo_pin(bo, AMDGPU_GEM_DOMAIN_GTT, NULL); + if (ret) + return ERR_PTR(ret); + + amdgpu_bo_unreserve(bo); + bo->prime_shared_count = 1; return &bo->gem_base; }