Re: [PATCH v11 7/9] arm64: dts: qcom: agatti: Add OPE node

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: qcom: camss: premature job scheduling in ope_job_ready()
--

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

arm64: dts: qcom: agatti: Add OPE node

This commit adds and enables the Offline Processing Engine (OPE) device tree
node for the QCM2290 (Agatti) platform, describing its register regions,
clocks, interrupt, interconnects, IOMMU mappings, power domains, and OPP
table.

> diff --git a/arch/arm64/boot/dts/qcom/agatti.dtsi b/arch/arm64/boot/dts/qcom/agatti.dtsi
> index 590bd2432d85b..69237959adc15 100644
> --- a/arch/arm64/boot/dts/qcom/agatti.dtsi
> +++ b/arch/arm64/boot/dts/qcom/agatti.dtsi
> @@ -1984,6 +1984,83 @@ port@1 {
>  			};
>  		};
>  
> +		isp_ope: isp@5c42000 {
> +			compatible = "qcom,qcm2290-camss-ope";

[Severity: High]
Enabling isp_ope exposes the qcom-camss-ope driver added in this series. Can
ope_job_ready() in drivers/media/platform/qcom/camss/ope/core.c schedule hardware
DMA before ope_start_streaming() has validated the pipeline on the capture
queue?

In drivers/media/platform/qcom/camss/ope/core.c, ope_job_ready() checks only
ctx->started (which is set when OPE_QUEUE_FRAME_IN starts streaming) and
whether the input and output rdy_queue lists are non-empty:

drivers/media/platform/qcom/camss/ope/core.c:ope_job_ready() {
	struct ope_ctx *ctx = priv;

	guard(spinlock_irqsave)(&ctx->queues_lock);

	return ctx->started &&
	       !list_empty(&ctx->queues[OPE_QUEUE_FRAME_IN].rdy_queue) &&
	       !list_empty(&ctx->queues[OPE_QUEUE_DISP_OUT].rdy_queue);
}

When OPE_QUEUE_FRAME_IN is already streaming with a queued input buffer and
userspace pre-queues a capture buffer on OPE_QUEUE_DISP_OUT before calling
VIDIOC_STREAMON, vb2_start_streaming() passes the pre-queued capture buffer to
ope_buf_queue() prior to calling ope_start_streaming():

vb2_start_streaming()
  -> ope_buf_queue()
       -> list_add_tail(&buf->list, &ctx->queues[idx].rdy_queue);
       -> ope_try_schedule(ctx)
            -> ope_job_ready(ctx) /* returns true */
            -> schedule_work(&ope->run_work)

Because ope_run_work() runs asynchronously without holding ctx->vbq_lock,
ope_run_job() can start hardware DMA into the capture buffer before
ope_start_streaming() on OPE_QUEUE_DISP_OUT runs
video_device_pipeline_alloc_start() (including ope_disp_output_link_validate())
or pm_runtime_resume_and_get().

If video_device_pipeline_alloc_start() or pm_runtime_resume_and_get() then
fails in ope_start_streaming(), the error path drains rdy_queue without
canceling the scheduled or running job:

drivers/media/platform/qcom/camss/ope/core.c:ope_start_streaming() {
...
err_pipeline_stop:
	video_device_pipeline_stop(vdev);
err_drain:
	ope_queue_drain(ctx, idx, VB2_BUF_STATE_QUEUED);
	return ret;
}

Wouldn't this return the capture buffer to VB2_BUF_STATE_QUEUED while the OPE
hardware is actively DMA-writing to it, or trigger
WARN_ON(!list_empty(&q->done_list)) in vb2_start_streaming() if
ope_job_finish() already completed the buffer?

Should ope_job_ready() require both OPE_QUEUE_FRAME_IN and OPE_QUEUE_DISP_OUT
to have completed ope_start_streaming() before returning true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-camss-isp-ope-v11-0-0c2937195298@oss.qualcomm.com?part=7