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@xxxxxxxxxxxxxxxx>
> > ---
> >
> > 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
--
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
