diff mbox series

[07/10] media: ar0521: Adjust exposure and blankings limits

Message ID 20221005190613.394277-8-jacopo@jmondi.org (mailing list archive)
State New, archived
Headers show
Series media: ar0521: Add analog gain, rework clock tree | expand

Commit Message

Jacopo Mondi Oct. 5, 2022, 7:06 p.m. UTC 
Adjust the control limits for V4L2_CID_VBLANK, V4L2_CID_HBLANK and
V4L2_CID_EXPOSURE when a new format is applied to the sensor.

Also update the exposure control when a new blanking value is applied.

Also change the controls initialization to use valid values for the
default format.

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

Comments

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

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v6.0]
[cannot apply to 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: i386-randconfig-a003-20221003
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
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/800a6425eee843e2cb1f8047accb401df327f45d
        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 800a6425eee843e2cb1f8047accb401df327f45d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/media/i2c/

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

All warnings (new ones prefixed by >>):

                                   ^
   include/linux/minmax.h:20:39: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                                ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:36:38: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                               ^
   include/linux/minmax.h:26:46: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                                                              ^
   include/linux/minmax.h:23:40: note: expanded from macro '__no_side_effects'
                   (__is_constexpr(x) && __is_constexpr(y))
                                                        ^
   include/linux/const.h:12:48: note: expanded from macro '__is_constexpr'
           (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
                                                         ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:37:12: note: expanded from macro '__careful_cmp'
                   __cmp(x, y, op), \
                            ^
   include/linux/minmax.h:28:34: note: expanded from macro '__cmp'
   #define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
                                    ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:37:12: note: expanded from macro '__careful_cmp'
                   __cmp(x, y, op), \
                            ^
   include/linux/minmax.h:28:46: note: expanded from macro '__cmp'
   #define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
                                                ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:38:17: note: expanded from macro '__careful_cmp'
                   __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
                                 ^
   include/linux/minmax.h:32:10: note: expanded from macro '__cmp_once'
                   typeof(y) unique_y = (y);               \
                          ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:38:17: note: expanded from macro '__careful_cmp'
                   __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
                                 ^
   include/linux/minmax.h:32:25: note: expanded from macro '__cmp_once'
                   typeof(y) unique_y = (y);               \
                                         ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:612:7: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                                            exp_max, sensor->ctrls.exposure->step,
                                            ^~~~~~~
                                            exp_val
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
>> drivers/media/i2c/ar0521.c:609:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   1 warning and 8 errors generated.


vim +609 drivers/media/i2c/ar0521.c

   597	
   598	static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
   599	{
   600		struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
   601		struct ar0521_dev *sensor = to_ar0521_dev(sd);
   602		int ret;
   603	
   604		/* v4l2_ctrl_lock() locks our own mutex */
   605	
   606		switch (ctrl->id) {
   607		case V4L2_CID_VBLANK:
   608			int exp_max = sensor->fmt.height + ctrl->val - 4;
 > 609			int exp_val = min(sensor->ctrls.exposure->val, exp_max);
   610			__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
   611						 sensor->ctrls.exposure->minimum,
   612						 exp_max, sensor->ctrls.exposure->step,
   613						 exp_val);
   614			break;
   615		}
   616	
   617		/* access the sensor only if it's powered up */
   618		if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
   619			return 0;
   620	
   621		switch (ctrl->id) {
   622		case V4L2_CID_HBLANK:
   623		case V4L2_CID_VBLANK:
   624			ret = ar0521_set_geometry(sensor);
   625			break;
   626		case V4L2_CID_ANALOGUE_GAIN:
   627			ret = ar0521_set_analog_gain(sensor);
   628			break;
   629		case V4L2_CID_GAIN:
   630		case V4L2_CID_RED_BALANCE:
   631		case V4L2_CID_BLUE_BALANCE:
   632			ret = ar0521_set_gains(sensor);
   633			break;
   634		case V4L2_CID_EXPOSURE:
   635			ret = ar0521_write_reg(sensor,
   636					       AR0521_REG_COARSE_INTEGRATION_TIME,
   637					       ctrl->val);
   638			break;
   639		case V4L2_CID_TEST_PATTERN:
   640			ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
   641					       ctrl->val);
   642			break;
   643		}
   644	
   645		pm_runtime_put(&sensor->i2c_client->dev);
   646		return ret;
   647	}
   648
