[alsa-devel] [GIT PULL FOR 2.6.39] Media controller and OMAP3 ISP driver

[Laurent Pinchart]

Hi Mauro,

On Monday 07 March 2011 14:00:20 Mauro Carvalho Chehab wrote:
> Em 07-03-2011 09:02, Hans Verkuil escreveu:
> > On Monday, March 07, 2011 12:50:28 Mauro Carvalho Chehab wrote:
> >> Em 06-03-2011 14:21, Laurent Pinchart escreveu:
> >> > On Sunday 06 March 2011 14:32:44 Mauro Carvalho Chehab wrote:
> >> >> Em 06-03-2011 08:38, Laurent Pinchart escreveu:
> >> >>> On Sunday 06 March 2011 11:56:04 Mauro Carvalho Chehab wrote:
> >> >>>> Em 05-03-2011 20:23, Sylwester Nawrocki escreveu:
> >> >>>> 
> >> >>>> A somewhat unrelated question that occurred to me today: what
> >> >>>> happens when a format change happens while streaming?
> >> >>>> 
> >> >>>> Considering that some formats need more bits than others, this could
> >> >>>> lead into buffer overflows, either internally at the device or
> >> >>>> externally, on bridges that just forward whatever it receives to the
> >> >>>> DMA buffers (there are some that just does that). I didn't see
> >> >>>> anything inside the mc code preventing such condition to happen, and
> >> >>>> probably implementing it won't be an easy job. So, one alternative
> >> >>>> would be to require some special CAPS if userspace tries to set the
> >> >>>> mbus format directly, or to recommend userspace to create media
> >> >>>> controller nodes with 0600 permission.
> >> >>> 
> >> >>> That's not really a media controller issue. Whether formats can be
> >> >>> changed during streaming is a driver decision. The OMAP3 ISP driver
> >> >>> won't allow formats to be changed during streaming. If the hardware
> >> >>> allows for such format changes, drivers can implement support for
> >> >>> that and make sure that no buffer overflow will occur.
> >> >> 
> >> >> Such issues is caused by having two API's that allow format changes,
> >> >> one that does it device-based, and another one doing it subdev-based.
> >> >> 
> >> >> Ok, drivers can implementing locks to prevent such troubles, but,
> >> >> without the core providing a reliable mechanism, it is hard to
> >> >> implement a correct lock.
> >> >> 
> >> >> For example, let's suppose that some driver is using mt9m111 subdev
> >> >> (I just picked one random sensor that supports lots of MBUS formats).
> >> >> There's nothing there preventing a subdev call for it to change mbus
> >> >> format while streaming. Worse than that, the sensor driver has no way
> >> >> to block it, as it doesn't know that the bridge driver is streaming or
> >> >> not.
> >> >> 
> >> >> The code at subdev_do_ioctl() is just:
> >> >> 
> >> >> case VIDIOC_SUBDEV_S_FMT: {
> >> >> struct v4l2_subdev_format *format = arg;
> >> >> 
> >> >> if (format->which != V4L2_SUBDEV_FORMAT_TRY &&
> >> >> format->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> >> >> return -EINVAL;
> >> >> 
> >> >> if (format->pad >= sd->entity.num_pads)
> >> >> return -EINVAL;
> >> >> 
> >> >> return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh, format);
> >> >> }
> >> >> 
> >> >> So, mc core won't be preventing it.
> >> >> 
> >> >> So, I can't see how such subdev request would be implementing a logic
> >> >> to return -EBUSY on those cases.
> >> > 
> >> > Drivers can use the media_device graph_mutex to serialize format and
> >> > stream management calls. A finer grain locking mechanism implemented in
> >> > the core might be better, but we're not stuck without a solution at the
> >> > moment.
> > 
> > Am I missing something here? Isn't it as simple as remembering whether
> > the subdev is in streaming mode (s_stream(1) was called) or not? When
> > streaming any attempt to change the format should return an error (unless
> > the hardware can handle it, of course).
> > 
> > This is the same as for the 'regular' V4L2 API.
> 
> Not all subdevs implement s_stream, and I suspect that not all bridge
> drivers calls it. The random example I've looked didn't implement
> (mt9m111.c), but even some that implements it (like mt9m001.c) currently
> don't store the stream status or use it to prevent a format change.

Those drivers don't implement subdev pad-level operations, so they won't work 
with the media controller anyway. They will need to be fixed, and locking can 
then be implemented.

> At the moment we open the possibility to directly access the subdev,
> developers might think that all they need to use the new API is to enable
> the subdev to create subdev nodes (btw, the first mc patch series were
> enabling it by default). However, opening subdev access without address
> such issues will lead into a security breach, as buffer overflows will
> happen if hardware can't handle format changes in the middle of a
> streaming [1].
> 
> Also, a lock there will only work if properly implemented at the bridge
> driver, as a bridge driver that implement the media controller should
> implement something like the following sequence (at VIDIOC_REQBUFS):
> 
> 	lock_format_changes_at_subdev();			/* step 1 */
> 	get_subdev_formats();					/* step 2 */
> 	program_bridge_to follow_subdev_format_and_s_fmt();	/* step 3 */
> 	reserve_memory();					/* step 4 */
> 	start_streaming();					/* step 5 */
> 
> 
> In the above, s_stream should be called at the step 1, and not at step 5,
> as, otherwise, a race condition will happen, if a MBUS format change
> happens between step 1 and 5.

s_stream should be called at step 5, but media_entity_pipeline_start() should 
be called at step 1. Subdev drivers can then check the media_entity 
stream_count field (protected by the media_device graph_mutex lock) and refuse 
to change formats if the stream count is > 0. This is explained in 
Documentation/media-framework.txt, and the OMAP3 ISP driver implements this.

Sensor drivers will obviously need to be fixed, but that's not a real issue as 
they won't work with the OMAP3 ISP as-is anyway.

> [1] Btw, is there any hardware that supports a random change at the input
> format provided by a subdevice without any need of reconfiguring anything,
> and keeping providing the same output format? It seems doubtful for me, as
> hardware would need to have a format auto-detection logic, and some
> changes are impossible to track (for example, changing chroma order from
> YUYV to YVYU or from RGB to BGR can't be auto-detected). Perhaps the
> better would be to just block such changes while streaming.

I think it makes sense to just prevent those changes, especially as we have no 
real use case for such dynamic reconfiguration. Let's not try to over-engineer 
a solution that nobody will use.

-- 
Regards,

Laurent Pinchart