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

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

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

It's really not about allocation on demand at all. It's about hiding the
implementation, using the mechanisms that C provides us for
abstractions, and reducing the header dependencies.

> 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.

Yeah, the approach probably won't be applied to the whole driver anytime
soon. Maybe never. What matters to me though is that all of these set
the example. People look at drm_i915_private, and always just add a new
struct member, and never even stop to think if they could make it an
opaque pointer instead.

Heck, the same damn approach was copy-pasted to the xe driver, warts and
all.

Anyway.

All that said, I think I'm going to drop this, along with all
refactoring that isn't strictly related to display.

BR,
Jani.

-- 
Jani Nikula, Intel