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: <aXvOfXNn2gGsmkfg@valkosipuli.retiisi.eu>
Date: Thu, 29 Jan 2026 23:17:49 +0200
From: Sakari Ailus <sakari.ailus@....fi>
To: "Du, Bin" <bin.du@....com>
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

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.

...

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

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

> > > +	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_ */

-- 
Kind regards,

Sakari Ailus