On 02/11/2023 16:47, Jani Nikula wrote:
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.
On the other hand i915_pmu.h is a small header file, which does not
include _any_ other internal i915 headers (only uapi) and is always
present (if you ignore gens <= 2) which does not driver the allocate on
demand approach. In the past three years it had like seven edits.
Given all that, the change of direction you propose here sounds a bit
radical and I would rather not replace that increment with a function
call, when it is questionable if the same separation of components
approach can be, or will be, applied to the whole driver. Gut feeling
says it is bound to be hard and possibly not happen in which case I
don't see what is gained by churning on the tiny and quiet i915_pmu.h|c.
Regards,
Tvrtko
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(-)
