| Message ID | 20200618190506.11892-3-kgupta@es.iitr.ac.in (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Add colors' order and other info over test image | expand |
Hi Kaaira,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v5.8-rc1 next-20200618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kaaira-Gupta/media-Add-colors-order-and-other-info-over-test-image/20200619-030709
base: git://linuxtv.org/media_tree.git master
config: nds32-randconfig-r016-20200619 (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.3.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
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "tpg_g_color_order" [drivers/media/test-drivers/vimc/vimc.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi, thanks for the patch On 18.06.20 21:05, Kaaira Gupta wrote: > Add a control in VIMC to display information such as the correct oder of > colors for a given test pattern, brightness, hue, saturation, contrast > and, width and height at sensor over test image; and display that > information. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > drivers/media/test-drivers/vimc/Kconfig | 2 + > drivers/media/test-drivers/vimc/vimc-common.h | 1 + > drivers/media/test-drivers/vimc/vimc-sensor.c | 47 ++++++++++++++++++- > 3 files changed, 49 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/test-drivers/vimc/Kconfig b/drivers/media/test-drivers/vimc/Kconfig > index 4068a67585f9..da4b2ad6e40c 100644 > --- a/drivers/media/test-drivers/vimc/Kconfig > +++ b/drivers/media/test-drivers/vimc/Kconfig > @@ -2,6 +2,8 @@ > config VIDEO_VIMC > tristate "Virtual Media Controller Driver (VIMC)" > depends on VIDEO_DEV && VIDEO_V4L2 > + select FONT_SUPPORT > + select FONT_8x16 > select MEDIA_CONTROLLER > select VIDEO_V4L2_SUBDEV_API > select VIDEOBUF2_VMALLOC > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h > index ae163dec2459..afda52253402 100644 > --- a/drivers/media/test-drivers/vimc/vimc-common.h > +++ b/drivers/media/test-drivers/vimc/vimc-common.h > @@ -20,6 +20,7 @@ > #define VIMC_CID_VIMC_CLASS (0x00f00000 | 1) > #define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0) > #define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1) > +#define VIMC_CID_SHOW_INFO (VIMC_CID_VIMC_BASE + 2) > > #define VIMC_FRAME_MAX_WIDTH 4096 > #define VIMC_FRAME_MAX_HEIGHT 2160 > diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c > index a2f09ac9a360..f5352b115aac 100644 > --- a/drivers/media/test-drivers/vimc/vimc-sensor.c > +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com> > */ > > +#include <linux/font.h> > #include <linux/v4l2-mediabus.h> > #include <linux/vmalloc.h> > #include <media/v4l2-ctrls.h> > @@ -19,6 +20,7 @@ struct vimc_sen_device { > struct v4l2_subdev sd; > struct tpg_data tpg; > u8 *frame; > + bool show_info; I see that vivid saves the 'v4l2_ctrl*' of the controls, maybe you should also do that instead of saving a boolean, > /* The active format */ > struct v4l2_mbus_framefmt mbus_format; > struct v4l2_ctrl_handler hdl; > @@ -185,10 +187,29 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = { > static void *vimc_sen_process_frame(struct vimc_ent_device *ved, > const void *sink_frame) > { > + u8 *basep[TPG_MAX_PLANES][2]; > + char *order; > + char str[100]; > + int line = 1; Those vars declarations can be inside the 'if (vsen->show_info)' > struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device, > ved); > - > tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame); > + if (vsen->show_info) { > + tpg_calc_text_basep(&vsen->tpg, basep, 0, vsen->frame); > + order = tpg_g_color_order(&vsen->tpg); > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, order); > + snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ", > + vsen->tpg.brightness, > + vsen->tpg.contrast, > + vsen->tpg.saturation, > + vsen->tpg.hue); > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); > + > + snprintf(str, sizeof(str), " sensor size: %dx%d", > + vsen->mbus_format.width, vsen->mbus_format.height); > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); > + } > + > return vsen->frame; > } > > @@ -200,6 +221,14 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable) > if (enable) { > const struct vimc_pix_map *vpix; > unsigned int frame_size; > + const struct font_desc *font = find_font("VGA8x16"); > + > + if (font == NULL) { Using 'if (!font)' is the way to check null pointer, instead of compering to null. Running checkpatch.pl with '--strict' will catch that. > + pr_err("vimc: could not find font\n"); 'dev_err' should be used instead of 'pr_err'. Also, maybe checking the font here is a bit late, since the user already wants to stream and expect the info to be shown. Maybe it is better to check the font on 'vimc_sen_s_ctrl'. Thanks, Dafna > + vsen->show_info = 0; > + } else { > + tpg_set_font(font->data); > + } > > /* Calculate the frame size */ > vpix = vimc_pix_map_by_code(vsen->mbus_format.code); > @@ -269,6 +298,9 @@ static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_SATURATION: > tpg_s_saturation(&vsen->tpg, ctrl->val); > break; > + case VIMC_CID_SHOW_INFO: > + vsen->show_info = ctrl->val; > + break; > default: > return -EINVAL; > } > @@ -307,6 +339,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = { > .qmenu = tpg_pattern_strings, > }; > > +static const struct v4l2_ctrl_config vimc_sen_ctrl_show_info = { > + .ops = &vimc_sen_ctrl_ops, > + .id = VIMC_CID_SHOW_INFO, > + .name = "Show Information", > + .type = V4L2_CTRL_TYPE_BOOLEAN, > + .min = 0, > + .max = 1, > + .step = 1, > + .def = 1, > +}; > + > static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > const char *vcfg_name) > { > @@ -323,6 +366,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_class, NULL); > v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_test_pattern, NULL); > + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_show_info, NULL); > v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, > V4L2_CID_VFLIP, 0, 1, 1, 0); > v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, > @@ -362,6 +406,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > /* Initialize the frame format */ > vsen->mbus_format = fmt_default; > + vsen->show_info = vimc_sen_ctrl_show_info.def; > > return &vsen->ved; > >
On Sat, Jun 20, 2020 at 12:05:28PM +0200, Dafna Hirschfeld wrote: > Hi, thanks for the patch > > On 18.06.20 21:05, Kaaira Gupta wrote: > > Add a control in VIMC to display information such as the correct oder of > > colors for a given test pattern, brightness, hue, saturation, contrast > > and, width and height at sensor over test image; and display that > > information. > > > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > > --- > > drivers/media/test-drivers/vimc/Kconfig | 2 + > > drivers/media/test-drivers/vimc/vimc-common.h | 1 + > > drivers/media/test-drivers/vimc/vimc-sensor.c | 47 ++++++++++++++++++- > > 3 files changed, 49 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/test-drivers/vimc/Kconfig b/drivers/media/test-drivers/vimc/Kconfig > > index 4068a67585f9..da4b2ad6e40c 100644 > > --- a/drivers/media/test-drivers/vimc/Kconfig > > +++ b/drivers/media/test-drivers/vimc/Kconfig > > @@ -2,6 +2,8 @@ > > config VIDEO_VIMC > > tristate "Virtual Media Controller Driver (VIMC)" > > depends on VIDEO_DEV && VIDEO_V4L2 > > + select FONT_SUPPORT > > + select FONT_8x16 > > select MEDIA_CONTROLLER > > select VIDEO_V4L2_SUBDEV_API > > select VIDEOBUF2_VMALLOC > > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h > > index ae163dec2459..afda52253402 100644 > > --- a/drivers/media/test-drivers/vimc/vimc-common.h > > +++ b/drivers/media/test-drivers/vimc/vimc-common.h > > @@ -20,6 +20,7 @@ > > #define VIMC_CID_VIMC_CLASS (0x00f00000 | 1) > > #define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0) > > #define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1) > > +#define VIMC_CID_SHOW_INFO (VIMC_CID_VIMC_BASE + 2) > > #define VIMC_FRAME_MAX_WIDTH 4096 > > #define VIMC_FRAME_MAX_HEIGHT 2160 > > diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c > > index a2f09ac9a360..f5352b115aac 100644 > > --- a/drivers/media/test-drivers/vimc/vimc-sensor.c > > +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c > > @@ -5,6 +5,7 @@ > > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com> > > */ > > +#include <linux/font.h> > > #include <linux/v4l2-mediabus.h> > > #include <linux/vmalloc.h> > > #include <media/v4l2-ctrls.h> > > @@ -19,6 +20,7 @@ struct vimc_sen_device { > > struct v4l2_subdev sd; > > struct tpg_data tpg; > > u8 *frame; > > + bool show_info; > > I see that vivid saves the 'v4l2_ctrl*' of the controls, > maybe you should also do that instead of saving a boolean, Hi, I don't understand..isn't boolean the control? > > > /* The active format */ > > struct v4l2_mbus_framefmt mbus_format; > > struct v4l2_ctrl_handler hdl; > > @@ -185,10 +187,29 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = { > > static void *vimc_sen_process_frame(struct vimc_ent_device *ved, > > const void *sink_frame) > > { > > + u8 *basep[TPG_MAX_PLANES][2]; > > + char *order; > > + char str[100]; > > + int line = 1; > > Those vars declarations can be inside the 'if (vsen->show_info)' I declared it outside because I felt all declarations should be together? > > > struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device, > > ved); > > - > > tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame); > > + if (vsen->show_info) { > > + tpg_calc_text_basep(&vsen->tpg, basep, 0, vsen->frame); > > + order = tpg_g_color_order(&vsen->tpg); > > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, order); > > + snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ", > > + vsen->tpg.brightness, > > + vsen->tpg.contrast, > > + vsen->tpg.saturation, > > + vsen->tpg.hue); > > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); > > + > > + snprintf(str, sizeof(str), " sensor size: %dx%d", > > + vsen->mbus_format.width, vsen->mbus_format.height); > > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); > > + } > > + > > return vsen->frame; > > } > > @@ -200,6 +221,14 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable) > > if (enable) { > > const struct vimc_pix_map *vpix; > > unsigned int frame_size; > > + const struct font_desc *font = find_font("VGA8x16"); > > + > > + if (font == NULL) { > Using 'if (!font)' is the way to check null pointer, instead of compering to null. Running checkpatch.pl with '--strict' > will catch that. I didn't do that to be consistent with vivid's style of code. Plus I thought it makes it more clear to read. Should i change this? > > + pr_err("vimc: could not find font\n"); > 'dev_err' should be used instead of 'pr_err'. yes sorry, i didn't now the difference. > > Also, maybe checking the font here is a bit late, since the user already > wants to stream and expect the info to be shown. > Maybe it is better to check the font on 'vimc_sen_s_ctrl'. Like show the control only of font is available? I think showing the error is enough maybe? > > Thanks, > Dafna > > > + vsen->show_info = 0; > > + } else { > > + tpg_set_font(font->data); > > + } > > /* Calculate the frame size */ > > vpix = vimc_pix_map_by_code(vsen->mbus_format.code); > > @@ -269,6 +298,9 @@ static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl) > > case V4L2_CID_SATURATION: > > tpg_s_saturation(&vsen->tpg, ctrl->val); > > break; > > + case VIMC_CID_SHOW_INFO: > > + vsen->show_info = ctrl->val; > > + break; > > default: > > return -EINVAL; > > } > > @@ -307,6 +339,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = { > > .qmenu = tpg_pattern_strings, > > }; > > +static const struct v4l2_ctrl_config vimc_sen_ctrl_show_info = { > > + .ops = &vimc_sen_ctrl_ops, > > + .id = VIMC_CID_SHOW_INFO, > > + .name = "Show Information", > > + .type = V4L2_CTRL_TYPE_BOOLEAN, > > + .min = 0, > > + .max = 1, > > + .step = 1, > > + .def = 1, > > +}; > > + > > static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > const char *vcfg_name) > > { > > @@ -323,6 +366,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_class, NULL); > > v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_test_pattern, NULL); > > + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_show_info, NULL); > > v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, > > V4L2_CID_VFLIP, 0, 1, 1, 0); > > v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, > > @@ -362,6 +406,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > /* Initialize the frame format */ > > vsen->mbus_format = fmt_default; > > + vsen->show_info = vimc_sen_ctrl_show_info.def; > > return &vsen->ved; > >
On 18/06/2020 21:05, Kaaira Gupta wrote: > Add a control in VIMC to display information such as the correct oder of odor -> order > colors for a given test pattern, brightness, hue, saturation, contrast > and, width and height at sensor over test image; and display that > information. and, width -> and width '; and display that information': just drop this since you already mentioned that in 'a control in VIMC to display information'. It is also useful to show the sequence counter (i.e. the sequence field in struct v4l2_buffer) since this always changes for every frame. Perhaps base VIMC_CID_SHOW_INFO on VIVID_CID_OSD_TEXT_MODE? Doing this as a menu control allows you to add other combinations in the future. > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > drivers/media/test-drivers/vimc/Kconfig | 2 + > drivers/media/test-drivers/vimc/vimc-common.h | 1 + > drivers/media/test-drivers/vimc/vimc-sensor.c | 47 ++++++++++++++++++- > 3 files changed, 49 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/test-drivers/vimc/Kconfig b/drivers/media/test-drivers/vimc/Kconfig > index 4068a67585f9..da4b2ad6e40c 100644 > --- a/drivers/media/test-drivers/vimc/Kconfig > +++ b/drivers/media/test-drivers/vimc/Kconfig > @@ -2,6 +2,8 @@ > config VIDEO_VIMC > tristate "Virtual Media Controller Driver (VIMC)" > depends on VIDEO_DEV && VIDEO_V4L2 > + select FONT_SUPPORT > + select FONT_8x16 > select MEDIA_CONTROLLER > select VIDEO_V4L2_SUBDEV_API > select VIDEOBUF2_VMALLOC > diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h > index ae163dec2459..afda52253402 100644 > --- a/drivers/media/test-drivers/vimc/vimc-common.h > +++ b/drivers/media/test-drivers/vimc/vimc-common.h > @@ -20,6 +20,7 @@ > #define VIMC_CID_VIMC_CLASS (0x00f00000 | 1) > #define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0) > #define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1) > +#define VIMC_CID_SHOW_INFO (VIMC_CID_VIMC_BASE + 2) > > #define VIMC_FRAME_MAX_WIDTH 4096 > #define VIMC_FRAME_MAX_HEIGHT 2160 > diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c > index a2f09ac9a360..f5352b115aac 100644 > --- a/drivers/media/test-drivers/vimc/vimc-sensor.c > +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com> > */ > > +#include <linux/font.h> > #include <linux/v4l2-mediabus.h> > #include <linux/vmalloc.h> > #include <media/v4l2-ctrls.h> > @@ -19,6 +20,7 @@ struct vimc_sen_device { > struct v4l2_subdev sd; > struct tpg_data tpg; > u8 *frame; > + bool show_info; > /* The active format */ > struct v4l2_mbus_framefmt mbus_format; > struct v4l2_ctrl_handler hdl; > @@ -185,10 +187,29 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = { > static void *vimc_sen_process_frame(struct vimc_ent_device *ved, > const void *sink_frame) > { > + u8 *basep[TPG_MAX_PLANES][2]; > + char *order; > + char str[100]; > + int line = 1; > struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device, > ved); > - > tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame); > + if (vsen->show_info) { > + tpg_calc_text_basep(&vsen->tpg, basep, 0, vsen->frame); > + order = tpg_g_color_order(&vsen->tpg); > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, order); > + snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ", The previous tpg_gen_text doesn't have a leading space, but this and the next line does. That should be consistent. I can't quite remember why I added spaces before/after the text in the vivid implementation. I think it was because it looked weird without a space, probably because the text color was identical to the first colorbar in some of the patterns, making it hard to read in some cases. > + vsen->tpg.brightness, > + vsen->tpg.contrast, > + vsen->tpg.saturation, > + vsen->tpg.hue); > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); > + > + snprintf(str, sizeof(str), " sensor size: %dx%d", > + vsen->mbus_format.width, vsen->mbus_format.height); > + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); > + } > + > return vsen->frame; > } > > @@ -200,6 +221,14 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable) > if (enable) { > const struct vimc_pix_map *vpix; > unsigned int frame_size; > + const struct font_desc *font = find_font("VGA8x16"); > + > + if (font == NULL) { > + pr_err("vimc: could not find font\n"); > + vsen->show_info = 0; > + } else { > + tpg_set_font(font->data); > + } Like vivid, this should be done in the probe/init/whatever function, and not when you start streaming. Kconfig selects this font, so if this doesn't succeed, then the driver shouldn't finish the probe(). > > /* Calculate the frame size */ > vpix = vimc_pix_map_by_code(vsen->mbus_format.code); > @@ -269,6 +298,9 @@ static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl) > case V4L2_CID_SATURATION: > tpg_s_saturation(&vsen->tpg, ctrl->val); > break; > + case VIMC_CID_SHOW_INFO: > + vsen->show_info = ctrl->val; > + break; > default: > return -EINVAL; > } > @@ -307,6 +339,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = { > .qmenu = tpg_pattern_strings, > }; > > +static const struct v4l2_ctrl_config vimc_sen_ctrl_show_info = { > + .ops = &vimc_sen_ctrl_ops, > + .id = VIMC_CID_SHOW_INFO, > + .name = "Show Information", > + .type = V4L2_CTRL_TYPE_BOOLEAN, > + .min = 0, > + .max = 1, > + .step = 1, > + .def = 1, > +}; > + > static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > const char *vcfg_name) > { > @@ -323,6 +366,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_class, NULL); > v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_test_pattern, NULL); > + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_show_info, NULL); > v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, > V4L2_CID_VFLIP, 0, 1, 1, 0); > v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, > @@ -362,6 +406,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > /* Initialize the frame format */ > vsen->mbus_format = fmt_default; > + vsen->show_info = vimc_sen_ctrl_show_info.def; > > return &vsen->ved; > > Regards, Hans
On 21.06.20 22:32, Kaaira Gupta wrote: > On Sat, Jun 20, 2020 at 12:05:28PM +0200, Dafna Hirschfeld wrote: >> Hi, thanks for the patch >> >> On 18.06.20 21:05, Kaaira Gupta wrote: >>> Add a control in VIMC to display information such as the correct oder of >>> colors for a given test pattern, brightness, hue, saturation, contrast >>> and, width and height at sensor over test image; and display that >>> information. >>> >>> Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> >>> --- >>> drivers/media/test-drivers/vimc/Kconfig | 2 + >>> drivers/media/test-drivers/vimc/vimc-common.h | 1 + >>> drivers/media/test-drivers/vimc/vimc-sensor.c | 47 ++++++++++++++++++- >>> 3 files changed, 49 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/test-drivers/vimc/Kconfig b/drivers/media/test-drivers/vimc/Kconfig >>> index 4068a67585f9..da4b2ad6e40c 100644 >>> --- a/drivers/media/test-drivers/vimc/Kconfig >>> +++ b/drivers/media/test-drivers/vimc/Kconfig >>> @@ -2,6 +2,8 @@ >>> config VIDEO_VIMC >>> tristate "Virtual Media Controller Driver (VIMC)" >>> depends on VIDEO_DEV && VIDEO_V4L2 >>> + select FONT_SUPPORT >>> + select FONT_8x16 >>> select MEDIA_CONTROLLER >>> select VIDEO_V4L2_SUBDEV_API >>> select VIDEOBUF2_VMALLOC >>> diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h >>> index ae163dec2459..afda52253402 100644 >>> --- a/drivers/media/test-drivers/vimc/vimc-common.h >>> +++ b/drivers/media/test-drivers/vimc/vimc-common.h >>> @@ -20,6 +20,7 @@ >>> #define VIMC_CID_VIMC_CLASS (0x00f00000 | 1) >>> #define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0) >>> #define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1) >>> +#define VIMC_CID_SHOW_INFO (VIMC_CID_VIMC_BASE + 2) >>> #define VIMC_FRAME_MAX_WIDTH 4096 >>> #define VIMC_FRAME_MAX_HEIGHT 2160 >>> diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c >>> index a2f09ac9a360..f5352b115aac 100644 >>> --- a/drivers/media/test-drivers/vimc/vimc-sensor.c >>> +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c >>> @@ -5,6 +5,7 @@ >>> * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com> >>> */ >>> +#include <linux/font.h> >>> #include <linux/v4l2-mediabus.h> >>> #include <linux/vmalloc.h> >>> #include <media/v4l2-ctrls.h> >>> @@ -19,6 +20,7 @@ struct vimc_sen_device { >>> struct v4l2_subdev sd; >>> struct tpg_data tpg; >>> u8 *frame; >>> + bool show_info; >> >> I see that vivid saves the 'v4l2_ctrl*' of the controls, >> maybe you should also do that instead of saving a boolean, > > Hi, I don't understand..isn't boolean the control? You can see that vivid saves the controls as 'v4l2_ctrl*' and then the value of the control can be read from the 'cur.val' field. Note also that the mutex lock: "mutex_lock(dev->ctrl_hdl_user_vid.lock);" when reading the values. This way you have don't have to set the value yourself, the framework takes care of it. You only have to read the value. > >> >>> /* The active format */ >>> struct v4l2_mbus_framefmt mbus_format; >>> struct v4l2_ctrl_handler hdl; >>> @@ -185,10 +187,29 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = { >>> static void *vimc_sen_process_frame(struct vimc_ent_device *ved, >>> const void *sink_frame) >>> { >>> + u8 *basep[TPG_MAX_PLANES][2]; >>> + char *order; >>> + char str[100]; >>> + int line = 1; >> >> Those vars declarations can be inside the 'if (vsen->show_info)' > > I declared it outside because I felt all declarations should be > together? Not crucial, but I think it is nicer to declare variables in the most inner scope where they are used. > >> >>> struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device, >>> ved); >>> - >>> tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame); >>> + if (vsen->show_info) { >>> + tpg_calc_text_basep(&vsen->tpg, basep, 0, vsen->frame); >>> + order = tpg_g_color_order(&vsen->tpg); >>> + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, order); >>> + snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ", >>> + vsen->tpg.brightness, >>> + vsen->tpg.contrast, >>> + vsen->tpg.saturation, >>> + vsen->tpg.hue); >>> + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); >>> + >>> + snprintf(str, sizeof(str), " sensor size: %dx%d", >>> + vsen->mbus_format.width, vsen->mbus_format.height); >>> + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); >>> + } >>> + >>> return vsen->frame; >>> } >>> @@ -200,6 +221,14 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable) >>> if (enable) { >>> const struct vimc_pix_map *vpix; >>> unsigned int frame_size; >>> + const struct font_desc *font = find_font("VGA8x16"); >>> + >>> + if (font == NULL) { >> Using 'if (!font)' is the way to check null pointer, instead of compering to null. Running checkpatch.pl with '--strict' >> will catch that. > > I didn't do that to be consistent with vivid's style of code. Plus I > thought it makes it more clear to read. Should i change this? I don't know why vivid do it this way. Again, it is not that crucial, but in general it is better to send a patch that passes the issues found in checkpatch. Thanks, Dafna > >>> + pr_err("vimc: could not find font\n"); >> 'dev_err' should be used instead of 'pr_err'. > > yes sorry, i didn't now the difference. > >> >> Also, maybe checking the font here is a bit late, since the user already >> wants to stream and expect the info to be shown. >> Maybe it is better to check the font on 'vimc_sen_s_ctrl'. > > Like show the control only of font is available? > > I think showing the error is enough maybe? > >> >> Thanks, >> Dafna >> >>> + vsen->show_info = 0; >>> + } else { >>> + tpg_set_font(font->data); >>> + } >>> /* Calculate the frame size */ >>> vpix = vimc_pix_map_by_code(vsen->mbus_format.code); >>> @@ -269,6 +298,9 @@ static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl) >>> case V4L2_CID_SATURATION: >>> tpg_s_saturation(&vsen->tpg, ctrl->val); >>> break; >>> + case VIMC_CID_SHOW_INFO: >>> + vsen->show_info = ctrl->val; >>> + break; >>> default: >>> return -EINVAL; >>> } >>> @@ -307,6 +339,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = { >>> .qmenu = tpg_pattern_strings, >>> }; >>> +static const struct v4l2_ctrl_config vimc_sen_ctrl_show_info = { >>> + .ops = &vimc_sen_ctrl_ops, >>> + .id = VIMC_CID_SHOW_INFO, >>> + .name = "Show Information", >>> + .type = V4L2_CTRL_TYPE_BOOLEAN, >>> + .min = 0, >>> + .max = 1, >>> + .step = 1, >>> + .def = 1, >>> +}; >>> + >>> static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>> const char *vcfg_name) >>> { >>> @@ -323,6 +366,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>> v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_class, NULL); >>> v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_test_pattern, NULL); >>> + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_show_info, NULL); >>> v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, >>> V4L2_CID_VFLIP, 0, 1, 1, 0); >>> v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, >>> @@ -362,6 +406,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, >>> /* Initialize the frame format */ >>> vsen->mbus_format = fmt_default; >>> + vsen->show_info = vimc_sen_ctrl_show_info.def; >>> return &vsen->ved; >>>
diff --git a/drivers/media/test-drivers/vimc/Kconfig b/drivers/media/test-drivers/vimc/Kconfig index 4068a67585f9..da4b2ad6e40c 100644 --- a/drivers/media/test-drivers/vimc/Kconfig +++ b/drivers/media/test-drivers/vimc/Kconfig @@ -2,6 +2,8 @@ config VIDEO_VIMC tristate "Virtual Media Controller Driver (VIMC)" depends on VIDEO_DEV && VIDEO_V4L2 + select FONT_SUPPORT + select FONT_8x16 select MEDIA_CONTROLLER select VIDEO_V4L2_SUBDEV_API select VIDEOBUF2_VMALLOC diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h index ae163dec2459..afda52253402 100644 --- a/drivers/media/test-drivers/vimc/vimc-common.h +++ b/drivers/media/test-drivers/vimc/vimc-common.h @@ -20,6 +20,7 @@ #define VIMC_CID_VIMC_CLASS (0x00f00000 | 1) #define VIMC_CID_TEST_PATTERN (VIMC_CID_VIMC_BASE + 0) #define VIMC_CID_MEAN_WIN_SIZE (VIMC_CID_VIMC_BASE + 1) +#define VIMC_CID_SHOW_INFO (VIMC_CID_VIMC_BASE + 2) #define VIMC_FRAME_MAX_WIDTH 4096 #define VIMC_FRAME_MAX_HEIGHT 2160 diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c index a2f09ac9a360..f5352b115aac 100644 --- a/drivers/media/test-drivers/vimc/vimc-sensor.c +++ b/drivers/media/test-drivers/vimc/vimc-sensor.c @@ -5,6 +5,7 @@ * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com> */ +#include <linux/font.h> #include <linux/v4l2-mediabus.h> #include <linux/vmalloc.h> #include <media/v4l2-ctrls.h> @@ -19,6 +20,7 @@ struct vimc_sen_device { struct v4l2_subdev sd; struct tpg_data tpg; u8 *frame; + bool show_info; /* The active format */ struct v4l2_mbus_framefmt mbus_format; struct v4l2_ctrl_handler hdl; @@ -185,10 +187,29 @@ static const struct v4l2_subdev_pad_ops vimc_sen_pad_ops = { static void *vimc_sen_process_frame(struct vimc_ent_device *ved, const void *sink_frame) { + u8 *basep[TPG_MAX_PLANES][2]; + char *order; + char str[100]; + int line = 1; struct vimc_sen_device *vsen = container_of(ved, struct vimc_sen_device, ved); - tpg_fill_plane_buffer(&vsen->tpg, 0, 0, vsen->frame); + if (vsen->show_info) { + tpg_calc_text_basep(&vsen->tpg, basep, 0, vsen->frame); + order = tpg_g_color_order(&vsen->tpg); + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, order); + snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ", + vsen->tpg.brightness, + vsen->tpg.contrast, + vsen->tpg.saturation, + vsen->tpg.hue); + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); + + snprintf(str, sizeof(str), " sensor size: %dx%d", + vsen->mbus_format.width, vsen->mbus_format.height); + tpg_gen_text(&vsen->tpg, basep, line++ * 16, 16, str); + } + return vsen->frame; } @@ -200,6 +221,14 @@ static int vimc_sen_s_stream(struct v4l2_subdev *sd, int enable) if (enable) { const struct vimc_pix_map *vpix; unsigned int frame_size; + const struct font_desc *font = find_font("VGA8x16"); + + if (font == NULL) { + pr_err("vimc: could not find font\n"); + vsen->show_info = 0; + } else { + tpg_set_font(font->data); + } /* Calculate the frame size */ vpix = vimc_pix_map_by_code(vsen->mbus_format.code); @@ -269,6 +298,9 @@ static int vimc_sen_s_ctrl(struct v4l2_ctrl *ctrl) case V4L2_CID_SATURATION: tpg_s_saturation(&vsen->tpg, ctrl->val); break; + case VIMC_CID_SHOW_INFO: + vsen->show_info = ctrl->val; + break; default: return -EINVAL; } @@ -307,6 +339,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = { .qmenu = tpg_pattern_strings, }; +static const struct v4l2_ctrl_config vimc_sen_ctrl_show_info = { + .ops = &vimc_sen_ctrl_ops, + .id = VIMC_CID_SHOW_INFO, + .name = "Show Information", + .type = V4L2_CTRL_TYPE_BOOLEAN, + .min = 0, + .max = 1, + .step = 1, + .def = 1, +}; + static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, const char *vcfg_name) { @@ -323,6 +366,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_class, NULL); v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_test_pattern, NULL); + v4l2_ctrl_new_custom(&vsen->hdl, &vimc_sen_ctrl_show_info, NULL); v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, V4L2_CID_VFLIP, 0, 1, 1, 0); v4l2_ctrl_new_std(&vsen->hdl, &vimc_sen_ctrl_ops, @@ -362,6 +406,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, /* Initialize the frame format */ vsen->mbus_format = fmt_default; + vsen->show_info = vimc_sen_ctrl_show_info.def; return &vsen->ved;
Add a control in VIMC to display information such as the correct oder of colors for a given test pattern, brightness, hue, saturation, contrast and, width and height at sensor over test image; and display that information. Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- drivers/media/test-drivers/vimc/Kconfig | 2 + drivers/media/test-drivers/vimc/vimc-common.h | 1 + drivers/media/test-drivers/vimc/vimc-sensor.c | 47 ++++++++++++++++++- 3 files changed, 49 insertions(+), 1 deletion(-)