Re: [PATCH v4 6/6] media: qcom: camss: Add support for Hawi

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] media: qcom: camss: unconditional AUP strobes in
  csid_configure_stream()
- [Medium] media: qcom: camss: race on csid->addr_update and
  csid->reg_update
- [Medium] media: qcom: camss: wrong camnoc_rt_axi clock rates in
  vfe_res_hawi
- [Low] media: qcom: camss: duplicate CSID/VFE 1080 code and inaccurate
  changelog
- [Low] media: qcom: camss: commit message mismatch with hawi_resources
--

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

media: qcom: camss: Add support for Hawi

This commit adds CAMSS support for the Qualcomm Hawi SoC, including CSID
1190, VFE 1190, and the Hawi subdevice resource tables.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1190.c b/drivers/media/platform/qcom/camss/camss-csid-1190.c
> new file mode 100644
> index 0000000000000..4bd2cce094402
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1190.c

[ ... ]

> +static void __csid_aup_rup_trigger(struct csid_device *csid)
> +{
> +	/* trigger SET in combined register */
> +	writel(RUP_SET, csid->base + CSID_RUP_AUP_CMD);
> +}
> +
> +static void __csid_addr_update(struct csid_device *csid, int port_id)
> +{
> +	csid->addr_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->addr_update, csid->base + CSID_AUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_addr_update_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	csid->addr_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
> +
> +static void __csid_reg_update(struct csid_device *csid, int port_id)
> +{
> +	csid->reg_update |= CSID_RUP_AUP_RDI(port_id);
> +	writel(csid->reg_update, csid->base + CSID_RUP_CMD);
> +
> +	__csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_reg_update_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	csid->reg_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}

[Severity: Medium]
Can concurrent updates to csid->addr_update and csid->reg_update race with
csid_isr()?

vfe_queue_buffer_v2() and vfe_enable_output_v2() call ops->addr_update() and
ops->reg_update() under vfe->output_lock, whereas csid_isr() clears those
bitmasks on INFO_RUP_DONE without holding vfe->output_lock or a CSID
spinlock, and csid_configure_stream() sets them without a spinlock:

CPU0                                  CPU1 (<hardirq>)
vfe_queue_buffer_v2()                 csid_isr()
  spin_lock_irqsave(&vfe->output_lock)
  ...                                   __csid_addr_update_clear(csid, i)
  __csid_addr_update(csid, port_id)       load csid->addr_update
    load csid->addr_update                and ~CSID_RUP_AUP_RDI(i)
    or CSID_RUP_AUP_RDI(port_id)          store csid->addr_update
    store csid->addr_update  <-- clobbers clear (or loses set bit)
    writel(csid->addr_update, ...)

This data race can drop a pending RDI address or register update or
re-write a stale RDI bit to CSID_AUP_CMD and CSID_RUP_CMD.

[ ... ]

> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
> +{
> +	u8 i, k;
> +
> +	__csid_configure_rx(csid, &csid->phy);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_1190; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			__csid_configure_rdi_stream(csid, enable, i, 0);
> +			__csid_configure_rx_vc(csid, 0);
> +
> +			for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
> +				__csid_addr_update(csid, i);
> +
> +			__csid_reg_update(csid, i);
> +
> +			__csid_ctrl_rdi(csid, enable, i);
> +		}
> +	}
> +}

[Severity: High]
Does unconditionally issuing CAMSS_INIT_BUF_COUNT address updates here
desynchronize the hardware write master when fewer than two buffers are
queued before VIDIOC_STREAMON?

Because vb2_queue.min_queued_buffers is 0 in msm_video_register(),
vfe_enable_output_v2() only programs VFE_BUS_WM_IMAGE_ADDR for as many
pending buffers as are actually queued (0, 1, or 2):

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() then runs and triggers two __csid_addr_update()
strobes even if 0 or 1 buffer was written to VFE_BUS_WM_IMAGE_ADDR (and also
when enable is 0 or on other link-enabled RDI ports in csid->phy.en_vc).
Latching uninitialized or duplicate buffer addresses into the hardware FIFO
and then issuing further addr_update strobes from vfe_queue_buffer_v2() when
buffers arrive while active_num < 2 corrupts buffer tracking and DMA target
addresses.

[ ... ]

> +const struct csid_hw_ops csid_ops_1190 = {
> +	.configure_stream = csid_configure_stream,
> +	.configure_testgen_pattern = csid_configure_testgen_pattern,
> +	.hw_version = csid_hw_version,
> +	.isr = csid_isr,
> +	.reset = csid_reset,
> +	.src_pad_code = csid_src_pad_code,
> +	.subdev_init = csid_subdev_init,
> +	.reg_update = csid_subdev_reg_update,
> +	.addr_update = csid_subdev_addr_update,
> +};

