lkml.org 
[lkml]   [2021]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v9 1/5] drm: Add a sharable drm page-pool implementation
    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?

    Thanks again for the feedback!

    thanks
    -john

    \
     
     \ /
      Last update: 2021-07-06 23:05    [W:2.414 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site