[PATCH v3 0/9] OMAP3 ISP preview engine configuration improvement

[PATCH v3 0/9] OMAP3 ISP preview engine configuration improvement

[Laurent Pinchart]

Hello,

Here's the third version of the OMAP3 ISP preview engine configuration
improvement patches.

Patches 1 to 3 have already been acked and haven't changed since v1. Patches 4
to 8 are new small improvements and cleanups, patch 9 is a complete rework of
the shadow update issue fix previously submitted as 4/4 in v2.

Laurent Pinchart (9):
  omap3isp: preview: Skip brightness and contrast in configuration
    ioctl
  omap3isp: preview: Optimize parameters setup for the common case
  omap3isp: preview: Remove averager parameter update flag
  omap3isp: preview: Remove unused isptables_update structure
    definition
  omap3isp: preview: Merge configuration and feature bits
  omap3isp: preview: Remove update_attrs feature_bit field
  omap3isp: preview: Rename prev_params fields to match userspace API
  omap3isp: preview: Simplify configuration parameters access
  omap3isp: preview: Shorten shadow update delay

 drivers/media/video/omap3isp/isppreview.c |  511 +++++++++++++++++------------
 drivers/media/video/omap3isp/isppreview.h |   76 ++---
 2 files changed, 326 insertions(+), 261 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/9] omap3isp: preview: Skip brightness and contrast in configuration ioctl

[Laurent Pinchart]

