Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
On 10/12/20 9:23 pm, Alex Deucher wrote: > On Thu, Dec 10, 2020 at 8:03 AM Shashank Sharma > wrote: >> >> On 10/12/20 3:58 pm, Christian König wrote: >>> Am 10.12.20 um 05:49 schrieb Shashank Sharma: On 09/12/20 11:00 pm, Alex Deucher wrote: > On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: >> And drop it when we detach. If the shared buffer is in vram, >> we need to make sure we don't put the device into runtime >> suspend. >> >> Signed-off-by: Alex Deucher > Ping? Any thoughts on this? We really only need this for p2p since > device memory in involved, but I'm not sure of the best way to handle > that. > > Alex >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> index 5b465ab774d1..f63f182f37f9 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> @@ -40,6 +40,7 @@ >> #include >> #include >> #include >> +#include >> >> /** >>* amdgpu_gem_prime_vmap - _buf_ops.vmap implementation >> @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf >> *dmabuf, >> if (attach->dev->driver == adev->dev->driver) >> return 0; >> >> + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); >> + if (r < 0) >> + goto out; >> + I am a bit skeptical if we should fail the BO reserve if we don't get the sync ? I was hoping to continue with it, with a warning maybe, so that it doesn't block the existing functionality ? >>> I'm not sure why pm_runtime_get_sync() could fail, but not attaching the >>> DMA-buf is certainly the best we could do here in that moment. >> Ah, I see. Just curious about, before this patch, when we tried to reserve >> the BO, without the PM sync, how was that doing OK ? > p2p is not widely used at this point, so we never really hit is except > for specific testing of the functionality. > > Alex Got it. Apart from this, looks fine by me. Acked-by: Shashank Sharma > >> - Shashank >> >>> Otherwise we could end up in accessing the PCIe BAR with power disabled >>> and that's indeed kind of bad. >>> >>> Christian. >>> >> r = amdgpu_bo_reserve(bo, false); >> if (unlikely(r != 0)) >> - return r; >> + goto out; >> >> /* >> * We only create shared fences for internal use, but importers >> @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf >> *dmabuf, >> */ >> r = __dma_resv_make_exclusive(bo->tbo.base.resv); >> if (r) >> - return r; >> + goto out; >> >> bo->prime_shared_count++; >> amdgpu_bo_unreserve(bo); >> return 0; >> + >> +out: >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); Why not just pm_runtime_put_sync ? Why autosuspend ? - Shashank >> + return r; >> } >> >> /** >> @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf >> *dmabuf, >> >> if (attach->dev->driver != adev->dev->driver && >> bo->prime_shared_count) >> bo->prime_shared_count--; >> + >> + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> } >> >> /** >> -- >> 2.25.4 >> > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7C222e41b3837c4d7f3ec908d89d23ca8b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432124337679194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=bgSvn4XqmoKffyCF1v1ldVptiKMZXfEfkzFnJj3Ca0Y%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7C222e41b3837c4d7f3ec908d89d23ca8b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432124337679194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=bgSvn4XqmoKffyCF1v1ldVptiKMZXfEfkzFnJj3Ca0Y%3Dreserved=0 >> ___ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >>
Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
On Wed, Dec 9, 2020 at 11:49 PM Shashank Sharma wrote: > > > On 09/12/20 11:00 pm, Alex Deucher wrote: > > On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: > >> And drop it when we detach. If the shared buffer is in vram, > >> we need to make sure we don't put the device into runtime > >> suspend. > >> > >> Signed-off-by: Alex Deucher > > > > Ping? Any thoughts on this? We really only need this for p2p since > > device memory in involved, but I'm not sure of the best way to handle > > that. > > > > Alex > > > > > >> --- > >> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- > >> 1 file changed, 14 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> index 5b465ab774d1..f63f182f37f9 100644 > >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > >> @@ -40,6 +40,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> /** > >> * amdgpu_gem_prime_vmap - _buf_ops.vmap implementation > >> @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf > >> *dmabuf, > >> if (attach->dev->driver == adev->dev->driver) > >> return 0; > >> > >> + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); > >> + if (r < 0) > >> + goto out; > >> + > I am a bit skeptical if we should fail the BO reserve if we don't get the > sync ? I was hoping to continue with it, with a warning maybe, so that it > doesn't block the existing functionality ? > >> r = amdgpu_bo_reserve(bo, false); > >> if (unlikely(r != 0)) > >> - return r; > >> + goto out; > >> > >> /* > >> * We only create shared fences for internal use, but importers > >> @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf > >> *dmabuf, > >> */ > >> r = __dma_resv_make_exclusive(bo->tbo.base.resv); > >> if (r) > >> - return r; > >> + goto out; > >> > >> bo->prime_shared_count++; > >> amdgpu_bo_unreserve(bo); > >> return 0; > >> + > >> +out: > >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); > > Why not just pm_runtime_put_sync ? Why autosuspend ? Not sure. I'm just copying what we do in other cases which happens to work. I'm not really a runtime pm expert. Alex > > - Shashank > > >> + return r; > >> } > >> > >> /** > >> @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf > >> *dmabuf, > >> > >> if (attach->dev->driver != adev->dev->driver && > >> bo->prime_shared_count) > >> bo->prime_shared_count--; > >> + > >> + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); > >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); > >> } > >> > >> /** > >> -- > >> 2.25.4 > >> > > ___ > > amd-gfx mailing list > > amd-gfx@lists.freedesktop.org > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7Cd5fccf427c34414ff4e708d89c682898%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431318483043257%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=NpCTY%2FVKd%2FBDi1wsFC79qSUTmNHx3nBp0HUj3m0cFeM%3Dreserved=0 > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
On Thu, Dec 10, 2020 at 8:03 AM Shashank Sharma wrote: > > > On 10/12/20 3:58 pm, Christian König wrote: > > Am 10.12.20 um 05:49 schrieb Shashank Sharma: > >> On 09/12/20 11:00 pm, Alex Deucher wrote: > >>> On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: > And drop it when we detach. If the shared buffer is in vram, > we need to make sure we don't put the device into runtime > suspend. > > Signed-off-by: Alex Deucher > >>> Ping? Any thoughts on this? We really only need this for p2p since > >>> device memory in involved, but I'm not sure of the best way to handle > >>> that. > >>> > >>> Alex > >>> > >>> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index 5b465ab774d1..f63f182f37f9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -40,6 +40,7 @@ > #include > #include > #include > +#include > > /** > * amdgpu_gem_prime_vmap - _buf_ops.vmap implementation > @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf > *dmabuf, > if (attach->dev->driver == adev->dev->driver) > return 0; > > + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); > + if (r < 0) > + goto out; > + > >> I am a bit skeptical if we should fail the BO reserve if we don't get the > >> sync ? I was hoping to continue with it, with a warning maybe, so that it > >> doesn't block the existing functionality ? > > I'm not sure why pm_runtime_get_sync() could fail, but not attaching the > > DMA-buf is certainly the best we could do here in that moment. > > Ah, I see. Just curious about, before this patch, when we tried to reserve > the BO, without the PM sync, how was that doing OK ? p2p is not widely used at this point, so we never really hit is except for specific testing of the functionality. Alex > > - Shashank > > > Otherwise we could end up in accessing the PCIe BAR with power disabled > > and that's indeed kind of bad. > > > > Christian. > > > r = amdgpu_bo_reserve(bo, false); > if (unlikely(r != 0)) > - return r; > + goto out; > > /* > * We only create shared fences for internal use, but importers > @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf > *dmabuf, > */ > r = __dma_resv_make_exclusive(bo->tbo.base.resv); > if (r) > - return r; > + goto out; > > bo->prime_shared_count++; > amdgpu_bo_unreserve(bo); > return 0; > + > +out: > + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); > >> Why not just pm_runtime_put_sync ? Why autosuspend ? > >> > >> - Shashank > >> > + return r; > } > > /** > @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf > *dmabuf, > > if (attach->dev->driver != adev->dev->driver && > bo->prime_shared_count) > bo->prime_shared_count--; > + > + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); > + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); > } > > /** > -- > 2.25.4 > > >>> ___ > >>> amd-gfx mailing list > >>> amd-gfx@lists.freedesktop.org > >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7C9957c5b838fc49ae225e08d89cf649a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637431928895902617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=AMtWNSZRyFZDRHhE7hsIdBWxTHVLGYVOHZOSeRSR07s%3Dreserved=0 > >> ___ > >> amd-gfx mailing list > >> amd-gfx@lists.freedesktop.org > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7C9957c5b838fc49ae225e08d89cf649a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637431928895902617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=AMtWNSZRyFZDRHhE7hsIdBWxTHVLGYVOHZOSeRSR07s%3Dreserved=0 > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list
Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
On 10/12/20 3:58 pm, Christian König wrote: > Am 10.12.20 um 05:49 schrieb Shashank Sharma: >> On 09/12/20 11:00 pm, Alex Deucher wrote: >>> On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: And drop it when we detach. If the shared buffer is in vram, we need to make sure we don't put the device into runtime suspend. Signed-off-by: Alex Deucher >>> Ping? Any thoughts on this? We really only need this for p2p since >>> device memory in involved, but I'm not sure of the best way to handle >>> that. >>> >>> Alex >>> >>> --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 5b465ab774d1..f63f182f37f9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -40,6 +40,7 @@ #include #include #include +#include /** * amdgpu_gem_prime_vmap - _buf_ops.vmap implementation @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, if (attach->dev->driver == adev->dev->driver) return 0; + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); + if (r < 0) + goto out; + >> I am a bit skeptical if we should fail the BO reserve if we don't get the >> sync ? I was hoping to continue with it, with a warning maybe, so that it >> doesn't block the existing functionality ? > I'm not sure why pm_runtime_get_sync() could fail, but not attaching the > DMA-buf is certainly the best we could do here in that moment. Ah, I see. Just curious about, before this patch, when we tried to reserve the BO, without the PM sync, how was that doing OK ? - Shashank > Otherwise we could end up in accessing the PCIe BAR with power disabled > and that's indeed kind of bad. > > Christian. > r = amdgpu_bo_reserve(bo, false); if (unlikely(r != 0)) - return r; + goto out; /* * We only create shared fences for internal use, but importers @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, */ r = __dma_resv_make_exclusive(bo->tbo.base.resv); if (r) - return r; + goto out; bo->prime_shared_count++; amdgpu_bo_unreserve(bo); return 0; + +out: + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> Why not just pm_runtime_put_sync ? Why autosuspend ? >> >> - Shashank >> + return r; } /** @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf *dmabuf, if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count) bo->prime_shared_count--; + + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); } /** -- 2.25.4 >>> ___ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7C9957c5b838fc49ae225e08d89cf649a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637431928895902617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=AMtWNSZRyFZDRHhE7hsIdBWxTHVLGYVOHZOSeRSR07s%3Dreserved=0 >> ___ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7C9957c5b838fc49ae225e08d89cf649a1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C1%7C637431928895902617%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=AMtWNSZRyFZDRHhE7hsIdBWxTHVLGYVOHZOSeRSR07s%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
Am 10.12.20 um 05:49 schrieb Shashank Sharma: On 09/12/20 11:00 pm, Alex Deucher wrote: On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: And drop it when we detach. If the shared buffer is in vram, we need to make sure we don't put the device into runtime suspend. Signed-off-by: Alex Deucher Ping? Any thoughts on this? We really only need this for p2p since device memory in involved, but I'm not sure of the best way to handle that. Alex --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 5b465ab774d1..f63f182f37f9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -40,6 +40,7 @@ #include #include #include +#include /** * amdgpu_gem_prime_vmap - _buf_ops.vmap implementation @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, if (attach->dev->driver == adev->dev->driver) return 0; + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); + if (r < 0) + goto out; + I am a bit skeptical if we should fail the BO reserve if we don't get the sync ? I was hoping to continue with it, with a warning maybe, so that it doesn't block the existing functionality ? I'm not sure why pm_runtime_get_sync() could fail, but not attaching the DMA-buf is certainly the best we could do here in that moment. Otherwise we could end up in accessing the PCIe BAR with power disabled and that's indeed kind of bad. Christian. r = amdgpu_bo_reserve(bo, false); if (unlikely(r != 0)) - return r; + goto out; /* * We only create shared fences for internal use, but importers @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, */ r = __dma_resv_make_exclusive(bo->tbo.base.resv); if (r) - return r; + goto out; bo->prime_shared_count++; amdgpu_bo_unreserve(bo); return 0; + +out: + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); Why not just pm_runtime_put_sync ? Why autosuspend ? - Shashank + return r; } /** @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf *dmabuf, if (attach->dev->driver != adev->dev->driver && bo->prime_shared_count) bo->prime_shared_count--; + + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); } /** -- 2.25.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7Cd5fccf427c34414ff4e708d89c682898%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431318483043257%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=NpCTY%2FVKd%2FBDi1wsFC79qSUTmNHx3nBp0HUj3m0cFeM%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
On 09/12/20 11:00 pm, Alex Deucher wrote: > On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: >> And drop it when we detach. If the shared buffer is in vram, >> we need to make sure we don't put the device into runtime >> suspend. >> >> Signed-off-by: Alex Deucher > > Ping? Any thoughts on this? We really only need this for p2p since > device memory in involved, but I'm not sure of the best way to handle > that. > > Alex > > >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- >> 1 file changed, 14 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> index 5b465ab774d1..f63f182f37f9 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >> @@ -40,6 +40,7 @@ >> #include >> #include >> #include >> +#include >> >> /** >> * amdgpu_gem_prime_vmap - _buf_ops.vmap implementation >> @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, >> if (attach->dev->driver == adev->dev->driver) >> return 0; >> >> + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); >> + if (r < 0) >> + goto out; >> + I am a bit skeptical if we should fail the BO reserve if we don't get the sync ? I was hoping to continue with it, with a warning maybe, so that it doesn't block the existing functionality ? >> r = amdgpu_bo_reserve(bo, false); >> if (unlikely(r != 0)) >> - return r; >> + goto out; >> >> /* >> * We only create shared fences for internal use, but importers >> @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf >> *dmabuf, >> */ >> r = __dma_resv_make_exclusive(bo->tbo.base.resv); >> if (r) >> - return r; >> + goto out; >> >> bo->prime_shared_count++; >> amdgpu_bo_unreserve(bo); >> return 0; >> + >> +out: >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); Why not just pm_runtime_put_sync ? Why autosuspend ? - Shashank >> + return r; >> } >> >> /** >> @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf *dmabuf, >> >> if (attach->dev->driver != adev->dev->driver && >> bo->prime_shared_count) >> bo->prime_shared_count--; >> + >> + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> } >> >> /** >> -- >> 2.25.4 >> > ___ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfxdata=04%7C01%7Cshashank.sharma%40amd.com%7Cd5fccf427c34414ff4e708d89c682898%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431318483043257%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=NpCTY%2FVKd%2FBDi1wsFC79qSUTmNHx3nBp0HUj3m0cFeM%3Dreserved=0 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer
On Fri, Dec 4, 2020 at 3:41 PM Alex Deucher wrote: > > And drop it when we detach. If the shared buffer is in vram, > we need to make sure we don't put the device into runtime > suspend. > > Signed-off-by: Alex Deucher Ping? Any thoughts on this? We really only need this for p2p since device memory in involved, but I'm not sure of the best way to handle that. Alex > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 16 ++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index 5b465ab774d1..f63f182f37f9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -40,6 +40,7 @@ > #include > #include > #include > +#include > > /** > * amdgpu_gem_prime_vmap - _buf_ops.vmap implementation > @@ -187,9 +188,13 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, > if (attach->dev->driver == adev->dev->driver) > return 0; > > + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); > + if (r < 0) > + goto out; > + > r = amdgpu_bo_reserve(bo, false); > if (unlikely(r != 0)) > - return r; > + goto out; > > /* > * We only create shared fences for internal use, but importers > @@ -201,11 +206,15 @@ static int amdgpu_dma_buf_attach(struct dma_buf *dmabuf, > */ > r = __dma_resv_make_exclusive(bo->tbo.base.resv); > if (r) > - return r; > + goto out; > > bo->prime_shared_count++; > amdgpu_bo_unreserve(bo); > return 0; > + > +out: > + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); > + return r; > } > > /** > @@ -225,6 +234,9 @@ static void amdgpu_dma_buf_detach(struct dma_buf *dmabuf, > > if (attach->dev->driver != adev->dev->driver && > bo->prime_shared_count) > bo->prime_shared_count--; > + > + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); > + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); > } > > /** > -- > 2.25.4 > ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
