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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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