[PATCH] media: ov8856: Remove 3280x2464 mode

[PATCH] media: ov8856: Remove 3280x2464 mode

[Robert Foss]

Remove the 3280x2464 mode as it can't be reproduced and yields
an output resolution of 3264x2448 instead of the desired one.

Furthermore the 3264x2448 resolution is the highest resolution
that the product brief lists.

Since 3280x2464 neither works correctly nor seems to be supported
by the sensor, let's remove it.

Signed-off-by: Robert Foss <robert.foss@linaro.org>
---
 drivers/media/i2c/ov8856.c | 202 -------------------------------------
 1 file changed, 202 deletions(-)

diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
index 2f4ceaa80593..3365d19a303d 100644
--- a/drivers/media/i2c/ov8856.c
+++ b/drivers/media/i2c/ov8856.c
@@ -148,196 +148,6 @@ static const struct ov8856_reg mipi_data_rate_360mbps[] = {
 	{0x031e, 0x0c},
 };
 
-static const struct ov8856_reg mode_3280x2464_regs[] = {
-	{0x3000, 0x20},
-	{0x3003, 0x08},
-	{0x300e, 0x20},
-	{0x3010, 0x00},
-	{0x3015, 0x84},
-	{0x3018, 0x72},
-	{0x3021, 0x23},
-	{0x3033, 0x24},
-	{0x3500, 0x00},
-	{0x3501, 0x9a},
-	{0x3502, 0x20},
-	{0x3503, 0x08},
-	{0x3505, 0x83},
-	{0x3508, 0x01},
-	{0x3509, 0x80},
-	{0x350c, 0x00},
-	{0x350d, 0x80},
-	{0x350e, 0x04},
-	{0x350f, 0x00},
-	{0x3510, 0x00},
-	{0x3511, 0x02},
-	{0x3512, 0x00},
-	{0x3600, 0x72},
-	{0x3601, 0x40},
-	{0x3602, 0x30},
-	{0x3610, 0xc5},
-	{0x3611, 0x58},
-	{0x3612, 0x5c},
-	{0x3613, 0xca},
-	{0x3614, 0x20},
-	{0x3628, 0xff},
-	{0x3629, 0xff},
-	{0x362a, 0xff},
-	{0x3633, 0x10},
-	{0x3634, 0x10},
-	{0x3635, 0x10},
-	{0x3636, 0x10},
-	{0x3663, 0x08},
-	{0x3669, 0x34},
-	{0x366e, 0x10},
-	{0x3706, 0x86},
-	{0x370b, 0x7e},
-	{0x3714, 0x23},
-	{0x3730, 0x12},
-	{0x3733, 0x10},
-	{0x3764, 0x00},
-	{0x3765, 0x00},
-	{0x3769, 0x62},
-	{0x376a, 0x2a},
-	{0x376b, 0x30},
-	{0x3780, 0x00},
-	{0x3781, 0x24},
-	{0x3782, 0x00},
-	{0x3783, 0x23},
-	{0x3798, 0x2f},
-	{0x37a1, 0x60},
-	{0x37a8, 0x6a},
-	{0x37ab, 0x3f},
-	{0x37c2, 0x04},
-	{0x37c3, 0xf1},
-	{0x37c9, 0x80},
-	{0x37cb, 0x16},
-	{0x37cc, 0x16},
-	{0x37cd, 0x16},
-	{0x37ce, 0x16},
-	{0x3800, 0x00},
-	{0x3801, 0x00},
-	{0x3802, 0x00},
-	{0x3803, 0x06},
-	{0x3804, 0x0c},
-	{0x3805, 0xdf},
-	{0x3806, 0x09},
-	{0x3807, 0xa7},
-	{0x3808, 0x0c},
-	{0x3809, 0xd0},
-	{0x380a, 0x09},
-	{0x380b, 0xa0},
-	{0x380c, 0x07},
-	{0x380d, 0x88},
-	{0x380e, 0x09},
-	{0x380f, 0xb8},
-	{0x3810, 0x00},
-	{0x3811, 0x00},
-	{0x3812, 0x00},
-	{0x3813, 0x01},
-	{0x3814, 0x01},
-	{0x3815, 0x01},
-	{0x3816, 0x00},
-	{0x3817, 0x00},
-	{0x3818, 0x00},
-	{0x3819, 0x10},
-	{0x3820, 0x80},
-	{0x3821, 0x46},
-	{0x382a, 0x01},
-	{0x382b, 0x01},
-	{0x3830, 0x06},
-	{0x3836, 0x02},
-	{0x3862, 0x04},
-	{0x3863, 0x08},
-	{0x3cc0, 0x33},
-	{0x3d85, 0x17},
-	{0x3d8c, 0x73},
-	{0x3d8d, 0xde},
-	{0x4001, 0xe0},
-	{0x4003, 0x40},
-	{0x4008, 0x00},
-	{0x4009, 0x0b},
-	{0x400a, 0x00},
-	{0x400b, 0x84},
-	{0x400f, 0x80},
-	{0x4010, 0xf0},
-	{0x4011, 0xff},
-	{0x4012, 0x02},
-	{0x4013, 0x01},
-	{0x4014, 0x01},
-	{0x4015, 0x01},
-	{0x4042, 0x00},
-	{0x4043, 0x80},
-	{0x4044, 0x00},
-	{0x4045, 0x80},
-	{0x4046, 0x00},
-	{0x4047, 0x80},
-	{0x4048, 0x00},
-	{0x4049, 0x80},
-	{0x4041, 0x03},
-	{0x404c, 0x20},
-	{0x404d, 0x00},
-	{0x404e, 0x20},
-	{0x4203, 0x80},
-	{0x4307, 0x30},
-	{0x4317, 0x00},
-	{0x4503, 0x08},
-	{0x4601, 0x80},
-	{0x4800, 0x44},
-	{0x4816, 0x53},
-	{0x481b, 0x58},
-	{0x481f, 0x27},
-	{0x4837, 0x16},
-	{0x483c, 0x0f},
-	{0x484b, 0x05},
-	{0x5000, 0x57},
-	{0x5001, 0x0a},
-	{0x5004, 0x04},
-	{0x502e, 0x03},
-	{0x5030, 0x41},
-	{0x5780, 0x14},
-	{0x5781, 0x0f},
-	{0x5782, 0x44},
-	{0x5783, 0x02},
-	{0x5784, 0x01},
-	{0x5785, 0x01},
-	{0x5786, 0x00},
-	{0x5787, 0x04},
-	{0x5788, 0x02},
-	{0x5789, 0x0f},
-	{0x578a, 0xfd},
-	{0x578b, 0xf5},
-	{0x578c, 0xf5},
-	{0x578d, 0x03},
-	{0x578e, 0x08},
-	{0x578f, 0x0c},
-	{0x5790, 0x08},
-	{0x5791, 0x04},
-	{0x5792, 0x00},
-	{0x5793, 0x52},
-	{0x5794, 0xa3},
-	{0x5795, 0x02},
-	{0x5796, 0x20},
-	{0x5797, 0x20},
-	{0x5798, 0xd5},
-	{0x5799, 0xd5},
-	{0x579a, 0x00},
-	{0x579b, 0x50},
-	{0x579c, 0x00},
-	{0x579d, 0x2c},
-	{0x579e, 0x0c},
-	{0x579f, 0x40},
-	{0x57a0, 0x09},
-	{0x57a1, 0x40},
-	{0x59f8, 0x3d},
-	{0x5a08, 0x02},
-	{0x5b00, 0x02},
-	{0x5b01, 0x10},
-	{0x5b02, 0x03},
-	{0x5b03, 0xcf},
-	{0x5b05, 0x6c},
-	{0x5e00, 0x00}
-};
-
 static const struct ov8856_reg mode_3264x2448_regs[] = {
 	{0x0103, 0x01},
 	{0x0302, 0x3c},
@@ -963,18 +773,6 @@ static const struct ov8856_link_freq_config link_freq_configs[] = {
 };
 
 static const struct ov8856_mode supported_modes[] = {
-	{
-		.width = 3280,
-		.height = 2464,
-		.hts = 1928,
-		.vts_def = 2488,
-		.vts_min = 2488,
-		.reg_list = {
-			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
-			.regs = mode_3280x2464_regs,
-		},
-		.link_freq_index = OV8856_LINK_FREQ_720MBPS,
-	},
 	{
 		.width = 3264,
 		.height = 2448,
-- 
2.27.0

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Dongchun Zhu]

Hi Robert,

Thanks for the patch.

On Mon, 2020-11-16 at 16:50 +0100, Robert Foss wrote:
> Remove the 3280x2464 mode as it can't be reproduced and yields
> an output resolution of 3264x2448 instead of the desired one.
> 
> Furthermore the 3264x2448 resolution is the highest resolution
> that the product brief lists.
> 
> Since 3280x2464 neither works correctly nor seems to be supported
> by the sensor, let's remove it.
> 

In fact, I was also confused about 3280x2464 setting at the beginning.
From datasheet, the OV8856 shall support an active array of 3264x2448
pixels (8-megapixel, the maximum) operating at 30fps.

> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  drivers/media/i2c/ov8856.c | 202 -------------------------------------
>  1 file changed, 202 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 2f4ceaa80593..3365d19a303d 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -148,196 +148,6 @@ static const struct ov8856_reg mipi_data_rate_360mbps[] = {
>  	{0x031e, 0x0c},
>  };
>  
> -static const struct ov8856_reg mode_3280x2464_regs[] = {
> -	{0x3000, 0x20},
> -	{0x3003, 0x08},
> -	{0x300e, 0x20},
> -	{0x3010, 0x00},
> -	{0x3015, 0x84},
> -	{0x3018, 0x72},
> -	{0x3021, 0x23},
> -	{0x3033, 0x24},
> -	{0x3500, 0x00},
> -	{0x3501, 0x9a},
> -	{0x3502, 0x20},
> -	{0x3503, 0x08},
> -	{0x3505, 0x83},
> -	{0x3508, 0x01},
> -	{0x3509, 0x80},
> -	{0x350c, 0x00},
> -	{0x350d, 0x80},
> -	{0x350e, 0x04},
> -	{0x350f, 0x00},
> -	{0x3510, 0x00},
> -	{0x3511, 0x02},
> -	{0x3512, 0x00},
> -	{0x3600, 0x72},
> -	{0x3601, 0x40},
> -	{0x3602, 0x30},
> -	{0x3610, 0xc5},
> -	{0x3611, 0x58},
> -	{0x3612, 0x5c},
> -	{0x3613, 0xca},
> -	{0x3614, 0x20},
> -	{0x3628, 0xff},
> -	{0x3629, 0xff},
> -	{0x362a, 0xff},
> -	{0x3633, 0x10},
> -	{0x3634, 0x10},
> -	{0x3635, 0x10},
> -	{0x3636, 0x10},
> -	{0x3663, 0x08},
> -	{0x3669, 0x34},
> -	{0x366e, 0x10},
> -	{0x3706, 0x86},
> -	{0x370b, 0x7e},
> -	{0x3714, 0x23},
> -	{0x3730, 0x12},
> -	{0x3733, 0x10},
> -	{0x3764, 0x00},
> -	{0x3765, 0x00},
> -	{0x3769, 0x62},
> -	{0x376a, 0x2a},
> -	{0x376b, 0x30},
> -	{0x3780, 0x00},
> -	{0x3781, 0x24},
> -	{0x3782, 0x00},
> -	{0x3783, 0x23},
> -	{0x3798, 0x2f},
> -	{0x37a1, 0x60},
> -	{0x37a8, 0x6a},
> -	{0x37ab, 0x3f},
> -	{0x37c2, 0x04},
> -	{0x37c3, 0xf1},
> -	{0x37c9, 0x80},
> -	{0x37cb, 0x16},
> -	{0x37cc, 0x16},
> -	{0x37cd, 0x16},
> -	{0x37ce, 0x16},
> -	{0x3800, 0x00},
> -	{0x3801, 0x00},
> -	{0x3802, 0x00},
> -	{0x3803, 0x06},
> -	{0x3804, 0x0c},
> -	{0x3805, 0xdf},
> -	{0x3806, 0x09},
> -	{0x3807, 0xa7},
> -	{0x3808, 0x0c},
> -	{0x3809, 0xd0},
> -	{0x380a, 0x09},
> -	{0x380b, 0xa0},
> -	{0x380c, 0x07},
> -	{0x380d, 0x88},
> -	{0x380e, 0x09},
> -	{0x380f, 0xb8},
> -	{0x3810, 0x00},
> -	{0x3811, 0x00},
> -	{0x3812, 0x00},
> -	{0x3813, 0x01},
> -	{0x3814, 0x01},
> -	{0x3815, 0x01},
> -	{0x3816, 0x00},
> -	{0x3817, 0x00},
> -	{0x3818, 0x00},
> -	{0x3819, 0x10},
> -	{0x3820, 0x80},
> -	{0x3821, 0x46},
> -	{0x382a, 0x01},
> -	{0x382b, 0x01},
> -	{0x3830, 0x06},
> -	{0x3836, 0x02},
> -	{0x3862, 0x04},
> -	{0x3863, 0x08},
> -	{0x3cc0, 0x33},
> -	{0x3d85, 0x17},
> -	{0x3d8c, 0x73},
> -	{0x3d8d, 0xde},
> -	{0x4001, 0xe0},
> -	{0x4003, 0x40},
> -	{0x4008, 0x00},
> -	{0x4009, 0x0b},
> -	{0x400a, 0x00},
> -	{0x400b, 0x84},
> -	{0x400f, 0x80},
> -	{0x4010, 0xf0},
> -	{0x4011, 0xff},
> -	{0x4012, 0x02},
> -	{0x4013, 0x01},
> -	{0x4014, 0x01},
> -	{0x4015, 0x01},
> -	{0x4042, 0x00},
> -	{0x4043, 0x80},
> -	{0x4044, 0x00},
> -	{0x4045, 0x80},
> -	{0x4046, 0x00},
> -	{0x4047, 0x80},
> -	{0x4048, 0x00},
> -	{0x4049, 0x80},
> -	{0x4041, 0x03},
> -	{0x404c, 0x20},
> -	{0x404d, 0x00},
> -	{0x404e, 0x20},
> -	{0x4203, 0x80},
> -	{0x4307, 0x30},
> -	{0x4317, 0x00},
> -	{0x4503, 0x08},
> -	{0x4601, 0x80},
> -	{0x4800, 0x44},
> -	{0x4816, 0x53},
> -	{0x481b, 0x58},
> -	{0x481f, 0x27},
> -	{0x4837, 0x16},
> -	{0x483c, 0x0f},
> -	{0x484b, 0x05},
> -	{0x5000, 0x57},
> -	{0x5001, 0x0a},
> -	{0x5004, 0x04},
> -	{0x502e, 0x03},
> -	{0x5030, 0x41},
> -	{0x5780, 0x14},
> -	{0x5781, 0x0f},
> -	{0x5782, 0x44},
> -	{0x5783, 0x02},
> -	{0x5784, 0x01},
> -	{0x5785, 0x01},
> -	{0x5786, 0x00},
> -	{0x5787, 0x04},
> -	{0x5788, 0x02},
> -	{0x5789, 0x0f},
> -	{0x578a, 0xfd},
> -	{0x578b, 0xf5},
> -	{0x578c, 0xf5},
> -	{0x578d, 0x03},
> -	{0x578e, 0x08},
> -	{0x578f, 0x0c},
> -	{0x5790, 0x08},
> -	{0x5791, 0x04},
> -	{0x5792, 0x00},
> -	{0x5793, 0x52},
> -	{0x5794, 0xa3},
> -	{0x5795, 0x02},
> -	{0x5796, 0x20},
> -	{0x5797, 0x20},
> -	{0x5798, 0xd5},
> -	{0x5799, 0xd5},
> -	{0x579a, 0x00},
> -	{0x579b, 0x50},
> -	{0x579c, 0x00},
> -	{0x579d, 0x2c},
> -	{0x579e, 0x0c},
> -	{0x579f, 0x40},
> -	{0x57a0, 0x09},
> -	{0x57a1, 0x40},
> -	{0x59f8, 0x3d},
> -	{0x5a08, 0x02},
> -	{0x5b00, 0x02},
> -	{0x5b01, 0x10},
> -	{0x5b02, 0x03},
> -	{0x5b03, 0xcf},
> -	{0x5b05, 0x6c},
> -	{0x5e00, 0x00}
> -};
> -
>  static const struct ov8856_reg mode_3264x2448_regs[] = {
>  	{0x0103, 0x01},
>  	{0x0302, 0x3c},
> @@ -963,18 +773,6 @@ static const struct ov8856_link_freq_config link_freq_configs[] = {
>  };
>  
>  static const struct ov8856_mode supported_modes[] = {
> -	{
> -		.width = 3280,
> -		.height = 2464,
> -		.hts = 1928,
> -		.vts_def = 2488,
> -		.vts_min = 2488,
> -		.reg_list = {
> -			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
> -			.regs = mode_3280x2464_regs,
> -		},
> -		.link_freq_index = OV8856_LINK_FREQ_720MBPS,
> -	},

If 3280x2464 resolution is removed, bayer order needs to be updated in
the meantime. From OV8856's datasheet, bayer order turns to be BGGR if
sensor adopts full mode (3264x2448) or binning mode (1632x1224).

>  	{
>  		.width = 3264,
>  		.height = 2448,

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Sakari Ailus]

Hi Dongchun,

On Tue, Nov 24, 2020 at 03:40:51PM +0800, Dongchun Zhu wrote:
> >  static const struct ov8856_mode supported_modes[] = {
> > -	{
> > -		.width = 3280,
> > -		.height = 2464,
> > -		.hts = 1928,
> > -		.vts_def = 2488,
> > -		.vts_min = 2488,
> > -		.reg_list = {
> > -			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
> > -			.regs = mode_3280x2464_regs,
> > -		},
> > -		.link_freq_index = OV8856_LINK_FREQ_720MBPS,
> > -	},
> 
> If 3280x2464 resolution is removed, bayer order needs to be updated in
> the meantime. From OV8856's datasheet, bayer order turns to be BGGR if
> sensor adopts full mode (3264x2448) or binning mode (1632x1224).

How is this related to the patch?

The next largest size is 16 by 16 less, so the Bayer order is the same. If
it's wrong currently (as it would appear to), it should be a separate
patch.

-- 
Sakari Ailus

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Bingbu Cao]

Hi, Robert

I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
frames based on current settings.

Do you have any issues with this mode?

On 11/16/20 11:50 PM, Robert Foss wrote:
> 0x3812, 0x00},
> 236 {0x3813, 0x01},

-- 
Best regards,
Bingbu Cao

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Dongchun Zhu]

Hi Sakari,

On Tue, 2020-11-24 at 10:43 +0200, Sakari Ailus wrote:
> Hi Dongchun,
> 
> On Tue, Nov 24, 2020 at 03:40:51PM +0800, Dongchun Zhu wrote:
> > >  static const struct ov8856_mode supported_modes[] = {
> > > -	{
> > > -		.width = 3280,
> > > -		.height = 2464,
> > > -		.hts = 1928,
> > > -		.vts_def = 2488,
> > > -		.vts_min = 2488,
> > > -		.reg_list = {
> > > -			.num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
> > > -			.regs = mode_3280x2464_regs,
> > > -		},
> > > -		.link_freq_index = OV8856_LINK_FREQ_720MBPS,
> > > -	},
> > 
> > If 3280x2464 resolution is removed, bayer order needs to be updated in
> > the meantime. From OV8856's datasheet, bayer order turns to be BGGR if
> > sensor adopts full mode (3264x2448) or binning mode (1632x1224).
> 
> How is this related to the patch?
> 

Yes, it seems to be another issue.
But it is very often that bayer order is strongly related to the image
window size and mirror/flip setting.

> The next largest size is 16 by 16 less, so the Bayer order is the same. If
> it's wrong currently (as it would appear to), it should be a separate
> patch.
> 

OV8856 sensor array region consists of 3 main window settings.
The inner window is controlled by [H_win_off, V_win_off].
From the old unusual 3280x2464 and 1640x1232 setting,
H_win_off(R3810-R3811) is 0, V_win_off(R3812-R3813) is 1.

Considering that the register TEST_PATTERN_CTRL(R4320) controlling pixel
order is not set (default: 0x80, meaning BG/GR) and mirror/flip are both
OFF, the absolute coordinate of crop_start is expressed as:
[H_crop_start+H_win_off, V_crop_start+V_win_off] = [0, 7]

Thus the first pixel shall start with G channel(according to datasheet).
This is different with current resolutions (3264x2448 and 1632x1224).

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Robert Foss]

On Tue, 24 Nov 2020 at 11:17, Dongchun Zhu (朱东春)
<Dongchun.Zhu@mediatek.com> wrote:
>
> Hi Robert,
>
>
>
> Just updated the identification method of first pixel on list.
>
> The bayer order for the new setting (either FULL or BINNING mode) shall be BG/GR.
>
> This has been proved both theoretically and experimentally.

Thank you for verifying that BGGR mode actually what is produced.

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Robert Foss]

On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
> Hi, Robert
>
> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
> frames based on current settings.
>
> Do you have any issues with this mode?

As far as I can tell using the 3280x2464 mode actually yields an
output resolution that is 3264x2448.

What does your hardware setup look like? And which revision of the
sensor are you using?

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Robert Foss]

>
> OV8856 sensor array region consists of 3 main window settings.
> The inner window is controlled by [H_win_off, V_win_off].
> From the old unusual 3280x2464 and 1640x1232 setting,
> H_win_off(R3810-R3811) is 0, V_win_off(R3812-R3813) is 1.
>
> Considering that the register TEST_PATTERN_CTRL(R4320) controlling pixel
> order is not set (default: 0x80, meaning BG/GR) and mirror/flip are both
> OFF, the absolute coordinate of crop_start is expressed as:
> [H_crop_start+H_win_off, V_crop_start+V_win_off] = [0, 7]
>
> Thus the first pixel shall start with G channel(according to datasheet).
> This is different with current resolutions (3264x2448 and 1632x1224).
>

Sakari: So this means that the patches introducing 3264x2448 and
1632x1224 modes really should have included code for configuring BGGR
format for those two specific modes only. Let me whip up another patch
for that, and put a pin in the bayer mode part of this conversation.

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Tomasz Figa]

Hi Robert,

On Tue, Nov 17, 2020 at 12:52 AM Robert Foss <robert.foss@linaro.org> wrote:
>
> Remove the 3280x2464 mode as it can't be reproduced and yields
> an output resolution of 3264x2448 instead of the desired one.
>
> Furthermore the 3264x2448 resolution is the highest resolution
> that the product brief lists.
>
> Since 3280x2464 neither works correctly nor seems to be supported
> by the sensor, let's remove it.
>

