[AMD Official Use Only - General] > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Wu, > David > Sent: Wednesday, March 20, 2024 8:02 PM > To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] drm/amdkfd: Check cgroup when returning DMABuf info > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On 3/20/2024 4:21 PM, Felix Kuehling wrote: > > On 2024-03-18 16:12, Felix Kuehling wrote: > >> > >> On 2024-03-15 14:17, Mukul Joshi wrote: > >>> Check cgroup permissions when returning DMA-buf info and based on > >>> cgroup check return the id of the GPU that has access to the BO. > >>> > >>> Signed-off-by: Mukul Joshi <mukul.joshi@xxxxxxx> > >>> --- > >>> drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 4 ++-- > >>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > >>> b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > >>> index dfa8c69532d4..f9631f4b1a02 100644 > >>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > >>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c > >>> @@ -1523,7 +1523,7 @@ static int kfd_ioctl_get_dmabuf_info(struct > >>> file *filep, > >>> /* Find a KFD GPU device that supports the get_dmabuf_info > >>> query */ > >>> for (i = 0; kfd_topology_enum_kfd_devices(i, &dev) == 0; i++) > >>> - if (dev) > >>> + if (dev && !kfd_devcgroup_check_permission(dev)) > >>> break; > >>> if (!dev) > >>> return -EINVAL; > >>> @@ -1545,7 +1545,7 @@ static int kfd_ioctl_get_dmabuf_info(struct > >>> file *filep, > >>> if (xcp_id >= 0) > >>> args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id; > >>> else > >>> - args->gpu_id = dmabuf_adev->kfd.dev->nodes[0]->id; > >>> + args->gpu_id = dev->id; > >> > >> If I remember correctly, this was meant as a fallback in case for GTT > >> BOs where the exporting partition wasn't known and the application > >> didn't have access to the first partition. I think the way you wrote > >> this, it could also change the behaviour (report the wrong GPU ID) on > >> single-partition GPUs, which is probably not intended. > > > > Never mind. I double checked: On single-partition GPUs, bo->xcp_id > > always seems to be 0. So your code won't change the behaviour here. > > The patch is > should bo->xcp_id be >= 0 for all cases? if yes then I think all tests can be > moved. (like below) > args->gpu_id = dmabuf_adev->kfd.dev->nodes[xcp_id]->id; > No it can be -1 on GPUs that support spatial partitioning, especially for GTT BOs. Regards, Mukul > David > > > > Reviewed-by: Felix Kuehling <felix.kuehling@xxxxxxx> > > > > > > > >> Maybe this would preserve the behaviour for that case: > >> > >> ... > >> - else > >> + else if > >> (!kfd_devcgroup_check_permission(dmabuf_adev->kfd.dev->nodes[0])) > >> args->gpu_id = dmabuf_adev->kfd.dev->nodes[0]->id; > >> + else > >> + args->gpu_id = dev->id; > >> > >> Or maybe a more general solution would make DMABuf import work when > >> the exporter is really unknown or not even a GPU. This came up not so > >> long ago in the context of interop with 3rd-party devices. This may > >> require user mode changes as well. > >> > >> Regards, > >> Felix > >> > >> > >>> args->flags = flags; > >>> /* Copy metadata buffer to user mode */