kernel test robot Oct. 6, 2022, 4:17 a.m. UTC | #2 
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]
[cannot apply to 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: i386-randconfig-a003-20221003
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
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/800a6425eee843e2cb1f8047accb401df327f45d
        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 800a6425eee843e2cb1f8047accb401df327f45d
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 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 >>):

>> drivers/media/i2c/ar0521.c:608:3: error: expected expression
                   int exp_max = sensor->fmt.height + ctrl->val - 4;
                   ^
>> drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:36:38: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                               ^
   include/linux/minmax.h:26:19: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                                   ^
   include/linux/minmax.h:20:39: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                                                ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
>> drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:36:38: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                               ^
   include/linux/minmax.h:26:46: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                                                              ^
   include/linux/minmax.h:23:40: note: expanded from macro '__no_side_effects'
                   (__is_constexpr(x) && __is_constexpr(y))
                                                        ^
   include/linux/const.h:12:48: note: expanded from macro '__is_constexpr'
           (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
                                                         ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
>> drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:37:12: note: expanded from macro '__careful_cmp'
                   __cmp(x, y, op), \
                            ^
   include/linux/minmax.h:28:34: note: expanded from macro '__cmp'
   #define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
                                    ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
>> drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:37:12: note: expanded from macro '__careful_cmp'
                   __cmp(x, y, op), \
                            ^
   include/linux/minmax.h:28:46: note: expanded from macro '__cmp'
   #define __cmp(x, y, op) ((x) op (y) ? (x) : (y))
                                                ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
>> drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:38:17: note: expanded from macro '__careful_cmp'
                   __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
                                 ^
   include/linux/minmax.h:32:10: note: expanded from macro '__cmp_once'
                   typeof(y) unique_y = (y);               \
                          ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
>> drivers/media/i2c/ar0521.c:609:50: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                                                                  ^~~~~~~
                                                                  exp_val
   include/linux/minmax.h:45:36: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                                            ^
   include/linux/minmax.h:38:17: note: expanded from macro '__careful_cmp'
                   __cmp_once(x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y), op))
                                 ^
   include/linux/minmax.h:32:25: note: expanded from macro '__cmp_once'
                   typeof(y) unique_y = (y);               \
                                         ^
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:612:7: error: use of undeclared identifier 'exp_max'; did you mean 'exp_val'?
                                            exp_max, sensor->ctrls.exposure->step,
                                            ^~~~~~~
                                            exp_val
   drivers/media/i2c/ar0521.c:609:7: note: 'exp_val' declared here
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   drivers/media/i2c/ar0521.c:609:7: warning: mixing declarations and code is incompatible with standards before C99 [-Wdeclaration-after-statement]
                   int exp_val = min(sensor->ctrls.exposure->val, exp_max);
                       ^
   1 warning and 8 errors generated.


vim +608 drivers/media/i2c/ar0521.c

   597	
   598	static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
   599	{
   600		struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
   601		struct ar0521_dev *sensor = to_ar0521_dev(sd);
   602		int ret;
   603	
   604		/* v4l2_ctrl_lock() locks our own mutex */
   605	
   606		switch (ctrl->id) {
   607		case V4L2_CID_VBLANK:
 > 608			int exp_max = sensor->fmt.height + ctrl->val - 4;
 > 609			int exp_val = min(sensor->ctrls.exposure->val, exp_max);
   610			__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
   611						 sensor->ctrls.exposure->minimum,
   612						 exp_max, sensor->ctrls.exposure->step,
   613						 exp_val);
   614			break;
   615		}
   616	
   617		/* access the sensor only if it's powered up */
   618		if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
   619			return 0;
   620	
   621		switch (ctrl->id) {
   622		case V4L2_CID_HBLANK:
   623		case V4L2_CID_VBLANK:
   624			ret = ar0521_set_geometry(sensor);
   625			break;
   626		case V4L2_CID_ANALOGUE_GAIN:
   627			ret = ar0521_set_analog_gain(sensor);
   628			break;
   629		case V4L2_CID_GAIN:
   630		case V4L2_CID_RED_BALANCE:
   631		case V4L2_CID_BLUE_BALANCE:
   632			ret = ar0521_set_gains(sensor);
   633			break;
   634		case V4L2_CID_EXPOSURE:
   635			ret = ar0521_write_reg(sensor,
   636					       AR0521_REG_COARSE_INTEGRATION_TIME,
   637					       ctrl->val);
   638			break;
   639		case V4L2_CID_TEST_PATTERN:
   640			ret = ar0521_write_reg(sensor, AR0521_REG_TEST_PATTERN_MODE,
   641					       ctrl->val);
   642			break;
   643		}
   644	
   645		pm_runtime_put(&sensor->i2c_client->dev);
   646		return ret;
   647	}
   648
Dave Stevenson Oct. 6, 2022, 3:41 p.m. UTC | #3 
Hi Jacopo

On Wed, 5 Oct 2022 at 20:07, Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Adjust the control limits for V4L2_CID_VBLANK, V4L2_CID_HBLANK and
> V4L2_CID_EXPOSURE when a new format is applied to the sensor.
>
> Also update the exposure control when a new blanking value is applied.
>
> Also change the controls initialization to use valid values for the
> default format.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ar0521.c | 77 ++++++++++++++++++++++++++++----------
>  1 file changed, 57 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
> index b1580c99f5e3..26bb1b8f7453 100644
> --- a/drivers/media/i2c/ar0521.c
> +++ b/drivers/media/i2c/ar0521.c
> @@ -42,7 +42,8 @@
>
>  #define AR0521_WIDTH_BLANKING_MIN     572u
>  #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
> -#define AR0521_TOTAL_WIDTH_MIN      2968u
> +#define AR0521_TOTAL_HEIGHT_MAX      2464u /* max value of y_addr_end reg */
> +#define AR0521_TOTAL_WIDTH_MAX       3280u /* max value of x_addr_end reg */
>
>  #define AR0521_ANA_GAIN_MIN            0x00
>  #define AR0521_ANA_GAIN_MAX            0x3f
> @@ -131,8 +132,6 @@ struct ar0521_dev {
>         struct v4l2_mbus_framefmt fmt;
>         struct ar0521_ctrls ctrls;
>         unsigned int lane_count;
> -       u16 total_width;
> -       u16 total_height;
>         struct {
>                 u16 pre;
>                 u16 mult;
> @@ -544,7 +543,8 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
>                           struct v4l2_subdev_format *format)
>  {
>         struct ar0521_dev *sensor = to_ar0521_dev(sd);
> -       int ret = 0;
> +       int exposure_max, exposure_val;
> +       int max_vblank, max_hblank;
>
>         ar0521_adj_fmt(&format->format);
>
> @@ -555,13 +555,44 @@ static int ar0521_set_fmt(struct v4l2_subdev *sd,
>
>                 fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
>                 *fmt = format->format;
> -       } else {
> -               sensor->fmt = format->format;
> -               ar0521_calc_mode(sensor);
> +
> +               mutex_unlock(&sensor->lock);
> +
> +               return 0;
>         }
>
> +       sensor->fmt = format->format;
> +       ar0521_calc_mode(sensor);
> +
> +       /*
> +        * Update the exposure and blankings limits. Blankings are also reset
> +        * to the minimum.
> +        */
> +       max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
> +       __v4l2_ctrl_modify_range(sensor->ctrls.hblank,
> +                                sensor->ctrls.hblank->minimum,
> +                                max_hblank, sensor->ctrls.hblank->step,
> +                                sensor->ctrls.hblank->minimum);
> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.hblank->minimum);
> +
> +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
> +       __v4l2_ctrl_modify_range(sensor->ctrls.vblank,
> +                                sensor->ctrls.vblank->minimum,
> +                                max_vblank, sensor->ctrls.vblank->step,
> +                                sensor->ctrls.vblank->minimum);
> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->minimum);
> +
> +       exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
> +       exposure_val = min(sensor->ctrls.exposure->val, exposure_max);
> +       __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> +                                sensor->ctrls.exposure->minimum,
> +                                exposure_max, sensor->ctrls.exposure->step,
> +                                exposure_val);
> +       __v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, exposure_val);

