[PATCH 0/3] OMAP3 ISP preview engine fixes

[PATCH 0/3] OMAP3 ISP preview engine fixes

[Laurent Pinchart]

Hello,

Here are three patches for the OMAP3 ISP that improve the preview engine
configuration.

Patch 1/3 fixes in bug in the driver that shouldn't cause any misbehaviour,
but is still wrong nonetheless. Patch 2/3 speeds up interrupt handling for the
common case when no parameter has been modified, and patch 3/3 fixes a shadow
update issue that resulted in parameters never being applied in a common race
condition case.

Laurent Pinchart (3):
  omap3isp: preview: Skip brightness and contrast in configuration
    ioctl
  omap3isp: preview: Optimize parameters setup for the common case
  omap3isp: preview: Shorten shadow update delay

 drivers/media/video/omap3isp/isppreview.c |  129 ++++++++++++++++++++---------
 drivers/media/video/omap3isp/isppreview.h |   19 +++--
 2 files changed, 101 insertions(+), 47 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] 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>
---
 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

Re: [PATCH 1/3] omap3isp: preview: Skip brightness and contrast in configuration ioctl

[Sakari Ailus]

On Mon, Mar 26, 2012 at 04:42:29PM +0200, Laurent Pinchart wrote:
> 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>

Thanks!

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

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

[PATCH 2/3] 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>
---
 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..2b5c137 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 (params->update == 0)
+		return;
+
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
 		attr = &update_attrs[i];
 
-- 
1.7.3.4

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

[Sakari Ailus]

Thanks!

On Mon, Mar 26, 2012 at 04:42:30PM +0200, 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>

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

[PATCH 3/3] 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 replace the
active parameters with the shadow values atomically.

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

diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
index 2b5c137..34fecc9 100644
--- a/drivers/media/video/omap3isp/isppreview.c
+++ b/drivers/media/video/omap3isp/isppreview.c
@@ -649,12 +649,17 @@ 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;
 
 	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
 		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
