[PATCH] drm/amdgpu: take runtime pm reference when we attach a buffer

[Christian König]

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 <alexdeucher at gmail.com> 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 <alexander.deucher at amd.com>
>> 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 <linux/dma-buf.h>
>>>   #include <linux/dma-fence-array.h>
>>>   #include <linux/pci-p2pdma.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>>   /**
>>>    * amdgpu_gem_prime_vmap - &dma_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 at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cshashank.sharma%40amd.com%7Cd5fccf427c34414ff4e708d89c682898%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431318483043257%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NpCTY%2FVKd%2FBDi1wsFC79qSUTmNHx3nBp0HUj3m0cFeM%3D&reserved=0
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx