From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id B61023264FB; Thu, 7 May 2026 05:10:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778130632; cv=none; b=i561lQw6SeC6NP4aTLRQr+RcoKX7X1j8OOalzP/9pG+wmDNoYAwCSYYVB9vl+HI2Y7VmBJ5Tk2VsUePSqSMQSYlYvBdQkLhGpNPo3Kn4b28UVw88saXdqXMv5JN/U/dt/ZKP8BGFtItRU47xGjLnP5ow027vZSnBmJbCK47VLCk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778130632; c=relaxed/simple; bh=rhHqq9QOJUcYWnAhSTCZoNodlZt4AlyKXe796kzQmyA=; h=Message-ID:Date:MIME-Version:From:Subject:To:Cc:References: In-Reply-To:Content-Type; b=iUnuTQZYjyi/h2A9RXhHuhNzZsOdm0CIRnGvzSO815Atm5uL3BSDzAnW0i1EugOPhWLdI9Nmyx1I6aGPiP/d4yH4do+nyYTjlhZKRKd8I5mKz3avCedJjLOWULxnMDeGOdVdDgylBUBfptp/sebKoIhjpFEqyNmxbUj8DbkYhj4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=nCmrOM89; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="nCmrOM89" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 074FFC2BCB8; Thu, 7 May 2026 05:10:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1778130632; bh=rhHqq9QOJUcYWnAhSTCZoNodlZt4AlyKXe796kzQmyA=; h=Date:From:Subject:To:Cc:References:In-Reply-To:From; b=nCmrOM89xdsuJjky3xh9MzjQh7SEk2+h6C3Tg98pb7FVaqovANcnlDpEizFLklMGZ jW9sUeMrVGe7TCcGEO/2Fxb6NdRR0G4U5H9GmceiTtzkD7b7mTtKwDLM5hWDmpz6tN +NLkcDe1Qlx9yP3yBGsdJ3hPh0Tjx1O/Xig0+bcl+UWDPfRfGrW09TvTTzg64Q979Z yKsdy8oZDEQivqU/N7R9b1odCgH3Vb67VFA4AKWXAZPTGO05PGnyZBxgCIheXrO1o0 5povcRjpVzD6Hfrr4VSuZwFx+ILP5K96okniyYaNE+mRjRYxr6Tsa4kem9Y8QZD2sh ZQEYxEmoJNVfA== Message-ID: <0e668d98-9a7b-4422-86cf-8f9163d768d0@kernel.org> Date: Thu, 7 May 2026 07:10:29 +0200 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Hans Verkuil Subject: Re: [PATCH v5] media: pci: add AVMatrix HWS capture driver To: Ben Hoff Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, mchehab@kernel.org, kernel test robot References: <20260330233636.381969-1-hoff.benjamin.k@gmail.com> <20260403135709.46163-1-hoff.benjamin.k@gmail.com> <336b8ec9-f3e3-4850-9261-752392a898ac@kernel.org> Content-Language: en-US, nl In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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 >>> >>> 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 >>> Closes: https://lore.kernel.org/oe-kbuild-all/202604020522.z22eZuW8-lkp@intel.com/ >>> Signed-off-by: Ben Hoff >>> --- >>> 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 >>> >> >> >> >>> 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 @@ >> >> >> >>> +/* 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); >>> +} >> >> >> >>> 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 @@ >> >> >> >>> +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 >