Is it correct to set the value of VBLANK, HBLANK, and EXPOSURE when
changing mode? The control handler framework will handle resetting the
value if it is out of range, but my understanding is that otherwise
they should be left alone.

This is the one I've just hit with OV9282 and trying to test a
configurable HBLANK when libcamera didn't support it. Set HBLANK with
v4l2-ctl --set-ctrl, then run a libcamera app and it gets reset
because libcamera chose a mode.

> +
>         mutex_unlock(&sensor->lock);
> -       return ret;
> +
> +       return 0;
>  }
>
>  static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -573,15 +604,13 @@ static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>         /* v4l2_ctrl_lock() locks our own mutex */
>
>         switch (ctrl->id) {
> -       case V4L2_CID_HBLANK:
>         case V4L2_CID_VBLANK:
> -               sensor->total_width = sensor->fmt.width +
> -                       sensor->ctrls.hblank->val;
> -               sensor->total_height = sensor->fmt.width +
> -                       sensor->ctrls.vblank->val;
> -               break;
> -       default:
> -               ret = -EINVAL;
> +               int exp_max = sensor->fmt.height + ctrl->val - 4;
> +               int exp_val = min(sensor->ctrls.exposure->val, exp_max);
> +               __v4l2_ctrl_modify_range(sensor->ctrls.exposure,
> +                                        sensor->ctrls.exposure->minimum,
> +                                        exp_max, sensor->ctrls.exposure->step,
> +                                        exp_val);
>                 break;
>         }
>
> @@ -633,6 +662,7 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>         const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
>         struct ar0521_ctrls *ctrls = &sensor->ctrls;
>         struct v4l2_ctrl_handler *hdl = &ctrls->handler;
> +       int max_vblank, max_hblank, exposure_max;
>         int ret;
>
>         v4l2_ctrl_handler_init(hdl, 32);
> @@ -655,11 +685,17 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>                                                 -512, 511, 1, 0);
>         v4l2_ctrl_cluster(3, &ctrls->gain);
>
> +       /* Initialize blanking limits using the default 2592x1944 format. */
> +       max_hblank = AR0521_TOTAL_WIDTH_MAX - AR0521_WIDTH_MAX;
>         ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
> -                                         AR0521_WIDTH_BLANKING_MIN, 4094, 1,
> +                                         AR0521_WIDTH_BLANKING_MIN,
> +                                         max_hblank, 1,
>                                           AR0521_WIDTH_BLANKING_MIN);
> +
> +       max_vblank = AR0521_TOTAL_HEIGHT_MAX - AR0521_HEIGHT_MAX;

This looks lower than it should be.
AR0521_TOTAL_HEIGHT_MAX = 2464 ("max value of y_addr_end reg")
AR0521_HEIGHT_MAX = 1958
Max is therefore 506.

Except that the frame height + blanking is written to
FRAME_LENGTH_LINES (0x0340), and not y_addr_end (0x034a).
The maximum value permitted for FRAME_LENGTH_LINES is 65535, so
vblank_max = 65535 - 1958 = 63577.

