Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation

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

Am 06.07.21 um 23:19 schrieb John Stultz:
> On Tue, Jul 6, 2021 at 2:15 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Jul 6, 2021 at 11:04 PM John Stultz <john.stultz@linaro.org> wrote:
>>> On Wed, Jun 30, 2021 at 11:52 PM Christian König
>>> <christian.koenig@amd.com> wrote:
>>>> Am 01.07.21 um 00:24 schrieb John Stultz:
>>>>> On Wed, Jun 30, 2021 at 2:10 AM Christian König
>>>>> <christian.koenig@amd.com> wrote:
>>>>>> Am 30.06.21 um 03:34 schrieb John Stultz:
>>>>>>> +static unsigned long page_pool_size; /* max size of the pool */
>>>>>>> +
>>>>>>> +MODULE_PARM_DESC(page_pool_size, "Number of pages in the drm page pool");
>>>>>>> +module_param(page_pool_size, ulong, 0644);
>>>>>>> +
>>>>>>> +static atomic_long_t nr_managed_pages;
>>>>>>> +
>>>>>>> +static struct mutex shrinker_lock;
>>>>>>> +static struct list_head shrinker_list;
>>>>>>> +static struct shrinker mm_shrinker;
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * drm_page_pool_set_max - Sets maximum size of all pools
>>>>>>> + *
>>>>>>> + * Sets the maximum number of pages allows in all pools.
>>>>>>> + * This can only be set once, and the first caller wins.
>>>>>>> + */
>>>>>>> +void drm_page_pool_set_max(unsigned long max)
>>>>>>> +{
>>>>>>> +     if (!page_pool_size)
>>>>>>> +             page_pool_size = max;
>>>>>>> +}
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * drm_page_pool_get_max - Maximum size of all pools
>>>>>>> + *
>>>>>>> + * Return the maximum number of pages allows in all pools
>>>>>>> + */
>>>>>>> +unsigned long drm_page_pool_get_max(void)
>>>>>>> +{
>>>>>>> +     return page_pool_size;
>>>>>>> +}
>>>>>> Well in general I don't think it is a good idea to have getters/setters
>>>>>> for one line functionality, similar applies to locking/unlocking the
>>>>>> mutex below.
>>>>>>
>>>>>> Then in this specific case what those functions do is to aid
>>>>>> initializing the general pool manager and that in turn should absolutely
>>>>>> not be exposed.
>>>>>>
>>>>>> The TTM pool manager exposes this as function because initializing the
>>>>>> pool manager is done in one part of the module and calculating the
>>>>>> default value for the pages in another one. But that is not something I
>>>>>> would like to see here.
>>>>> So, I guess I'm not quite clear on what you'd like to see...
>>>>>
>>>>> Part of what I'm balancing here is the TTM subsystem normally sets a
>>>>> global max size, whereas the old ION pool didn't have caps (instead
>>>>> just relying on the shrinker when needed).
>>>>> So I'm trying to come up with a solution that can serve both uses. So
>>>>> I've got this drm_page_pool_set_max() function to optionally set the
>>>>> maximum value, which is called in the TTM initialization path or set
>>>>> the boot argument. But for systems that use the dmabuf system heap,
>>>>> but don't use TTM, no global limit is enforced.
>>>> Yeah, exactly that's what I'm trying to prevent.
>>>>
>>>> See if we have the same functionality used by different use cases we
>>>> should not have different behavior depending on what drivers are loaded.
>>>>
>>>> Is it a problem if we restrict the ION pool to 50% of system memory as
>>>> well? If yes than I would rather drop the limit from TTM and only rely
>>>> on the shrinker there as well.
>>> Would having the default value as a config option (still overridable
>>> via boot argument) be an acceptable solution?
>> We're also trying to get ttm over to the shrinker model, and a first
>> cut of that even landed, but didn't really work out yet. So maybe just
>> aiming for the shrinker? I do agree this should be consistent across
>> the board, otherwise we're just sharing code but not actually sharing
>> functionality, which is a recipe for disaster because one side will
>> end up breaking the other side's use-case.
> Fair enough, maybe it would be best to remove the default limit, but
> leave the logic so it can still be set via the boot argument?

Yeah, that would work for me and the shrinker implementation should 
already be good enough.

Regards,
Christian.

>
> thanks
> -john