hverkuil: ping
pinchartl: pong
I'd like to discuss video device registration for the VIN driver with you and Niklas at some point. when would you be available ?
I'll get back to you once I found the bug that I'm hunting down :-)
no worries :-)
pinchartl: my preference would be to discuss this once the gen3 vin series is merged (except for that patch). As far as I am aware Niklas has all the acks and just needs to post a final version (probably best to do that once 4.15-rc1 is merged back in our master) and I'll make a pull request.
I will need to dig into the details and think about it before we discuss it.
Unless there is a reason this is really urgent?
hverkuil: it's an early patch in the series, it complicates the implementation to move it out
and I don't think it would be a good idea to do so
registering the video devices at probe time is much cleaner
so I'd like to agree on how to proceed before asking Niklas to modify the series again
I am very strongly opposed to registering video devices that can't be used until some unspecified time in the future. That breaks what applications can expect. This ties in with the 'partial configuration' issue where some subdevs are missing but you still want to continue.
So to support something like this requires API additions/modifications.
I feel this should be discussed separately.
I also have the feeling that what happens in the vin driver should be done differently, but I need to sit down for that and put my thinking cap on.
Is it really that much work to move the patch to the end?
hverkuil: most drivers are currently broken already, in the sense that they register video devices before completing full initialization and create race conditions with userspace
with registration at probe time we're forced to handle that correctly, and it becomes much easier to test it with compliance tools
also, I don't agree that we'll break userspace applications by doing so
Actually, most are OK: we had breakage reports in the past with udev opening video devices early on and those have been fixed. I'm sure there are some that still have race conditions, but that doesn't mean we shouldn't do this right for new drivers.
the truth is that we have never fully specified all this, and it's now hitting us back
registration of video devices likely needs to be reworked completely, but that's out of scope for this series
in particular we need to split init and registration
and don't get me started about subdev nodes registration :-)
it's really awful too
hverkuil: one of the problems with moving the vdev registration to the complete() handler I see is that it can't be unregisterd until remove() time. So a flag is needed to only register the vdev the first time the complete() callback is called. So the driver and users still need to be able to handle a vdev which is not complete/usable as one of the subdevices might be unbound at anytime
and that's a really dirty hack too
I disagree about the 'hitting us back' part. This specific issue is because someone (as I understand it) wants to replace the i2c bus from underneath the device. That's hardly standard behavior.
I agree that this needs to be looked at, but not as part of a much larger patch series.
IMHO there is much more involved here and I think the vin implementation is really a hack to workaround this.
I've never liked that patch from the very beginning.
I'm not saying we shouldn't discuss it, but split it off from the rest of the series, get that in first, then we look at this.
replacing the I2C bus ?
why is this a hack ?
pinchartl: this is blocking Wolframs i2c work
"Someone is working on runtime switching of i2c IP core (for example switching to/from i2c-gpio) and that work causes all i2c devices on the bus to removed and then re-added causing the rcar-vin to unbind/re-bind its subdevices and if the vdev is handled in the notifier callback it oopses as the kref can't be initilized twice"
ah that
I can't argue that it's not a hack indeed
but in my opinion that's a separate issue
unbinding/rebinding is not only for I2C switching
and creating video device nodes at probe time is in my opinion the right thing to do regardless of that
Well, I disagree :-) And that's why this patch shouldn't be part of this series: it should be dealt with separately.
but the question still stands, how do we come to an agreement ?
and if we move it out for now
That way it doesn't block the merging of the other 24 patches.
how do we handle unbind/rebind ?
(regardless of I2C controller switching)
The short answer is, I don't know. I need to look at that carefully and for that I need to set time aside.
I would really like to consider this together with the 'partial configuration' issue.
hverkuil: In a wider context than with just rcar-vin --- video devices are not related to a particular sensor, ISPs can be used from memory from memory and with other sensors.
Registering the video devices at probe time is simply a matter of reliability: if one I²C device fails, there's no reason why the entire camera system should remain unusable.
It will actually help if rcar-vin is merged before discussing this, since it makes for a nice use-case.
That's there with omap3isp, too.
even without talking about hardware failure, you might not want a given driver in your video pipeline because you don't want to use its features
Just with fewer cameras.
so that wouldn't be compiled in, at all
Mind that there never was a moment at which all related devices would be exposed to the user: they are all registered one by one, and this is visible to the user. The time window in which this happens is typically smaller, that's for sure.
I keep saying: right now applications expect that when a video device appears it is ready for use. That's a fact. If we want to support HW failures for some components (and we want to support that), then that implies some sort of userspace change and that takes much more time to figure out and agree on.
hverkuil: what applications are you concerned about that would use the rcar-vin driver ?
and Sakari's point is that it's already broken, with a smaller race window of course
This really is something you need to think about. Why the haste? To do this right you need an RFC.
Perhaps one additional note to this is that in most cases there's no practical difference: drivers are loaded during system bootup in a small time window and the device nodes are created at that time as well.
The difference is seen when something goes wrong.
neg: is it really that much work to move the patch to the end of the series? I find that hard to believe.
hverkuil: I don't think is that much work, I moved it before back and forth between complete() and probe(). It will have effect on some of the later patches in the series which might need to be reviewed again. From my side I'm mostly concernd about that all reviewers are happy where it's moved to as if possible I would only like to move it one more time before VIN Gen3 could be picked-up
hverkuil: if the agrement comes to that the patch should be moved out of the rcar-vin series, is your prefered soultion to register the vdev(s) at complete time and unregister in unbind() or register the first time complete() is called and unregister at remove() time?
neg: the latter is a very dirty hack and the former will break unbind/rebind
I'd say register at complete time and unregister in unbind.
If the patch is moved to the end, then I am happy to merge all but that last patch.
hverkuil: but then unbind/rebind is broken
pinchartl: yes. And it should be fixed. I am just not convinced that the approach taken here is the right one.
pinchartl: yes, I would prefere to solve it without a hack. Just asking to understand what the different options are to be able to move forward
The simple fact is that when you start doing unbind/bind all hell breaks loose. For pretty much all drivers. This should be solved properly, and not by applying hacks to random drivers.
hverkuil: it's not a hack, it's the proper fix :-)
and all drivers will likely need to be touched
we have pretty much ignored unbind/rebind until now
I'm not convinced at all that it is the proper fix.
the V4L2 core is a mess when it comes to lifetime management of objects
and that will require changes to drivers
pinchartl: Supporting unbinding requires implementing reference counting for sub-devices, too.
sailus: yes, and that's completely broken when it comes to subdev nodes
it would be hard to fix
neg: let's make it easy for you: I'm not going to merge that patch. So if you want to get the rcar-vin patches merged, then it has to be moved to the end (or dropped completely).
This needs more thought, so let's not mix this in with all the other 24 patches.
hverkuil: and you will merge the series with a known issue caused by unbind/rebind ?
yes. It's bug-compatible with all the other v4l2 drivers. Almost all will break when you start messing around with that.
This needs a proper fix, not random band-aids.
and it definitely should not block merging this series.
hverkuil: understood and no problem :-) My goal is that all reviewers agree with the direction of the series, not to try sneak this change in.
hverkuil: I'm all for a proper fix, but given that this is the proper fix in my opinion, I'll need you to tell us what your idea of a proper fix is
pinchartl: as I've said multiple times before: I don't know. I need to sit down and spend time on this. I am 90% certain though that this *isn't* the right fix.
hverkuil: I'll give you a chair to sit down then :) when would you like to revisit the topic ?
pinchartl: I honestly don't know. My preference would be to wait until this series is merged, then set aside half a day or a full day for me to dig into this mess and write up some initial RFC.
Are there urgent reasons for doing this earlier?
(I'm about to leave for home)
Dropping the patch would allow for Gen3 support to hopefully be picked-up at the cost of making the driver a bit more complex and some patches might needed to be reviewed again. The end result would be that the driver would still have issues with unbind/rebind. I'm not happy about that but I would be OK with that as we do not add any new problems.
it's next on our todo list. I'm fine giving you time to think about it, but then I'd like to schedule a discussion
The question for me is than if you both are OK with dropping the patch from the series. Register the vdev's in the complete() callback and unregestering them in the unbind() callback. This would leve the driver in the state it is in now, where unbind/re-bind is broken. Or if the agreement is that I should stop posting the series until there is fix in the framework for the issue (I would not like this option, but
I would rather know if that's the case).
neg: I truly think that waiting for a fix in the framework could take way too long. To do unbind/bind you have to be root so this is not something the average user can do. It's broken for many if not all v4l2 drivers anyway, so this is not a blocker IMHO.
Realistically I'm pretty sure there won't be a proper fix this year.
got to go, ttyl
hverkuil: thanks for taking the time to discuss this, have a nice evening
hverkuil: again if you think this isn't the proper fix, then please have a look at it yourself at some point and propose another solution
pinchartl: "registration of video devices likely needs to be reworked completely, but that's out of scope for this series, in particular we need to split init and registration"
I agree with that.
It's the recommended way of doing things and we should do the same.
If you are looking for a place to start, then that's something that you can start working on.
hverkuil: I've set up a rpi with two cec-gpio connected, works great, thanks.
I just noticed that cec-ctl man page does not list --user-control-pressed et al
not sure it really matters, I found it with --help-remote-control-passthrough