-		prev->update |= PREV_CONTRAST;
+		params->update |= PREV_CONTRAST;
 	}
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /*
@@ -681,12 +686,17 @@ 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;
 
 	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
 		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
-		prev->update |= PREV_BRIGHTNESS;
+		params->update |= PREV_BRIGHTNESS;
 	}
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 }
 
 /*
@@ -886,20 +896,24 @@ static int preview_config(struct isp_prev_device *prev,
 			  struct omap3isp_prev_update_config *cfg)
 {
 	struct prev_params *params;
+	struct prev_params *shadow;
 	struct preview_update *attr;
+	unsigned long flags;
 	int i, bit, rval = 0;
 
-	params = &prev->params;
 	if (cfg->update == 0)
 		return 0;
 
-	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
-		unsigned long flags;
+	params = kmalloc(sizeof(*params), GFP_KERNEL);
+	if (params == NULL)
+		return -ENOMEM;
 
-		spin_lock_irqsave(&prev->lock, flags);
-		prev->shadow_update = 1;
-		spin_unlock_irqrestore(&prev->lock, flags);
-	}
+	spin_lock_irqsave(&prev->params.lock, flags);
+	memcpy(params, prev->params.shadow ? : prev->params.active,
+	       sizeof(*params));
+	spin_unlock_irqrestore(&prev->params.lock, flags);
+
+	params->update = 0;
 
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
 		attr = &update_attrs[i];
@@ -926,11 +940,28 @@ static int preview_config(struct isp_prev_device *prev,
 			params->features &= ~attr->feature_bit;
 		}
 
-		prev->update |= attr->feature_bit;
+		params->update |= attr->feature_bit;
+	}
+
+	if (rval < 0) {
+		kfree(params);
+		return rval;
 	}
 
-	prev->shadow_update = 0;
-	return rval;
+	spin_lock_irqsave(&prev->params.lock, flags);
+	/* If shadow parameters are still present, keep their update flags as
+	 * the hardware hasn't been updated yet. The values have been copied at
+	 * the beginning of the function.
+	 */
+	if (prev->params.shadow)
+		params->update |= prev->params.shadow->update;
+
+	shadow = prev->params.shadow;
+	prev->params.shadow = params;
+	spin_unlock_irqrestore(&prev->params.lock, flags);
+
+	kfree(shadow);
+	return 0;
 }
 
 /*
@@ -941,7 +972,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 prev_params *params = prev->params.active;
 	struct preview_update *attr;
 	int i, bit;
 	void *param_ptr;
@@ -952,7 +983,7 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
 		attr = &update_attrs[i];
 
-		if (!(prev->update & attr->feature_bit))
+		if (!(params->update & attr->feature_bit))
 			continue;
 		bit = params->features & attr->feature_bit;
 		if (bit) {
@@ -967,7 +998,7 @@ static void preview_setup_hw(struct isp_prev_device *prev)
 			if (attr->enable)
 				attr->enable(prev, 0);
 
-		prev->update &= ~attr->feature_bit;
+		params->update &= ~attr->feature_bit;
 	}
 }
 
@@ -1004,14 +1035,15 @@ preview_config_ycpos(struct isp_prev_device *prev,
  */
 static void preview_config_averager(struct isp_prev_device *prev, u8 average)
 {
+	struct prev_params *params = prev->params.active;
 	struct isp_device *isp = to_isp_device(prev);
 	int reg = 0;
 
-	if (prev->params.cfa.format == OMAP3ISP_CFAFMT_BAYER)
+	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;
@@ -1032,7 +1064,7 @@ static void preview_config_averager(struct isp_prev_device *prev, u8 average)
 static void preview_config_input_size(struct isp_prev_device *prev)
 {
 	struct isp_device *isp = to_isp_device(prev);
-	struct prev_params *params = &prev->params;
+	struct prev_params *params = prev->params.active;
 	unsigned int sph = prev->crop.left;
 	unsigned int eph = prev->crop.left + prev->crop.width - 1;
 	unsigned int slv = prev->crop.top;
@@ -1189,7 +1221,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.params.active->update = PREV_FEATURES_END - 1;
 	preview_setup_hw(&isp->isp_prev);
 }
 
@@ -1249,12 +1281,19 @@ 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
+ *
+ * Returns 0 on success or -ENOMEM if parameters memory can't be allocated.
  */
-static void preview_init_params(struct isp_prev_device *prev)
+static int 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);
+
+	params = kzalloc(sizeof(*params), GFP_KERNEL);
+	if (params == NULL)
+		return -ENOMEM;
 
 	/* Init values */
 	params->contrast = ISPPRV_CONTRAST_DEF * ISPPRV_CONTRAST_UNITS;
@@ -1297,7 +1336,10 @@ static void preview_init_params(struct isp_prev_device *prev)
 			 | PREV_RGB2RGB | PREV_COLOR_CONV | PREV_WB
 			 | PREV_BRIGHTNESS | PREV_CONTRAST;
 
-	prev->update = PREV_FEATURES_END - 1;
+	params->update = PREV_FEATURES_END - 1;
+
+	prev->params.active = params;
+	return 0;
 }
 
 /*
@@ -1457,16 +1499,17 @@ void omap3isp_preview_isr(struct isp_prev_device *prev)
 	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);
+	if (prev->params.shadow) {
+		kfree(prev->params.active);
+		prev->params.active = prev->params.shadow;
+		prev->params.shadow = NULL;
+	}
+	spin_unlock_irqrestore(&prev->params.lock, flags);
 
 	preview_setup_hw(prev);
 	preview_config_input_size(prev);
 
-done:
-	spin_unlock_irqrestore(&prev->lock, flags);
-
 	if (prev->input == PREVIEW_INPUT_MEMORY ||
 	    prev->output & PREVIEW_OUTPUT_MEMORY)
 		preview_isr_buffer(prev);
@@ -1557,7 +1600,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)
@@ -1594,11 +1636,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;
 	}
@@ -2206,17 +2246,20 @@ error_video_in:
 }
 
 /*
- * isp_preview_init - Previewer initialization.
+ * omap3isp_preview_init - Previewer initialization.
  * @dev : Pointer to ISP device
  * return -ENOMEM or zero on success
  */
 int omap3isp_preview_init(struct isp_device *isp)
 {
 	struct isp_prev_device *prev = &isp->isp_prev;
+	int ret;
 
-	spin_lock_init(&prev->lock);
 	init_waitqueue_head(&prev->wait);
-	preview_init_params(prev);
+
+	ret = preview_init_params(prev);
+	if (ret < 0)
+		return ret;
 
 	return preview_init_entities(prev);
 }
