Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver
From: Ben Hoff
Date: Wed May 06 2026 - 15:43:34 EST
Hi Hans,
Thanks for the review.
I posted an updated version here:
https://patchwork.linuxtv.org/project/linux-media/patch/20260506192618.35384-1-hoff.benjamin.k@xxxxxxxxx/
This version removes the unnecessary `(void)` casts and fixes
the`sizeimage` handling in `queue_setup()`.
`queue_setup()` no longer tries to rebuild `vid->pix.sizeimage` on the
fly. The driver initializes the default format state during channel
setup, and format/timing changes update `vid->pix.sizeimage` through
`hws_calc_sizeimage()` before the queue is used.
I also removed the inconsistent `PAGE_ALIGN()` handling from
`queue_setup()`. The requested plane size is now checked against
`vid->pix.sizeimage`, and new buffers are sized to that same logical
V4L2 `sizeimage` value. If the hardware path later needs additional
padding, then that should be reflected in `pix.sizeimage` itself. This
behavior is consistent with the original driver.
Not sure where that crept in, but I had multiple allocation paths in
this driver at one point that I consolidated down for ease of
maintaining, so guessing during that shuffle.
Finally, `alloc_sizeimage` was only used as a debug/accounting value,
so I removed it entirely.
Thanks again for catching this.
Best,
Ben
On Tue, May 5, 2026 at 6:37 AM Hans Verkuil <hverkuil+cisco@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx wrote:
> > From: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202604020522.z22eZuW8-lkp@xxxxxxxxx/
> > Signed-off-by: Ben Hoff <hoff.benjamin.k@xxxxxxxxx>
> > ---
> > 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