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