Brightness and contrast are handled through V4L2 controls. Their
configuration bit in the preview engine update attributes table is set
to -1 to reflect that. However, the VIDIOC_OMAP3ISP_PRV_CFG ioctl
handler doesn't handle -1 correctly as a configuration bit value, and
erroneously considers that the parameter has been selected for update by
the ioctl caller. Fix this.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/isppreview.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 6d0fb2c..cf5014f 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -903,7 +903,7 @@ static int preview_config(struct isp_prev_device *prev,
 		attr = &update_attrs[i];
 		bit = 0;
 
-		if (!(cfg->update & attr->cfg_bit))
+		if (attr->cfg_bit == -1 || !(cfg->update & attr->cfg_bit))
 			continue;
 
 		bit = cfg->flag & attr->cfg_bit;
-- 
1.7.3.4

[PATCH v3 2/9] omap3isp: preview: Optimize parameters setup for the common case

[Laurent Pinchart]

If no parameter needs to be modified, make preview_config() and
preview_setup_hw() return immediately. This speeds up interrupt handling
in the common case.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/isppreview.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index cf5014f..4e803a3 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -890,6 +890,8 @@ static int preview_config(struct isp_prev_device *prev,
 	int i, bit, rval = 0;
 
 	params = &prev->params;
+	if (cfg->update == 0)
+		return 0;
 
 	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
 		unsigned long flags;
@@ -944,6 +946,9 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 	int i, bit;
 	void *param_ptr;
 
+	if (prev->update == 0)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
 		attr = &update_attrs[i];
 
-- 
1.7.3.4

Re: [PATCH v3 2/9] omap3isp: preview: Optimize parameters setup for the common case

[Sakari Ailus]

Hi Laurent,

Thanks for the patch. Just one more comment.

Laurent Pinchart wrote:
> If no parameter needs to be modified, make preview_config() and
> preview_setup_hw() return immediately. This speeds up interrupt handling
> in the common case.
>
> Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>
> Acked-by: Sakari Ailus<sakari.ailus@iki.fi>
> ---
>   drivers/media/video/omap3isp/isppreview.c |    5 +++++
>   1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index cf5014f..4e803a3 100644
> --- a/drivers/media/video/omap3isp/isppreview.c
> +++ b/drivers/media/video/omap3isp/isppreview.c
> @@ -890,6 +890,8 @@ static int preview_config(struct isp_prev_device *prev,
>   	int i, bit, rval = 0;
>
>   	params =&prev->params;
> +	if (cfg->update == 0)
> +		return 0;

You could do the check right at the beginning of the function, i.e. 
before settings params. Tiny issue, but it'd look better that way. :-)

>   	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
>   		unsigned long flags;
> @@ -944,6 +946,9 @@ static void preview_setup_hw(struct isp_prev_device *prev)
>   	int i, bit;
>   	void *param_ptr;
>
> +	if (prev->update == 0)
> +		return;
> +
>   	for (i = 0; i<  ARRAY_SIZE(update_attrs); i++) {
>   		attr =&update_attrs[i];
>


-- 
Sakari Ailus
sakari.ailus@iki.fi

[PATCH v3 3/9] omap3isp: preview: Remove averager parameter update flag

[Laurent Pinchart]

The flag isn't used, remove it.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/media/video/omap3isp/isppreview.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index 0968660..67723c7 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -66,8 +66,7 @@
 
 #define PREV_CONTRAST			(1 << 17)
 #define PREV_BRIGHTNESS			(1 << 18)
-#define PREV_AVERAGER			(1 << 19)
-#define PREV_FEATURES_END		(1 << 20)
+#define PREV_FEATURES_END		(1 << 19)
 
 enum preview_input_entity {
 	PREVIEW_INPUT_NONE,
-- 
1.7.3.4

[PATCH v3 4/9] omap3isp: preview: Remove unused isptables_update structure definition

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.h |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index 67723c7..b7f979a 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -121,26 +121,6 @@ struct prev_params {
 	u8 brightness;
 };
 
-/*
- * struct isptables_update - Structure for Table Configuration.
- * @update: Specifies which tables should be updated.
- * @flag: Specifies which tables should be enabled.
- * @nf: Pointer to structure for Noise Filter
- * @lsc: Pointer to LSC gain table. (currently not used)
- * @gamma: Pointer to gamma correction tables.
- * @cfa: Pointer to color filter array configuration.
- * @wbal: Pointer to colour and digital gain configuration.
- */
-struct isptables_update {
-	u32 update;
-	u32 flag;
-	struct omap3isp_prev_nf *nf;
-	u32 *lsc;
-	struct omap3isp_prev_gtables *gamma;
-	struct omap3isp_prev_cfa *cfa;
-	struct omap3isp_prev_wbal *wbal;
-};
-
 /* Sink and source previewer pads */
 #define PREV_PAD_SINK			0
 #define PREV_PAD_SOURCE			1
-- 
1.7.3.4

[PATCH v3 5/9] omap3isp: preview: Merge configuration and feature bits

[Laurent Pinchart]

The preview engine parameters are referenced by a value suitable for
being used in a bitmask. Two macros named OMAP3ISP_PREV_* and PREV_* are
declared for each parameter and are used interchangeably. Remove the
private macro.

Replace the configuration bit field in the parameter update attributes
structure with a boolean that indicates whether the parameter can be
updated through the preview engine configuration ioctl.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  103 +++++++++++++++--------------
 drivers/media/video/omap3isp/isppreview.h |   26 +------
 2 files changed, 57 insertions(+), 72 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 4e803a3..da031c1 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -653,7 +653,7 @@ preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
 
 	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
 		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
-		prev->update |= PREV_CONTRAST;
+		prev->update |= OMAP3ISP_PREV_CONTRAST;
 	}
 }
 
@@ -685,7 +685,7 @@ preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
 
 	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
 		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
-		prev->update |= PREV_BRIGHTNESS;
+		prev->update |= OMAP3ISP_PREV_BRIGHTNESS;
 	}
 }
 
@@ -723,70 +723,70 @@ preview_config_yc_range(struct isp_prev_device *prev, const void *yclimit)
 
 /* preview parameters update structure */
 struct preview_update {
-	int cfg_bit;
 	int feature_bit;
 	void (*config)(struct isp_prev_device *, const void *);
 	void (*enable)(struct isp_prev_device *, u8);
+	bool skip;
 };
 
 static struct preview_update update_attrs[] = {
-	{OMAP3ISP_PREV_LUMAENH, PREV_LUMA_ENHANCE,
+	{OMAP3ISP_PREV_LUMAENH,
 		preview_config_luma_enhancement,
 		preview_enable_luma_enhancement},
-	{OMAP3ISP_PREV_INVALAW, PREV_INVERSE_ALAW,
+	{OMAP3ISP_PREV_INVALAW,
 		NULL,
 		preview_enable_invalaw},
-	{OMAP3ISP_PREV_HRZ_MED, PREV_HORZ_MEDIAN_FILTER,
+	{OMAP3ISP_PREV_HRZ_MED,
 		preview_config_hmed,
 		preview_enable_hmed},
-	{OMAP3ISP_PREV_CFA, PREV_CFA,
+	{OMAP3ISP_PREV_CFA,
 		preview_config_cfa,
 		preview_enable_cfa},
-	{OMAP3ISP_PREV_CHROMA_SUPP, PREV_CHROMA_SUPPRESS,
+	{OMAP3ISP_PREV_CHROMA_SUPP,
 		preview_config_chroma_suppression,
 		preview_enable_chroma_suppression},
-	{OMAP3ISP_PREV_WB, PREV_WB,
+	{OMAP3ISP_PREV_WB,
 		preview_config_whitebalance,
 		NULL},
-	{OMAP3ISP_PREV_BLKADJ, PREV_BLKADJ,
+	{OMAP3ISP_PREV_BLKADJ,
 		preview_config_blkadj,
 		NULL},
-	{OMAP3ISP_PREV_RGB2RGB, PREV_RGB2RGB,
+	{OMAP3ISP_PREV_RGB2RGB,
 		preview_config_rgb_blending,
 		NULL},
-	{OMAP3ISP_PREV_COLOR_CONV, PREV_COLOR_CONV,
+	{OMAP3ISP_PREV_COLOR_CONV,
 		preview_config_rgb_to_ycbcr,
 		NULL},
-	{OMAP3ISP_PREV_YC_LIMIT, PREV_YCLIMITS,
+	{OMAP3ISP_PREV_YC_LIMIT,
 		preview_config_yc_range,
 		NULL},
-	{OMAP3ISP_PREV_DEFECT_COR, PREV_DEFECT_COR,
+	{OMAP3ISP_PREV_DEFECT_COR,
 		preview_config_dcor,
 		preview_enable_dcor},
-	{OMAP3ISP_PREV_GAMMABYPASS, PREV_GAMMA_BYPASS,
+	{OMAP3ISP_PREV_GAMMABYPASS,
 		NULL,
 		preview_enable_gammabypass},
-	{OMAP3ISP_PREV_DRK_FRM_CAPTURE, PREV_DARK_FRAME_CAPTURE,
+	{OMAP3ISP_PREV_DRK_FRM_CAPTURE,
 		NULL,
 		preview_enable_drkframe_capture},
-	{OMAP3ISP_PREV_DRK_FRM_SUBTRACT, PREV_DARK_FRAME_SUBTRACT,
+	{OMAP3ISP_PREV_DRK_FRM_SUBTRACT,
 		NULL,
 		preview_enable_drkframe},
-	{OMAP3ISP_PREV_LENS_SHADING, PREV_LENS_SHADING,
+	{OMAP3ISP_PREV_LENS_SHADING,
 		preview_config_drkf_shadcomp,
 		preview_enable_drkframe},
-	{OMAP3ISP_PREV_NF, PREV_NOISE_FILTER,
+	{OMAP3ISP_PREV_NF,
 		preview_config_noisefilter,
 		preview_enable_noisefilter},
-	{OMAP3ISP_PREV_GAMMA, PREV_GAMMA,
+	{OMAP3ISP_PREV_GAMMA,
 		preview_config_gammacorrn,
 		NULL},
-	{-1, PREV_CONTRAST,
+	{OMAP3ISP_PREV_CONTRAST,
 		preview_config_contrast,
-		NULL},
-	{-1, PREV_BRIGHTNESS,
+		NULL, true},
+	{OMAP3ISP_PREV_BRIGHTNESS,
 		preview_config_brightness,
-		NULL},
+		NULL, true},
 };
 
 /*
@@ -810,59 +810,59 @@ __preview_get_ptrs(struct prev_params *params, void **param,
 	}
 
 	switch (bit) {
-	case PREV_HORZ_MEDIAN_FILTER:
+	case OMAP3ISP_PREV_HRZ_MED:
 		*param = &params->hmed;
 		CHKARG(configs, config, hmed)
 		return sizeof(params->hmed);
-	case PREV_NOISE_FILTER:
+	case OMAP3ISP_PREV_NF:
 		*param = &params->nf;
 		CHKARG(configs, config, nf)
 		return sizeof(params->nf);
 		break;
-	case PREV_CFA:
+	case OMAP3ISP_PREV_CFA:
 		*param = &params->cfa;
 		CHKARG(configs, config, cfa)
 		return sizeof(params->cfa);
-	case PREV_LUMA_ENHANCE:
+	case OMAP3ISP_PREV_LUMAENH:
 		*param = &params->luma;
 		CHKARG(configs, config, luma)
 		return sizeof(params->luma);
-	case PREV_CHROMA_SUPPRESS:
+	case OMAP3ISP_PREV_CHROMA_SUPP:
 		*param = &params->csup;
 		CHKARG(configs, config, csup)
 		return sizeof(params->csup);
-	case PREV_DEFECT_COR:
+	case OMAP3ISP_PREV_DEFECT_COR:
 		*param = &params->dcor;
 		CHKARG(configs, config, dcor)
 		return sizeof(params->dcor);
-	case PREV_BLKADJ:
+	case OMAP3ISP_PREV_BLKADJ:
 		*param = &params->blk_adj;
 		CHKARG(configs, config, blkadj)
 		return sizeof(params->blk_adj);
-	case PREV_YCLIMITS:
+	case OMAP3ISP_PREV_YC_LIMIT:
 		*param = &params->yclimit;
 		CHKARG(configs, config, yclimit)
 		return sizeof(params->yclimit);
-	case PREV_RGB2RGB:
+	case OMAP3ISP_PREV_RGB2RGB:
 		*param = &params->rgb2rgb;
 		CHKARG(configs, config, rgb2rgb)
 		return sizeof(params->rgb2rgb);
-	case PREV_COLOR_CONV:
+	case OMAP3ISP_PREV_COLOR_CONV:
 		*param = &params->rgb2ycbcr;
 		CHKARG(configs, config, csc)
 		return sizeof(params->rgb2ycbcr);
-	case PREV_WB:
+	case OMAP3ISP_PREV_WB:
 		*param = &params->wbal;
 		CHKARG(configs, config, wbal)
 		return sizeof(params->wbal);
-	case PREV_GAMMA:
+	case OMAP3ISP_PREV_GAMMA:
 		*param = &params->gamma;
 		CHKARG(configs, config, gamma)
 		return sizeof(params->gamma);
-	case PREV_CONTRAST:
+	case OMAP3ISP_PREV_CONTRAST:
 		*param = &params->contrast;
 		return 0;
-	case PREV_BRIGHTNESS:
+	case OMAP3ISP_PREV_BRIGHTNESS:
 		*param = &params->brightness;
 		return 0;
 	default:
@@ -905,10 +905,10 @@ static int preview_config(struct isp_prev_device *prev,
 		attr = &update_attrs[i];
 		bit = 0;
 
-		if (attr->cfg_bit == -1 || !(cfg->update & attr->cfg_bit))
+		if (attr->skip || !(cfg->update & attr->feature_bit))
 			continue;
 
-		bit = cfg->flag & attr->cfg_bit;
+		bit = cfg->flag & attr->feature_bit;
 		if (bit) {
 			void *to = NULL, __user *from = NULL;
 			unsigned long sz = 0;
@@ -1038,23 +1038,24 @@ static void preview_config_input_size(struct isp_prev_device *prev)
 	unsigned int slv = prev->crop.top;
 	unsigned int elv = prev->crop.top + prev->crop.height - 1;
 
-	if (params->features & PREV_CFA) {
+	if (params->features & OMAP3ISP_PREV_CFA) {
 		sph -= 2;
 		eph += 2;
 		slv -= 2;
 		elv += 2;
 	}
-	if (params->features & (PREV_DEFECT_COR | PREV_NOISE_FILTER)) {
+	if (params->features & (OMAP3ISP_PREV_DEFECT_COR | OMAP3ISP_PREV_NF)) {
 		sph -= 2;
 		eph += 2;
 		slv -= 2;
 		elv += 2;
 	}
-	if (params->features & PREV_HORZ_MEDIAN_FILTER) {
+	if (params->features & OMAP3ISP_PREV_HRZ_MED) {
 		sph -= 2;
 		eph += 2;
 	}
-	if (params->features & (PREV_CHROMA_SUPPRESS | PREV_LUMA_ENHANCE))
+	if (params->features & (OMAP3ISP_PREV_CHROMA_SUPP |
+				OMAP3ISP_PREV_LUMAENH))
 		sph -= 2;
 
 	isp_reg_writel(isp, (sph << ISPPRV_HORZ_INFO_SPH_SHIFT) | eph,
@@ -1189,7 +1190,7 @@ int omap3isp_preview_busy(struct isp_prev_device *prev)
  */
 void omap3isp_preview_restore_context(struct isp_device *isp)
 {
-	isp->isp_prev.update = PREV_FEATURES_END - 1;
+	isp->isp_prev.update = OMAP3ISP_PREV_FEATURES_END - 1;
 	preview_setup_hw(&isp->isp_prev);
 }
 
@@ -1292,12 +1293,14 @@ static void preview_init_params(struct isp_prev_device *prev)
 	params->yclimit.minY = ISPPRV_YC_MIN;
 	params->yclimit.maxY = ISPPRV_YC_MAX;
 
-	params->features = PREV_CFA | PREV_DEFECT_COR | PREV_NOISE_FILTER
-			 | PREV_GAMMA | PREV_BLKADJ | PREV_YCLIMITS
-			 | PREV_RGB2RGB | PREV_COLOR_CONV | PREV_WB
-			 | PREV_BRIGHTNESS | PREV_CONTRAST;
+	params->features = OMAP3ISP_PREV_CFA | OMAP3ISP_PREV_DEFECT_COR
+			 | OMAP3ISP_PREV_NF | OMAP3ISP_PREV_GAMMA
+			 | OMAP3ISP_PREV_BLKADJ | OMAP3ISP_PREV_YC_LIMIT
+			 | OMAP3ISP_PREV_RGB2RGB | OMAP3ISP_PREV_COLOR_CONV
+			 | OMAP3ISP_PREV_WB | OMAP3ISP_PREV_BRIGHTNESS
+			 | OMAP3ISP_PREV_CONTRAST;
 
-	prev->update = PREV_FEATURES_END - 1;
+	prev->update = OMAP3ISP_PREV_FEATURES_END - 1;
 }
 
 /*
diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index b7f979a..a0d2807 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -45,28 +45,10 @@
 #define ISPPRV_CONTRAST_HIGH		0xFF
 #define ISPPRV_CONTRAST_UNITS		0x1
 
-/* Features list */
-#define PREV_LUMA_ENHANCE		OMAP3ISP_PREV_LUMAENH
-#define PREV_INVERSE_ALAW		OMAP3ISP_PREV_INVALAW
-#define PREV_HORZ_MEDIAN_FILTER		OMAP3ISP_PREV_HRZ_MED
-#define PREV_CFA			OMAP3ISP_PREV_CFA
-#define PREV_CHROMA_SUPPRESS		OMAP3ISP_PREV_CHROMA_SUPP
-#define PREV_WB				OMAP3ISP_PREV_WB
-#define PREV_BLKADJ			OMAP3ISP_PREV_BLKADJ
-#define PREV_RGB2RGB			OMAP3ISP_PREV_RGB2RGB
-#define PREV_COLOR_CONV			OMAP3ISP_PREV_COLOR_CONV
-#define PREV_YCLIMITS			OMAP3ISP_PREV_YC_LIMIT
-#define PREV_DEFECT_COR			OMAP3ISP_PREV_DEFECT_COR
-#define PREV_GAMMA_BYPASS		OMAP3ISP_PREV_GAMMABYPASS
-#define PREV_DARK_FRAME_CAPTURE		OMAP3ISP_PREV_DRK_FRM_CAPTURE
-#define PREV_DARK_FRAME_SUBTRACT	OMAP3ISP_PREV_DRK_FRM_SUBTRACT
-#define PREV_LENS_SHADING		OMAP3ISP_PREV_LENS_SHADING
-#define PREV_NOISE_FILTER		OMAP3ISP_PREV_NF
-#define PREV_GAMMA			OMAP3ISP_PREV_GAMMA
-
-#define PREV_CONTRAST			(1 << 17)
-#define PREV_BRIGHTNESS			(1 << 18)
-#define PREV_FEATURES_END		(1 << 19)
+/* Additional features not listed in linux/omap3isp.h */
+#define OMAP3ISP_PREV_CONTRAST		(1 << 17)
+#define OMAP3ISP_PREV_BRIGHTNESS	(1 << 18)
+#define OMAP3ISP_PREV_FEATURES_END	(1 << 19)
 
 enum preview_input_entity {
 	PREVIEW_INPUT_NONE,
-- 
1.7.3.4

Re: [PATCH v3 5/9] omap3isp: preview: Merge configuration and feature bits

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

Laurent Pinchart wrote:
> The preview engine parameters are referenced by a value suitable for
> being used in a bitmask. Two macros named OMAP3ISP_PREV_* and PREV_* are
> declared for each parameter and are used interchangeably. Remove the
> private macro.
>
> Replace the configuration bit field in the parameter update attributes
> structure with a boolean that indicates whether the parameter can be
> updated through the preview engine configuration ioctl.
>
> Signed-off-by: Laurent Pinchart<laurent.pinchart@ideasonboard.com>

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
sakari.ailus@iki.fi

[PATCH v3 6/9] omap3isp: preview: Remove update_attrs feature_bit field

[Laurent Pinchart]

The feature_bit is always equal to 1 << array position, remove it and
compute the value dynamically.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  107 +++++++++++++++--------------
 1 files changed, 54 insertions(+), 53 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index da031c1..c487995 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -723,70 +723,71 @@ preview_config_yc_range(struct isp_prev_device *prev, const void *yclimit)
 
 /* preview parameters update structure */
 struct preview_update {
-	int feature_bit;
 	void (*config)(struct isp_prev_device *, const void *);
 	void (*enable)(struct isp_prev_device *, u8);
 	bool skip;
 };
 
+/* Keep the array indexed by the OMAP3ISP_PREV_* bit number. */
 static struct preview_update update_attrs[] = {
-	{OMAP3ISP_PREV_LUMAENH,
+	/* OMAP3ISP_PREV_LUMAENH */ {
 		preview_config_luma_enhancement,
-		preview_enable_luma_enhancement},
-	{OMAP3ISP_PREV_INVALAW,
+		preview_enable_luma_enhancement,
+	}, /* OMAP3ISP_PREV_INVALAW */ {
 		NULL,
-		preview_enable_invalaw},
-	{OMAP3ISP_PREV_HRZ_MED,
+		preview_enable_invalaw,
+	}, /* OMAP3ISP_PREV_HRZ_MED */ {
 		preview_config_hmed,
-		preview_enable_hmed},
-	{OMAP3ISP_PREV_CFA,
+		preview_enable_hmed,
+	}, /* OMAP3ISP_PREV_CFA */ {
 		preview_config_cfa,
-		preview_enable_cfa},
-	{OMAP3ISP_PREV_CHROMA_SUPP,
+		preview_enable_cfa,
+	}, /* OMAP3ISP_PREV_CHROMA_SUPP */ {
 		preview_config_chroma_suppression,
-		preview_enable_chroma_suppression},
-	{OMAP3ISP_PREV_WB,
+		preview_enable_chroma_suppression,
+	}, /* OMAP3ISP_PREV_WB */ {
 		preview_config_whitebalance,
-		NULL},
-	{OMAP3ISP_PREV_BLKADJ,
+		NULL,
+	}, /* OMAP3ISP_PREV_BLKADJ */ {
 		preview_config_blkadj,
-		NULL},
-	{OMAP3ISP_PREV_RGB2RGB,
+		NULL,
+	}, /* OMAP3ISP_PREV_RGB2RGB */ {
 		preview_config_rgb_blending,
-		NULL},
-	{OMAP3ISP_PREV_COLOR_CONV,
+		NULL,
+	}, /* OMAP3ISP_PREV_COLOR_CONV */ {
 		preview_config_rgb_to_ycbcr,
-		NULL},
-	{OMAP3ISP_PREV_YC_LIMIT,
+		NULL,
+	}, /* OMAP3ISP_PREV_YC_LIMIT */ {
 		preview_config_yc_range,
-		NULL},
-	{OMAP3ISP_PREV_DEFECT_COR,
+		NULL,
+	}, /* OMAP3ISP_PREV_DEFECT_COR */ {
 		preview_config_dcor,
-		preview_enable_dcor},
-	{OMAP3ISP_PREV_GAMMABYPASS,
+		preview_enable_dcor,
+	}, /* OMAP3ISP_PREV_GAMMABYPASS */ {
 		NULL,
-		preview_enable_gammabypass},
-	{OMAP3ISP_PREV_DRK_FRM_CAPTURE,
+		preview_enable_gammabypass,
+	}, /* OMAP3ISP_PREV_DRK_FRM_CAPTURE */ {
 		NULL,
-		preview_enable_drkframe_capture},
-	{OMAP3ISP_PREV_DRK_FRM_SUBTRACT,
+		preview_enable_drkframe_capture,
+	}, /* OMAP3ISP_PREV_DRK_FRM_SUBTRACT */ {
 		NULL,
-		preview_enable_drkframe},
-	{OMAP3ISP_PREV_LENS_SHADING,
+		preview_enable_drkframe,
+	}, /* OMAP3ISP_PREV_LENS_SHADING */ {
 		preview_config_drkf_shadcomp,
-		preview_enable_drkframe},
-	{OMAP3ISP_PREV_NF,
+		preview_enable_drkframe,
+	}, /* OMAP3ISP_PREV_NF */ {
 		preview_config_noisefilter,
-		preview_enable_noisefilter},
-	{OMAP3ISP_PREV_GAMMA,
+		preview_enable_noisefilter,
+	}, /* OMAP3ISP_PREV_GAMMA */ {
 		preview_config_gammacorrn,
-		NULL},
-	{OMAP3ISP_PREV_CONTRAST,
+		NULL,
+	}, /* OMAP3ISP_PREV_CONTRAST */ {
 		preview_config_contrast,
-		NULL, true},
-	{OMAP3ISP_PREV_BRIGHTNESS,
+		NULL, true,
+	}, /* OMAP3ISP_PREV_BRIGHTNESS */ {
 		preview_config_brightness,
-		NULL, true},
+		NULL, true,
+	},
 };
 
 /*
@@ -903,30 +904,28 @@ static int preview_config(struct isp_prev_device *prev,
 
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
 		attr = &update_attrs[i];
-		bit = 0;
+		bit = 1 << i;
 
-		if (attr->skip || !(cfg->update & attr->feature_bit))
+		if (attr->skip || !(cfg->update & bit))
 			continue;
 
-		bit = cfg->flag & attr->feature_bit;
-		if (bit) {
+		if (cfg->flag & bit) {
 			void *to = NULL, __user *from = NULL;
 			unsigned long sz = 0;
 
-			sz = __preview_get_ptrs(params, &to, cfg, &from,
-						   bit);
+			sz = __preview_get_ptrs(params, &to, cfg, &from, bit);
 			if (to && from && sz) {
 				if (copy_from_user(to, from, sz)) {
 					rval = -EFAULT;
 					break;
 				}
 			}
-			params->features |= attr->feature_bit;
+			params->features |= bit;
 		} else {
-			params->features &= ~attr->feature_bit;
+			params->features &= ~bit;
 		}
 
-		prev->update |= attr->feature_bit;
+		prev->update |= bit;
 	}
 
 	prev->shadow_update = 0;
@@ -943,7 +942,8 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 {
 	struct prev_params *params = &prev->params;
 	struct preview_update *attr;
-	int i, bit;
+	unsigned int bit;
+	unsigned int i;
 	void *param_ptr;
 
 	if (prev->update == 0)
@@ -951,11 +951,12 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
 		attr = &update_attrs[i];
+		bit = 1 << i;
 
-		if (!(prev->update & attr->feature_bit))
+		if (!(prev->update & bit))
 			continue;
-		bit = params->features & attr->feature_bit;
-		if (bit) {
+
+		if (params->features & bit) {
 			if (attr->config) {
 				__preview_get_ptrs(params, &param_ptr, NULL,
 						      NULL, bit);
@@ -967,7 +968,7 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 			if (attr->enable)
 				attr->enable(prev, 0);
 
-		prev->update &= ~attr->feature_bit;
+		prev->update &= ~bit;
 	}
 }
 
-- 
1.7.3.4

Re: [PATCH v3 6/9] omap3isp: preview: Remove update_attrs feature_bit field

[Sakari Ailus]

Hi Laurent,

Thanks for the patch!

On Mon, Apr 16, 2012 at 03:29:51PM +0200, Laurent Pinchart wrote:
> The feature_bit is always equal to 1 << array position, remove it and
> compute the value dynamically.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

[PATCH v3 7/9] omap3isp: preview: Rename prev_params fields to match userspace API

[Laurent Pinchart]

Rename the blk_adj and rgb2ycbcr fields to blkadj and csc respectively.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |   16 ++++++++--------
 drivers/media/video/omap3isp/isppreview.h |    8 ++++----
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index c487995..b75b675 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -837,9 +837,9 @@ __preview_get_ptrs(struct prev_params *params, void **param,
 		CHKARG(configs, config, dcor)
 		return sizeof(params->dcor);
 	case OMAP3ISP_PREV_BLKADJ:
-		*param = &params->blk_adj;
+		*param = &params->blkadj;
 		CHKARG(configs, config, blkadj)
-		return sizeof(params->blk_adj);
+		return sizeof(params->blkadj);
 	case OMAP3ISP_PREV_YC_LIMIT:
 		*param = &params->yclimit;
 		CHKARG(configs, config, yclimit)
@@ -849,9 +849,9 @@ __preview_get_ptrs(struct prev_params *params, void **param,
 		CHKARG(configs, config, rgb2rgb)
 		return sizeof(params->rgb2rgb);
 	case OMAP3ISP_PREV_COLOR_CONV:
-		*param = &params->rgb2ycbcr;
+		*param = &params->csc;
 		CHKARG(configs, config, csc)
-		return sizeof(params->rgb2ycbcr);
+		return sizeof(params->csc);
 	case OMAP3ISP_PREV_WB:
 		*param = &params->wbal;
 		CHKARG(configs, config, wbal)
@@ -1284,11 +1284,11 @@ static void preview_init_params(struct isp_prev_device *prev)
 	params->wbal.coef1 = FLR_WBAL_COEF;
 	params->wbal.coef2 = FLR_WBAL_COEF;
 	params->wbal.coef3 = FLR_WBAL_COEF;
-	params->blk_adj.red = FLR_BLKADJ_RED;
-	params->blk_adj.green = FLR_BLKADJ_GREEN;
-	params->blk_adj.blue = FLR_BLKADJ_BLUE;
+	params->blkadj.red = FLR_BLKADJ_RED;
+	params->blkadj.green = FLR_BLKADJ_GREEN;
+	params->blkadj.blue = FLR_BLKADJ_BLUE;
 	params->rgb2rgb = flr_rgb2rgb;
-	params->rgb2ycbcr = flr_prev_csc;
+	params->csc = flr_prev_csc;
 	params->yclimit.minC = ISPPRV_YC_MIN;
 	params->yclimit.maxC = ISPPRV_YC_MAX;
 	params->yclimit.minY = ISPPRV_YC_MIN;
diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index a0d2807..6ee8306 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -77,9 +77,9 @@ enum preview_ycpos_mode {
  * @dcor: Noise filter coefficients.
  * @gamma: Gamma coefficients.
  * @wbal: White Balance parameters.
- * @blk_adj: Black adjustment parameters.
+ * @blkadj: Black adjustment parameters.
  * @rgb2rgb: RGB blending parameters.
- * @rgb2ycbcr: RGB to ycbcr parameters.
+ * @csc: Color space conversion (RGB to YCbCr) parameters.
  * @hmed: Horizontal median filter.
  * @yclimit: YC limits parameters.
  * @contrast: Contrast.
@@ -94,9 +94,9 @@ struct prev_params {
 	struct omap3isp_prev_dcor dcor;
 	struct omap3isp_prev_gtables gamma;
 	struct omap3isp_prev_wbal wbal;
-	struct omap3isp_prev_blkadj blk_adj;
+	struct omap3isp_prev_blkadj blkadj;
 	struct omap3isp_prev_rgbtorgb rgb2rgb;
-	struct omap3isp_prev_csc rgb2ycbcr;
+	struct omap3isp_prev_csc csc;
 	struct omap3isp_prev_hmed hmed;
 	struct omap3isp_prev_yclimit yclimit;
 	u8 contrast;
-- 
1.7.3.4

Re: [PATCH v3 7/9] omap3isp: preview: Rename prev_params fields to match userspace API

[Sakari Ailus]

hi Laurent,

On Mon, Apr 16, 2012 at 03:29:52PM +0200, Laurent Pinchart wrote:
> Rename the blk_adj and rgb2ycbcr fields to blkadj and csc respectively.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

[PATCH v3 8/9] omap3isp: preview: Simplify configuration parameters access

[Laurent Pinchart]

Instead of using a large switch/case statement to return offsets to and
sizes of individual preview engine parameters in the parameters and
configuration structures, store the information in the update_attrs
table and use it at runtime.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  134 +++++++++++++---------------
 1 files changed, 62 insertions(+), 72 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index b75b675..e12df2c 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -725,44 +725,77 @@ preview_config_yc_range(struct isp_prev_device *prev, const void *yclimit)
 struct preview_update {
 	void (*config)(struct isp_prev_device *, const void *);
 	void (*enable)(struct isp_prev_device *, u8);
+	unsigned int param_offset;
+	unsigned int param_size;
+	unsigned int config_offset;
 	bool skip;
 };
 
 /* Keep the array indexed by the OMAP3ISP_PREV_* bit number. */
-static struct preview_update update_attrs[] = {
+static const struct preview_update update_attrs[] = {
 	/* OMAP3ISP_PREV_LUMAENH */ {
 		preview_config_luma_enhancement,
 		preview_enable_luma_enhancement,
+		offsetof(struct prev_params, luma),
+		FIELD_SIZEOF(struct prev_params, luma),
+		offsetof(struct omap3isp_prev_update_config, luma),
 	}, /* OMAP3ISP_PREV_INVALAW */ {
 		NULL,
 		preview_enable_invalaw,
 	}, /* OMAP3ISP_PREV_HRZ_MED */ {
 		preview_config_hmed,
 		preview_enable_hmed,
+		offsetof(struct prev_params, hmed),
+		FIELD_SIZEOF(struct prev_params, hmed),
+		offsetof(struct omap3isp_prev_update_config, hmed),
 	}, /* OMAP3ISP_PREV_CFA */ {
 		preview_config_cfa,
 		preview_enable_cfa,
+		offsetof(struct prev_params, cfa),
+		FIELD_SIZEOF(struct prev_params, cfa),
+		offsetof(struct omap3isp_prev_update_config, cfa),
 	}, /* OMAP3ISP_PREV_CHROMA_SUPP */ {
 		preview_config_chroma_suppression,
 		preview_enable_chroma_suppression,
+		offsetof(struct prev_params, csup),
+		FIELD_SIZEOF(struct prev_params, csup),
+		offsetof(struct omap3isp_prev_update_config, csup),
 	}, /* OMAP3ISP_PREV_WB */ {
 		preview_config_whitebalance,
 		NULL,
+		offsetof(struct prev_params, wbal),
+		FIELD_SIZEOF(struct prev_params, wbal),
+		offsetof(struct omap3isp_prev_update_config, wbal),
 	}, /* OMAP3ISP_PREV_BLKADJ */ {
 		preview_config_blkadj,
 		NULL,
+		offsetof(struct prev_params, blkadj),
+		FIELD_SIZEOF(struct prev_params, blkadj),
+		offsetof(struct omap3isp_prev_update_config, blkadj),
 	}, /* OMAP3ISP_PREV_RGB2RGB */ {
 		preview_config_rgb_blending,
 		NULL,
+		offsetof(struct prev_params, rgb2rgb),
+		FIELD_SIZEOF(struct prev_params, rgb2rgb),
+		offsetof(struct omap3isp_prev_update_config, rgb2rgb),
 	}, /* OMAP3ISP_PREV_COLOR_CONV */ {
 		preview_config_rgb_to_ycbcr,
 		NULL,
+		offsetof(struct prev_params, csc),
+		FIELD_SIZEOF(struct prev_params, csc),
+		offsetof(struct omap3isp_prev_update_config, csc),
 	}, /* OMAP3ISP_PREV_YC_LIMIT */ {
 		preview_config_yc_range,
 		NULL,
+		offsetof(struct prev_params, yclimit),
+		FIELD_SIZEOF(struct prev_params, yclimit),
+		offsetof(struct omap3isp_prev_update_config, yclimit),
 	}, /* OMAP3ISP_PREV_DEFECT_COR */ {
 		preview_config_dcor,
 		preview_enable_dcor,
+		offsetof(struct prev_params, dcor),
+		FIELD_SIZEOF(struct prev_params, dcor),
+		offsetof(struct omap3isp_prev_update_config, dcor),
 	}, /* OMAP3ISP_PREV_GAMMABYPASS */ {
 		NULL,
 		preview_enable_gammabypass,
@@ -778,15 +811,25 @@ static struct preview_update update_attrs[] = {
 	}, /* OMAP3ISP_PREV_NF */ {
 		preview_config_noisefilter,
 		preview_enable_noisefilter,
+		offsetof(struct prev_params, nf),
+		FIELD_SIZEOF(struct prev_params, nf),
+		offsetof(struct omap3isp_prev_update_config, nf),
 	}, /* OMAP3ISP_PREV_GAMMA */ {
 		preview_config_gammacorrn,
 		NULL,
+		offsetof(struct prev_params, gamma),
+		FIELD_SIZEOF(struct prev_params, gamma),
+		offsetof(struct omap3isp_prev_update_config, gamma),
 	}, /* OMAP3ISP_PREV_CONTRAST */ {
 		preview_config_contrast,
-		NULL, true,
+		NULL,
+		offsetof(struct prev_params, contrast),
+		0, true,
 	}, /* OMAP3ISP_PREV_BRIGHTNESS */ {
 		preview_config_brightness,
-		NULL, true,
+		NULL,
+		offsetof(struct prev_params, brightness),
+		0, true,
 	},
 };
 
@@ -803,75 +846,22 @@ static struct preview_update update_attrs[] = {
 static u32
 __preview_get_ptrs(struct prev_params *params, void **param,
 		   struct omap3isp_prev_update_config *configs,
-		   void __user **config, u32 bit)
+		   void __user **config, unsigned int index)
 {
-#define CHKARG(cfgs, cfg, field)				\
-	if (cfgs && cfg) {					\
-		*(cfg) = (cfgs)->field;				\
-	}
+	const struct preview_update *info = &update_attrs[index];
 
-	switch (bit) {
-	case OMAP3ISP_PREV_HRZ_MED:
-		*param = &params->hmed;
-		CHKARG(configs, config, hmed)
-		return sizeof(params->hmed);
-	case OMAP3ISP_PREV_NF:
-		*param = &params->nf;
-		CHKARG(configs, config, nf)
-		return sizeof(params->nf);
-		break;
-	case OMAP3ISP_PREV_CFA:
-		*param = &params->cfa;
-		CHKARG(configs, config, cfa)
-		return sizeof(params->cfa);
-	case OMAP3ISP_PREV_LUMAENH:
-		*param = &params->luma;
-		CHKARG(configs, config, luma)
-		return sizeof(params->luma);
-	case OMAP3ISP_PREV_CHROMA_SUPP:
-		*param = &params->csup;
-		CHKARG(configs, config, csup)
-		return sizeof(params->csup);
-	case OMAP3ISP_PREV_DEFECT_COR:
-		*param = &params->dcor;
-		CHKARG(configs, config, dcor)
-		return sizeof(params->dcor);
-	case OMAP3ISP_PREV_BLKADJ:
-		*param = &params->blkadj;
-		CHKARG(configs, config, blkadj)
-		return sizeof(params->blkadj);
-	case OMAP3ISP_PREV_YC_LIMIT:
-		*param = &params->yclimit;
-		CHKARG(configs, config, yclimit)
-		return sizeof(params->yclimit);
-	case OMAP3ISP_PREV_RGB2RGB:
-		*param = &params->rgb2rgb;
-		CHKARG(configs, config, rgb2rgb)
-		return sizeof(params->rgb2rgb);
-	case OMAP3ISP_PREV_COLOR_CONV:
-		*param = &params->csc;
-		CHKARG(configs, config, csc)
-		return sizeof(params->csc);
-	case OMAP3ISP_PREV_WB:
-		*param = &params->wbal;
-		CHKARG(configs, config, wbal)
-		return sizeof(params->wbal);
-	case OMAP3ISP_PREV_GAMMA:
-		*param = &params->gamma;
-		CHKARG(configs, config, gamma)
-		return sizeof(params->gamma);
-	case OMAP3ISP_PREV_CONTRAST:
-		*param = &params->contrast;
-		return 0;
-	case OMAP3ISP_PREV_BRIGHTNESS:
-		*param = &params->brightness;
-		return 0;
-	default:
+	if (index >= ARRAY_SIZE(update_attrs)) {
+		if (config)
+			*config = NULL;
 		*param = NULL;
-		*config = NULL;
-		break;
+		return 0;
 	}
-	return 0;
+
+	if (configs && config)
+		*config = *(void **)((void *)configs + info->config_offset);
+
+	*param = (void *)params + info->param_offset;
+	return info->param_size;
 }
 
 /*
@@ -886,8 +876,8 @@ __preview_get_ptrs(struct prev_params *params, void **param,
 static int preview_config(struct isp_prev_device *prev,
 			  struct omap3isp_prev_update_config *cfg)
 {
+	const struct preview_update *attr;
 	struct prev_params *params;
-	struct preview_update *attr;
 	int i, bit, rval = 0;
 
 	params = &prev->params;
@@ -913,7 +903,7 @@ static int preview_config(struct isp_prev_device *prev,
 			void *to = NULL, __user *from = NULL;
 			unsigned long sz = 0;
 
-			sz = __preview_get_ptrs(params, &to, cfg, &from, bit);
+			sz = __preview_get_ptrs(params, &to, cfg, &from, i);
 			if (to && from && sz) {
 				if (copy_from_user(to, from, sz)) {
 					rval = -EFAULT;
@@ -941,7 +931,7 @@ static int preview_config(struct isp_prev_device *prev,
 static void preview_setup_hw(struct isp_prev_device *prev)
 {
 	struct prev_params *params = &prev->params;
-	struct preview_update *attr;
+	const struct preview_update *attr;
 	unsigned int bit;
 	unsigned int i;
 	void *param_ptr;
@@ -959,7 +949,7 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 		if (params->features & bit) {
 			if (attr->config) {
 				__preview_get_ptrs(params, &param_ptr, NULL,
-						      NULL, bit);
+						   NULL, i);
 				attr->config(prev, param_ptr);
 			}
 			if (attr->enable)
-- 
1.7.3.4

Re: [PATCH v3 8/9] omap3isp: preview: Simplify configuration parameters access

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Mon, Apr 16, 2012 at 03:29:53PM +0200, Laurent Pinchart wrote:
> Instead of using a large switch/case statement to return offsets to and
> sizes of individual preview engine parameters in the parameters and
> configuration structures, store the information in the update_attrs
> table and use it at runtime.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

[PATCH v3 9/9] omap3isp: preview: Shorten shadow update delay

[Laurent Pinchart]

When applications modify preview engine parameters, the new values are
applied to the hardware by the preview engine interrupt handler during
vertical blanking. If the parameters are being changed when the
interrupt handler is called, it just delays applying the parameters
until the next frame.

If an application modifies the parameters for every frame, and the
preview engine interrupt is triggerred synchronously, the parameters are
never applied to the hardware.

Fix this by storing new parameters in a shadow copy, and switch the
active parameters with the shadow values atomically.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isppreview.c |  298 +++++++++++++++++++----------
 drivers/media/video/omap3isp/isppreview.h |   21 ++-
 2 files changed, 212 insertions(+), 107 deletions(-)

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index e12df2c..5ccfe46 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -649,12 +649,18 @@ preview_config_rgb_to_ycbcr(struct isp_prev_device *prev, const void *prev_csc)
 static void
 preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
 {
-	struct prev_params *params = &prev->params;
+	struct prev_params *params;
+	unsigned long flags;
+
+	spin_lock_irqsave(&prev->params.lock, flags);
+	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
+	       ? &prev->params.params[0] : &prev->params.params[1];
 
 	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
 		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
-		prev->update |= OMAP3ISP_PREV_CONTRAST;
+		params->update |= OMAP3ISP_PREV_CONTRAST;
 	}
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /*
@@ -681,12 +687,18 @@ preview_config_contrast(struct isp_prev_device *prev, const void *params)
 static void
 preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
 {
-	struct prev_params *params = &prev->params;
+	struct prev_params *params;
+	unsigned long flags;
+
+	spin_lock_irqsave(&prev->params.lock, flags);
+	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
+	       ? &prev->params.params[0] : &prev->params.params[1];
 
 	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
 		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
-		prev->update |= OMAP3ISP_PREV_BRIGHTNESS;
+		params->update |= OMAP3ISP_PREV_BRIGHTNESS;
 	}
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /*
@@ -721,6 +733,75 @@ preview_config_yc_range(struct isp_prev_device *prev, const void *yclimit)
 		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SETUP_YC);
 }
 
+static u32
+preview_params_lock(struct isp_prev_device *prev, u32 update, bool shadow)
+{
+	u32 active = prev->params.active;
+
+	if (shadow) {
+		/* Mark all shadow parameters we are going to touch as busy. */
+		prev->params.params[0].busy |= ~active & update;
+		prev->params.params[1].busy |= active & update;
+	} else {
+		/* Mark all active parameters we are going to touch as busy. */
+		update = (prev->params.params[0].update & active)
+		       | (prev->params.params[1].update & ~active);
+
+		prev->params.params[0].busy |= active & update;
+		prev->params.params[1].busy |= ~active & update;
+	}
+
+	return update;
+}
+
+static void
+preview_params_unlock(struct isp_prev_device *prev, u32 update, bool shadow)
+{
+	u32 active = prev->params.active;
+
+	if (shadow) {
+		/* Set the update flag for shadow parameters that have been
+		 * updated and clear the busy flag for all shadow parameters.
+		 */
+		prev->params.params[0].update |= (~active & update);
+		prev->params.params[1].update |= (active & update);
+		prev->params.params[0].busy &= active;
+		prev->params.params[1].busy &= ~active;
+	} else {
+		/* Clear the update flag for active parameters that have been
+		 * applied and the busy flag for all active parameters.
+		 */
+		prev->params.params[0].update &= ~(active & update);
+		prev->params.params[1].update &= ~(~active & update);
+		prev->params.params[0].busy &= ~active;
+		prev->params.params[1].busy &= active;
+	}
+}
+
+static void preview_params_switch(struct isp_prev_device *prev)
+{
+	u32 to_switch;
+
+	/* Switch active parameters with updated shadow parameters when the
+	 * shadow parameter has been updated and neither the active not the
+	 * shadow parameter is busy.
+	 */
+	to_switch = (prev->params.params[0].update & ~prev->params.active)
+		  | (prev->params.params[1].update & prev->params.active);
+	to_switch &= ~(prev->params.params[0].busy |
+		       prev->params.params[1].busy);
+	if (to_switch == 0)
+		return;
+
+	prev->params.active ^= to_switch;
+
+	/* Remove the update flag for the shadow copy of parameters we have
+	 * switched.
+	 */
+	prev->params.params[0].update &= ~(~prev->params.active & to_switch);
+	prev->params.params[1].update &= ~(prev->params.active & to_switch);
+}
+
 /* preview parameters update structure */
 struct preview_update {
 	void (*config)(struct isp_prev_device *, const void *);
@@ -834,37 +915,6 @@ static const struct preview_update update_attrs[] = {
 };
 
 /*
- * __preview_get_ptrs - helper function which return pointers to members
- *                         of params and config structures.
- * @params - pointer to preview_params structure.
- * @param - return pointer to appropriate structure field.
- * @configs - pointer to update config structure.
- * @config - return pointer to appropriate structure field.
- * @bit - for which feature to return pointers.
- * Return size of corresponding prev_params member
- */
-static u32
-__preview_get_ptrs(struct prev_params *params, void **param,
-		   struct omap3isp_prev_update_config *configs,
-		   void __user **config, unsigned int index)
-{
-	const struct preview_update *info = &update_attrs[index];
-
-	if (index >= ARRAY_SIZE(update_attrs)) {
-		if (config)
-			*config = NULL;
-		*param = NULL;
-		return 0;
-	}
-
-	if (configs && config)
-		*config = *(void **)((void *)configs + info->config_offset);
-
-	*param = (void *)params + info->param_offset;
-	return info->param_size;
-}
-
-/*
  * preview_config - Copy and update local structure with userspace preview
  *                  configuration.
  * @prev: ISP preview engine
@@ -876,36 +926,41 @@ __preview_get_ptrs(struct prev_params *params, void **param,
 static int preview_config(struct isp_prev_device *prev,
 			  struct omap3isp_prev_update_config *cfg)
 {
-	const struct preview_update *attr;
-	struct prev_params *params;
-	int i, bit, rval = 0;
+	unsigned long flags;
+	unsigned int i;
+	int rval = 0;
+	u32 update;
+	u32 active;
 
-	params = &prev->params;
 	if (cfg->update == 0)
 		return 0;
 
-	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
-		unsigned long flags;
+	/* Mark the shadow parameters we're going to update as busy. */
+	spin_lock_irqsave(&prev->params.lock, flags);
+	preview_params_lock(prev, cfg->update, true);
+	active = prev->params.active;
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 
-		spin_lock_irqsave(&prev->lock, flags);
-		prev->shadow_update = 1;
-		spin_unlock_irqrestore(&prev->lock, flags);
-	}
+	update = 0;
 
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
-		attr = &update_attrs[i];
-		bit = 1 << i;
+		const struct preview_update *attr = &update_attrs[i];
+		struct prev_params *params;
+		unsigned int bit = 1 << i;
 
 		if (attr->skip || !(cfg->update & bit))
 			continue;
 
+		params = &prev->params.params[!!(active & bit)];
+
 		if (cfg->flag & bit) {
-			void *to = NULL, __user *from = NULL;
-			unsigned long sz = 0;
+			void __user *from = *(void * __user *)
+				((void *)cfg + attr->config_offset);
+			void *to = (void *)params + attr->param_offset;
+			size_t size = attr->param_size;
 
-			sz = __preview_get_ptrs(params, &to, cfg, &from, i);
-			if (to && from && sz) {
-				if (copy_from_user(to, from, sz)) {
+			if (to && from && size) {
+				if (copy_from_user(to, from, size)) {
 					rval = -EFAULT;
 					break;
 				}
@@ -915,50 +970,59 @@ static int preview_config(struct isp_prev_device *prev,
 			params->features &= ~bit;
 		}
 
-		prev->update |= bit;
+		update |= bit;
 	}
 
-	prev->shadow_update = 0;
+	spin_lock_irqsave(&prev->params.lock, flags);
+	preview_params_unlock(prev, update, true);
+	preview_params_switch(prev);
+	spin_unlock_irqrestore(&prev->params.lock, flags);
+
 	return rval;
 }
 
 /*
  * preview_setup_hw - Setup preview registers and/or internal memory
  * @prev: pointer to preview private structure
+ * @update: Bitmask of parameters to setup
+ * @active: Bitmask of parameters active in set 0
  * Note: can be called from interrupt context
  * Return none
  */
-static void preview_setup_hw(struct isp_prev_device *prev)
+static void preview_setup_hw(struct isp_prev_device *prev, u32 update,
+			     u32 active)
 {
-	struct prev_params *params = &prev->params;
-	const struct preview_update *attr;
-	unsigned int bit;
 	unsigned int i;
-	void *param_ptr;
+	u32 features;
 
-	if (prev->update == 0)
+	if (update == 0)
 		return;
 
+	features = (prev->params.params[0].features & active)
+		 | (prev->params.params[1].features & ~active);
+
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
-		attr = &update_attrs[i];
-		bit = 1 << i;
+		const struct preview_update *attr = &update_attrs[i];
+		struct prev_params *params;
+		unsigned int bit = 1 << i;
+		void *param_ptr;
 
-		if (!(prev->update & bit))
+		if (!(update & bit))
 			continue;
 
+		params = &prev->params.params[!(active & bit)];
+
 		if (params->features & bit) {
 			if (attr->config) {
-				__preview_get_ptrs(params, &param_ptr, NULL,
-						   NULL, i);
+				param_ptr = (void *)params + attr->param_offset;
 				attr->config(prev, param_ptr);
 			}
 			if (attr->enable)
 				attr->enable(prev, 1);
-		} else
+		} else {
 			if (attr->enable)
 				attr->enable(prev, 0);
-
-		prev->update &= ~bit;
+		}
 	}
 }
 
@@ -996,13 +1060,17 @@ preview_config_ycpos(struct isp_prev_device *prev,
 static void preview_config_averager(struct isp_prev_device *prev, u8 average)
 {
 	struct isp_device *isp = to_isp_device(prev);
+	struct prev_params *params;
 	int reg = 0;
 
-	if (prev->params.cfa.format == OMAP3ISP_CFAFMT_BAYER)
+	params = (prev->params.active & OMAP3ISP_PREV_CFA)
+	       ? &prev->params.params[0] : &prev->params.params[1];
+
+	if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
 		reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
 		      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
 		      average;
-	else if (prev->params.cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
+	else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
 		reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
 		      ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
 		      average;
@@ -1020,33 +1088,35 @@ static void preview_config_averager(struct isp_prev_device *prev, u8 average)
  *
  * See the explanation at the PREV_MARGIN_* definitions for more details.
  */
-static void preview_config_input_size(struct isp_prev_device *prev)
+static void preview_config_input_size(struct isp_prev_device *prev, u32 active)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	struct prev_params *params = &prev->params;
 	unsigned int sph = prev->crop.left;
 	unsigned int eph = prev->crop.left + prev->crop.width - 1;
 	unsigned int slv = prev->crop.top;
 	unsigned int elv = prev->crop.top + prev->crop.height - 1;
+	u32 features;
 
-	if (params->features & OMAP3ISP_PREV_CFA) {
+	features = (prev->params.params[0].features & active)
+		 | (prev->params.params[1].features & ~active);
+
+	if (features & OMAP3ISP_PREV_CFA) {
 		sph -= 2;
 		eph += 2;
 		slv -= 2;
 		elv += 2;
 	}
-	if (params->features & (OMAP3ISP_PREV_DEFECT_COR | OMAP3ISP_PREV_NF)) {
+	if (features & (OMAP3ISP_PREV_DEFECT_COR | OMAP3ISP_PREV_NF)) {
 		sph -= 2;
 		eph += 2;
 		slv -= 2;
 		elv += 2;
 	}
-	if (params->features & OMAP3ISP_PREV_HRZ_MED) {
+	if (features & OMAP3ISP_PREV_HRZ_MED) {
 		sph -= 2;
 		eph += 2;
 	}
-	if (params->features & (OMAP3ISP_PREV_CHROMA_SUPP |
-				OMAP3ISP_PREV_LUMAENH))
+	if (features & (OMAP3ISP_PREV_CHROMA_SUPP | OMAP3ISP_PREV_LUMAENH))
 		sph -= 2;
 
 	isp_reg_writel(isp, (sph << ISPPRV_HORZ_INFO_SPH_SHIFT) | eph,
@@ -1181,8 +1251,16 @@ int omap3isp_preview_busy(struct isp_prev_device *prev)
  */
 void omap3isp_preview_restore_context(struct isp_device *isp)
 {
-	isp->isp_prev.update = OMAP3ISP_PREV_FEATURES_END - 1;
-	preview_setup_hw(&isp->isp_prev);
+	struct isp_prev_device *prev = &isp->isp_prev;
+	const u32 update = OMAP3ISP_PREV_FEATURES_END - 1;
+
+	prev->params.params[0].update = prev->params.active & update;
+	prev->params.params[1].update = ~prev->params.active & update;
+
+	preview_setup_hw(prev, update, prev->params.active);
+
+	prev->params.params[0].update = 0;
+	prev->params.params[1].update = 0;
 }
 
 /*
@@ -1241,12 +1319,21 @@ static void preview_print_status(struct isp_prev_device *prev)
 /*
  * preview_init_params - init image processing parameters.
  * @prev: pointer to previewer private structure
- * return none
  */
 static void preview_init_params(struct isp_prev_device *prev)
 {
-	struct prev_params *params = &prev->params;
-	int i = 0;
+	struct prev_params *params;
+	unsigned int i;
+
+	spin_lock_init(&prev->params.lock);
+
+	prev->params.active = ~0;
+	prev->params.params[0].busy = 0;
+	prev->params.params[0].update = OMAP3ISP_PREV_FEATURES_END - 1;
+	prev->params.params[1].busy = 0;
+	prev->params.params[1].update = 0;
+
+	params = &prev->params.params[0];
 
 	/* Init values */
 	params->contrast = ISPPRV_CONTRAST_DEF * ISPPRV_CONTRAST_UNITS;
@@ -1290,8 +1377,6 @@ static void preview_init_params(struct isp_prev_device *prev)
 			 | OMAP3ISP_PREV_RGB2RGB | OMAP3ISP_PREV_COLOR_CONV
 			 | OMAP3ISP_PREV_WB | OMAP3ISP_PREV_BRIGHTNESS
 			 | OMAP3ISP_PREV_CONTRAST;
-
-	prev->update = OMAP3ISP_PREV_FEATURES_END - 1;
 }
 
 /*
@@ -1320,8 +1405,17 @@ static void preview_configure(struct isp_prev_device *prev)
 {
 	struct isp_device *isp = to_isp_device(prev);
 	struct v4l2_mbus_framefmt *format;
+	unsigned long flags;
+	u32 update;
+	u32 active;
 
-	preview_setup_hw(prev);
+	spin_lock_irqsave(&prev->params.lock, flags);
+	/* Mark all active parameters we are going to touch as busy. */
+	update = preview_params_lock(prev, 0, false);
+	active = prev->params.active;
+	spin_unlock_irqrestore(&prev->params.lock, flags);
+
+	preview_setup_hw(prev, update, active);
 
 	if (prev->output & PREVIEW_OUTPUT_MEMORY)
 		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
@@ -1342,7 +1436,7 @@ static void preview_configure(struct isp_prev_device *prev)
 
 	preview_adjust_bandwidth(prev);
 
-	preview_config_input_size(prev);
+	preview_config_input_size(prev, active);
 
 	if (prev->input == PREVIEW_INPUT_CCDC)
 		preview_config_inlineoffset(prev, 0);
@@ -1359,6 +1453,10 @@ static void preview_configure(struct isp_prev_device *prev)
 
 	preview_config_averager(prev, 0);
 	preview_config_ycpos(prev, format->code);
+
+	spin_lock_irqsave(&prev->params.lock, flags);
+	preview_params_unlock(prev, update, false);
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /* -----------------------------------------------------------------------------
@@ -1447,25 +1545,30 @@ static void preview_isr_buffer(struct isp_prev_device *prev)
 void omap3isp_preview_isr(struct isp_prev_device *prev)
 {
 	unsigned long flags;
+	u32 update;
+	u32 active;
 
 	if (omap3isp_module_sync_is_stopping(&prev->wait, &prev->stopping))
 		return;
 
-	spin_lock_irqsave(&prev->lock, flags);
-	if (prev->shadow_update)
-		goto done;
+	spin_lock_irqsave(&prev->params.lock, flags);
+	preview_params_switch(prev);
+	update = preview_params_lock(prev, 0, false);
+	active = prev->params.active;
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 
-	preview_setup_hw(prev);
-	preview_config_input_size(prev);
-
-done:
-	spin_unlock_irqrestore(&prev->lock, flags);
+	preview_setup_hw(prev, update, active);
+	preview_config_input_size(prev, active);
 
 	if (prev->input == PREVIEW_INPUT_MEMORY ||
 	    prev->output & PREVIEW_OUTPUT_MEMORY)
 		preview_isr_buffer(prev);
 	else if (prev->state == ISP_PIPELINE_STREAM_CONTINUOUS)
 		preview_enable_oneshot(prev);
+
+	spin_lock_irqsave(&prev->params.lock, flags);
+	preview_params_unlock(prev, update, false);
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /* -----------------------------------------------------------------------------
@@ -1551,7 +1654,6 @@ static int preview_set_stream(struct v4l2_subdev *sd, int enable)
 	struct isp_video *video_out = &prev->video_out;
 	struct isp_device *isp = to_isp_device(prev);
 	struct device *dev = to_device(prev);
-	unsigned long flags;
 
 	if (prev->state == ISP_PIPELINE_STREAM_STOPPED) {
 		if (enable == ISP_PIPELINE_STREAM_STOPPED)
@@ -1588,11 +1690,9 @@ static int preview_set_stream(struct v4l2_subdev *sd, int enable)
 		if (omap3isp_module_sync_idle(&sd->entity, &prev->wait,
 					      &prev->stopping))
 			dev_dbg(dev, "%s: stop timeout.\n", sd->name);
-		spin_lock_irqsave(&prev->lock, flags);
 		omap3isp_sbl_disable(isp, OMAP3_ISP_SBL_PREVIEW_READ);
 		omap3isp_sbl_disable(isp, OMAP3_ISP_SBL_PREVIEW_WRITE);
 		omap3isp_subclk_disable(isp, OMAP3_ISP_SUBCLK_PREVIEW);
-		spin_unlock_irqrestore(&prev->lock, flags);
 		isp_video_dmaqueue_flags_clr(video_out);
 		break;
 	}
@@ -2200,7 +2300,7 @@ error_video_in:
 }
 
 /*
- * isp_preview_init - Previewer initialization.
+ * omap3isp_preview_init - Previewer initialization.
  * @dev : Pointer to ISP device
  * return -ENOMEM or zero on success
  */
@@ -2208,8 +2308,8 @@ int omap3isp_preview_init(struct isp_device *isp)
 {
 	struct isp_prev_device *prev = &isp->isp_prev;
 
-	spin_lock_init(&prev->lock);
 	init_waitqueue_head(&prev->wait);
+
 	preview_init_params(prev);
 
 	return preview_init_entities(prev);
diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index 6ee8306..6663ab6 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -69,6 +69,8 @@ enum preview_ycpos_mode {
 
 /*
  * struct prev_params - Structure for all configuration
+ * @busy: Bitmask of busy parameters (being updated or used)
+ * @update: Bitmask of the parameters to be updated
  * @features: Set of features enabled.
  * @cfa: CFA coefficients.
  * @csup: Chroma suppression coefficients.
@@ -86,6 +88,8 @@ enum preview_ycpos_mode {
  * @brightness: Brightness.
  */
 struct prev_params {
+	u32 busy;
+	u32 update;
 	u32 features;
 	struct omap3isp_prev_cfa cfa;
 	struct omap3isp_prev_csup csup;
@@ -118,12 +122,11 @@ struct prev_params {
  * @output: Bitmask of the active output
  * @video_in: Input video entity
  * @video_out: Output video entity
- * @params: Module configuration data
- * @shadow_update: If set, update the hardware configured in the next interrupt
+ * @params.params : Active and shadow parameters sets
+ * @params.active: Bitmask of parameters active in set 0
+ * @params.lock: Parameters lock, protects params.active and params.shadow
  * @underrun: Whether the preview entity has queued buffers on the output
  * @state: Current preview pipeline state
- * @lock: Shadow update lock
- * @update: Bitmask of the parameters to be updated
  *
  * This structure is used to store the OMAP ISP Preview module Information.
  */
@@ -140,13 +143,15 @@ struct isp_prev_device {
 	struct isp_video video_in;
 	struct isp_video video_out;
 
-	struct prev_params params;
-	unsigned int shadow_update:1;
+	struct {
+		struct prev_params params[2];
+		u32 active;
+		spinlock_t lock;
+	} params;
+
 	enum isp_pipeline_stream_state state;
 	wait_queue_head_t wait;
 	atomic_t stopping;
-	spinlock_t lock;
-	u32 update;
 };
 
 struct isp_device;
-- 
1.7.3.4

Re: [PATCH v3 9/9] omap3isp: preview: Shorten shadow update delay

[Sakari Ailus]

Hi Laurent,

Many thanks for the patch!!

On Mon, Apr 16, 2012 at 03:29:54PM +0200, Laurent Pinchart wrote:
> When applications modify preview engine parameters, the new values are
> applied to the hardware by the preview engine interrupt handler during
> vertical blanking. If the parameters are being changed when the
> interrupt handler is called, it just delays applying the parameters
> until the next frame.
> 
> If an application modifies the parameters for every frame, and the
> preview engine interrupt is triggerred synchronously, the parameters are
> never applied to the hardware.
> 
> Fix this by storing new parameters in a shadow copy, and switch the
> active parameters with the shadow values atomically.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isppreview.c |  298 +++++++++++++++++++----------
>  drivers/media/video/omap3isp/isppreview.h |   21 ++-
>  2 files changed, 212 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index e12df2c..5ccfe46 100644
> --- a/drivers/media/video/omap3isp/isppreview.c
> +++ b/drivers/media/video/omap3isp/isppreview.c
> @@ -649,12 +649,18 @@ preview_config_rgb_to_ycbcr(struct isp_prev_device *prev, const void *prev_csc)

Not related to this patch, but shouldn't the above function be called
preview_config_csc()?

>  static void
>  preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
>  {
> -	struct prev_params *params = &prev->params;
> +	struct prev_params *params;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
> +	       ? &prev->params.params[0] : &prev->params.params[1];
>  
>  	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
>  		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
> -		prev->update |= OMAP3ISP_PREV_CONTRAST;
> +		params->update |= OMAP3ISP_PREV_CONTRAST;
>  	}
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  }
>  
>  /*
> @@ -681,12 +687,18 @@ preview_config_contrast(struct isp_prev_device *prev, const void *params)
>  static void
>  preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
>  {
> -	struct prev_params *params = &prev->params;
> +	struct prev_params *params;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
> +	       ? &prev->params.params[0] : &prev->params.params[1];

params = prev->params.params[!(prev->params.active & OMAP3ISP_PREV_CONTRAST)];

Same in contrast.

And shouldn't this be brightness here btw.?

?

>  	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
>  		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
> -		prev->update |= OMAP3ISP_PREV_BRIGHTNESS;
> +		params->update |= OMAP3ISP_PREV_BRIGHTNESS;
>  	}
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  }
>  
>  /*
> @@ -721,6 +733,75 @@ preview_config_yc_range(struct isp_prev_device *prev, const void *yclimit)
>  		       OMAP3_ISP_IOMEM_PREV, ISPPRV_SETUP_YC);
>  }
>  
> +static u32
> +preview_params_lock(struct isp_prev_device *prev, u32 update, bool shadow)
> +{
> +	u32 active = prev->params.active;
> +
> +	if (shadow) {
> +		/* Mark all shadow parameters we are going to touch as busy. */
> +		prev->params.params[0].busy |= ~active & update;
> +		prev->params.params[1].busy |= active & update;
> +	} else {
> +		/* Mark all active parameters we are going to touch as busy. */
> +		update = (prev->params.params[0].update & active)
> +		       | (prev->params.params[1].update & ~active);
> +
> +		prev->params.params[0].busy |= active & update;
> +		prev->params.params[1].busy |= ~active & update;
> +	}
> +
> +	return update;
> +}
> +
> +static void
> +preview_params_unlock(struct isp_prev_device *prev, u32 update, bool shadow)
> +{
> +	u32 active = prev->params.active;
> +
> +	if (shadow) {
> +		/* Set the update flag for shadow parameters that have been
> +		 * updated and clear the busy flag for all shadow parameters.
> +		 */
> +		prev->params.params[0].update |= (~active & update);
> +		prev->params.params[1].update |= (active & update);
> +		prev->params.params[0].busy &= active;
> +		prev->params.params[1].busy &= ~active;
> +	} else {
> +		/* Clear the update flag for active parameters that have been
> +		 * applied and the busy flag for all active parameters.
> +		 */
> +		prev->params.params[0].update &= ~(active & update);
> +		prev->params.params[1].update &= ~(~active & update);
> +		prev->params.params[0].busy &= ~active;
> +		prev->params.params[1].busy &= active;
> +	}
> +}
> +
> +static void preview_params_switch(struct isp_prev_device *prev)
> +{
> +	u32 to_switch;
> +
> +	/* Switch active parameters with updated shadow parameters when the
> +	 * shadow parameter has been updated and neither the active not the
> +	 * shadow parameter is busy.
> +	 */
> +	to_switch = (prev->params.params[0].update & ~prev->params.active)
> +		  | (prev->params.params[1].update & prev->params.active);
> +	to_switch &= ~(prev->params.params[0].busy |
> +		       prev->params.params[1].busy);
> +	if (to_switch == 0)
> +		return;
> +
> +	prev->params.active ^= to_switch;
> +
> +	/* Remove the update flag for the shadow copy of parameters we have
> +	 * switched.
> +	 */
> +	prev->params.params[0].update &= ~(~prev->params.active & to_switch);
> +	prev->params.params[1].update &= ~(prev->params.active & to_switch);
> +}
> +
>  /* preview parameters update structure */
>  struct preview_update {
>  	void (*config)(struct isp_prev_device *, const void *);
> @@ -834,37 +915,6 @@ static const struct preview_update update_attrs[] = {
>  };
>  
>  /*
> - * __preview_get_ptrs - helper function which return pointers to members
> - *                         of params and config structures.
> - * @params - pointer to preview_params structure.
> - * @param - return pointer to appropriate structure field.
> - * @configs - pointer to update config structure.
> - * @config - return pointer to appropriate structure field.
> - * @bit - for which feature to return pointers.
> - * Return size of corresponding prev_params member
> - */
> -static u32
> -__preview_get_ptrs(struct prev_params *params, void **param,
> -		   struct omap3isp_prev_update_config *configs,
> -		   void __user **config, unsigned int index)
> -{
> -	const struct preview_update *info = &update_attrs[index];
> -
> -	if (index >= ARRAY_SIZE(update_attrs)) {
> -		if (config)
> -			*config = NULL;
> -		*param = NULL;
> -		return 0;
> -	}
> -
> -	if (configs && config)
> -		*config = *(void **)((void *)configs + info->config_offset);
> -
> -	*param = (void *)params + info->param_offset;
> -	return info->param_size;
> -}
> -
> -/*
>   * preview_config - Copy and update local structure with userspace preview
>   *                  configuration.
>   * @prev: ISP preview engine
> @@ -876,36 +926,41 @@ __preview_get_ptrs(struct prev_params *params, void **param,
>  static int preview_config(struct isp_prev_device *prev,
>  			  struct omap3isp_prev_update_config *cfg)
>  {
> -	const struct preview_update *attr;
> -	struct prev_params *params;
> -	int i, bit, rval = 0;
> +	unsigned long flags;
> +	unsigned int i;
> +	int rval = 0;
> +	u32 update;
> +	u32 active;
>  
> -	params = &prev->params;
>  	if (cfg->update == 0)
>  		return 0;
>  
> -	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
> -		unsigned long flags;
> +	/* Mark the shadow parameters we're going to update as busy. */
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	preview_params_lock(prev, cfg->update, true);
> +	active = prev->params.active;
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  
> -		spin_lock_irqsave(&prev->lock, flags);
> -		prev->shadow_update = 1;
> -		spin_unlock_irqrestore(&prev->lock, flags);
> -	}
> +	update = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
> -		attr = &update_attrs[i];
> -		bit = 1 << i;
> +		const struct preview_update *attr = &update_attrs[i];
> +		struct prev_params *params;
> +		unsigned int bit = 1 << i;
>  
>  		if (attr->skip || !(cfg->update & bit))
>  			continue;
>  
> +		params = &prev->params.params[!!(active & bit)];
> +
>  		if (cfg->flag & bit) {
> -			void *to = NULL, __user *from = NULL;
> -			unsigned long sz = 0;
> +			void __user *from = *(void * __user *)
> +				((void *)cfg + attr->config_offset);
> +			void *to = (void *)params + attr->param_offset;
> +			size_t size = attr->param_size;
>  
> -			sz = __preview_get_ptrs(params, &to, cfg, &from, i);
> -			if (to && from && sz) {
> -				if (copy_from_user(to, from, sz)) {
> +			if (to && from && size) {
> +				if (copy_from_user(to, from, size)) {
>  					rval = -EFAULT;
>  					break;
>  				}
> @@ -915,50 +970,59 @@ static int preview_config(struct isp_prev_device *prev,
>  			params->features &= ~bit;
>  		}
>  
> -		prev->update |= bit;
> +		update |= bit;
>  	}
>  
> -	prev->shadow_update = 0;
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	preview_params_unlock(prev, update, true);
> +	preview_params_switch(prev);
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
> +
>  	return rval;
>  }
>  
>  /*
>   * preview_setup_hw - Setup preview registers and/or internal memory
>   * @prev: pointer to preview private structure
> + * @update: Bitmask of parameters to setup
> + * @active: Bitmask of parameters active in set 0
>   * Note: can be called from interrupt context
>   * Return none
>   */
> -static void preview_setup_hw(struct isp_prev_device *prev)
> +static void preview_setup_hw(struct isp_prev_device *prev, u32 update,
> +			     u32 active)
>  {
> -	struct prev_params *params = &prev->params;
> -	const struct preview_update *attr;
> -	unsigned int bit;
>  	unsigned int i;
> -	void *param_ptr;
> +	u32 features;
>  
> -	if (prev->update == 0)
> +	if (update == 0)
>  		return;
>  
> +	features = (prev->params.params[0].features & active)
> +		 | (prev->params.params[1].features & ~active);
> +
>  	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
> -		attr = &update_attrs[i];
> -		bit = 1 << i;
> +		const struct preview_update *attr = &update_attrs[i];
> +		struct prev_params *params;
> +		unsigned int bit = 1 << i;
> +		void *param_ptr;
>  
> -		if (!(prev->update & bit))
> +		if (!(update & bit))
>  			continue;
>  
> +		params = &prev->params.params[!(active & bit)];
> +
>  		if (params->features & bit) {
>  			if (attr->config) {
> -				__preview_get_ptrs(params, &param_ptr, NULL,
> -						   NULL, i);
> +				param_ptr = (void *)params + attr->param_offset;
>  				attr->config(prev, param_ptr);
>  			}
>  			if (attr->enable)
>  				attr->enable(prev, 1);
> -		} else
> +		} else {
>  			if (attr->enable)
>  				attr->enable(prev, 0);
> -
> -		prev->update &= ~bit;
> +		}
>  	}
>  }
>  
> @@ -996,13 +1060,17 @@ preview_config_ycpos(struct isp_prev_device *prev,
>  static void preview_config_averager(struct isp_prev_device *prev, u8 average)
>  {
>  	struct isp_device *isp = to_isp_device(prev);
> +	struct prev_params *params;
>  	int reg = 0;
>  
> -	if (prev->params.cfa.format == OMAP3ISP_CFAFMT_BAYER)
> +	params = (prev->params.active & OMAP3ISP_PREV_CFA)
> +	       ? &prev->params.params[0] : &prev->params.params[1];



> +	if (params->cfa.format == OMAP3ISP_CFAFMT_BAYER)
>  		reg = ISPPRV_AVE_EVENDIST_2 << ISPPRV_AVE_EVENDIST_SHIFT |
>  		      ISPPRV_AVE_ODDDIST_2 << ISPPRV_AVE_ODDDIST_SHIFT |
>  		      average;
> -	else if (prev->params.cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
> +	else if (params->cfa.format == OMAP3ISP_CFAFMT_RGBFOVEON)
>  		reg = ISPPRV_AVE_EVENDIST_3 << ISPPRV_AVE_EVENDIST_SHIFT |
>  		      ISPPRV_AVE_ODDDIST_3 << ISPPRV_AVE_ODDDIST_SHIFT |
>  		      average;
> @@ -1020,33 +1088,35 @@ static void preview_config_averager(struct isp_prev_device *prev, u8 average)
>   *
>   * See the explanation at the PREV_MARGIN_* definitions for more details.
>   */
> -static void preview_config_input_size(struct isp_prev_device *prev)
> +static void preview_config_input_size(struct isp_prev_device *prev, u32 active)
>  {
>  	struct isp_device *isp = to_isp_device(prev);
> -	struct prev_params *params = &prev->params;
>  	unsigned int sph = prev->crop.left;
>  	unsigned int eph = prev->crop.left + prev->crop.width - 1;
>  	unsigned int slv = prev->crop.top;
>  	unsigned int elv = prev->crop.top + prev->crop.height - 1;
> +	u32 features;
>  
> -	if (params->features & OMAP3ISP_PREV_CFA) {
> +	features = (prev->params.params[0].features & active)
> +		 | (prev->params.params[1].features & ~active);
> +
> +	if (features & OMAP3ISP_PREV_CFA) {
>  		sph -= 2;
>  		eph += 2;
>  		slv -= 2;
>  		elv += 2;
>  	}
> -	if (params->features & (OMAP3ISP_PREV_DEFECT_COR | OMAP3ISP_PREV_NF)) {
> +	if (features & (OMAP3ISP_PREV_DEFECT_COR | OMAP3ISP_PREV_NF)) {
>  		sph -= 2;
>  		eph += 2;
>  		slv -= 2;
>  		elv += 2;
>  	}
> -	if (params->features & OMAP3ISP_PREV_HRZ_MED) {
> +	if (features & OMAP3ISP_PREV_HRZ_MED) {
>  		sph -= 2;
>  		eph += 2;
>  	}
> -	if (params->features & (OMAP3ISP_PREV_CHROMA_SUPP |
> -				OMAP3ISP_PREV_LUMAENH))
> +	if (features & (OMAP3ISP_PREV_CHROMA_SUPP | OMAP3ISP_PREV_LUMAENH))
>  		sph -= 2;
>  
>  	isp_reg_writel(isp, (sph << ISPPRV_HORZ_INFO_SPH_SHIFT) | eph,
> @@ -1181,8 +1251,16 @@ int omap3isp_preview_busy(struct isp_prev_device *prev)
>   */
>  void omap3isp_preview_restore_context(struct isp_device *isp)
>  {
> -	isp->isp_prev.update = OMAP3ISP_PREV_FEATURES_END - 1;
> -	preview_setup_hw(&isp->isp_prev);
> +	struct isp_prev_device *prev = &isp->isp_prev;
> +	const u32 update = OMAP3ISP_PREV_FEATURES_END - 1;
> +
> +	prev->params.params[0].update = prev->params.active & update;
> +	prev->params.params[1].update = ~prev->params.active & update;
> +
> +	preview_setup_hw(prev, update, prev->params.active);
> +
> +	prev->params.params[0].update = 0;
> +	prev->params.params[1].update = 0;
>  }
>  
>  /*
> @@ -1241,12 +1319,21 @@ static void preview_print_status(struct isp_prev_device *prev)
>  /*
>   * preview_init_params - init image processing parameters.
>   * @prev: pointer to previewer private structure
> - * return none
>   */
>  static void preview_init_params(struct isp_prev_device *prev)
>  {
> -	struct prev_params *params = &prev->params;
> -	int i = 0;
> +	struct prev_params *params;
> +	unsigned int i;
> +
> +	spin_lock_init(&prev->params.lock);
> +
> +	prev->params.active = ~0;
> +	prev->params.params[0].busy = 0;
> +	prev->params.params[0].update = OMAP3ISP_PREV_FEATURES_END - 1;
> +	prev->params.params[1].busy = 0;
> +	prev->params.params[1].update = 0;
> +
> +	params = &prev->params.params[0];
>  
>  	/* Init values */
>  	params->contrast = ISPPRV_CONTRAST_DEF * ISPPRV_CONTRAST_UNITS;
> @@ -1290,8 +1377,6 @@ static void preview_init_params(struct isp_prev_device *prev)
>  			 | OMAP3ISP_PREV_RGB2RGB | OMAP3ISP_PREV_COLOR_CONV
>  			 | OMAP3ISP_PREV_WB | OMAP3ISP_PREV_BRIGHTNESS
>  			 | OMAP3ISP_PREV_CONTRAST;
> -
> -	prev->update = OMAP3ISP_PREV_FEATURES_END - 1;
>  }
>  
>  /*
> @@ -1320,8 +1405,17 @@ static void preview_configure(struct isp_prev_device *prev)
>  {
>  	struct isp_device *isp = to_isp_device(prev);
>  	struct v4l2_mbus_framefmt *format;
> +	unsigned long flags;
> +	u32 update;
> +	u32 active;
>  
> -	preview_setup_hw(prev);
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	/* Mark all active parameters we are going to touch as busy. */
> +	update = preview_params_lock(prev, 0, false);
> +	active = prev->params.active;
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
> +
> +	preview_setup_hw(prev, update, active);
>  
>  	if (prev->output & PREVIEW_OUTPUT_MEMORY)
>  		isp_reg_set(isp, OMAP3_ISP_IOMEM_PREV, ISPPRV_PCR,
> @@ -1342,7 +1436,7 @@ static void preview_configure(struct isp_prev_device *prev)
>  
>  	preview_adjust_bandwidth(prev);
>  
> -	preview_config_input_size(prev);
> +	preview_config_input_size(prev, active);
>  
>  	if (prev->input == PREVIEW_INPUT_CCDC)
>  		preview_config_inlineoffset(prev, 0);
> @@ -1359,6 +1453,10 @@ static void preview_configure(struct isp_prev_device *prev)
>  
>  	preview_config_averager(prev, 0);
>  	preview_config_ycpos(prev, format->code);
> +
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	preview_params_unlock(prev, update, false);
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -1447,25 +1545,30 @@ static void preview_isr_buffer(struct isp_prev_device *prev)
>  void omap3isp_preview_isr(struct isp_prev_device *prev)
>  {
>  	unsigned long flags;
> +	u32 update;
> +	u32 active;
>  
>  	if (omap3isp_module_sync_is_stopping(&prev->wait, &prev->stopping))
>  		return;
>  
> -	spin_lock_irqsave(&prev->lock, flags);
> -	if (prev->shadow_update)
> -		goto done;
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	preview_params_switch(prev);
> +	update = preview_params_lock(prev, 0, false);
> +	active = prev->params.active;
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  
> -	preview_setup_hw(prev);
> -	preview_config_input_size(prev);
> -
> -done:
> -	spin_unlock_irqrestore(&prev->lock, flags);
> +	preview_setup_hw(prev, update, active);
> +	preview_config_input_size(prev, active);
>  
>  	if (prev->input == PREVIEW_INPUT_MEMORY ||
>  	    prev->output & PREVIEW_OUTPUT_MEMORY)
>  		preview_isr_buffer(prev);
>  	else if (prev->state == ISP_PIPELINE_STREAM_CONTINUOUS)
>  		preview_enable_oneshot(prev);
> +
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	preview_params_unlock(prev, update, false);
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -1551,7 +1654,6 @@ static int preview_set_stream(struct v4l2_subdev *sd, int enable)
>  	struct isp_video *video_out = &prev->video_out;
>  	struct isp_device *isp = to_isp_device(prev);
>  	struct device *dev = to_device(prev);
> -	unsigned long flags;
>  
>  	if (prev->state == ISP_PIPELINE_STREAM_STOPPED) {
>  		if (enable == ISP_PIPELINE_STREAM_STOPPED)
> @@ -1588,11 +1690,9 @@ static int preview_set_stream(struct v4l2_subdev *sd, int enable)
>  		if (omap3isp_module_sync_idle(&sd->entity, &prev->wait,
>  					      &prev->stopping))
>  			dev_dbg(dev, "%s: stop timeout.\n", sd->name);
> -		spin_lock_irqsave(&prev->lock, flags);
>  		omap3isp_sbl_disable(isp, OMAP3_ISP_SBL_PREVIEW_READ);
>  		omap3isp_sbl_disable(isp, OMAP3_ISP_SBL_PREVIEW_WRITE);
>  		omap3isp_subclk_disable(isp, OMAP3_ISP_SUBCLK_PREVIEW);
> -		spin_unlock_irqrestore(&prev->lock, flags);
>  		isp_video_dmaqueue_flags_clr(video_out);
>  		break;
>  	}
> @@ -2200,7 +2300,7 @@ error_video_in:
>  }
>  
>  /*
> - * isp_preview_init - Previewer initialization.
> + * omap3isp_preview_init - Previewer initialization.
>   * @dev : Pointer to ISP device
>   * return -ENOMEM or zero on success
>   */
> @@ -2208,8 +2308,8 @@ int omap3isp_preview_init(struct isp_device *isp)
>  {
>  	struct isp_prev_device *prev = &isp->isp_prev;
>  
> -	spin_lock_init(&prev->lock);
>  	init_waitqueue_head(&prev->wait);
> +
>  	preview_init_params(prev);
>  
>  	return preview_init_entities(prev);
> diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
> index 6ee8306..6663ab6 100644
> --- a/drivers/media/video/omap3isp/isppreview.h
> +++ b/drivers/media/video/omap3isp/isppreview.h
> @@ -69,6 +69,8 @@ enum preview_ycpos_mode {
>  
>  /*
>   * struct prev_params - Structure for all configuration
> + * @busy: Bitmask of busy parameters (being updated or used)
> + * @update: Bitmask of the parameters to be updated
>   * @features: Set of features enabled.
>   * @cfa: CFA coefficients.
>   * @csup: Chroma suppression coefficients.
> @@ -86,6 +88,8 @@ enum preview_ycpos_mode {
>   * @brightness: Brightness.
>   */
>  struct prev_params {
> +	u32 busy;
> +	u32 update;
>  	u32 features;
>  	struct omap3isp_prev_cfa cfa;
>  	struct omap3isp_prev_csup csup;
> @@ -118,12 +122,11 @@ struct prev_params {
>   * @output: Bitmask of the active output
>   * @video_in: Input video entity
>   * @video_out: Output video entity
> - * @params: Module configuration data
> - * @shadow_update: If set, update the hardware configured in the next interrupt
> + * @params.params : Active and shadow parameters sets
> + * @params.active: Bitmask of parameters active in set 0
> + * @params.lock: Parameters lock, protects params.active and params.shadow
>   * @underrun: Whether the preview entity has queued buffers on the output
>   * @state: Current preview pipeline state
> - * @lock: Shadow update lock
> - * @update: Bitmask of the parameters to be updated
>   *
>   * This structure is used to store the OMAP ISP Preview module Information.
>   */
> @@ -140,13 +143,15 @@ struct isp_prev_device {
>  	struct isp_video video_in;
>  	struct isp_video video_out;
>  
> -	struct prev_params params;
> -	unsigned int shadow_update:1;
> +	struct {
> +		struct prev_params params[2];
> +		u32 active;
> +		spinlock_t lock;
> +	} params;
> +
>  	enum isp_pipeline_stream_state state;
>  	wait_queue_head_t wait;
>  	atomic_t stopping;
> -	spinlock_t lock;
> -	u32 update;
>  };
>  
>  struct isp_device;
> -- 
> 1.7.3.4
> 

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

Re: [PATCH v3 9/9] omap3isp: preview: Shorten shadow update delay

[Laurent Pinchart]

Hi Sakari,

On Tuesday 17 April 2012 17:26:00 Sakari Ailus wrote:
> Hi Laurent,
> 
> Many thanks for the patch!!

And thank you for the review.

> On Mon, Apr 16, 2012 at 03:29:54PM +0200, Laurent Pinchart wrote:
> > When applications modify preview engine parameters, the new values are
> > applied to the hardware by the preview engine interrupt handler during
> > vertical blanking. If the parameters are being changed when the
> > interrupt handler is called, it just delays applying the parameters
> > until the next frame.
> > 
> > If an application modifies the parameters for every frame, and the
> > preview engine interrupt is triggerred synchronously, the parameters are
> > never applied to the hardware.
> > 
> > Fix this by storing new parameters in a shadow copy, and switch the
> > active parameters with the shadow values atomically.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/video/omap3isp/isppreview.c |  298 +++++++++++++++---------
> >  drivers/media/video/omap3isp/isppreview.h |   21 ++-
> >  2 files changed, 212 insertions(+), 107 deletions(-)
> > 
> > diff --git a/drivers/media/video/omap3isp/isppreview.c
> > b/drivers/media/video/omap3isp/isppreview.c index e12df2c..5ccfe46 100644
> > --- a/drivers/media/video/omap3isp/isppreview.c
> > +++ b/drivers/media/video/omap3isp/isppreview.c
> > @@ -649,12 +649,18 @@ preview_config_rgb_to_ycbcr(struct isp_prev_device
> > *prev, const void *prev_csc)
>
> Not related to this patch, but shouldn't the above function be called
> preview_config_csc()?

That would make sense, yes. I'll add a patch for that.

> >  static void
> >  preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
> >  {
> > -	struct prev_params *params = &prev->params;
> > +	struct prev_params *params;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&prev->params.lock, flags);
> > +	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
> > +	       ? &prev->params.params[0] : &prev->params.params[1];
> > 
> >  	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
> >  		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
> > -		prev->update |= OMAP3ISP_PREV_CONTRAST;
> > +		params->update |= OMAP3ISP_PREV_CONTRAST;
> >  	}
> > +	spin_unlock_irqrestore(&prev->params.lock, flags);
> >  }
> >  
> >  /*
> > @@ -681,12 +687,18 @@ preview_config_contrast(struct isp_prev_device
> > *prev, const void *params)
> >  static void
> >  preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
> >  {
> > -	struct prev_params *params = &prev->params;
> > +	struct prev_params *params;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&prev->params.lock, flags);
> > +	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
> > +	       ? &prev->params.params[0] : &prev->params.params[1];
> 
> params = prev->params.params[!(prev->params.active &
> OMAP3ISP_PREV_CONTRAST)];

I've thought about that, but it doesn't fit on a single line. After being 
split in two lines the result is less readable in my opinion. Do you think I 
should change it nonetheless ?

> Same in contrast.
> 
> And shouldn't this be brightness here btw.?

Good point. Fixed.

> >  	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
> >  		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
> > -		prev->update |= OMAP3ISP_PREV_BRIGHTNESS;
> > +		params->update |= OMAP3ISP_PREV_BRIGHTNESS;
> >  	}
> > +	spin_unlock_irqrestore(&prev->params.lock, flags);
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 9/9] omap3isp: preview: Shorten shadow update delay

[Sakari Ailus]

Hi Laurent,

On Tue, Apr 17, 2012 at 06:09:27PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 17 April 2012 17:26:00 Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > Many thanks for the patch!!
> 
> And thank you for the review.
> 
> > On Mon, Apr 16, 2012 at 03:29:54PM +0200, Laurent Pinchart wrote:
> > > When applications modify preview engine parameters, the new values are
> > > applied to the hardware by the preview engine interrupt handler during
> > > vertical blanking. If the parameters are being changed when the
> > > interrupt handler is called, it just delays applying the parameters
> > > until the next frame.
> > > 
> > > If an application modifies the parameters for every frame, and the
> > > preview engine interrupt is triggerred synchronously, the parameters are
> > > never applied to the hardware.
> > > 
> > > Fix this by storing new parameters in a shadow copy, and switch the
> > > active parameters with the shadow values atomically.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/video/omap3isp/isppreview.c |  298 +++++++++++++++---------
> > >  drivers/media/video/omap3isp/isppreview.h |   21 ++-
> > >  2 files changed, 212 insertions(+), 107 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/omap3isp/isppreview.c
> > > b/drivers/media/video/omap3isp/isppreview.c index e12df2c..5ccfe46 100644
> > > --- a/drivers/media/video/omap3isp/isppreview.c
> > > +++ b/drivers/media/video/omap3isp/isppreview.c
> > > @@ -649,12 +649,18 @@ preview_config_rgb_to_ycbcr(struct isp_prev_device
> > > *prev, const void *prev_csc)
> >
> > Not related to this patch, but shouldn't the above function be called
> > preview_config_csc()?
> 
> That would make sense, yes. I'll add a patch for that.
> 
> > >  static void
> > >  preview_update_contrast(struct isp_prev_device *prev, u8 contrast)
> > >  {
> > > -	struct prev_params *params = &prev->params;
> > > +	struct prev_params *params;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&prev->params.lock, flags);
> > > +	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
> > > +	       ? &prev->params.params[0] : &prev->params.params[1];
> > > 
> > >  	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
> > >  		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
> > > -		prev->update |= OMAP3ISP_PREV_CONTRAST;
> > > +		params->update |= OMAP3ISP_PREV_CONTRAST;
> > >  	}
> > > +	spin_unlock_irqrestore(&prev->params.lock, flags);
> > >  }
> > >  
> > >  /*
> > > @@ -681,12 +687,18 @@ preview_config_contrast(struct isp_prev_device
> > > *prev, const void *params)
> > >  static void
> > >  preview_update_brightness(struct isp_prev_device *prev, u8 brightness)
> > >  {
> > > -	struct prev_params *params = &prev->params;
> > > +	struct prev_params *params;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&prev->params.lock, flags);
> > > +	params = (prev->params.active & OMAP3ISP_PREV_CONTRAST)
> > > +	       ? &prev->params.params[0] : &prev->params.params[1];
> > 
> > params = prev->params.params[!(prev->params.active &
> > OMAP3ISP_PREV_CONTRAST)];
> 
> I've thought about that, but it doesn't fit on a single line. After being 
> split in two lines the result is less readable in my opinion. Do you think I 
> should change it nonetheless ?

I would do it as you use similar constructs elsewhere. It's up to you.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk