From: "Thomas Hellström (Intel)" <thomas_os@shipmail.org> To: "Yu, Lang" <Lang.Yu@amd.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org> Cc: "Koenig, Christian" <Christian.Koenig@amd.com> Subject: Re: [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY Date: Mon, 31 May 2021 11:52:43 +0200 [thread overview] Message-ID: <14d7f047-cf6d-c84a-14ff-3f1d833a770b@shipmail.org> (raw) In-Reply-To: <DM6PR12MB4250B79297F587313D7645EBFB3F9@DM6PR12MB4250.namprd12.prod.outlook.com> Hi, Lang, On 5/31/21 10:19 AM, Yu, Lang wrote: > [Public] > >> Hi, >> On 5/27/21 3:30 AM, Lang Yu wrote: >>> Make TTM_PL_FLAG_* start from zero and add >>> TTM_PL_FLAG_TEMPORARY flag for temporary >>> GTT allocation use. >> GTT is a driver private acronym, right? And it doesn't look like >> TTM_PL_FLAG_TEMPORARY will be used in core TTM, so should we instead set >> aside a mask in the PL flag for driver-private use? > Hi Thomas, > > Thanks for your comments and advice, GTT means Graphics Translation Table here, seems > a general acronym. TTM_PL_FLAG_TEMPORARY may also be used by other drives. > I have made other patches for this. Please help review. > > Regards, > Lang > My point was really that the flag naming and documentation should reflect what core ttm is doing with that flag. If there is no specific core TTM usage, IMO we should move it to a driver specific flag to avoid future confusion. In particular a writer of a generic TTM resource manager should be able to know without looking at an old commit message what the placement flag is intended for. So here we add a flag where core TTM forces a bo move on validate and that's it. And that appears to be how it's used when an amdgpu bo is evicted to GTT, (btw should it be accounted in this situation?) The other use is to force the amdgpu driver to temporarily accept it into GTT when there is a lack of space, and IMHO that's a driver specific use and we should allocate a mask for driver specific flags for that. So shouldn't this be two flags, really? TTM_PL_FLAG_FORCE_MOVE and AMDGPU_PL_FLAG_TEMPORARY Thanks, /Thomas >> Thomas >> -----Original Message----- >> From: Yu, Lang <Lang.Yu@amd.com> >> Sent: Thursday, May 27, 2021 9:31 AM >> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Huang, Ray >> <Ray.Huang@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; >> Yu, Lang <Lang.Yu@amd.com> >> Subject: [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY >> >> Make TTM_PL_FLAG_* start from zero and add TTM_PL_FLAG_TEMPORARY flag >> for temporary GTT allocation use. >> >> Signed-off-by: Lang Yu <Lang.Yu@amd.com> >> --- >> include/drm/ttm/ttm_placement.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/include/drm/ttm/ttm_placement.h >> b/include/drm/ttm/ttm_placement.h index aa6ba4d0cf78..9f5cfc7c2d5a 100644 >> --- a/include/drm/ttm/ttm_placement.h >> +++ b/include/drm/ttm/ttm_placement.h >> @@ -47,8 +47,9 @@ >> * top of the memory area, instead of the bottom. >> */ >> >> -#define TTM_PL_FLAG_CONTIGUOUS (1 << 19) >> -#define TTM_PL_FLAG_TOPDOWN (1 << 22) >> +#define TTM_PL_FLAG_CONTIGUOUS (1 << 0) >> +#define TTM_PL_FLAG_TOPDOWN (1 << 1) >> +#define TTM_PL_FLAG_TEMPORARY (1 << 2) >> >> /** >> * struct ttm_place >> -- >> 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: "Thomas Hellström (Intel)" <thomas_os@shipmail.org> To: "Yu, Lang" <Lang.Yu@amd.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, "amd-gfx@lists.freedesktop.org" <amd-gfx@lists.freedesktop.org> Cc: "Koenig, Christian" <Christian.Koenig@amd.com> Subject: Re: [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY Date: Mon, 31 May 2021 11:52:43 +0200 [thread overview] Message-ID: <14d7f047-cf6d-c84a-14ff-3f1d833a770b@shipmail.org> (raw) In-Reply-To: <DM6PR12MB4250B79297F587313D7645EBFB3F9@DM6PR12MB4250.namprd12.prod.outlook.com> Hi, Lang, On 5/31/21 10:19 AM, Yu, Lang wrote: > [Public] > >> Hi, >> On 5/27/21 3:30 AM, Lang Yu wrote: >>> Make TTM_PL_FLAG_* start from zero and add >>> TTM_PL_FLAG_TEMPORARY flag for temporary >>> GTT allocation use. >> GTT is a driver private acronym, right? And it doesn't look like >> TTM_PL_FLAG_TEMPORARY will be used in core TTM, so should we instead set >> aside a mask in the PL flag for driver-private use? > Hi Thomas, > > Thanks for your comments and advice, GTT means Graphics Translation Table here, seems > a general acronym. TTM_PL_FLAG_TEMPORARY may also be used by other drives. > I have made other patches for this. Please help review. > > Regards, > Lang > My point was really that the flag naming and documentation should reflect what core ttm is doing with that flag. If there is no specific core TTM usage, IMO we should move it to a driver specific flag to avoid future confusion. In particular a writer of a generic TTM resource manager should be able to know without looking at an old commit message what the placement flag is intended for. So here we add a flag where core TTM forces a bo move on validate and that's it. And that appears to be how it's used when an amdgpu bo is evicted to GTT, (btw should it be accounted in this situation?) The other use is to force the amdgpu driver to temporarily accept it into GTT when there is a lack of space, and IMHO that's a driver specific use and we should allocate a mask for driver specific flags for that. So shouldn't this be two flags, really? TTM_PL_FLAG_FORCE_MOVE and AMDGPU_PL_FLAG_TEMPORARY Thanks, /Thomas >> Thomas >> -----Original Message----- >> From: Yu, Lang <Lang.Yu@amd.com> >> Sent: Thursday, May 27, 2021 9:31 AM >> To: amd-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org >> Cc: Koenig, Christian <Christian.Koenig@amd.com>; Huang, Ray >> <Ray.Huang@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; >> Yu, Lang <Lang.Yu@amd.com> >> Subject: [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY >> >> Make TTM_PL_FLAG_* start from zero and add TTM_PL_FLAG_TEMPORARY flag >> for temporary GTT allocation use. >> >> Signed-off-by: Lang Yu <Lang.Yu@amd.com> >> --- >> include/drm/ttm/ttm_placement.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/include/drm/ttm/ttm_placement.h >> b/include/drm/ttm/ttm_placement.h index aa6ba4d0cf78..9f5cfc7c2d5a 100644 >> --- a/include/drm/ttm/ttm_placement.h >> +++ b/include/drm/ttm/ttm_placement.h >> @@ -47,8 +47,9 @@ >> * top of the memory area, instead of the bottom. >> */ >> >> -#define TTM_PL_FLAG_CONTIGUOUS (1 << 19) >> -#define TTM_PL_FLAG_TOPDOWN (1 << 22) >> +#define TTM_PL_FLAG_CONTIGUOUS (1 << 0) >> +#define TTM_PL_FLAG_TOPDOWN (1 << 1) >> +#define TTM_PL_FLAG_TEMPORARY (1 << 2) >> >> /** >> * struct ttm_place >> -- >> 2.25.1 _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2021-05-31 9:52 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-27 1:30 [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY Lang Yu 2021-05-27 1:30 ` Lang Yu 2021-05-27 1:30 ` [PATCH 2/2] drm/amdgpu: stop bookkeeping of temporary GTT allocation Lang Yu 2021-05-27 1:30 ` Lang Yu 2021-05-27 11:51 ` Christian König 2021-05-27 11:51 ` Christian König 2021-05-28 9:47 ` Yu, Lang 2021-05-28 9:47 ` Yu, Lang 2021-05-28 12:23 ` Christian König 2021-05-28 12:23 ` Christian König 2021-05-27 7:42 ` [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY Thomas Hellström (Intel) 2021-05-27 11:45 ` Christian König 2021-05-27 11:45 ` Christian König 2021-05-31 8:19 ` Yu, Lang 2021-05-31 8:19 ` Yu, Lang 2021-05-31 9:52 ` Thomas Hellström (Intel) [this message] 2021-05-31 9:52 ` Thomas Hellström (Intel) 2021-05-31 10:32 ` Christian König 2021-05-31 10:32 ` Christian König 2021-05-31 10:46 ` Thomas Hellström (Intel) 2021-05-31 10:46 ` Thomas Hellström (Intel) 2021-05-31 10:56 ` Christian König 2021-05-31 10:56 ` Christian König 2021-05-31 11:19 ` Thomas Hellström (Intel) 2021-05-31 11:19 ` Thomas Hellström (Intel) 2021-05-31 12:02 ` Christian König 2021-05-31 12:02 ` Christian König 2021-05-31 12:09 ` Thomas Hellström (Intel) 2021-05-31 12:09 ` Thomas Hellström (Intel)
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=14d7f047-cf6d-c84a-14ff-3f1d833a770b@shipmail.org \ --to=thomas_os@shipmail.org \ --cc=Christian.Koenig@amd.com \ --cc=Lang.Yu@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.