[PATCH v2 0/9] media: qcom: camss: Bugfix series

[PATCH v2 0/9] media: qcom: camss: Bugfix series

[Bryan O'Donoghue]

V2:
- Amends commit log for TPG fix to cover dropping of fixed
  VC when setting up a TPG - Konrad

- Leaves GENMASK etc out. I'm happy to do a "make it pretty"
  series later on. - bod

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commits/Bugfix-series-v2?ref_type=tags

V1:
- Drops dt_id = vc * 4 in favour of a patch in a later series - Hans
  Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/commit/d4c382c5d6ee153b410a01e172b3e811011d0b14
- Adds Konrad's Acked-by as indicated

V0:
This series covers a number of Fixes: all of which are for application to
stable as well as -next with the exception of the second patch which is a
fix for a SHA that is still in -next.

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-08-07-db410c-rb3-camss-dts-v3

This series is part of a larger set of fixes, improvements developed/found
when adding a new SoC.

Link: https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/lenovo-x13s-v6.5-rc4-x13s-camss-patches

First pass on that larger series is to get all of the current Fixes: in the
branch out.

Andrey Konovalov (1):
  media: qcom: camss: Fix csid-gen2 for test pattern generator

Bryan O'Donoghue (8):
  media: qcom: camss: Fix pm_domain_on sequence in probe
  media: qcom: camss: Fix V4L2 async notifier error path
  media: qcom: camss: Fix vfe_get() error jump
  media: qcom: camss: Fix VFE-17x vfe_disable_output()
  media: qcom: camss: Fix VFE-480 vfe_disable_output()
  media: qcom: camss: Fix missing vfe_lite clocks check
  media: qcom: camss: Fix invalid clock enable bit disjunction
  media: qcom: camss: Fix set CSI2_RX_CFG1_VC_MODE when VC is greater
    than 3

 .../platform/qcom/camss/camss-csid-gen2.c     | 11 ++++----
 .../qcom/camss/camss-csiphy-3ph-1-0.c         |  2 +-
 .../media/platform/qcom/camss/camss-vfe-170.c | 19 +++-----------
 .../media/platform/qcom/camss/camss-vfe-480.c | 19 +++-----------
 drivers/media/platform/qcom/camss/camss-vfe.c |  5 ++--
 drivers/media/platform/qcom/camss/camss.c     | 26 +++++++++----------
 6 files changed, 28 insertions(+), 54 deletions(-)

-- 
2.41.0

[PATCH v2 1/9] media: qcom: camss: Fix pm_domain_on sequence in probe

[Bryan O'Donoghue]

We need to make sure camss_configure_pd() happens before
camss_register_entities() as the vfe_get() path relies on the pointer
provided by camss_configure_pd().

Fix the ordering sequence in probe to ensure the pointers vfe_get() demands
are present by the time camss_register_entities() runs.

In order to facilitate backporting to stable kernels I've moved the
configure_pd() call pretty early on the probe() function so that
irrespective of the existence of the old error handling jump labels this
patch should still apply to -next circa Aug 2023 to v5.13 inclusive.

Fixes: 2f6f8af67203 ("media: camss: Refactor VFE power domain toggling")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index f11dc59135a5a..75991d849b571 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1619,6 +1619,12 @@ static int camss_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto err_cleanup;
 
+	ret = camss_configure_pd(camss);
+	if (ret < 0) {
+		dev_err(dev, "Failed to configure power domains: %d\n", ret);
+		goto err_cleanup;
+	}
+
 	ret = camss_init_subdevices(camss);
 	if (ret < 0)
 		goto err_cleanup;
@@ -1678,12 +1684,6 @@ static int camss_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = camss_configure_pd(camss);
-	if (ret < 0) {
-		dev_err(dev, "Failed to configure power domains: %d\n", ret);
-		return ret;
-	}
-
 	pm_runtime_enable(dev);
 
 	return 0;
-- 
2.41.0

Re: [PATCH v2 1/9] media: qcom: camss: Fix pm_domain_on sequence in probe

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:18PM +0100, Bryan O'Donoghue wrote:
> We need to make sure camss_configure_pd() happens before
> camss_register_entities() as the vfe_get() path relies on the pointer
> provided by camss_configure_pd().
> 
> Fix the ordering sequence in probe to ensure the pointers vfe_get() demands
> are present by the time camss_register_entities() runs.
> 
> In order to facilitate backporting to stable kernels I've moved the
> configure_pd() call pretty early on the probe() function so that
> irrespective of the existence of the old error handling jump labels this
> patch should still apply to -next circa Aug 2023 to v5.13 inclusive.
> 
> Fixes: 2f6f8af67203 ("media: camss: Refactor VFE power domain toggling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

It seems like the device links and power domains won't be properly
cleaned up if probe fails. The problem predates this patch though, so
even if moving genpd initialization may make it worse, it's not a reason
to block this patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Maybe a patch further in the series will fix this :-)

> ---
>  drivers/media/platform/qcom/camss/camss.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index f11dc59135a5a..75991d849b571 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1619,6 +1619,12 @@ static int camss_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto err_cleanup;
>  
> +	ret = camss_configure_pd(camss);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to configure power domains: %d\n", ret);
> +		goto err_cleanup;
> +	}
> +
>  	ret = camss_init_subdevices(camss);
>  	if (ret < 0)
>  		goto err_cleanup;
> @@ -1678,12 +1684,6 @@ static int camss_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	ret = camss_configure_pd(camss);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to configure power domains: %d\n", ret);
> -		return ret;
> -	}
> -
>  	pm_runtime_enable(dev);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/9] media: qcom: camss: Fix V4L2 async notifier error path

[Bryan O'Donoghue]

Previously the jump label err_cleanup was used higher in the probe()
function to release the async notifier however the async notifier
registration was moved later in the code rendering the previous four jumps
redundant.

Rename the label from err_cleanup to err_v4l2_device_register to capture
what the jump does.

Fixes: 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier later")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
index 75991d849b571..f4948bdf3f8f9 100644
--- a/drivers/media/platform/qcom/camss/camss.c
+++ b/drivers/media/platform/qcom/camss/camss.c
@@ -1617,21 +1617,21 @@ static int camss_probe(struct platform_device *pdev)
 
 	ret = camss_icc_get(camss);
 	if (ret < 0)
-		goto err_cleanup;
+		return ret;
 
 	ret = camss_configure_pd(camss);
 	if (ret < 0) {
 		dev_err(dev, "Failed to configure power domains: %d\n", ret);
-		goto err_cleanup;
+		return ret;
 	}
 
 	ret = camss_init_subdevices(camss);
 	if (ret < 0)
-		goto err_cleanup;
+		return ret;
 
 	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
 	if (ret)
-		goto err_cleanup;
+		return ret;
 
 	camss->media_dev.dev = camss->dev;
 	strscpy(camss->media_dev.model, "Qualcomm Camera Subsystem",
@@ -1643,7 +1643,7 @@ static int camss_probe(struct platform_device *pdev)
 	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
 	if (ret < 0) {
 		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
-		goto err_cleanup;
+		return ret;
 	}
 
 	v4l2_async_nf_init(&camss->notifier, &camss->v4l2_dev);
@@ -1651,12 +1651,12 @@ static int camss_probe(struct platform_device *pdev)
 	num_subdevs = camss_of_parse_ports(camss);
 	if (num_subdevs < 0) {
 		ret = num_subdevs;
-		goto err_cleanup;
+		goto err_v4l2_device_register;
 	}
 
 	ret = camss_register_entities(camss);
 	if (ret < 0)
-		goto err_cleanup;
+		goto err_v4l2_device_register;
 
 	if (num_subdevs) {
 		camss->notifier.ops = &camss_subdev_notifier_ops;
@@ -1690,7 +1690,7 @@ static int camss_probe(struct platform_device *pdev)
 
 err_register_subdevs:
 	camss_unregister_entities(camss);
-err_cleanup:
+err_v4l2_device_register:
 	v4l2_device_unregister(&camss->v4l2_dev);
 	v4l2_async_nf_cleanup(&camss->notifier);
 
-- 
2.41.0

Re: [PATCH v2 2/9] media: qcom: camss: Fix V4L2 async notifier error path

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:19PM +0100, Bryan O'Donoghue wrote:
> Previously the jump label err_cleanup was used higher in the probe()
> function to release the async notifier however the async notifier
> registration was moved later in the code rendering the previous four jumps
> redundant.
> 
> Rename the label from err_cleanup to err_v4l2_device_register to capture
> what the jump does.

Shouldn't it be named err_v4l2_device_unregister then ? As the
err_register_subdevs label should also be renamed err_unregister_subdevs
you could rename them all in a separate patch.

> Fixes: 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier later")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 75991d849b571..f4948bdf3f8f9 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -1617,21 +1617,21 @@ static int camss_probe(struct platform_device *pdev)
>  
>  	ret = camss_icc_get(camss);
>  	if (ret < 0)
> -		goto err_cleanup;
> +		return ret;
>  
>  	ret = camss_configure_pd(camss);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to configure power domains: %d\n", ret);
> -		goto err_cleanup;
> +		return ret;
>  	}
>  
>  	ret = camss_init_subdevices(camss);
>  	if (ret < 0)
> -		goto err_cleanup;
> +		return ret;
>  
>  	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
>  	if (ret)
> -		goto err_cleanup;
> +		return ret;

This doesn't seem right, you should call v4l2_async_nf_cleanup() when
v4l2_async_nf_init() has been called, and that's done before
camss_icc_get().

>  
>  	camss->media_dev.dev = camss->dev;
>  	strscpy(camss->media_dev.model, "Qualcomm Camera Subsystem",
> @@ -1643,7 +1643,7 @@ static int camss_probe(struct platform_device *pdev)
>  	ret = v4l2_device_register(camss->dev, &camss->v4l2_dev);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to register V4L2 device: %d\n", ret);
> -		goto err_cleanup;
> +		return ret;
>  	}
>  
>  	v4l2_async_nf_init(&camss->notifier, &camss->v4l2_dev);
> @@ -1651,12 +1651,12 @@ static int camss_probe(struct platform_device *pdev)
>  	num_subdevs = camss_of_parse_ports(camss);
>  	if (num_subdevs < 0) {
>  		ret = num_subdevs;
> -		goto err_cleanup;
> +		goto err_v4l2_device_register;
>  	}
>  
>  	ret = camss_register_entities(camss);
>  	if (ret < 0)
> -		goto err_cleanup;
> +		goto err_v4l2_device_register;
>  
>  	if (num_subdevs) {
>  		camss->notifier.ops = &camss_subdev_notifier_ops;
> @@ -1690,7 +1690,7 @@ static int camss_probe(struct platform_device *pdev)
>  
>  err_register_subdevs:
>  	camss_unregister_entities(camss);
> -err_cleanup:
> +err_v4l2_device_register:
>  	v4l2_device_unregister(&camss->v4l2_dev);
>  	v4l2_async_nf_cleanup(&camss->notifier);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/9] media: qcom: camss: Fix V4L2 async notifier error path

[Bryan O'Donoghue]

On 28/08/2023 18:05, Laurent Pinchart wrote:
> Hi Bryan,
> 
> Thank you for the patch.
> 
> On Tue, Aug 22, 2023 at 09:06:19PM +0100, Bryan O'Donoghue wrote:
>> Previously the jump label err_cleanup was used higher in the probe()
>> function to release the async notifier however the async notifier
>> registration was moved later in the code rendering the previous four jumps
>> redundant.
>>
>> Rename the label from err_cleanup to err_v4l2_device_register to capture
>> what the jump does.
> 
> Shouldn't it be named err_v4l2_device_unregister then ? As the
> err_register_subdevs label should also be renamed err_unregister_subdevs
> you could rename them all in a separate patch.
> 
>> Fixes: 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier later")
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   drivers/media/platform/qcom/camss/camss.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
>> index 75991d849b571..f4948bdf3f8f9 100644
>> --- a/drivers/media/platform/qcom/camss/camss.c
>> +++ b/drivers/media/platform/qcom/camss/camss.c
>> @@ -1617,21 +1617,21 @@ static int camss_probe(struct platform_device *pdev)
>>   
>>   	ret = camss_icc_get(camss);
>>   	if (ret < 0)
>> -		goto err_cleanup;
>> +		return ret;
>>   
>>   	ret = camss_configure_pd(camss);
>>   	if (ret < 0) {
>>   		dev_err(dev, "Failed to configure power domains: %d\n", ret);
>> -		goto err_cleanup;
>> +		return ret;
>>   	}
>>   
>>   	ret = camss_init_subdevices(camss);
>>   	if (ret < 0)
>> -		goto err_cleanup;
>> +		return ret;
>>   
>>   	ret = dma_set_mask_and_coherent(dev, 0xffffffff);
>>   	if (ret)
>> -		goto err_cleanup;
>> +		return ret;
> 
> This doesn't seem right, you should call v4l2_async_nf_cleanup() when
> v4l2_async_nf_init() has been called, and that's done before
> camss_icc_get().

Ah no, after 51397a4ec75d ("media: qcom: Initialise V4L2 async notifier 
later") the behaviour changes.

-       v4l2_async_nf_init(&camss->notifier);
-
-       num_subdevs = camss_of_parse_ports(camss);
-       if (num_subdevs < 0) {
-               ret = num_subdevs;
-               goto err_cleanup;
-       }
-
         ret = camss_icc_get(camss);
         if (ret < 0)
                 goto err_cleanup;
@@ -1648,9 +1640,17 @@ static int camss_probe(struct platform_device *pdev)
                 goto err_cleanup;
         }

+       v4l2_async_nf_init(&camss->notifier);
+
+       num_subdevs = camss_of_parse_ports(camss);
+       if (num_subdevs < 0) {
+               ret = num_subdevs;
+               goto err_cleanup;
+       }


This patch is still a good opportunity to fix the jump labels for 
example genpd which you mentioned earlier.

---
bod

[PATCH v2 3/9] media: qcom: camss: Fix vfe_get() error jump

[Bryan O'Donoghue]

Right now it is possible to do a vfe_get() with the internal reference
count at 1. If vfe_check_clock_rates() returns non-zero then we will
leave the reference count as-is and

run:
- pm_runtime_put_sync()
- vfe->ops->pm_domain_off()

skip:
- camss_disable_clocks()

Subsequent vfe_put() calls will when the ref-count is non-zero
unconditionally run:

- pm_runtime_put_sync()
- vfe->ops->pm_domain_off()
- camss_disable_clocks()

vfe_get() should not attempt to roll-back on error when the ref-count is
non-zero as the upper layers will still do their own vfe_put() operations.

vfe_put() will drop the reference count and do the necessary power
domain release, the cleanup jumps in vfe_get() should only be run when
the ref-count is zero.

[   50.095796] CPU: 7 PID: 3075 Comm: cam Not tainted 6.3.2+ #80
[   50.095798] Hardware name: LENOVO 21BXCTO1WW/21BXCTO1WW, BIOS N3HET82W (1.54 ) 05/26/2023
[   50.095799] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   50.095802] pc : refcount_warn_saturate+0xf4/0x148
[   50.095804] lr : refcount_warn_saturate+0xf4/0x148
[   50.095805] sp : ffff80000c7cb8b0
[   50.095806] x29: ffff80000c7cb8b0 x28: ffff16ecc0e3fc10 x27: 0000000000000000
[   50.095810] x26: 0000000000000000 x25: 0000000000020802 x24: 0000000000000000
[   50.095813] x23: ffff16ecc7360640 x22: 00000000ffffffff x21: 0000000000000005
[   50.095815] x20: ffff16ed175f4400 x19: ffffb4d9852942a8 x18: ffffffffffffffff
[   50.095818] x17: ffffb4d9852d4a48 x16: ffffb4d983da5db8 x15: ffff80000c7cb320
[   50.095821] x14: 0000000000000001 x13: 2e656572662d7265 x12: 7466612d65737520
[   50.095823] x11: 00000000ffffefff x10: ffffb4d9850cebf0 x9 : ffffb4d9835cf954
[   50.095826] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000057fa8
[   50.095829] x5 : ffff16f813fe3d08 x4 : 0000000000000000 x3 : ffff621e8f4d2000
[   50.095832] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff16ed32119040
[   50.095835] Call trace:
[   50.095836]  refcount_warn_saturate+0xf4/0x148
[   50.095838]  device_link_put_kref+0x84/0xc8
[   50.095843]  device_link_del+0x38/0x58
[   50.095846]  vfe_pm_domain_off+0x3c/0x50 [qcom_camss]
[   50.095860]  vfe_put+0x114/0x140 [qcom_camss]
[   50.095869]  csid_set_power+0x2c8/0x408 [qcom_camss]
[   50.095878]  pipeline_pm_power_one+0x164/0x170 [videodev]
[   50.095896]  pipeline_pm_power+0xc4/0x110 [videodev]
[   50.095909]  v4l2_pipeline_pm_use+0x5c/0xa0 [videodev]
[   50.095923]  v4l2_pipeline_pm_get+0x1c/0x30 [videodev]
[   50.095937]  video_open+0x7c/0x100 [qcom_camss]
[   50.095945]  v4l2_open+0x84/0x130 [videodev]
[   50.095960]  chrdev_open+0xc8/0x250
[   50.095964]  do_dentry_open+0x1bc/0x498
[   50.095966]  vfs_open+0x34/0x40
[   50.095968]  path_openat+0xb44/0xf20
[   50.095971]  do_filp_open+0xa4/0x160
[   50.095974]  do_sys_openat2+0xc8/0x188
[   50.095975]  __arm64_sys_openat+0x6c/0xb8
[   50.095977]  invoke_syscall+0x50/0x128
[   50.095982]  el0_svc_common.constprop.0+0x4c/0x100
[   50.095985]  do_el0_svc+0x40/0xa8
[   50.095988]  el0_svc+0x2c/0x88
[   50.095991]  el0t_64_sync_handler+0xf4/0x120
[   50.095994]  el0t_64_sync+0x190/0x198
[   50.095996] ---[ end trace 0000000000000000 ]---

Fixes: 779096916dae ("media: camss: vfe: Fix runtime PM imbalance on error")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index dabfd613b2f94..938f373bcd1fd 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -611,7 +611,7 @@ int vfe_get(struct vfe_device *vfe)
 	} else {
 		ret = vfe_check_clock_rates(vfe);
 		if (ret < 0)
-			goto error_pm_runtime_get;
+			goto error_pm_domain;
 	}
 	vfe->power_count++;
 
-- 
2.41.0

Re: [PATCH v2 3/9] media: qcom: camss: Fix vfe_get() error jump

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:20PM +0100, Bryan O'Donoghue wrote:
> Right now it is possible to do a vfe_get() with the internal reference
> count at 1. If vfe_check_clock_rates() returns non-zero then we will
> leave the reference count as-is and
> 
> run:
> - pm_runtime_put_sync()
> - vfe->ops->pm_domain_off()
> 
> skip:
> - camss_disable_clocks()
> 
> Subsequent vfe_put() calls will when the ref-count is non-zero
> unconditionally run:
> 
> - pm_runtime_put_sync()
> - vfe->ops->pm_domain_off()
> - camss_disable_clocks()
> 
> vfe_get() should not attempt to roll-back on error when the ref-count is
> non-zero as the upper layers will still do their own vfe_put() operations.
> 
> vfe_put() will drop the reference count and do the necessary power
> domain release, the cleanup jumps in vfe_get() should only be run when
> the ref-count is zero.
> 
> [   50.095796] CPU: 7 PID: 3075 Comm: cam Not tainted 6.3.2+ #80
> [   50.095798] Hardware name: LENOVO 21BXCTO1WW/21BXCTO1WW, BIOS N3HET82W (1.54 ) 05/26/2023
> [   50.095799] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   50.095802] pc : refcount_warn_saturate+0xf4/0x148
> [   50.095804] lr : refcount_warn_saturate+0xf4/0x148
> [   50.095805] sp : ffff80000c7cb8b0
> [   50.095806] x29: ffff80000c7cb8b0 x28: ffff16ecc0e3fc10 x27: 0000000000000000
> [   50.095810] x26: 0000000000000000 x25: 0000000000020802 x24: 0000000000000000
> [   50.095813] x23: ffff16ecc7360640 x22: 00000000ffffffff x21: 0000000000000005
> [   50.095815] x20: ffff16ed175f4400 x19: ffffb4d9852942a8 x18: ffffffffffffffff
> [   50.095818] x17: ffffb4d9852d4a48 x16: ffffb4d983da5db8 x15: ffff80000c7cb320
> [   50.095821] x14: 0000000000000001 x13: 2e656572662d7265 x12: 7466612d65737520
> [   50.095823] x11: 00000000ffffefff x10: ffffb4d9850cebf0 x9 : ffffb4d9835cf954
> [   50.095826] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 0000000000057fa8
> [   50.095829] x5 : ffff16f813fe3d08 x4 : 0000000000000000 x3 : ffff621e8f4d2000
> [   50.095832] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff16ed32119040
> [   50.095835] Call trace:
> [   50.095836]  refcount_warn_saturate+0xf4/0x148
> [   50.095838]  device_link_put_kref+0x84/0xc8
> [   50.095843]  device_link_del+0x38/0x58
> [   50.095846]  vfe_pm_domain_off+0x3c/0x50 [qcom_camss]
> [   50.095860]  vfe_put+0x114/0x140 [qcom_camss]
> [   50.095869]  csid_set_power+0x2c8/0x408 [qcom_camss]
> [   50.095878]  pipeline_pm_power_one+0x164/0x170 [videodev]
> [   50.095896]  pipeline_pm_power+0xc4/0x110 [videodev]
> [   50.095909]  v4l2_pipeline_pm_use+0x5c/0xa0 [videodev]
> [   50.095923]  v4l2_pipeline_pm_get+0x1c/0x30 [videodev]
> [   50.095937]  video_open+0x7c/0x100 [qcom_camss]
> [   50.095945]  v4l2_open+0x84/0x130 [videodev]
> [   50.095960]  chrdev_open+0xc8/0x250
> [   50.095964]  do_dentry_open+0x1bc/0x498
> [   50.095966]  vfs_open+0x34/0x40
> [   50.095968]  path_openat+0xb44/0xf20
> [   50.095971]  do_filp_open+0xa4/0x160
> [   50.095974]  do_sys_openat2+0xc8/0x188
> [   50.095975]  __arm64_sys_openat+0x6c/0xb8
> [   50.095977]  invoke_syscall+0x50/0x128
> [   50.095982]  el0_svc_common.constprop.0+0x4c/0x100
> [   50.095985]  do_el0_svc+0x40/0xa8
> [   50.095988]  el0_svc+0x2c/0x88
> [   50.095991]  el0t_64_sync_handler+0xf4/0x120
> [   50.095994]  el0t_64_sync+0x190/0x198
> [   50.095996] ---[ end trace 0000000000000000 ]---
> 
> Fixes: 779096916dae ("media: camss: vfe: Fix runtime PM imbalance on error")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index dabfd613b2f94..938f373bcd1fd 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -611,7 +611,7 @@ int vfe_get(struct vfe_device *vfe)
>  	} else {
>  		ret = vfe_check_clock_rates(vfe);
>  		if (ret < 0)
> -			goto error_pm_runtime_get;
> +			goto error_pm_domain;
>  	}
>  	vfe->power_count++;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/9] media: qcom: camss: Fix VFE-17x vfe_disable_output()

[Bryan O'Donoghue]

There are two problems with the current vfe_disable_output() routine.

Firstly we rightly use a spinlock to protect output->gen2.active_num
everywhere except for in the IDLE timeout path of vfe_disable_output().
Even if that is not racy "in practice" somehow it is by happenstance not
by design.

Secondly we do not get consistent behaviour from this routine. On
sc8280xp 50% of the time I get "VFE idle timeout - resetting". In this
case the subsequent capture will succeed. The other 50% of the time, we
don't hit the idle timeout, never do the VFE reset and subsequent
captures stall indefinitely.

Rewrite the vfe_disable_output() routine to

- Quiesce write masters with vfe_wm_stop()
- Set active_num = 0

remembering to hold the spinlock when we do so followed by

- Reset the VFE

Testing on sc8280xp and sdm845 shows this to be a valid fix.

Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-vfe-170.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
index 02494c89da91c..ae9137633c301 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
@@ -500,28 +500,15 @@ static int vfe_disable_output(struct vfe_line *line)
 	struct vfe_output *output = &line->output;
 	unsigned long flags;
 	unsigned int i;
-	bool done;
-	int timeout = 0;
-
-	do {
-		spin_lock_irqsave(&vfe->output_lock, flags);
-		done = !output->gen2.active_num;
-		spin_unlock_irqrestore(&vfe->output_lock, flags);
-		usleep_range(10000, 20000);
-
-		if (timeout++ == 100) {
-			dev_err(vfe->camss->dev, "VFE idle timeout - resetting\n");
-			vfe_reset(vfe);
-			output->gen2.active_num = 0;
-			return 0;
-		}
-	} while (!done);
 
 	spin_lock_irqsave(&vfe->output_lock, flags);
 	for (i = 0; i < output->wm_num; i++)
 		vfe_wm_stop(vfe, output->wm_idx[i]);
+	output->gen2.active_num = 0;
 	spin_unlock_irqrestore(&vfe->output_lock, flags);
 
+	vfe_reset(vfe);
+
 	return 0;
 }
 
