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: Fri, 15 Jul 2016 16:09:19 -0700 Message-ID: <57896D1F.3000002@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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1468587534.7213.41.camel@pengutronix.de> Sender: linux-kernel-owner@vger.kernel.org To: Philipp Zabel Cc: dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Suresh Dhandapani , Hans Verkuil List-Id: dri-devel@lists.freedesktop.org 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. Adding Hans. 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 S1751660AbcGOXJZ (ORCPT ); Fri, 15 Jul 2016 19:09:25 -0400 Received: from relay1.mentorg.com ([192.94.38.131]:47087 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751475AbcGOXJW (ORCPT ); Fri, 15 Jul 2016 19:09:22 -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> CC: , , Suresh Dhandapani , Hans Verkuil From: Steve Longerbeam Message-ID: <57896D1F.3000002@mentor.com> Date: Fri, 15 Jul 2016 16:09:19 -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: <1468587534.7213.41.camel@pengutronix.de> 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 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. Adding Hans. Steve