Re: [PATCH 00/11] media: rcar-vin: fix OPS and format/pad index issues

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Hans,

On 2017-02-13 21:57:48 +0100, Hans Verkuil wrote:
> On 02/13/2017 06:47 PM, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > Thanks for your feedback.
> > 
> > On 2017-02-13 15:19:13 +0100, Hans Verkuil wrote:
> >> Hi Niklas,
> >>
> >> One general remark: in many commit logs you mistype 'subdeivce'. Can you
> >> fix that for the v2?
> > 
> > Will fix for v2.
> > 
> >>
> >> On 01/31/2017 04:40 PM, Niklas Söderlund wrote:
> >>> Hi,
> >>>
> >>> This series address issues with the R-Car Gen2 VIN driver. The most 
> >>> serious issue is the OPS when unbind and rebinding the i2c driver for 
> >>> the video source subdevice which have popped up as a blocker for other 
> >>> work.
> >>>
> >>> This series is broken out of my much larger R-Car Gen3 enablement series 
> >>> '[PATCHv2 00/32] rcar-vin: Add Gen3 with media controller support'. I 
> >>> plan to remove that series form patchwork and focus on these fixes first 
> >>> as they are blocking other development. Once the blocking issues are 
> >>> removed I will rebase and repost the larger Gen3 series.
> >>>
> >>> Patch 1-4 fix simple problems found while testing
> >>>     1-2 Fix format problems when the format is (re)set.
> >>>     3   Fix media pad errors
> >>>     4   Fix standard enumeration problem
> >>>
> >>> Patch 5 adds a wrapper function to retrieve the active video source 
> >>> subdevice. This is strictly not needed on Gen2 which only have one 
> >>> possible video source per VIN instance (This will change on Gen3). But 
> >>> patch 6-8,11 which fixes real issues on Gen2 make use of this wrapper, as 
> >>> not risk breaking things by removing this wrapper in this series and 
> >>> then readding it in a later Gen3 series I have chosen to keep the patch.  
> >>> Please let me know if I should drop it and rewrite patch 6-11 (if 
> >>> possible I would like to avoid that).
> >>>
> >>> Patch 6-8 deals with video source subdevice pad index handling by moving 
> >>> the information from struct rvin_dev to struct rvin_graph_entity and 
> >>> moving the pad index probing to the struct v4l2_async_notifier complete 
> >>> callback. This is needed to allow the bind/unbind fix in patch 10-11.
> >>>
> >>> Patch 9 use the pad information when calling enum_mbus_code.
> >>>
> >>> Patch 10-11 fix a OPS when unbinding/binding the video source subdevice.
> >>
> >> This will not help: you can unbind a subdev at any time, including when
> >> it is in use.
> > 
> > Yes, this series will not help remedy the problem if a device is in use.  
> > But I still find it useful since it unblocks other work, solves a OPS 
> > and if there are no current users the driver can supports unbind/bind of 
> > its subdevices, it's not perfect but it do make things a little better 
> > IMHO.
> > 
> > If it where not for me wishing to reuse the behavior introduced in patch 
> > 10-11 on R-Car Gen3 when a subdevice could not available for for other 
> > reasons than it's not bound (see bellow) I would be happy to drop the 
> > patches from this series.
> > 
> >>
> >> But why do you need this at all? You can also set suppress_bind_attrs in
> >> the adv7180 driver to prevent the bind/unbind files from appearing.
> > 
> > The primary use-case for this is on R-Car Gen3 where there are a limited 
> > number of possible routing options to connect CSI-2 devices to VIN 
> > devices (set in hardware), see table bellow. The routing possibilities 
> > are set per VIN group (VIN0-3 and VIN4-7) and not individually for each 
> > VIN device. Given this limitation some routing options will result in an 
> > N/A video source for one or more VIN devices in a VIN "group".
> > 
> >    - VIN0-3 controlled by chsel register in VIN0
> >    chsel    VIN0        VIN1        VIN2        VIN3
> >    0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
> >    1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
> >    2        N/A         CSI40/VC0   CSI20/VC0   N/A
> >    3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
> >    4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3
> > 
> >    - VIN4-7 controlled by chsel register in VIN4
> >    chsel    VIN4        VIN5        VIN6        VIN7
> >    0        CSI40/VC0   CSI20/VC0   N/A         CSI40/VC1
> >    1        CSI20/VC0   N/A         CSI40/VC0   CSI20/VC1
> >    2        N/A         CSI40/VC0   CSI20/VC0   N/A
> >    3        CSI40/VC0   CSI40/VC1   CSI40/VC2   CSI40/VC3
> >    4        CSI20/VC0   CSI20/VC1   CSI20/VC2   CSI20/VC3
> > 
> > Example: If a VIN1 device is exposed as /dev/video1 and the current 
> > CSI-2 to VIN routing configuration controlled by the chsel register in 
> > VIN0 is set to 1 the video source routed to VIN1 is N/A. The idea then 
> > is that any open of /dev/video1 should result in -EBUSY until the CSI-2 
> > to VIN routing changes so that VIN1 is connected to a valid subdevice 
> > again. (side note: changing chsel value will not be allowed while any 
> > VIN device is opened and is done using MC)
> > 
> > This series was originally part of my R-Car Gen3 enablement series so I
> > chose to keep this behavior even if the underlying Gen2 OPS could be 
> > fixed in a different way. With this solution a unavailable subdevice 
> > (subdev not bound on Gen2+Gen3 or a N/A subdevice due to routing setup 
> > on Gen3) would be handled the same (-EBUSY) on both Gen2 and Gen3.
> > 
> > All testing I have done on the driver both on Gen2 and Gen3 have been 
> > based on this solution for quiet a while now. And it seemed strange for 
> > me to try and solve the Gen2 issue differently only to rework it again 
> > later in the Gen3 enablement series.
> > 
> > I'm sorry that I did not explain more about this in the original cover 
> > letter. Did this explanation help clear things up? And is the idea of 
> > returning -EBUSY a OK solution in general to the problem that a video 
> > device who once had all its subdevices available no longer do so, but it 
> > might get them back in the future? I'm happy too keep working and 
> > improving this solution, this is only the best one i found so far :-)
> > 
> >>
> >> It really makes no sense for subdevs. In fact, all subdevs should set this
> >> flag since in the current implementation this is completely impossible to
> >> implement safely.
> >>
> >> I suggest you drop the patches relating to this and instead set the suppress
> >> flag.
> > 
> > If possible I would like to explore the possibility to keep it in the 
> > series. I think it would be an advantage to treat on unbound subdevice 
> > on Gen2 in the same way as a VIN instance on Gen3 would treat a CSI-2 to 
> > VIN routing configuration with a N/A route.
> > 
> > I am of course willing to rework the behavior to something else then 
> > returning -EBUSY if a VIN instance currently have all subdevices 
> > available for some reason. I would like input on how such a scheme could 
> > look like since the -EBUSY one is the only solution I have managed to 
> > figure out and implement.
> 
> I think the core problem here is perhaps less the patches but more the commit
> logs you wrote. You say in several places that is it 'To support unbind and
> rebinding of subdevices'. But it doesn't support that, instead it really
> prepares for a specific Gen3 use-case.
> 
> All existing drivers with subdevs can be induced to oops if you unbind/bind
> a subdev. It is broken at the core, and without fixing it at the core first,
> anything you try to do in a driver is just a band-aid.

