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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 02 Nov 2023, Tvrtko Ursulin <tvrtko.ursulin@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.


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