Message-ID: <aOtF5C2p0KGTQSji@sultan-box>
Date: Sat, 11 Oct 2025 23:08:36 -0700
From: Sultan Alsawaf <sultan@...neltoast.com>
To: "Du, Bin" <bin.du@....com>
Cc: mchehab@...nel.org, hverkuil@...all.nl,
	laurent.pinchart+renesas@...asonboard.com,
	bryan.odonoghue@...aro.org, sakari.ailus@...ux.intel.com,
	prabhakar.mahadev-lad.rj@...renesas.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	pratap.nirujogi@....com, benjamin.chan@....com, king.li@....com,
	gjorgji.rosikopulos@....com, Phil.Jawich@....com,
	Dominic.Antony@....com, mario.limonciello@....com,
	richard.gong@....com, anson.tsao@....com,
	Svetoslav Stoilov <Svetoslav.Stoilov@....com>,
	Alexey Zagorodnikov <xglooom@...il.com>
Subject: Re: [PATCH v4 5/7] media: platform: amd: isp4 video node and buffers
 handling added

On Sat, Oct 11, 2025 at 05:30:43PM +0800, Du, Bin wrote:
> On 10/1/2025 2:53 PM, Sultan Alsawaf wrote:
> > On Thu, Sep 11, 2025 at 06:08:45PM +0800, Bin Du wrote:
> > > +
> > > +/* Sizes must be in increasing order */
> > > +static const struct v4l2_frmsize_discrete isp4vid_frmsize[] = {
> > > +	{640, 360},
> > > +	{640, 480},
> > > +	{1280, 720},
> > > +	{1280, 960},
> > > +	{1920, 1080},
> > > +	{1920, 1440},
> > > +	{2560, 1440},
> > > +	{2880, 1620},
> > > +	{2880, 1624},
> > > +	{2888, 1808},
> > > +};
> > > +
> > > +static const u32 isp4vid_formats[] = {
> > > +	V4L2_PIX_FMT_NV12,
> > > +	V4L2_PIX_FMT_YUYV
> > > +};
> > > +
> > > +/* timeperframe list */
> > > +static const struct v4l2_fract isp4vid_tpfs[] = {
> > > +	{.numerator = 1, .denominator = ISP4VID_MAX_PREVIEW_FPS}
> > 
> > Add a space after { and a space before }.
> > 
> > Also, it is more common to see v4l2_fract initialized without specifying the
> > struct member names.
> > 
> > To summarize, change to `{ 1, ISP4VID_MAX_PREVIEW_FPS }`
> > 
> 
> Will follow your advice. Seems no explicit description about this in "Linux
> kernel coding style" and both spacing options after { are common in the
> current kernel code.

True, it's not explicitly stated in "Linux kernel coding style", but it _is_
specified in the .clang-format [1][2] via `Cpp11BracedListStyle: false`. And in
my experience, it's much more common to see spaces padded inside braced lists.

> > > +};
> > > +
> > > +static void isp4vid_handle_frame_done(struct isp4vid_dev *isp_vdev,
> > > +				      const struct isp4if_img_buf_info *img_buf,
> > > +				      bool done_suc)
> > > +{
> > > +	struct isp4vid_capture_buffer *isp4vid_buf;
> > 
> > Rename isp4vid_buf to isp_buf like in isp4vid_qops_start_streaming().
> > 
> 
> Seems isp4vid_buf appears to be more descriptive, plan to rename isp_buf in
> isp4vid_qops_start_streaming to isp4vid_buf, what do you think?

Either way is fine so long as it is consistent. It's just my own personal
preference to use shorter variable names, especially for pointers, which is why
I suggested isp_buf. :)

