Moi, On Fri, Sep 30, 2022 at 03:10:16PM +0300, Tomi Valkeinen wrote: > Hei, > > On 30/09/2022 14:21, Sakari Ailus wrote: > > Moi, > > > > On Wed, Sep 28, 2022 at 10:54:44AM +0300, Tomi Valkeinen wrote: > > > Hi, > > > > > > On 28/09/2022 00:13, Sakari Ailus wrote: > > > > Moi, > > > > > > > > On Tue, Sep 27, 2022 at 03:33:15PM +0300, Tomi Valkeinen wrote: > > > > > On 27/09/2022 13:23, Sakari Ailus wrote: > > > > > > > > > > <snip> > > > > > > > > > > > > > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This > > > > > > > > > +means that the userspace mut reconfigure all streams after calling the ioctl > > > > > > > > > +with e.g. ``VIDIOC_SUBDEV_S_FMT``. > > > > > > > > > > > > > > > > How about this: > > > > > > > > > > > > > > > > Calling ``VIDIOC_SUBDEV_S_ROUTING`` will cause the selections and subdev > > > > > > > > formats being propagated from the sink pads towards the sources. > > > > > > > > > > > > > > Hmm, but that's not true. The selections and formats will be zeroed, unless > > > > > > > the driver initializes them to a value. There's no propagation done. > > > > > > > > > > > > They need to be propagated. The driver is responsible for maintaining a > > > > > > valid configuration for the processing steps in a sub-device, and with > > > > > > routes that must apply to routes as well. > > > > > > > > > > Hmm, no, they don't need to be propagated. The driver needs to initialize > > > > > the formats and selections to valid configuration, that is true, but it > > > > > doesn't mean the driver propagates settings from the sink pads to the source > > > > > pads. In theory the formats on sink and source sides could be different. > > > > > > > > After propagation, the user may set the format (or selection) later on in > > > > the processing steps. The propagation is required by the spec and I don't > > > > see why it would be different for drivers with support for streams. Of > > > > course this needs to take place taking hardware limitations into account. > > > > > > I don't disagree with the above, but I still don't see why it matters here. > > > > It does. The user needs to be able to rely on the ability of the driver to > > maintain valid internal configuration. That user generally has less > > information on this than the driver. > > I had a short chat with Sakari, and it seems there's no real issue here, > mostly just a misunderstanding. > > The action point is to clarify what "reset" means (it means resetting to > driver default values, keeping the subdev configuration valid). And perhaps > also highlighting somewhere that when e.g. setting the formats, propagation > happens with subdevices using routes too. Thanks. Sometimes e-mail just isn't enough. :-) ... > > > If we have a CSI-2 receiver that has a hardcoded handling of the VC & DT, > > > how does the userspace configure the routes? The userspace doesn't see the > > > VCs or DTs. We could have static routes defined in the receiver subdevice, > > > but does that help? > > > > Good point. I think not. > > > > I guess we would then leave the routes for the user to create and driver to > > try to configure the hardware accordingly or fail in link validation? > > > > Perhaps we won't need a static route flag then after all. > > > > > > > > The HW I use, TI's CAL, has the means to configure VC/DT freely. But it has > > > 8 DMA engines, and, of course, each stream has to go to a single DMA engine. > > > So I think we could say that it has 8 static streams, and the user can only > > > enable or disable them. But I'm not sure how much adding a new flag for this > > > helps. > > > > Could this be limited by only allowing to create eight routes? > > Yes, and the driver should do that. But the driver should also verify the > routing, so that each DMA engine only gets one route. > > So here it might be possible to use a STATIC flag for the routes. Afaics, > the only benefit of that would be to give a hint to the userspace about the > possible routes. It's difficult to say if it's worth the trouble. I'm fine with leaving out the flag. It seems it wouldn't have been as useful in practice as I originally thought. > > > > > Using one flag for two different purposes may thus prove problematic over > > > > time. I'd thus define another for the other purpose. In the worst case it > > > > won't be needed and we can make it obsolete later on. > > > > > > I'd like to have a clear example of a setup where we need this flag and > > > benefit from it before adding it. > > > > > > In the CAL case I don't see much benefit. I think the only thing it gives us > > > is a minimal discovery mechanism for the userspace to understand how CAL > > > routes can be configured. I say minimal, as it still won't cover it fully as > > > the validity of the routing depends on the actual VCs and DTs too (which > > > will be found out only at the stream start time). > > > > > > And this would only give us discovery for the receiver and wouldn't help > > > with the bridges. > > > > > > > > > > > > > > > > > But as I said above, I haven't figured out a use for this. > > > > > > > > > > > > > > > > + - 1 > > > > > > > > > + - The route is immutable. Set by the driver. > > > > > > > > > + * - V4L2_SUBDEV_ROUTE_FL_SOURCE > > > > > > > > > + - 2 > > > > > > > > > + - The route is a source route, and the ``sink_pad`` and ``sink_stream`` > > > > > > > > > + fields are unused. Set by the driver. > > > > > > > > > + > > > > > > > > > +Return Value > > > > > > > > > +============ > > > > > > > > > + > > > > > > > > > +On success 0 is returned, on error -1 and the ``errno`` variable is set > > > > > > > > > +appropriately. The generic error codes are described at the > > > > > > > > > +:ref:`Generic Error Codes <gen-errors>` chapter. > > > > > > > > > + > > > > > > > > > +ENOSPC > > > > > > > > > + The number of provided route entries is less than the available ones. > > > > > > > > > > > > > > > > What does "available ones" mean in this context? More than is supported? > > > > > > > > Wouldn't E2BIG be the appropriate code in that case? > > > > > > > > > > > > > > Good question. I don't think I wrote this part =). ENOSPC refers to the case > > > > > > > where VIDIOC_SUBDEV_G_ROUTING is called without enough space for the routing > > > > > > > table. So "available ones" mean the routes in the subdev's routing table, > > > > > > > and "provided route entries" refers to the userspace target routing table. > > > > > > > > > > > > > > It sounds pretty odd, and obviously needs a clarification. > > > > > > > > > > > > I think I actually can think what this did mean. It means that the > > > > > > num_routes is smaller than the number of routes in a routing table when > > > > > > G_ROUTING is called. For that I think ENOSPC is the right code actually. > > > > > > > > > > > > But also I think we need to document what happens when there are too many > > > > > > routes. For that E2BIG would be appropriate. > > > > > > > > > > v4l2-ioctl.c returns EINVAL if there are over 256 routes. The drivers > > > > > should, of course, do additional check if needed. In v4l2-ioctl.c it seems > > > > > common to return EINVAL if there's too much data, but we can of course > > > > > define E2BIG for routing ioctls. > > > > > > > > The number (256) is just the current limit. I don't expect more though. > > > > > > > > But the user space could know the number is too large if we have a proper > > > > error code for it. Up to you. However at least documentation needs to be > > > > amended since this case remains undocumented. > > > > > > I can change the returned error from EINVAL to E2BIG and document it. But > > > everything else in check_array_args return EINVAL, so it would be going into > > > different direction. > > > > Could this be beneficial in telling the user too many routes have been > > configured (as I wrote above)? > > Yes, I think the driver should return E2BIG if there are too many routes. > > But my question is, should v4l2-ioctl.c return E2BIG for > 256 routes? > That's not how it works for all the other ioctls there, they return EINVAL. > I don't mind changing that to E2BIG, but usually it's nice if the code is > consistent. >From the user's point of view the "too many routes" condition is the same independently of whether the information comes from the framework or the driver. So I think using -E2BIG in the framework is the right thing to do. -- Terveisin, Sakari Ailus
