Messages in this thread | | | Subject | Re: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation | From | Christian König <> | Date | Wed, 7 Jul 2021 08:52:12 +0200 |
| |
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
| |