| Message ID | 155aba6ab759e98f66349e6bb4f69e2410486c09.1722084704.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | Accepted |
| Commit | 81ee62e8d09ee3c7107d11c8bbfd64073ab601ad |
| Headers | show |
| Series | media: atomisp: Use clamp_t() in ia_css_eed1_8_vmem_encode() | expand |
From: Christophe JAILLET > Sent: 27 July 2024 13:52 > > Using clamp_t() instead of min_t(max_t()) is easier to read. It shouldn't need to be clamp_t(), a plain clamp() looks to be ok. (dew_enhance_seg_x is s32). David > > It also reduces the size of the preprocessed files by ~ 193 ko. > (see [1] for a discussion about it) > > $ ls -l ia_css_eed1_8.host*.i > 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i > 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i > > [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > index e4fc90f88e24..96c13ebc4331 100644 > --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( > base = shuffle_block * i; > > for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_x[j], 0), > - 8191); > - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_y[j], -8192), > - 8191); > + to->e_dew_enh_x[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_x[j], > + 0, 8191); > + to->e_dew_enh_y[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_y[j], > + -8192, 8191); > } > > for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { > - to->e_dew_enh_a[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_slope[j], > - -8192), 8191); > + to->e_dew_enh_a[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_slope[j], > + -8192, 8191); > /* Convert dew_enhance_seg_exp to flag: > * 0 -> 0 > * 1...13 -> 1 > */ > - to->e_dew_enh_f[0][base + j] = (min_t(int, max_t(int, > - from->dew_enhance_seg_exp[j], > - 0), 13) > 0); > + to->e_dew_enh_f[0][base + j] = (clamp_t(int, > + from->dew_enhance_seg_exp[j], > + 0, 13) > 0); > } > > /* Hard-coded to 0, in order to be able to handle out of > -- > 2.45.2 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi, On 7/27/24 2:51 PM, Christophe JAILLET wrote: > Using clamp_t() instead of min_t(max_t()) is easier to read. > > It also reduces the size of the preprocessed files by ~ 193 ko. > (see [1] for a discussion about it) > > $ ls -l ia_css_eed1_8.host*.i > 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i > 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i > > [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Thank you for your patch(es). I have merged this/these in my media-atomisp branch: https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp And this/these will be included in my next pull-request to Mauro (to media subsystem maintainer) Regards, Hans > --- > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > index e4fc90f88e24..96c13ebc4331 100644 > --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( > base = shuffle_block * i; > > for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_x[j], 0), > - 8191); > - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_y[j], -8192), > - 8191); > + to->e_dew_enh_x[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_x[j], > + 0, 8191); > + to->e_dew_enh_y[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_y[j], > + -8192, 8191); > } > > for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { > - to->e_dew_enh_a[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_slope[j], > - -8192), 8191); > + to->e_dew_enh_a[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_slope[j], > + -8192, 8191); > /* Convert dew_enhance_seg_exp to flag: > * 0 -> 0 > * 1...13 -> 1 > */ > - to->e_dew_enh_f[0][base + j] = (min_t(int, max_t(int, > - from->dew_enhance_seg_exp[j], > - 0), 13) > 0); > + to->e_dew_enh_f[0][base + j] = (clamp_t(int, > + from->dew_enhance_seg_exp[j], > + 0, 13) > 0); > } > > /* Hard-coded to 0, in order to be able to handle out of
Em Sat, 27 Jul 2024 14:51:56 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> escreveu: > Using clamp_t() instead of min_t(max_t()) is easier to read. > > It also reduces the size of the preprocessed files by ~ 193 ko. > (see [1] for a discussion about it) > > $ ls -l ia_css_eed1_8.host*.i > 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i > 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i > > [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > index e4fc90f88e24..96c13ebc4331 100644 > --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( > base = shuffle_block * i; > > for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_x[j], 0), > - 8191); > - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, > - from->dew_enhance_seg_y[j], -8192), > - 8191); > + to->e_dew_enh_x[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_x[j], > + 0, 8191); > + to->e_dew_enh_y[0][base + j] = clamp_t(int, > + from->dew_enhance_seg_y[j], > + -8192, 8191); Such change introduces two warnings on smatch: drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + j]' drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + j]' Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? Thanks, Mauro
Hi Mauro, On 9/6/24 8:15 AM, Mauro Carvalho Chehab wrote: > Em Sat, 27 Jul 2024 14:51:56 +0200 > Christophe JAILLET <christophe.jaillet@wanadoo.fr> escreveu: > >> Using clamp_t() instead of min_t(max_t()) is easier to read. >> >> It also reduces the size of the preprocessed files by ~ 193 ko. >> (see [1] for a discussion about it) >> >> $ ls -l ia_css_eed1_8.host*.i >> 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i >> 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i >> >> [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- >> 1 file changed, 12 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> index e4fc90f88e24..96c13ebc4331 100644 >> --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( >> base = shuffle_block * i; >> >> for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { >> - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, >> - from->dew_enhance_seg_x[j], 0), >> - 8191); >> - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, >> - from->dew_enhance_seg_y[j], -8192), >> - 8191); >> + to->e_dew_enh_x[0][base + j] = clamp_t(int, >> + from->dew_enhance_seg_x[j], >> + 0, 8191); >> + to->e_dew_enh_y[0][base + j] = clamp_t(int, >> + from->dew_enhance_seg_y[j], >> + -8192, 8191); > > Such change introduces two warnings on smatch: > > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + j]' > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + j]' > > Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? These already are s32, the problem is that e_dew_enh_a is of type t_vmem_elem which is: typedef u16 t_vmem_elem; And that type is used in a lot of places, so we cannot just change that. I guess we could add a t_signed_vmem_elem (s16) and use that for these vmem-arrays ? I tried fixing it like this: diff --git a/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c b/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c index 1e01d354152b..7c0195d15f53 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c @@ -428,18 +428,13 @@ static int t4ka3_s_stream(struct v4l2_subdev *sd, int enable) goto error_unlock; } - ret = cci_multi_reg_write(sensor->regmap, t4ka3_init_config, - ARRAY_SIZE(t4ka3_init_config), NULL); - if (ret) - goto error_powerdown; - + cci_multi_reg_write(sensor->regmap, t4ka3_init_config, + ARRAY_SIZE(t4ka3_init_config), &ret); /* enable group hold */ - ret = cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, - ARRAY_SIZE(t4ka3_param_hold), NULL); - if (ret) - goto error_powerdown; - - ret = cci_multi_reg_write(sensor->regmap, sensor->res->regs, sensor->res->regs_len, NULL); + cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, + ARRAY_SIZE(t4ka3_param_hold), &ret); + cci_multi_reg_write(sensor->regmap, sensor->res->regs, + sensor->res->regs_len, &ret); if (ret) goto error_powerdown; diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c index b79d78e5b77f..c9043d516192 100644 --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c @@ -172,21 +172,21 @@ ia_css_eed1_8_vmem_encode( base = shuffle_block * i; for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { - to->e_dew_enh_x[0][base + j] = clamp(from->dew_enhance_seg_x[j], - 0, 8191); - to->e_dew_enh_y[0][base + j] = clamp(from->dew_enhance_seg_y[j], - -8192, 8191); + to->e_dew_enh_x[0][base + j] = (u16)clamp(from->dew_enhance_seg_x[j], + 0, 8191); + to->e_dew_enh_y[0][base + j] = (u16)clamp(from->dew_enhance_seg_y[j], + -8192, 8191); } for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { - to->e_dew_enh_a[0][base + j] = clamp(from->dew_enhance_seg_slope[j], - -8192, 8191); + to->e_dew_enh_a[0][base + j] = (u16)clamp(from->dew_enhance_seg_slope[j], + -8192, 8191); /* Convert dew_enhance_seg_exp to flag: * 0 -> 0 * 1...13 -> 1 */ - to->e_dew_enh_f[0][base + j] = clamp(from->dew_enhance_seg_exp[j], - 0, 13) > 0; + to->e_dew_enh_f[0][base + j] = (u16)clamp(from->dew_enhance_seg_exp[j], + 0, 13) > 0; } /* Hard-coded to 0, in order to be able to handle out of but smatch still complains after this... Regards, Hans
From: Mauro Carvalho Chehab > Sent: 06 September 2024 07:16 > > Em Sat, 27 Jul 2024 14:51:56 +0200 > Christophe JAILLET <christophe.jaillet@wanadoo.fr> escreveu: > > > Using clamp_t() instead of min_t(max_t()) is easier to read. > > > > It also reduces the size of the preprocessed files by ~ 193 ko. > > (see [1] for a discussion about it) > > > > $ ls -l ia_css_eed1_8.host*.i > > 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i > > 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i > > > > [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > > index e4fc90f88e24..96c13ebc4331 100644 > > --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > > @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( > > base = shuffle_block * i; > > > > for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > > - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, > > - from->dew_enhance_seg_x[j], 0), > > - 8191); > > - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, > > - from->dew_enhance_seg_y[j], -8192), > > - 8191); > > + to->e_dew_enh_x[0][base + j] = clamp_t(int, > > + from->dew_enhance_seg_x[j], > > + 0, 8191); > > + to->e_dew_enh_y[0][base + j] = clamp_t(int, > > + from->dew_enhance_seg_y[j], > > + -8192, 8191); > > Such change introduces two warnings on smatch: > > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 > ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + > j]' > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 > ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + > j]' > > Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? Someone clearly needs to read the code and work out what it is doing. First stage is to use clamp() (not clamp_t) to get warnings from the compiler for the RHS. The snippet implies that the _x values are unsigned but the _y ones can be negative. Holding negative values in unsigned variables is a recipe for disaster. David > > > Thanks, > Mauro - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, On 9/6/24 9:56 AM, David Laight wrote: > From: Mauro Carvalho Chehab >> Sent: 06 September 2024 07:16 >> >> Em Sat, 27 Jul 2024 14:51:56 +0200 >> Christophe JAILLET <christophe.jaillet@wanadoo.fr> escreveu: >> >>> Using clamp_t() instead of min_t(max_t()) is easier to read. >>> >>> It also reduces the size of the preprocessed files by ~ 193 ko. >>> (see [1] for a discussion about it) >>> >>> $ ls -l ia_css_eed1_8.host*.i >>> 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i >>> 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i >>> >>> [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ >>> >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>> --- >>> .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- >>> 1 file changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >>> index e4fc90f88e24..96c13ebc4331 100644 >>> --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >>> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >>> @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( >>> base = shuffle_block * i; >>> >>> for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { >>> - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, >>> - from->dew_enhance_seg_x[j], 0), >>> - 8191); >>> - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, >>> - from->dew_enhance_seg_y[j], -8192), >>> - 8191); >>> + to->e_dew_enh_x[0][base + j] = clamp_t(int, >>> + from->dew_enhance_seg_x[j], >>> + 0, 8191); >>> + to->e_dew_enh_y[0][base + j] = clamp_t(int, >>> + from->dew_enhance_seg_y[j], >>> + -8192, 8191); >> >> Such change introduces two warnings on smatch: >> >> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: >> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 >> ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + >> j]' >> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: >> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 >> ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + >> j]' >> >> Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? > > Someone clearly needs to read the code and work out what it is doing. Ack, I'm looking into this now. > First stage is to use clamp() (not clamp_t) to get warnings from the > compiler for the RHS. I already changed this to plain clamp() when merging it, that does not cause any changes since the clamp-s are doing clamp(s32, -8192, 8192) Regards, Hans
From: Hans de Goede > Sent: 06 September 2024 08:53 > > Hi Mauro, > > On 9/6/24 8:15 AM, Mauro Carvalho Chehab wrote: > > Em Sat, 27 Jul 2024 14:51:56 +0200 > > Christophe JAILLET <christophe.jaillet@wanadoo.fr> escreveu: > > > >> Using clamp_t() instead of min_t(max_t()) is easier to read. > >> > >> It also reduces the size of the preprocessed files by ~ 193 ko. > >> (see [1] for a discussion about it) > >> > >> $ ls -l ia_css_eed1_8.host*.i > >> 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i > >> 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i > >> > >> [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ > >> > >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > >> --- > >> .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- > >> 1 file changed, 12 insertions(+), 12 deletions(-) > >> > >> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > >> index e4fc90f88e24..96c13ebc4331 100644 > >> --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > >> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > >> @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( > >> base = shuffle_block * i; > >> > >> for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > >> - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, > >> - from->dew_enhance_seg_x[j], 0), > >> - 8191); > >> - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, > >> - from->dew_enhance_seg_y[j], -8192), > >> - 8191); > >> + to->e_dew_enh_x[0][base + j] = clamp_t(int, > >> + from->dew_enhance_seg_x[j], > >> + 0, 8191); > >> + to->e_dew_enh_y[0][base + j] = clamp_t(int, > >> + from->dew_enhance_seg_y[j], > >> + -8192, 8191); > > > > Such change introduces two warnings on smatch: > > > > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 > ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + > j]' > > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: > drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 > ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + > j]' > > > > Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? > > These already are s32, the problem is that e_dew_enh_a is of type t_vmem_elem which is: > > typedef u16 t_vmem_elem; Ugg... :-) > > And that type is used in a lot of places, so we cannot > just change that. > > I guess we could add a t_signed_vmem_elem (s16) and use that for these vmem-arrays ? > > I tried fixing it like this: > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > b/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > index 1e01d354152b..7c0195d15f53 100644 > --- a/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > +++ b/drivers/staging/media/atomisp/i2c/atomisp-t4ka3.c > @@ -428,18 +428,13 @@ static int t4ka3_s_stream(struct v4l2_subdev *sd, int enable) > goto error_unlock; > } > > - ret = cci_multi_reg_write(sensor->regmap, t4ka3_init_config, > - ARRAY_SIZE(t4ka3_init_config), NULL); > - if (ret) > - goto error_powerdown; > - > + cci_multi_reg_write(sensor->regmap, t4ka3_init_config, > + ARRAY_SIZE(t4ka3_init_config), &ret); > /* enable group hold */ > - ret = cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, > - ARRAY_SIZE(t4ka3_param_hold), NULL); > - if (ret) > - goto error_powerdown; > - > - ret = cci_multi_reg_write(sensor->regmap, sensor->res->regs, sensor->res->regs_len, NULL); > + cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, > + ARRAY_SIZE(t4ka3_param_hold), &ret); > + cci_multi_reg_write(sensor->regmap, sensor->res->regs, > + sensor->res->regs_len, &ret); > if (ret) > goto error_powerdown; Isn't that unrelated? > diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > index b79d78e5b77f..c9043d516192 100644 > --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c > @@ -172,21 +172,21 @@ ia_css_eed1_8_vmem_encode( > base = shuffle_block * i; > > for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { > - to->e_dew_enh_x[0][base + j] = clamp(from->dew_enhance_seg_x[j], > - 0, 8191); > - to->e_dew_enh_y[0][base + j] = clamp(from->dew_enhance_seg_y[j], > - -8192, 8191); > + to->e_dew_enh_x[0][base + j] = (u16)clamp(from->dew_enhance_seg_x[j], > + 0, 8191); > + to->e_dew_enh_y[0][base + j] = (u16)clamp(from->dew_enhance_seg_y[j], > + -8192, 8191); How about an explicit clamp(...) & 0xffffu? > > for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { > - to->e_dew_enh_a[0][base + j] = clamp(from->dew_enhance_seg_slope[j], > - -8192, 8191); > + to->e_dew_enh_a[0][base + j] = (u16)clamp(from->dew_enhance_seg_slope[j], > + -8192, 8191); > /* Convert dew_enhance_seg_exp to flag: > * 0 -> 0 > * 1...13 -> 1 > */ > - to->e_dew_enh_f[0][base + j] = clamp(from->dew_enhance_seg_exp[j], > - 0, 13) > 0; > + to->e_dew_enh_f[0][base + j] = (u16)clamp(from->dew_enhance_seg_exp[j], > + 0, 13) > 0; Isn't the RHS just from->dew_enhance_seg_exp[j] > 0 ? That shouldn't be generating any kind of warning anyway. David > } > > /* Hard-coded to 0, in order to be able to handle out of > > but smatch still complains after this... > > Regards, > > Hans > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi, On 9/6/24 10:05 AM, David Laight wrote: > From: Hans de Goede >> Sent: 06 September 2024 08:53 >> >> Hi Mauro, >> >> On 9/6/24 8:15 AM, Mauro Carvalho Chehab wrote: >>> Em Sat, 27 Jul 2024 14:51:56 +0200 >>> Christophe JAILLET <christophe.jaillet@wanadoo.fr> escreveu: >>> >>>> Using clamp_t() instead of min_t(max_t()) is easier to read. >>>> >>>> It also reduces the size of the preprocessed files by ~ 193 ko. >>>> (see [1] for a discussion about it) >>>> >>>> $ ls -l ia_css_eed1_8.host*.i >>>> 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i >>>> 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i >>>> >>>> [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ >>>> >>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>>> --- >>>> .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- >>>> 1 file changed, 12 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >>>> index e4fc90f88e24..96c13ebc4331 100644 >>>> --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >>>> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >>>> @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( >>>> base = shuffle_block * i; >>>> >>>> for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { >>>> - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, >>>> - from->dew_enhance_seg_x[j], 0), >>>> - 8191); >>>> - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, >>>> - from->dew_enhance_seg_y[j], -8192), >>>> - 8191); >>>> + to->e_dew_enh_x[0][base + j] = clamp_t(int, >>>> + from->dew_enhance_seg_x[j], >>>> + 0, 8191); >>>> + to->e_dew_enh_y[0][base + j] = clamp_t(int, >>>> + from->dew_enhance_seg_y[j], >>>> + -8192, 8191); >>> >>> Such change introduces two warnings on smatch: >>> >>> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: >> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:177 >> ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_y[0][base + >> j]' >>> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c: >> drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c:182 >> ia_css_eed1_8_vmem_encode() warn: assigning (-8192) to unsigned variable 'to->e_dew_enh_a[0][base + >> j]' >>> >>> Should dew_enhance_seg_x and dew_enhance_seg_y be converted to signed? >> >> These already are s32, the problem is that e_dew_enh_a is of type t_vmem_elem which is: >> >> typedef u16 t_vmem_elem; > > Ugg... :-) > >> >> And that type is used in a lot of places, so we cannot >> just change that. >> >> I guess we could add a t_signed_vmem_elem (s16) and use that for these vmem-arrays ? >> >> I tried fixing it like this: <snip> >> /* enable group hold */ >> - ret = cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, >> - ARRAY_SIZE(t4ka3_param_hold), NULL); >> - if (ret) >> - goto error_powerdown; >> - >> - ret = cci_multi_reg_write(sensor->regmap, sensor->res->regs, sensor->res->regs_len, NULL); >> + cci_multi_reg_write(sensor->regmap, t4ka3_param_hold, >> + ARRAY_SIZE(t4ka3_param_hold), &ret); >> + cci_multi_reg_write(sensor->regmap, sensor->res->regs, >> + sensor->res->regs_len, &ret); >> if (ret) >> goto error_powerdown; > > Isn't that unrelated? Yes my bad. >> diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> index b79d78e5b77f..c9043d516192 100644 >> --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c >> @@ -172,21 +172,21 @@ ia_css_eed1_8_vmem_encode( >> base = shuffle_block * i; >> >> for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { >> - to->e_dew_enh_x[0][base + j] = clamp(from->dew_enhance_seg_x[j], >> - 0, 8191); >> - to->e_dew_enh_y[0][base + j] = clamp(from->dew_enhance_seg_y[j], >> - -8192, 8191); >> + to->e_dew_enh_x[0][base + j] = (u16)clamp(from->dew_enhance_seg_x[j], >> + 0, 8191); >> + to->e_dew_enh_y[0][base + j] = (u16)clamp(from->dew_enhance_seg_y[j], >> + -8192, 8191); > > How about an explicit clamp(...) & 0xffffu? Yes that should work, I tihnk. I actually have changed the type of e_dew_enh_y and e_dew_enh_f to s16 now and that does the trick of silencing smatch and seems like a nicer fix. I need to go and test the fix on actual hw to make sure nothing breaks and then I'll submit it. > >> >> for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { >> - to->e_dew_enh_a[0][base + j] = clamp(from->dew_enhance_seg_slope[j], >> - -8192, 8191); >> + to->e_dew_enh_a[0][base + j] = (u16)clamp(from->dew_enhance_seg_slope[j], >> + -8192, 8191); >> /* Convert dew_enhance_seg_exp to flag: >> * 0 -> 0 >> * 1...13 -> 1 >> */ >> - to->e_dew_enh_f[0][base + j] = clamp(from->dew_enhance_seg_exp[j], >> - 0, 13) > 0; >> + to->e_dew_enh_f[0][base + j] = (u16)clamp(from->dew_enhance_seg_exp[j], >> + 0, 13) > 0; > > Isn't the RHS just from->dew_enhance_seg_exp[j] > 0 ? > That shouldn't be generating any kind of warning anyway. It indeed does not generate a warning I changed all the clamp() calls here to keep things consistent. Regards, Hans
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c index e4fc90f88e24..96c13ebc4331 100644 --- a/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c +++ b/drivers/staging/media/atomisp/pci/isp/kernels/eed1_8/ia_css_eed1_8.host.c @@ -172,25 +172,25 @@ ia_css_eed1_8_vmem_encode( base = shuffle_block * i; for (j = 0; j < IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS; j++) { - to->e_dew_enh_x[0][base + j] = min_t(int, max_t(int, - from->dew_enhance_seg_x[j], 0), - 8191); - to->e_dew_enh_y[0][base + j] = min_t(int, max_t(int, - from->dew_enhance_seg_y[j], -8192), - 8191); + to->e_dew_enh_x[0][base + j] = clamp_t(int, + from->dew_enhance_seg_x[j], + 0, 8191); + to->e_dew_enh_y[0][base + j] = clamp_t(int, + from->dew_enhance_seg_y[j], + -8192, 8191); } for (j = 0; j < (IA_CSS_NUMBER_OF_DEW_ENHANCE_SEGMENTS - 1); j++) { - to->e_dew_enh_a[0][base + j] = min_t(int, max_t(int, - from->dew_enhance_seg_slope[j], - -8192), 8191); + to->e_dew_enh_a[0][base + j] = clamp_t(int, + from->dew_enhance_seg_slope[j], + -8192, 8191); /* Convert dew_enhance_seg_exp to flag: * 0 -> 0 * 1...13 -> 1 */ - to->e_dew_enh_f[0][base + j] = (min_t(int, max_t(int, - from->dew_enhance_seg_exp[j], - 0), 13) > 0); + to->e_dew_enh_f[0][base + j] = (clamp_t(int, + from->dew_enhance_seg_exp[j], + 0, 13) > 0); } /* Hard-coded to 0, in order to be able to handle out of
Using clamp_t() instead of min_t(max_t()) is easier to read. It also reduces the size of the preprocessed files by ~ 193 ko. (see [1] for a discussion about it) $ ls -l ia_css_eed1_8.host*.i 4829993 27 juil. 14:36 ia_css_eed1_8.host.old.i 4636649 27 juil. 14:42 ia_css_eed1_8.host.new.i [1]: https://lore.kernel.org/all/23bdb6fc8d884ceebeb6e8b8653b8cfe@AcuMS.aculab.com/ Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- .../isp/kernels/eed1_8/ia_css_eed1_8.host.c | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-)