Re: [PATCH v2 6/8] drm/panthor: Support GLB_REQ.STATE field for Mali-G1 GPUs

[Karunika Choo]

On 26/10/2025 08:27, Boris Brezillon wrote:
> On Fri, 24 Oct 2025 21:21:15 +0100
> Karunika Choo <karunika.choo@xxxxxxx> wrote:
> 
>> +static bool panthor_fw_has_glb_state(struct panthor_device *ptdev)
>> +{
>> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
>> +
>> +	return glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0);
>> +}
> 
> It's okay for now, but if we start having more of these, we probably
> want to automate the generation of those has_<featurex>() helpers with
> something like:
> 
> #define FW_FEATURE(feat_name, major, minor)						\
> static bool panthor_fw_has_ ## feat_name(struct panthor_device *ptdev) 			\
> {											\
> 	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev); 	\
> 											\
> 	return glb_iface->control->version >= CSF_IFACE_VERSION(major, minor, 0);	\
> }
> 
> Same goes for the HW features BTW.
> 

I wonder if at that point, would a bitmask as previously proposed be a
cleaner solution? I have a minor bone to pick with MACROs that generate
functions as they make finding its definition unnecessarily complicated
and obtuse. Not to mention if the conditions for a feature changes and
is disjoint from all the others, updating the macro to handle these
would cause additional churn that I would really hope to avoid.

>> +
>>  /**
>>   * panthor_fw_conv_timeout() - Convert a timeout into a cycle-count
>>   * @ptdev: Device.
>> @@ -995,6 +1003,9 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
>>  					 GLB_IDLE_EN |
>>  					 GLB_IDLE;
>>
>> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
> 
> 	if (panthor_fw_has_glb_state(ptdev))
> 

Thanks for catching this. Will fix in v3.

Kind regards,
Karunika Choo

>> +		glb_iface->input->ack_irq_mask |= GLB_STATE_MASK;
>> +
>>  	panthor_fw_update_reqs(glb_iface, req, GLB_IDLE_EN, GLB_IDLE_EN);
>>  	panthor_fw_toggle_reqs(glb_iface, req, ack,
>>  			       GLB_CFG_ALLOC_EN |
>> @@ -1068,6 +1079,54 @@ static void panthor_fw_stop(struct panthor_device *ptdev)
>>  		drm_err(&ptdev->base, "Failed to stop MCU");
>>  }
>