Re: [Intel-gfx] [PATCH 2/3] drm/i915/pmu: add event_to_pmu() helper

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 26/10/2023 11:36, Andi Shyti wrote:
> Hi,
> 
>> On 26/10/2023 11:22, Jani Nikula wrote:
>>> On Wed, 25 Oct 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote:
>>>> On Wed, Oct 25, 2023 at 11:20:25AM +0100, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 24/10/2023 13:42, Jani Nikula wrote:
>>>>>> On Tue, 24 Oct 2023, Andi Shyti <andi.shyti@linux.intel.com> wrote:
>>>>>>> Hi Jani,
>>>>>>>
>>>>>>> On Mon, Oct 23, 2023 at 06:02:55PM +0300, Jani Nikula wrote:
>>>>>>>> It's tedious to duplicate the container_of() everywhere. Add a helper.
>>>>>>>>
>>>>>>>> Also do the logical steps of first getting from struct perf_event to
>>>>>>>> struct i915_pmu, and then from struct i915_pmu to struct
>>>>>>>> drm_i915_private if needed, instead of perf_event->i915->pmu. Not all
>>>>>>>> places even need the i915 pointer.
>>>>>>>>
>>>>>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/i915/i915_pmu.c | 45 +++++++++++++++------------------
>>>>>>>>     1 file changed, 20 insertions(+), 25 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>>>>>>> index dcae2fcd8d36..d45b40ec6d47 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>>>>>>> @@ -31,6 +31,11 @@
>>>>>>>>     static cpumask_t i915_pmu_cpumask;
>>>>>>>>     static unsigned int i915_pmu_target_cpu = -1;
>>>>>>>> +static struct i915_pmu *event_to_pmu(struct perf_event *event)
>>>>>>>
>>>>>>> I would call it perfevent (or perf_event), event is too generic.
>>>>>>> We have other kind of events, too.
>>>>>>
>>>>>> Fair enough.
>>>>>
>>>>> Counter argument is that i915_pmu.c consistently names this event (which is
>>>>> likely lifted from other PMU drivers) so is the proposal to churn it all, or
>>>>> create an inconsistency?
>>>>
>>>> The first that comes to my mind is that the debugger is also
>>>> using the term "event"... on the other hand there is no debugger
>>>> in i915.
>>>
>>> Have you settled on this? I don't care either way, could apply either
>>> patch.
> 
> no... unfortunately not...

:(

$ grep "struct perf_event \*event" . -r | wc -l
1912
$ grep "struct perf_event \*perf_event" . -r | wc -l
5

;)

Now seriously, I don't mind perf_event, as long as _whole_ i915_pmu.c is 
switched over. At which point I questioned would the churn be worth it.

Regards,

Tvrtko

>> To me it is clear that preference should be to remain consistent within the
>> file, that is, leave it as you originally had.
> 
> ... so I'm not going to be strong on this... please feel free to
> ignore my comment, then.
> 
> Thanks!
> Andi