Em Mon, 2 Jul 2018 12:38:23 +0200 Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > On 02/07/18 11:41, Mauro Carvalho Chehab wrote: > > Em Mon, 2 Jul 2018 10:18:37 +0200 > > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu: > > > >> While working on v4l2-compliance I noticed that entity to interface links > >> have just the MEDIA_LNK_FL_ENABLED flag set. > >> > >> Shouldn't we also set the MEDIA_LNK_FL_IMMUTABLE? After all, you cannot change > >> an entity-interface link. It feels inconsistent not to have this flag. > > > > It could make sense for non-hybrid devices, but this may not be true > > for hybrid ones. See below. > > > >> I also propose that media_create_intf_link() drops the last flags argument: > >> it can set the link flags directly since they are always the same anyway. > > > > When we came with this design, the idea is that an interface can be > > disabled/enabled at runtime, if the entity it links can't be used, > > because the hardware is busy doing something else. > > > > That could happen with hybrid devices, where the analog part could > > be consuming resources that would be needed for the digital part. > > > > Disabling the link at runtime has an advantage that it makes easier > > to check - as open() syscalls could just use it to return -EBUSY, > > instead of doing a complete graph analysis. Also, applications can > > test it directly, in order to have a hint if a device is ready for > > usage. > > > > That was one of the approaches we considered at the design, but I > > don't remember if Shuah's patch series actually used it or not, > > as I don't look at her pending patches for a long time. I suspect > > she took a different approach. > > > > Anyway, before touching it, I'd like to see her patches merged, > > and do some tests with real case scenarios before changing it. > > Hmm, this also highlights another deficiency in the spec. Currently > the description of IMMUTABLE says: > > "The link enabled state can’t be modified at runtime. An immutable link > is always enabled." > > But in the plan above the link can change, but only the driver can > enable/disable the link. It makes no sense AFAICS for userspace to > enable/disable an interface link. > > If I am right that it makes no sense for userspace to disable an interface > link, then we should update the spec to clarify that this is not allowed > (in fact, it is not possible today anyway). And we should also clarify > that the driver can disable an interface link and what that means. > > If userspace CAN in some circumstances disable an interface link, then > we need to add something like a READ_ONLY flag to signal whether or not > userspace can change an interface link. If it is READ_ONLY, then only the > driver can change that. Yes, this makes sense. > > That said, given that today there are no drivers that disable an interface > link, I still think that we should mark them all as IMMUTABLE. That flag > can be removed when we actually let drivers change this. Ok, let's do this, but without touching at the media_create_intf_link(), as we need first to apply Shuah's patch, and see how we'll handle, before start stripping code that will probably be needed. > > It would be consistent with the current usage of interface links. > > Regards, > > Hans Thanks, Mauro
