Hi Laurent,
Thanks for the patch. I've got a few comments below.
On Wed, Mar 28, 2012 at 02:00:01PM +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@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/video/omap3isp/isppreview.c | 137 +++++++++++++++++++++--------
> drivers/media/video/omap3isp/isppreview.h | 21 +++--
> 2 files changed, 112 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/video/omap3isp/isppreview.c b/drivers/media/video/omap3isp/isppreview.c
> index 2b5c137..3267d83 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);
> }
>
> /*
> @@ -887,19 +897,19 @@ static int preview_config(struct isp_prev_device *prev,
> {
> struct prev_params *params;
> 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;
> + spin_lock_irqsave(&prev->params.lock, flags);
> + params = prev->params.shadow;
> + memcpy(params, prev->params.active, sizeof(*params));
Why memcpy()? Couldn't the same be achieved by swapping the pointers?
I also have a feeling that the implementation is still more complex than
would really be needed.
Calls to preview_config() should also be serialised. Otherwise it's possible
to call this simultaneously from user space. That likely wasn't an issue in
the old implementation.
> + params->busy = true;
> + spin_unlock_irqrestore(&prev->params.lock, flags);
>
> - spin_lock_irqsave(&prev->lock, flags);
> - prev->shadow_update = 1;
> - spin_unlock_irqrestore(&prev->lock, flags);
> - }
> + params->update = 0;
>
> for (i = 0; i < ARRAY_SIZE(update_attrs); i++) {
> attr = &update_attrs[i];
> @@ -926,11 +936,34 @@ 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);
I think this must be a remnant from the earlier version of the patch. :-)
> + return rval;
> }
>
> - prev->shadow_update = 0;
> - return rval;
> + spin_lock_irqsave(&prev->params.lock, flags);
> + params->busy = false;
> +
> + /* If active parameters are not in use, switch the active and shadow
> + * parameters.
> + */
> + if (!prev->params.active->busy) {
> + /* Make sure to keep the active update flags as the hardware
> + * hasn't been updated yet. The values have been copied at the
> + * beginning of the function.
> + */
> + params->update |= prev->params.active->update;
> + prev->params.active->update = 0;
> +
> + prev->params.shadow = prev->params.active;
> + prev->params.active = params;
> + }
> + spin_unlock_irqrestore(&prev->params.lock, flags);
> +
> + return 0;
> }
>
> /*
> @@ -941,7 +974,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 +985,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 +1000,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 +1037,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 +1066,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 +1223,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 +1283,18 @@ 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.
This comment no longer needs to be changed.
> */
> -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 = &prev->params.params[0];
> + params->busy = false;
>
> /* Init values */
> params->contrast = ISPPRV_CONTRAST_DEF * ISPPRV_CONTRAST_UNITS;
> @@ -1297,7 +1337,12 @@ 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 = &prev->params.params[0];
> + prev->params.shadow = &prev->params.params[1];
> +
> + return 0;
> }
>
> /*
> @@ -1326,6 +1371,11 @@ 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;
> +
> + spin_lock_irqsave(&prev->params.lock, flags);
> + prev->params.active->busy = true;
> + spin_unlock_irqrestore(&prev->params.lock, flags);
>
> preview_setup_hw(prev);
>
> @@ -1365,6 +1415,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);
> + prev->params.active->busy = false;
> + spin_unlock_irqrestore(&prev->params.lock, flags);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -1452,26 +1506,33 @@ static void preview_isr_buffer(struct isp_prev_device *prev)
> */
> void omap3isp_preview_isr(struct isp_prev_device *prev)
> {
> + struct prev_params *params;
> unsigned long flags;
>
> 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->busy && prev->params.shadow->update) {
> + params = prev->params.active;
> + prev->params.active = prev->params.shadow;
> + prev->params.shadow = params;
> + }
> + prev->params.active->busy = true;
> + 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);
> else if (prev->state == ISP_PIPELINE_STREAM_CONTINUOUS)
> preview_enable_oneshot(prev);
> +
> + spin_lock_irqsave(&prev->params.lock, flags);
> + prev->params.active->busy = false;
> + spin_unlock_irqrestore(&prev->params.lock, flags);
> }
>
> /* -----------------------------------------------------------------------------
> @@ -1557,7 +1618,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 +1654,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 +2264,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);
> }
> diff --git a/drivers/media/video/omap3isp/isppreview.h b/drivers/media/video/omap3isp/isppreview.h
> index 67723c7..e756807 100644
> --- a/drivers/media/video/omap3isp/isppreview.h
> +++ b/drivers/media/video/omap3isp/isppreview.h
> @@ -88,6 +88,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.
> @@ -104,7 +105,9 @@ enum preview_ycpos_mode {
> * @brightness: Brightness.
> */
> struct prev_params {
> + bool busy;
> u32 features;
> + u32 update;
> struct omap3isp_prev_cfa cfa;
> struct omap3isp_prev_csup csup;
> struct omap3isp_prev_luma luma;
> @@ -156,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.
> */
> @@ -178,13 +180,16 @@ 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];
> + 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
>
Kind regards,
--
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