Let me check which modes are used by our projects. For one I'm sure
it's the 3264, but not sure about the other.

To be fair, 3280 sounds like a valid setup, with black pixels on the
edges. It's sometimes needed to add the black pixels either due to ISP
requirements or to obtain the black pixel values.

Best regards,
Tomasz

> Signed-off-by: Robert Foss <robert.foss@linaro.org>
> ---
>  drivers/media/i2c/ov8856.c | 202 -------------------------------------
>  1 file changed, 202 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 2f4ceaa80593..3365d19a303d 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -148,196 +148,6 @@ static const struct ov8856_reg mipi_data_rate_360mbps[] = {
>         {0x031e, 0x0c},
>  };
>
> -static const struct ov8856_reg mode_3280x2464_regs[] = {
> -       {0x3000, 0x20},
> -       {0x3003, 0x08},
> -       {0x300e, 0x20},
> -       {0x3010, 0x00},
> -       {0x3015, 0x84},
> -       {0x3018, 0x72},
> -       {0x3021, 0x23},
> -       {0x3033, 0x24},
> -       {0x3500, 0x00},
> -       {0x3501, 0x9a},
> -       {0x3502, 0x20},
> -       {0x3503, 0x08},
> -       {0x3505, 0x83},
> -       {0x3508, 0x01},
> -       {0x3509, 0x80},
> -       {0x350c, 0x00},
> -       {0x350d, 0x80},
> -       {0x350e, 0x04},
> -       {0x350f, 0x00},
> -       {0x3510, 0x00},
> -       {0x3511, 0x02},
> -       {0x3512, 0x00},
> -       {0x3600, 0x72},
> -       {0x3601, 0x40},
> -       {0x3602, 0x30},
> -       {0x3610, 0xc5},
> -       {0x3611, 0x58},
> -       {0x3612, 0x5c},
> -       {0x3613, 0xca},
> -       {0x3614, 0x20},
> -       {0x3628, 0xff},
> -       {0x3629, 0xff},
> -       {0x362a, 0xff},
> -       {0x3633, 0x10},
> -       {0x3634, 0x10},
> -       {0x3635, 0x10},
> -       {0x3636, 0x10},
> -       {0x3663, 0x08},
> -       {0x3669, 0x34},
> -       {0x366e, 0x10},
> -       {0x3706, 0x86},
> -       {0x370b, 0x7e},
> -       {0x3714, 0x23},
> -       {0x3730, 0x12},
> -       {0x3733, 0x10},
> -       {0x3764, 0x00},
> -       {0x3765, 0x00},
> -       {0x3769, 0x62},
> -       {0x376a, 0x2a},
> -       {0x376b, 0x30},
> -       {0x3780, 0x00},
> -       {0x3781, 0x24},
> -       {0x3782, 0x00},
> -       {0x3783, 0x23},
> -       {0x3798, 0x2f},
> -       {0x37a1, 0x60},
> -       {0x37a8, 0x6a},
> -       {0x37ab, 0x3f},
> -       {0x37c2, 0x04},
> -       {0x37c3, 0xf1},
> -       {0x37c9, 0x80},
> -       {0x37cb, 0x16},
> -       {0x37cc, 0x16},
> -       {0x37cd, 0x16},
> -       {0x37ce, 0x16},
> -       {0x3800, 0x00},
> -       {0x3801, 0x00},
> -       {0x3802, 0x00},
> -       {0x3803, 0x06},
> -       {0x3804, 0x0c},
> -       {0x3805, 0xdf},
> -       {0x3806, 0x09},
> -       {0x3807, 0xa7},
> -       {0x3808, 0x0c},
> -       {0x3809, 0xd0},
> -       {0x380a, 0x09},
> -       {0x380b, 0xa0},
> -       {0x380c, 0x07},
> -       {0x380d, 0x88},
> -       {0x380e, 0x09},
> -       {0x380f, 0xb8},
> -       {0x3810, 0x00},
> -       {0x3811, 0x00},
> -       {0x3812, 0x00},
> -       {0x3813, 0x01},
> -       {0x3814, 0x01},
> -       {0x3815, 0x01},
> -       {0x3816, 0x00},
> -       {0x3817, 0x00},
> -       {0x3818, 0x00},
> -       {0x3819, 0x10},
> -       {0x3820, 0x80},
> -       {0x3821, 0x46},
> -       {0x382a, 0x01},
> -       {0x382b, 0x01},
> -       {0x3830, 0x06},
> -       {0x3836, 0x02},
> -       {0x3862, 0x04},
> -       {0x3863, 0x08},
> -       {0x3cc0, 0x33},
> -       {0x3d85, 0x17},
> -       {0x3d8c, 0x73},
> -       {0x3d8d, 0xde},
> -       {0x4001, 0xe0},
> -       {0x4003, 0x40},
> -       {0x4008, 0x00},
> -       {0x4009, 0x0b},
> -       {0x400a, 0x00},
> -       {0x400b, 0x84},
> -       {0x400f, 0x80},
> -       {0x4010, 0xf0},
> -       {0x4011, 0xff},
> -       {0x4012, 0x02},
> -       {0x4013, 0x01},
> -       {0x4014, 0x01},
> -       {0x4015, 0x01},
> -       {0x4042, 0x00},
> -       {0x4043, 0x80},
> -       {0x4044, 0x00},
> -       {0x4045, 0x80},
> -       {0x4046, 0x00},
> -       {0x4047, 0x80},
> -       {0x4048, 0x00},
> -       {0x4049, 0x80},
> -       {0x4041, 0x03},
> -       {0x404c, 0x20},
> -       {0x404d, 0x00},
> -       {0x404e, 0x20},
> -       {0x4203, 0x80},
> -       {0x4307, 0x30},
> -       {0x4317, 0x00},
> -       {0x4503, 0x08},
> -       {0x4601, 0x80},
> -       {0x4800, 0x44},
> -       {0x4816, 0x53},
> -       {0x481b, 0x58},
> -       {0x481f, 0x27},
> -       {0x4837, 0x16},
> -       {0x483c, 0x0f},
> -       {0x484b, 0x05},
> -       {0x5000, 0x57},
> -       {0x5001, 0x0a},
> -       {0x5004, 0x04},
> -       {0x502e, 0x03},
> -       {0x5030, 0x41},
> -       {0x5780, 0x14},
> -       {0x5781, 0x0f},
> -       {0x5782, 0x44},
> -       {0x5783, 0x02},
> -       {0x5784, 0x01},
> -       {0x5785, 0x01},
> -       {0x5786, 0x00},
> -       {0x5787, 0x04},
> -       {0x5788, 0x02},
> -       {0x5789, 0x0f},
> -       {0x578a, 0xfd},
> -       {0x578b, 0xf5},
> -       {0x578c, 0xf5},
> -       {0x578d, 0x03},
> -       {0x578e, 0x08},
> -       {0x578f, 0x0c},
> -       {0x5790, 0x08},
> -       {0x5791, 0x04},
> -       {0x5792, 0x00},
> -       {0x5793, 0x52},
> -       {0x5794, 0xa3},
> -       {0x5795, 0x02},
> -       {0x5796, 0x20},
> -       {0x5797, 0x20},
> -       {0x5798, 0xd5},
> -       {0x5799, 0xd5},
> -       {0x579a, 0x00},
> -       {0x579b, 0x50},
> -       {0x579c, 0x00},
> -       {0x579d, 0x2c},
> -       {0x579e, 0x0c},
> -       {0x579f, 0x40},
> -       {0x57a0, 0x09},
> -       {0x57a1, 0x40},
> -       {0x59f8, 0x3d},
> -       {0x5a08, 0x02},
> -       {0x5b00, 0x02},
> -       {0x5b01, 0x10},
> -       {0x5b02, 0x03},
> -       {0x5b03, 0xcf},
> -       {0x5b05, 0x6c},
> -       {0x5e00, 0x00}
> -};
> -
>  static const struct ov8856_reg mode_3264x2448_regs[] = {
>         {0x0103, 0x01},
>         {0x0302, 0x3c},
> @@ -963,18 +773,6 @@ static const struct ov8856_link_freq_config link_freq_configs[] = {
>  };
>
>  static const struct ov8856_mode supported_modes[] = {
> -       {
> -               .width = 3280,
> -               .height = 2464,
> -               .hts = 1928,
> -               .vts_def = 2488,
> -               .vts_min = 2488,
> -               .reg_list = {
> -                       .num_of_regs = ARRAY_SIZE(mode_3280x2464_regs),
> -                       .regs = mode_3280x2464_regs,
> -               },
> -               .link_freq_index = OV8856_LINK_FREQ_720MBPS,
> -       },
>         {
>                 .width = 3264,
>                 .height = 2448,
> --
> 2.27.0
>

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Bingbu Cao]

On 11/24/20 6:20 PM, Robert Foss wrote:
> On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>>
>> Hi, Robert
>>
>> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
>> frames based on current settings.
>>
>> Do you have any issues with this mode?
> 
> As far as I can tell using the 3280x2464 mode actually yields an
> output resolution that is 3264x2448.
> 
> What does your hardware setup look like? And which revision of the
> sensor are you using?
> 

Robert, the sensor revision I am using is v1.1. I just checked the actual output pixels on our
hardware, the output resolution with 2464 mode is 3280x2464, no black pixels.

As Tomasz said, some ISP has the requirement of extra pixel padding, From the ov8856 datasheet,
the central 3264x2448 pixels are *suggested* to be output from the pixel array and the boundary
pixels can be used for additional processing. In my understanding, the 32 dummy lines are not
black lines.

-- 
Best regards,
Bingbu Cao

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Tomasz Figa]

Hi Bingbu,

On Wed, Nov 25, 2020 at 1:15 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
>
>
>
> On 11/24/20 6:20 PM, Robert Foss wrote:
> > On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> >>
> >> Hi, Robert
> >>
> >> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
> >> frames based on current settings.
> >>
> >> Do you have any issues with this mode?
> >
> > As far as I can tell using the 3280x2464 mode actually yields an
> > output resolution that is 3264x2448.
> >
> > What does your hardware setup look like? And which revision of the
> > sensor are you using?
> >
>
> Robert, the sensor revision I am using is v1.1. I just checked the actual output pixels on our
> hardware, the output resolution with 2464 mode is 3280x2464, no black pixels.
>
> As Tomasz said, some ISP has the requirement of extra pixel padding, From the ov8856 datasheet,
> the central 3264x2448 pixels are *suggested* to be output from the pixel array and the boundary
> pixels can be used for additional processing. In my understanding, the 32 dummy lines are not
> black lines.

The datasheet says that only 3264x2448 are active pixels. What pixel
values are you seeing outside of that central area? In the datasheet,
those look like "optically black" pixels, which are not 100% black,
but rather as if the sensor cells didn't receive any light - noise can
be still there.

Best regards,
Tomasz

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Robert Foss]

