[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxx	jabber/XMPP/Gmail: sailus@xxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Linux Input]     [Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Devices]     [Yosemite Backpacking]

Add to Google Powered by Linux