On Mon, 2017-08-21 at 11:01 -0300, Mauro Carvalho Chehab wrote: > Em Mon, 21 Aug 2017 15:52:17 +0200 > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > > > On 08/21/2017 02:07 PM, Mauro Carvalho Chehab wrote: > > > Em Mon, 21 Aug 2017 12:14:18 +0200 > > > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > > > > > > > On 08/21/2017 11:08 AM, Mauro Carvalho Chehab wrote: > > > > > Em Mon, 21 Aug 2017 09:36:49 +0300 > > > > > Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> escreveu: > > > > > > > > > > > Hi Mauro, > > > > > > > > > > > > Mauro Carvalho Chehab wrote: > > > > > > > Hi Sakari, > > > > > > > > > > > > > > Em Wed, 16 Aug 2017 15:20:17 +0300 > > > > > > > Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> escreveu: > > > > > > > > > > > > > > > As we begin to add support for systems with Media > > > > > > > > controller pipelines > > > > > > > > controlled by more than one device driver, it is > > > > > > > > essential that we > > > > > > > > precisely define the responsibilities of each component > > > > > > > > in the stream > > > > > > > > control and common practices. > > > > > > > > > > > > > > > > Specifically, streaming control is done per sub-device > > > > > > > > and sub-device > > > > > > > > drivers themselves are responsible for streaming setup > > > > > > > > in upstream > > > > > > > > sub-devices. > > > > > > > > > > > > > > IMO, before this patch, we need something like this: > > > > > > > https://patchwork.linuxtv.org/patch/43325/ ; > > > > > > > > > > > > Thanks. I'll reply separately to that thread. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sakari Ailus > > > > > > > > <sakari.ailus@xxxxxxxxxxxxxxx> > > > > > > > > Acked-by: Niklas Söderlund > > > > > > > > <niklas.soderlund+renesas@xxxxxxxxxxxx> > > > > > > > > --- > > > > > > > > Documentation/media/kapi/v4l2-subdev.rst | 29 > > > > > > > > +++++++++++++++++++++++++++++ > > > > > > > > 1 file changed, 29 insertions(+) > > > > > > > > > > > > > > > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst > > > > > > > > b/Documentation/media/kapi/v4l2-subdev.rst > > > > > > > > index e1f0b72..45088ad 100644 > > > > > > > > --- a/Documentation/media/kapi/v4l2-subdev.rst > > > > > > > > +++ b/Documentation/media/kapi/v4l2-subdev.rst > > > > > > > > @@ -262,6 +262,35 @@ is called. After all subdevices > > > > > > > > have been located the .complete() callback is > > > > > > > > called. When a subdevice is removed from the system > > > > > > > > the .unbind() method is > > > > > > > > called. All three callbacks are optional. > > > > > > > > > > > > > > > > +Streaming control on Media controller enabled devices > > > > > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > > > + > > > > > > > > +Starting and stopping the stream are somewhat complex > > > > > > > > operations that > > > > > > > > +often require walking the media graph to enable > > > > > > > > streaming on > > > > > > > > +sub-devices which the pipeline consists of. This > > > > > > > > involves interaction > > > > > > > > +between multiple drivers, sometimes more than > > > > > > > > two. > > > > > > > > > > > > > > That's still not ok, as it creates a black hole for > > > > > > > devnode-based > > > > > > > devices. > > > > > > > > > > > > > > I would change it to something like: > > > > > > > > > > > > > > Streaming control > > > > > > > ^^^^^^^^^^^^^^^^^ > > > > > > > > > > > > > > Starting and stopping the stream are somewhat complex > > > > > > > operations that > > > > > > > often require to enable streaming on sub-devices which > > > > > > > the pipeline > > > > > > > consists of. This involves interaction between multiple > > > > > > > drivers, sometimes > > > > > > > more than two. > > > > > > > > > > > > > > The ``.s_stream()`` op in > > > > > > > :c:type:`v4l2_subdev_video_ops` is responsible > > > > > > > for starting and stopping the stream on the sub-device > > > > > > > it is called > > > > > > > on. > > > > > > > > > > > > > > Streaming control on devnode-centric devices > > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > On **devnode-centric** devices, the main driver is > > > > > > > responsible enable > > > > > > > stream all all sub-devices. On most cases, all the main > > > > > > > driver need > > > > > > > to do is to broadcast s_stream to all connected sub- > > > > > > > devices by calling > > > > > > > :c:func:`v4l2_device_call_all`, e. g.:: > > > > > > > > > > > > > > v4l2_device_call_all(&dev->v4l2_dev, 0, video, > > > > > > > s_stream, 1); > > > > > > > > > > > > Looks good to me. > > > > > > > > > > > > > > > > > > > > Streaming control on mc-centric devices > > > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > > > > > On **mc-centric** devices, it usually requires walking > > > > > > > the media graph > > > > > > > to enable streaming only at the sub-devices which the > > > > > > > pipeline consists > > > > > > > of. > > > > > > > > > > > > > > (place here the details for such scenario) > > > > > > > > > > > > This part requires a more detailed description of the > > > > > > problem area. What > > > > > > makes a difference here is that there's a pipeline this > > > > > > pipeline may be > > > > > > controlled more than one driver. (More elaborate discussion > > > > > > below.) > > > > > > > > > > > > > > > > > > > > > +The ``.s_stream()`` op in > > > > > > > > :c:type:`v4l2_subdev_video_ops` is responsible > > > > > > > > +for starting and stopping the stream on the sub-device > > > > > > > > it is called > > > > > > > > +on. A device driver is only responsible for calling > > > > > > > > the ``.s_stream()`` ops > > > > > > > > +of the adjacent sub-devices that are connected to its > > > > > > > > sink pads > > > > > > > > +through an enabled link. A driver may not call > > > > > > > > ``.s_stream()`` op > > > > > > > > +of any other sub-device further up in the pipeline, > > > > > > > > for instance. > > > > > > > > + > > > > > > > > +This means that a sub-device driver is thus in direct > > > > > > > > control of > > > > > > > > +whether the upstream sub-devices start (or stop) > > > > > > > > streaming before or > > > > > > > > +after the sub-device itself is set up for streaming. > > > > > > > > + > > > > > > > > +.. note:: > > > > > > > > + > > > > > > > > + As the ``.s_stream()`` callback is called > > > > > > > > recursively through the > > > > > > > > + sub-devices along the pipeline, it is important to > > > > > > > > keep the > > > > > > > > + recursion as short as possible. To this end, > > > > > > > > drivers are encouraged > > > > > > > > + to avoid recursively calling ``.s_stream()`` > > > > > > > > internally to reduce > > > > > > > > + stack usage. Instead, the ``.s_stream()`` op of the > > > > > > > > directly > > > > > > > > + connected sub-devices should come from the callback > > > > > > > > through which > > > > > > > > + the driver was first called. > > > > > > > > + > > > > > > > > > > > > > > That sounds too complex, and can lead into troubles, if > > > > > > > the same > > > > > > > sub-device driver is used on completely different > > > > > > > devices. > > > > > > > > > > > > > > IMHO, it should be up to the main driver to navigate at > > > > > > > the MC > > > > > > > pipeline and call s_stream(), and not to the sub- > > > > > > > drivers. > > > > > > > > > > > > I would agree with the above statement *if* we had no > > > > > > devices that > > > > > > require doing this in a different way. > > > > > > > > > > > > Consider the following case: > > > > > > > > > > > > sensor -> CSI-2 receiver -> ISP (DMA) > > > > > > subdev A -> subdev B -> video node > > > > > > > > > > Let me be clearer about the issue I see. > > > > > > > > > > In the above example, what subdevs are supposed to multicast > > > > > the > > > > > s_stream() to their neighbors, and how they will know that > > > > > they > > > > > need to multicast it. > > > > > > > > > > Let's say, that, in the first pipeline, it would be the > > > > > sensor > > > > > and subdev A. How "sensor" and "subdev A" will know that > > > > > they're > > > > > meant to broadcast s_stream(), and the other entities know > > > > > they > > > > > won't? > > > > > > > > So my understanding is that the bridge driver (ISP) will call > > > > s_stream > > > > for the CSI-2 receiver, and that in turn calls s_stream of the > > > > sensor. > > > > > > Alternatively, the ISP driver could call s_stream for both CSI-2 > > > and > > > sensor. > > > > > > > This should only be done for mc-centric devices, so we need a > > > > clear > > > > property telling a subdev whether it is part of an mc-centric > > > > pipeline > > > > or a devnode-centric pipeline. Since in the latter case it > > > > should not > > > > call s_stream in this way. For devnode-centric pipelines the > > > > bridge > > > > driver broadcasts s_stream to all subdevs. > > > > > > It would be easier to have a logic called by the ISP driver that > > > would > > > get all elements from an active pipeline and call s_stream() > > > there. > > > > > > That would keep the flexibility that would be needed by devices > > > with a > > > separate CSI-2 receiver, while preventing the addition of an > > > extra > > > logic on every sub-device to teach them that s_stream() should be > > > called to communicate with some specific sub-device. > > > > > > The thing is that, if you have a pipeline like: > > > > > > source subdev -> subdev A -> subdev B -> subdev C -> subdev D -> > > > DMA > > > > > > due to PM constraints, all subdevs on such pipeline may require > > > s_stream() > > > to control its power consumption. So, it is not just a CSI-2 > > > device that > > > would need to enable power at the sensor, but an entire pipeline > > > that > > > would need to receive s_stream() calls. > > > > So? If DMA calls s_stream for subdev D, which calls s_stream for > > subdev C, etc. > > until the source subdev's s_stream is called, then they are all > > powered up, > > aren't they? > > > > I don't see the difference between this and the DMA calling > > s_stream for all > > subdevs. > > > > One clear difference is (as Sakari mentioned) stack usage. There > > having the > > DMA call s_stream is clearly better. See below for more comments... > > That's not the only difference. If we pass this to subdevs, they > need to know that they're working on a mc-centric way. > > There is another problem, with is more serious, and it is somewhat > related to stack usage: what happens if there's a loop inside a > pipeline? That would cause a Kernel crash. I can't see any easy > way to avoid that (nor stack starving, if the pipeline is too big). > > > > > > IMO, the best would be a logic similar to > > > media_entity_pipeline_start() that, > > > for each entity at the pipeline, s_stream() would be called, e. > > > g. something > > > like: > > > > > > v4l2_subdev_pipeline_call() > > > > > > > For the record, I am not aware of any subdevs that are used by > > > > both > > > > mc and devnode-centric scenarios AND that can sit in the middle > > > > of a > > > > pipeline. Sensors/video receiver subdevs can certainly be used > > > > in both > > > > scenarios, but they don't have to propagate a s_stream call. > > > > > > em28xx has a bunch of sensors that are also used on embedded > > > drivers. > > > It also has a tvp5150, with is also used by omap3. > > > > > > On the only OMAP3 board whose has a DT upstream, the tvp5150 has > > > both source > > > pads connected to S-Video connectors, but nothing prevents that > > > someone > > > would add a tuner before it. I remember someone mentioned that > > > such device > > > exists (although its DT is not upstream). > > > > > > On an embedded device with both tvp5150 and a tuner, s_stream() > > > should be > > > called by both. > > > > > > > > > > > It would be very helpful if we have a good description of these > > > > two > > > > scenarios in our documentation, and a capability indicating mc- > > > > centric > > > > behavior for devnodes. And also for v4l2-subdevs internally > > > > (i.e. > > > > am I used in a mc-centric scenario or not?). > > > > > > > > Then this documentation will start to make more sense as well. > > > > > > > > > Also, the same sensor may be used on a device whose CSI-2 is > > > > > integrated at the ISP driver (the main driver). That's why > > > > > I think that such logic should be started by the main driver, > > > > > as > > > > > it is the only part of the pipeline that it is aware about > > > > > what it is needed. Also, as the DMA engines are controlled by > > > > > the main driver (via its associated video devnodes), it is > > > > > the only > > > > > part of the pipeline that knows when a stream starts. > > > > > > > > Yes, and this driver is the one that calls s_stream on the > > > > adjacent subdevs. But just those and not all. > > > > > > > > > > > > > > > Assume that the CSI-2 receiver requires hardware setup both > > > > > > *before and > > > > > > after* streaming has been enabled on the sensor. > > > > > > > > > > calling s_stream() before and after seems to be an abuse of > > > > > it. > > > > > > > > I think you misunderstand what Sakari tries to say. > > > > > > > > In the scenario above the bridge driver calls s_stream for the > > > > CSI receiver. That in turn has code like this: > > > > > > > > s_stream(bool enable) > > > > { > > > > ... initialize CSI ... > > > > if error initializing CSI > > > > return error > > > > call s_stream for adjacent source subdev (i.e. sensor) > > > > if success > > > > return 0 > > > > ... de-initialize CSI > > > > return error > > > > } > > > > > > > > This makes a lot of sense for mc-centric devices and is also > > > > much more > > > > precise than the broadcast that a devnode-centric device would > > > > do. > > > > > > > > In the very unlikely case that this CSI subdev would also be > > > > used in > > > > a devnode-centric scenario the s_stream implementation would > > > > just > > > > return 0 after it initializes the CSI hardware. It will depend > > > > on > > > > the hardware whether that works or not. > > > > > > > > subdevs used in devnode-centric scenarios tend to be pretty > > > > simple > > > > and are able to handle this. > > > > > > If I understand well, you're basically concerned about error > > > handling, right? > > > > > > That could easily be handled with something like: > > > > > > ret = v4l2_subdev_pipeline_call(source_entity, video, s_stream, > > > 1); > > > if (ret < 0) { > > > v4l2_subdev_pipeline_call(source_entity, video, > > > s_stream, 0); > > > return ret; > > > } > > > > That's not it. It is that some subdevs need to execute > > initialization code > > first before you can call s_stream on the sensor. > > > > If you want to use a pipeline_call you would need to introduce > > stream_prepare > > and stream_unprepare ops to achieve the same (idea stolen from the > > clk API). > > > > I'm not sure if this would fully resolve Sakari's issue. > > If the problem is with initialization, all we need to enforce is > that the graph traversal will happen at the reverse order, e. g. > from subdev D to subdev A. > > But yeah, if it is more complex, we may need a stream_prepare ops. Have we just come full circle [1]? [1] https://patchwork.linuxtv.org/patch/37321/ I'm not convinced anymore that this would be enough for all possible use cases, though. What if there are multiple chained CSI-2 links links in the pipeline, like a CSI-2 merge element between cameras and receiver that need to be perpared and started one link after the other? regards Philipp
