| Message ID | 20210723192934.1004427-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/10] drm/i915: Check for nomodeset in i915_init() first | expand |
Hi Daniel,
I love your patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
[cannot apply to drm-intel/for-linux-next drm-exynos/exynos-drm-next tegra-drm/drm/tegra/for-next drm/drm-next v5.14-rc2 next-20210723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-i915-Check-for-nomodeset-in-i915_init-first/20210724-033145
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
config: x86_64-rhel-8.3-kselftests (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/acd1c2f5060c5b06036bf6d0950141ee55906a24
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Daniel-Vetter/drm-i915-Check-for-nomodeset-in-i915_init-first/20210724-033145
git checkout acd1c2f5060c5b06036bf6d0950141ee55906a24
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/i915/gt/intel_context.c:17:19: sparse: sparse: symbol 'slab_ce' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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 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 }, >
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. --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
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. > --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
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
On Mon, Jul 26, 2021 at 11:08 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > 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. Is i915_drv.c about a single instance bound to a single GPU? If so, then, yeah, maybe not the right place. Maybe a i915_module.c would be better. It's all different shades of shed paint. --Jason > 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
On 26/07/2021 17:20, Jason Ekstrand wrote: > On Mon, Jul 26, 2021 at 11:08 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> 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. > > Is i915_drv.c about a single instance bound to a single GPU? If so, Yep, all functions there either take drm_dev, pdev or dev_priv as argument, or return/initialize dev_priv. > then, yeah, maybe not the right place. Maybe a i915_module.c would be > better. It's all different shades of shed paint. Hm not really just different shades IMO. Because I argue the patch series as is is a retrograde step in the above discussed respect. I think i915_globals is cleaner code organisation. Because even if we add i915_module.c, then that can be made initialize globals and register with pci in cleanly separated steps without the need to include many driver internals. I see r-b are accumulating but I hope reasonable objections will be considered. Regards, Tvrtko > > --Jason > >> 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
On Mon, Jul 26, 2021 at 11:31 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 26/07/2021 17:20, Jason Ekstrand wrote: > > On Mon, Jul 26, 2021 at 11:08 AM Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > >> 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. > > > > Is i915_drv.c about a single instance bound to a single GPU? If so, > > Yep, all functions there either take drm_dev, pdev or dev_priv as > argument, or return/initialize dev_priv. > > > then, yeah, maybe not the right place. Maybe a i915_module.c would be > > better. It's all different shades of shed paint. > > Hm not really just different shades IMO. Because I argue the patch > series as is is a retrograde step in the above discussed respect. > > I think i915_globals is cleaner code organisation. Because even if we > add i915_module.c, then that can be made initialize globals and register > with pci in cleanly separated steps without the need to include many > driver internals. Ok, so maybe I'm missing something in what you're saying. I was under the impression that your primary concern was separating PCI setup from per-device from per-module stuff. If so, moving it all to an i915_module.c fixes that. Are you arguing that a flat list of module init steps is bad? And that having globals be its own sub-list is good? If so, then I have to disagree. I don't think splitting the calling of misc "set up my slabs" functions from the final "register with PCI" at the end gains us anything. What it does do is give more files to search through, more layers to think about when understanding the code, and more infrastructure to maintain. If, on the other hand, we had a globals infrastructure that actually gained us something in terms of code simplicity, I might be able to get behind that. For instance, I could imagine something like this: I915_DECL_SLAB(foo); /* Someone allocates something */ kmem_cache_alloc(foo.slab); where we somehow automagically declare the slab, initialize it on module load, and tear it down on module exit. That would add real value. As is, all i915_globals.c adds is extra layers. --Jason > I see r-b are accumulating but I hope reasonable objections will be > considered. > > Regards, > > Tvrtko > > > > > --Jason > > > >> 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
On 26/07/2021 19:17, Jason Ekstrand wrote: > On Mon, Jul 26, 2021 at 11:31 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> On 26/07/2021 17:20, Jason Ekstrand wrote: >>> On Mon, Jul 26, 2021 at 11:08 AM Tvrtko Ursulin >>> <tvrtko.ursulin@linux.intel.com> wrote: >>>> 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. >>> >>> Is i915_drv.c about a single instance bound to a single GPU? If so, >> >> Yep, all functions there either take drm_dev, pdev or dev_priv as >> argument, or return/initialize dev_priv. >> >>> then, yeah, maybe not the right place. Maybe a i915_module.c would be >>> better. It's all different shades of shed paint. >> >> Hm not really just different shades IMO. Because I argue the patch >> series as is is a retrograde step in the above discussed respect. >> >> I think i915_globals is cleaner code organisation. Because even if we >> add i915_module.c, then that can be made initialize globals and register >> with pci in cleanly separated steps without the need to include many >> driver internals. > > Ok, so maybe I'm missing something in what you're saying. I was under > the impression that your primary concern was separating PCI setup from > per-device from per-module stuff. If so, moving it all to an > i915_module.c fixes that. Yeah, but i915_module.c was just a side discussion which started from whether it is right or not to put module init into i915_drv.c. It does not fix the fundamental question of the below discussion. > Are you arguing that a flat list of module init steps is bad? And > that having globals be its own sub-list is good? If so, then I have > to disagree. I don't think splitting the calling of misc "set up my I think having to include all the driver sub-sub component headers from any top level module init file is a step towards a worse design. Currently those steps were nicely encapsulated and I don't see a reason to break them out to top level. > slabs" functions from the final "register with PCI" at the end gains > us anything. What it does do is give more files to search through, > more layers to think about when understanding the code, and more > infrastructure to maintain. > > If, on the other hand, we had a globals infrastructure that actually > gained us something in terms of code simplicity, I might be able to > get behind that. For instance, I could imagine something like this: > > I915_DECL_SLAB(foo); > > /* Someone allocates something */ > kmem_cache_alloc(foo.slab); > > where we somehow automagically declare the slab, initialize it on > module load, and tear it down on module exit. That would add real > value. As is, all i915_globals.c adds is extra layers. I read this as a statement that at least one part of your thinking is that current globals code is too verbose for simple global slab usage? This I can buy, given how all but two usages are creating a single slab. Then there are two callers which use two slabs, but nothing outside the slab territory. So argument to simplify can be made yeah, only question is how. Simple macro like you suggest sounds very attractive indeed if it could replace boiler plate code for single slab users (albeit with some more arguments). Problem is I don't know that we could make it call a registration helper from global scope. Hm removing the list and going for fixed table slots might work, if logistics of reserving indices for it in i915_globals.h wouldn't be too ugly? Hm maybe not too ugly, maybe it is just an enum every global slab users adds for themselves and references it in a macro. With macro magic like DEV_INFO_FOR_EACH_FLAG and company to generate the table etc? Regards, Tvrtko > --Jason > > >> I see r-b are accumulating but I hope reasonable objections will be >> considered. >> >> Regards, >> >> Tvrtko >> >>> >>> --Jason >>> >>>> 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
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" #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 },
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(-)