[PATCH 8/9] drm/ttm: Don't count pages in SG BOs against pages_limit
Christian König
christian.koenig at amd.com
Wed Apr 14 12:49:28 UTC 2021
Am 14.04.21 um 14:47 schrieb Daniel Vetter:
> On Wed, Apr 14, 2021 at 2:43 PM Christian König
> <christian.koenig at 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 at 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 at amd.com>
>>>>>>>> Reviewed-by: Christian König <christian.koenig at 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 at 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
>
More information about the amd-gfx
mailing list