Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface

["Du, Bin" <bin.du@amd.com>]

Thanks Sakari Ailus for the comments

On 8/11/2025 7:46 PM, Sakari Ailus wrote:
> Hi Bin,
> 
> On Tue, Jul 29, 2025 at 05:12:03PM +0800, Du, Bin wrote:
>> Many thanks Askari Ailus for your careful review
>>
>> On 7/28/2025 3:23 PM, Sakari Ailus wrote:
>>> Hi Bin,
>>>
>>> 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.
> 
> ...
> 

Got it, thanks.

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

Yes, totally agree.

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

Thanks, will double check all new patches to avoid similar problem.

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

Yes, you are absolutely right. Sorry, missed the kmalloc before the 
memcpy, will fix in the next patch.

>>>> +
>>>> +	guard(mutex)(&ispif->cmdq_mutex);
>>>> +
>>>> +	list_add_tail(&copy_command->list, &ispif->cmdq);
>>>> +
>>>> +	return copy_command;
>>>> +}
> 

Regards,
Bin