Re: [PATCH v3 06/15] media: qcom: camss: Assign the correct number of RDIs per VFE

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Bryan,

Thank you for the patch.

On Wed, Aug 23, 2023 at 11:44:35AM +0100, Bryan O'Donoghue wrote:
> Each Video Front End - VFE - has a variable number of Raw Data Interfaces -
> RDIs associated with it.
> 
> The CAMSS code started from a naive implementation where a fixed define was
> used as a control in a for(){} loop iterating through RDIs.
> 
> That model scales badly. An attempt was made with  VFE_LINE_NUM_GEN2 and
> VFE_LINE_NUM_GEN1 to differentiate between SoCs but, the problem with that
> is "gen1" and "gen2" have no meaning in the silicon. There is no fixed
> constraint in the silicon between VFE and RDI, it is entirely up to the SoC
> designers how many VFEs are populated and how many RDIs to associate with
> each VFE.
> 
> As an example sdm845 has VFE version 175 and sm8250 VFE version 480.
> sdm845 has 2 VFEs with 4 RDIs and 1 VFE Lite with 4 RDIs.
> sm8250 has 2 VFEs with 3 RDIs and 2 VFE Lite with 4 RDIs.
> 
> Clearly then we need a more granular model to capture the necessary data.
> 
> The defines have gone away to be replaced with per-SoC data but, we haven't
> populated the parameter data with the real values.

I think you forgot to drop the macros from camss-vfe.h.

> 
> Let's call those values out now
> 
> msm8916:
> 1 x VFE
> 3 x RDI per VFE (not 4)
> 
> msm8996:
> 2 x VFE
> 3 x RDI per VFE (not 4)
> 
> sdm660:
> 2 x VFE
> 3 x RDI per VFE (not 4)
> 
> sdm845:
> 2 x VFE
> 4 x RDI per VFE (not 3)
> 1 x VFE Lite
> 4 x RDI per VFE Lite (not 3)
> 
> sm8250:
> 2 x VFE
> 3 x RDI per VFE (not 4)
> 2 x VFE Lite
> 4 x RDI per VFE

Did you mean per VFE Lite here ?

> This more complex and correct mapping was not possible prior to passing
> values via driver data. Now that we have that change in place we can
> correctly map VFEs to RDIs for each VFE.

We could store the value per VFE type (VFE vs. VFE Lite), instead of per
VFE instance, but that would be more complex I suppose, for little gain.
However, if there are more values that depend on the VFE type instead of
the VFE instance, this should be considered.

> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>

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

> ---
>  drivers/media/platform/qcom/camss/camss.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index ce0d86e45fe48..c8b8ad176ee2b 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -124,7 +124,7 @@ static const struct resources vfe_res_8x16[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	}
>  };
>  
> @@ -265,7 +265,7 @@ static const struct resources vfe_res_8x96[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	},
>  
>  	/* VFE1 */
> @@ -284,7 +284,7 @@ static const struct resources vfe_res_8x96[] = {
>  				{ 0 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	}
>  };
>  
> @@ -446,7 +446,7 @@ static const struct resources vfe_res_660[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	},
>  
>  	/* VFE1 */
> @@ -468,7 +468,7 @@ static const struct resources vfe_res_660[] = {
>  				{ 0 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = VFE_LINE_NUM_GEN1,
> +		.line_num = 3,
>  	}
>  };
>  
> @@ -627,7 +627,7 @@ static const struct resources vfe_res_845[] = {
>  				{ 384000000 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = VFE_LINE_NUM_GEN2,
> +		.line_num = 4,
>  	},
>  
>  	/* VFE1 */
> @@ -648,7 +648,7 @@ static const struct resources vfe_res_845[] = {
>  				{ 384000000 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = VFE_LINE_NUM_GEN2,
> +		.line_num = 4,
>  	},
>  
>  	/* VFE-lite */
> @@ -668,7 +668,7 @@ static const struct resources vfe_res_845[] = {
>  				{ 384000000 } },
>  		.reg = { "vfe_lite" },
>  		.interrupt = { "vfe_lite" },
> -		.line_num = VFE_LINE_NUM_GEN2,
> +		.line_num = 4,
>  	}
>  };
>  
> @@ -796,7 +796,7 @@ static const struct resources vfe_res_8250[] = {
>  				{ 0 } },
>  		.reg = { "vfe0" },
>  		.interrupt = { "vfe0" },
> -		.line_num = 4,
> +		.line_num = 3,
>  	},
>  	/* VFE1 */
>  	{
> @@ -815,7 +815,7 @@ static const struct resources vfe_res_8250[] = {
>  				{ 0 } },
>  		.reg = { "vfe1" },
>  		.interrupt = { "vfe1" },
> -		.line_num = 4,
> +		.line_num = 3,
>  	},
>  	/* VFE2 (lite) */
>  	{

-- 
Regards,

Laurent Pinchart