Message-ID: <aNOZM2fj1X9TfZSF@sultan-box>
Date: Wed, 24 Sep 2025 00:09:39 -0700
From: Sultan Alsawaf <sultan@...neltoast.com>
To: "Du, Bin" <bin.du@....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

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.

> > > +	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.

> > > +
> > > +	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);
}

> > > +	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.

> > > +
> > > +	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!

> > > +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.

> > > +
> > > +	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);
}

> > > +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.

> > > +
> > > +#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

Powered by blists - more mailing lists