Message-ID: <2f6c190d-aed0-4a27-8b20-1a4833d7edf7@amd.com>
Date: Thu, 25 Sep 2025 11:56:13 +0800
From: "Du, Bin" <bin.du@....com>
To: Sultan Alsawaf <sultan@...neltoast.com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
 laurent.pinchart+renesas@...asonboard.com, bryan.odonoghue@...aro.org,
 sakari.ailus@...ux.intel.com, prabhakar.mahadev-lad.rj@...renesas.com,
 linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
 pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
 gjorgji.rosikopulos@....com, Phil.Jawich@....com, Dominic.Antony@....com,
 mario.limonciello@....com, richard.gong@....com, anson.tsao@....com,
 Alexey Zagorodnikov <xglooom@...il.com>
Subject: Re: [PATCH v4 3/7] media: platform: amd: Add isp4 fw and hw interface

Thanks Sultan.

On 9/24/2025 3:09 PM, Sultan Alsawaf wrote:
> On Tue, Sep 23, 2025 at 05:24:11PM +0800, Du, Bin wrote:
>> On 9/22/2025 5:55 AM, Sultan Alsawaf wrote:
>>> On Thu, Sep 11, 2025 at 06:08:43PM +0800, Bin Du wrote:
>>>> +	if (!mem_info)
>>>> +		return NULL;
>>>> +
>>>> +	mem_info->mem_size = mem_size;
>>>
>>> mem_info->mem_size is not used anywhere, remove it.
>>>
>>
>> Actually, mem_size will be used in following patches in isp4_subdev.c, so,
>> i'd like to keep it.
> 
> Ah, I didn't notice, my apologies.
> 

It's okay. Actually, it is not used in this patch, so I don't think you 
are wrong :)

>>>> +	for (i = 0; i < buf_size / sizeof(u32); i++)
>>>> +		checksum += buffer[i];
>>>> +
>>>> +	surplus_ptr = (u8 *)&buffer[i];
>>>
>>> Change cast to `(const u32 *)`
>>>
>>
>> Sure, will modify in the next version. I guess you mean `(const u8 *)`
> 
> Yes, it should be `(const u8 *)`, apologies for the typo.
> 

Never mind, just coding details, what's most important is the idea, 
thanks for that.

>>>> +
>>>> +	guard(mutex)(&ispif->cmdq_mutex);
>>>> +
>>>> +	list_for_each_entry_safe(buf_node, tmp_node, &ispif->cmdq, list) {
>>>> +		list_del(&buf_node->list);
>>>> +		kfree(buf_node);
>>>> +	}
>>>
>>> Move the whole list to a local LIST_HEAD(free_list) variable and then release
>>> the lock. Then you can list_for_each_entry_safe() without needing to do a
>>> list_del() every time, and you won't need to hold the lock the whole time.
>>>
>>
>> Thanks for the suggestion, seems that will make code complicated, e.g. this
>> is the suggested implementation in my mind if i don't get you wrong,
>>
>> void isp4if_clear_cmdq(struct isp4_interface *ispif)
>> {
>> 	struct isp4if_cmd_element *buf_node;
>> 	struct isp4if_cmd_element *tmp_node;
>> 	LIST_HEAD(free_list);
>>
>> 	{
>> 		guard(mutex)(&ispif->cmdq_mutex);
>> 		if (list_empty(&ispif->cmdq))
>> 			return;
>> 		free_list = ispif->cmdq;
>> 		INIT_LIST_HEAD(&ispif->cmdq);
>> 	}
>>
>> 	list_for_each_entry_safe(buf_node, tmp_node, &free_list, list) {
>> 		bool quit = false;
>>
>> 		if (buf_node->list.next == &ispif->cmdq)
>> 			quit = true;
>> 		kfree(buf_node);
>> 		if (quit)
>> 			return;
>> 	}
>> }
>> So, I'd like to keep previous implementation by removing list_del and adding
>> INIT_LIST_HEAD, so this will be the code after refine,
>>
>> void isp4if_clear_cmdq(struct isp4_interface *ispif)
>> {
>> 	struct isp4if_cmd_element *buf_node;
>> 	struct isp4if_cmd_element *tmp_node;
>>
>> 	guard(mutex)(&ispif->cmdq_mutex);
>>
>> 	list_for_each_entry_safe(buf_node, tmp_node, &ispif->cmdq, list)
>> 		kfree(buf_node);
>> 	INIT_LIST_HEAD(&ispif->cmdq);
>> }
>> It's much simpler, and based on our test, for command and buffer queue, the
>> elements in the queue won't exceed 5 when run to here, so the lock time will
>> be very short. What do you think?
> 
> This is what I am thinking (with cmdq_mutex replaced with a spin lock):
> 
> void isp4if_clear_cmdq(struct isp4_interface *ispif)
> {
> 	struct isp4if_cmd_element *buf_node, *tmp_node;
> 	LIST_HEAD(free_list);
> 
> 	scoped_guard(spinlock, &ispif->cmdq_lock)
> 		list_splice_init(&ispif->cmdq, &free_list);
> 
> 	list_for_each_entry_safe(buf_node, tmp_node, &free_list, list)
> 		kfree(buf_node);
> }
> 

Many thanks for the reference code, it's concise and easy to understand, 
will incorporate it into the next version.

