Re: [Intel-gfx] [PATCH 04/10] drm/i915: move intel_context slab to direct module init/exit

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 26/07/2021 16:42, Jason Ekstrand wrote:
> On Mon, Jul 26, 2021 at 10:30 AM Jason Ekstrand <jason@jlekstrand.net> wrote:
>>
>> On Mon, Jul 26, 2021 at 3:35 AM Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>
>>>
>>> On 23/07/2021 20:29, Daniel Vetter wrote:
>>>> With the global kmem_cache shrink infrastructure gone there's nothing
>>>> special and we can convert them over.
>>>>
>>>> I'm doing this split up into each patch because there's quite a bit of
>>>> noise with removing the static global.slab_ce to just a
>>>> slab_ce.
>>>>
>>>> Cc: Jason Ekstrand <jason@jlekstrand.net>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gt/intel_context.c | 25 ++++++++-----------------
>>>>    drivers/gpu/drm/i915/gt/intel_context.h |  3 +++
>>>>    drivers/gpu/drm/i915/i915_globals.c     |  2 --
>>>>    drivers/gpu/drm/i915/i915_globals.h     |  1 -
>>>>    drivers/gpu/drm/i915/i915_pci.c         |  2 ++
>>>>    5 files changed, 13 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>>>> index baa05fddd690..283382549a6f 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>>>> @@ -7,7 +7,6 @@
>>>>    #include "gem/i915_gem_pm.h"
>>>>
>>>>    #include "i915_drv.h"
>>>> -#include "i915_globals.h"
>>>>    #include "i915_trace.h"
>>>>
>>>>    #include "intel_context.h"
>>>> @@ -15,14 +14,11 @@
>>>>    #include "intel_engine_pm.h"
>>>>    #include "intel_ring.h"
>>>>
>>>> -static struct i915_global_context {
>>>> -     struct i915_global base;
>>>> -     struct kmem_cache *slab_ce;
>>>> -} global;
>>>> +struct kmem_cache *slab_ce;
>>
>> Static?  With that,
>>
>> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
>>
>>>>
>>>>    static struct intel_context *intel_context_alloc(void)
>>>>    {
>>>> -     return kmem_cache_zalloc(global.slab_ce, GFP_KERNEL);
>>>> +     return kmem_cache_zalloc(slab_ce, GFP_KERNEL);
>>>>    }
>>>>
>>>>    static void rcu_context_free(struct rcu_head *rcu)
>>>> @@ -30,7 +26,7 @@ static void rcu_context_free(struct rcu_head *rcu)
>>>>        struct intel_context *ce = container_of(rcu, typeof(*ce), rcu);
>>>>
>>>>        trace_intel_context_free(ce);
>>>> -     kmem_cache_free(global.slab_ce, ce);
>>>> +     kmem_cache_free(slab_ce, ce);
>>>>    }
>>>>
>>>>    void intel_context_free(struct intel_context *ce)
>>>> @@ -410,22 +406,17 @@ void intel_context_fini(struct intel_context *ce)
>>>>        i915_active_fini(&ce->active);
>>>>    }
>>>>
>>>> -static void i915_global_context_exit(void)
>>>> +void i915_context_module_exit(void)
>>>>    {
>>>> -     kmem_cache_destroy(global.slab_ce);
>>>> +     kmem_cache_destroy(slab_ce);
>>>>    }
>>>>
>>>> -static struct i915_global_context global = { {
>>>> -     .exit = i915_global_context_exit,
>>>> -} };
>>>> -
>>>> -int __init i915_global_context_init(void)
>>>> +int __init i915_context_module_init(void)
>>>>    {
>>>> -     global.slab_ce = KMEM_CACHE(intel_context, SLAB_HWCACHE_ALIGN);
>>>> -     if (!global.slab_ce)
>>>> +     slab_ce = KMEM_CACHE(intel_context, SLAB_HWCACHE_ALIGN);
>>>> +     if (!slab_ce)
>>>>                return -ENOMEM;
>>>>
>>>> -     i915_global_register(&global.base);
>>>>        return 0;
>>>>    }
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.h b/drivers/gpu/drm/i915/gt/intel_context.h
>>>> index 974ef85320c2..a0ca82e3c40d 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_context.h
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
>>>> @@ -30,6 +30,9 @@ void intel_context_init(struct intel_context *ce,
>>>>                        struct intel_engine_cs *engine);
>>>>    void intel_context_fini(struct intel_context *ce);
>>>>
>>>> +void i915_context_module_exit(void);
>>>> +int i915_context_module_init(void);
>>>> +
>>>>    struct intel_context *
>>>>    intel_context_create(struct intel_engine_cs *engine);
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_globals.c b/drivers/gpu/drm/i915/i915_globals.c
>>>> index 3de7cf22ec76..d36eb7dc40aa 100644
>>>> --- a/drivers/gpu/drm/i915/i915_globals.c
>>>> +++ b/drivers/gpu/drm/i915/i915_globals.c
>>>> @@ -7,7 +7,6 @@
>>>>    #include <linux/slab.h>
>>>>    #include <linux/workqueue.h>
>>>>
>>>> -#include "gem/i915_gem_context.h"
>>>>    #include "gem/i915_gem_object.h"
>>>>    #include "i915_globals.h"
>>>>    #include "i915_request.h"
>>>> @@ -32,7 +31,6 @@ static void __i915_globals_cleanup(void)
>>>>    }
>>>>
>>>>    static __initconst int (* const initfn[])(void) = {
>>>> -     i915_global_context_init,
>>>>        i915_global_gem_context_init,
>>>>        i915_global_objects_init,
>>>>        i915_global_request_init,
>>>> diff --git a/drivers/gpu/drm/i915/i915_globals.h b/drivers/gpu/drm/i915/i915_globals.h
>>>> index d80901ba75e3..60daa738a188 100644
>>>> --- a/drivers/gpu/drm/i915/i915_globals.h
>>>> +++ b/drivers/gpu/drm/i915/i915_globals.h
>>>> @@ -23,7 +23,6 @@ int i915_globals_init(void);
>>>>    void i915_globals_exit(void);
>>>>
>>>>    /* constructors */
>>>> -int i915_global_context_init(void);
>>>>    int i915_global_gem_context_init(void);
>>>>    int i915_global_objects_init(void);
>>>>    int i915_global_request_init(void);
>>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>>>> index f9527269e30a..266618157775 100644
>>>> --- a/drivers/gpu/drm/i915/i915_pci.c
>>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>>>> @@ -33,6 +33,7 @@
>>>>    #include "i915_active.h"
>>>>    #include "i915_buddy.h"
>>>>    #include "i915_drv.h"
>>>> +#include "gem/i915_gem_context.h"
>>>
>>> It's a bit ugly to go to a design where i915_pci.c has to include so
>>> many random parts of i915. IMO for a complex driver like i915,
>>> compartmentalizing so much knowledge about the internals was better
>>> inside the globals layer.
>>
>> I agree that i915_pci feels like the wrong place to put this but I
>> don't think that's so much because globals don't belong in i915_pci
>> but because i915_init/exit don't belong there.  Maybe, once this is
>> all said and done (or at the start of the series), we should move
>> i915_init/exit to i915_drv.c?  Of course, there's a bunch of PCI
>> probing stuff in i915_drv.c so..... yeah.... our organization is
>> pretty busted.
> 
> To put a finer point on this, the new "design" is really to have a
> single flat list instead of two, one nested inside the other.  There's
> nothing wrong with that at all.  The fact that all this stuff now
> lives in i915_pci.c is ugly.  But, as I said, that's kind-of an
> accident of history because that's where i915_init() and i915_exit()
> currently live.  We should just move the lot to i915_drv.c.

Hmm.. on one hand it does sounds better to move to i915_drv.c, but is it 
just because all these new include directive are so visibly out of place 
in i915_pci.c?

Perhaps we need i915_module.c and then i915_globals is a completely fine 
concept. Desired IMO even since we have to avoid globals in general 
(multi-gpu) so it sticks out nicely that all that is allowed to be 
global has a special place.

And i915_drv.c can remain being about a driver instance as bound to one GPU.

That feels like the best of both worlds to me.

Regards,

Tvrtko

> 
>> --Jason
>>
>>> Maybe add a cover letter to explain the perceived pros and cons and
>>> thinking in general?
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>    #include "i915_perf.h"
>>>>    #include "i915_globals.h"
>>>>    #include "i915_selftest.h"
>>>> @@ -1297,6 +1298,7 @@ static const struct {
>>>>        { i915_check_nomodeset, NULL },
>>>>        { i915_active_module_init, i915_active_module_exit },
>>>>        { i915_buddy_module_init, i915_buddy_module_exit },
>>>> +     { i915_context_module_init, i915_context_module_exit },
>>>>        { i915_globals_init, i915_globals_exit },
>>>>        { i915_mock_selftests, NULL },
>>>>        { i915_pmu_init, i915_pmu_exit },
>>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx