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

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

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@gmail.com/
> 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@kernel.org> 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@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
>