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

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

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?

Regards,

Tvrtko

> 
>>
>>> +{
>>> +	return container_of(event->pmu, struct i915_pmu, base);
>>> +}
>>> +
>>>   static struct drm_i915_private *pmu_to_i915(struct i915_pmu *pmu)
>>>   {
>>>   	return container_of(pmu, struct drm_i915_private, pmu);
>>> @@ -510,8 +515,8 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>>>   
>>>   static void i915_pmu_event_destroy(struct perf_event *event)
>>>   {
>>> -	struct drm_i915_private *i915 =
>>> -		container_of(event->pmu, typeof(*i915), pmu.base);
>>> +	struct i915_pmu *pmu = event_to_pmu(event);
>>> +	struct drm_i915_private *i915 = pmu_to_i915(pmu);
>>
>> perf_event_to_i915() ?
> 
> Nah. Most places that need i915 also need pmu. Feels a bit much to add a
> helper to combine two helpers.
> 
> Thanks for the review.
> 
> BR,
> Jani.
> 
>>
>> Andi
>