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

[Shashank Sharma]

On 10/12/20 9:23 pm, Alex Deucher wrote:
> On Thu, Dec 10, 2020 at 8:03 AM Shashank Sharma <shashank.sharma at amd.com> 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 <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.
>> 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.sharma at amd.com>

>
>> - 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 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%7C222e41b3837c4d7f3ec908d89d23ca8b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432124337679194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bgSvn4XqmoKffyCF1v1ldVptiKMZXfEfkzFnJj3Ca0Y%3D&reserved=0
>>>> _______________________________________________
>>>> 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%7C222e41b3837c4d7f3ec908d89d23ca8b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432124337679194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bgSvn4XqmoKffyCF1v1ldVptiKMZXfEfkzFnJj3Ca0Y%3D&reserved=0
>> _______________________________________________
>> 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%7C222e41b3837c4d7f3ec908d89d23ca8b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637432124337679194%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=bgSvn4XqmoKffyCF1v1ldVptiKMZXfEfkzFnJj3Ca0Y%3D&reserved=0