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

[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 ?

Andrey


>
>> -Daniel
>