From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Longerbeam Subject: Re: [PATCH 12/16] gpu: ipu-v3: Fix CSI0 blur in NTSC format Date: Sat, 16 Jul 2016 13:24:02 -0700 Message-ID: <578A97E2.1090308@mentor.com> References: <1467932621-358-1-git-send-email-steve_longerbeam@mentor.com> <1467932621-358-13-git-send-email-steve_longerbeam@mentor.com> <1467999258.2365.71.camel@pengutronix.de> <578278C7.8020503@mentor.com> <5786C876.50300@mentor.com> <1468587534.7213.41.camel@pengutronix.de> <57896D1F.3000002@mentor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <57896D1F.3000002@mentor.com> Sender: linux-kernel-owner@vger.kernel.org To: Philipp Zabel Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Hans Verkuil List-Id: dri-devel@lists.freedesktop.org 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 >>>>>> >>>>>> 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 >>>>>> --- >>>>>> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950AbcGPUYK (ORCPT ); Sat, 16 Jul 2016 16:24:10 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:36350 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751698AbcGPUYH (ORCPT ); Sat, 16 Jul 2016 16:24:07 -0400 Subject: Re: [PATCH 12/16] gpu: ipu-v3: Fix CSI0 blur in NTSC format To: Philipp Zabel References: <1467932621-358-1-git-send-email-steve_longerbeam@mentor.com> <1467932621-358-13-git-send-email-steve_longerbeam@mentor.com> <1467999258.2365.71.camel@pengutronix.de> <578278C7.8020503@mentor.com> <5786C876.50300@mentor.com> <1468587534.7213.41.camel@pengutronix.de> <57896D1F.3000002@mentor.com> CC: , , Hans Verkuil From: Steve Longerbeam Message-ID: <578A97E2.1090308@mentor.com> Date: Sat, 16 Jul 2016 13:24:02 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <57896D1F.3000002@mentor.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>>>> >>>>>> 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 >>>>>> --- >>>>>> 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