linux-kernel - Re: [PATCH 12/16] gpu: ipu-v3: Fix CSI0 blur in NTSC format

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <578A97E2.1090308@mentor.com>
Date:	Sat, 16 Jul 2016 13:24:02 -0700
From:	Steve Longerbeam <steve_longerbeam@...tor.com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
CC:	<dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
	Hans Verkuil <hverkuil@...all.nl>
Subject: Re: [PATCH 12/16] gpu: ipu-v3: Fix CSI0 blur in NTSC format



On 07/15/2016 04:09 PM, Steve Longerbeam wrote:
>
>
> On 07/15/2016 05:58 AM, Philipp Zabel wrote:
>> Am Mittwoch, den 13.07.2016, 16:02 -0700 schrieb Steve Longerbeam:
>>> On 07/10/2016 09:33 AM, Steve Longerbeam wrote:
>>>>
>>>> On 07/08/2016 10:34 AM, Philipp Zabel wrote:
>>>>> Am Donnerstag, den 07.07.2016, 16:03 -0700 schrieb Steve Longerbeam:
>>>>>> From: Suresh Dhandapani <Suresh.Dhandapani@...bosch.com>
>>>>>>
>>>>>> This patch will change the register IPU_CSI0_CCIR_CODE_2 value from
>>>>>> 0x40596 to 0x405A6. The change is related to the Start of field 1
>>>>>> first blanking line command bit[5-3] for NTSC format only. This
>>>>>> change is dependent with ADV chip where the NEWAVMODE is set to 0
>>>>>> in register 0x31. Setting NEWAVMODE to "0" in ADV means "EAV/SAV
>>>>>> codes generated to suit analog devices encoders".
>>>>>>
>>>>>> Signed-off-by: Suresh Dhandapani <Suresh.Dhandapani@...bosch.com>
>>>>>> ---
>>>>>>    drivers/gpu/ipu-v3/ipu-csi.c | 2 +-
>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/ipu-v3/ipu-csi.c 
>>>>>> b/drivers/gpu/ipu-v3/ipu-csi.c
>>>>>> index 0eac28c..ec81958 100644
>>>>>> --- a/drivers/gpu/ipu-v3/ipu-csi.c
>>>>>> +++ b/drivers/gpu/ipu-v3/ipu-csi.c
>>>>>> @@ -422,7 +422,7 @@ int ipu_csi_init_interface(struct ipu_csi *csi,
>>>>>>                  ipu_csi_write(csi, 0xD07DF | CSI_CCIR_ERR_DET_EN,
>>>>>>                          CSI_CCIR_CODE_1);
>>>>>> -            ipu_csi_write(csi, 0x40596, CSI_CCIR_CODE_2);
>>>>>> +            ipu_csi_write(csi, 0x405A6, CSI_CCIR_CODE_2);
>>>>>>                ipu_csi_write(csi, 0xFF0000, CSI_CCIR_CODE_3);
>>>>>>            } else {
>>>>>>                dev_err(csi->ipu->dev,
>>>>> This looks like a very hardware specific hack? I'll at least have to
>>>>> test if that also works with other analog decoders.
>>>> Hi Philipp,
>>>>
>>>> Yes it's a hack, but it has always been a hack (hardcoded values). 
>>>> And the
>>>> reason is simple, nobody AFAIK (including me) understands how to 
>>>> program
>>>> these CSI_CCIR_CODE registers, the description in the reference 
>>>> manual is
>>>> complete gibberish.
>>> Hi Philipp, Ian over at linux-media helped me to understand these 
>>> registers a
>>> little better, although there are still mysteries given the poor 
>>> documentation.
>>> You should have been copied on that linux-media thread.
>>>
>>>> The reason we made this change is that, in discussions with Analog 
>>>> Devices,
>>>> they recommended setting NEWAVMODE, which changes the positions of
>>>> the AV codes sent by the ADV7180 on the bt.656 bus. It took Suresh 
>>>> at least
>>>> a full day of reverse engineering (Suresh correct me if I am wrong) 
>>>> to hit
>>>> on the correct values in these registers to regain stable video 
>>>> after switching
>>>> the ADV7180 to NEWAVMODE.
>>> So this NEWAVMODE is somehow breaking from the BT.656 standard, which
>>> necessitated the change to CSI_CCIR_CODE_2. So NEWAVMODE if enabled in
>>> the ADV7180 will break other capture backends that are expecting 
>>> standard
>>> BT.656 SAV/EAV codes. So NEWAVMODE should not be used and I will remove
>>> this patch in the next version.
>> Ok. To use that mode, first a new v4l2 mbus type and corresponding DT
>> bindings would have to be added.
>
> Hmm, do you mean define something like a V4L2_MBUS_BT656_NEWAVMODE,
> and then add a new "newavmode" boolean DT binding parsed by
> v4l2_of_parse_endpoint()?
>
> I don't know if that would make sense given that this NEWAVMODE is a kind
> of hack of the BT.656 standard, only used by Analog Devices 
> encoders/decoders.
>
> Although there a _lot_ of AD encoder/decoder chips (and subdev drivers 
> written for
> them), so maybe it would make sense to do this.
>

I don't think a "newavmode" boolean property would necessitate a whole new
mbus type, but perhaps just a new parallel bus flag. I will propose a 
patch at
linux-media that adds this.

Steve