| Message ID | 20221005190613.394277-7-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ar0521: Add analog gain, rework clock tree | expand |
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
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.
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); > +
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 --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;
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(-)