On Thu, 02 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > On 02/11/2023 15:42, Jani Nikula wrote: >> The implementation details of pmu should be implementation details >> hidden inside i915_pmu.c. Make it so. > > Don't tell me i915->pmu bothers xe somehow? It doesn't bother xe, it bothers me... > I am not a fan of the series > on a glance. Replacing an increment with a function call for instance. I think you glanced at the wart of this series. ;) It just bugs me that we expose a plethora of data that should be internal to pmu, basically just for that one increment. And we pull in a bunch of headers to define struct i915_pmu, and then we implicitly depend on those headers in a ton of places through incredible chains of includes. And we rebuild everything and a half when those headers change. Or when the pmu implementation details change. BR, Jani. > > Regards, > > Tvrtko > >> BR, >> Jani. >> >> >> Jani Nikula (5): >> drm/i915/pmu: report irqs to pmu code >> drm/i915/pmu: convert one more container_of() to event_to_pmu() >> drm/i915/pmu: change attr_group allocation and initialization >> drm/i915/pmu: hide struct i915_pmu inside i915_pmu.c >> drm/i915: add a number of explicit includes to avoid implicit ones >> >> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 1 + >> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 1 + >> .../gpu/drm/i915/gem/selftests/huge_pages.c | 1 + >> .../drm/i915/gem/selftests/i915_gem_context.c | 2 + >> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 1 + >> drivers/gpu/drm/i915/gt/selftest_execlists.c | 1 + >> drivers/gpu/drm/i915/gt/selftest_migrate.c | 1 + >> drivers/gpu/drm/i915/gt/selftest_slpc.c | 2 + >> drivers/gpu/drm/i915/i915_drv.h | 5 +- >> drivers/gpu/drm/i915/i915_irq.c | 6 +- >> drivers/gpu/drm/i915/i915_pmu.c | 216 ++++++++++++++++-- >> drivers/gpu/drm/i915/i915_pmu.h | 138 +---------- >> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 1 + >> drivers/gpu/drm/i915/selftests/i915_request.c | 4 +- >> drivers/gpu/drm/i915/selftests/igt_mmap.c | 2 + >> .../drm/i915/selftests/intel_memory_region.c | 1 + >> 16 files changed, 214 insertions(+), 169 deletions(-) >> -- Jani Nikula, Intel
