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.y...@microchip.com wrote:
>>> Hi Hans,
>>>
>>>> -----Original Message-----
>>>> From: Hans Verkuil [mailto:hverk...@xs4all.nl]
>>>> Sent: 2017年8月22日 15:00
>>>> To: Wenyou Yang - A41535 <wenyou.y...@microchip.com>; Mauro Carvalho
>>>> Chehab <mche...@s-opensource.com>
>>>> Cc: Nicolas Ferre - M43238 <nicolas.fe...@microchip.com>; linux-
>>>> ker...@vger.kernel.org; Sakari Ailus <sakari.ai...@iki.fi>; Jonathan Corbet
>>>> <cor...@lwn.net>; linux-arm-ker...@lists.infradead.org; Linux Media
>>>> Mailing List
>>>> <linux-me...@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.y...@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.
>> 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
