Re: [PATCH v7 5/9] media: Documentation: uapi: Add qcom-camss ISP params documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: uapi: qcom-isp redefines V4L2 ISP block enable semantics
- [Medium] media: uapi: qcom-isp code example uses uninitialized memory for ISP params
- [Medium] media: uapi: qcom-isp code example for gamma curve conflicts with header
--

commit e41b4aecb94fe466cf8c4b1792d1ce5d03151ede
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>

media: Documentation: uapi: Add qcom-camss ISP params documentation

Add documentation for the Qualcomm CAMSS ISP parameters metadata format
(V4L2_META_FMT_QCOM_ISP_PARAMS). This describes the OPE processing
pipeline and provides a userspace programming example.

> diff --git a/Documentation/userspace-api/media/v4l/metafmt-qcom-isp.rst b/Documentation/userspace-api/media/v4l/metafmt-qcom-isp.rst
> new file mode 100644
> index 0000000000000..da3dd5f3a5899
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/metafmt-qcom-isp.rst
> @@ -0,0 +1,145 @@
[ ... ]
> +The set of supported blocks depends on the CAMSS engine consuming the buffer.
> +Currently the Offline Processing Engine (OPE) is the only engine defining
> +parameter blocks, exposed through its ``ope_params`` metadata output video
> +node; additional engines and blocks may be added to this format in the future.
> +
> +Blocks whose header does not carry V4L2_ISP_PARAMS_FL_BLOCK_ENABLE leave the
> +corresponding hardware module bypassed. Blocks omitted from a buffer keep
> +their previously programmed configuration.

[Severity: High]
Does this redefine the standard V4L2 ISP block enable semantics? 

If userspace follows the standard V4L2 ISP UAPI to update parameters by 
omitting ENABLE/DISABLE flags for an active block, wouldn't this cause the
QCOM CAMSS module to be accidentally bypassed?

[ ... ]
> +The input node accepts 8-bit and 10-bit packed Bayer RAW formats (for example
> +``V4L2_PIX_FMT_SRGGB8`` and ``V4L2_PIX_FMT_SRGGB10P``). The output node
> +produces semi-planar YUV (``V4L2_PIX_FMT_NV12`` / ``NV21`` / ``NV16`` /
> +``NV61`` / ``NV24`` / ``NV42``) or luma-only ``V4L2_PIX_FMT_GREY``. Each
> +configurable stage is bypassed unless its block carries
> +``V4L2_ISP_PARAMS_FL_BLOCK_ENABLE``.

[Severity: High]
Similar to the above, does this documentation break ABI compatibility with
generic V4L2 ISP userspace tools? 

If a generic implementation writes to the metadata node to update a block
without the ENABLE flag set, this rule suggests the module gets disabled
instead of just updated.

> +
> +The following example populates an OPE parameters buffer with a white balance
> +and a gamma correction block:
> +
> +.. code-block:: c
> +
> +	struct v4l2_isp_params_buffer *params =
> +		(struct v4l2_isp_params_buffer *)buffer;
> +
> +	params->version = V4L2_ISP_PARAMS_VERSION_V1;
> +	params->data_size = 0;
> +
> +	void *data = (void *)params->data;
> +
> +	struct camss_params_ope_wb_gain *wb =
> +		(struct camss_params_ope_wb_gain *)data;
> +
> +	wb->header.type = CAMSS_PARAMS_OPE_WB_GAIN;
> +	wb->header.flags |= V4L2_ISP_PARAMS_FL_BLOCK_ENABLE;
> +	wb->header.size = sizeof(struct camss_params_ope_wb_gain);

[Severity: Medium]
Does this code example use uninitialized memory for the ISP parameters?

Since the buffer data memory isn't initialized, applying a bitwise-OR to
wb->header.flags could accidentally include the DISABLE flag or leave
reserved fields undefined. If a developer copies this into a userspace
application, could it cause silent data corruption or an -EINVAL IOCTL
failure?

[ ... ]
> +	struct camss_params_ope_gamma *gamma =
> +		(struct camss_params_ope_gamma *)data;
> +
> +	gamma->header.type = CAMSS_PARAMS_OPE_GAMMA;
> +	gamma->header.flags |= V4L2_ISP_PARAMS_FL_BLOCK_ENABLE;

[Severity: Medium]
This seems to repeat the uninitialized memory issue for the gamma block.

> +	gamma->header.size = sizeof(struct camss_params_ope_gamma);
> +
> +	/* Identity curve (pass-through, gamma 1.0) */
> +	for (unsigned int i = 0; i < CAMSS_OPE_GAMMA_LUT_SIZE; i++)
> +		gamma->glut[i] = gamma->blut[i] = gamma->rlut[i] = 257 * i;

[Severity: Medium]
Does this calculation conflict with the UAPI header definition for the
gamma curve?

The calculation 257 * i distributes the 8-bit index into both the high and
low byte. If a future or different ISP implementation uses a 10-bit or
12-bit bus and masks the LSBs, the upper bits of the 8-bit value will spill
into the active LSBs. For example, 127 * 257 = 0x7F7F, which masked to 10
bits is 895 instead of 511. 

If the UAPI header explicitly dictates lut[i] = i for identity, shouldn't
the example strictly follow the 0-255 scaling to avoid a distorted curve?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915-camss-isp-ope-v7-0-77b13d131d3d@oss.qualcomm.com?part=5