>         ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
> -                                         AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
> +                                         AR0521_HEIGHT_BLANKING_MIN,
> +                                         max_vblank, 2,

Curious that the total height with blanking has to be a multiple of 2.

>                                           AR0521_HEIGHT_BLANKING_MIN);
>         v4l2_ctrl_cluster(2, &ctrls->hblank);
>
> @@ -669,9 +705,10 @@ static int ar0521_init_controls(struct ar0521_dev *sensor)
>                                            AR0521_PIXEL_CLOCK_MAX, 1,
>                                            AR0521_PIXEL_CLOCK_RATE);
>
> -       /* Manual exposure time */
> +       /* Manual exposure time: max exposure time = visible + blank - 4 */
> +       exposure_max = AR0521_HEIGHT_MAX + AR0521_HEIGHT_BLANKING_MIN - 4;
>         ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
> -                                           65535, 1, 360);
> +                                           exposure_max, 1, 360);
>
>         ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
>                                         ARRAY_SIZE(ar0521_link_frequencies) - 1,
> --
> 2.37.3
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ar0521.c b/drivers/media/i2c/ar0521.c
index b1580c99f5e3..26bb1b8f7453 100644
--- a/drivers/media/i2c/ar0521.c
+++ b/drivers/media/i2c/ar0521.c
@@ -42,7 +42,8 @@ 
 
 #define AR0521_WIDTH_BLANKING_MIN     572u
 #define AR0521_HEIGHT_BLANKING_MIN     38u /* must be even */
-#define AR0521_TOTAL_WIDTH_MIN	     2968u
+#define AR0521_TOTAL_HEIGHT_MAX      2464u /* max value of y_addr_end reg */
+#define AR0521_TOTAL_WIDTH_MAX       3280u /* max value of x_addr_end reg */
 
 #define AR0521_ANA_GAIN_MIN		0x00
 #define AR0521_ANA_GAIN_MAX		0x3f
@@ -131,8 +132,6 @@  struct ar0521_dev {
 	struct v4l2_mbus_framefmt fmt;
 	struct ar0521_ctrls ctrls;
 	unsigned int lane_count;
-	u16 total_width;
-	u16 total_height;
 	struct {
 		u16 pre;
 		u16 mult;
@@ -544,7 +543,8 @@  static int ar0521_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *format)
 {
 	struct ar0521_dev *sensor = to_ar0521_dev(sd);
-	int ret = 0;
+	int exposure_max, exposure_val;
+	int max_vblank, max_hblank;
 
 	ar0521_adj_fmt(&format->format);
 
@@ -555,13 +555,44 @@  static int ar0521_set_fmt(struct v4l2_subdev *sd,
 
 		fmt = v4l2_subdev_get_try_format(sd, sd_state, 0 /* pad */);
 		*fmt = format->format;
-	} else {
-		sensor->fmt = format->format;
-		ar0521_calc_mode(sensor);
+
+		mutex_unlock(&sensor->lock);
+
+		return 0;
 	}
 
+	sensor->fmt = format->format;
+	ar0521_calc_mode(sensor);
+
+	/*
+	 * Update the exposure and blankings limits. Blankings are also reset
+	 * to the minimum.
+	 */
+	max_hblank = AR0521_TOTAL_WIDTH_MAX - sensor->fmt.width;
+	__v4l2_ctrl_modify_range(sensor->ctrls.hblank,
+				 sensor->ctrls.hblank->minimum,
+				 max_hblank, sensor->ctrls.hblank->step,
+				 sensor->ctrls.hblank->minimum);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.hblank->minimum);
+
+	max_vblank = AR0521_TOTAL_HEIGHT_MAX - sensor->fmt.height;
+	__v4l2_ctrl_modify_range(sensor->ctrls.vblank,
+				 sensor->ctrls.vblank->minimum,
+				 max_vblank, sensor->ctrls.vblank->step,
+				 sensor->ctrls.vblank->minimum);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.vblank, sensor->ctrls.vblank->minimum);
+
+	exposure_max = sensor->fmt.height + AR0521_HEIGHT_BLANKING_MIN - 4;
+	exposure_val = min(sensor->ctrls.exposure->val, exposure_max);
+	__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+				 sensor->ctrls.exposure->minimum,
+				 exposure_max, sensor->ctrls.exposure->step,
+				 exposure_val);
+	__v4l2_ctrl_s_ctrl(sensor->ctrls.exposure, exposure_val);
+
 	mutex_unlock(&sensor->lock);
