| Message ID | 20210414064804.29356-9-Felix.Kuehling@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement multi-GPU DMA mappings for KFD | expand |
Am 14.04.21 um 08:48 schrieb Felix Kuehling: > Pages in SG BOs were not allocated by TTM. So don't count them against > TTM's pages limit. > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> Going to pick that one up for inclusion in drm-misc-next. Regards, Christian. > --- > drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > index 5d8820725b75..e8b8c3257392 100644 > --- a/drivers/gpu/drm/ttm/ttm_tt.c > +++ b/drivers/gpu/drm/ttm/ttm_tt.c > @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > if (ttm_tt_is_populated(ttm)) > return 0; > > - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > - if (bdev->pool.use_dma32) > - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > + if (bdev->pool.use_dma32) > + atomic_long_add(ttm->num_pages, > + &ttm_dma32_pages_allocated); > + } > > while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || > atomic_long_read(&ttm_dma32_pages_allocated) > > @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > return 0; > > error: > - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > - if (bdev->pool.use_dma32) > - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > + if (bdev->pool.use_dma32) > + atomic_long_sub(ttm->num_pages, > + &ttm_dma32_pages_allocated); > + } > return ret; > } > EXPORT_SYMBOL(ttm_tt_populate); > @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) > else > ttm_pool_free(&bdev->pool, ttm); > > - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > - if (bdev->pool.use_dma32) > - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > + if (bdev->pool.use_dma32) > + atomic_long_sub(ttm->num_pages, > + &ttm_dma32_pages_allocated); > + } > > ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; > }
On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: > Am 14.04.21 um 08:48 schrieb Felix Kuehling: > > Pages in SG BOs were not allocated by TTM. So don't count them against > > TTM's pages limit. > > > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > > Reviewed-by: Christian König <christian.koenig@amd.com> > > Going to pick that one up for inclusion in drm-misc-next. See my other email, but why do we need this? A bit more explanation is imo needed here at least, since we still need to guarantee that allocations don't over the limit in total for all gpu buffers together. At least until the shrinker has landed. And this here just opens up the barn door without any explanation why it's ok. -Daniel > > Regards, > Christian. > > > --- > > drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > > index 5d8820725b75..e8b8c3257392 100644 > > --- a/drivers/gpu/drm/ttm/ttm_tt.c > > +++ b/drivers/gpu/drm/ttm/ttm_tt.c > > @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > > if (ttm_tt_is_populated(ttm)) > > return 0; > > - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > > - if (bdev->pool.use_dma32) > > - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); > > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > > + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > > + if (bdev->pool.use_dma32) > > + atomic_long_add(ttm->num_pages, > > + &ttm_dma32_pages_allocated); > > + } > > while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || > > atomic_long_read(&ttm_dma32_pages_allocated) > > > @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > > return 0; > > error: > > - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > - if (bdev->pool.use_dma32) > > - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > > + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > + if (bdev->pool.use_dma32) > > + atomic_long_sub(ttm->num_pages, > > + &ttm_dma32_pages_allocated); > > + } > > return ret; > > } > > EXPORT_SYMBOL(ttm_tt_populate); > > @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) > > else > > ttm_pool_free(&bdev->pool, ttm); > > - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > - if (bdev->pool.use_dma32) > > - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > > + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > + if (bdev->pool.use_dma32) > > + atomic_long_sub(ttm->num_pages, > > + &ttm_dma32_pages_allocated); > > + } > > ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; > > } > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 14.04.21 um 11:15 schrieb Daniel Vetter: > On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: >> Am 14.04.21 um 08:48 schrieb Felix Kuehling: >>> Pages in SG BOs were not allocated by TTM. So don't count them against >>> TTM's pages limit. >>> >>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >> Reviewed-by: Christian König <christian.koenig@amd.com> >> >> Going to pick that one up for inclusion in drm-misc-next. > See my other email, but why do we need this? A bit more explanation is imo > needed here at least, since we still need to guarantee that allocations > don't over the limit in total for all gpu buffers together. At least until > the shrinker has landed. > > And this here just opens up the barn door without any explanation why it's > ok. The SG based BOs might not even be backed by pages. E.g. exported VRAM. So either they are exported by a driver which should have accounted for the allocation, exported by TTM which already did the accounting or doesn't even point to pages at all. This is really a bug fix to recreate the behavior we had before moving the accounting to this place. Christian. > -Daniel > >> Regards, >> Christian. >> >>> --- >>> drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- >>> 1 file changed, 18 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c >>> index 5d8820725b75..e8b8c3257392 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_tt.c >>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >>> @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>> if (ttm_tt_is_populated(ttm)) >>> return 0; >>> - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>> - if (bdev->pool.use_dma32) >>> - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); >>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>> + if (bdev->pool.use_dma32) >>> + atomic_long_add(ttm->num_pages, >>> + &ttm_dma32_pages_allocated); >>> + } >>> while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || >>> atomic_long_read(&ttm_dma32_pages_allocated) > >>> @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>> return 0; >>> error: >>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>> - if (bdev->pool.use_dma32) >>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>> + if (bdev->pool.use_dma32) >>> + atomic_long_sub(ttm->num_pages, >>> + &ttm_dma32_pages_allocated); >>> + } >>> return ret; >>> } >>> EXPORT_SYMBOL(ttm_tt_populate); >>> @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) >>> else >>> ttm_pool_free(&bdev->pool, ttm); >>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>> - if (bdev->pool.use_dma32) >>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>> + if (bdev->pool.use_dma32) >>> + atomic_long_sub(ttm->num_pages, >>> + &ttm_dma32_pages_allocated); >>> + } >>> ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; >>> } >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C3075d7fd16644322a13608d8ff25d59b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539885255795187%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KOnHA1CbNFjjMZR2rgHmGkH%2B7C84YCtA6u9V1wBAay4%3D&reserved=0
On Wed, Apr 14, 2021 at 11:19:41AM +0200, Christian König wrote: > Am 14.04.21 um 11:15 schrieb Daniel Vetter: > > On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: > > > Am 14.04.21 um 08:48 schrieb Felix Kuehling: > > > > Pages in SG BOs were not allocated by TTM. So don't count them against > > > > TTM's pages limit. > > > > > > > > Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > > > Reviewed-by: Christian König <christian.koenig@amd.com> > > > > > > Going to pick that one up for inclusion in drm-misc-next. > > See my other email, but why do we need this? A bit more explanation is imo > > needed here at least, since we still need to guarantee that allocations > > don't over the limit in total for all gpu buffers together. At least until > > the shrinker has landed. > > > > And this here just opens up the barn door without any explanation why it's > > ok. > > The SG based BOs might not even be backed by pages. E.g. exported VRAM. > > So either they are exported by a driver which should have accounted for the > allocation, exported by TTM which already did the accounting or doesn't even > point to pages at all. > > This is really a bug fix to recreate the behavior we had before moving the > accounting to this place. Throw that into the commit message and a-b: me. Ideally with a Fixes: line or so pointing at the offending commit that broke stuff. Commit messages should really go into more detail when there's an entire story behind a small change like this one. -Daniel > > Christian. > > > -Daniel > > > > > Regards, > > > Christian. > > > > > > > --- > > > > drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- > > > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > > > > index 5d8820725b75..e8b8c3257392 100644 > > > > --- a/drivers/gpu/drm/ttm/ttm_tt.c > > > > +++ b/drivers/gpu/drm/ttm/ttm_tt.c > > > > @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > > > > if (ttm_tt_is_populated(ttm)) > > > > return 0; > > > > - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > > > > - if (bdev->pool.use_dma32) > > > > - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); > > > > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > > > > + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > > > > + if (bdev->pool.use_dma32) > > > > + atomic_long_add(ttm->num_pages, > > > > + &ttm_dma32_pages_allocated); > > > > + } > > > > while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || > > > > atomic_long_read(&ttm_dma32_pages_allocated) > > > > > @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > > > > return 0; > > > > error: > > > > - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > > > - if (bdev->pool.use_dma32) > > > > - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > > > > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > > > > + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > > > + if (bdev->pool.use_dma32) > > > > + atomic_long_sub(ttm->num_pages, > > > > + &ttm_dma32_pages_allocated); > > > > + } > > > > return ret; > > > > } > > > > EXPORT_SYMBOL(ttm_tt_populate); > > > > @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) > > > > else > > > > ttm_pool_free(&bdev->pool, ttm); > > > > - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > > > - if (bdev->pool.use_dma32) > > > > - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > > > > + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > > > > + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > > > > + if (bdev->pool.use_dma32) > > > > + atomic_long_sub(ttm->num_pages, > > > > + &ttm_dma32_pages_allocated); > > > > + } > > > > ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; > > > > } > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C3075d7fd16644322a13608d8ff25d59b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539885255795187%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KOnHA1CbNFjjMZR2rgHmGkH%2B7C84YCtA6u9V1wBAay4%3D&reserved=0 >
Am 14.04.21 um 12:26 schrieb Daniel Vetter: > On Wed, Apr 14, 2021 at 11:19:41AM +0200, Christian König wrote: >> Am 14.04.21 um 11:15 schrieb Daniel Vetter: >>> On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: >>>> Am 14.04.21 um 08:48 schrieb Felix Kuehling: >>>>> Pages in SG BOs were not allocated by TTM. So don't count them against >>>>> TTM's pages limit. >>>>> >>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>> Reviewed-by: Christian König <christian.koenig@amd.com> >>>> >>>> Going to pick that one up for inclusion in drm-misc-next. >>> See my other email, but why do we need this? A bit more explanation is imo >>> needed here at least, since we still need to guarantee that allocations >>> don't over the limit in total for all gpu buffers together. At least until >>> the shrinker has landed. >>> >>> And this here just opens up the barn door without any explanation why it's >>> ok. >> The SG based BOs might not even be backed by pages. E.g. exported VRAM. >> >> So either they are exported by a driver which should have accounted for the >> allocation, exported by TTM which already did the accounting or doesn't even >> point to pages at all. >> >> This is really a bug fix to recreate the behavior we had before moving the >> accounting to this place. > Throw that into the commit message and a-b: me. Ideally with a Fixes: line > or so pointing at the offending commit that broke stuff. Commit messages > should really go into more detail when there's an entire story behind a > small change like this one. Sorry I though that this would be obvious :) I've already pushed the patch in the morning, but going to keep that in mind for the next time. Christian. > -Daniel > >> Christian. >> >>> -Daniel >>> >>>> Regards, >>>> Christian. >>>> >>>>> --- >>>>> drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- >>>>> 1 file changed, 18 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c >>>>> index 5d8820725b75..e8b8c3257392 100644 >>>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c >>>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >>>>> @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>>>> if (ttm_tt_is_populated(ttm)) >>>>> return 0; >>>>> - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>>>> - if (bdev->pool.use_dma32) >>>>> - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>>>> + if (bdev->pool.use_dma32) >>>>> + atomic_long_add(ttm->num_pages, >>>>> + &ttm_dma32_pages_allocated); >>>>> + } >>>>> while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || >>>>> atomic_long_read(&ttm_dma32_pages_allocated) > >>>>> @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>>>> return 0; >>>>> error: >>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>> - if (bdev->pool.use_dma32) >>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>> + if (bdev->pool.use_dma32) >>>>> + atomic_long_sub(ttm->num_pages, >>>>> + &ttm_dma32_pages_allocated); >>>>> + } >>>>> return ret; >>>>> } >>>>> EXPORT_SYMBOL(ttm_tt_populate); >>>>> @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) >>>>> else >>>>> ttm_pool_free(&bdev->pool, ttm); >>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>> - if (bdev->pool.use_dma32) >>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>> + if (bdev->pool.use_dma32) >>>>> + atomic_long_sub(ttm->num_pages, >>>>> + &ttm_dma32_pages_allocated); >>>>> + } >>>>> ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; >>>>> } >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C3075d7fd16644322a13608d8ff25d59b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539885255795187%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KOnHA1CbNFjjMZR2rgHmGkH%2B7C84YCtA6u9V1wBAay4%3D&reserved=0
On Wed, Apr 14, 2021 at 12:49 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 14.04.21 um 12:26 schrieb Daniel Vetter: > > On Wed, Apr 14, 2021 at 11:19:41AM +0200, Christian König wrote: > >> Am 14.04.21 um 11:15 schrieb Daniel Vetter: > >>> On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: > >>>> Am 14.04.21 um 08:48 schrieb Felix Kuehling: > >>>>> Pages in SG BOs were not allocated by TTM. So don't count them against > >>>>> TTM's pages limit. > >>>>> > >>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > >>>> Reviewed-by: Christian König <christian.koenig@amd.com> > >>>> > >>>> Going to pick that one up for inclusion in drm-misc-next. > >>> See my other email, but why do we need this? A bit more explanation is imo > >>> needed here at least, since we still need to guarantee that allocations > >>> don't over the limit in total for all gpu buffers together. At least until > >>> the shrinker has landed. > >>> > >>> And this here just opens up the barn door without any explanation why it's > >>> ok. > >> The SG based BOs might not even be backed by pages. E.g. exported VRAM. > >> > >> So either they are exported by a driver which should have accounted for the > >> allocation, exported by TTM which already did the accounting or doesn't even > >> point to pages at all. > >> > >> This is really a bug fix to recreate the behavior we had before moving the > >> accounting to this place. > > Throw that into the commit message and a-b: me. Ideally with a Fixes: line > > or so pointing at the offending commit that broke stuff. Commit messages > > should really go into more detail when there's an entire story behind a > > small change like this one. > > Sorry I though that this would be obvious :) > > I've already pushed the patch in the morning, but going to keep that in > mind for the next time. I'll keep reminding you to pls elaborate more in commit messages, it's coming up every once in a while :-) Also in general I think a few days of letting patches soak out there, especially shared code, is good curtesy. Some folks demand 2 weeks, which I think is too much, but less than 24h just means you're guaranteed to leave out half the globe with their feedback. Which isn't great. Driver code I don't care since there you know all the stakeholders ofc. -Daniel > > Christian. > > > -Daniel > > > >> Christian. > >> > >>> -Daniel > >>> > >>>> Regards, > >>>> Christian. > >>>> > >>>>> --- > >>>>> drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- > >>>>> 1 file changed, 18 insertions(+), 9 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > >>>>> index 5d8820725b75..e8b8c3257392 100644 > >>>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c > >>>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c > >>>>> @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > >>>>> if (ttm_tt_is_populated(ttm)) > >>>>> return 0; > >>>>> - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > >>>>> - if (bdev->pool.use_dma32) > >>>>> - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); > >>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > >>>>> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > >>>>> + if (bdev->pool.use_dma32) > >>>>> + atomic_long_add(ttm->num_pages, > >>>>> + &ttm_dma32_pages_allocated); > >>>>> + } > >>>>> while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || > >>>>> atomic_long_read(&ttm_dma32_pages_allocated) > > >>>>> @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > >>>>> return 0; > >>>>> error: > >>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>> - if (bdev->pool.use_dma32) > >>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > >>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > >>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>> + if (bdev->pool.use_dma32) > >>>>> + atomic_long_sub(ttm->num_pages, > >>>>> + &ttm_dma32_pages_allocated); > >>>>> + } > >>>>> return ret; > >>>>> } > >>>>> EXPORT_SYMBOL(ttm_tt_populate); > >>>>> @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) > >>>>> else > >>>>> ttm_pool_free(&bdev->pool, ttm); > >>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>> - if (bdev->pool.use_dma32) > >>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > >>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > >>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>> + if (bdev->pool.use_dma32) > >>>>> + atomic_long_sub(ttm->num_pages, > >>>>> + &ttm_dma32_pages_allocated); > >>>>> + } > >>>>> ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; > >>>>> } > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@lists.freedesktop.org > >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C3075d7fd16644322a13608d8ff25d59b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539885255795187%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=KOnHA1CbNFjjMZR2rgHmGkH%2B7C84YCtA6u9V1wBAay4%3D&reserved=0 >
Am 14.04.21 um 14:25 schrieb Daniel Vetter: > On Wed, Apr 14, 2021 at 12:49 PM Christian König > <ckoenig.leichtzumerken@gmail.com> wrote: >> Am 14.04.21 um 12:26 schrieb Daniel Vetter: >>> On Wed, Apr 14, 2021 at 11:19:41AM +0200, Christian König wrote: >>>> Am 14.04.21 um 11:15 schrieb Daniel Vetter: >>>>> On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: >>>>>> Am 14.04.21 um 08:48 schrieb Felix Kuehling: >>>>>>> Pages in SG BOs were not allocated by TTM. So don't count them against >>>>>>> TTM's pages limit. >>>>>>> >>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>> Reviewed-by: Christian König <christian.koenig@amd.com> >>>>>> >>>>>> Going to pick that one up for inclusion in drm-misc-next. >>>>> See my other email, but why do we need this? A bit more explanation is imo >>>>> needed here at least, since we still need to guarantee that allocations >>>>> don't over the limit in total for all gpu buffers together. At least until >>>>> the shrinker has landed. >>>>> >>>>> And this here just opens up the barn door without any explanation why it's >>>>> ok. >>>> The SG based BOs might not even be backed by pages. E.g. exported VRAM. >>>> >>>> So either they are exported by a driver which should have accounted for the >>>> allocation, exported by TTM which already did the accounting or doesn't even >>>> point to pages at all. >>>> >>>> This is really a bug fix to recreate the behavior we had before moving the >>>> accounting to this place. >>> Throw that into the commit message and a-b: me. Ideally with a Fixes: line >>> or so pointing at the offending commit that broke stuff. Commit messages >>> should really go into more detail when there's an entire story behind a >>> small change like this one. >> Sorry I though that this would be obvious :) >> >> I've already pushed the patch in the morning, but going to keep that in >> mind for the next time. > I'll keep reminding you to pls elaborate more in commit messages, it's > coming up every once in a while :-) Well, describing stuff in a commit message which is obvious is just redundant. I can of course explain the whole background of the code in question in the commit message, but for obvious bug fixes like this it is just overkill. > Also in general I think a few days of letting patches soak out there, > especially shared code, is good curtesy. Some folks demand 2 weeks, > which I think is too much, but less than 24h just means you're > guaranteed to leave out half the globe with their feedback. Which > isn't great. Well for structural changes I certainly agree, but not for a bug fix which adds a missing check for a special case. Christian. > > Driver code I don't care since there you know all the stakeholders ofc. > -Daniel > >> Christian. >> >>> -Daniel >>> >>>> Christian. >>>> >>>>> -Daniel >>>>> >>>>>> Regards, >>>>>> Christian. >>>>>> >>>>>>> --- >>>>>>> drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- >>>>>>> 1 file changed, 18 insertions(+), 9 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c >>>>>>> index 5d8820725b75..e8b8c3257392 100644 >>>>>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c >>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >>>>>>> @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>>>>>> if (ttm_tt_is_populated(ttm)) >>>>>>> return 0; >>>>>>> - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>>>>>> - if (bdev->pool.use_dma32) >>>>>>> - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>>>> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>>>>>> + if (bdev->pool.use_dma32) >>>>>>> + atomic_long_add(ttm->num_pages, >>>>>>> + &ttm_dma32_pages_allocated); >>>>>>> + } >>>>>>> while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || >>>>>>> atomic_long_read(&ttm_dma32_pages_allocated) > >>>>>>> @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>>>>>> return 0; >>>>>>> error: >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>> - if (bdev->pool.use_dma32) >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>> + if (bdev->pool.use_dma32) >>>>>>> + atomic_long_sub(ttm->num_pages, >>>>>>> + &ttm_dma32_pages_allocated); >>>>>>> + } >>>>>>> return ret; >>>>>>> } >>>>>>> EXPORT_SYMBOL(ttm_tt_populate); >>>>>>> @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) >>>>>>> else >>>>>>> ttm_pool_free(&bdev->pool, ttm); >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>> - if (bdev->pool.use_dma32) >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>> + if (bdev->pool.use_dma32) >>>>>>> + atomic_long_sub(ttm->num_pages, >>>>>>> + &ttm_dma32_pages_allocated); >>>>>>> + } >>>>>>> ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; >>>>>>> } >>>>>> _______________________________________________ >>>>>> dri-devel mailing list >>>>>> dri-devel@lists.freedesktop.org >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C503f240d409740c1333508d8ff406545%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539999355330481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6sW53%2FGpxk4rZKM7mpHDfgBhreCXY4McypKGqTH13b8%3D&reserved=0 >
On Wed, Apr 14, 2021 at 2:43 PM Christian König <christian.koenig@amd.com> wrote: > > Am 14.04.21 um 14:25 schrieb Daniel Vetter: > > On Wed, Apr 14, 2021 at 12:49 PM Christian König > > <ckoenig.leichtzumerken@gmail.com> wrote: > >> Am 14.04.21 um 12:26 schrieb Daniel Vetter: > >>> On Wed, Apr 14, 2021 at 11:19:41AM +0200, Christian König wrote: > >>>> Am 14.04.21 um 11:15 schrieb Daniel Vetter: > >>>>> On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: > >>>>>> Am 14.04.21 um 08:48 schrieb Felix Kuehling: > >>>>>>> Pages in SG BOs were not allocated by TTM. So don't count them against > >>>>>>> TTM's pages limit. > >>>>>>> > >>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> > >>>>>> Reviewed-by: Christian König <christian.koenig@amd.com> > >>>>>> > >>>>>> Going to pick that one up for inclusion in drm-misc-next. > >>>>> See my other email, but why do we need this? A bit more explanation is imo > >>>>> needed here at least, since we still need to guarantee that allocations > >>>>> don't over the limit in total for all gpu buffers together. At least until > >>>>> the shrinker has landed. > >>>>> > >>>>> And this here just opens up the barn door without any explanation why it's > >>>>> ok. > >>>> The SG based BOs might not even be backed by pages. E.g. exported VRAM. > >>>> > >>>> So either they are exported by a driver which should have accounted for the > >>>> allocation, exported by TTM which already did the accounting or doesn't even > >>>> point to pages at all. > >>>> > >>>> This is really a bug fix to recreate the behavior we had before moving the > >>>> accounting to this place. > >>> Throw that into the commit message and a-b: me. Ideally with a Fixes: line > >>> or so pointing at the offending commit that broke stuff. Commit messages > >>> should really go into more detail when there's an entire story behind a > >>> small change like this one. > >> Sorry I though that this would be obvious :) > >> > >> I've already pushed the patch in the morning, but going to keep that in > >> mind for the next time. > > I'll keep reminding you to pls elaborate more in commit messages, it's > > coming up every once in a while :-) > > Well, describing stuff in a commit message which is obvious is just > redundant. > > I can of course explain the whole background of the code in question in > the commit message, but for obvious bug fixes like this it is just overkill. > > > Also in general I think a few days of letting patches soak out there, > > especially shared code, is good curtesy. Some folks demand 2 weeks, > > which I think is too much, but less than 24h just means you're > > guaranteed to leave out half the globe with their feedback. Which > > isn't great. > > Well for structural changes I certainly agree, but not for a bug fix > which adds a missing check for a special case. Well if it's a bugfix should at least indicate that, and regression fixes should come with Fixes: tags. Obvious for you who screamed at the code is generally not implying it's obvious for anyone else. So yeah I think in general more explanations would be good. -Daniel > > Christian. > > > > > Driver code I don't care since there you know all the stakeholders ofc. > > -Daniel > > > >> Christian. > >> > >>> -Daniel > >>> > >>>> Christian. > >>>> > >>>>> -Daniel > >>>>> > >>>>>> Regards, > >>>>>> Christian. > >>>>>> > >>>>>>> --- > >>>>>>> drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- > >>>>>>> 1 file changed, 18 insertions(+), 9 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > >>>>>>> index 5d8820725b75..e8b8c3257392 100644 > >>>>>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c > >>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c > >>>>>>> @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > >>>>>>> if (ttm_tt_is_populated(ttm)) > >>>>>>> return 0; > >>>>>>> - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > >>>>>>> - if (bdev->pool.use_dma32) > >>>>>>> - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); > >>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > >>>>>>> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); > >>>>>>> + if (bdev->pool.use_dma32) > >>>>>>> + atomic_long_add(ttm->num_pages, > >>>>>>> + &ttm_dma32_pages_allocated); > >>>>>>> + } > >>>>>>> while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || > >>>>>>> atomic_long_read(&ttm_dma32_pages_allocated) > > >>>>>>> @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, > >>>>>>> return 0; > >>>>>>> error: > >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>>>> - if (bdev->pool.use_dma32) > >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > >>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > >>>>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>>>> + if (bdev->pool.use_dma32) > >>>>>>> + atomic_long_sub(ttm->num_pages, > >>>>>>> + &ttm_dma32_pages_allocated); > >>>>>>> + } > >>>>>>> return ret; > >>>>>>> } > >>>>>>> EXPORT_SYMBOL(ttm_tt_populate); > >>>>>>> @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) > >>>>>>> else > >>>>>>> ttm_pool_free(&bdev->pool, ttm); > >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>>>> - if (bdev->pool.use_dma32) > >>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); > >>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { > >>>>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); > >>>>>>> + if (bdev->pool.use_dma32) > >>>>>>> + atomic_long_sub(ttm->num_pages, > >>>>>>> + &ttm_dma32_pages_allocated); > >>>>>>> + } > >>>>>>> ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; > >>>>>>> } > >>>>>> _______________________________________________ > >>>>>> dri-devel mailing list > >>>>>> dri-devel@lists.freedesktop.org > >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C503f240d409740c1333508d8ff406545%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539999355330481%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=6sW53%2FGpxk4rZKM7mpHDfgBhreCXY4McypKGqTH13b8%3D&reserved=0 > > >
Am 14.04.21 um 14:47 schrieb Daniel Vetter: > On Wed, Apr 14, 2021 at 2:43 PM Christian König > <christian.koenig@amd.com> wrote: >> Am 14.04.21 um 14:25 schrieb Daniel Vetter: >>> On Wed, Apr 14, 2021 at 12:49 PM Christian König >>> <ckoenig.leichtzumerken@gmail.com> wrote: >>>> Am 14.04.21 um 12:26 schrieb Daniel Vetter: >>>>> On Wed, Apr 14, 2021 at 11:19:41AM +0200, Christian König wrote: >>>>>> Am 14.04.21 um 11:15 schrieb Daniel Vetter: >>>>>>> On Wed, Apr 14, 2021 at 08:51:51AM +0200, Christian König wrote: >>>>>>>> Am 14.04.21 um 08:48 schrieb Felix Kuehling: >>>>>>>>> Pages in SG BOs were not allocated by TTM. So don't count them against >>>>>>>>> TTM's pages limit. >>>>>>>>> >>>>>>>>> Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> >>>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com> >>>>>>>> >>>>>>>> Going to pick that one up for inclusion in drm-misc-next. >>>>>>> See my other email, but why do we need this? A bit more explanation is imo >>>>>>> needed here at least, since we still need to guarantee that allocations >>>>>>> don't over the limit in total for all gpu buffers together. At least until >>>>>>> the shrinker has landed. >>>>>>> >>>>>>> And this here just opens up the barn door without any explanation why it's >>>>>>> ok. >>>>>> The SG based BOs might not even be backed by pages. E.g. exported VRAM. >>>>>> >>>>>> So either they are exported by a driver which should have accounted for the >>>>>> allocation, exported by TTM which already did the accounting or doesn't even >>>>>> point to pages at all. >>>>>> >>>>>> This is really a bug fix to recreate the behavior we had before moving the >>>>>> accounting to this place. >>>>> Throw that into the commit message and a-b: me. Ideally with a Fixes: line >>>>> or so pointing at the offending commit that broke stuff. Commit messages >>>>> should really go into more detail when there's an entire story behind a >>>>> small change like this one. >>>> Sorry I though that this would be obvious :) >>>> >>>> I've already pushed the patch in the morning, but going to keep that in >>>> mind for the next time. >>> I'll keep reminding you to pls elaborate more in commit messages, it's >>> coming up every once in a while :-) >> Well, describing stuff in a commit message which is obvious is just >> redundant. >> >> I can of course explain the whole background of the code in question in >> the commit message, but for obvious bug fixes like this it is just overkill. >> >>> Also in general I think a few days of letting patches soak out there, >>> especially shared code, is good curtesy. Some folks demand 2 weeks, >>> which I think is too much, but less than 24h just means you're >>> guaranteed to leave out half the globe with their feedback. Which >>> isn't great. >> Well for structural changes I certainly agree, but not for a bug fix >> which adds a missing check for a special case. > Well if it's a bugfix should at least indicate that, and regression > fixes should come with Fixes: tags. Obvious for you who screamed at > the code is generally not implying it's obvious for anyone else. So > yeah I think in general more explanations would be good. Ok, seconded. The missing Fixes tag is a good point and the wording doesn't made it clear that this is a bug fix. Going to keep that in mind. Christian. > -Daniel > >> Christian. >> >>> Driver code I don't care since there you know all the stakeholders ofc. >>> -Daniel >>> >>>> Christian. >>>> >>>>> -Daniel >>>>> >>>>>> Christian. >>>>>> >>>>>>> -Daniel >>>>>>> >>>>>>>> Regards, >>>>>>>> Christian. >>>>>>>> >>>>>>>>> --- >>>>>>>>> drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- >>>>>>>>> 1 file changed, 18 insertions(+), 9 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c >>>>>>>>> index 5d8820725b75..e8b8c3257392 100644 >>>>>>>>> --- a/drivers/gpu/drm/ttm/ttm_tt.c >>>>>>>>> +++ b/drivers/gpu/drm/ttm/ttm_tt.c >>>>>>>>> @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>>>>>>>> if (ttm_tt_is_populated(ttm)) >>>>>>>>> return 0; >>>>>>>>> - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>>>>>>>> - if (bdev->pool.use_dma32) >>>>>>>>> - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>>>>>> + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); >>>>>>>>> + if (bdev->pool.use_dma32) >>>>>>>>> + atomic_long_add(ttm->num_pages, >>>>>>>>> + &ttm_dma32_pages_allocated); >>>>>>>>> + } >>>>>>>>> while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || >>>>>>>>> atomic_long_read(&ttm_dma32_pages_allocated) > >>>>>>>>> @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, >>>>>>>>> return 0; >>>>>>>>> error: >>>>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>>>> - if (bdev->pool.use_dma32) >>>>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>>>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>>>> + if (bdev->pool.use_dma32) >>>>>>>>> + atomic_long_sub(ttm->num_pages, >>>>>>>>> + &ttm_dma32_pages_allocated); >>>>>>>>> + } >>>>>>>>> return ret; >>>>>>>>> } >>>>>>>>> EXPORT_SYMBOL(ttm_tt_populate); >>>>>>>>> @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) >>>>>>>>> else >>>>>>>>> ttm_pool_free(&bdev->pool, ttm); >>>>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>>>> - if (bdev->pool.use_dma32) >>>>>>>>> - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); >>>>>>>>> + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { >>>>>>>>> + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); >>>>>>>>> + if (bdev->pool.use_dma32) >>>>>>>>> + atomic_long_sub(ttm->num_pages, >>>>>>>>> + &ttm_dma32_pages_allocated); >>>>>>>>> + } >>>>>>>>> ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; >>>>>>>>> } >>>>>>>> _______________________________________________ >>>>>>>> dri-devel mailing list >>>>>>>> dri-devel@lists.freedesktop.org >>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel&data=04%7C01%7Cchristian.koenig%40amd.com%7C1be30de6774b44ce302808d8ff437774%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637540012543510114%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=R5IkjKJV%2BmGDul5YqoUCDQKCNaYtbm3oOqT9fTF%2Bguk%3D&reserved=0 >
Am 2021-04-14 um 8:25 a.m. schrieb Daniel Vetter: >> Sorry I though that this would be obvious :) >> >> I've already pushed the patch in the morning, but going to keep that in >> mind for the next time. > I'll keep reminding you to pls elaborate more in commit messages, it's > coming up every once in a while :-) It was actually my patch and commit message. I was not aware of the history of this bug or the fact that it was a regression. Otherwise I would have included a "Fixes:" tag. From my point of view it was just a pretty obvious problem exposed when testing my DMA mapping patch series for KFD. Regards, Felix > > Also in general I think a few days of letting patches soak out there, > especially shared code, is good curtesy. Some folks demand 2 weeks, > which I think is too much, but less than 24h just means you're > guaranteed to leave out half the globe with their feedback. Which > isn't great. > > Driver code I don't care since there you know all the stakeholders ofc. > -Daniel >
diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 5d8820725b75..e8b8c3257392 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -317,9 +317,12 @@ int ttm_tt_populate(struct ttm_device *bdev, if (ttm_tt_is_populated(ttm)) return 0; - atomic_long_add(ttm->num_pages, &ttm_pages_allocated); - if (bdev->pool.use_dma32) - atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated); + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { + atomic_long_add(ttm->num_pages, &ttm_pages_allocated); + if (bdev->pool.use_dma32) + atomic_long_add(ttm->num_pages, + &ttm_dma32_pages_allocated); + } while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit || atomic_long_read(&ttm_dma32_pages_allocated) > @@ -350,9 +353,12 @@ int ttm_tt_populate(struct ttm_device *bdev, return 0; error: - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); - if (bdev->pool.use_dma32) - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); + if (bdev->pool.use_dma32) + atomic_long_sub(ttm->num_pages, + &ttm_dma32_pages_allocated); + } return ret; } EXPORT_SYMBOL(ttm_tt_populate); @@ -382,9 +388,12 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm) else ttm_pool_free(&bdev->pool, ttm); - atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); - if (bdev->pool.use_dma32) - atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated); + if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) { + atomic_long_sub(ttm->num_pages, &ttm_pages_allocated); + if (bdev->pool.use_dma32) + atomic_long_sub(ttm->num_pages, + &ttm_dma32_pages_allocated); + } ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED; }
Pages in SG BOs were not allocated by TTM. So don't count them against TTM's pages limit. Signed-off-by: Felix Kuehling <Felix.Kuehling@amd.com> --- drivers/gpu/drm/ttm/ttm_tt.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)