diff mbox series

[v3,07/15] media: qcom: camss: Capture VFE CSID dependency in a helper function

Message ID 20230823104444.1954663-8-bryan.odonoghue@linaro.org (mailing list archive)
State New, archived
Headers show
Series media: qcom: camss: Add parameter passing to remove several outstanding bugs | expand

Commit Message

Bryan O'Donoghue Aug. 23, 2023, 10:44 a.m. UTC
From sdm845 onwards we need to ensure the VFE is powered on prior to
switching on the CSID.

Alternatively we could model up the GDSCs and clocks the CSID needs
without the VFE but, there's a real question of the legitimacy of such a
use-case.

For now drawing a line at sdm845 and switching on the associated VFEs is
a perfectly valid thing to do.

Rather than continually extend out this clause for at least two new SoCs
with this same model - making the vfe_get/vfe_put path start to look
like spaghetti we can encoded the dependency in a helper function.

Use csid_depends_vfe() for this purpose.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Konrad Dybcio Aug. 26, 2023, 10:02 a.m. UTC | #1
On 23.08.2023 12:44, Bryan O'Donoghue wrote:
> From sdm845 onwards we need to ensure the VFE is powered on prior to
> switching on the CSID.
And what's the symptom if we fail to ensure this? How can someone
adding support for another platform tell whether the match-list
should be expanded?

> 
> Alternatively we could model up the GDSCs and clocks the CSID needs
> without the VFE but, there's a real question of the legitimacy of such a
> use-case.
> 
> For now drawing a line at sdm845 and switching on the associated VFEs is
> a perfectly valid thing to do.
> 
> Rather than continually extend out this clause for at least two new SoCs
> with this same model - making the vfe_get/vfe_put path start to look
> like spaghetti we can encoded the dependency in a helper function.
> 
> Use csid_depends_vfe() for this purpose.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 08991b070bd61..fd04ed112b564 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
>  	return 0;
>  }
>  
> +static bool csid_depends_vfe(u32 version)
toggle_vfe_before_csid?

> +{
> +	bool ret = false;
> +
> +	switch (version) {
> +	case CAMSS_845:
> +	case CAMSS_8250:
> +		ret = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
I'm not sure if it would be okay with like C conventions and
stuff, but this can be made shorter by returning from within
the switch statement

Konrad
Bryan O'Donoghue Aug. 26, 2023, 12:01 p.m. UTC | #2
On 26/08/2023 11:02, Konrad Dybcio wrote:
> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>>  From sdm845 onwards we need to ensure the VFE is powered on prior to
>> switching on the CSID.
> And what's the symptom if we fail to ensure this? How can someone
> adding support for another platform tell whether the match-list
> should be expanded?

That person has to understand the dependency.

The first version of this patch >= SDM845 would mitigate needing to know 
to expand the list.

Rather than revisit that discussion, I will amend the commit log.

> 
>>
>> Alternatively we could model up the GDSCs and clocks the CSID needs
>> without the VFE but, there's a real question of the legitimacy of such a
>> use-case.
>>
>> For now drawing a line at sdm845 and switching on the associated VFEs is
>> a perfectly valid thing to do.
>>
>> Rather than continually extend out this clause for at least two new SoCs
>> with this same model - making the vfe_get/vfe_put path start to look
>> like spaghetti we can encoded the dependency in a helper function.
>>
>> Use csid_depends_vfe() for this purpose.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
>>   1 file changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
>> index 08991b070bd61..fd04ed112b564 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
>>   	return 0;
>>   }
>>   
>> +static bool csid_depends_vfe(u32 version)
> toggle_vfe_before_csid?

If that's clearer np.

>> +{
>> +	bool ret = false;
>> +
>> +	switch (version) {
>> +	case CAMSS_845:
>> +	case CAMSS_8250:
>> +		ret = true;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return ret;
> I'm not sure if it would be okay with like C conventions and
> stuff, but this can be made shorter by returning from within
> the switch statement

Yes but you still need the explicit return at the end of the function or 
from memory at least some of the compiler/static analysis or checkpatch 
stuff - I forget which - will complain.

---
bod
Konrad Dybcio Aug. 26, 2023, 12:04 p.m. UTC | #3
On 26.08.2023 14:01, Bryan O'Donoghue wrote:
> On 26/08/2023 11:02, Konrad Dybcio wrote:
>> On 23.08.2023 12:44, Bryan O'Donoghue wrote:
>>>  From sdm845 onwards we need to ensure the VFE is powered on prior to
>>> switching on the CSID.
>> And what's the symptom if we fail to ensure this? How can someone
>> adding support for another platform tell whether the match-list
>> should be expanded?
> 
> That person has to understand the dependency.
If we need this workaround, there surely must be something that doesn't
work without it, a measurable symptom. What is it?

Konrad
Bryan O'Donoghue Aug. 26, 2023, 12:12 p.m. UTC | #4
On 26/08/2023 13:04, Konrad Dybcio wrote:
>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>> switching on the CSID.
>>> And what's the symptom if we fail to ensure this? How can someone
>>> adding support for another platform tell whether the match-list
>>> should be expanded?
>> That person has to understand the dependency.
> If we need this workaround, there surely must be something that doesn't
> work without it, a measurable symptom. What is it?

The CSID lives inside of the VFE therefore the VFE must be power prior 
to the CSID.

The symptom will be .. the CSID doesn't come out of reset.

---
bod
Konrad Dybcio Aug. 26, 2023, 12:13 p.m. UTC | #5
On 26.08.2023 14:12, Bryan O'Donoghue wrote:
> On 26/08/2023 13:04, Konrad Dybcio wrote:
>>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>>> switching on the CSID.
>>>> And what's the symptom if we fail to ensure this? How can someone
>>>> adding support for another platform tell whether the match-list
>>>> should be expanded?
>>> That person has to understand the dependency.
>> If we need this workaround, there surely must be something that doesn't
>> work without it, a measurable symptom. What is it?
> 
> The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID.
> 
> The symptom will be .. the CSID doesn't come out of reset.
Good, that's what I needed to know.

Now we can rename that function to something like camss_csid_inside_vfe()
to make it more meaningful

Konrad
Bryan O'Donoghue Aug. 26, 2023, 12:16 p.m. UTC | #6
On 26/08/2023 13:13, Konrad Dybcio wrote:
> On 26.08.2023 14:12, Bryan O'Donoghue wrote:
>> On 26/08/2023 13:04, Konrad Dybcio wrote:
>>>>>>    From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>>>> switching on the CSID.
>>>>> And what's the symptom if we fail to ensure this? How can someone
>>>>> adding support for another platform tell whether the match-list
>>>>> should be expanded?
>>>> That person has to understand the dependency.
>>> If we need this workaround, there surely must be something that doesn't
>>> work without it, a measurable symptom. What is it?
>>
>> The CSID lives inside of the VFE therefore the VFE must be power prior to the CSID.
>>
>> The symptom will be .. the CSID doesn't come out of reset.
> Good, that's what I needed to know.
> 
> Now we can rename that function to something like camss_csid_inside_vfe()
> to make it more meaningful

If you feel there is a meaningful distinction between 
"csid_depends_vfe()" and "camss_csid_inside_vfe()"

I'm happy to humour you.

---
bod
Laurent Pinchart Aug. 28, 2023, 6:47 p.m. UTC | #7
Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
> From sdm845 onwards we need to ensure the VFE is powered on prior to
> switching on the CSID.

Based on the discussions with Konrad in the mail thread, I would record
here the reason for this requirement.

What happens if you unconditionally power up the VFE prior to switching
the CSID on ? Will this break something on platforms older than SDM845 ?

> Alternatively we could model up the GDSCs and clocks the CSID needs
> without the VFE but, there's a real question of the legitimacy of such a
> use-case.
> 
> For now drawing a line at sdm845 and switching on the associated VFEs is
> a perfectly valid thing to do.
> 
> Rather than continually extend out this clause for at least two new SoCs
> with this same model - making the vfe_get/vfe_put path start to look
> like spaghetti we can encoded the dependency in a helper function.
> 
> Use csid_depends_vfe() for this purpose.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../media/platform/qcom/camss/camss-csid.c    | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
> index 08991b070bd61..fd04ed112b564 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
> @@ -146,6 +146,22 @@ static int csid_set_clock_rates(struct csid_device *csid)
>  	return 0;
>  }
>  
> +static bool csid_depends_vfe(u32 version)
> +{
> +	bool ret = false;
> +
> +	switch (version) {
> +	case CAMSS_845:
> +	case CAMSS_8250:
> +		ret = true;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * csid_set_power - Power on/off CSID module
>   * @sd: CSID V4L2 subdevice
> @@ -163,7 +179,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>  	int ret = 0;
>  
>  	if (on) {
> -		if (version == CAMSS_8250 || version == CAMSS_845) {
> +		if (csid_depends_vfe(version)) {
>  			ret = vfe_get(vfe);
>  			if (ret < 0)
>  				return ret;
> @@ -217,7 +233,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>  		regulator_bulk_disable(csid->num_supplies,
>  				       csid->supplies);
>  		pm_runtime_put_sync(dev);
> -		if (version == CAMSS_8250 || version == CAMSS_845)
> +		if (csid_depends_vfe(version))
>  			vfe_put(vfe);
>  	}
>
Bryan O'Donoghue Aug. 28, 2023, 7:37 p.m. UTC | #8
On 28/08/2023 19:47, Laurent Pinchart wrote:
> Hi Bryan,
> 
> Thank you for the patch.
> 
> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
>>  From sdm845 onwards we need to ensure the VFE is powered on prior to
>> switching on the CSID.
> 
> Based on the discussions with Konrad in the mail thread, I would record
> here the reason for this requirement.
> 
> What happens if you unconditionally power up the VFE prior to switching
> the CSID on ? Will this break something on platforms older than SDM845 ?

vfe->power_count will bump and you will be fine.

---
bod
Laurent Pinchart Aug. 28, 2023, 7:40 p.m. UTC | #9
On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote:
> On 28/08/2023 19:47, Laurent Pinchart wrote:
> > Hi Bryan,
> > 
> > Thank you for the patch.
> > 
> > On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
> >>  From sdm845 onwards we need to ensure the VFE is powered on prior to
> >> switching on the CSID.
> > 
> > Based on the discussions with Konrad in the mail thread, I would record
> > here the reason for this requirement.
> > 
> > What happens if you unconditionally power up the VFE prior to switching
> > the CSID on ? Will this break something on platforms older than SDM845 ?
> 
> vfe->power_count will bump and you will be fine.

Then maybe that would be a simpler solution than having a
device-specific power sequence ?
Bryan O'Donoghue Aug. 28, 2023, 7:41 p.m. UTC | #10
On 28/08/2023 20:40, Laurent Pinchart wrote:
> On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote:
>> On 28/08/2023 19:47, Laurent Pinchart wrote:
>>> Hi Bryan,
>>>
>>> Thank you for the patch.
>>>
>>> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>> switching on the CSID.
>>>
>>> Based on the discussions with Konrad in the mail thread, I would record
>>> here the reason for this requirement.
>>>
>>> What happens if you unconditionally power up the VFE prior to switching
>>> the CSID on ? Will this break something on platforms older than SDM845 ?
>>
>> vfe->power_count will bump and you will be fine.
> 
> Then maybe that would be a simpler solution than having a
> device-specific power sequence ?
> 

A fair point.

Worth a try.

---
bod
Bryan O'Donoghue Sept. 4, 2023, 10:15 a.m. UTC | #11
On 28/08/2023 20:40, Laurent Pinchart wrote:
> On Mon, Aug 28, 2023 at 08:37:44PM +0100, Bryan O'Donoghue wrote:
>> On 28/08/2023 19:47, Laurent Pinchart wrote:
>>> Hi Bryan,
>>>
>>> Thank you for the patch.
>>>
>>> On Wed, Aug 23, 2023 at 11:44:36AM +0100, Bryan O'Donoghue wrote:
>>>>   From sdm845 onwards we need to ensure the VFE is powered on prior to
>>>> switching on the CSID.
>>>
>>> Based on the discussions with Konrad in the mail thread, I would record
>>> here the reason for this requirement.
>>>
>>> What happens if you unconditionally power up the VFE prior to switching
>>> the CSID on ? Will this break something on platforms older than SDM845 ?
>>
>> vfe->power_count will bump and you will be fine.
> 
> Then maybe that would be a simpler solution than having a
> device-specific power sequence ?
> 

So, this works.

I'll send out a patch based on this suggestion.

---
bod
diff mbox series

Patch

diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
index 08991b070bd61..fd04ed112b564 100644
--- a/drivers/media/platform/qcom/camss/camss-csid.c
+++ b/drivers/media/platform/qcom/camss/camss-csid.c
@@ -146,6 +146,22 @@  static int csid_set_clock_rates(struct csid_device *csid)
 	return 0;
 }
 
+static bool csid_depends_vfe(u32 version)
+{
+	bool ret = false;
+
+	switch (version) {
+	case CAMSS_845:
+	case CAMSS_8250:
+		ret = true;
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
 /*
  * csid_set_power - Power on/off CSID module
  * @sd: CSID V4L2 subdevice
@@ -163,7 +179,7 @@  static int csid_set_power(struct v4l2_subdev *sd, int on)
 	int ret = 0;
 
 	if (on) {
-		if (version == CAMSS_8250 || version == CAMSS_845) {
+		if (csid_depends_vfe(version)) {
 			ret = vfe_get(vfe);
 			if (ret < 0)
 				return ret;
@@ -217,7 +233,7 @@  static int csid_set_power(struct v4l2_subdev *sd, int on)
 		regulator_bulk_disable(csid->num_supplies,
 				       csid->supplies);
 		pm_runtime_put_sync(dev);
-		if (version == CAMSS_8250 || version == CAMSS_845)
+		if (csid_depends_vfe(version))
 			vfe_put(vfe);
 	}