diff mbox series

[06/10] media: ar0521: Configure pixel rate using LINK_FREQ

Message ID 20221005190613.394277-7-jacopo@jmondi.org (mailing list archive)
State Changes Requested
Headers
Series media: ar0521: Add analog gain, rework clock tree |

Commit Message

Jacopo Mondi Oct. 5, 2022, 7:06 p.m. UTC 
  Now that the V4L2_LINK_FREQUENCY control is available, use it to
configure the sensor pixel rate.

Adjust the default pixel rate to match the first listed link_frequency.

Validated with:

$ v4l2-ctl -l -d /dev/v4l-subdev3
 link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=0
 pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=207000000 flags=read-only

26.493166 (30.78 fps) cam0-stream0 seq: 000008 bytesused: 1843200
26.525629 (30.80 fps) cam0-stream0 seq: 000009 bytesused: 1843200

$ yavta -w "0x009f0901 1" /dev/v4l-subdev3
$ v4l2-ctl -l -d /dev/v4l-subdev3
 link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=1
 pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=414000000 flags=read-only

54.700859 (61.37 fps) cam0-stream0 seq: 000039 bytesused: 1843200
54.717192 (61.23 fps) cam0-stream0 seq: 000040 bytesused: 1843200

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ar0521.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

Comments

kernel test robot Oct. 6, 2022, 5:51 a.m. UTC | #1 
Hi Jacopo,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.0 next-20221005]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacopo-Mondi/media-ar0521-Add-analog-gain-rework-clock-tree/20221006-030859
base:   git://linuxtv.org/media_tree.git master
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/fcdb5eaf45546fb67ef4257538539ee1e5ca7824
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jacopo-Mondi/media-ar0521-Add-analog-gain-rework-clock-tree/20221006-030859
        git checkout fcdb5eaf45546fb67ef4257538539ee1e5ca7824
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/media/i2c/ar0521.o: in function `ar0521_calc_mode':
>> ar0521.c:(.text+0x4ec): undefined reference to `__divdi3'
   `.exit.text' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
Sakari Ailus Oct. 6, 2022, 3:42 p.m. UTC | #2 
Hi Jacopo,

On Wed, Oct 05, 2022 at 09:06:09PM +0200, Jacopo Mondi wrote:
> Now that the V4L2_LINK_FREQUENCY control is available, use it to
> configure the sensor pixel rate.
> 
> Adjust the default pixel rate to match the first listed link_frequency.
> 
> Validated with:
> 
> $ v4l2-ctl -l -d /dev/v4l-subdev3
>  link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=0
>  pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=207000000 flags=read-only
> 
> 26.493166 (30.78 fps) cam0-stream0 seq: 000008 bytesused: 1843200
> 26.525629 (30.80 fps) cam0-stream0 seq: 000009 bytesused: 1843200
> 
> $ yavta -w "0x009f0901 1" /dev/v4l-subdev3
> $ v4l2-ctl -l -d /dev/v4l-subdev3
>  link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=1
>  pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=414000000 flags=read-only
> 
> 54.700859 (61.37 fps) cam0-stream0 seq: 000039 bytesused: 1843200
> 54.717192 (61.23 fps) cam0-stream0 seq: 000040 bytesused: 1843200
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ar0521.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index c5410b091654..b1580c99f5e3 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -24,7 +24,7 @@
>  #define AR0521_PLL_MAX		(1280 * 1000 * 1000)
>  
>  /* Effective pixel sample rate on the pixel array. */
> -#define AR0521_PIXEL_CLOCK_RATE	 (184 * 1000 * 1000)
> +#define AR0521_PIXEL_CLOCK_RATE	 (207 * 1000 * 1000)
>  #define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
>  #define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
>  
> @@ -91,7 +91,10 @@ static const char * const ar0521_supply_names[] = {
>  };
>  
>  static const s64 ar0521_link_frequencies[] = {
> -	184000000,
> +	/* 30 FPS at full resolution */
> +	207000000,
> +	/* 60 FPS at full resolution */
> +	414000000,
>  };
>  
>  struct ar0521_ctrls {
> @@ -330,10 +333,21 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
>  static void ar0521_calc_mode(struct ar0521_dev *sensor)
>  {
>  	unsigned int pixel_clock;
> +	unsigned int link_freq;
> +	s64 frequency;
> +	u32 pixel_rate;
>  	u16 pre, mult;
>  	u32 vco;
>  	int bpp;
>  
> +	/* Update the PIXEL_RATE value using the desired link_frequency. */
> +	bpp = ar0521_code_to_bpp(sensor);
> +	link_freq = sensor->ctrls.link_freq->val;
> +	frequency = ar0521_link_frequencies[link_freq];
> +	pixel_rate = frequency * sensor->lane_count * 2 / bpp;
> +
> +	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixrate, pixel_rate);
> +
>  	/*
>  	 * PLL1 and PLL2 are computed equally even if the application note
>  	 * suggests a slower PLL1 clock. Maintain pll1 and pll2 divider and
> @@ -380,8 +394,7 @@ static void ar0521_calc_mode(struct ar0521_dev *sensor)
>  	 * VCO not to exceed the limits specified by the datasheet and
>  	 * consequentially reduce the obtained pixel clock.
>  	 */
> -	pixel_clock = AR0521_PIXEL_CLOCK_RATE * 2 / sensor->lane_count;
> -	bpp = ar0521_code_to_bpp(sensor);
> +	pixel_clock = pixel_rate * 2 / sensor->lane_count;
>  	sensor->pll.vt_pix = bpp / 2;
>  	vco = pixel_clock * sensor->pll.vt_pix;
>  

I checked how the driver validates the link frequencies and it seems this
one doesn't. It's also missing from DT bindings.

Would you be kind enough to add it?

Thanks.
Krzysztof Hałasa Oct. 7, 2022, 5:52 a.m. UTC | #3 
Jacopo Mondi <jacopo@jmondi.org> writes:

> Now that the V4L2_LINK_FREQUENCY control is available, use it to
> configure the sensor pixel rate.
>
> Adjust the default pixel rate to match the first listed link_frequency.
> @@ -330,10 +333,21 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
>  static void ar0521_calc_mode(struct ar0521_dev *sensor)
>  {
>  	unsigned int pixel_clock;
> +	unsigned int link_freq;
> +	s64 frequency;
> +	u32 pixel_rate;
>  	u16 pre, mult;
>  	u32 vco;
>  	int bpp;
>  
> +	/* Update the PIXEL_RATE value using the desired link_frequency. */
> +	bpp = ar0521_code_to_bpp(sensor);
> +	link_freq = sensor->ctrls.link_freq->val;
> +	frequency = ar0521_link_frequencies[link_freq];
> +	pixel_rate = frequency * sensor->lane_count * 2 / bpp;

Must be that div64 thing.

> +
> +	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixrate, pixel_rate);
> +
Jacopo Mondi Oct. 7, 2022, 7:25 a.m. UTC | #4 
Hi Sakari

On Thu, Oct 06, 2022 at 06:42:31PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Oct 05, 2022 at 09:06:09PM +0200, Jacopo Mondi wrote:
> > Now that the V4L2_LINK_FREQUENCY control is available, use it to
> > configure the sensor pixel rate.
> >
> > Adjust the default pixel rate to match the first listed link_frequency.
> >
> > Validated with:
> >
> > $ v4l2-ctl -l -d /dev/v4l-subdev3
> >  link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=0
> >  pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=207000000 flags=read-only
> >
> > 26.493166 (30.78 fps) cam0-stream0 seq: 000008 bytesused: 1843200
> > 26.525629 (30.80 fps) cam0-stream0 seq: 000009 bytesused: 1843200
> >
> > $ yavta -w "0x009f0901 1" /dev/v4l-subdev3
> > $ v4l2-ctl -l -d /dev/v4l-subdev3
> >  link_frequency 0x009f0901 (intmenu): min=0 max=1 default=0 value=1
> >  pixel_rate 0x009f0902 (int64)  : min=168000000 max=414000000 step=1 default=414000000 value=414000000 flags=read-only
> >
> > 54.700859 (61.37 fps) cam0-stream0 seq: 000039 bytesused: 1843200
> > 54.717192 (61.23 fps) cam0-stream0 seq: 000040 bytesused: 1843200
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ar0521.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> > index c5410b091654..b1580c99f5e3 100644
> > --- a/drivers/media/i2c/ar0521.c
> > +++ b/drivers/media/i2c/ar0521.c
> > @@ -24,7 +24,7 @@
> >  #define AR0521_PLL_MAX		(1280 * 1000 * 1000)
> >
> >  /* Effective pixel sample rate on the pixel array. */
> > -#define AR0521_PIXEL_CLOCK_RATE	 (184 * 1000 * 1000)
> > +#define AR0521_PIXEL_CLOCK_RATE	 (207 * 1000 * 1000)
> >  #define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
> >  #define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
> >
> > @@ -91,7 +91,10 @@ static const char * const ar0521_supply_names[] = {
> >  };
> >
> >  static const s64 ar0521_link_frequencies[] = {
> > -	184000000,
> > +	/* 30 FPS at full resolution */
> > +	207000000,
> > +	/* 60 FPS at full resolution */
> > +	414000000,
> >  };
> >
> >  struct ar0521_ctrls {
> > @@ -330,10 +333,21 @@ static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
> >  static void ar0521_calc_mode(struct ar0521_dev *sensor)
> >  {
> >  	unsigned int pixel_clock;
> > +	unsigned int link_freq;
> > +	s64 frequency;
> > +	u32 pixel_rate;
> >  	u16 pre, mult;
> >  	u32 vco;
> >  	int bpp;
> >
> > +	/* Update the PIXEL_RATE value using the desired link_frequency. */
> > +	bpp = ar0521_code_to_bpp(sensor);
> > +	link_freq = sensor->ctrls.link_freq->val;
> > +	frequency = ar0521_link_frequencies[link_freq];
> > +	pixel_rate = frequency * sensor->lane_count * 2 / bpp;
> > +
> > +	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixrate, pixel_rate);
> > +
> >  	/*
> >  	 * PLL1 and PLL2 are computed equally even if the application note
> >  	 * suggests a slower PLL1 clock. Maintain pll1 and pll2 divider and
> > @@ -380,8 +394,7 @@ static void ar0521_calc_mode(struct ar0521_dev *sensor)
> >  	 * VCO not to exceed the limits specified by the datasheet and
> >  	 * consequentially reduce the obtained pixel clock.
> >  	 */
> > -	pixel_clock = AR0521_PIXEL_CLOCK_RATE * 2 / sensor->lane_count;
> > -	bpp = ar0521_code_to_bpp(sensor);
> > +	pixel_clock = pixel_rate * 2 / sensor->lane_count;
> >  	sensor->pll.vt_pix = bpp / 2;
> >  	vco = pixel_clock * sensor->pll.vt_pix;
> >
>
> I checked how the driver validates the link frequencies and it seems this
> one doesn't. It's also missing from DT bindings.

As Dave noted, I don't have an handler for LINK_FREQ in s_ctrl.
I will add one

>
> Would you be kind enough to add it?

And add the property to bindings..

>
> Thanks.
>
> --
> Kind regards,
>
> Sakari Ailus
diff mbox series

Patch

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index c5410b091654..b1580c99f5e3 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -24,7 +24,7 @@ 
 #define AR0521_PLL_MAX		(1280 * 1000 * 1000)
 
 /* Effective pixel sample rate on the pixel array. */
-#define AR0521_PIXEL_CLOCK_RATE	 (184 * 1000 * 1000)
+#define AR0521_PIXEL_CLOCK_RATE	 (207 * 1000 * 1000)
 #define AR0521_PIXEL_CLOCK_MIN	 (168 * 1000 * 1000)
 #define AR0521_PIXEL_CLOCK_MAX	 (414 * 1000 * 1000)
 
@@ -91,7 +91,10 @@  static const char * const ar0521_supply_names[] = {
 };
 
 static const s64 ar0521_link_frequencies[] = {
-	184000000,
+	/* 30 FPS at full resolution */
+	207000000,
+	/* 60 FPS at full resolution */
+	414000000,
 };
 
 struct ar0521_ctrls {
@@ -330,10 +333,21 @@  static u32 calc_pll(struct ar0521_dev *sensor, u32 freq, u16 *pre_ptr, u16 *mult
 static void ar0521_calc_mode(struct ar0521_dev *sensor)
 {
 	unsigned int pixel_clock;
+	unsigned int link_freq;
+	s64 frequency;
+	u32 pixel_rate;
 	u16 pre, mult;
 	u32 vco;
 	int bpp;
 
+	/* Update the PIXEL_RATE value using the desired link_frequency. */
+	bpp = ar0521_code_to_bpp(sensor);
+	link_freq = sensor->ctrls.link_freq->val;
+	frequency = ar0521_link_frequencies[link_freq];
+	pixel_rate = frequency * sensor->lane_count * 2 / bpp;
+
+	__v4l2_ctrl_s_ctrl_int64(sensor->ctrls.pixrate, pixel_rate);
+
 	/*
 	 * PLL1 and PLL2 are computed equally even if the application note
 	 * suggests a slower PLL1 clock. Maintain pll1 and pll2 divider and
@@ -380,8 +394,7 @@  static void ar0521_calc_mode(struct ar0521_dev *sensor)
 	 * VCO not to exceed the limits specified by the datasheet and
 	 * consequentially reduce the obtained pixel clock.
 	 */
-	pixel_clock = AR0521_PIXEL_CLOCK_RATE * 2 / sensor->lane_count;
-	bpp = ar0521_code_to_bpp(sensor);
+	pixel_clock = pixel_rate * 2 / sensor->lane_count;
 	sensor->pll.vt_pix = bpp / 2;
 	vco = pixel_clock * sensor->pll.vt_pix;