Re: [PATCH v7 5/7] media: platform: amd: isp4 video node and buffers handling added

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

Thank you, Sakari. I sincerely appreciate you taking the time from your 
busy schedule to provide the valuable feedback.

On 1/30/2026 5:17 AM, Sakari Ailus wrote:
> Hi Bin,
> 
> On Fri, Jan 09, 2026 at 06:08:00PM +0800, Du, Bin wrote:
>>>> +static const struct vb2_mem_ops isp4vid_vb2_memops = {
>>>> +	.alloc		= isp4vid_vb2_alloc,
>>>> +	.put		= isp4vid_vb2_put,
>>>> +#ifdef CONFIG_HAS_DMA
>>>> +	.get_dmabuf	= isp4vid_vb2_get_dmabuf,
>>>> +#endif
>>>> +	.map_dmabuf	= isp4vid_vb2_map_dmabuf,
>>>> +	.unmap_dmabuf	= isp4vid_vb2_unmap_dmabuf,
>>>> +	.attach_dmabuf	= isp4vid_vb2_attach_dmabuf,
>>>> +	.detach_dmabuf	= isp4vid_vb2_detach_dmabuf,
>>>> +	.vaddr		= isp4vid_vb2_vaddr,
>>>> +	.mmap		= isp4vid_vb2_mmap,
>>>> +	.num_users	= isp4vid_vb2_num_users,
>>>> +};
>>>
>>> Could you elaborate a bit why do you need your own videobuf mem ops?
>>>
>>
>> Sure, ISP FW/HW can access system memory only via GPU VA, not system VA, so
>> vb2_vmalloc_memops can't be used directly, we need to base on it to
>> implement our own videobuf mem ops to support both GPU VA and system VA.
> 
> It's fine to use other virtual addresses than system ones (a lot of other
> drivers do in fact, e.g. IPU6), you generally don't need to add new memory
> types for this.
> 
> ...
> 

Thanks for the guidance, will work on it.

>>>> +struct isp4vid_capture_buffer {
>>>> +	/*
>>>> +	 * struct vb2_v4l2_buffer must be the first element
>>>> +	 * the videobuf2 framework will allocate this struct based on
>>>> +	 * buf_struct_size and use the first sizeof(struct vb2_buffer) bytes of
>>>> +	 * memory as a vb2_buffer
>>>> +	 */
>>>> +	struct vb2_v4l2_buffer vb2;
>>>> +	struct isp4if_img_buf_info img_buf;
>>>> +	struct list_head list;
>>>> +};
>>>> +
>>>> +struct isp4vid_ops {
>>>> +	int (*send_buffer)(struct v4l2_subdev *sd,
>>>> +			   struct isp4if_img_buf_info *img_buf);
>>>
>>> Is there a reason why isp4sd_ioc_send_img_buf() isn't called directly?
>>>
>>
>> In our design, isp4sd serves as the upper layer of video. Therefore, isp4sd
>> can directly call functions within video, but not vice versa, This callback
>> mechanism is implemented to support the call from video to isp4sd by
>> indirect way.
> 
> You still have a single module, don't you? Thus you can make a direct
> function call. Only use a callback pointer when you actually need one.
> 

Yes, exactly, will switch to direct function call which is more 
straightforward.

>>
>>>> +};
>>>> +
>>>> +struct isp4vid_dev {
>>>> +	struct video_device vdev;
>>>> +	struct media_pad vdev_pad;
>>>> +	struct v4l2_pix_format format;
>>>> +
>>>> +	/* mutex that protects vbq */
>>>> +	struct mutex vbq_lock;
>>>> +	struct vb2_queue vbq;
>>>> +
>>>> +	/* mutex that protects buf_list */
>>>> +	struct mutex buf_list_lock;
>>>> +	struct list_head buf_list;
>>>> +
>>>> +	u32 sequence;
>>>> +	bool stream_started;
>>>> +
>>>> +	struct media_pipeline pipe;
> 
> You might not need this for the time being at least.
> 

Sure, will drop it.

>>>> +	struct device *dev;
>>>> +	struct v4l2_subdev *isp_sdev;
>>>> +	struct v4l2_fract timeperframe;
>>>> +
>>>> +	/* Callback operations */
>>>> +	const struct isp4vid_ops *ops;
>>>> +};
>>>> +
>>>> +int isp4vid_dev_init(struct isp4vid_dev *isp_vdev,
>>>> +		     struct v4l2_subdev *isp_sdev,
>>>> +		     const struct isp4vid_ops *ops);
>>>> +
>>>> +void isp4vid_dev_deinit(struct isp4vid_dev *isp_vdev);
>>>> +
>>>> +void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
>>>> +			       const struct isp4if_img_buf_info *img_buf);
>>>> +
>>>> +#endif /* _ISP4_VIDEO_H_ */
> 

-- 
Regards,
Bin