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

[Ben Hoff]

Hi Hans,

I ran the  v4l2-compliance from the repo and posted the output as part
of the newest patch here:
https://patchwork.linuxtv.org/project/linux-media/patch/20260510235037.24876-1-hoff.benjamin.k@xxxxxxxxx/

The results are also here for ease:
48 tests succeeded, 0 failed, 1 warning

I also realized that when preparing v6, I hadn’t copied the latest
changes from my standalone repo into the Linux tree correctly.
To avoid that happening again, I added a small script that compares
the driver files byte-for-byte between the two trees before sending
the patch.

Thanks again for the review. I appreciate it.
-Ben


On Thu, May 7, 2026 at 1:10 AM Hans Verkuil <hverkuil+cisco@xxxxxxxxxx> wrote:
>
> On 06/05/2026 21:43, Ben Hoff wrote:
> > 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.
>
> Thank you for the quick turnaround.
>
> I hope to review v6 soon (hopefully early next week), so unless I find something
> else I should be able to merge it for v7.2.
>
> One request: can you do another run with v4l2-compliance and post the output?
>
> If possible, please compile v4l2-compliance from the git repository (git://linuxtv.org/v4l-utils.git)
> so you test with the latest version.
>
> Regards,
>
>         Hans
>
> >
> > 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
> >
>