On Mon, May 08, 2023 at 03:24:10PM +0300, Sakari Ailus wrote: > Moi, こんにちは > On Mon, May 08, 2023 at 01:14:07PM +0300, Tomi Valkeinen wrote: > > On 06/05/2023 00:52, Sakari Ailus wrote: > > > Take the new INTERNAL_SOURCE pad flag into account in validating routing > > > IOCTL argument. Effectively this is a SINK pad in this respect. Due to the > > > union there's no need to check for the particular name. > > > > What union? The one you add in the next patch? > > Oops. I think we can reorder the patches. > > > As a concept the internal source pads sound good, and they work in practice > > in my tests. But this union is what grates me a bit. We have a flag, > > MEDIA_PAD_FL_INTERNAL_SOURCE, which tells which field of the union to use, > > and then we go and do not use the new union field. Well, and also the > > naming, as we normally have source and sink pads, but now we also have > > internal source pads, which are actually identical to sink pads... > > The union still should be used by the user space. We're testing flags here > and those flags are the same independently of the INTERNAL_SOURCE flag. > > I'm fine by not adding that union though, but for the user space I think > it's better we have it: explaining that the sink_pad has a different > meaning if that flag is set is harder than making it a union member. > > > I understand the idea and reasoning, but the two points above do confuse me > > and I'm sure would confuse others also. > > > > I wonder if it would be less or more confusing to simplify this by just > > adding a MEDIA_PAD_FL_INTERNAL, which could be applied to either a source or > > a sink pad, and would prevent linking to it. The flag would indicate that > > the stream from/to that pad is generated/consumed internally. (I don't know > > when you would need an internal pad to consume data, but... who knows, the > > need might pop up =). > > This is another option. But I envision there will be more compatibility > issues. Although... generally using such hardware requires knowledge of the > device, and we haven't obviously had any support for devices needing this > functionality in the tree. So in the end it might not matter much. > > > That would mean that an "internal sink pad" would generate a data stream, > > i.e. it's a "source", but I think a normal sink pad is almost the same > > anyway: when considering entity's internal routing, the normal sink pad is a > > "source", and the same logic would apply with the internal pads too. > > > > An internal sink pad that generates a stream is a bit more confusing than an > > internal source pad, but I think that confusion is less than the rest of the > > confusion in the code-side that comes with the internal source pads. > > I prefer having the possible sources of the confusion in the framework than > in the drivers. Therefore I think INTERNAL_SOURCE flag is a (slightly) > better option. > > I wonder what Llaurent thinks. I like the idea of a MEDIA_PAD_FL_INTERNAL flag. That's actually how I read patch 1/7, I didn't notice it used MEDIA_PAD_FL_INTERNAL*_SOURCE* :-) We could define MEDIA_PAD_FL_INTERNAL_SOURCE #define MEDIA_PAD_FL_INTERNAL_SOURCE (MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL) but I'm not sure it would be less confusing. Regarding isolating the sources of confusion in the framework rather than spreading them through drivers, I can't disagree, but I think that, for raw camera sensors at least, the best option would be to create a new camera sensor object with a much more tailored API than v4l2_subdev (and of course wrapping that new object in a v4l2_subdev in the framework). This won't address the other types of drivers, but I'm not sure we'll get any in the foreseable future. -- Regards, Laurent Pinchart
