Re: [PATCH RFC 5/5] drm/amdgpu: Add accounting of buffer object creation request via DRM cgroup

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Am 27.11.18 um 19:15 schrieb Kenny Ho:
> Hey Christian,
>
> Sorry for the late reply, I missed this for some reason.
>
> On Wed, Nov 21, 2018 at 5:00 AM Christian König
> <ckoenig.leichtzumerken@gmail.com> wrote:
>>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>>> index 370e9a5536ef..531726443104 100644
>>> --- a/include/uapi/drm/amdgpu_drm.h
>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>> @@ -72,6 +72,18 @@ extern "C" {
>>>    #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>>    #define DRM_IOCTL_AMDGPU_SCHED              DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>>
>>> +enum amdgpu_mem_domain {
>>> +     AMDGPU_MEM_DOMAIN_CPU,
>>> +     AMDGPU_MEM_DOMAIN_GTT,
>>> +     AMDGPU_MEM_DOMAIN_VRAM,
>>> +     AMDGPU_MEM_DOMAIN_GDS,
>>> +     AMDGPU_MEM_DOMAIN_GWS,
>>> +     AMDGPU_MEM_DOMAIN_OA,
>>> +     __MAX_AMDGPU_MEM_DOMAIN
>>> +};
>> Well that is a clear NAK since it duplicates the TTM defines. Please use
>> that one instead and don't make this UAPI.
> This is defined to help with the chunk of changes below.  The
> AMDGPU_GEM_DOMAIN* already exists and this is similar to how TTM has
> TTM_PL_* to help with the creation of TTM_PL_FLAG_*:
> https://elixir.bootlin.com/linux/v4.20-rc4/source/include/drm/ttm/ttm_placement.h#L36
>
> I don't disagree that there is a duplication here but it's
> pre-existing so if you can help clarify my confusion that would be
> much appreciated.

The AMDGPU_GEM_DOMAIN are masks which are used in the frontend IOCTL 
interface to create BOs.

TTM defines the backend pools where the memory is then allocated from to 
fill the BOs.

So you are mixing frontend and backend here.

In other words for the whole cgroup interface you should not make a 
single change to amdgpu_drm.h or otherwise you are doing something wrong.

Regards,
Christian.

>
> Reards,
> Kenny
>
>>> +
>>> +extern char const *amdgpu_mem_domain_names[];
>>> +
>>>    /**
>>>     * DOC: memory domains
>>>     *
>>> @@ -95,12 +107,12 @@ extern "C" {
>>>     * %AMDGPU_GEM_DOMAIN_OA    Ordered append, used by 3D or Compute engines
>>>     * for appending data.
>>>     */
>>> -#define AMDGPU_GEM_DOMAIN_CPU                0x1
>>> -#define AMDGPU_GEM_DOMAIN_GTT                0x2
>>> -#define AMDGPU_GEM_DOMAIN_VRAM               0x4
>>> -#define AMDGPU_GEM_DOMAIN_GDS                0x8
>>> -#define AMDGPU_GEM_DOMAIN_GWS                0x10
>>> -#define AMDGPU_GEM_DOMAIN_OA         0x20
>>> +#define AMDGPU_GEM_DOMAIN_CPU                (1 << AMDGPU_MEM_DOMAIN_CPU)
>>> +#define AMDGPU_GEM_DOMAIN_GTT                (1 << AMDGPU_MEM_DOMAIN_GTT)
>>> +#define AMDGPU_GEM_DOMAIN_VRAM               (1 << AMDGPU_MEM_DOMAIN_VRAM)
>>> +#define AMDGPU_GEM_DOMAIN_GDS                (1 << AMDGPU_MEM_DOMAIN_GDS)
>>> +#define AMDGPU_GEM_DOMAIN_GWS                (1 << AMDGPU_MEM_DOMAIN_GWS)
>>> +#define AMDGPU_GEM_DOMAIN_OA         (1 << AMDGPU_MEM_DOMAIN_OA)
>>>    #define AMDGPU_GEM_DOMAIN_MASK              (AMDGPU_GEM_DOMAIN_CPU | \
>>>                                         AMDGPU_GEM_DOMAIN_GTT | \
>>>                                         AMDGPU_GEM_DOMAIN_VRAM | \
> _______________________________________________
> 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