> > > +
> > > +	buf->dev = dev;
> > > +	buf->dbuf = dbuf;
> > > +	buf->size = size;
> > > +
> > > +	dev_dbg(dev, "attach dmabuf of isp user bo 0x%llx size %ld",
> > > +		dbg_buf->gpu_addr, dbg_buf->size);
> > > +
> > > +	return buf;
> > > +}
> > > +
> > > +static void isp4vid_vb2_unmap_dmabuf(void *mem_priv)
> > > +{
> > > +	struct isp4vid_vb2_buf *buf = mem_priv;
> > > +	struct iosys_map map = IOSYS_MAP_INIT_VADDR(buf->vaddr);
> > > +
> > > +	dev_dbg(buf->dev, "unmap dmabuf of isp user bo 0x%llx size %ld",
> > > +		buf->gpu_addr, buf->size);
> > > +
> > > +	dma_buf_vunmap_unlocked(buf->dbuf, &map);
> > > +	buf->vaddr = NULL;
> > > +}
> > > +
> > > +static int isp4vid_vb2_map_dmabuf(void *mem_priv)
> > > +{
> > > +	struct isp4vid_vb2_buf *mmap_buf = NULL;
> > 
> > Remove unnecessary initialization of `mmap_buf`, and combine it onto one line
> > with `buf`:
> > 
> 
> Sure, will remove unnecessary initialization of `mmap_buf, but i'd rather
> not combine because it is mentioned in "Linux kernel coding style" that "to
> this end, use just one data declaration per line (no commas for multiple
> data declarations). This leaves you room for a small comment on each item,
> explaining its use.", what do you think?

Huh, that quote is odd, as it's quite common in the kernel to declare multiple
local variables of the same type on one line. In fact, Linus has done this
himself [3][4].

I think it's better to put `mmap_buf` on the same line because the type name is
quite long, so declaring `buf` and `mmap_buf` on the same line makes it easier
to see that they are exactly the same type.

Also, creating a new line for every local variable declaration would really
bloat a lot of code around the kernel and hurt readability. That quote from
"Linux kernel coding style" was added almost *20 years* ago (commit b3fc9941fbc6
"[PATCH] CodingStyle updates"), so maybe it is just obsolete.

> > > +	if (ret) {
> > > +		dev_err(v4l2_dev->dev, "fail to fill buffer size: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = isp4vid_set_fmt_2_isp(isp_sdev, &isp_vdev->format);
> > > +	if (ret) {
> > > +		dev_err(v4l2_dev->dev, "fail init format :%d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/* Initialize the video_device struct */
> > > +	isp_vdev->vdev.entity.name = vdev_name;
> > > +	isp_vdev->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
> > > +	isp_vdev->vdev_pad.flags = MEDIA_PAD_FL_SINK;
> > > +	ret = media_entity_pads_init(&isp_vdev->vdev.entity, 1,
> > > +				     &isp_vdev->vdev_pad);
> > > +
> > > +	if (ret) {
> > > +		dev_err(v4l2_dev->dev, "init media entity pad fail:%d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE |
> > > +			    V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
> > > +	vdev->entity.ops = &isp4vid_vdev_ent_ops;
> > > +	vdev->release = video_device_release_empty;
> > > +	vdev->fops = &isp4vid_vdev_fops;
> > > +	vdev->ioctl_ops = &isp4vid_vdev_ioctl_ops;
> > > +	vdev->lock = NULL;
> > > +	vdev->queue = q;
> > > +	vdev->v4l2_dev = v4l2_dev;
> > > +	vdev->vfl_dir = VFL_DIR_RX;
> > > +	strscpy(vdev->name, vdev_name, sizeof(vdev->name));
> > > +	video_set_drvdata(vdev, isp_vdev);
> > > +
> > > +	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > > +	if (ret)
> > > +		dev_err(v4l2_dev->dev, "register video device fail:%d\n", ret);
> > 
> > No error handling?
> > 
> 
> Not necessary here because the caller isp4sd_init() will handle this.

This doesn't seem to be the case; in fact, isp4sd_init() doesn't seem to handle
any of the error cleanup for isp4vid_dev_init().

I started looking deeper at it and saw that vb2_queue_release() is never called
to clean up after vb2_queue_init(). See my next comment below about changing
video_unregister_device() to vb2_video_unregister_device(), which calls
vb2_queue_release().

And isp4sd_init() calls media_entity_cleanup() for the subdev, not the vdev.
So you need to add `media_entity_cleanup(&isp_vdev->vdev.entity)`.

To summarize, make the following changes to isp4vid_dev_init():

1. On error starting from isp4vid_fill_buffer_size(), call vb2_queue_release()
   to do cleanup for vb2_queue_init().

2. When video_register_device() fails, do cleanup for media_entity_pads_init()
   by adding `media_entity_cleanup(&isp_vdev->vdev.entity)`.

> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +void isp4vid_dev_deinit(struct isp4vid_dev *isp_vdev)
> > > +{
> > > +	video_unregister_device(&isp_vdev->vdev);
> > > +}

I just noticed: isp4vid_dev_deinit() should call vb2_video_unregister_device()
instead of video_unregister_device().

See the comment in include/media/videobuf2-v4l2.h [5]:

 * If the driver uses vb2_fop_release()/_vb2_fop_release(), then it should use
 * vb2_video_unregister_device() instead of video_unregister_device().

Since vb2_fop_release() is used, vb2_video_unregister_device() should be used.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/.clang-format?h=v6.17#n61
[2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#cpp11bracedliststyle
[3] https://github.com/torvalds/linux/commit/fe673d3f5bf1fc50cdc4b754831db91a2ec10126#diff-b7746cf0f2ab471c30d08fe3391b7d3ba45adbe7616e4fae0504b29f40b49dd5L1747-R1747
[4] https://github.com/torvalds/linux/commit/d7fe75cbc23c7d225eee2ef04def239b6603dce7#diff-b8c8d3c5770869f743e155faac7cccc97082918c3e092ffdf592efa796725c79L2019-R2019
[5] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/media/videobuf2-v4l2.h?h=v6.17#n360

Sultan

Powered by blists - more mailing lists