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.
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.
> 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
