[00:38] *** agd5f has quit IRC (Ping timeout: 260 seconds)
[04:44] *** bparrot has quit IRC (Remote host closed the connection)
[07:39] <NiksDev>  test
[07:45] <hverkuil_> sailus: can you take a final look at Tomor's camss patch series?
[07:45] <hverkuil_> I'm OK with it (except for a few comments on comments :-) ), so once you're OK as well then I can make a pull request for it. Would be nice for 4.14.
[08:20] <pinchartl> hverkuil_: ping
[08:57] <sailus> hverkuil_: Something in particular I should take a look at?
[08:57] <sailus> I'm not sure I'll have time today but early next week should be possible.
[09:09] <sailus> neg: Hejssan!
[09:09] <neg> sailus: hej
[09:10] <sailus> Vilkommen tillbaka!
[09:10] <sailus> Hur mår du?
[09:11] <neg> Bara bra tack, I'm now playing the catch-up game :-) Hur mår du?
[09:14] <sailus> Bra, tack!
[09:16] <sailus> Jag ville diskutera om dina lappar som gäller med virtuella kanaler i csi-2.
[09:16] <sailus> När skulle det passa för dig?
[09:17] <neg> sailus: yes I suspected that :-) När som idag, ju tidigare destobättre. När passar det dig?
[09:19] <sailus> I tio minuter?
[09:19] <neg> sure
[09:34] <sailus> neg: Ping?
[09:34] <neg> pong
[09:35] <sailus> Let's start then.
[09:35] <sailus> I had a look at the patch series, and it left me wondering a bit.
[09:36] <sailus> I think it implements a sub-set of the user case and leaves some of the problems unaddressed.
[09:36] <sailus> Whereas these problems are addressed in my original patchset (which I don't see being used at all).
[09:37] <sailus> For instance, linking a multiplexed pad to a non-multiplexed pad isn't inherently wrong and must be supported.
[09:38] <sailus> Otherwise we're dividing all drivers using CSI-2 into two incompatible groups: those that use multiplexed pads and those that do not.
[09:38] <pinchartl> sailus: what's the use case for linking a multiplexed pad to a non-multiplexed pad ?
[09:38] <pinchartl> backward-compatibility ?
[09:39] <sailus> A sub-device driver that does not have a multiplexed pad paired with a CSI-2 receiver driver that does, for instance.
[09:39] <sailus> Even if you have CSI-2 bus, there is not necessarily any benefit from using a multiplexed source pad, in other words you'll only have one when it's useful.
[09:40] <sailus> A sole stream on a single virtual channel, for instance.
[09:40] <neg> Yes first of I messed up the subject of the patches it should have stated RFC like the cover letter do. And the focus of the patches where just to get something going, what exact patches of yours are you refering to?
[09:40] <pinchartl> if we were to implement everything from scratch today I'd make that sensor driver use a multiplexed source pad though
[09:41] <sailus> neg: Patches 4 and 5.
[09:41] <sailus> You should add also a type field and not use flags to tell the bus type in patch 3.
[09:41] <sailus> (Which is already a part of my original patchset.)
[09:45] <neg> sailus: I'm sorry I can't find a reference to your original patchset, when did you send it out?
[09:45] <sailus> I haven't sent it out since it's not finished.
[09:45] <sailus> The patches are here:
[09:46] <sailus> http://git.retiisi.org.uk/?p=~sailus/linux.git;a=shortlog;h=refs/heads/vc
[09:46] <sailus> I rebased them on mediatree master a few days ago.
[09:50] <neg> OK, my bad yes I should have used them as base for my patches
[09:53] <neg> About the flag in patch 3, I agree your solution is much better
[09:54] <neg> I'm sorry I did not base my patches ontop of yours :-)
[09:54] <sailus> Could you consider rebasing your patches on my vc branch?
[09:54] <sailus> No worries.
[09:54] <neg> will do
[09:55] <sailus> Streaming management isn't addressed in my patches.
[09:55] <sailus> I was wondering whether we should try changing the existing op.
[09:55] <sailus> Probably not.
[09:55] <sailus> So creating a new pad op is the way to go, as you've done.
[09:56] <neg> Yes but that will have the same issues and trying to link none multiplexed pads with multiplexed ones
[09:57] <neg> I think we either need to try and change the exisiting s_stream op, or make some helper in the frame work which calls video_ops.s_streme if pad_ops.s_stream was called but the remote subdevice don't implement it (and I don't really like that solution)
[09:59] <neg> But modifying my patches to allow a non-muliplexed pad to a multiplexed pad I think can be done and still be able to keep the in kernel format validation
[10:01] <sailus> s_stream() is used very widely.
[10:01] <sailus> It may be challenging to change the users without breaking anything.
[10:02] <sailus> Btw. I think we should probably have a bitmask there.
[10:02] <sailus> Otherwise we can't tell to upstream sub-devices which streams to start.
[10:03] <sailus> Or... what could be done is to start them individually, and let the upstream driver decide how to actually start them.
[10:03] <sailus> But in that case we have to way to tell streams should be started simultaneously in case the hardware supports both.
[10:05] <neg> you mean a bitmask to be able to start multiple streams at the sametime?
[10:05] <sailus> Yes.
[10:05] <neg> I thought the 'stream' member you add to v4l2_mbus_frame_desc_entry where to be used for this
[10:07] <sailus> Yes, that'd be the stream number.
[10:08] <sailus> But if you want to tell the sub-device to start streaming on multiple streams simultaneously, a single integer telling the stream number isn't enough.
[10:09] <neg> Ahh I see, I tought that if there where a dependecie two or more entry's would have the same stream number
[10:09] <sailus> No, the stream must be unique for a... stream.
[10:10] <sailus> I don't remember if I have it in the current patchset, but I thought of grouping the streams (stream group ID) to tell they're related, i.e. start and stop simultaneously.
[10:11] <neg> I see, then yes a bitmask would probobly be the neatest solution
[10:14] <neg> Nice, then I will rebase it on your branch and change the s_stream to take a bitmask and let's see how it grows from there
[10:15] <sailus> neg: Thanks.
[10:15] <sailus> Let me know if you have any issues.
[10:15] <neg> sailus: will do and thanks for your feedback
[10:15] <sailus> What I remember there *might* be issues in power management code.
[10:15] <sailus> On the other hand, we should probably move to runtime PM and let drivers decide themselves.
[10:16] <sailus> And ditch the current approach.
[10:16] <sailus> It's just very non-trivial to do.
[10:17] <sailus> I remember some Intel-submitted Omnivision sensor drivers use purely runtime PM.
[11:05] <pinchartl> neg: we'll likely need a bitmask given that the max9286 can't start/stop streams independently :-/
[11:20] <pinchartl> neg: what's your opinion about getting rid of subdev reprobe completely ?
[11:35] <neg> I feel it would be nice to get rid of it and I'm happy to update my patches to drop it, but at the same time it's a huge change how can we be sure we won't break anything?
[11:36] <pinchartl> I'd be happy to review and test :-)
[11:36] <pinchartl> what does it imply on the subdev side ?
[11:38] <neg> well with the reprobe as it is today, once a subdev have been part of a notifer its remove() and probe() will be called before it gets put back on the async waiting list
[11:39] <pinchartl> so we know it will start from a clean slate
[11:40] <neg> so removing that we remove the assurance that a subdevice state is prestine, so there might be 'bugs' from state beeing different after that is removed if for example the master video device changed the state of the subdevice somehow
[11:43] <neg> but IMHO, the subdev driers should be able to cope with this, but then again unbind/bind seems to have a few other issues already and the code path not excersiced a lot so I would not be suprised if other issues pop up if we remove the reprobe, but I still think it's the correct thing to do in the long run
[11:44] <pinchartl> the change isn't difficult, it's more the risk of breaking drivers, right ?
[11:44] <neg> yes
[11:44] <pinchartl> I think we need to do what we can to avoid breakages
[11:44] <pinchartl> which will involve some level of review for subdev drivers I believe
[11:44] <pinchartl> there might be breakages
[11:45] <pinchartl> but we should get rid of reprobind anyway
[11:45] <pinchartl> now or later, the risk of breakage will be the same
[11:45] <pinchartl> so I don't think it will help us to delay the change
[11:48] <neg> For reference Hans managed to find this http://lkml.iu.edu/hypermail/linux/kernel/1210.2/00713.html which talks a bit why this was added in the first place
[11:49] <pinchartl> A has
[11:49] <pinchartl> to go back into "deferred-probing" state. How do we do it?
[11:49] <pinchartl> -> through the unbound notifier
[11:50] <pinchartl> if you remove the subdev the unbound notifier should tell the master driver that the subdev is gone
[11:50] <pinchartl> but as far as I understand that's not what reprobing does
[11:50] <pinchartl> we reprobe subdevs, not master drivers
[11:53] <neg> yes, as the code in question is in v4l2_async_notifier_unregister(), all subdevices who are part of the notifier who is being unregisters 'done' list are reprobed
[11:53] <pinchartl> what could happen is this
[11:53] <pinchartl> the master could export resources used by the subdev
[11:53] <pinchartl> the omap3 isp driver, for instance, is a clock source
[11:54] <pinchartl> and the clock can be used by sensors
[11:54] <pinchartl> so if you remove the omap3 isp, the clock won't be there anymore
[11:54] <pinchartl> and that's bad for the subdev
[11:54] <pinchartl> but I think you should remove subdevs first
[11:55] <pinchartl> sailus: what's your opinion ?
[11:55] <pinchartl> this might be why reprobing is there
[11:56] <neg> is not the ideal solution for such subdevs to use the struct v4l2_subdev_internal_ops registered/unregistered to handle resources exported by the master?
[11:58] <pinchartl> the subdev doesn't know where the resource comes from
[11:58] <pinchartl> so subdevs would have to always register/unregister all their clocks in the registered/unregistered handlers
[11:58] <pinchartl> sorry, to acquire/release all their clocks
[11:58] <pinchartl> and what happens if one clock can't be acquired then ?
[11:59] <pinchartl> currently subdev probe is deferred when the clock can't be acquired
[11:59] <pinchartl> so there's indeed an issue if we want to get rid of the reprobal
[11:59] <pinchartl> sailus: ping
[11:59] <pinchartl> s/reprobal/reprobe/
[12:01] <neg> ahh I see your point
[12:03] <neg> pinchartl: about the max9286 and our specific need for a bitmask in s_stream, I don't think we need it (but I still think the argument should be a bitmask)
[12:04] <pinchartl> I'm not sure what we need exactly for the max9286. the fact that we can't start/stop streams independently is annoying
[12:04] <pinchartl> it certainly has to be taken into account in the API
[12:04] <pinchartl> synchronization of the cameras will likely need to be taken into account too
[12:04] *** Guest29997 is now known as mchehab
[12:07] <neg> I think that the max9286 who is the one with the knowledge about the dependecie should hide this. It should look at the MC graph and find out which sensors have enabled links and once s_stream is called setup the CSI-2 bus to transfer all those streams. And internaly keep track of invocations of start/stop and only act if the usage count go from 0->1 or 1->0
[12:08] <pinchartl> I think we need to experiment more with the hardware first, we're still not sure how it works exactly
[12:10] <neg> Yes, more testing is needed :-)
[13:08] <hverkuil_> pinchartl: kbingham: I think this series is superseded? https://www.spinics.net/lists/linux-renesas-soc/msg16251.html
[13:09] <pinchartl> hverkuil_: yes
[13:22] *** prabhakarlad has left 
[13:55] <sailus> neg, pinchartl: Thanks for finding the original reasoning.
[13:57] <sailus> I think it could be worth keeping because of this.
[13:57] <sailus> It'd be nice to add a comment documenting why it's there.
[13:59] <sailus> I wonder if there's a reason for constructing a list of all the devices first. The notifier isn't visible to the V4L2 async once it has been removed from the notifier list, so just re-probing all devices from the notifier done list should be enough.
[13:59] <pinchartl> sailus: it's still a hack though. I'd like to get rid of it
[13:59] <sailus> So getting that mutex is not needed either.
[13:59] <sailus> I don't really object removing it.
[14:00] <sailus> But how do we handle that situation otherwise?
[14:00] <sailus> For what it's worth, omap3isp has been broken regarding that all along and no-one ever complained.
[14:00] <sailus> It's unregistering the notifier before the clocks so that the sub-device driver immediately gets the clock that's just about to be removed anyway.
[14:01] <sailus> I'm ok with just saying that we don't support this.
[14:01] <pinchartl> I think we should force unregistration of subdevs before the master
[14:02] <sailus> Currently unregistering sub-devices before the master isn't supported at all.
[14:02] <sailus> One reason for that is that we can't safely remove media entities.
[14:05] <pinchartl> how does CCF handle removal of clock providers when clocks are still in use ?
[14:08] <sailus> There will be a big, hefty warning.
[14:08] <sailus> AFAIR WARN_ON.
[14:09] <mchehab> sailus: unregistering sub-devices were always supported on devnode-based drivers
[14:09] <mchehab> that's actually currently the only way to remove master node
[14:10] <mchehab> you need to first remove subdev drivers
[14:10] <sailus> mchehab: We can't safely remove media entities from the MC graph.
[14:10] <sailus> We need to address that first.
[14:10] <mchehab> yes, this should be addressed
[14:12] <mchehab> to be frank I don't mind myself if removing subdevs first could be racy
[14:12] <mchehab> the only usecase *I* have for it is to allow testing a new driver without rebooting
[14:13] <mchehab> I always stop apps before removing drivers
[14:13] <larsc> hotplug
[14:13] <mchehab> but unbind is another matter
[14:13] <mchehab> and hotplug
[14:14] <mchehab> larsc: for hotplug (and for KVM USB redirection to VM), unbind should work, but it would be OK to unbind master before subdevs
[14:14] <pinchartl> mchehab: module unloading is known to big buggy in Linux, it's race-prone, crash-prone, and there's no plan to fix that
[14:15] <pinchartl> unbinding devices from drivers, however, should be supported
[14:15] <mchehab> yes, unbinding should be supported
[14:15] <sailus> mchehab: You can unbind devices currently but the driver that created the Media device will go first.
[14:15] <sailus> That's how it's always been for MC-enabled drivers.
[14:16] <mchehab> while module unloading should *usually* work, even being buggy... as it makes a lot easier for developers to test stuff
[14:16] <sailus> I'm not saying this is my preference, just what's the current implementation.
[14:17] <mchehab> sailus: Aurora project was abandoned, but we still need to support subdev removal without removing MC
[14:18] <mchehab> because one could unbind just the ALSA, DVB or V4L2 part of a device that supports multiple APIs
[14:18] <pinchartl> Aurora ?
[14:18] <pinchartl> do you mean ara ?
[14:18] <sailus> mchehab: No arguments against that.
[14:19] <mchehab> pinchartl: sorry, I meant to say "ara"
[14:19] <mchehab> :-)
[14:19] <mchehab> you know a lot more about that than me... if I remember well, you were the one that came with project Ara's requirements to some of our summits ;-)
[14:21] <pinchartl> yes
[14:21] <pinchartl> there are rumours that facebook is now working on a similar project
[14:22] <mchehab> good
[14:22] <mchehab> IMHO, the concept is very nice
[16:34] <headless> hverkuil_: hi, are planning a "slow" IMR review as well? :-)
[17:25] <headless> +you
[17:25] <headless> prolly gone for real now :-)