Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver

[Hans Verkuil <hverkuil+cisco@kernel.org>]

Hi Ben,

While reviewing v5 I discovered some issues, one of them (sizeimage handling)
important enough to warrant a v6.

On 4/3/26 15:57, hoff.benjamin.k@gmail.com wrote:
> From: Ben Hoff <hoff.benjamin.k@gmail.com>
> 
> Add an in-tree AVMatrix HWS PCIe capture driver. The driver supports
> up to four HDMI inputs and exposes the video capture path through
> V4L2 with vb2-dma-contig streaming, DV timings, and per-input
> controls. Audio support is intentionally omitted from this
> submission.
> 
> This patch also adds the MAINTAINERS entry for the new driver.
> 
> This driver is derived from a GPL out-of-tree driver.
> 
> Changes since v4:
> - replace plain 64-bit elapsed-time divisions in debug logging with
>   div_u64() so i386 module builds do not emit __udivdi3 references
> 
> Changes since v3:
> - fold the MAINTAINERS update into this patch so per-patch CI sees the
>   new file pattern
> - wrap the validation text for checkpatch
> 
> Changes since v2:
> - keep scratch DMA allocation on a single probe-owned path
> - avoid double-freeing V4L2 control handlers on register unwind
> - drop the extra per-node resolution sysfs ABI
> - turn live geometry changes into explicit SOURCE_CHANGE renegotiation
> - report live DV timings and reject attempts to retime a live source
> - stop advertising RESOLUTION source changes for fps-only updates
> - keep live fps state across harmless S_FMT restarts
> - stop exposing an unvalidated DV RX power-present signal
> - clean the imported sources for checkpatch and W=1 builds
> 
> Validation:
> - build-tested with W=1 against a local kernel build tree
> - compiled the driver with ARCH=i386 allmodconfig and verified the
>   resulting hws_pci.o, hws_video.o, and hws.o do not reference
>   __udivdi3
> - v4l2-compliance 1.32.0 on /dev/video1: 51 tests succeeded,
>   0 failed, 1 warning
> 
> DV_RX_POWER_PRESENT is intentionally left unsupported in this revision
> because current hardware evidence does not expose a validated
> receiver-side power-detect signal distinct from active video presence.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202604020522.z22eZuW8-lkp@intel.com/
> Signed-off-by: Ben Hoff <hoff.benjamin.k@gmail.com>
> ---
>  MAINTAINERS                            |    6 +
>  drivers/media/pci/Kconfig              |    1 +
>  drivers/media/pci/Makefile             |    1 +
>  drivers/media/pci/hws/Kconfig          |   12 +
>  drivers/media/pci/hws/Makefile         |    4 +
>  drivers/media/pci/hws/hws.h            |  174 +++
>  drivers/media/pci/hws/hws_irq.c        |  271 +++++
>  drivers/media/pci/hws/hws_irq.h        |   10 +
>  drivers/media/pci/hws/hws_pci.c        |  865 ++++++++++++++
>  drivers/media/pci/hws/hws_reg.h        |  136 +++
>  drivers/media/pci/hws/hws_v4l2_ioctl.c |  924 +++++++++++++++
>  drivers/media/pci/hws/hws_v4l2_ioctl.h |   36 +
>  drivers/media/pci/hws/hws_video.c      | 1506 ++++++++++++++++++++++++
>  drivers/media/pci/hws/hws_video.h      |   29 +
>  14 files changed, 3975 insertions(+)
>  create mode 100644 drivers/media/pci/hws/Kconfig
>  create mode 100644 drivers/media/pci/hws/Makefile
>  create mode 100644 drivers/media/pci/hws/hws.h
>  create mode 100644 drivers/media/pci/hws/hws_irq.c
>  create mode 100644 drivers/media/pci/hws/hws_irq.h
>  create mode 100644 drivers/media/pci/hws/hws_pci.c
>  create mode 100644 drivers/media/pci/hws/hws_reg.h
>  create mode 100644 drivers/media/pci/hws/hws_v4l2_ioctl.c
>  create mode 100644 drivers/media/pci/hws/hws_v4l2_ioctl.h
>  create mode 100644 drivers/media/pci/hws/hws_video.c
>  create mode 100644 drivers/media/pci/hws/hws_video.h
> 