>>>> +	struct isp4if_cmd_element *cmd_ele = NULL;
>>>> +	struct isp4if_rb_config *rb_config;
>>>> +	struct device *dev = ispif->dev;
>>>> +	struct isp4fw_cmd cmd = {};
>>>
>>> Use memset() to guarantee padding bits of cmd are zeroed, since this may not
>>> guarantee it on all compilers.
>>>
>>
>> Sure, will do it in the next version. Just a question, padding bits seem
>> never to be used, will it cause any problem if they are not zeroed?
> 
> Padding bits, if there are any, are used by isp4if_compute_check_sum() and are
> also sent to the firmware.
> 

Yes, this will impact the checksum value. However, based on my 
understanding, it will not affect the error detection outcome, since the 
firmware uses the same padding bits during both checksum calculation and 
comparison. I apologize for the minor disagreement—I just want to avoid 
introducing redundant code, especially given that similar scenarios 
appear a lot. Originally, we used memset in the initial version, but 
switched to { } initialization in subsequent versions based on review 
feedback. Please feel free to share your ideas, if you believe it is 
still necessary, we will add them.

>>>> +
>>>> +	ret = isp4if_insert_isp_fw_cmd(ispif, stream, &cmd);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "fail for insert_isp_fw_cmd camId (0x%08x)\n", cmd_id);
>>>> +		if (cmd_ele) {
>>>> +			isp4if_rm_cmd_from_cmdq(ispif, cmd_ele->seq_num, cmd_ele->cmd_id);
>>>
>>> Using isp4if_rm_cmd_from_cmdq() sort of implies that there is a risk that
>>> cmd_ele may have been removed from the list somehow, even though the fw cmd
>>> insertion failed? In any case, either make it truly safe by assuming that it's
>>> unsafe to dereference cmd_ele right now, or just delete cmd_ele directly from
>>> the list under the lock.
>>>
>>
>> Good consideration, so will change it to following in the next version,
>> ret = isp4if_insert_isp_fw_cmd(ispif, stream, &cmd);
>> if (ret) {
>> 	dev_err(dev, "fail for insert_isp_fw_cmd camId %s(0x%08x)\n",
>> 		isp4dbg_get_cmd_str(cmd_id), cmd_id);
>> 	if (cmd_ele) {
>> 		cmd_ele = isp4if_rm_cmd_from_cmdq(ispif, seq_num, cmd_id);
>> 		kfree(cmd_ele);
>> 	}
>> }
>> The final cmd_ele to be freed will come from cmdq which will be protected by
>> mutex, so it will be safe.
> 
> Looks good to me!
> 

Thanks for the confirmation.

>>>> +static int isp4if_send_buffer(struct isp4_interface *ispif, struct isp4if_img_buf_info *buf_info)
>>>> +{
>>>> +	struct isp4fw_cmd_send_buffer cmd = {};
>>>
>>> Use memset() to guarantee padding bits are zeroed, since this may not guarantee
>>> it on all compilers.
>>>
>>
>> Sure, will do it in the next version. as mentioned above, padding bits seem
>> never to be used, will it cause any problem if they are not zeroed?
> 
> Padding bits, if there are any, are used by isp4if_compute_check_sum() and are
> also sent to the firmware.
> 

Same comments as above

>>>> +
>>>> +	guard(mutex)(&ispif->bufq_mutex);
>>>> +
>>>> +	list_for_each_entry_safe(buf_node, tmp_node, &ispif->bufq, node) {
>>>> +		list_del(&buf_node->node);
>>>> +		kfree(buf_node);
>>>> +	}
>>>
>>> Move the whole list to a local LIST_HEAD(free_list) variable and then release
>>> the lock. Then you can list_for_each_entry_safe() without needing to do a
>>> list_del() every time, and you won't need to hold the lock the whole time.
>>>
>>
>> Same comments as above cmdq
> 
> This is what I am thinking (with bufq_mutex replaced with a spin lock):
> 
> void isp4if_clear_bufq(struct isp4_interface *ispif)
> {
> 	struct isp4if_img_buf_node *buf_node, *tmp_node;
> 	LIST_HEAD(free_list);
> 
> 	scoped_guard(spinlock, &ispif->bufq_lock)
> 		list_splice_init(&ispif->bufq, &free_list);
> 
> 	list_for_each_entry_safe(buf_node, tmp_node, &free_list, node)
> 		kfree(buf_node);
> }
> 

Very good reference, will update in the next version.

>>>> +struct isp4_interface {
>>>> +	struct device *dev;
>>>> +	void __iomem *mmio;
>>>> +
>>>> +	struct mutex cmdq_mutex; /* used for cmdq access */
>>>> +	struct mutex bufq_mutex; /* used for bufq access */
>>>
>>> It makes more sense for cmdq_mutex and bufq_mutex to be spin locks since they
>>> are only held briefly for list traversal.
>>>
>>
>> I'd like to keep them as mutex, because 1.no sync with hard/soft interrupt
>> is needed, 2.Not very time critical 3.Make guard mutex optimization
>> possible. What do you think?
> 
> For very quick/short critical sections that don't sleep, it makes more sense to
> use a spin lock. A mutex lock is heavy when it needs to sleep on contention,
> which isn't worth it if the critical section has very few instructions.
> 
> Also, guard and scoped_guard are available for spin locks too, as shown in my
> replies above.
> 

Thank you for the detailed explanation. Really appreciate the insights 
and will make sure to include these updates in the next version.

>>>> +
>>>> +#endif
>>>
>>> Add /* _ISP4_INTERFACE_ */
>>>
>>
>> Sure, will add it in the next version and check all header files. BTW, will
>> change the macro name to _ISP4_INTERFACE_H_ to align with others
> 
> Good catch, sounds good.
> 
> Sultan

-- 
Regards,
Bin

Powered by blists - more mailing lists