[Intel-gfx] [PATCH 04/10] drm/i915: move intel_context slab to direct module init/exit
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Jul 26 16:08:06 UTC 2021
On 26/07/2021 16:42, Jason Ekstrand wrote:
> On Mon, Jul 26, 2021 at 10:30 AM Jason Ekstrand <jason at jlekstrand.net> wrote:
>>
>> On Mon, Jul 26, 2021 at 3:35 AM Tvrtko Ursulin
>> <tvrtko.ursulin at 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 at jlekstrand.net>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at 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 at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the dri-devel
mailing list