<snip>

> diff --git a/drivers/media/pci/hws/hws_v4l2_ioctl.c b/drivers/media/pci/hws/hws_v4l2_ioctl.c
> new file mode 100644
> index 000000000000..9c0826c0f9f9
> --- /dev/null
> +++ b/drivers/media/pci/hws/hws_v4l2_ioctl.c
> @@ -0,0 +1,924 @@

<snip>

> +/* Query the *current detected* DV timings on the input.
> + * If you have a real hardware detector, call it here; otherwise we
> + * derive from the cached pix state and map to the closest supported DV mode.
> + */
> +int hws_vidioc_query_dv_timings(struct file *file, void *fh,
> +				struct v4l2_dv_timings *timings)
> +{
> +	struct hws_video *vid = video_drvdata(file);
> +	u32 w, h;
> +	u32 fps;
> +	bool interlace;
> +
> +	if (!timings)
> +		return -EINVAL;
> +
> +	w = vid->pix.width;
> +	h = vid->pix.height;
> +	interlace = vid->pix.interlaced;
> +	(void)hws_get_live_dv_geometry(vid, &w, &h, &interlace);

No need to cast to (void). I've seen it several times in this patch, just drop it.

> +	fps = hws_get_live_fps(vid);
> +	if (!fps)
> +		fps = vid->current_fps ? vid->current_fps :
> +		      hws_pick_fps_from_mode(w, h, interlace);
> +
> +	return hws_fill_dv_timings(w, h, interlace, fps, timings);
> +}

<snip>

> diff --git a/drivers/media/pci/hws/hws_video.c b/drivers/media/pci/hws/hws_video.c
> new file mode 100644
> index 000000000000..9c81af6e7d7f
> --- /dev/null
> +++ b/drivers/media/pci/hws/hws_video.c
> @@ -0,0 +1,1506 @@

<snip>

> +static int hws_queue_setup(struct vb2_queue *q, unsigned int *num_buffers,
> +			   unsigned int *nplanes, unsigned int sizes[],
> +			   struct device *alloc_devs[])
> +{
> +	struct hws_video *vid = q->drv_priv;
> +
> +	(void)num_buffers;
> +	(void)alloc_devs;

This shouldn't be needed.

> +
> +	if (!vid->pix.sizeimage) {

Why would this ever be 0? At probe time this should be set to something
sane.

> +		vid->pix.bytesperline = ALIGN(vid->pix.width * 2, 64);

Apparently vid->pix.width/height are valid (non-0), so why would sizeimage
be 0? vid->pix should always have sane consistent data.

> +		vid->pix.sizeimage = vid->pix.bytesperline * vid->pix.height;
> +	}
> +	if (*nplanes) {
> +		if (sizes[0] < vid->pix.sizeimage)

If PAGE_ALIGN is used below, then it should also be used here.
This can cause memory overwrite if you pass a buffer with VIDIOC_CREATEBUF
that is of size 'sizeimage' when it should be 'PAGE_ALIGN(sizeimage)'.

> +			return -EINVAL;
> +	} else {
> +		*nplanes = 1;
> +		sizes[0] = PAGE_ALIGN(vid->pix.sizeimage);

But if you need PAGE_ALIGN, why isn't vid->pix.sizeimage set with PAGE_ALIGN
in the first place?

> +	}
> +
> +	vid->alloc_sizeimage = PAGE_ALIGN(vid->pix.sizeimage);

What is alloc_sizeimage used for? I see it used only in a v4l2_dbg message.

> +	return 0;
> +}
Regards,

	Hans