Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface
From: Du, Bin
Date: Mon Aug 11 2025 - 23:36:52 EST
Many thanks Laurent Pinchart for the comments.
On 8/11/2025 8:31 PM, Laurent Pinchart wrote:
On Mon, Aug 11, 2025 at 11:46:27AM +0000, Sakari Ailus wrote:
On Tue, Jul 29, 2025 at 05:12:03PM +0800, Du, Bin wrote:
On 7/28/2025 3:23 PM, Sakari Ailus wrote:
On Wed, Jun 18, 2025 at 05:19:55PM +0800, Bin Du wrote:
ISP firmware controls ISP HW pipeline using dedicated embedded processor
called ccpu.
The communication between ISP FW and driver is using commands and
response messages sent through the ring buffer. Command buffers support
either global setting that is not specific to the steam and support stream
specific parameters. Response buffers contains ISP FW notification
information such as frame buffer done and command done. IRQ is used for
receiving response buffer from ISP firmware, which is handled in the main
isp4 media device. ISP ccpu is booted up through the firmware loading
helper function prior to stream start.
Memory used for command buffer and response buffer needs to be allocated
from amdgpu buffer manager because isp4 is a child device of amdgpu.
Please rewrap this, some lines above are quite short.
Thanks, the line after the short line is supposed to be a new paragraph?
Should we put all the description in one paragraph?
One or more paragraphs work fine, but a new paragraph is separated from the
previous one by another newline.
...
Paragraphs are defined as a block of text that convey one idea. They
should be visually separated by a space. As we can't have fractional
line spacing in plain text, paragraphs need to be separated by a blank
line. This is a typography rule that maximizes readability. There should
be no line break between sentences in a single paragraph.
Whether you write commit messages, formal documentation or comments in
code, typography is important to give the best experience to readers.
After all, a block of text that wouldn't focus on the readers would have
no reason to exist.
Now compare the above with
Paragraphs are defined as a block of text that convey one idea. They
should be visually separated by a space.
As we can't have fractional line spacing in plain text, paragraphs need
to be separated by a blank line.
This is a typography rule that maximizes readability. There should be no
line break between sentences in a single paragraph. Whether you write
commit messages, formal documentation or comments in code, typography is
important to give the best experience to readers.
After all, a block of text that wouldn't focus on the readers would have
no reason to exist.
Really appreciate the detailed guide, will follow it. May I summarize
like this? 1 Separate paragraphs by a blank line. 2 Don't add line break
between sentences in a single paragraph, an exception to this is commit
message, because of the 75-character patch check limit, line break can
be added, but it should at the 75-character limit boundary
+ void *cpu_ptr;
+ u64 gpu_addr;
+ u32 ret;
+
+ dev = ispif->dev;
+
+ if (!mem_size)
+ return NULL;
+
+ mem_info = kzalloc(sizeof(*mem_info), GFP_KERNEL);
+ if (!mem_info)
+ return NULL;
+
+ adev = (struct amdgpu_device *)ispif->adev;
Why the cast?
adev isn't a great name here as it's usually used for struct acpi_devices.
In the next patch, will use new helper function for this and will no longer
use amdgpu_device
Use correct types when you can; either way this doesn't seem to be changed
by the further patches in the set.
...
+static int isp4if_gpu_mem_free(struct isp4_interface *ispif,
+ struct isp4if_gpu_mem_info *mem_info)
+{
+ struct device *dev = ispif->dev;
+ struct amdgpu_bo *bo;
+
+ if (!mem_info) {
+ dev_err(dev, "invalid mem_info\n");
+ return -EINVAL;
+ }
+
+ bo = (struct amdgpu_bo *)mem_info->mem_handle;
Why do you need to cast here?
In the next patch, will use new helper function for this and will no longer
use amdgpu_bo
Not quite, on top of this patch number 6 adds more of the same.
...
+static struct isp4if_cmd_element *
+isp4if_append_cmd_2_cmdq(struct isp4_interface *ispif,
+ struct isp4if_cmd_element *cmd_ele)
+{
+ struct isp4if_cmd_element *copy_command = NULL;
+
+ copy_command = kmalloc(sizeof(*copy_command), GFP_KERNEL);
+ if (!copy_command)
+ return NULL;
+
+ memcpy(copy_command, cmd_ele, sizeof(*copy_command));
kmemdup()?
Kmemdup is to allocate memory and copy, can't be used here.
Isn't that what you're doing above?
+
+ guard(mutex)(&ispif->cmdq_mutex);
+
+ list_add_tail(©_command->list, &ispif->cmdq);
+
+ return copy_command;
+}