* Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver [not found] ` <20260403135709.46163-1-hoff.benjamin.k@gmail.com> @ 2026-05-05 10:37 ` Hans Verkuil 2026-05-06 19:43 ` Ben Hoff 0 siblings, 1 reply; 4+ messages in thread From: Hans Verkuil @ 2026-05-05 10:37 UTC (permalink / raw) To: hoff.benjamin.k, linux-media; +Cc: linux-kernel, mchehab, kernel test robot 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver 2026-05-05 10:37 ` [PATCH v5] media: pci: add AVMatrix HWS capture driver Hans Verkuil @ 2026-05-06 19:43 ` Ben Hoff 2026-05-07 5:10 ` Hans Verkuil 0 siblings, 1 reply; 4+ messages in thread From: Ben Hoff @ 2026-05-06 19:43 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media, linux-kernel, mchehab, kernel test robot 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver 2026-05-06 19:43 ` Ben Hoff @ 2026-05-07 5:10 ` Hans Verkuil 2026-05-11 0:09 ` Ben Hoff 0 siblings, 1 reply; 4+ messages in thread From: Hans Verkuil @ 2026-05-07 5:10 UTC (permalink / raw) To: Ben Hoff; +Cc: linux-media, linux-kernel, mchehab, kernel test robot 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver 2026-05-07 5:10 ` Hans Verkuil @ 2026-05-11 0:09 ` Ben Hoff 0 siblings, 0 replies; 4+ messages in thread From: Ben Hoff @ 2026-05-11 0:09 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media, linux-kernel, mchehab, kernel test robot 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 > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-05-11 0:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20260330233636.381969-1-hoff.benjamin.k@gmail.com>
[not found] ` <20260403135709.46163-1-hoff.benjamin.k@gmail.com>
2026-05-05 10:37 ` [PATCH v5] media: pci: add AVMatrix HWS capture driver Hans Verkuil
2026-05-06 19:43 ` Ben Hoff
2026-05-07 5:10 ` Hans Verkuil
2026-05-11 0:09 ` Ben Hoff
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox