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

[Yang, Wenyou]

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?

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