On 08/24/2017 08:25 AM, Yang, Wenyou wrote: > > > On 2017/8/23 18:37, Hans Verkuil wrote: >> On 08/22/17 09:30, Wenyou.Yang@xxxxxxxxxxxxx wrote: >>> Hi Hans, >>> >>>> -----Original Message----- >>>> From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] >>>> Sent: 2017年8月22日 15:00 >>>> To: Wenyou Yang - A41535 <Wenyou.Yang@xxxxxxxxxxxxx>; Mauro Carvalho >>>> Chehab <mchehab@xxxxxxxxxxxxxxxx> >>>> Cc: Nicolas Ferre - M43238 <Nicolas.Ferre@xxxxxxxxxxxxx>; linux- >>>> kernel@xxxxxxxxxxxxxxx; Sakari Ailus <sakari.ailus@xxxxxx>; Jonathan Corbet >>>> <corbet@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Linux Media Mailing List >>>> <linux-media@xxxxxxxxxxxxxxx> >>>> 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 <wenyou.yang@xxxxxxxxxxxxx> >>>>>>> --- >>>>>>> >>>>>>> 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? Two options spring to mind: split into two tables or add a bool that tells whether the format can be created by the isc or not. Regards, Hans
