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

[Kenny Ho]

Ah I see.  Thank you for the clarification.

Regards,
Kenny
On Tue, Nov 27, 2018 at 3:31 PM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> 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 at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>