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

[Christian König]

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.

> -Daniel