Re: [PATCH v8 11/16] media: qcom: camss: csid: Only add TPG v4l2 ctrl if TPG hardware is available

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 08/01/2025 14:37, Depeng Shao wrote:
> There is no CSID TPG on some SoCs, so the v4l2 ctrl in CSID driver
> shouldn't be registered. Checking the supported TPG modes to indicate
> if the TPG hardware exists or not and only registering v4l2 ctrl for
> CSID only when the TPG hardware is present.
> 
> Signed-off-by: Depeng Shao <quic_depengs@quicinc.com>
> ---
>   .../media/platform/qcom/camss/camss-csid.c    | 60 +++++++++++--------
>   1 file changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 6cf8e434dc05..e26a69a454a7 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -760,11 +760,13 @@ static int csid_set_stream(struct v4l2_subdev *sd, int enable)
>   	int ret;
>   
>   	if (enable) {
> -		ret = v4l2_ctrl_handler_setup(&csid->ctrls);
> -		if (ret < 0) {
> -			dev_err(csid->camss->dev,
> -				"could not sync v4l2 controls: %d\n", ret);
> -			return ret;
> +		if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) {
> +			ret = v4l2_ctrl_handler_setup(&csid->ctrls);
> +			if (ret < 0) {
> +				dev_err(csid->camss->dev,
> +					"could not sync v4l2 controls: %d\n", ret);
> +				return ret;
> +			}
>   		}
>   
>   		if (!csid->testgen.enabled &&
> @@ -838,7 +840,8 @@ static void csid_try_format(struct csid_device *csid,
>   		break;
>   
>   	case MSM_CSID_PAD_SRC:
> -		if (csid->testgen_mode->cur.val == 0) {
> +		if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED ||
> +		    csid->testgen_mode->cur.val == 0) {
>   			/* Test generator is disabled, */
>   			/* keep pad formats in sync */
>   			u32 code = fmt->code;
> @@ -888,7 +891,8 @@ static int csid_enum_mbus_code(struct v4l2_subdev *sd,
>   
>   		code->code = csid->res->formats->formats[code->index].code;
>   	} else {
> -		if (csid->testgen_mode->cur.val == 0) {
> +		if (csid->testgen.nmodes == CSID_PAYLOAD_MODE_DISABLED ||
> +		    csid->testgen_mode->cur.val == 0) {
>   			struct v4l2_mbus_framefmt *sink_fmt;
>   
>   			sink_fmt = __csid_get_format(csid, sd_state,
> @@ -1267,7 +1271,8 @@ static int csid_link_setup(struct media_entity *entity,
>   
>   		/* If test generator is enabled */
>   		/* do not allow a link from CSIPHY to CSID */
> -		if (csid->testgen_mode->cur.val != 0)
> +		if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED &&
> +		    csid->testgen_mode->cur.val != 0)
>   			return -EBUSY;
>   
>   		sd = media_entity_to_v4l2_subdev(remote->entity);
> @@ -1360,24 +1365,27 @@ int msm_csid_register_entity(struct csid_device *csid,
>   		 MSM_CSID_NAME, csid->id);
>   	v4l2_set_subdevdata(sd, csid);
>   
> -	ret = v4l2_ctrl_handler_init(&csid->ctrls, 1);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to init ctrl handler: %d\n", ret);
> -		return ret;
> -	}
> +	if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED) {
> +		ret = v4l2_ctrl_handler_init(&csid->ctrls, 1);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to init ctrl handler: %d\n", ret);
> +			return ret;
> +		}
>   
> -	csid->testgen_mode = v4l2_ctrl_new_std_menu_items(&csid->ctrls,
> -				&csid_ctrl_ops, V4L2_CID_TEST_PATTERN,
> -				csid->testgen.nmodes, 0, 0,
> -				csid->testgen.modes);
> +		csid->testgen_mode =
> +			v4l2_ctrl_new_std_menu_items(&csid->ctrls,
> +						     &csid_ctrl_ops, V4L2_CID_TEST_PATTERN,
> +						     csid->testgen.nmodes, 0, 0,
> +						     csid->testgen.modes);
>   
> -	if (csid->ctrls.error) {
> -		dev_err(dev, "Failed to init ctrl: %d\n", csid->ctrls.error);
> -		ret = csid->ctrls.error;
> -		goto free_ctrl;
> -	}
> +		if (csid->ctrls.error) {
> +			dev_err(dev, "Failed to init ctrl: %d\n", csid->ctrls.error);
> +			ret = csid->ctrls.error;
> +			goto free_ctrl;
> +		}
>   
> -	csid->subdev.ctrl_handler = &csid->ctrls;
> +		csid->subdev.ctrl_handler = &csid->ctrls;
> +	}
>   
>   	ret = csid_init_formats(sd, NULL);
>   	if (ret < 0) {
> @@ -1408,7 +1416,8 @@ int msm_csid_register_entity(struct csid_device *csid,
>   media_cleanup:
>   	media_entity_cleanup(&sd->entity);
>   free_ctrl:
> -	v4l2_ctrl_handler_free(&csid->ctrls);
> +	if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED)
> +		v4l2_ctrl_handler_free(&csid->ctrls);
>   
>   	return ret;
>   }
> @@ -1421,7 +1430,8 @@ void msm_csid_unregister_entity(struct csid_device *csid)
>   {
>   	v4l2_device_unregister_subdev(&csid->subdev);
>   	media_entity_cleanup(&csid->subdev.entity);
> -	v4l2_ctrl_handler_free(&csid->ctrls);
> +	if (csid->testgen.nmodes != CSID_PAYLOAD_MODE_DISABLED)
> +		v4l2_ctrl_handler_free(&csid->ctrls);
>   }
>   
>   inline bool csid_is_lite(struct csid_device *csid)

The TPG on the RB5 has a known bug that not all test patterns work. I 
verified that the coloured box TPG still works after this change.

Like so:

# colour bars test pattern 9
media-ctl --reset
yavta --no-query -w '0x009f0903 9' /dev/v4l-subdev6
yavta --list /dev/v4l-subdev6
media-ctl -d /dev/media0 -V '"msm_csid0":0[fmt:SGRBG10_1X10/3280x2464]'
media-ctl -d /dev/media0 -V '"msm_vfe0_rdi0":0[fmt:SGRBG10_1X10/3280x2464]'
media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
media-ctl -d /dev/media0 -p
yavta -B capture-mplane --capture=5 -n 5 -I -f SGRBG10P -s 3280x2464 
--file=TPG-SGRBG10-3280x2464-000-#.bin /dev/video0

I think we had some confusion about the TPG regressing on v6/v7 of this 
patch but, I suspect the wrong test pattern was tested.

This works as expected for me.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # qrb5165 rb5