[v5,2/2] media: sun6i-csi: capture: Implement vidioc_enum_framesizes
Commit Message
Report the stepwise frame size range supported by the CSI capture
hardware for the pixel formats exposed by the driver.
The hardware does not perform scaling and accepts any even width and
height within the reported limits.
Signed-off-by: Arash Golgol <arash.golgol@gmail.com>
---
.../sunxi/sun6i-csi/sun6i_csi_capture.c | 28 +++++++++++++++++++
1 file changed, 28 insertions(+)
Comments
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Inconsistent frame size constraints between `VIDIOC_ENUM_FRAMESIZES` and `VIDIOC_TRY_FMT` / `VIDIOC_S_FMT` for `V4L2_PIX_FMT_NV12_16L16`.
Pre-existing issues:
- [High] `TRY_FMT` calculates `sizeimage` incorrectly for `NV12_16L16` with unaligned dimensions, leading to potential DMA buffer overflows.
--
--- Patch [2]: [PATCH v5 2/2] media: sun6i-csi: capture: Implement vidioc_enum_framesizes ---
commit f5cc5843b18ce4fd2988ebb6c61247d1f5cd63a3
Author: Arash Golgol <arash.golgol@gmail.com>
media: sun6i-csi: capture: Implement vidioc_enum_framesizes
This commit reports the stepwise frame size range supported by the CSI
capture hardware for the pixel formats exposed by the driver.
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> index d90abba213098..e17c7f82ff478 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c
> @@ -738,6 +738,33 @@ static int sun6i_csi_capture_enum_fmt(struct file *file, void *priv,
> return 0;
> }
>
> +static int sun6i_csi_capture_enum_framesize(struct file *file, void *fh,
> + struct v4l2_frmsizeenum *fsize)
> +{
> + if (fsize->index)
> + return -EINVAL;
> +
> + /* Only accept format in map table. */
> + if (!sun6i_csi_capture_format_find(fsize->pixel_format))
> + return -EINVAL;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> + fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
> + fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
> + fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
> + fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
> +
> + if (fsize->pixel_format == V4L2_PIX_FMT_NV12_16L16) {
> + fsize->stepwise.step_width = 16;
> + fsize->stepwise.step_height = 16;
> + } else {
> + fsize->stepwise.step_width = 2;
> + fsize->stepwise.step_height = 2;
> + }
[Severity: Medium]
Will this lead to inconsistent frame size constraints?
While this reports a 16-pixel step width and height for
V4L2_PIX_FMT_NV12_16L16 via ENUM_FRAMESIZES,
drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c:sun6i_csi_capture_format_prepare()
(used by TRY_FMT and S_FMT) unconditionally uses v4l_bound_align_image()
with an alignment of 2:
v4l_bound_align_image(&pix_format->width, SUN6I_CSI_CAPTURE_WIDTH_MIN,
SUN6I_CSI_CAPTURE_WIDTH_MAX, 1,
&pix_format->height, SUN6I_CSI_CAPTURE_HEIGHT_MIN,
SUN6I_CSI_CAPTURE_HEIGHT_MAX, 1, 0);
Could this allow userspace to successfully request sizes via TRY_FMT that
ENUM_FRAMESIZES claims are unsupported, potentially leading to
v4l2-compliance test failures and contradictory format negotiation?
[Severity: High]
This isn't a bug introduced by this patch, but does TRY_FMT calculate
sizeimage incorrectly for NV12_16L16 with unaligned dimensions?
Since
drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c:sun6i_csi_capture_format_prepare()
allows width and height to be unaligned multiples of 2 and computes
sizeimage purely linearly:
case V4L2_PIX_FMT_NV12_16L16:
pix_format->bytesperline = width * 12 / 8;
pix_format->sizeimage = pix_format->bytesperline * height;
break;
Could this lead to a DMA buffer overflow?
The CSI DMA hardware natively writes in 16x16 tiles. If a user requests an
unaligned frame size (e.g., 34x34) for V4L2_PIX_FMT_NV12_16L16 via
VIDIOC_TRY_FMT, the allocated vb2 buffer will be smaller than what the
hardware DMA actually writes.
For example, a 34x34 image will be written as 3x3 tiles (48x48 pixels),
consuming 3456 bytes. However, the allocated buffer for 34x34 is only 1734
bytes (34 * 1.5 * 34), which would cause a buffer overflow when the DMA
writes the padded tiles past the end of the allocation.
> +
> + return 0;
> +}
@@ -738,6 +738,33 @@ static int sun6i_csi_capture_enum_fmt(struct file *file, void *priv,
return 0;
}
+static int sun6i_csi_capture_enum_framesize(struct file *file, void *fh,
+ struct v4l2_frmsizeenum *fsize)
+{
+ if (fsize->index)
+ return -EINVAL;
+
+ /* Only accept format in map table. */
+ if (!sun6i_csi_capture_format_find(fsize->pixel_format))
+ return -EINVAL;
+
+ fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
+ fsize->stepwise.min_width = SUN6I_CSI_CAPTURE_WIDTH_MIN;
+ fsize->stepwise.max_width = SUN6I_CSI_CAPTURE_WIDTH_MAX;
+ fsize->stepwise.min_height = SUN6I_CSI_CAPTURE_HEIGHT_MIN;
+ fsize->stepwise.max_height = SUN6I_CSI_CAPTURE_HEIGHT_MAX;
+
+ if (fsize->pixel_format == V4L2_PIX_FMT_NV12_16L16) {
+ fsize->stepwise.step_width = 16;
+ fsize->stepwise.step_height = 16;
+ } else {
+ fsize->stepwise.step_width = 2;
+ fsize->stepwise.step_height = 2;
+ }
+
+ return 0;
+}
+
static int sun6i_csi_capture_g_fmt(struct file *file, void *priv,
struct v4l2_format *format)
{
@@ -805,6 +832,7 @@ static const struct v4l2_ioctl_ops sun6i_csi_capture_ioctl_ops = {
.vidioc_querycap = sun6i_csi_capture_querycap,
.vidioc_enum_fmt_vid_cap = sun6i_csi_capture_enum_fmt,
+ .vidioc_enum_framesizes = sun6i_csi_capture_enum_framesize,
.vidioc_g_fmt_vid_cap = sun6i_csi_capture_g_fmt,
.vidioc_s_fmt_vid_cap = sun6i_csi_capture_s_fmt,
.vidioc_try_fmt_vid_cap = sun6i_csi_capture_try_fmt,