[Intel-gfx] [PATCH 0/5] drm/i915/pmu: hide struct i915_pmu

[Tvrtko Ursulin]

On 02/11/2023 16:47, Jani Nikula wrote:
> On Thu, 02 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com> 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(-)
>>>
>