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 <[email protected]> >>> --- >>> 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 <[email protected]> >>> */ >>> >>> +#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
