* [PATCH v2 0/2] media: Add colors' order over test image @ 2020-06-14 20:02 Kaaira Gupta 2020-06-14 20:02 ` [PATCH v2 1/2] media: tpg: Add function to return colors' order of " Kaaira Gupta 2020-06-14 20:02 ` [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order Kaaira Gupta 0 siblings, 2 replies; 7+ messages in thread From: Kaaira Gupta @ 2020-06-14 20:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, hverkuil Cc: Kaaira Gupta This patchset aims to add a method to display the correct order of colors for a test image generated. It does so by adding a function which returns a string of correct order of the colors for a test pattern and a control using which displays the string over test image. Changes since v1: - Divided the patch into two patches. - Returned NULL for patterns whose color order cannot be defined. (Reported-by: kernel test robot <lkp@intel.com>) - Made separate switch cases for separate test patterns (Reported-by: kernel test robot <lkp@intel.com>) - Renamed variables from camelcase to use '_' - prefixed 'media' to the patches. Kaaira Gupta (2): media: tpg: Add function to return colors' order of test image media: vimc: Add a control to show test pattern colors' order drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 32 +++++++++++++++-- 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 +++++++++++++++++++ include/media/tpg/v4l2-tpg.h | 1 + 5 files changed, 68 insertions(+), 2 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] media: tpg: Add function to return colors' order of test image 2020-06-14 20:02 [PATCH v2 0/2] media: Add colors' order over test image Kaaira Gupta @ 2020-06-14 20:02 ` Kaaira Gupta 2020-06-15 11:42 ` Kieran Bingham 2020-06-14 20:02 ` [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order Kaaira Gupta 1 sibling, 1 reply; 7+ messages in thread From: Kaaira Gupta @ 2020-06-14 20:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, hverkuil Cc: Kaaira Gupta Currently there is no method to know the correct order of the colors for a test image generated by tpg. Write a function that returns a string of colors' order given a tpg. It returns a NULL pointer in case of test patterns which do not have a well defined colors' order. Hence add a NULL check for text in tpg_gen_text(). Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> --- drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 32 +++++++++++++++++-- include/media/tpg/v4l2-tpg.h | 1 + 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c index 50f1e0b28b25..c8257e860c6e 100644 --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c @@ -1959,12 +1959,14 @@ void tpg_gen_text(const struct tpg_data *tpg, u8 *basep[TPG_MAX_PLANES][2], unsigned step = V4L2_FIELD_HAS_T_OR_B(tpg->field) ? 2 : 1; unsigned div = step; unsigned first = 0; - unsigned len = strlen(text); + unsigned len; unsigned p; - if (font8x16 == NULL || basep == NULL) + if (font8x16 == NULL || basep == NULL || text == NULL) return; + len = strlen(text); + /* Checks if it is possible to show string */ if (y + 16 >= tpg->compose.height || x + 8 >= tpg->compose.width) return; @@ -2006,6 +2008,32 @@ void tpg_gen_text(const struct tpg_data *tpg, u8 *basep[TPG_MAX_PLANES][2], } EXPORT_SYMBOL_GPL(tpg_gen_text); +char *tpg_g_color_order(const struct tpg_data *tpg) +{ + switch (tpg->pattern) { + case TPG_PAT_75_COLORBAR: + return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; + case TPG_PAT_100_COLORBAR: + return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; + case TPG_PAT_CSC_COLORBAR: + return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; + case TPG_PAT_100_HCOLORBAR: + return "Top to bottom: white, yellow, cyan, green, magenta, red, blue, black"; + case TPG_PAT_BLACK: + return "Black"; + case TPG_PAT_WHITE: + return "White"; + case TPG_PAT_RED: + return "Red"; + case TPG_PAT_GREEN: + return "Green"; + case TPG_PAT_BLUE: + return "Blue"; + default: + return NULL; + } +} + void tpg_update_mv_step(struct tpg_data *tpg) { int factor = tpg->mv_hor_mode > TPG_MOVE_NONE ? -1 : 1; diff --git a/include/media/tpg/v4l2-tpg.h b/include/media/tpg/v4l2-tpg.h index eb191e85d363..4f79cac87b85 100644 --- a/include/media/tpg/v4l2-tpg.h +++ b/include/media/tpg/v4l2-tpg.h @@ -252,6 +252,7 @@ void tpg_fillbuffer(struct tpg_data *tpg, v4l2_std_id std, bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc); void tpg_s_crop_compose(struct tpg_data *tpg, const struct v4l2_rect *crop, const struct v4l2_rect *compose); +char *tpg_g_color_order(const struct tpg_data *tpg); static inline void tpg_s_pattern(struct tpg_data *tpg, enum tpg_pattern pattern) { -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] media: tpg: Add function to return colors' order of test image 2020-06-14 20:02 ` [PATCH v2 1/2] media: tpg: Add function to return colors' order of " Kaaira Gupta @ 2020-06-15 11:42 ` Kieran Bingham 0 siblings, 0 replies; 7+ messages in thread From: Kieran Bingham @ 2020-06-15 11:42 UTC (permalink / raw) To: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, hverkuil Hi Kaaira, On 14/06/2020 21:02, Kaaira Gupta wrote: > Currently there is no method to know the correct order of the colors for > a test image generated by tpg. Write a function that returns a string of > colors' order given a tpg. It returns a NULL pointer in case of test > patterns which do not have a well defined colors' order. Hence add a > NULL check for text in tpg_gen_text(). > > Signed-off-by: Kaaira Gupta <kgupta@es.iitr.ac.in> > --- > drivers/media/common/v4l2-tpg/v4l2-tpg-core.c | 32 +++++++++++++++++-- > include/media/tpg/v4l2-tpg.h | 1 + > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > index 50f1e0b28b25..c8257e860c6e 100644 > --- a/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > +++ b/drivers/media/common/v4l2-tpg/v4l2-tpg-core.c > @@ -1959,12 +1959,14 @@ void tpg_gen_text(const struct tpg_data *tpg, u8 *basep[TPG_MAX_PLANES][2], > unsigned step = V4L2_FIELD_HAS_T_OR_B(tpg->field) ? 2 : 1; > unsigned div = step; > unsigned first = 0; > - unsigned len = strlen(text); > + unsigned len; > unsigned p; > > - if (font8x16 == NULL || basep == NULL) > + if (font8x16 == NULL || basep == NULL || text == NULL) > return; > > + len = strlen(text); > + > /* Checks if it is possible to show string */ > if (y + 16 >= tpg->compose.height || x + 8 >= tpg->compose.width) > return; > @@ -2006,6 +2008,32 @@ void tpg_gen_text(const struct tpg_data *tpg, u8 *basep[TPG_MAX_PLANES][2], > } > EXPORT_SYMBOL_GPL(tpg_gen_text); > > +char *tpg_g_color_order(const struct tpg_data *tpg) > +{ > + switch (tpg->pattern) { > + case TPG_PAT_75_COLORBAR: > + return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; > + case TPG_PAT_100_COLORBAR: > + return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; > + case TPG_PAT_CSC_COLORBAR: > + return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; Those three options return identical strings. In C you can 'fallthrough' on case statements like this: > switch (tpg->pattern) { > case TPG_PAT_75_COLORBAR: > case TPG_PAT_100_COLORBAR: > case TPG_PAT_CSC_COLORBAR: > return "Left to right: white, yellow, cyan, green, magenta, red, blue, black"; So all three of those options will go to the same return statement. > + case TPG_PAT_100_HCOLORBAR: > + return "Top to bottom: white, yellow, cyan, green, magenta, red, blue, black"; It would be nice to be able to factor out the common color sequence from those (two remaining) strings too, as only the direction changes. But perhaps it's a pain to have to do a string join in this call, especially given it's static string data otherwise, but that's also a good reason not to duplicate unnecessarily (as it wastes space in memory). With those fixed, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + case TPG_PAT_BLACK: > + return "Black"; > + case TPG_PAT_WHITE: > + return "White"; > + case TPG_PAT_RED: > + return "Red"; > + case TPG_PAT_GREEN: > + return "Green"; > + case TPG_PAT_BLUE: > + return "Blue"; > + default: > + return NULL; > + } > +} > + > void tpg_update_mv_step(struct tpg_data *tpg) > { > int factor = tpg->mv_hor_mode > TPG_MOVE_NONE ? -1 : 1; > diff --git a/include/media/tpg/v4l2-tpg.h b/include/media/tpg/v4l2-tpg.h > index eb191e85d363..4f79cac87b85 100644 > --- a/include/media/tpg/v4l2-tpg.h > +++ b/include/media/tpg/v4l2-tpg.h > @@ -252,6 +252,7 @@ void tpg_fillbuffer(struct tpg_data *tpg, v4l2_std_id std, > bool tpg_s_fourcc(struct tpg_data *tpg, u32 fourcc); > void tpg_s_crop_compose(struct tpg_data *tpg, const struct v4l2_rect *crop, > const struct v4l2_rect *compose); > +char *tpg_g_color_order(const struct tpg_data *tpg); > > static inline void tpg_s_pattern(struct tpg_data *tpg, enum tpg_pattern pattern) > { > -- Regards -- Kieran ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order 2020-06-14 20:02 [PATCH v2 0/2] media: Add colors' order over test image Kaaira Gupta 2020-06-14 20:02 ` [PATCH v2 1/2] media: tpg: Add function to return colors' order of " Kaaira Gupta @ 2020-06-14 20:02 ` Kaaira Gupta 2020-06-15 11:48 ` Kieran Bingham 1 sibling, 1 reply; 7+ messages in thread From: Kaaira Gupta @ 2020-06-14 20:02 UTC (permalink / raw) To: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, Kieran Bingham, hverkuil Cc: Kaaira Gupta 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@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 | 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) #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@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_colors_order; /* 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; + } + + 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, + .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; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order 2020-06-14 20:02 ` [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order Kaaira Gupta @ 2020-06-15 11:48 ` Kieran Bingham 2020-06-15 14:17 ` Kaaira Gupta 0 siblings, 1 reply; 7+ messages in thread From: Kieran Bingham @ 2020-06-15 11:48 UTC (permalink / raw) To: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, hverkuil 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@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 | 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. > > #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@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_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. > /* 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? It might be reasonable to compile the kernel without the font-library ? > + } > + > + 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. > + .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; > > -- Regards -- Kieran ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order 2020-06-15 11:48 ` Kieran Bingham @ 2020-06-15 14:17 ` Kaaira Gupta 2020-06-15 15:55 ` Kieran Bingham 0 siblings, 1 reply; 7+ messages in thread From: Kaaira Gupta @ 2020-06-15 14:17 UTC (permalink / raw) To: Kieran Bingham Cc: Kaaira Gupta, Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, hverkuil 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@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 | 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@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_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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order 2020-06-15 14:17 ` Kaaira Gupta @ 2020-06-15 15:55 ` Kieran Bingham 0 siblings, 0 replies; 7+ messages in thread From: Kieran Bingham @ 2020-06-15 15:55 UTC (permalink / raw) To: Kaaira Gupta Cc: Helen Koike, Shuah Khan, Mauro Carvalho Chehab, linux-media, linux-kernel, hverkuil Hi Kaaira, On 15/06/2020 15:17, Kaaira Gupta wrote: > 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@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 | 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? This patch can be first and on it's own, but the key point is that once you define a control name - this becomes the public interface, so it can't change. (The kernel ABI interfaces should be stable). You couldn't later decide "oh - now we display more than just the TEST_PATTERN_ORDER, so we should rename it to VIMC_CID_INFO", (unless the patches haven't been merged) - so if you know you will add more information, we should just use a generic name from the start. > > Should the colors' order text be added to VIVID as well? Potentially yes, I think that would be useful if the code could be shared. I think your tpg_g_color_order() is already usable by vivid? >>> #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@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_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? show_info sounds good to me. > >> >> >>> /* 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? I think it's probably fine to keep the pr_err() or pr_warn() perhaps - but instead of return -ENODEV, set show_info = false; .... I don't think there's a need to stop the stream running if the system doesn't have a font. But maybe someone will disagree ... if the info is requested, but can't be displayed - perhaps it should be a failure :-S >> >> 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 Thanks for yours ;-) > >> >> -- >> Regards >> -- >> Kieran -- Regards -- Kieran ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-06-15 15:55 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-14 20:02 [PATCH v2 0/2] media: Add colors' order over test image Kaaira Gupta 2020-06-14 20:02 ` [PATCH v2 1/2] media: tpg: Add function to return colors' order of " Kaaira Gupta 2020-06-15 11:42 ` Kieran Bingham 2020-06-14 20:02 ` [PATCH v2 2/2] media: vimc: Add a control to show test pattern colors' order Kaaira Gupta 2020-06-15 11:48 ` Kieran Bingham 2020-06-15 14:17 ` Kaaira Gupta 2020-06-15 15:55 ` Kieran Bingham
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).