@@ -2229,4 +2272,7 @@ void omap3isp_preview_cleanup(struct isp_device *isp)
 	omap3isp_video_cleanup(&prev->video_in);
 	omap3isp_video_cleanup(&prev->video_out);
 	media_entity_cleanup(&prev->subdev.entity);
+
+	kfree(prev->params.active);
+	kfree(prev->params.shadow);
 }
diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
index 0968660..c38ed09 100644
--- a/drivers/media/video/omap3isp/isppreview.h
+++ b/drivers/media/video/omap3isp/isppreview.h
@@ -89,6 +89,7 @@ enum preview_ycpos_mode {
 /*
  * struct prev_params - Structure for all configuration
  * @features: Set of features enabled.
+ * @update: Bitmask of the parameters to be updated
  * @cfa: CFA coefficients.
  * @csup: Chroma suppression coefficients.
  * @luma: Luma enhancement coefficients.
@@ -106,6 +107,7 @@ enum preview_ycpos_mode {
  */
 struct prev_params {
 	u32 features;
+	u32 update;
 	struct omap3isp_prev_cfa cfa;
 	struct omap3isp_prev_csup csup;
 	struct omap3isp_prev_luma luma;
@@ -157,12 +159,11 @@ struct isptables_update {
  * @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.active: Active module configuration data
+ * @params.shadow: Shadow module configuration data
+ * @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.
  */
@@ -179,13 +180,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 *active;
+		struct prev_params *shadow;
+		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 3/3] omap3isp: preview: Shorten shadow update delay

[Sakari Ailus]

Hi Laurent,

Thanks for the patch.

On Mon, Mar 26, 2012 at 04:42:31PM +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 replace the
> active parameters with the shadow values atomically.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/omap3isp/isppreview.c |  122 ++++++++++++++++++++---------
>  drivers/media/video/omap3isp/isppreview.h |   19 +++--
>  2 files changed, 95 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index 2b5c137..34fecc9 100644
> --- a/drivers/media/video/omap3isp/isppreview.c
> +++ b/drivers/media/video/omap3isp/isppreview.c
> @@ -649,12 +649,17 @@ 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;
>  
>  	if (params->contrast != (contrast * ISPPRV_CONTRAST_UNITS)) {
>  		params->contrast = contrast * ISPPRV_CONTRAST_UNITS;
> -		prev->update |= PREV_CONTRAST;
> +		params->update |= PREV_CONTRAST;
>  	}
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  }
>  
>  /*
> @@ -681,12 +686,17 @@ 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;
>  
>  	if (params->brightness != (brightness * ISPPRV_BRIGHT_UNITS)) {
>  		params->brightness = brightness * ISPPRV_BRIGHT_UNITS;
> -		prev->update |= PREV_BRIGHTNESS;
> +		params->update |= PREV_BRIGHTNESS;
>  	}
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  }
>  
>  /*
> @@ -886,20 +896,24 @@ static int preview_config(struct isp_prev_device *prev,
>  			  struct omap3isp_prev_update_config *cfg)
>  {
>  	struct prev_params *params;
> +	struct prev_params *shadow;
>  	struct preview_update *attr;
> +	unsigned long flags;
>  	int i, bit, rval = 0;
>  
> -	params = &prev->params;
>  	if (cfg->update == 0)
>  		return 0;
>  
> -	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
> -		unsigned long flags;
> +	params = kmalloc(sizeof(*params), GFP_KERNEL);
> +	if (params == NULL)
> +		return -ENOMEM;
>  
> -		spin_lock_irqsave(&prev->lock, flags);
> -		prev->shadow_update = 1;
> -		spin_unlock_irqrestore(&prev->lock, flags);
> -	}
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	memcpy(params, prev->params.shadow ? : prev->params.active,
> +	       sizeof(*params));
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
> +
> +	params->update = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
>  		attr = &update_attrs[i];

I think it's partly a matter of taste but --- would you think it'd make
sense to allocate the another configuration structure statically? I didn't
check the actual size of the configuration but it seems to be pretty big.
Handling allocation failures in applications is a nuisance, but also
allocating such largish chunks to just to be on the safe side doesn't sound
very appealing either.

Say, if you're capturing a photo and you allocation fails here. Should you
just retry it a few times, or fail immediately? Random allocation failures
are not unforeseen even on systems with plenty of memory. Not that it should
work this way I guess...

Have you checked what's the size of this struct btw.?

> @@ -926,11 +940,28 @@ static int preview_config(struct isp_prev_device *prev,
>  			params->features &= ~attr->feature_bit;
>  		}
>  
> -		prev->update |= attr->feature_bit;
> +		params->update |= attr->feature_bit;
> +	}
> +
> +	if (rval < 0) {
> +		kfree(params);
> +		return rval;
>  	}
>  
> -	prev->shadow_update = 0;
> -	return rval;
> +	spin_lock_irqsave(&prev->params.lock, flags);
> +	/* If shadow parameters are still present, keep their update flags as
> +	 * the hardware hasn't been updated yet. The values have been copied at
> +	 * the beginning of the function.
> +	 */
> +	if (prev->params.shadow)
> +		params->update |= prev->params.shadow->update;
> +
> +	shadow = prev->params.shadow;
> +	prev->params.shadow = params;
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
> +
> +	kfree(shadow);
> +	return 0;
>  }
>  
>  /*
> @@ -941,7 +972,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 prev_params *params = prev->params.active;
>  	struct preview_update *attr;
>  	int i, bit;
>  	void *param_ptr;
> @@ -952,7 +983,7 @@ static void preview_setup_hw(struct isp_prev_device *prev)
>  	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
>  		attr = &update_attrs[i];
>  
> -		if (!(prev->update & attr->feature_bit))
> +		if (!(params->update & attr->feature_bit))
>  			continue;
>  		bit = params->features & attr->feature_bit;
>  		if (bit) {
> @@ -967,7 +998,7 @@ static void preview_setup_hw(struct isp_prev_device *prev)
>  			if (attr->enable)
>  				attr->enable(prev, 0);
>  
> -		prev->update &= ~attr->feature_bit;
> +		params->update &= ~attr->feature_bit;
>  	}
>  }
>  
> @@ -1004,14 +1035,15 @@ preview_config_ycpos(struct isp_prev_device *prev,
>   */
>  static void preview_config_averager(struct isp_prev_device *prev, u8 average)
>  {
> +	struct prev_params *params = prev->params.active;
>  	struct isp_device *isp = to_isp_device(prev);
>  	int reg = 0;
>  
> -	if (prev->params.cfa.format == OMAP3ISP_CFAFMT_BAYER)
> +	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;
> @@ -1032,7 +1064,7 @@ static void preview_config_averager(struct isp_prev_device *prev, u8 average)
>  static void preview_config_input_size(struct isp_prev_device *prev)
>  {
>  	struct isp_device *isp = to_isp_device(prev);
> -	struct prev_params *params = &prev->params;
> +	struct prev_params *params = prev->params.active;
>  	unsigned int sph = prev->crop.left;
>  	unsigned int eph = prev->crop.left + prev->crop.width - 1;
>  	unsigned int slv = prev->crop.top;
> @@ -1189,7 +1221,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.params.active->update = PREV_FEATURES_END - 1;
>  	preview_setup_hw(&isp->isp_prev);
>  }
>  
> @@ -1249,12 +1281,19 @@ 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
> + *
> + * Returns 0 on success or -ENOMEM if parameters memory can't be allocated.
>   */
> -static void preview_init_params(struct isp_prev_device *prev)
> +static int 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);
> +
> +	params = kzalloc(sizeof(*params), GFP_KERNEL);
> +	if (params == NULL)
> +		return -ENOMEM;
>  
>  	/* Init values */
>  	params->contrast = ISPPRV_CONTRAST_DEF * ISPPRV_CONTRAST_UNITS;
> @@ -1297,7 +1336,10 @@ static void preview_init_params(struct isp_prev_device *prev)
>  			 | PREV_RGB2RGB | PREV_COLOR_CONV | PREV_WB
>  			 | PREV_BRIGHTNESS | PREV_CONTRAST;
>  
> -	prev->update = PREV_FEATURES_END - 1;
> +	params->update = PREV_FEATURES_END - 1;
> +
> +	prev->params.active = params;
> +	return 0;
>  }
>  
>  /*
> @@ -1457,16 +1499,17 @@ void omap3isp_preview_isr(struct isp_prev_device *prev)
>  	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);
> +	if (prev->params.shadow) {
> +		kfree(prev->params.active);
> +		prev->params.active = prev->params.shadow;
> +		prev->params.shadow = NULL;
> +	}
> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>  
>  	preview_setup_hw(prev);
>  	preview_config_input_size(prev);
>  
> -done:
> -	spin_unlock_irqrestore(&prev->lock, flags);
> -
>  	if (prev->input == PREVIEW_INPUT_MEMORY ||
>  	    prev->output & PREVIEW_OUTPUT_MEMORY)
>  		preview_isr_buffer(prev);
> @@ -1557,7 +1600,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)
> @@ -1594,11 +1636,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;
>  	}
> @@ -2206,17 +2246,20 @@ error_video_in:
>  }
>  
>  /*
> - * isp_preview_init - Previewer initialization.
> + * omap3isp_preview_init - Previewer initialization.
>   * @dev : Pointer to ISP device
>   * return -ENOMEM or zero on success
>   */
>  int omap3isp_preview_init(struct isp_device *isp)
>  {
>  	struct isp_prev_device *prev = &isp->isp_prev;
> +	int ret;
>  
> -	spin_lock_init(&prev->lock);
>  	init_waitqueue_head(&prev->wait);
> -	preview_init_params(prev);
> +
> +	ret = preview_init_params(prev);
> +	if (ret < 0)
> +		return ret;
>  
>  	return preview_init_entities(prev);
>  }
> @@ -2229,4 +2272,7 @@ void omap3isp_preview_cleanup(struct isp_device *isp)
>  	omap3isp_video_cleanup(&prev->video_in);
>  	omap3isp_video_cleanup(&prev->video_out);
>  	media_entity_cleanup(&prev->subdev.entity);
> +
> +	kfree(prev->params.active);
> +	kfree(prev->params.shadow);
>  }
> diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
> index 0968660..c38ed09 100644
> --- a/drivers/media/video/omap3isp/isppreview.h
> +++ b/drivers/media/video/omap3isp/isppreview.h
> @@ -89,6 +89,7 @@ enum preview_ycpos_mode {
>  /*
>   * struct prev_params - Structure for all configuration
>   * @features: Set of features enabled.
> + * @update: Bitmask of the parameters to be updated
>   * @cfa: CFA coefficients.
>   * @csup: Chroma suppression coefficients.
>   * @luma: Luma enhancement coefficients.
> @@ -106,6 +107,7 @@ enum preview_ycpos_mode {
>   */
>  struct prev_params {
>  	u32 features;
> +	u32 update;
>  	struct omap3isp_prev_cfa cfa;
>  	struct omap3isp_prev_csup csup;
>  	struct omap3isp_prev_luma luma;
> @@ -157,12 +159,11 @@ struct isptables_update {
>   * @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.active: Active module configuration data
> + * @params.shadow: Shadow module configuration data
> + * @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.
>   */
> @@ -179,13 +180,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 *active;
> +		struct prev_params *shadow;
> +		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
> 

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

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

[Laurent Pinchart]

Hi Sakari,

On Monday 26 March 2012 19:23:23 Sakari Ailus wrote:
> On Mon, Mar 26, 2012 at 04:42:31PM +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 replace the
> > active parameters with the shadow values atomically.

[snip]

> > @@-886,20 +896,24@@ static int preview_config(struct isp_prev_device*prev,
> >  			  struct omap3isp_prev_update_config *cfg)
> >  {
> >  	struct prev_params *params;
> > +	struct prev_params *shadow;
> >  	struct preview_update *attr;
> > +	unsigned long flags;
> >  	int i, bit, rval = 0;
> > 
> > -	params = &prev->params;
> >  	if (cfg->update == 0)
> >  		return 0;
> > 
> > -	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
> > -		unsigned long flags;
> > +	params = kmalloc(sizeof(*params), GFP_KERNEL);
> > +	if (params == NULL)
> > +		return -ENOMEM;
> > 
> > -		spin_lock_irqsave(&prev->lock, flags);
> > -		prev->shadow_update = 1;
> > -		spin_unlock_irqrestore(&prev->lock, flags);
> > -	}
> > +	spin_lock_irqsave(&prev->params.lock, flags);
> > +	memcpy(params, prev->params.shadow ? : prev->params.active,
> > +	       sizeof(*params));
> > +	spin_unlock_irqrestore(&prev->params.lock, flags);
> > +
> > +	params->update = 0;
> > 
> >  	for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
> >  	
> >  		attr = &update_attrs[i];
> 
> I think it's partly a matter of taste but --- would you think it'd make
> sense to allocate the another configuration structure statically? I didn't
> check the actual size of the configuration but it seems to be pretty big.
> Handling allocation failures in applications is a nuisance, but also
> allocating such largish chunks to just to be on the safe side doesn't sound
> very appealing either.

I'd like that better as well, but then we'll run into the same issue that this 
patch tries to fix. I'll try to find a better solution.

> Say, if you're capturing a photo and you allocation fails here. Should you
> just retry it a few times, or fail immediately? Random allocation failures
> are not unforeseen even on systems with plenty of memory. Not that it should
> work this way I guess...
> 
> Have you checked what's the size of this struct btw.?

It's big. Around 16k if I'm not mistaken.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> On Monday 26 March 2012 19:23:23 Sakari Ailus wrote:
>> On Mon, Mar 26, 2012 at 04:42:31PM +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 replace the
>>> active parameters with the shadow values atomically.
>
> [snip]
>
>>> @@-886,20 +896,24@@ static int preview_config(struct isp_prev_device*prev,
>>>   			  struct omap3isp_prev_update_config *cfg)
>>>   {
>>>   	struct prev_params *params;
>>> +	struct prev_params *shadow;
>>>   	struct preview_update *attr;
>>> +	unsigned long flags;
>>>   	int i, bit, rval = 0;
>>>
>>> -	params =&prev->params;
>>>   	if (cfg->update == 0)
>>>   		return 0;
>>>
>>> -	if (prev->state != ISP_PIPELINE_STREAM_STOPPED) {
>>> -		unsigned long flags;
>>> +	params = kmalloc(sizeof(*params), GFP_KERNEL);
>>> +	if (params == NULL)
>>> +		return -ENOMEM;
>>>
>>> -		spin_lock_irqsave(&prev->lock, flags);
>>> -		prev->shadow_update = 1;
>>> -		spin_unlock_irqrestore(&prev->lock, flags);
>>> -	}
>>> +	spin_lock_irqsave(&prev->params.lock, flags);
>>> +	memcpy(params, prev->params.shadow ? : prev->params.active,
>>> +	       sizeof(*params));
>>> +	spin_unlock_irqrestore(&prev->params.lock, flags);
>>> +
>>> +	params->update = 0;
>>>
>>>   	for (i = 0; i<  ARRAY_SIZE(update_attrs); i++) {
>>>   	
>>>   		attr =&update_attrs[i];
>>
>> I think it's partly a matter of taste but --- would you think it'd make
>> sense to allocate the another configuration structure statically? I didn't
>> check the actual size of the configuration but it seems to be pretty big.
>> Handling allocation failures in applications is a nuisance, but also
>> allocating such largish chunks to just to be on the safe side doesn't sound
>> very appealing either.
>
> I'd like that better as well, but then we'll run into the same issue that this
> patch tries to fix. I'll try to find a better solution.
>
>> Say, if you're capturing a photo and you allocation fails here. Should you
>> just retry it a few times, or fail immediately? Random allocation failures
>> are not unforeseen even on systems with plenty of memory. Not that it should
>> work this way I guess...
>>
>> Have you checked what's the size of this struct btw.?
>
> It's big. Around 16k if I'm not mistaken.

As this is only 16 kiB, I'd go with a static allocation.

In the long run such allocations should be done dynamically when the 
first user accesses the device. I guess this isn't the only struct of 
its kind so likely more than 16 kiB would be gained by making the 
allocation depend on actual users.

We could also use vmalloc() instead --- this memory doesn't have to be 
physically contiguous. It would "fix" in-ioctl allocation but on the 
expense on larger (I suppose) allocation time, so I still wouldn't do it 
on every ioctl.

Albeit I feel it wouldn't be much of a job. One function call to preview 
code in both isp_get() / isp_put() mostly plus a bit of error handling, 
instead of omap3isp_preview_init() / omap3isp_preview_cleanup()?

Still, I'm fine with introducing dynamic allocation later on.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi