diff mbox series

[v8,4/6] media: rockchip: rkisp1: reduce number of histogram grid elements in uapi

Message ID 20210121144407.9045-5-dafna.hirschfeld@collabora.com (mailing list archive)
State New, archived
Headers show
Series Fix the rkisp1 userspace API for later IP versions | expand

Commit Message

Dafna Hirschfeld Jan. 21, 2021, 2:44 p.m. UTC 
From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The uapi right now specifies an array size of 28 but the actual number
of elements is only 25 with the last 3 being unused.

Reduce the array size to the correct number of elements and change
the params code to iterate the array 25 times.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 ++-
 include/uapi/linux/rkisp1-config.h                     | 3 +--
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Helen Koike Jan. 21, 2021, 5:04 p.m. UTC | #1 
Hi Dafna,

On 1/21/21 11:44 AM, Dafna Hirschfeld wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> The uapi right now specifies an array size of 28 but the actual number
> of elements is only 25 with the last 3 being unused.
> 
> Reduce the array size to the correct number of elements and change
> the params code to iterate the array 25 times.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 ++-
>  include/uapi/linux/rkisp1-config.h                     | 3 +--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 6af4d551ffb5..021939466b24 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -589,7 +589,6 @@ static void rkisp1_hst_config(struct rkisp1_params *params,
>  		RKISP1_CIF_ISP_HIST_WEIGHT_22TO03,
>  		RKISP1_CIF_ISP_HIST_WEIGHT_13TO43,
>  		RKISP1_CIF_ISP_HIST_WEIGHT_04TO34,
> -		RKISP1_CIF_ISP_HIST_WEIGHT_44,
>  	};
>  	const u8 *weight;
>  	unsigned int i;
> @@ -622,6 +621,8 @@ static void rkisp1_hst_config(struct rkisp1_params *params,
>  							    weight[2],
>  							    weight[3]),
>  				 hist_weight_regs[i]);
> +
> +	rkisp1_write(params->rkisp1, weight[0] & 0x1F, RKISP1_CIF_ISP_HIST_WEIGHT_44);

I just noticed that the default value to arg->hist_weight was:

	memset(hst.hist_weight, 0x01, sizeof(hst.hist_weight));

Shouldn't the value be RKISP1_CIF_ISP_HIST_WEIGHT_SET(weight[0], 0x01, 0x01, 0x01) ?
Or am I missing something?

Thanks
Helen


>  }
>  
>  static void
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 24f05d6d258f..35aa82d5f6dd 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -102,8 +102,7 @@
>  /*
>   * Histogram calculation
>   */
> -/* Last 3 values unused. */
> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 25
>  
>  /*
>   * Defect Pixel Cluster Correction
>
Heiko Stuebner Jan. 21, 2021, 5:12 p.m. UTC | #2 
Hi Helen,

Am Donnerstag, 21. Januar 2021, 18:04:49 CET schrieb Helen Koike:
> Hi Dafna,
> 
> On 1/21/21 11:44 AM, Dafna Hirschfeld wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > The uapi right now specifies an array size of 28 but the actual number
> > of elements is only 25 with the last 3 being unused.
> > 
> > Reduce the array size to the correct number of elements and change
> > the params code to iterate the array 25 times.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 ++-
> >  include/uapi/linux/rkisp1-config.h                     | 3 +--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 6af4d551ffb5..021939466b24 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -589,7 +589,6 @@ static void rkisp1_hst_config(struct rkisp1_params *params,
> >  		RKISP1_CIF_ISP_HIST_WEIGHT_22TO03,
> >  		RKISP1_CIF_ISP_HIST_WEIGHT_13TO43,
> >  		RKISP1_CIF_ISP_HIST_WEIGHT_04TO34,
> > -		RKISP1_CIF_ISP_HIST_WEIGHT_44,
> >  	};
> >  	const u8 *weight;
> >  	unsigned int i;
> > @@ -622,6 +621,8 @@ static void rkisp1_hst_config(struct rkisp1_params *params,
> >  							    weight[2],
> >  							    weight[3]),
> >  				 hist_weight_regs[i]);
> > +
> > +	rkisp1_write(params->rkisp1, weight[0] & 0x1F, RKISP1_CIF_ISP_HIST_WEIGHT_44);
> 
> I just noticed that the default value to arg->hist_weight was:
> 
> 	memset(hst.hist_weight, 0x01, sizeof(hst.hist_weight));
> 
> Shouldn't the value be RKISP1_CIF_ISP_HIST_WEIGHT_SET(weight[0], 0x01, 0x01, 0x01) ?
> Or am I missing something?

The last register only has that one value from weight[24];

I.e. before we were writing the 4 values into that last register as well
from indices 24, 25, 26, 27 ... hence the 3 unused values in the weight array.

But that was merely a crutch to make that fit into the write loop.


Heiko


> >  }
> >  
> >  static void
> > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > index 24f05d6d258f..35aa82d5f6dd 100644
> > --- a/include/uapi/linux/rkisp1-config.h
> > +++ b/include/uapi/linux/rkisp1-config.h
> > @@ -102,8 +102,7 @@
> >  /*
> >   * Histogram calculation
> >   */
> > -/* Last 3 values unused. */
> > -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
> > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 25
> >  
> >  /*
> >   * Defect Pixel Cluster Correction
> > 
>
Helen Koike Jan. 21, 2021, 5:22 p.m. UTC | #3 
On 1/21/21 2:12 PM, Heiko Stübner wrote:
> Hi Helen,
> 
> Am Donnerstag, 21. Januar 2021, 18:04:49 CET schrieb Helen Koike:
>> Hi Dafna,
>>
>> On 1/21/21 11:44 AM, Dafna Hirschfeld wrote:
>>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>>
>>> The uapi right now specifies an array size of 28 but the actual number
>>> of elements is only 25 with the last 3 being unused.
>>>
>>> Reduce the array size to the correct number of elements and change
>>> the params code to iterate the array 25 times.
>>>
>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>>  drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 ++-
>>>  include/uapi/linux/rkisp1-config.h                     | 3 +--
>>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
>>> index 6af4d551ffb5..021939466b24 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
>>> @@ -589,7 +589,6 @@ static void rkisp1_hst_config(struct rkisp1_params *params,
>>>  		RKISP1_CIF_ISP_HIST_WEIGHT_22TO03,
>>>  		RKISP1_CIF_ISP_HIST_WEIGHT_13TO43,
>>>  		RKISP1_CIF_ISP_HIST_WEIGHT_04TO34,
>>> -		RKISP1_CIF_ISP_HIST_WEIGHT_44,
>>>  	};
>>>  	const u8 *weight;
>>>  	unsigned int i;
>>> @@ -622,6 +621,8 @@ static void rkisp1_hst_config(struct rkisp1_params *params,
>>>  							    weight[2],
>>>  							    weight[3]),
>>>  				 hist_weight_regs[i]);
>>> +
>>> +	rkisp1_write(params->rkisp1, weight[0] & 0x1F, RKISP1_CIF_ISP_HIST_WEIGHT_44);
>>
>> I just noticed that the default value to arg->hist_weight was:
>>
>> 	memset(hst.hist_weight, 0x01, sizeof(hst.hist_weight));
>>
>> Shouldn't the value be RKISP1_CIF_ISP_HIST_WEIGHT_SET(weight[0], 0x01, 0x01, 0x01) ?
>> Or am I missing something?
> 
> The last register only has that one value from weight[24];
> 
> I.e. before we were writing the 4 values into that last register as well
> from indices 24, 25, 26, 27 ... hence the 3 unused values in the weight array.
> 
> But that was merely a crutch to make that fit into the write loop.

right, lgtm, thank you and Dafna for working on this.

For the whole series:

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

Regards,
Helen

> 
> 
> Heiko
> 
> 
>>>  }
>>>  
>>>  static void
>>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
>>> index 24f05d6d258f..35aa82d5f6dd 100644
>>> --- a/include/uapi/linux/rkisp1-config.h
>>> +++ b/include/uapi/linux/rkisp1-config.h
>>> @@ -102,8 +102,7 @@
>>>  /*
>>>   * Histogram calculation
>>>   */
>>> -/* Last 3 values unused. */
>>> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
>>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 25
>>>  
>>>  /*
>>>   * Defect Pixel Cluster Correction
>>>
>>
> 
> 
> 
>
diff mbox series

Patch

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 6af4d551ffb5..021939466b24 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -589,7 +589,6 @@  static void rkisp1_hst_config(struct rkisp1_params *params,
 		RKISP1_CIF_ISP_HIST_WEIGHT_22TO03,
 		RKISP1_CIF_ISP_HIST_WEIGHT_13TO43,
 		RKISP1_CIF_ISP_HIST_WEIGHT_04TO34,
-		RKISP1_CIF_ISP_HIST_WEIGHT_44,
 	};
 	const u8 *weight;
 	unsigned int i;
@@ -622,6 +621,8 @@  static void rkisp1_hst_config(struct rkisp1_params *params,
 							    weight[2],
 							    weight[3]),
 				 hist_weight_regs[i]);
+
+	rkisp1_write(params->rkisp1, weight[0] & 0x1F, RKISP1_CIF_ISP_HIST_WEIGHT_44);
 }
 
 static void
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 24f05d6d258f..35aa82d5f6dd 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -102,8 +102,7 @@ 
 /*
  * Histogram calculation
  */
-/* Last 3 values unused. */
-#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 25
 
 /*
  * Defect Pixel Cluster Correction