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@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 ?
I would do it as you use similar constructs elsewhere. It's up to you.
Cheers,
--
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
