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@xxxxxxx>
Sent: 08 January 2021 09:52
To: Grodzovsky, Andrey <Andrey.Grodzovsky@xxxxxxx>; Daniel Vetter
<daniel@xxxxxxxx>
Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>;
dri-devel@xxxxxxxxxxxxxxxxxxxxx <dri-devel@xxxxxxxxxxxxxxxxxxxxx>;
daniel.vetter@xxxxxxxx <daniel.vetter@xxxxxxxx>; robh@xxxxxxxxxx
<robh@xxxxxxxxxx>; l.stach@xxxxxxxxxxxxxx <l.stach@xxxxxxxxxxxxxx>;
yuq825@xxxxxxxxx <yuq825@xxxxxxxxx>; eric@xxxxxxxxxx <eric@xxxxxxxxxx>;
Deucher, Alexander <Alexander.Deucher@xxxxxxx>;
gregkh@xxxxxxxxxxxxxxxxxxx <gregkh@xxxxxxxxxxxxxxxxxxx>;
ppaalanen@xxxxxxxxx <ppaalanen@xxxxxxxxx>; Wentland, Harry
<Harry.Wentland@xxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
