ttm_resource_manager::use_tt

ttm_resource_manager::use_tt

[Zack Rusin]

Hi, Christian.

I was just going over some old patches and I was just looking at your series introducing use_tt:
https://patchwork.freedesktop.org/series/80078/ and the change https://patchwork.freedesktop.org/patch/382079/?series=80078&rev=1

Do you happen to remember what was the worry behind the /* TODO: This is most likely not correct */ in vmwgfx_ttm_buffer.c? I'm trying to figure out if we need to address it.

z

Re: ttm_resource_manager::use_tt

[Christian König]

Hi Zack,

IIRC that was for the VMW_PL_GMR type, wasn't it?

As far as I have seen that backend was just giving out unique numbers 
and it looked questionable that we allocated pages for that.

E.g. when you set that flag then for each allocation we also allocate a 
TTM tt structure and a corresponding page.

Regards,
Christian.

Am 21.05.21 um 20:17 schrieb Zack Rusin:
> Hi, Christian.
>
> I was just going over some old patches and I was just looking at your 
> series introducing use_tt:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F80078%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C95b3c758592d4f0c247c08d91c84b48c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637572178561737650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5uZDVUi3W1WH29%2B2nE9uFUFZjQkKJyUxc6MwEce6SQ8%3D&reserved=0 
> and the change 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F382079%2F%3Fseries%3D80078%26rev%3D1&data=04%7C01%7Cchristian.koenig%40amd.com%7C95b3c758592d4f0c247c08d91c84b48c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637572178561737650%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=5XWSt07%2FzlXDa0GH8hnwEulyCBNocB2fUJ6CLwzubbE%3D&reserved=0
>
> Do you happen to remember what was the worry behind the /* TODO: This 
> is most likely not correct */ in vmwgfx_ttm_buffer.c? I'm trying to 
> figure out if we need to address it.
>
> z

Re: ttm_resource_manager::use_tt

[Zack Rusin]

On 5/22/21 3:13 AM, Christian König wrote:
> Hi Zack,
> 
> IIRC that was for the VMW_PL_GMR type, wasn't it?
> 
> As far as I have seen that backend was just giving out unique numbers and it looked questionable that we allocated pages for that.
> 
> E.g. when you set that flag then for each allocation we also allocate a TTM tt structure and a corresponding page.
>

Got ya. Yea, it's a little messy. I think it's correct. Those unique numbers are just identifiers for the bo's but the actual memory for them is regular system memory (e.g. we just tell our virtual hardware, here's some guest system pages and here's a unique id that we'll be using the refer to them).

Tangentially this also relates to a small issue with the rework of the memory accounting and removing the old page allocator. In the old page allocator we could specify what's the limit of system memory that the allocator could use (via ttm_check_under_lowerlimit) so the memory accounting that we've moved back to vmwgfx does nothing right now (well, it "accounts" just doesn't act on the limit ;) ).

We could probably add a call to ttm_check_under_lowerlimit in our ttm_populate callback (vmw_ttm_populate) but it is a little wacky. That's because in some situations we do want to ignore the limit on system memory allocations, purpose which we used to use ttm_operation_ctx::force_alloc for. I don't love designs that are so driver specific so I'd prefer to avoid using force_alloc that is only used internally by vmwgfx but I don't see a clean way of being able to put a limit on system memory that our driver is using.

Just to explain, our virtual hardware is basically an integrated gpu nowadays, so all the memory it allocates comes from system memory (with those unique numbers to identify it) and, especially on vm's that have lower amount of ram, we would like to limit how much of it will be used for graphics.

z

Re: ttm_resource_manager::use_tt

[Thomas Hellström (Intel)]

On 5/28/21 9:30 PM, Zack Rusin wrote:
> On 5/22/21 3:13 AM, Christian König wrote:
>> Hi Zack,
>>
>> IIRC that was for the VMW_PL_GMR type, wasn't it?
>>
>> As far as I have seen that backend was just giving out unique numbers 
>> and it looked questionable that we allocated pages for that.
>>
>> E.g. when you set that flag then for each allocation we also allocate 
>> a TTM tt structure and a corresponding page.
>>
>
> Got ya. Yea, it's a little messy. I think it's correct. Those unique 
> numbers are just identifiers for the bo's but the actual memory for 
> them is regular system memory (e.g. we just tell our virtual hardware, 
> here's some guest system pages and here's a unique id that we'll be 
> using the refer to them).
>
>
Yes, allocating pages for that memory type is correct. The main 
difference to other hardware here is that on other hardware those are 
bound to gpu using a range in a translation table. With vmwgfx they are 
bound using a slot with a unique integer..

/Thomas

Re: ttm_resource_manager::use_tt

[Christian König]

Am 29.05.21 um 17:23 schrieb Thomas Hellström (Intel):
>
> On 5/28/21 9:30 PM, Zack Rusin wrote:
>> On 5/22/21 3:13 AM, Christian König wrote:
>>> Hi Zack,
>>>
>>> IIRC that was for the VMW_PL_GMR type, wasn't it?
>>>
>>> As far as I have seen that backend was just giving out unique 
>>> numbers and it looked questionable that we allocated pages for that.
>>>
>>> E.g. when you set that flag then for each allocation we also 
>>> allocate a TTM tt structure and a corresponding page.
>>>
>>
>> Got ya. Yea, it's a little messy. I think it's correct. Those unique 
>> numbers are just identifiers for the bo's but the actual memory for 
>> them is regular system memory (e.g. we just tell our virtual 
>> hardware, here's some guest system pages and here's a unique id that 
>> we'll be using the refer to them).
>>
>>
> Yes, allocating pages for that memory type is correct. The main 
> difference to other hardware here is that on other hardware those are 
> bound to gpu using a range in a translation table. With vmwgfx they 
> are bound using a slot with a unique integer..

Ok that makes more sense then. In this case just drop the TODO.

Christian.

>
> /Thomas
>
>