Hi Laurent,
Laurent Pinchart wrote:
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.
As this is only 16 kiB, I'd go with a static allocation.
In the long run such allocations should be done dynamically when the
first user accesses the device. I guess this isn't the only struct of
its kind so likely more than 16 kiB would be gained by making the
allocation depend on actual users.
We could also use vmalloc() instead --- this memory doesn't have to be
physically contiguous. It would "fix" in-ioctl allocation but on the
expense on larger (I suppose) allocation time, so I still wouldn't do it
on every ioctl.
Albeit I feel it wouldn't be much of a job. One function call to preview
code in both isp_get() / isp_put() mostly plus a bit of error handling,
instead of omap3isp_preview_init() / omap3isp_preview_cleanup()?
Still, I'm fine with introducing dynamic allocation later on.
Cheers,
--
Sakari Ailus
sakari.ailus@xxxxxx
--
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
