Message-ID: <d0e66d15-f710-41b9-aa31-7648430034a9@amd.com>
Date: Mon, 13 Oct 2025 17:55:24 +0800
From: "Du, Bin" <bin.du@....com>
To: Sultan Alsawaf <sultan@...neltoast.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

Thanks Sultan.

On 10/12/2025 2:08 PM, Sultan Alsawaf wrote:
> 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.
> 

Thanks for the reference, yes, will add space pad.

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

Yes, will make them consistent.

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

Will combine them, thanks for the clear explanation.

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

Thank you for identifying the missed vb2 queue release and providing the 
modification guide, will follow it.

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

Yes, vb2_video_unregister_device() should be used in this case, will 
follow your advice.

> [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

-- 
Regards,
Bin

Powered by blists - more mailing lists