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.