-- 
2.41.0

Re: [PATCH v2 4/9] media: qcom: camss: Fix VFE-17x vfe_disable_output()

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:21PM +0100, Bryan O'Donoghue wrote:
> There are two problems with the current vfe_disable_output() routine.
> 
> Firstly we rightly use a spinlock to protect output->gen2.active_num
> everywhere except for in the IDLE timeout path of vfe_disable_output().
> Even if that is not racy "in practice" somehow it is by happenstance not
> by design.
> 
> Secondly we do not get consistent behaviour from this routine. On
> sc8280xp 50% of the time I get "VFE idle timeout - resetting". In this
> case the subsequent capture will succeed. The other 50% of the time, we
> don't hit the idle timeout, never do the VFE reset and subsequent
> captures stall indefinitely.
> 
> Rewrite the vfe_disable_output() routine to
> 
> - Quiesce write masters with vfe_wm_stop()
> - Set active_num = 0
> 
> remembering to hold the spinlock when we do so followed by
> 
> - Reset the VFE
> 
> Testing on sc8280xp and sdm845 shows this to be a valid fix.
> 
> Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

I can't comment on the validity of the fix, but nothing shocks me in the
patch, so I'm fine with it.

> ---
>  .../media/platform/qcom/camss/camss-vfe-170.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-170.c b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> index 02494c89da91c..ae9137633c301 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-170.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-170.c
> @@ -500,28 +500,15 @@ static int vfe_disable_output(struct vfe_line *line)
>  	struct vfe_output *output = &line->output;
>  	unsigned long flags;
>  	unsigned int i;
> -	bool done;
> -	int timeout = 0;
> -
> -	do {
> -		spin_lock_irqsave(&vfe->output_lock, flags);
> -		done = !output->gen2.active_num;
> -		spin_unlock_irqrestore(&vfe->output_lock, flags);
> -		usleep_range(10000, 20000);

Now that you don't sleep anymore, I think you can drop the inclusion of
linux/delay.h.

> -
> -		if (timeout++ == 100) {
> -			dev_err(vfe->camss->dev, "VFE idle timeout - resetting\n");
> -			vfe_reset(vfe);
> -			output->gen2.active_num = 0;
> -			return 0;
> -		}
> -	} while (!done);
>  
>  	spin_lock_irqsave(&vfe->output_lock, flags);
>  	for (i = 0; i < output->wm_num; i++)
>  		vfe_wm_stop(vfe, output->wm_idx[i]);
> +	output->gen2.active_num = 0;
>  	spin_unlock_irqrestore(&vfe->output_lock, flags);
>  
> +	vfe_reset(vfe);
> +
>  	return 0;

