linux-kernel - Re: [PATCH v2 6/8] drm/panthor: Support GLB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6c301d19-7f80-471d-a431-8b936d7eb362@arm.com>
Date: Mon, 27 Oct 2025 10:43:42 +0000
From: Karunika Choo <karunika.choo@....com>
To: Boris Brezillon <boris.brezillon@...labora.com>
Cc: dri-devel@...ts.freedesktop.org, nd@....com,
 Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/8] drm/panthor: Support GLB_REQ.STATE field for
 Mali-G1 GPUs

On 26/10/2025 08:27, Boris Brezillon wrote:
> On Fri, 24 Oct 2025 21:21:15 +0100
> Karunika Choo <karunika.choo@....com> 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");
>>  }
>