[PATCH v3 01/12] drm: Add dummy page per device or GEM object

[Christian König]

Am 12.01.21 um 16:59 schrieb Andrey Grodzovsky:
>
> On 1/12/21 7:32 AM, Christian König wrote:
>> Am 12.01.21 um 10:10 schrieb Daniel Vetter:
>>> On Mon, Jan 11, 2021 at 03:45:10PM -0500, Andrey Grodzovsky wrote:
>>>> On 1/11/21 11:15 AM, Daniel Vetter wrote:
>>>>> On Mon, Jan 11, 2021 at 05:13:56PM +0100, Daniel Vetter wrote:
>>>>>> On Fri, Jan 08, 2021 at 04:49:55PM +0000, Grodzovsky, Andrey wrote:
>>>>>>> Ok then, I guess I will proceed with the dummy pages list 
>>>>>>> implementation then.
>>>>>>>
>>>>>>> Andrey
>>>>>>>
>>>>>>> ________________________________
>>>>>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>>>>> Sent: 08 January 2021 09:52
>>>>>>> To: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com>; Daniel 
>>>>>>> Vetter <daniel at ffwll.ch>
>>>>>>> Cc: amd-gfx at lists.freedesktop.org 
>>>>>>> <amd-gfx at lists.freedesktop.org>; dri-devel at lists.freedesktop.org 
>>>>>>> <dri-devel at lists.freedesktop.org>; daniel.vetter at ffwll.ch 
>>>>>>> <daniel.vetter at ffwll.ch>; robh at kernel.org <robh at kernel.org>; 
>>>>>>> l.stach at pengutronix.de <l.stach at pengutronix.de>; 
>>>>>>> yuq825 at gmail.com <yuq825 at gmail.com>; eric at anholt.net 
>>>>>>> <eric at anholt.net>; Deucher, Alexander 
>>>>>>> <Alexander.Deucher at amd.com>; gregkh at linuxfoundation.org 
>>>>>>> <gregkh at linuxfoundation.org>; ppaalanen at gmail.com 
>>>>>>> <ppaalanen at gmail.com>; Wentland, Harry <Harry.Wentland at amd.com>
>>>>>>> Subject: Re: [PATCH v3 01/12] drm: Add dummy page per device or 
>>>>>>> GEM object
>>>>>>>
>>>>>>> Mhm, I'm not aware of any let over pointer between TTM and GEM 
>>>>>>> and we
>>>>>>> worked quite hard on reducing the size of the amdgpu_bo, so another
>>>>>>> extra pointer just for that corner case would suck quite a bit.
>>>>>> We have a ton of other pointers in struct amdgpu_bo (or any of 
>>>>>> it's lower
>>>>>> things) which are fairly single-use, so I'm really not much 
>>>>>> seeing the
>>>>>> point in making this a special case. It also means the lifetime 
>>>>>> management
>>>>>> becomes a bit iffy, since we can't throw away the dummy page then 
>>>>>> the last
>>>>>> reference to the bo is released (since we don't track it there), 
>>>>>> but only
>>>>>> when the last pointer to the device is released. Potentially this 
>>>>>> means a
>>>>>> pile of dangling pages hanging around for too long.
>>>>> Also if you really, really, really want to have this list, please 
>>>>> don't
>>>>> reinvent it since we have it already. drmm_ is exactly meant for 
>>>>> resources
>>>>> that should be freed when the final drm_device reference disappears.
>>>>> -Daniel
>>>>
>>>> I maybe was eager to early, see i need to explicitly allocate the 
>>>> dummy page
>>>> using page_alloc so
>>>> i cannot use drmm_kmalloc for this, so once again like with the 
>>>> list i need
>>>> to wrap it with a container struct
>>>> which i can then allocate using drmm_kmalloc and inside there will 
>>>> be page
>>>> pointer. But then
>>>> on release it needs to free the page and so i supposedly need to 
>>>> use drmm_add_action
>>>> to free the page before the container struct is released but 
>>>> drmm_kmalloc
>>>> doesn't allow to set
>>>> release action on struct allocation. So I created a new
>>>> drmm_kmalloc_with_action API function
>>>> but then you also need to supply the optional data pointer for the 
>>>> release
>>>> action (the struct page in this case)
>>>> and so this all becomes a bit overcomplicated (but doable). Is this 
>>>> extra
>>>> API worth adding ? Maybe it can
>>>> be useful in general.
>>> drm_add_action_or_reset (for better control flow) has both a void * 
>>> data
>>> and a cleanup function (and it internally allocates the tracking 
>>> structure
>>> for that for you). So should work as-is? Allocating a tracking 
>>> structure
>>> for our tracking structure for a page would definitely be a bit too 
>>> much.
>>>
>>> Essentiall drmm_add_action is your kcalloc_with_action function you 
>>> want,
>>> as long as all you need is a single void * pointer (we could do the
>>> kzalloc_with_action though, there's enough space, just no need yet 
>>> for any
>>> of the current users).
>>
>> Yeah, but my thinking was that we should use the page LRU for this 
>> and not another container structure.
>>
>> Christian.
>
>
> Which specific list did you mean ?

The struct page * you get from get_free_page() already has an lru member 
of type list_head.

This way you can link pages together for later destruction without the 
need of a container object.

Christian.

>
> Andrey
>
>
>>
>>> -Daniel
>>