RE: [PATCH 1/3] media: atmel-isc: Not support RBG format from sensor.

[<Wenyou.Yang@microchip.com>]

Hi Hans,

> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
> Sent: 2017年8月22日 15:00
> To: Wenyou Yang - A41535 <Wenyou.Yang@microchip.com>; Mauro Carvalho
> Chehab <mchehab@s-opensource.com>
> Cc: Nicolas Ferre - M43238 <Nicolas.Ferre@microchip.com>; linux-
> kernel@vger.kernel.org; Sakari Ailus <sakari.ailus@iki.fi>; Jonathan Corbet
> <corbet@lwn.net>; linux-arm-kernel@lists.infradead.org; Linux Media Mailing List
> <linux-media@vger.kernel.org>
> 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@microchip.com>
> >>> ---
> >>>
> >>>   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.

> 
> Regards,
> 
> 	Hans
> 
> >
> >> Regards,
> >>
> >> 	Hans
> >>
> >>> +
> >>>   		fmt = find_format_by_code(mbus_code.code, &i);
> >>>   		if (!fmt)
> >>>   			continue;
> >>>
> >
> > Best Regards,
> > Wenyou Yang
> >

Best Regards,
Wenyou Yang