This function could become void, especially given that its only caller
doesn't check the return value.

>  }
>  
> -- 
> 2.41.0
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 5/9] media: qcom: camss: Fix VFE-480 vfe_disable_output()

[Bryan O'Donoghue]

vfe-480 is copied from vfe-17x and has the same racy idle timeout bug as in
17x.

Fix the vfe_disable_output() logic to no longer be racy and to conform
to the 17x way of quiescing and then resetting the VFE.

Fixes: 4edc8eae715c ("media: camss: Add initial support for VFE hardware version Titan 480")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-vfe-480.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
index f70aad2e8c237..a64d660abc538 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
@@ -334,28 +334,15 @@ static int vfe_disable_output(struct vfe_line *line)
 	struct vfe_output *output = &line->output;
 	unsigned long flags;
 	unsigned int i;
-	bool done;
-	int timeout = 0;
-
-	do {
-		spin_lock_irqsave(&vfe->output_lock, flags);
-		done = !output->gen2.active_num;
-		spin_unlock_irqrestore(&vfe->output_lock, flags);
-		usleep_range(10000, 20000);
-
-		if (timeout++ == 100) {
-			dev_err(vfe->camss->dev, "VFE idle timeout - resetting\n");
-			vfe_reset(vfe);
-			output->gen2.active_num = 0;
-			return 0;
-		}
-	} while (!done);
 
 	spin_lock_irqsave(&vfe->output_lock, flags);
 	for (i = 0; i < output->wm_num; i++)
 		vfe_wm_stop(vfe, output->wm_idx[i]);
+	output->gen2.active_num = 0;
 	spin_unlock_irqrestore(&vfe->output_lock, flags);
 
+	vfe_reset(vfe);
+
 	return 0;
 }
 
-- 
2.41.0

Re: [PATCH v2 5/9] media: qcom: camss: Fix VFE-480 vfe_disable_output()

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:22PM +0100, Bryan O'Donoghue wrote:
> vfe-480 is copied from vfe-17x and has the same racy idle timeout bug as in
> 17x.
> 
> Fix the vfe_disable_output() logic to no longer be racy and to conform
> to the 17x way of quiescing and then resetting the VFE.

How about dropping the duplicate function and share a single
implementation for the two files ?

> Fixes: 4edc8eae715c ("media: camss: Add initial support for VFE hardware version Titan 480")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-vfe-480.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-480.c b/drivers/media/platform/qcom/camss/camss-vfe-480.c
> index f70aad2e8c237..a64d660abc538 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-480.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-480.c
> @@ -334,28 +334,15 @@ static int vfe_disable_output(struct vfe_line *line)
>  	struct vfe_output *output = &line->output;
>  	unsigned long flags;
>  	unsigned int i;
> -	bool done;
> -	int timeout = 0;
> -
> -	do {
> -		spin_lock_irqsave(&vfe->output_lock, flags);
> -		done = !output->gen2.active_num;
> -		spin_unlock_irqrestore(&vfe->output_lock, flags);
> -		usleep_range(10000, 20000);
> -
> -		if (timeout++ == 100) {
> -			dev_err(vfe->camss->dev, "VFE idle timeout - resetting\n");
> -			vfe_reset(vfe);
> -			output->gen2.active_num = 0;
> -			return 0;
> -		}
> -	} while (!done);
>  
>  	spin_lock_irqsave(&vfe->output_lock, flags);
>  	for (i = 0; i < output->wm_num; i++)
>  		vfe_wm_stop(vfe, output->wm_idx[i]);
> +	output->gen2.active_num = 0;
>  	spin_unlock_irqrestore(&vfe->output_lock, flags);
>  
> +	vfe_reset(vfe);
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/9] media: qcom: camss: Fix VFE-480 vfe_disable_output()

[Bryan O'Donoghue]

On 28/08/2023 18:17, Laurent Pinchart wrote:
>> vfe-480 is copied from vfe-17x and has the same racy idle timeout bug as in
>> 17x.
>>
>> Fix the vfe_disable_output() logic to no longer be racy and to conform
>> to the 17x way of quiescing and then resetting the VFE.
> How about dropping the duplicate function and share a single
> implementation for the two files ?
> 

Hmm, so I looked at this.

In principle I like it. Right now vfe-170 only deals with a single 
write-master = 0, whereas vfe-480 deals with multiple write-masters.

Collapsing down into one place is the right thing to do however, it 
feels like a larger update to vfe-170 that merits its own series along 
the lines of "Support multiple write-masters for vfe-17x" or better 
still "Support virtual channels for vfe-17x" which is what is implied by 
this.

Yet another thing to add to the TODO list here.

---
bod

[PATCH v2 6/9] media: qcom: camss: Fix missing vfe_lite clocks check

[Bryan O'Donoghue]

check_clock doesn't account for vfe_lite which means that vfe_lite will
never get validated by this routine. Add the clock name to the expected set
to remediate.

Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-vfe.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
index 938f373bcd1fd..b021f81cef123 100644
--- a/drivers/media/platform/qcom/camss/camss-vfe.c
+++ b/drivers/media/platform/qcom/camss/camss-vfe.c
@@ -535,7 +535,8 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
 		struct camss_clock *clock = &vfe->clock[i];
 
 		if (!strcmp(clock->name, "vfe0") ||
-		    !strcmp(clock->name, "vfe1")) {
+		    !strcmp(clock->name, "vfe1") ||
+		    !strcmp(clock->name, "vfe_lite")) {
 			u64 min_rate = 0;
 			unsigned long rate;
 
-- 
2.41.0

Re: [PATCH v2 6/9] media: qcom: camss: Fix missing vfe_lite clocks check

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.


On Tue, Aug 22, 2023 at 09:06:23PM +0100, Bryan O'Donoghue wrote:
> check_clock doesn't account for vfe_lite which means that vfe_lite will
> never get validated by this routine. Add the clock name to the expected set
> to remediate.
> 
> Fixes: 7319cdf189bb ("media: camss: Add support for VFE hardware version Titan 170")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss-vfe.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index 938f373bcd1fd..b021f81cef123 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -535,7 +535,8 @@ static int vfe_check_clock_rates(struct vfe_device *vfe)
>  		struct camss_clock *clock = &vfe->clock[i];
>  
>  		if (!strcmp(clock->name, "vfe0") ||
> -		    !strcmp(clock->name, "vfe1")) {
> +		    !strcmp(clock->name, "vfe1") ||
> +		    !strcmp(clock->name, "vfe_lite")) {
>  			u64 min_rate = 0;
>  			unsigned long rate;
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 7/9] media: qcom: camss: Fix invalid clock enable bit disjunction

[Bryan O'Donoghue]

define CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE BIT(7)

disjunction for gen2 ? BIT(7) : is a nop we are setting the same bit
either way.

Fixes: 4abb21309fda ("media: camss: csiphy: Move to hardcode CSI Clock Lane number")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
index 04baa80494c66..4dba61b8d3f2a 100644
--- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
+++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
@@ -476,7 +476,7 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
 
 	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
 
-	val = is_gen2 ? BIT(7) : CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
+	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
 	for (i = 0; i < c->num_data; i++)
 		val |= BIT(c->data[i].pos * 2);
 
-- 
2.41.0

Re: [PATCH v2 7/9] media: qcom: camss: Fix invalid clock enable bit disjunction

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:24PM +0100, Bryan O'Donoghue wrote:
> define CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE BIT(7)
> 
> disjunction for gen2 ? BIT(7) : is a nop we are setting the same bit
> either way.
> 
> Fixes: 4abb21309fda ("media: camss: csiphy: Move to hardcode CSI Clock Lane number")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> index 04baa80494c66..4dba61b8d3f2a 100644
> --- a/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> +++ b/drivers/media/platform/qcom/camss/camss-csiphy-3ph-1-0.c
> @@ -476,7 +476,7 @@ static void csiphy_lanes_enable(struct csiphy_device *csiphy,
>  
>  	settle_cnt = csiphy_settle_cnt_calc(link_freq, csiphy->timer_clk_rate);
>  
> -	val = is_gen2 ? BIT(7) : CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>  	for (i = 0; i < c->num_data; i++)
>  		val |= BIT(c->data[i].pos * 2);
>  
> -- 
> 2.41.0
> 

-- 
Regards,

Laurent Pinchart

[PATCH v2 8/9] media: qcom: camss: Fix set CSI2_RX_CFG1_VC_MODE when VC is greater than 3

[Bryan O'Donoghue]

VC_MODE = 0 implies a two bit VC address.
VC_MODE = 1 is required for VCs with a larger address than two bits.

Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csid-gen2.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 45c7986d4a8d0..140c584bfb8b1 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -449,6 +449,8 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
 	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
 
 	val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
+	if (vc > 3)
+		val |= 1 << CSI2_RX_CFG1_VC_MODE;
 	val |= 1 << CSI2_RX_CFG1_MISR_EN;
 	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
 
-- 
2.41.0

Re: [PATCH v2 8/9] media: qcom: camss: Fix set CSI2_RX_CFG1_VC_MODE when VC is greater than 3

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:25PM +0100, Bryan O'Donoghue wrote:
> VC_MODE = 0 implies a two bit VC address.
> VC_MODE = 1 is required for VCs with a larger address than two bits.
> 
> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-csid-gen2.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> index 45c7986d4a8d0..140c584bfb8b1 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> @@ -449,6 +449,8 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
>  	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG0);
>  
>  	val = 1 << CSI2_RX_CFG1_PACKET_ECC_CORRECTION_EN;
> +	if (vc > 3)
> +		val |= 1 << CSI2_RX_CFG1_VC_MODE;

It looks like CSI2_RX_CFG1_VC_MODE should be defined as BIT(2) instead
of 2, and this line should drop the '1 <<'. Same for lots of other bits.
Could you fix this in a separate patch ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	val |= 1 << CSI2_RX_CFG1_MISR_EN;
>  	writel_relaxed(val, csid->base + CSID_CSI2_RX_CFG1);
>  

-- 
Regards,

Laurent Pinchart

[PATCH v2 9/9] media: qcom: camss: Fix csid-gen2 for test pattern generator

[Bryan O'Donoghue]

From: Andrey Konovalov <andrey.konovalov@linaro.org>

In the current driver csid Test Pattern Generator (TPG) doesn't work.
This change:
- fixes writing frame width and height values into CSID_TPG_DT_n_CFG_0
- fixes the shift by one between test_pattern control value and the
  actual pattern.
- drops fixed VC of 0x0a which testing showed prohibited some test
  patterns in the CSID to produce output.
So that TPG starts working, but with the below limitations:
- only test_pattern=9 works as it should
- test_pattern=8 and test_pattern=7 produce black frame (all zeroes)
- the rest of test_pattern's don't work (yavta doesn't get the data)
- regardless of the CFA pattern set by 'media-ctl -V' the actual pixel
  order is always the same (RGGB for any RAW8 or RAW10P format in
  4608x2592 resolution).

Tested with:

RAW10P format, VC0:
 media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4608x2592 field:none]'
 media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4608x2592 field:none]'
 media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
 v4l2-ctl -d /dev/v4l-subdev6 -c test_pattern=9
 yavta -B capture-mplane --capture=3 -n 3 -f SRGGB10P -s 4608x2592 /dev/video0

RAW10P format, VC1:
 media-ctl -V '"msm_csid0":2[fmt:SRGGB10/4608x2592 field:none]'
 media-ctl -V '"msm_vfe0_rdi1":0[fmt:SRGGB10/4608x2592 field:none]'
 media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
 v4l2-ctl -d /dev/v4l-subdev6 -c test_pattern=9
 yavta -B capture-mplane --capture=3 -n 3 -f SRGGB10P -s 4608x2592 /dev/video1

RAW8 format, VC0:
 media-ctl --reset
 media-ctl -V '"msm_csid0":0[fmt:SRGGB8/4608x2592 field:none]'
 media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB8/4608x2592 field:none]'
 media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
 yavta -B capture-mplane --capture=3 -n 3 -f SRGGB8 -s 4608x2592 /dev/video0

Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")
Cc: stable@vger.kernel.org
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/platform/qcom/camss/camss-csid-gen2.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
index 140c584bfb8b1..6ba2b10326444 100644
--- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
+++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
@@ -355,9 +355,6 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
 		u8 dt_id = vc;
 
 		if (tg->enabled) {
-			/* Config Test Generator */
-			vc = 0xa;
-
 			/* configure one DT, infinite frames */
 			val = vc << TPG_VC_CFG0_VC_NUM;
 			val |= INTELEAVING_MODE_ONE_SHOT << TPG_VC_CFG0_LINE_INTERLEAVING_MODE;
@@ -370,14 +367,14 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
 
 			writel_relaxed(0x12345678, csid->base + CSID_TPG_LFSR_SEED);
 
-			val = input_format->height & 0x1fff << TPG_DT_n_CFG_0_FRAME_HEIGHT;
-			val |= input_format->width & 0x1fff << TPG_DT_n_CFG_0_FRAME_WIDTH;
+			val = (input_format->height & 0x1fff) << TPG_DT_n_CFG_0_FRAME_HEIGHT;
+			val |= (input_format->width & 0x1fff) << TPG_DT_n_CFG_0_FRAME_WIDTH;
 			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_0(0));
 
 			val = format->data_type << TPG_DT_n_CFG_1_DATA_TYPE;
 			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_1(0));
 
-			val = tg->mode << TPG_DT_n_CFG_2_PAYLOAD_MODE;
+			val = (tg->mode - 1) << TPG_DT_n_CFG_2_PAYLOAD_MODE;
 			val |= 0xBE << TPG_DT_n_CFG_2_USER_SPECIFIED_PAYLOAD;
 			val |= format->decode_format << TPG_DT_n_CFG_2_ENCODE_FORMAT;
 			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_2(0));
-- 
2.41.0

Re: [PATCH v2 9/9] media: qcom: camss: Fix csid-gen2 for test pattern generator

[Laurent Pinchart]

Hi Bryan,

Thank you for the patch.

On Tue, Aug 22, 2023 at 09:06:26PM +0100, Bryan O'Donoghue wrote:
> From: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> In the current driver csid Test Pattern Generator (TPG) doesn't work.
> This change:
> - fixes writing frame width and height values into CSID_TPG_DT_n_CFG_0
> - fixes the shift by one between test_pattern control value and the
>   actual pattern.
> - drops fixed VC of 0x0a which testing showed prohibited some test
>   patterns in the CSID to produce output.
> So that TPG starts working, but with the below limitations:
> - only test_pattern=9 works as it should
> - test_pattern=8 and test_pattern=7 produce black frame (all zeroes)
> - the rest of test_pattern's don't work (yavta doesn't get the data)
> - regardless of the CFA pattern set by 'media-ctl -V' the actual pixel
>   order is always the same (RGGB for any RAW8 or RAW10P format in
>   4608x2592 resolution).
> 
> Tested with:
> 
> RAW10P format, VC0:
>  media-ctl -V '"msm_csid0":0[fmt:SRGGB10/4608x2592 field:none]'
>  media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB10/4608x2592 field:none]'
>  media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>  v4l2-ctl -d /dev/v4l-subdev6 -c test_pattern=9
>  yavta -B capture-mplane --capture=3 -n 3 -f SRGGB10P -s 4608x2592 /dev/video0
> 
> RAW10P format, VC1:
>  media-ctl -V '"msm_csid0":2[fmt:SRGGB10/4608x2592 field:none]'
>  media-ctl -V '"msm_vfe0_rdi1":0[fmt:SRGGB10/4608x2592 field:none]'
>  media-ctl -l '"msm_csid0":2->"msm_vfe0_rdi1":0[1]'
>  v4l2-ctl -d /dev/v4l-subdev6 -c test_pattern=9
>  yavta -B capture-mplane --capture=3 -n 3 -f SRGGB10P -s 4608x2592 /dev/video1
> 
> RAW8 format, VC0:
>  media-ctl --reset
>  media-ctl -V '"msm_csid0":0[fmt:SRGGB8/4608x2592 field:none]'
>  media-ctl -V '"msm_vfe0_rdi0":0[fmt:SRGGB8/4608x2592 field:none]'
>  media-ctl -l '"msm_csid0":1->"msm_vfe0_rdi0":0[1]'
>  yavta -B capture-mplane --capture=3 -n 3 -f SRGGB8 -s 4608x2592 /dev/video0
> 
> Fixes: eebe6d00e9bf ("media: camss: Add support for CSID hardware version Titan 170")
> Cc: stable@vger.kernel.org
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/qcom/camss/camss-csid-gen2.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> index 140c584bfb8b1..6ba2b10326444 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
> @@ -355,9 +355,6 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
>  		u8 dt_id = vc;
>  
>  		if (tg->enabled) {
> -			/* Config Test Generator */
> -			vc = 0xa;
> -
>  			/* configure one DT, infinite frames */
>  			val = vc << TPG_VC_CFG0_VC_NUM;
>  			val |= INTELEAVING_MODE_ONE_SHOT << TPG_VC_CFG0_LINE_INTERLEAVING_MODE;
> @@ -370,14 +367,14 @@ static void __csid_configure_stream(struct csid_device *csid, u8 enable, u8 vc)
>  
>  			writel_relaxed(0x12345678, csid->base + CSID_TPG_LFSR_SEED);
>  
> -			val = input_format->height & 0x1fff << TPG_DT_n_CFG_0_FRAME_HEIGHT;
> -			val |= input_format->width & 0x1fff << TPG_DT_n_CFG_0_FRAME_WIDTH;
> +			val = (input_format->height & 0x1fff) << TPG_DT_n_CFG_0_FRAME_HEIGHT;
> +			val |= (input_format->width & 0x1fff) << TPG_DT_n_CFG_0_FRAME_WIDTH;
>  			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_0(0));
>  
>  			val = format->data_type << TPG_DT_n_CFG_1_DATA_TYPE;
>  			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_1(0));
>  
> -			val = tg->mode << TPG_DT_n_CFG_2_PAYLOAD_MODE;
> +			val = (tg->mode - 1) << TPG_DT_n_CFG_2_PAYLOAD_MODE;
>  			val |= 0xBE << TPG_DT_n_CFG_2_USER_SPECIFIED_PAYLOAD;
>  			val |= format->decode_format << TPG_DT_n_CFG_2_ENCODE_FORMAT;
>  			writel_relaxed(val, csid->base + CSID_TPG_DT_n_CFG_2(0));

-- 
Regards,

Laurent Pinchart