Messages in this thread | | | From | Steve Longerbeam <> | Subject | Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility | Date | Mon, 7 Oct 2019 17:27:57 -0700 |
| |
On 10/2/19 2:31 PM, Tim Harvey wrote: > On Fri, Sep 27, 2019 at 1:43 PM Niklas Söderlund > <niklas.soderlund@ragnatech.se> wrote: >> Hi Tim, >> >> On 2019-09-27 12:26:40 -0700, Tim Harvey wrote: >>> On Fri, Sep 27, 2019 at 12:04 PM Niklas Söderlund >>> <niklas.soderlund@ragnatech.se> wrote: >>>> Hi Tim, >>>> >>>> Sorry for taking to so long to look at this. >>>> >>>> On 2019-09-23 15:04:47 -0700, Tim Harvey wrote: >>>>> On Thu, Aug 29, 2019 at 7:29 AM Niklas Söderlund >>>>> <niklas.soderlund@ragnatech.se> wrote: >>>>>> Hi, >>>>>> >>>>>> On 2019-08-29 13:43:49 +0200, Hans Verkuil wrote: >>>>>>> Adding Niklas. >>>>>>> >>>>>>> Niklas, can you take a look at this? >>>>>> I'm happy to have a look at this. I'm currently moving so all my boards >>>>>> are in a box somewhere. I hope to have my lab up and running next week, >>>>>> so if this is not urgent I will look at it then. >>>>>> >>>>> Niklas, >>>>> >>>>> Have you looked at this yet? Without this patch the ADV7280A does not >>>>> output proper BT.656. We tested this on a Gateworks Ventana GW5404-G >>>>> which uses the ADV7280A connected to the IMX6 CSI parallel bus. I'm >>>>> hoping to see this get merged and perhaps backported to older kernels. >>>> I only have access to an adv7180 so I was unable to test this patch. >>>> After reviewing the documentation I think the patch is OK if what you >>>> want is to unconditionally switch the driver from outputting BT.656-3 to >>>> outputting BT.656-4. >>>> >>>> As this change would effect a large number of compat strings (adv7280, >>>> adv7280-m, adv7281, adv7281-m, adv7281-ma, adv7282, adv7282-m) and the >>>> goal is to back port it I'm a bit reluctant to adding my tag to this >>>> patch as I'm not sure if this will break other setups. >>>> >>>> From the documentation about the BT.656-4 register (address 0x04 bit 7): >>>> >>>> Between Revision 3 and Revision 4 of the ITU-R BT.656 standards, >>>> the ITU has changed the toggling position for the V bit within >>>> the SAV EAV codes for NTSC. The ITU-R BT.656-4 standard >>>> bit allows the user to select an output mode that is compliant >>>> with either the previous or new standard. For further information, >>>> visit the International Telecommunication Union website. >>>> >>>> Note that the standard change only affects NTSC and has no >>>> bearing on PAL. >>>> >>>> When ITU-R BT.656-4 is 0 (default), the ITU-R BT.656-3 >>>> specification is used. The V bit goes low at EAV of Line 10 >>>> and Line 273. >>>> >>>> When ITU-R BT.656-4 is 1, the ITU-R BT.656-4 specification is >>>> used. The V bit goes low at EAV of Line 20 and Line 283. >>>> >>>> Do you know what effects such a change would bring? Looking at the >>>> driver BT.656-4 seems to be set unconditionally for some adv7180 chips. >>>> >>> Niklas, >>> >>> Quite simply, we have a board that has an ADV7180 attached to the >>> parallel CSI of an IMX6 that worked fine with mainline drivers then >>> when we revised this board to attach an ADV7280A in the same way >>> capture failed to sync. Investigation showed that the NEWAVMODE >>> differed between the two. >> I understand your problem, the driver configures adv7180 and adv7280 >> differently. >> >>> So if the point of the driver is to configure the variants in the same >>> way, this patch needs to be applied. >> I'm not sure that is the point of the driver. As the driver today >> configures different compatible strings differently. Some as ITU-R >> BT.656-3 and some as ITU-R BT.656-4, I can only assume there is a reason >> for that. >> >>> I would maintain that the adv7180 comes up with NEWAVMODE enabled and >>> in order to be compatible we must configure the adv7282 the same. >>> >>> The same argument can be made for setting the V bit end position in >>> NTSC mode - its done for the adv7180 so for compatible output it >>> should be done for the adv7282. >> I understand that this is needed to make it a drop-in replacement for >> the adv7180 in your use-case. But I'm not sure it is a good idea for >> other users of the driver. What if someone is already using a adv7282 on >> a board and depends on it providing ITU-R BT.656-3 and the old settings? >> If this patch is picked up there use-cases may break. >> >> I'm not sure what the best way forward is I'm afraid. Looking at >> video-interfaces.txt we have a device tree property bus-type which is >> used to describe the bus is a BT.656 bus but not which revision of it. >> >> I'm not really found of driver specific bus descriptions, but maybe this >> is a case where one might consider adding one? Hans what do you think? >> > Niklas / Hans, > > Thanks for the feedback. I thought that the goal of any 'compatible' > device should be to be configured identically. If that's not the case > then we need more discussion for sure. > > There are 3 registers being changed by this patch which do the > following for the adv7182/adv7280/adv7181/adv7282: > - change output from BT656-3 to BT656-4 (as the driver does this for adv7180) > - enable NEWAVMODE meaning EAV/SAV codes are configurable (new code > but adv7180 enables this by power-on default and adv7280 does not) > - configure V bit end count (to be what adv7180 uses; this isn't used > if NEWAVMODE is disabled) > > So its not only the question of how to decide to configure BT656-3 vs > BT656-4 but how to deal with differences in the EAV/SAV codes. I'm not > an expert so I don't know what configuration is BT656 compliant and of > course without knowing who is using these devices we can't tell what > would break even if we fix something that may be misconfigured already > (or even completely unused). > > I'm cc'ing Steve Longerbeam as well as at one point he was suggesting > adding a 'newavmode' property to the adv7180 bindings and likely > recalls the discussions there.
I've never understood why the adv7180 driver configures a non-standard V-bit end position (ADV7180_NTSC_V_BIT_END_MANUAL_NVEND), maybe because the driver was introduced along with a new bridge driver that assumes that V-bit position. The parallel CSI interface in imx6 driver is also configuring its crop window to work with this non-standard V-bit position.
The most straight-forward approach to decouple these adv7180 dependencies would be to remove the non-standard V-bit setting in the adv7180 driver, and it should output standard bt.656-3 or bt.656-4 SAV/EAV codes for all compatible chips. Then expand on the bus-type DT bindings to distinguish between bt.656-3 and bt.656-4. And finally all bridge drivers that use adv7180 would have to be fixed to work only with standard bt.656-3/4 codes. But I realize that last part isn't so easy, and it's even possible some bridge drivers may not be able to cope with the standard V-bit positions.
Steve
| |