I see your point, the patches where extracted from my Gen3 enablement 
series to solve the OPS reported by Wolfram when unbinding/binding on 
Gen2. But yes they try to solve the issue by preparing for the wanted 
Gen3 behavior where this particular OPS do not happen (if the device is 
unused).

I do now understand that trying to fix OPS related to unbind/bind is 
futile without core support.

> 
> Take patch 07/11, the commit log says:
> 
> "To fix support for unbind and rebinding of subdevices the
> rvin_v4l2_probe() needs to be called before there might be any subdevice
> bound. Move pad index discovery to when we know the subdevice is
> present."
> 
> This commit log is very misleading. It suggests that this patch fixes
> unbind/rebind support, which it doesn't. Instead, it just prepares for
> patch 11/11 where you want to (partially?) re-initialize the subdevs at
> first open. And that in turn is for future Gen3 support.
> 
> None of this has anything to do with unbind/rebind. The fact that you
> can now do this if there are no filehandles open is a side-effect, and not
> the main reason for the patches.
> 
> I recommend that you take another look and rewrite the commit logs so they
> reflect the real reason you do this.

I will do so and post a new version, thanks for your feedback.

> 
> > And thanks again for your feedback, I really love to see some R-Car VIN 
> > work move forward. Let me know if I can do anything to ease the process.
> 
> Sure, no problem. I've not been a good reviewer recently, but it's getting
> better as I have more time now.
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund