On Mon, Jun 15, 2020 at 12:48:20PM +0100, Kieran Bingham wrote: > Hi Kaaira, > > On 14/06/2020 21:02, Kaaira Gupta wrote: > > Add a control in VIMC to show the correct order of the colors for a > > given test pattern. > > The control can be accessed by using show_colors_order in v4l2-ctl > > > > Signed-off-by: Kaaira Gupta <kgupta@xxxxxxxxxxxxx> > > --- > > drivers/media/test-drivers/vimc/Kconfig | 2 ++ > > drivers/media/test-drivers/vimc/vimc-common.h | 1 + > > drivers/media/test-drivers/vimc/vimc-sensor.c | 34 +++++++++++++++++++ > > 3 files changed, 37 insertions(+) > > > > 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..52376ba6146b 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_TEST_PATTERN_ORDER (VIMC_CID_VIMC_BASE + 2) > > This should have the prefix CID like the others. I believe that stands > for "Control ID" > > As below, I think it might warrant being a more generic name for the OSD > overlay, as it could display more than just the colour sequence. Yes, Okay..I think I'll make a separate patch for adding all the text that VIMC can show including the color order, and let the 1st patch of this series be a separate patch? Should the colors' order text be added to VIVID as well? > > > > > > #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..da625f6accce 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@xxxxxxxxx> > > */ > > > > +#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_colors_order; > > I would say it's the 'sequence' 'show_color_sequence' but it's still a > bit long ... but maybe that doesn't really matter. If its generic for all the information maybe it should be show_info? > > > > /* The active format */ > > struct v4l2_mbus_framefmt mbus_format; > > struct v4l2_ctrl_handler hdl; > > @@ -185,10 +187,18 @@ 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 *str; > > 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_colors_order) { > > + tpg_calc_text_basep(&vsen->tpg, basep, 0, vsen->frame); > > + str = tpg_g_color_order(&vsen->tpg); > > + tpg_gen_text(&vsen->tpg, basep, 1, 1, str); > > + } > > + > > return vsen->frame; > > } > > > > @@ -200,6 +210,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"); > > + return -ENODEV; > > I wonder if this should instead just disable text rendering instead? Shouldn't the user know why he can't see the text? > > It might be reasonable to compile the kernel without the font-library ? I have added the support in its Kconfig, isn't that okay? > > > + } > > + > > + tpg_set_font(font->data); > > > > /* Calculate the frame size */ > > vpix = vimc_pix_map_by_code(vsen->mbus_format.code); > > @@ -269,6 +287,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_TEST_PATTERN_ORDER: > > + vsen->show_colors_order = ctrl->val; > > + break; > > default: > > return -EINVAL; > > } > > @@ -307,6 +328,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_order = { > > + .ops = &vimc_sen_ctrl_ops, > > + .id = VIMC_TEST_PATTERN_ORDER, > > Now that you've brought in support for rendering text on the frame, I > wonder if more information should be displayed just like VIVID does. > > In that case, it would probably be better to have a more generic control > that enables all of the text OSD with a more generic name. Yes, I will change that > > > > + .name = "Show Colors Order", > > + .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 +355,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_order, 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 +395,7 @@ static struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc, > > > > /* Initialize the frame format */ > > vsen->mbus_format = fmt_default; > > + vsen->show_colors_order = vimc_sen_ctrl_order.def; > > > > return &vsen->ved; > > > > Thanks for your time :D > > -- > Regards > -- > Kieran