The intention of the patch set is to add more configuration of
formats: GREY, ARGB444, ARGB555 and ARGB32, and add the checking
the format from the extern sensor which doesn't support RGB
formats from the external sensor.
Wenyou Yang (3):
media: atmel-isc: Not support RBG format from sensor.
media: atmel-isc: Remove the redundant assignment
media: atmel-isc: Add more format configurations
drivers/media/platform/atmel/atmel-isc.c | 89 ++++++++++++++++++++------------
1 file changed, 56 insertions(+), 33 deletions(-)
--
2.13.0
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 <[email protected]>
---
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;
+
fmt = find_format_by_code(mbus_code.code, &i);
if (!fmt)
continue;
--
2.13.0
Remove the redundant assignment of members in the isc_formats array.
Signed-off-by: Wenyou Yang <[email protected]>
---
drivers/media/platform/atmel/atmel-isc.c | 64 ++++++++++++++++----------------
1 file changed, 32 insertions(+), 32 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 535bb03783fe..d91f4e5f8a8d 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -187,74 +187,74 @@ struct isc_device {
#define ISC_FMT_IND_END 14
static struct isc_format isc_formats[] = {
+ /* 0 */
{ V4L2_PIX_FMT_SBGGR8, MEDIA_BUS_FMT_SBGGR8_1X8, 8,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 1 */
{ V4L2_PIX_FMT_SGBRG8, MEDIA_BUS_FMT_SGBRG8_1X8, 8,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 2 */
{ V4L2_PIX_FMT_SGRBG8, MEDIA_BUS_FMT_SGRBG8_1X8, 8,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 3 */
{ V4L2_PIX_FMT_SRGGB8, MEDIA_BUS_FMT_SRGGB8_1X8, 8,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 4 */
{ V4L2_PIX_FMT_SBGGR10, MEDIA_BUS_FMT_SBGGR10_1X10, 16,
ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 5 */
{ V4L2_PIX_FMT_SGBRG10, MEDIA_BUS_FMT_SGBRG10_1X10, 16,
ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 6 */
{ V4L2_PIX_FMT_SGRBG10, MEDIA_BUS_FMT_SGRBG10_1X10, 16,
ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 7 */
{ V4L2_PIX_FMT_SRGGB10, MEDIA_BUS_FMT_SRGGB10_1X10, 16,
ISC_PFG_CFG0_BPS_TEN, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT10,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 8 */
{ V4L2_PIX_FMT_SBGGR12, MEDIA_BUS_FMT_SBGGR12_1X12, 16,
ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 9 */
{ V4L2_PIX_FMT_SGBRG12, MEDIA_BUS_FMT_SGBRG12_1X12, 16,
ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_GBGB, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 10 */
{ V4L2_PIX_FMT_SGRBG12, MEDIA_BUS_FMT_SGRBG12_1X12, 16,
ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_GRGR, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 11 */
{ V4L2_PIX_FMT_SRGGB12, MEDIA_BUS_FMT_SRGGB12_1X12, 16,
ISC_PFG_CFG0_BPS_TWELVE, ISC_BAY_CFG_RGRG, ISC_RLP_CFG_MODE_DAT12,
- ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x0 },
+ /* 12 */
{ V4L2_PIX_FMT_YUV420, 0x0, 12,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
- ISC_DCFG_IMODE_YC420P, ISC_DCTRL_DVIEW_PLANAR, 0x7fb,
- false, false },
+ ISC_DCFG_IMODE_YC420P, ISC_DCTRL_DVIEW_PLANAR, 0x7fb },
+ /* 13 */
{ V4L2_PIX_FMT_YUV422P, 0x0, 16,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
- ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb,
- false, false },
+ ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb },
+ /* 14 */
{ 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 },
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
+ /* 15 */
{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8, 16,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
- ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0,
- false, false },
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
};
#define GAMMA_MAX 2
--
2.13.0
Add the configuration of formats: GREY, ARGB444, ARGB555 and ARGB32.
Signed-off-by: Wenyou Yang <[email protected]>
---
drivers/media/platform/atmel/atmel-isc.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index d91f4e5f8a8d..4e18fe1104c8 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -184,7 +184,7 @@ struct isc_device {
#define RAW_FMT_IND_START 0
#define RAW_FMT_IND_END 11
#define ISC_FMT_IND_START 12
-#define ISC_FMT_IND_END 14
+#define ISC_FMT_IND_END 18
static struct isc_format isc_formats[] = {
/* 0 */
@@ -246,12 +246,30 @@ static struct isc_format isc_formats[] = {
{ V4L2_PIX_FMT_YUV422P, 0x0, 16,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb },
+
/* 14 */
+ { V4L2_PIX_FMT_GREY, MEDIA_BUS_FMT_Y8_1X8, 8,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DATY8,
+ ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x1fb },
+
+ /* 15 */
+ { V4L2_PIX_FMT_ARGB444, MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE, 16,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB444,
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
+ /* 16 */
+ { V4L2_PIX_FMT_ARGB555, MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE, 16,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB555,
+ ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
+ /* 17 */
{ 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 },
+ /* 18 */
+ { V4L2_PIX_FMT_ARGB32, MEDIA_BUS_FMT_ARGB8888_1X32, 32,
+ ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB32,
+ ISC_DCFG_IMODE_PACKED32, ISC_DCTRL_DVIEW_PACKED, 0x7b },
- /* 15 */
+ /* 19 */
{ V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8, 16,
ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
--
2.13.0
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 <[email protected]>
> ---
>
> 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?
Regards,
Hans
> +
> fmt = find_format_by_code(mbus_code.code, &i);
> if (!fmt)
> continue;
>
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 <[email protected]>
>> ---
>>
>> 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.
> Regards,
>
> Hans
>
>> +
>> fmt = find_format_by_code(mbus_code.code, &i);
>> if (!fmt)
>> continue;
>>
Best Regards,
Wenyou Yang
On 08/17/2017 09:16 AM, Wenyou Yang wrote:
> Add the configuration of formats: GREY, ARGB444, ARGB555 and ARGB32.
>
> Signed-off-by: Wenyou Yang <[email protected]>
> ---
>
> drivers/media/platform/atmel/atmel-isc.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index d91f4e5f8a8d..4e18fe1104c8 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -184,7 +184,7 @@ struct isc_device {
> #define RAW_FMT_IND_START 0
> #define RAW_FMT_IND_END 11
> #define ISC_FMT_IND_START 12
> -#define ISC_FMT_IND_END 14
> +#define ISC_FMT_IND_END 18
Shouldn't this be 19?
Regards,
Hans
>
> static struct isc_format isc_formats[] = {
> /* 0 */
> @@ -246,12 +246,30 @@ static struct isc_format isc_formats[] = {
> { V4L2_PIX_FMT_YUV422P, 0x0, 16,
> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
> ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb },
> +
> /* 14 */
> + { V4L2_PIX_FMT_GREY, MEDIA_BUS_FMT_Y8_1X8, 8,
> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DATY8,
> + ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x1fb },
> +
> + /* 15 */
> + { V4L2_PIX_FMT_ARGB444, MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE, 16,
> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB444,
> + ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
> + /* 16 */
> + { V4L2_PIX_FMT_ARGB555, MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE, 16,
> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB555,
> + ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
> + /* 17 */
> { 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 },
> + /* 18 */
> + { V4L2_PIX_FMT_ARGB32, MEDIA_BUS_FMT_ARGB8888_1X32, 32,
> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB32,
> + ISC_DCFG_IMODE_PACKED32, ISC_DCTRL_DVIEW_PACKED, 0x7b },
>
> - /* 15 */
> + /* 19 */
> { V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8, 16,
> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
> ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
>
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 <[email protected]>
>>> ---
>>>
>>> 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.
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?
Regards,
Hans
>
>> Regards,
>>
>> Hans
>>
>>> +
>>> fmt = find_format_by_code(mbus_code.code, &i);
>>> if (!fmt)
>>> continue;
>>>
>
> Best Regards,
> Wenyou Yang
>
Hi Hans,
> -----Original Message-----
> From: Hans Verkuil [mailto:[email protected]]
> Sent: 2017年8月22日 15:00
> To: Wenyou Yang - A41535 <[email protected]>; Mauro Carvalho
> Chehab <[email protected]>
> Cc: Nicolas Ferre - M43238 <[email protected]>; linux-
> [email protected]; Sakari Ailus <[email protected]>; Jonathan Corbet
> <[email protected]>; [email protected]; Linux Media Mailing List
> <[email protected]>
> 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 <[email protected]>
> >>> ---
> >>>
> >>> 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
Hi Hans,
On 2017/8/22 14:54, Hans Verkuil wrote:
> On 08/17/2017 09:16 AM, Wenyou Yang wrote:
>> Add the configuration of formats: GREY, ARGB444, ARGB555 and ARGB32.
>>
>> Signed-off-by: Wenyou Yang <[email protected]>
>> ---
>>
>> drivers/media/platform/atmel/atmel-isc.c | 22 ++++++++++++++++++++--
>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
>> index d91f4e5f8a8d..4e18fe1104c8 100644
>> --- a/drivers/media/platform/atmel/atmel-isc.c
>> +++ b/drivers/media/platform/atmel/atmel-isc.c
>> @@ -184,7 +184,7 @@ struct isc_device {
>> #define RAW_FMT_IND_START 0
>> #define RAW_FMT_IND_END 11
>> #define ISC_FMT_IND_START 12
>> -#define ISC_FMT_IND_END 14
>> +#define ISC_FMT_IND_END 18
> Shouldn't this be 19?
From ISC_FMT_IND_START to ISC_FMT_IND_END, it describes the formats
they can be got from both the sensor (possibly) and the ISC controller.
The last format (19) is in not this category. It maybe be got from the
sensor, can't be generated by the controller.
> Regards,
>
> Hans
>
>>
>> static struct isc_format isc_formats[] = {
>> /* 0 */
>> @@ -246,12 +246,30 @@ static struct isc_format isc_formats[] = {
>> { V4L2_PIX_FMT_YUV422P, 0x0, 16,
>> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_YYCC,
>> ISC_DCFG_IMODE_YC422P, ISC_DCTRL_DVIEW_PLANAR, 0x3fb },
>> +
>> /* 14 */
>> + { V4L2_PIX_FMT_GREY, MEDIA_BUS_FMT_Y8_1X8, 8,
>> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DATY8,
>> + ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x1fb },
>> +
>> + /* 15 */
>> + { V4L2_PIX_FMT_ARGB444, MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE, 16,
>> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB444,
>> + ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
>> + /* 16 */
>> + { V4L2_PIX_FMT_ARGB555, MEDIA_BUS_FMT_RGB555_2X8_PADHI_LE, 16,
>> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB555,
>> + ISC_DCFG_IMODE_PACKED16, ISC_DCTRL_DVIEW_PACKED, 0x7b },
>> + /* 17 */
>> { 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 },
>> + /* 18 */
>> + { V4L2_PIX_FMT_ARGB32, MEDIA_BUS_FMT_ARGB8888_1X32, 32,
>> + ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_ARGB32,
>> + ISC_DCFG_IMODE_PACKED32, ISC_DCTRL_DVIEW_PACKED, 0x7b },
>>
>> - /* 15 */
>> + /* 19 */
>> { V4L2_PIX_FMT_YUYV, MEDIA_BUS_FMT_YUYV8_2X8, 16,
>> ISC_PFE_CFG0_BPS_EIGHT, ISC_BAY_CFG_BGBG, ISC_RLP_CFG_MODE_DAT8,
>> ISC_DCFG_IMODE_PACKED8, ISC_DCTRL_DVIEW_PACKED, 0x0 },
>>
Best Regards,
Wenyou Yang
On 08/22/17 09:30, [email protected] wrote:
> Hi Hans,
>
>> -----Original Message-----
>> From: Hans Verkuil [mailto:[email protected]]
>> Sent: 2017年8月22日 15:00
>> To: Wenyou Yang - A41535 <[email protected]>; Mauro Carvalho
>> Chehab <[email protected]>
>> Cc: Nicolas Ferre - M43238 <[email protected]>; linux-
>> [email protected]; Sakari Ailus <[email protected]>; Jonathan Corbet
>> <[email protected]>; [email protected]; Linux Media Mailing List
>> <[email protected]>
>> 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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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).
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
>
On 2017/8/23 18:37, Hans Verkuil wrote:
> On 08/22/17 09:30, [email protected] wrote:
>> Hi Hans,
>>
>>> -----Original Message-----
>>> From: Hans Verkuil [mailto:[email protected]]
>>> Sent: 2017年8月22日 15:00
>>> To: Wenyou Yang - A41535 <[email protected]>; Mauro Carvalho
>>> Chehab <[email protected]>
>>> Cc: Nicolas Ferre - M43238 <[email protected]>; linux-
>>> [email protected]; Sakari Ailus <[email protected]>; Jonathan Corbet
>>> <[email protected]>; [email protected]; Linux Media Mailing List
>>> <[email protected]>
>>> 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 <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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
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, [email protected] wrote:
>>> Hi Hans,
>>>
>>>> -----Original Message-----
>>>> From: Hans Verkuil [mailto:[email protected]]
>>>> Sent: 2017年8月22日 15:00
>>>> To: Wenyou Yang - A41535 <[email protected]>; Mauro Carvalho
>>>> Chehab <[email protected]>
>>>> Cc: Nicolas Ferre - M43238 <[email protected]>; linux-
>>>> [email protected]; Sakari Ailus <[email protected]>; Jonathan Corbet
>>>> <[email protected]>; [email protected]; Linux Media Mailing List
>>>> <[email protected]>
>>>> 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 <[email protected]>
>>>>>>> ---
>>>>>>>
>>>>>>> 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
Hi Hans,
On 2017/8/24 14:41, Hans Verkuil wrote:
> 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, [email protected] wrote:
>>>> Hi Hans,
>>>>
>>>>> -----Original Message-----
>>>>> From: Hans Verkuil [mailto:[email protected]]
>>>>> Sent: 2017年8月22日 15:00
>>>>> To: Wenyou Yang - A41535 <[email protected]>; Mauro Carvalho
>>>>> Chehab <[email protected]>
>>>>> Cc: Nicolas Ferre - M43238 <[email protected]>; linux-
>>>>> [email protected]; Sakari Ailus <[email protected]>; Jonathan Corbet
>>>>> <[email protected]>; [email protected]; Linux Media Mailing List
>>>>> <[email protected]>
>>>>> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> 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.
I reworked the format table,
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-August/529683.html
Please give your comments.
>
> Regards,
>
> Hans
Best Regards,
Wenyou Yang