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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
>   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 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.

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 },