[PATCH 0/4] media: staging: rkisp1: fixes related to the uapi lsc struct

[PATCH 0/4] media: staging: rkisp1: fixes related to the uapi lsc struct

[Dafna Hirschfeld]

Fix several issues related to the struct rkisp1_cif_isp_lsc_config
defined in:
drivers/staging/media/rkisp1/uapi/rkisp1-config.h

The issues are mentioned in comments from Tomasz Figa:
https://patchwork.kernel.org/patch/10267411/#21766475

Dafna Hirschfeld (4):
  media: staging: rkisp1: remove two unused fields in uapi struct
  media: staging: rkisp1: set "*_data_tbl" fields in
    rkisp1_cif_isp_lsc_config as 2D arries
  media: staging: rkisp1: replace to identical macros with one
  media: staging: rkisp1: change type of fields in
    'rkisp1_cif_isp_lsc_config' from u32 to u16

 drivers/staging/media/rkisp1/rkisp1-params.c  | 32 +++++++++----------
 drivers/staging/media/rkisp1/rkisp1-regs.h    |  1 -
 .../staging/media/rkisp1/uapi/rkisp1-config.h | 29 ++++++++---------
 3 files changed, 28 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH 1/4] media: staging: rkisp1: remove two unused fields in uapi struct

[Dafna Hirschfeld]

The fields 'config_width', 'config_height' in struct
'rkisp1_cif_isp_lsc_config' are not used by the driver and
therefore are not needed. This patch removes them.
In later patch, documentation of the fields in struct
'rkisp1_cif_isp_lsc_config' will be added.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
index ca0d031b14ac..7331bacf7dfd 100644
--- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
+++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
@@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
 
 	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
 	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
-	__u16 config_width;
-	__u16 config_height;
 } __packed;
 
 /**
-- 
2.17.1

[PATCH 2/4] media: staging: rkisp1: set "*_data_tbl" fields in rkisp1_cif_isp_lsc_config as 2D arries

[Dafna Hirschfeld]

Currently the fields '*_data_tbl' in the struct
'rkisp1_cif_isp_lsc_config' are one dimensional arries
of size 290. The entries are accessed in a nested
loop by summing two indexes for the rows and columns.
Therefore it fits better to define the arries as two
dimensional arries of size 17x17.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c  | 30 +++++++++----------
 drivers/staging/media/rkisp1/rkisp1-regs.h    |  1 -
 .../staging/media/rkisp1/uapi/rkisp1-config.h | 11 ++++---
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 5169b02731f1..5be57426d940 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -206,47 +206,45 @@ rkisp1_lsc_correct_matrix_config(struct rkisp1_params *params,
 		     RKISP1_CIF_ISP_LSC_B_TABLE_ADDR);
 
 	/* program data tables (table size is 9 * 17 = 153) */
-	for (i = 0;
-	     i < RKISP1_CIF_ISP_LSC_SECTORS_MAX * RKISP1_CIF_ISP_LSC_SECTORS_MAX;
-	     i += RKISP1_CIF_ISP_LSC_SECTORS_MAX) {
+	for (i = 0; i < RKISP1_CIF_ISP_LSC_SAMPLES_MAX; i++) {
 		/*
 		 * 17 sectors with 2 values in one DWORD = 9
 		 * DWORDs (2nd value of last DWORD unused)
 		 */
-		for (j = 0; j < RKISP1_CIF_ISP_LSC_SECTORS_MAX - 1; j += 2) {
-			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i + j],
-							     pconfig->r_data_tbl[i + j + 1]);
+		for (j = 0; j < RKISP1_CIF_ISP_LSC_SAMPLES_MAX - 1; j += 2) {
+			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i][j],
+							     pconfig->r_data_tbl[i][j + 1]);
 			rkisp1_write(params->rkisp1, data,
 				     RKISP1_CIF_ISP_LSC_R_TABLE_DATA);
 
-			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i + j],
-							     pconfig->gr_data_tbl[i + j + 1]);
+			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i][j],
+							     pconfig->gr_data_tbl[i][j + 1]);
 			rkisp1_write(params->rkisp1, data,
 				     RKISP1_CIF_ISP_LSC_GR_TABLE_DATA);
 
-			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i + j],
-							     pconfig->gb_data_tbl[i + j + 1]);
+			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i][j],
+							     pconfig->gb_data_tbl[i][j + 1]);
 			rkisp1_write(params->rkisp1, data,
 				     RKISP1_CIF_ISP_LSC_GB_TABLE_DATA);
 
-			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i + j],
-							     pconfig->b_data_tbl[i + j + 1]);
+			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i][j],
+							     pconfig->b_data_tbl[i][j + 1]);
 			rkisp1_write(params->rkisp1, data,
 				     RKISP1_CIF_ISP_LSC_B_TABLE_DATA);
 		}
-		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i + j], 0);
+		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i][j], 0);
 		rkisp1_write(params->rkisp1, data,
 			     RKISP1_CIF_ISP_LSC_R_TABLE_DATA);
 
-		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i + j], 0);
+		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i][j], 0);
 		rkisp1_write(params->rkisp1, data,
 			     RKISP1_CIF_ISP_LSC_GR_TABLE_DATA);
 
-		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i + j], 0);
+		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i][j], 0);
 		rkisp1_write(params->rkisp1, data,
 			     RKISP1_CIF_ISP_LSC_GB_TABLE_DATA);
 
-		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i + j], 0);
+		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i][j], 0);
 		rkisp1_write(params->rkisp1, data,
 			     RKISP1_CIF_ISP_LSC_B_TABLE_DATA);
 	}
diff --git a/drivers/staging/media/rkisp1/rkisp1-regs.h b/drivers/staging/media/rkisp1/rkisp1-regs.h
index 46018f435b6f..6667147149be 100644
--- a/drivers/staging/media/rkisp1/rkisp1-regs.h
+++ b/drivers/staging/media/rkisp1/rkisp1-regs.h
@@ -476,7 +476,6 @@
 #define RKISP1_CIF_ISP_LSC_SECT_SIZE_RESERVED		0xFC00FC00
 #define RKISP1_CIF_ISP_LSC_GRAD_RESERVED		0xF000F000
 #define RKISP1_CIF_ISP_LSC_SAMPLE_RESERVED		0xF000F000
-#define RKISP1_CIF_ISP_LSC_SECTORS_MAX			17
 #define RKISP1_CIF_ISP_LSC_TABLE_DATA(v0, v1)     \
 	(((v0) & 0xFFF) | (((v1) & 0xFFF) << 12))
 #define RKISP1_CIF_ISP_LSC_SECT_SIZE(v0, v1)      \
diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
index 7331bacf7dfd..d8a8e4457aa4 100644
--- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
+++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
@@ -87,9 +87,8 @@
 /*
  * The following matches the tuning process,
  * not the max capabilities of the chip.
- * Last value unused.
  */
-#define	RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE           290
+#define RKISP1_CIF_ISP_LSC_SAMPLES_MAX             17
 
 /*
  * Histogram calculation
@@ -275,10 +274,10 @@ struct rkisp1_cif_isp_sdg_config {
  * refer to REF_01 for details
  */
 struct rkisp1_cif_isp_lsc_config {
-	__u32 r_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
-	__u32 gr_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
-	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
-	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
+	__u32 r_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u32 gr_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
 
 	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
 	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
-- 
2.17.1

[PATCH 3/4] media: staging: rkisp1: replace two identical macros with one

[Dafna Hirschfeld]

The two macros RKISP1_CIF_ISP_LSC_{GRAD/SIZE}_TBL_SIZE have
the same value which is the number of sectors in the grads
and size parameters. Therefore there is no need for two
different macros. Replace them with one macro
RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE. Also use the macro
when iterating the arries instead of hardcoded '4'.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-params.c      |  2 +-
 drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 12 ++++++------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 5be57426d940..ea55532ab797 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -267,7 +267,7 @@ static void rkisp1_lsc_config(struct rkisp1_params *params,
 				RKISP1_CIF_ISP_LSC_CTRL_ENA);
 	rkisp1_lsc_correct_matrix_config(params, arg);
 
-	for (i = 0; i < 4; i++) {
+	for (i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE / 2; i++) {
 		/* program x size tables */
 		data = RKISP1_CIF_ISP_LSC_SECT_SIZE(arg->x_size_tbl[i * 2],
 						    arg->x_size_tbl[i * 2 + 1]);
diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
index d8a8e4457aa4..bca2e805c0f5 100644
--- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
+++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
@@ -82,8 +82,8 @@
 /*
  * Lens shade correction
  */
-#define RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE           8
-#define RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE           8
+#define RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE        8
+
 /*
  * The following matches the tuning process,
  * not the max capabilities of the chip.
@@ -279,11 +279,11 @@ struct rkisp1_cif_isp_lsc_config {
 	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
 	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
 
-	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
-	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
+	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
 
-	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
-	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
+	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
 } __packed;
 
 /**
-- 
2.17.1

[PATCH 4/4] media: staging: rkisp1: change type of fields in 'rkisp1_cif_isp_lsc_config' from u32 to u16

[Dafna Hirschfeld]

The fields in struct 'rkisp1_cif_isp_lsc_config' need to hold
values of 12 bits, therefore it is enough to declare them as u16.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../staging/media/rkisp1/uapi/rkisp1-config.h    | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
index bca2e805c0f5..c032c8403f17 100644
--- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
+++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
@@ -274,16 +274,16 @@ struct rkisp1_cif_isp_sdg_config {
  * refer to REF_01 for details
  */
 struct rkisp1_cif_isp_lsc_config {
-	__u32 r_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
-	__u32 gr_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
-	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
-	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u16 r_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u16 gr_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u16 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
+	__u16 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
 
-	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
-	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	__u16 x_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	__u16 y_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
 
-	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
-	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	__u16 x_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	__u16 y_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
 } __packed;
 
 /**
-- 
2.17.1

Re: [PATCH 1/4] media: staging: rkisp1: remove two unused fields in uapi struct

[Ezequiel Garcia]

Hi Dafna,

Thanks for all the rkisp1 patches, I hope we can get
the driver out of staging soon.

On Thu, 2020-06-25 at 20:50 +0200, Dafna Hirschfeld wrote:
> The fields 'config_width', 'config_height' in struct
> 'rkisp1_cif_isp_lsc_config' are not used by the driver and
> therefore are not needed. This patch removes them.
> In later patch, documentation of the fields in struct
> 'rkisp1_cif_isp_lsc_config' will be added.
> 

If I understand correctly, you are affecting the uAPI here.

The fact that the driver doesn't use it now, doesn't mean
it won't need it at some later point.

I'm not saying the change is wrong, but that the "not currently
in use" reason might be insufficient for a uAPI. If you
want to remove this field, I suggest you make sure
the field is inappropriate for this interface.

Also, it would be better if you could add a cover letter
on all the series, there are a bunch of rkisp1 patches now
and having a cover letter helps by adding some context.

Thanks,
Ezequiel

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index ca0d031b14ac..7331bacf7dfd 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
>  
>  	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>  	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
> -	__u16 config_width;
> -	__u16 config_height;
>  } __packed;
>  
>  /**

Re: [PATCH 1/4] media: staging: rkisp1: remove two unused fields in uapi struct

[Dafna Hirschfeld]

Hi,

On 26.06.20 14:47, Ezequiel Garcia wrote:
> Hi Dafna,
> 
> Thanks for all the rkisp1 patches, I hope we can get
> the driver out of staging soon.

me too.

> 
> On Thu, 2020-06-25 at 20:50 +0200, Dafna Hirschfeld wrote:
>> The fields 'config_width', 'config_height' in struct
>> 'rkisp1_cif_isp_lsc_config' are not used by the driver and
>> therefore are not needed. This patch removes them.
>> In later patch, documentation of the fields in struct
>> 'rkisp1_cif_isp_lsc_config' will be added.
>>
> 
> If I understand correctly, you are affecting the uAPI here.
> 
> The fact that the driver doesn't use it now, doesn't mean
> it won't need it at some later point.
> 
> I'm not saying the change is wrong, but that the "not currently
> in use" reason might be insufficient for a uAPI. If you
> want to remove this field, I suggest you make sure
> the field is inappropriate for this interface.
> 
I looked at the documentation and didn't find
any width/height values related to the LSC interface.
So I it seems to be ok to remove those fields.

> Also, it would be better if you could add a cover letter
> on all the series, there are a bunch of rkisp1 patches now
> and having a cover letter helps by adding some context.
> 

I did add a cover-letter: https://patchwork.kernel.org/cover/11625921/

Thanks,
Dafna

> Thanks,
> Ezequiel
> 
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> index ca0d031b14ac..7331bacf7dfd 100644
>> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> @@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
>>   
>>   	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>>   	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>> -	__u16 config_width;
>> -	__u16 config_height;
>>   } __packed;
>>   
>>   /**
> 
> 

Re: [PATCH 1/4] media: staging: rkisp1: remove two unused fields in uapi struct

[Helen Koike]

Hi Dafna,


On 6/25/20 3:50 PM, Dafna Hirschfeld wrote:
> The fields 'config_width', 'config_height' in struct
> 'rkisp1_cif_isp_lsc_config' are not used by the driver and
> therefore are not needed. This patch removes them.
> In later patch, documentation of the fields in struct
> 'rkisp1_cif_isp_lsc_config' will be added.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index ca0d031b14ac..7331bacf7dfd 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -285,8 +285,6 @@ struct rkisp1_cif_isp_lsc_config {
>  
>  	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
>  	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
> -	__u16 config_width;
> -	__u16 config_height;
>  } __packed;
>  
>  /**
> 


I was checking the Rockchip 3A library API, and they use these fields on:

https://github.com/rockchip-linux/camera_engine_rkisp/blob/master/rkisp/ia-engine/include/ia/rk_aiq_types.h#L409

Which I'm not sure what it means tbh, so I would rather leave these fields (with a comment to say it is not used
by the driver) just in case it is useful in the future.
Ideally we should try to understand what this field does to make a decision.

Regards,
Helen

Re: [PATCH 2/4] media: staging: rkisp1: set "*_data_tbl" fields in rkisp1_cif_isp_lsc_config as 2D arries

[Helen Koike]

Hi Dafna,

On 6/25/20 3:50 PM, Dafna Hirschfeld wrote:
> Currently the fields '*_data_tbl' in the struct
> 'rkisp1_cif_isp_lsc_config' are one dimensional arries
> of size 290. The entries are accessed in a nested
> loop by summing two indexes for the rows and columns.
> Therefore it fits better to define the arries as two
> dimensional arries of size 17x17.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Tested on Scarlet Chromebook with ChromeOS stack.

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c  | 30 +++++++++----------
>  drivers/staging/media/rkisp1/rkisp1-regs.h    |  1 -
>  .../staging/media/rkisp1/uapi/rkisp1-config.h | 11 ++++---
>  3 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 5169b02731f1..5be57426d940 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -206,47 +206,45 @@ rkisp1_lsc_correct_matrix_config(struct rkisp1_params *params,
>  		     RKISP1_CIF_ISP_LSC_B_TABLE_ADDR);
>  
>  	/* program data tables (table size is 9 * 17 = 153) */
> -	for (i = 0;
> -	     i < RKISP1_CIF_ISP_LSC_SECTORS_MAX * RKISP1_CIF_ISP_LSC_SECTORS_MAX;
> -	     i += RKISP1_CIF_ISP_LSC_SECTORS_MAX) {
> +	for (i = 0; i < RKISP1_CIF_ISP_LSC_SAMPLES_MAX; i++) {
>  		/*
>  		 * 17 sectors with 2 values in one DWORD = 9
>  		 * DWORDs (2nd value of last DWORD unused)
>  		 */
> -		for (j = 0; j < RKISP1_CIF_ISP_LSC_SECTORS_MAX - 1; j += 2) {
> -			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i + j],
> -							     pconfig->r_data_tbl[i + j + 1]);
> +		for (j = 0; j < RKISP1_CIF_ISP_LSC_SAMPLES_MAX - 1; j += 2) {
> +			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i][j],
> +							     pconfig->r_data_tbl[i][j + 1]);
>  			rkisp1_write(params->rkisp1, data,
>  				     RKISP1_CIF_ISP_LSC_R_TABLE_DATA);
>  
> -			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i + j],
> -							     pconfig->gr_data_tbl[i + j + 1]);
> +			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i][j],
> +							     pconfig->gr_data_tbl[i][j + 1]);
>  			rkisp1_write(params->rkisp1, data,
>  				     RKISP1_CIF_ISP_LSC_GR_TABLE_DATA);
>  
> -			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i + j],
> -							     pconfig->gb_data_tbl[i + j + 1]);
> +			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i][j],
> +							     pconfig->gb_data_tbl[i][j + 1]);
>  			rkisp1_write(params->rkisp1, data,
>  				     RKISP1_CIF_ISP_LSC_GB_TABLE_DATA);
>  
> -			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i + j],
> -							     pconfig->b_data_tbl[i + j + 1]);
> +			data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i][j],
> +							     pconfig->b_data_tbl[i][j + 1]);
>  			rkisp1_write(params->rkisp1, data,
>  				     RKISP1_CIF_ISP_LSC_B_TABLE_DATA);
>  		}
> -		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i + j], 0);
> +		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->r_data_tbl[i][j], 0);
>  		rkisp1_write(params->rkisp1, data,
>  			     RKISP1_CIF_ISP_LSC_R_TABLE_DATA);
>  
> -		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i + j], 0);
> +		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gr_data_tbl[i][j], 0);
>  		rkisp1_write(params->rkisp1, data,
>  			     RKISP1_CIF_ISP_LSC_GR_TABLE_DATA);
>  
> -		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i + j], 0);
> +		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->gb_data_tbl[i][j], 0);
>  		rkisp1_write(params->rkisp1, data,
>  			     RKISP1_CIF_ISP_LSC_GB_TABLE_DATA);
>  
> -		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i + j], 0);
> +		data = RKISP1_CIF_ISP_LSC_TABLE_DATA(pconfig->b_data_tbl[i][j], 0);
>  		rkisp1_write(params->rkisp1, data,
>  			     RKISP1_CIF_ISP_LSC_B_TABLE_DATA);
>  	}
> diff --git a/drivers/staging/media/rkisp1/rkisp1-regs.h b/drivers/staging/media/rkisp1/rkisp1-regs.h
> index 46018f435b6f..6667147149be 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-regs.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-regs.h
> @@ -476,7 +476,6 @@
>  #define RKISP1_CIF_ISP_LSC_SECT_SIZE_RESERVED		0xFC00FC00
>  #define RKISP1_CIF_ISP_LSC_GRAD_RESERVED		0xF000F000
>  #define RKISP1_CIF_ISP_LSC_SAMPLE_RESERVED		0xF000F000
> -#define RKISP1_CIF_ISP_LSC_SECTORS_MAX			17
>  #define RKISP1_CIF_ISP_LSC_TABLE_DATA(v0, v1)     \
>  	(((v0) & 0xFFF) | (((v1) & 0xFFF) << 12))
>  #define RKISP1_CIF_ISP_LSC_SECT_SIZE(v0, v1)      \
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index 7331bacf7dfd..d8a8e4457aa4 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -87,9 +87,8 @@
>  /*
>   * The following matches the tuning process,
>   * not the max capabilities of the chip.
> - * Last value unused.
>   */
> -#define	RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE           290
> +#define RKISP1_CIF_ISP_LSC_SAMPLES_MAX             17
>  
>  /*
>   * Histogram calculation
> @@ -275,10 +274,10 @@ struct rkisp1_cif_isp_sdg_config {
>   * refer to REF_01 for details
>   */
>  struct rkisp1_cif_isp_lsc_config {
> -	__u32 r_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
> -	__u32 gr_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
> -	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
> -	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_DATA_TBL_SIZE];
> +	__u32 r_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u32 gr_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
>  
>  	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
>  	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
> 

Re: [PATCH 3/4] media: staging: rkisp1: replace two identical macros with one

[Helen Koike]

On 6/25/20 3:50 PM, Dafna Hirschfeld wrote:
> The two macros RKISP1_CIF_ISP_LSC_{GRAD/SIZE}_TBL_SIZE have
> the same value which is the number of sectors in the grads
> and size parameters. Therefore there is no need for two
> different macros. Replace them with one macro
> RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE. Also use the macro
> when iterating the arries instead of hardcoded '4'.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-params.c      |  2 +-
>  drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 12 ++++++------
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 5be57426d940..ea55532ab797 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -267,7 +267,7 @@ static void rkisp1_lsc_config(struct rkisp1_params *params,
>  				RKISP1_CIF_ISP_LSC_CTRL_ENA);
>  	rkisp1_lsc_correct_matrix_config(params, arg);
>  
> -	for (i = 0; i < 4; i++) {
> +	for (i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE / 2; i++) {
>  		/* program x size tables */
>  		data = RKISP1_CIF_ISP_LSC_SECT_SIZE(arg->x_size_tbl[i * 2],
>  						    arg->x_size_tbl[i * 2 + 1]);
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index d8a8e4457aa4..bca2e805c0f5 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -82,8 +82,8 @@
>  /*
>   * Lens shade correction
>   */
> -#define RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE           8
> -#define RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE           8
> +#define RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE        8
> +
>  /*
>   * The following matches the tuning process,
>   * not the max capabilities of the chip.
> @@ -279,11 +279,11 @@ struct rkisp1_cif_isp_lsc_config {
>  	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
>  	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
>  
> -	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
> -	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_GRAD_TBL_SIZE];
> +	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
>  
> -	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
> -	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SIZE_TBL_SIZE];
> +	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
>  } __packed;
>  
>  /**
> 

Re: [PATCH 4/4] media: staging: rkisp1: change type of fields in 'rkisp1_cif_isp_lsc_config' from u32 to u16

[Helen Koike]

On 6/25/20 3:50 PM, Dafna Hirschfeld wrote:
> The fields in struct 'rkisp1_cif_isp_lsc_config' need to hold
> values of 12 bits, therefore it is enough to declare them as u16.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

I see this comes from:

#define RKISP1_CIF_ISP_LSC_TABLE_DATA(v0, v1)     \
	(((v0) & 0xFFF) | (((v1) & 0xFFF) << 12))

That is used in those fields.

Acked-by: Helen Koike <helen.koike@collabora.com>

On a side note, it would be nice to update this header in libcamera as well.

Thanks
Helen

> ---
>  .../staging/media/rkisp1/uapi/rkisp1-config.h    | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index bca2e805c0f5..c032c8403f17 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -274,16 +274,16 @@ struct rkisp1_cif_isp_sdg_config {
>   * refer to REF_01 for details
>   */
>  struct rkisp1_cif_isp_lsc_config {
> -	__u32 r_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> -	__u32 gr_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> -	__u32 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> -	__u32 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u16 r_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u16 gr_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u16 gb_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
> +	__u16 b_data_tbl[RKISP1_CIF_ISP_LSC_SAMPLES_MAX][RKISP1_CIF_ISP_LSC_SAMPLES_MAX];
>  
> -	__u32 x_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> -	__u32 y_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	__u16 x_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	__u16 y_grad_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
>  
> -	__u32 x_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> -	__u32 y_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	__u16 x_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	__u16 y_size_tbl[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
>  } __packed;
>  
>  /**
>