[PATCH v3 0/2] media: Add colors' order and other info over test image

[PATCH v3 0/2] media: Add colors' order and other info over test image

[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. It then adds a control in vimc which displays the string 
over test image. It also displays some other information like saturation,
hue, contrast and brightness over test image generated by vimc.

Changes since v3:
	In 1st patch:
	-Improved formatting of returned string.

	In 2nd patch:
	 - Add CID prefix in control name and change it to a more
	   generic name.
	 - Rename bool variable to a generic name.
	 - Disable text rendering instead of stopping stream if no
	   font found.
	 - Display more info like VIVID in VIMC.

Changes since v2:
	In 1st patch:
	- Create a 'define' to prevent repetition of the common color
          sequence string.
        - Use 'fallthrough' on case statement to prevent repetition of
          code.

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 display info on test image

 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 | 47 ++++++++++++++++++-
 include/media/tpg/v4l2-tpg.h                  |  1 +
 5 files changed, 80 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] media: tpg: Add function to return colors' order of test image

[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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 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..31e6044a4104 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)
+{
+	#define COLORBAR(order) #order "white, yellow, cyan, green, magenta, red, blue, black"
+
+	switch (tpg->pattern) {
+	case TPG_PAT_75_COLORBAR:
+	case TPG_PAT_100_COLORBAR:
+	case TPG_PAT_CSC_COLORBAR:
+		return COLORBAR(Left to right:);
+	case TPG_PAT_100_HCOLORBAR:
+		return COLORBAR(Top to bottom:);
+	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

[PATCH v3 2/2] media: vimc: Add a control to display info on test image

[Kaaira Gupta]

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;
 	/* 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;
 
-- 
2.17.1

Re: [PATCH v3 2/2] media: vimc: Add a control to display info on test image

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26741 bytes --]

Re: [PATCH v3 2/2] media: vimc: Add a control to display info on test image

[Dafna Hirschfeld]

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;
>   
> 

Re: [PATCH v3 2/2] media: vimc: Add a control to display info on test image

[Kaaira Gupta]

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;
> > 

Re: [PATCH v3 1/2] media: tpg: Add function to return colors' order of test image

[Hans Verkuil]

On 18/06/2020 21:05, 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  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..31e6044a4104 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)

This returns a const char *.

> +{
> +	#define COLORBAR(order) #order "white, yellow, cyan, green, magenta, red, blue, black"
> +
> +	switch (tpg->pattern) {
> +	case TPG_PAT_75_COLORBAR:
> +	case TPG_PAT_100_COLORBAR:
> +	case TPG_PAT_CSC_COLORBAR:
> +		return COLORBAR(Left to right:);
> +	case TPG_PAT_100_HCOLORBAR:

Just combine this with the other and just return the sequence:

		return "White, Yellow, Cyan, Green, Magenta, Red, Blue, Black";

It's obvious from the pattern that three are from left to right and one is from
top to bottom. No need to mention that here.

Regards,

	Hans

> +		return COLORBAR(Top to bottom:);
> +	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)
>  {
> 

Re: [PATCH v3 2/2] media: vimc: Add a control to display info on test image

[Hans Verkuil]

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

Re: [PATCH v3 2/2] media: vimc: Add a control to display info on test image

[Dafna Hirschfeld]

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;
>>>