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

[Andrey Grodzovsky <Andrey.Grodzovsky@amd.com>]

On 1/13/21 4:14 AM, Christian König wrote:
> 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@amd.com>
>>>>>>>> Sent: 08 January 2021 09:52
>>>>>>>> To: Grodzovsky, Andrey <Andrey.Grodzovsky@amd.com>; Daniel Vetter 
>>>>>>>> <daniel@ffwll.ch>
>>>>>>>> Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; 
>>>>>>>> dri-devel@lists.freedesktop.org <dri-devel@lists.freedesktop.org>; 
>>>>>>>> daniel.vetter@ffwll.ch <daniel.vetter@ffwll.ch>; robh@kernel.org 
>>>>>>>> <robh@kernel.org>; l.stach@pengutronix.de <l.stach@pengutronix.de>; 
>>>>>>>> yuq825@gmail.com <yuq825@gmail.com>; eric@anholt.net <eric@anholt.net>; 
>>>>>>>> Deucher, Alexander <Alexander.Deucher@amd.com>; 
>>>>>>>> gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>; 
>>>>>>>> ppaalanen@gmail.com <ppaalanen@gmail.com>; Wentland, Harry 
>>>>>>>> <Harry.Wentland@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.


I get it now, this is a good advise, and indeed makes the container struct i 
created obsolete but, currently I am going
with Daniel's suggestion to use drm_add_action_or_reset which makes the list 
itself also unneeded.

Andrey


>
>>
>> Andrey
>>
>>
>>>
>>>> -Daniel
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx