Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751286AbdHXG0H (ORCPT ); Thu, 24 Aug 2017 02:26:07 -0400 Received: from esa4.microchip.iphmx.com ([68.232.154.123]:33753 "EHLO esa4.microchip.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751038AbdHXG0G (ORCPT ); Thu, 24 Aug 2017 02:26:06 -0400 X-IronPort-AV: E=Sophos;i="5.41,420,1498546800"; d="scan'208";a="6262144" Subject: Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor. To: Hans Verkuil , CC: , , , , , References: <20170817071614.12767-1-wenyou.yang@microchip.com> <20170817071614.12767-2-wenyou.yang@microchip.com> <61cb51fa-8d05-6707-00cc-429c761fa6f5@xs4all.nl> <14941b74-8931-4d00-0664-0735fad9b5d1@Microchip.com> From: "Yang, Wenyou" Message-ID: <4556e520-a57a-6f26-4188-9b32f1701515@Microchip.com> Date: Thu, 24 Aug 2017 14:25:12 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4456 Lines: 111 On 2017/8/23 18:37, Hans Verkuil wrote: > On 08/22/17 09:30, Wenyou.Yang@microchip.com wrote: >> Hi Hans, >> >>> -----Original Message----- >>> From: Hans Verkuil [mailto:hverkuil@xs4all.nl] >>> Sent: 2017年8月22日 15:00 >>> To: Wenyou Yang - A41535 ; Mauro Carvalho >>> Chehab >>> Cc: Nicolas Ferre - M43238 ; linux- >>> kernel@vger.kernel.org; Sakari Ailus ; Jonathan Corbet >>> ; linux-arm-kernel@lists.infradead.org; Linux Media Mailing List >>> >>> Subject: Re: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor. >>> >>> On 08/22/2017 03:18 AM, Yang, Wenyou wrote: >>>> Hi Hans, >>>> >>>> On 2017/8/21 22:07, Hans Verkuil wrote: >>>>> On 08/17/2017 09:16 AM, Wenyou Yang wrote: >>>>>> The 12-bit parallel interface supports the Raw Bayer, YCbCr, >>>>>> Monochrome and JPEG Compressed pixel formats from the external >>>>>> sensor, not support RBG pixel format. >>>>>> >>>>>> Signed-off-by: Wenyou Yang >>>>>> --- >>>>>> >>>>>> drivers/media/platform/atmel/atmel-isc.c | 5 +++++ >>>>>> 1 file changed, 5 insertions(+) >>>>>> >>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc.c >>>>>> b/drivers/media/platform/atmel/atmel-isc.c >>>>>> index d4df3d4ccd85..535bb03783fe 100644 >>>>>> --- a/drivers/media/platform/atmel/atmel-isc.c >>>>>> +++ b/drivers/media/platform/atmel/atmel-isc.c >>>>>> @@ -1478,6 +1478,11 @@ static int isc_formats_init(struct isc_device *isc) >>>>>> while (!v4l2_subdev_call(subdev, pad, enum_mbus_code, >>>>>> NULL, &mbus_code)) { >>>>>> mbus_code.index++; >>>>>> + >>>>>> + /* Not support the RGB pixel formats from sensor */ >>>>>> + if ((mbus_code.code & 0xf000) == 0x1000) >>>>>> + continue; >>>>> Am I missing something? Here you skip any RGB mediabus formats, but >>>>> in patch 3/3 you add RGB mediabus formats. But this patch prevents >>>>> those new formats from being selected, right? >>>> This patch prevents getting the RGB format from the sensor directly. >>>> The RGB format can be produced by ISC controller by itself. >>> OK, I think I see what is going on here. The isc_formats array really is two arrays >>> in one: up to RAW_FMT_IND_END it describes what it can receive from the >>> source, and after that it describes what it can convert it to. >> Not exactly. >> >> Yes, up to RAW_FMT_IND_END, these formats must be got from the senor, they are RAW formats. >> From ISC_FMT_IND_START to ISC_FMT_IND_END, they can be generated by the ISC controller. >> It is possible they can be got from the sensor too, the driver will check it. >> If it can be got from both the sensor and the ISC controller, the user can use the "sensor_preferred" parameter to decide from which one to get. >> The RBG formats are the exception. >> >>> But if you can't handle RGB formats from the sensor, then why not make sure >>> none of the mbus codes in isc_formats uses RGB? That makes much more sense. >>> >>> E.g.: >>> >>> { V4L2_PIX_FMT_RGB565, MEDIA_BUS_FMT_RGB565_2X8_LE, 16, >>> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, >>> ISC_RLP_CFG_MODE_RGB565, >>> ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b, >>> false, false }, >>> >>> Why use MEDIA_BUS_FMT_RGB565_2X8_LE if this apparently is not supported? >> This array is also the lists of all formats supported by the ISC(including got from the sensor). >> The RGB formats are only generated by the ISC controller, not from the sensor. > You're adding code that skips any entries of the table where mbus_code is an > RGB code. But this can also be done by not having RGB mbus codes in the table > in the first place since they make no sense if the HW cannot handle that! > Set the mbus_code to e.g. 0 for such entries, that makes more sense. > > I also strongly suggest changing how the table is organized since those > _FMT_IND_ indices are all to easy to get wrong (and frankly hard to understand). Yes, you are right, I will change it. Do you have some advice? Thank you. > > Regards, > > Hans > >>> Regards, >>> >>> Hans >>> >>>>> Regards, >>>>> >>>>> Hans >>>>> >>>>>> + >>>>>> fmt = find_format_by_code(mbus_code.code, &i); >>>>>> if (!fmt) >>>>>> continue; >>>>>> >>>> Best Regards, >>>> Wenyou Yang >>>> >> Best Regards, >> Wenyou Yang Best Regards, Wenyou Yang