Re: [PATCH v1 04/10] drm/panthor: Add architecture-specific function operations

[Steven Price <steven.price@arm.com>]

On 23/10/2025 21:59, Karunika Choo wrote:
> On 20/10/2025 10:10, Steven Price wrote:
>> On 14/10/2025 10:43, Karunika Choo wrote:
>>> Introduce architecture-specific function pointers to support
>>> architecture-dependent behaviours. This patch adds the following
>>> function pointers and updates their usage accordingly:
>>>
>>> - soft_reset
>>> - l2_power_on
>>> - l2_power_off
>>>
>>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_device.c |  4 ++--
>>>  drivers/gpu/drm/panthor/panthor_fw.c     |  5 +++--
>>>  drivers/gpu/drm/panthor/panthor_gpu.c    | 13 ++++++++++---
>>>  drivers/gpu/drm/panthor/panthor_gpu.h    |  1 +
>>>  drivers/gpu/drm/panthor/panthor_hw.c     |  9 ++++++++-
>>>  drivers/gpu/drm/panthor/panthor_hw.h     | 23 +++++++++++++++++++++++
>>>  6 files changed, 47 insertions(+), 8 deletions(-)
>>>
>> <snip>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
>>> index 7a191e76aeec..5a4e4aad9099 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_hw.h
>>> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
>>> @@ -20,12 +20,35 @@ enum panthor_hw_feature {
>>>  };
>>>  
>>>  
>>> +/**
>>> + * struct panthor_hw_ops - HW operations that are specific to a GPU
>>> + */
>>> +struct panthor_hw_ops {
>>> +	/** @soft_reset: Soft reset function pointer */
>>> +	int (*soft_reset)(struct panthor_device *ptdev);
>>> +#define panthor_hw_soft_reset(__ptdev) \
>>> +	((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev) : 0)
>>> +
>>> +	/** @l2_power_off: L2 power off function pointer */
>>> +	int (*l2_power_off)(struct panthor_device *ptdev);
>>> +#define panthor_hw_l2_power_off(__ptdev) \
>>> +	((__ptdev)->hw->ops.l2_power_off ? (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
>>> +
>>> +	/** @l2_power_on: L2 power on function pointer */
>>> +	int (*l2_power_on)(struct panthor_device *ptdev);
>>> +#define panthor_hw_l2_power_on(__ptdev) \
>>> +	((__ptdev)->hw->ops.l2_power_on ? (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
>>> +};
>>
>> Minor comments:
>>
>>  * You are defining these to have a return value, but you haven't
>> updated any of the call-sites to deal with a failure (the return value
>> is ignored). This is actually an existing problem, but AFAICT the new
>> _pwr_ versions have more error codes which are simply getting thrown away.
>>
> 
> Hi Steve,
> 
> While I agree that there is an existing problem, I'd argue that most of
> these are called from void functions where there really isn't much
> benefit in handling the return value apart from printing a "whoops"
> (which the called functions themselves mostly already do) and
> continuing. In fact, in the one place it isn't called from a void
> function, we do handle the return value.
> 
> Still, I do think that it is an issue, biggest of which probably is the
> soft reset work. Perhaps we can revisit this topic when we want to have
> another go at the soft reset handling in the future?

I agree it's not making things worse. But if we're adding another
backend it's worth stopping to think about the API. Does it make sense
to return an error code if the call sites cannot handle the error?
Should any of these function be void return? Specifically the power off
call as I'm not sure what the caller can usefully do if that fails.

I'm happy for the soft reset to be left for now, it would be good to
handle errors properly in that path but it's an orthogonal problem to
this series.

Thanks,
Steve

>>  * Is there a good reason why we need to support these functions being
>> NULL? It seems unlikely to be useful, and TBH I'd prefer to just assign
>> a dummy (empty) function in those cases.
>>
>>  * A static inline function would be neater and would avoid any
>> potential issues from the multiple evaluation of __ptdev.
>>
>> Thanks,
>> Steve
>>
> 
> Thanks for pointing this out + the suggestion, I will change this in v2.
> 
> Kind regards,
> Karunika
> 
>>> +
>>>  /**
>>>   * struct panthor_hw - GPU specific register mapping and functions
>>>   */
>>>  struct panthor_hw {
>>>  	/** @features: Bitmap containing panthor_hw_feature */
>>>  	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
>>> +
>>> +	/** @ops: Panthor HW specific operations */
>>> +	struct panthor_hw_ops ops;
>>>  };
>>>  
>>>  int panthor_hw_init(struct panthor_device *ptdev);
>>
>