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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <35075404-d0ef-4571-bb86-05f63f95a444@amd.com>
Date: Tue, 3 Feb 2026 14:32:52 +0800
From: "Du, Bin" <bin.du@....com>
To: Sakari Ailus <sakari.ailus@....fi>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>,
 "mchehab@...nel.org" <mchehab@...nel.org>,
 "hverkuil@...all.nl" <hverkuil@...all.nl>,
 "laurent.pinchart+renesas@...asonboard.com"
 <laurent.pinchart+renesas@...asonboard.com>,
 "bryan.odonoghue@...aro.org" <bryan.odonoghue@...aro.org>,
 "prabhakar.mahadev-lad.rj@...renesas.com"
 <prabhakar.mahadev-lad.rj@...renesas.com>,
 "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "sultan@...neltoast.com" <sultan@...neltoast.com>,
 "Nirujogi, Pratap" <Pratap.Nirujogi@....com>,
 "Chan, Benjamin (Koon Pan)" <Benjamin.Chan@....com>,
 "Li, King" <King.Li@....com>,
 "Rosikopulos, Gjorgji" <Gjorgji.Rosikopulos@....com>,
 "Jawich, Phil" <Phil.Jawich@....com>,
 "Antony, Dominic" <Dominic.Antony@....com>,
 "Limonciello, Mario" <Mario.Limonciello@....com>,
 "Gong, Richard" <Richard.Gong@....com>, "Tsao, Anson" <anson.tsao@....com>,
 Svetoslav Stoilov <Svetoslav.Stoilov@....com>,
 Alexey Zagorodnikov <xglooom@...il.com>
Subject: Re: [PATCH v7 5/7] media: platform: amd: isp4 video node and buffers
 handling added

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