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

["Christian König" <christian.koenig@amd.com>]

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.

> -Daniel

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel