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 ?
Andrey
-Daniel