Re: [PATCH v25 10/15] media: qcom: camss: vfe: Add support for VFE 1080

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] media: qcom: camss: unsynchronized CSID RUP/AUP updates in
  vfe_ops_1080
- [Medium] media: qcom: camss: AUP_UPDATE count mismatch in
  vfe_enable_output_v2()
--

commit c95e0ff891b19f6115d3c4df041126b33e4f1889
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: vfe: Add support for VFE 1080

This commit adds hardware ops and resource definitions to support VFE
(TFE) 1080 on the Qualcomm Kaanapali SoC.

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-1080.c b/drivers/media/platform/qcom/camss/camss-vfe-1080.c
> new file mode 100644
> index 0000000000000..68798f13fbea8
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-1080.c

[ ... ]

> +static void vfe_reg_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_reg_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static void vfe_reg_update_clear(struct vfe_device *vfe,
> +				 enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_reg_update(vfe->camss, vfe->id, port_id, true);
> +}
> +
> +static void vfe_addr_update(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_addr_update(vfe->camss, vfe->id, port_id, false);
> +}
> +
> +static void vfe_addr_update_clear(struct vfe_device *vfe, enum vfe_line_id line_id)
> +{
> +	int port_id = line_id;
> +
> +	camss_addr_update(vfe->camss, vfe->id, port_id, true);
> +}

[Severity: High]
Can vfe_addr_update() and vfe_reg_update() race with csid_isr() on
csid->addr_update and csid->reg_update?

vfe_queue_buffer_v2() calls ops->addr_update() and ops->reg_update() under
vfe->output_lock, which forward to __csid_addr_update() and
__csid_reg_update().  Meanwhile, in camss-csid-1080.c, csid_isr() clears
bits in csid->addr_update and csid->reg_update before calling
camss_buf_done() (where vfe->output_lock is acquired):

CPU0                                  CPU1 (<hardirq>)
vfe_queue_buffer_v2()                 csid_isr()
  spin_lock_irqsave(&vfe->output_lock)
  vfe_addr_update()
    ...
      csid->addr_update |= BIT(...)
                                        if (val & INFO_RUP_DONE)
                                          __csid_addr_update_clear()
                                            csid->addr_update &= ~BIT(...)
      writel(csid->addr_update,
             csid->base + CSID_AUP_CMD)  <-- writes 0, losing update
      __csid_aup_rup_trigger(csid)

Because csid_reset() only unmasks INFO_RST_DONE | BUF_DONE_IRQ_STATUS in
CSID_TOP_IRQ_MASK, INFO_RUP_DONE stays latched until csid_isr() runs on
BUF_DONE_IRQ_STATUS at the end of the frame.  If userspace queues a buffer
when output->gen2.active_num == 1, the lost CSID_AUP_CMD or CSID_RUP_CMD
write leaves the hardware without the queued buffer address and stalls
capture.  Multiple active RDI lines on the same CSID can also clobber each
other's csid->addr_update and csid->reg_update bits.

[ ... ]

> +const struct vfe_hw_ops vfe_ops_1080 = {
> +	.addr_update = vfe_addr_update,
> +	.addr_update_clear = vfe_addr_update_clear,

[Severity: Medium]
Can starting a stream with fewer than two queued buffers mismatch the
number of AUP_UPDATE requests against the buffers written to the write
master?

In camss-video.c, msm_video_register() leaves q->min_queued_buffers at 0,
so VIDIOC_STREAMON can run vfe_enable_output_v2() with 0 or 1 buffer
pending:

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_enable_output_v2() {
    ...
	for (i = 0; i < CAMSS_INIT_BUF_COUNT; i++) {
		output->buf[i] = vfe_buf_get_pending(output);
		if (!output->buf[i])
			break;
		output->gen2.active_num++;
		ops->vfe_wm_update(vfe, output->wm_idx[0],
				   output->buf[i]->addr[0], line);
		if (!ops->addr_update)
			ops->reg_update(vfe, line->id);
	}
    ...
}

csid_configure_stream() in camss-csid-1080.c then unconditionally issues
CAMSS_INIT_BUF_COUNT (2) __csid_addr_update() calls even if 0 or 1 buffer
address was programmed into VFE_BUS_WM_IMAGE_ADDR.  When userspace later
queues buffers while output->gen2.active_num < 2, vfe_queue_buffer_v2()
calls ops->addr_update() again for each buffer, issuing 3 or 4 AUP_UPDATE
requests for the first two buffers and violating the requirement that
AUP_UPDATE requests match the number of buffers queued to the write
master.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-kaanapali-camss-v25-0-1d709f57f370@oss.qualcomm.com?part=10

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy