mbox series

[v1,0/2] media: camss: Fixups for large capture frames

Message ID 20240802152435.35796-1-jorcrous@amazon.com
Headers show
Series media: camss: Fixups for large capture frames | expand

Message

Jordan Crouse Aug. 2, 2024, 3:24 p.m. UTC 
A few small issues discovered while (thus far unsuccessfully) trying to bring
up a 64MP sensor. The chosen frame limitation of 8192 seems to be somewhat
arbitrary as there don't appear to be any hardware limits on maximum frame size.
Double the maximum allowable frame size to accommodate bigger sensors.

Next the larger data sizes end up needing bigger pixel clocks. This exposed a
bug for 8250 devices where the VFE clocks are shared between two blocks, but
the CSID block is being initialized second and overwriting the carefully
selected clock rates from VFE. This was likely not a problem earlier because
the lowest VFE clock rate that CSID was selecting was good enough for the
family of sensors that were being used.


Jordan Crouse (2):
  media: camss: Increase the maximum frame size
  media: camss: Avoid overwriting vfe clock rates for 8250

 .../media/platform/qcom/camss/camss-csid.c    |  8 +++----
 .../media/platform/qcom/camss/camss-csiphy.c  |  4 ++--
 .../media/platform/qcom/camss/camss-ispif.c   |  4 ++--
 drivers/media/platform/qcom/camss/camss-vfe.c |  4 ++--
 .../media/platform/qcom/camss/camss-video.c   |  6 +++---
 drivers/media/platform/qcom/camss/camss.c     | 21 +++++++++++++------
 6 files changed, 28 insertions(+), 19 deletions(-)

Comments

Bryan O'Donoghue Aug. 2, 2024, 10:20 p.m. UTC | #1 
On 02/08/2024 16:24, Jordan Crouse wrote:
> On sm8250 targets both the csid and vfe subsystems share a number of
> clocks. Commit b4436a18eedb ("media: camss: add support for SM8250 camss")
> reorganized the initialization sequence so that VFE gets initialized first
> but a side effect of that was that the CSID subsystem came in after and
> overwrites the set frequencies on the shared clocks.
> 
> Empty the frequency tables for the shared clocks in the CSID resources so
> they won't overwrite the clock rates that the VFE has already set.
> 
> Signed-off-by: Jordan Crouse <jorcrous@amazon.com>
> ---
> 
>   drivers/media/platform/qcom/camss/camss.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index 51b1d3550421..d78644c3ebe9 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -915,6 +915,15 @@ static const struct camss_subdev_resources csiphy_res_8250[] = {
>   	}
>   };
>   
> +/*
> + * Both CSID and VFE use some of the same vfe clocks and both
> + * should prepare/enable them but only the VFE subsystem should be in charge
> + * of setting the clock rates.
> + *
> + * Set the frequency tables for those clocks in the CSID resources to
> + * be empty so the csid subsystem doesn't overwrite the clock rates that the
> + * VFE already set.
> + */
>   static const struct camss_subdev_resources csid_res_8250[] = {
>   	/* CSID0 */
>   	{
> @@ -922,8 +931,8 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" },
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
> -				{ 350000000, 475000000, 576000000, 720000000 },
> -				{ 100000000, 200000000, 300000000, 400000000 },
> +				{ 0 },
> +				{ 0 },
>   				{ 0 } },
>   		.reg = { "csid0" },
>   		.interrupt = { "csid0" },
> @@ -939,8 +948,8 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" },
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
> -				{ 350000000, 475000000, 576000000, 720000000 },
> -				{ 100000000, 200000000, 300000000, 400000000 },
> +				{ 0 },
> +				{ 0 },
>   				{ 0 } },
>   		.reg = { "csid1" },
>   		.interrupt = { "csid1" },
> @@ -956,7 +965,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
> -				{ 400000000, 480000000 },
> +				{ 0 },
>   				{ 0 } },
>   		.reg = { "csid2" },
>   		.interrupt = { "csid2" },
> @@ -973,7 +982,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
>   		.clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
>   		.clock_rate = { { 400000000 },
>   				{ 400000000 },
> -				{ 400000000, 480000000 },
> +				{ 0 },
>   				{ 0 } },
>   		.reg = { "csid3" },
>   		.interrupt = { "csid3" },

Hi Jordan.

Thanks for your patch. Just looking at the clocks you are zeroing here, 
I think _probably_ these zeroized clocks can be removed from the CSID 
set entirely.

Could you investigate that ?

Also please add

Fixes: b4436a18eedb ("media: camss: add support for SM8250 camss") and 
cc stable@vger.kernel.org

---
bod
Jordan Crouse Aug. 6, 2024, 10:38 p.m. UTC | #2 
On Fri, Aug 02, 2024 at 11:20:17PM +0100, Bryan O'Donoghue wrote:
> 
> On 02/08/2024 16:24, Jordan Crouse wrote:
> >On sm8250 targets both the csid and vfe subsystems share a number of
> >clocks. Commit b4436a18eedb ("media: camss: add support for SM8250 camss")
> >reorganized the initialization sequence so that VFE gets initialized first
> >but a side effect of that was that the CSID subsystem came in after and
> >overwrites the set frequencies on the shared clocks.
> >
> >Empty the frequency tables for the shared clocks in the CSID resources so
> >they won't overwrite the clock rates that the VFE has already set.
> >
> >Signed-off-by: Jordan Crouse <jorcrous@amazon.com>
> >---
> >
> >  drivers/media/platform/qcom/camss/camss.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> >diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> >index 51b1d3550421..d78644c3ebe9 100644
> >--- a/drivers/media/platform/qcom/camss/camss.c
> >+++ b/drivers/media/platform/qcom/camss/camss.c
> >@@ -915,6 +915,15 @@ static const struct camss_subdev_resources csiphy_res_8250[] = {
> >      }
> >  };
> >
> >+/*
> >+ * Both CSID and VFE use some of the same vfe clocks and both
> >+ * should prepare/enable them but only the VFE subsystem should be in charge
> >+ * of setting the clock rates.
> >+ *
> >+ * Set the frequency tables for those clocks in the CSID resources to
> >+ * be empty so the csid subsystem doesn't overwrite the clock rates that the
> >+ * VFE already set.
> >+ */
> >  static const struct camss_subdev_resources csid_res_8250[] = {
> >      /* CSID0 */
> >      {
> >@@ -922,8 +931,8 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> >              .clock = { "vfe0_csid", "vfe0_cphy_rx", "vfe0", "vfe0_areg", "vfe0_ahb" },
> >              .clock_rate = { { 400000000 },
> >                              { 400000000 },
> >-                             { 350000000, 475000000, 576000000, 720000000 },
> >-                             { 100000000, 200000000, 300000000, 400000000 },
> >+                             { 0 },
> >+                             { 0 },
> >                              { 0 } },
> >              .reg = { "csid0" },
> >              .interrupt = { "csid0" },
> >@@ -939,8 +948,8 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> >              .clock = { "vfe1_csid", "vfe1_cphy_rx", "vfe1", "vfe1_areg", "vfe1_ahb" },
> >              .clock_rate = { { 400000000 },
> >                              { 400000000 },
> >-                             { 350000000, 475000000, 576000000, 720000000 },
> >-                             { 100000000, 200000000, 300000000, 400000000 },
> >+                             { 0 },
> >+                             { 0 },
> >                              { 0 } },
> >              .reg = { "csid1" },
> >              .interrupt = { "csid1" },
> >@@ -956,7 +965,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> >              .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
> >              .clock_rate = { { 400000000 },
> >                              { 400000000 },
> >-                             { 400000000, 480000000 },
> >+                             { 0 },
> >                              { 0 } },
> >              .reg = { "csid2" },
> >              .interrupt = { "csid2" },
> >@@ -973,7 +982,7 @@ static const struct camss_subdev_resources csid_res_8250[] = {
> >              .clock = { "vfe_lite_csid", "vfe_lite_cphy_rx", "vfe_lite",  "vfe_lite_ahb" },
> >              .clock_rate = { { 400000000 },
> >                              { 400000000 },
> >-                             { 400000000, 480000000 },
> >+                             { 0 },
> >                              { 0 } },
> >              .reg = { "csid3" },
> >              .interrupt = { "csid3" },
> 
> Hi Jordan.
> 
> Thanks for your patch. Just looking at the clocks you are zeroing here,
> I think _probably_ these zeroized clocks can be removed from the CSID
> set entirely.
> 
> Could you investigate that ?

I think that will work. It will cement the need for to VFE always run
first but we can add documentation warnings about that. It looks like
we can do this optimization for 845, sm8250 and sm2850xp.
> 
> Also please add
> 
> Fixes: b4436a18eedb ("media: camss: add support for SM8250 camss") and
> cc stable@vger.kernel.org

Will do.

Jordan