Message-ID: <ece1d3cc-891c-49a5-4639-a5a70385e1a4@gmail.com>
Date:   Mon, 7 Oct 2019 17:27:57 -0700
From:   Steve Longerbeam <slongerbeam@...il.com>
To:     Tim Harvey <tharvey@...eworks.com>,
        Niklas Söderlund <niklas.soderlund@...natech.se>,
        Hans Verkuil <hverkuil@...all.nl>
Cc:     Matthew Michilot <matthew.michilot@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media <linux-media@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] media: i2c: adv7180: fix adv7280 BT.656-4 compatibility



On 10/2/19 2:31 PM, Tim Harvey wrote:
> On Fri, Sep 27, 2019 at 1:43 PM Niklas Söderlund
> <niklas.soderlund@...natech.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@...natech.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@...natech.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

Powered by blists - more mailing lists