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

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

[Hans Verkuil]

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

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

[Ben Hoff]

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.

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

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

[Hans Verkuil]

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
> 

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

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