On Wed, 25 Nov 2020 at 08:32, Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Bingbu,
>
> On Wed, Nov 25, 2020 at 1:15 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> >
> >
> >
> > On 11/24/20 6:20 PM, Robert Foss wrote:
> > > On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > >>
> > >> Hi, Robert
> > >>
> > >> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
> > >> frames based on current settings.
> > >>
> > >> Do you have any issues with this mode?
> > >
> > > As far as I can tell using the 3280x2464 mode actually yields an
> > > output resolution that is 3264x2448.
> > >
> > > What does your hardware setup look like? And which revision of the
> > > sensor are you using?
> > >
> >
> > Robert, the sensor revision I am using is v1.1. I just checked the actual output pixels on our
> > hardware, the output resolution with 2464 mode is 3280x2464, no black pixels.
> >
> > As Tomasz said, some ISP has the requirement of extra pixel padding, From the ov8856 datasheet,
> > the central 3264x2448 pixels are *suggested* to be output from the pixel array and the boundary
> > pixels can be used for additional processing. In my understanding, the 32 dummy lines are not
> > black lines.
>
> The datasheet says that only 3264x2448 are active pixels. What pixel
> values are you seeing outside of that central area? In the datasheet,
> those look like "optically black" pixels, which are not 100% black,
> but rather as if the sensor cells didn't receive any light - noise can
> be still there.
>

I've been developing support for some Qcom ISP functionality, and
during the course of this I ran into the issue I was describing, where
the 3280x2464 mode actually outputs 3264x2448.

I can think of two reasons for this, either ISP driver bugs on my end
or the fact that the sensor is being run outside of the specification
and which could be resulting in differences between how the ov8856
sensors behave.

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Tomasz Figa]

On Thu, Nov 26, 2020 at 7:00 PM Robert Foss <robert.foss@linaro.org> wrote:
>
> On Wed, 25 Nov 2020 at 08:32, Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Bingbu,
> >
> > On Wed, Nov 25, 2020 at 1:15 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > >
> > >
> > >
> > > On 11/24/20 6:20 PM, Robert Foss wrote:
> > > > On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > >>
> > > >> Hi, Robert
> > > >>
> > > >> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
> > > >> frames based on current settings.
> > > >>
> > > >> Do you have any issues with this mode?
> > > >
> > > > As far as I can tell using the 3280x2464 mode actually yields an
> > > > output resolution that is 3264x2448.
> > > >
> > > > What does your hardware setup look like? And which revision of the
> > > > sensor are you using?
> > > >
> > >
> > > Robert, the sensor revision I am using is v1.1. I just checked the actual output pixels on our
> > > hardware, the output resolution with 2464 mode is 3280x2464, no black pixels.
> > >
> > > As Tomasz said, some ISP has the requirement of extra pixel padding, From the ov8856 datasheet,
> > > the central 3264x2448 pixels are *suggested* to be output from the pixel array and the boundary
> > > pixels can be used for additional processing. In my understanding, the 32 dummy lines are not
> > > black lines.
> >
> > The datasheet says that only 3264x2448 are active pixels. What pixel
> > values are you seeing outside of that central area? In the datasheet,
> > those look like "optically black" pixels, which are not 100% black,
> > but rather as if the sensor cells didn't receive any light - noise can
> > be still there.
> >
>
> I've been developing support for some Qcom ISP functionality, and
> during the course of this I ran into the issue I was describing, where
> the 3280x2464 mode actually outputs 3264x2448.
>
> I can think of two reasons for this, either ISP driver bugs on my end
> or the fact that the sensor is being run outside of the specification
> and which could be resulting in differences between how the ov8856
> sensors behave.