[Severity: Low]
This isn't a bug, but camss-csid-1190.c is register-for-register and
functionally identical to camss-csid-1080.c, including the unused local
variable lane_cnt in __csid_configure_rdi_stream().  Could csid_res_hawi
reuse csid_ops_1080 directly instead of duplicating the file, and could
camss-vfe-1190.c share the identical helper functions with camss-vfe-1080.c?

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 7976796dca598..70d4306581a23 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -4308,6 +4308,218 @@ static const struct resources_icc icc_res_sa8775p[] = {
>  	},
>  };
>  
> +static const struct camss_subdev_resources vfe_res_hawi[] = {
> +	/* VFE0 - TFE Full */
> +	{
> +		.clock = { "vfe0_fast_ahb", "vfe0", "cpas_vfe0",
> +			   "camnoc_rt_axi", "camnoc_nrt_axi", "qdss_debug_xo",
> +			   "cpas_mnoc_hf", "cpas_mnoc_sf" },
> +		.clock_rate = { { 0 },
> +				{ 238200000, 397000000, 440000000, 480000000,
> +				  630000000, 750000000, 833000000 },
> +				{ 0 },
> +				{ 171428571, 240000000, 300000000, 400000000,
> +				  480000000, 600000000 },

[Severity: Medium]
Do the camnoc_rt_axi frequencies in vfe_res_hawi (VFE0 through VFE4) match
the Hawi CAMCC clock driver?

In drivers/clk/qcom/camcc-hawi.c, ftbl_cam_cc_camnoc_rt_axi_clk_src defines
the supported rates on Hawi as 19.2 MHz, 120 MHz, 160 MHz, 240 MHz, 320 MHz,
and a maximum of 480 MHz:

drivers/clk/qcom/camcc-hawi.c:
static const struct freq_tbl ftbl_cam_cc_camnoc_rt_axi_clk_src[] = {
	F(19200000, P_BI_TCXO, 1, 0, 0),
	F(120000000, P_CAM_CC_PLL6_OUT_EVEN, 4, 0, 0),
	F(160000000, P_CAM_CC_PLL6_OUT_EVEN, 3, 0, 0),
	F(240000000, P_CAM_CC_PLL6_OUT_EVEN, 2, 0, 0),
	F(320000000, P_CAM_CC_PLL6_OUT_EVEN, 1.5, 0, 0),
	F(480000000, P_CAM_CC_PLL6_OUT_EVEN, 1, 0, 0),
	{ }
};

When vfe_set_clock_rates() selects 600 MHz for a min_rate between 480 MHz
and 600 MHz, clk_round_rate() clamps the rate down to 480 MHz, causing a
subsequent vfe_check_clock_rates() call on the same VFE to fail with -EBUSY
because clk_get_rate() is below min_rate.

[ ... ]

> @@ -6030,6 +6242,21 @@ static const struct camss_resources sm8650_resources = {
>  	.vfe_num = ARRAY_SIZE(vfe_res_sm8650),
>  };
>  
> +static const struct camss_resources hawi_resources = {
> +	.version = CAMSS_HAWI,
> +	.pd_name = "top",
> +	.pm_clks = { "cpas_fast_ahb", "cpas_ahb", "gcc_axi_hf" },
> +	.tpg_res = tpg_res_8775p,
> +	.csid_res = csid_res_hawi,
> +	.vfe_res = vfe_res_hawi,
> +	.icc_res = icc_res_hawi,
> +	.icc_path_num = ARRAY_SIZE(icc_res_hawi),
> +	.csiphy_num = 6,
> +	.tpg_num = ARRAY_SIZE(tpg_res_8775p),
> +	.csid_num = ARRAY_SIZE(csid_res_hawi),
> +	.vfe_num = ARRAY_SIZE(vfe_res_hawi),
> +};

[Severity: Low]
Does the commit message match hawi_resources?

The changelog states that the CSIPHY resource table is reused from Kaanapali,
the TPG table is reused from X1E80100, and the common CPAS clocks are managed
as PM clocks rather than per-VFE clocks.  However, hawi_resources sets
.tpg_res to tpg_res_8775p (not tpg_res_x1e80100), has no .csiphy_res table,
and vfe_res_hawi still lists the shared cpas_mnoc_hf and cpas_mnoc_sf clocks
in every VFE entry rather than in .pm_clks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-hawi-maili-camss-v4-0-d528c7a797b4@oss.qualcomm.com?part=6