-	return ret;
+
+	return 0;
 }
 
 static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -573,15 +604,13 @@  static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
 	/* v4l2_ctrl_lock() locks our own mutex */
 
 	switch (ctrl->id) {
-	case V4L2_CID_HBLANK:
 	case V4L2_CID_VBLANK:
-		sensor->total_width = sensor->fmt.width +
-			sensor->ctrls.hblank->val;
-		sensor->total_height = sensor->fmt.width +
-			sensor->ctrls.vblank->val;
-		break;
-	default:
-		ret = -EINVAL;
+		int exp_max = sensor->fmt.height + ctrl->val - 4;
+		int exp_val = min(sensor->ctrls.exposure->val, exp_max);
+		__v4l2_ctrl_modify_range(sensor->ctrls.exposure,
+					 sensor->ctrls.exposure->minimum,
+					 exp_max, sensor->ctrls.exposure->step,
+					 exp_val);
 		break;
 	}
 
@@ -633,6 +662,7 @@  static int ar0521_init_controls(struct ar0521_dev *sensor)
 	const struct v4l2_ctrl_ops *ops = &ar0521_ctrl_ops;
 	struct ar0521_ctrls *ctrls = &sensor->ctrls;
 	struct v4l2_ctrl_handler *hdl = &ctrls->handler;
+	int max_vblank, max_hblank, exposure_max;
 	int ret;
 
 	v4l2_ctrl_handler_init(hdl, 32);
@@ -655,11 +685,17 @@  static int ar0521_init_controls(struct ar0521_dev *sensor)
 						-512, 511, 1, 0);
 	v4l2_ctrl_cluster(3, &ctrls->gain);
 
+	/* Initialize blanking limits using the default 2592x1944 format. */
+	max_hblank = AR0521_TOTAL_WIDTH_MAX - AR0521_WIDTH_MAX;
 	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
-					  AR0521_WIDTH_BLANKING_MIN, 4094, 1,
+					  AR0521_WIDTH_BLANKING_MIN,
+					  max_hblank, 1,
 					  AR0521_WIDTH_BLANKING_MIN);
+
+	max_vblank = AR0521_TOTAL_HEIGHT_MAX - AR0521_HEIGHT_MAX;
 	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
-					  AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
+					  AR0521_HEIGHT_BLANKING_MIN,
+					  max_vblank, 2,
 					  AR0521_HEIGHT_BLANKING_MIN);
 	v4l2_ctrl_cluster(2, &ctrls->hblank);
 
@@ -669,9 +705,10 @@  static int ar0521_init_controls(struct ar0521_dev *sensor)
 					   AR0521_PIXEL_CLOCK_MAX, 1,
 					   AR0521_PIXEL_CLOCK_RATE);
 
-	/* Manual exposure time */
+	/* Manual exposure time: max exposure time = visible + blank - 4 */
+	exposure_max = AR0521_HEIGHT_MAX + AR0521_HEIGHT_BLANKING_MIN - 4;
 	ctrls->exposure = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_EXPOSURE, 0,
-					    65535, 1, 360);
+					    exposure_max, 1, 360);
 
 	ctrls->link_freq = v4l2_ctrl_new_int_menu(hdl, ops, V4L2_CID_LINK_FREQ,
 					ARRAY_SIZE(ar0521_link_frequencies) - 1,