I just confirmed and we're indeed using this mode in a number of our
projects based on the Intel ISP and it seems to be producing a proper
image with all pixels of the 3280x2464 matrix having proper values.
I'm now double checking whether this isn't some processing done by the
ISP, but I suspect the quality would be bad if it stretched the
central 3264x2448 part into the 3280x2464 frame.

Best regards,
Tomasz

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Robert Foss]

Thanks for digging into this everyone!

Assuming Tomasz doesn't find any stretching, I think we can conclude
that this mode works, and should be kept. Thanks Dongchun for parsing
the datasheet and finding the Bayer mode issue for the two other
recently added resolutions.

On Fri, 27 Nov 2020 at 11:26, Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Thu, Nov 26, 2020 at 7:00 PM Robert Foss <robert.foss@linaro.org> wrote:
> >
> > On Wed, 25 Nov 2020 at 08:32, Tomasz Figa <tfiga@chromium.org> wrote:
> > >
> > > Hi Bingbu,
> > >
> > > On Wed, Nov 25, 2020 at 1:15 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > >
> > > >
> > > >
> > > > On 11/24/20 6:20 PM, Robert Foss wrote:
> > > > > On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > > >>
> > > > >> Hi, Robert
> > > > >>
> > > > >> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
> > > > >> frames based on current settings.
> > > > >>
> > > > >> Do you have any issues with this mode?
> > > > >
> > > > > As far as I can tell using the 3280x2464 mode actually yields an
> > > > > output resolution that is 3264x2448.
> > > > >
> > > > > What does your hardware setup look like? And which revision of the
> > > > > sensor are you using?
> > > > >
> > > >
> > > > Robert, the sensor revision I am using is v1.1. I just checked the actual output pixels on our
> > > > hardware, the output resolution with 2464 mode is 3280x2464, no black pixels.
> > > >
> > > > As Tomasz said, some ISP has the requirement of extra pixel padding, From the ov8856 datasheet,
> > > > the central 3264x2448 pixels are *suggested* to be output from the pixel array and the boundary
> > > > pixels can be used for additional processing. In my understanding, the 32 dummy lines are not
> > > > black lines.
> > >
> > > The datasheet says that only 3264x2448 are active pixels. What pixel
> > > values are you seeing outside of that central area? In the datasheet,
> > > those look like "optically black" pixels, which are not 100% black,
> > > but rather as if the sensor cells didn't receive any light - noise can
> > > be still there.
> > >
> >
> > I've been developing support for some Qcom ISP functionality, and
> > during the course of this I ran into the issue I was describing, where
> > the 3280x2464 mode actually outputs 3264x2448.
> >
> > I can think of two reasons for this, either ISP driver bugs on my end
> > or the fact that the sensor is being run outside of the specification
> > and which could be resulting in differences between how the ov8856
> > sensors behave.
>
> I just confirmed and we're indeed using this mode in a number of our
> projects based on the Intel ISP and it seems to be producing a proper
> image with all pixels of the 3280x2464 matrix having proper values.
> I'm now double checking whether this isn't some processing done by the
> ISP, but I suspect the quality would be bad if it stretched the
> central 3264x2448 part into the 3280x2464 frame.
>
> Best regards,
> Tomasz

Re: [PATCH] media: ov8856: Remove 3280x2464 mode

[Tomasz Figa]

On Fri, Nov 27, 2020 at 10:38 PM Robert Foss <robert.foss@linaro.org> wrote:
>
> Thanks for digging into this everyone!
>
> Assuming Tomasz doesn't find any stretching, I think we can conclude
> that this mode works, and should be kept. Thanks Dongchun for parsing
> the datasheet and finding the Bayer mode issue for the two other
> recently added resolutions.

I checked the raw output and it actually seems to have 3296x2464
non-black pixels. The rightmost 16 ones seem a copy of the ones from
3248. That might be just some padding from the output DMA, though.

Generally all the datasheets I've seen still suggest that only the
middle 3264x2448 are active pixels to be output, so this warrants
double checking this with Omnivision. Let me see what we can do about
this.

Best regards,
Tomasz

>
> On Fri, 27 Nov 2020 at 11:26, Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > On Thu, Nov 26, 2020 at 7:00 PM Robert Foss <robert.foss@linaro.org> wrote:
> > >
> > > On Wed, 25 Nov 2020 at 08:32, Tomasz Figa <tfiga@chromium.org> wrote:
> > > >
> > > > Hi Bingbu,
> > > >
> > > > On Wed, Nov 25, 2020 at 1:15 PM Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > On 11/24/20 6:20 PM, Robert Foss wrote:
> > > > > > On Tue, 24 Nov 2020 at 10:42, Bingbu Cao <bingbu.cao@linux.intel.com> wrote:
> > > > > >>
> > > > > >> Hi, Robert
> > > > > >>
> > > > > >> I remember that the full size of ov8856 image sensor is 3296x2480 and we can get the 3280x2464
> > > > > >> frames based on current settings.
> > > > > >>
> > > > > >> Do you have any issues with this mode?
> > > > > >
> > > > > > As far as I can tell using the 3280x2464 mode actually yields an
> > > > > > output resolution that is 3264x2448.
> > > > > >
> > > > > > What does your hardware setup look like? And which revision of the
> > > > > > sensor are you using?
> > > > > >
> > > > >
> > > > > Robert, the sensor revision I am using is v1.1. I just checked the actual output pixels on our
> > > > > hardware, the output resolution with 2464 mode is 3280x2464, no black pixels.
> > > > >
> > > > > As Tomasz said, some ISP has the requirement of extra pixel padding, From the ov8856 datasheet,
> > > > > the central 3264x2448 pixels are *suggested* to be output from the pixel array and the boundary
> > > > > pixels can be used for additional processing. In my understanding, the 32 dummy lines are not
> > > > > black lines.
> > > >
> > > > The datasheet says that only 3264x2448 are active pixels. What pixel
> > > > values are you seeing outside of that central area? In the datasheet,
> > > > those look like "optically black" pixels, which are not 100% black,
> > > > but rather as if the sensor cells didn't receive any light - noise can
> > > > be still there.
> > > >
> > >
> > > I've been developing support for some Qcom ISP functionality, and
> > > during the course of this I ran into the issue I was describing, where
> > > the 3280x2464 mode actually outputs 3264x2448.
> > >
> > > I can think of two reasons for this, either ISP driver bugs on my end
> > > or the fact that the sensor is being run outside of the specification
> > > and which could be resulting in differences between how the ov8856
> > > sensors behave.
> >
> > I just confirmed and we're indeed using this mode in a number of our
> > projects based on the Intel ISP and it seems to be producing a proper
> > image with all pixels of the 3280x2464 matrix having proper values.
> > I'm now double checking whether this isn't some processing done by the
> > ISP, but I suspect the quality would be bad if it stretched the
> > central 3264x2448 part into the 3280x2464 frame.
> >
> > Best regards,
> > Tomasz