[PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Sasha Levin]

From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

[ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]

Each entry in the array is a 20 bits value composed of 16 bits unsigned
integer and 4 bits fractional part. So the type should change to __u32.
In addition add a documentation of how the measurements are done.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
index 432cb6be55b47..c19fe059c2442 100644
--- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
+++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
@@ -848,13 +848,18 @@ struct rkisp1_cif_isp_af_stat {
 /**
  * struct rkisp1_cif_isp_hist_stat - statistics histogram data
  *
- * @hist_bins: measured bin counters
+ * @hist_bins: measured bin counters. Each bin is a 20 bits unsigned fixed point
+ *	       type. Bits 0-4 are the fractional part and bits 5-19 are the
+ *	       integer part.
  *
- * Measurement window divided into 25 sub-windows, set
- * with ISP_HIST_XXX
+ * The window of the measurements area is divided to 5x5 sub-windows. The
+ * histogram is then computed for each sub-window independently and the final
+ * result is a weighted average of the histogram measurements on all
+ * sub-windows. The window of the measurements area and the weight of each
+ * sub-window are configurable using struct @rkisp1_cif_isp_hst_config.
  */
 struct rkisp1_cif_isp_hist_stat {
-	__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
+	__u32 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
 };
 
 /**
-- 
2.27.0

Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Hans Verkuil]

On 08/02/2021 18:57, Sasha Levin wrote:
> From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> [ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]
> 
> Each entry in the array is a 20 bits value composed of 16 bits unsigned
> integer and 4 bits fractional part. So the type should change to __u32.
> In addition add a documentation of how the measurements are done.

Dafna, Helen, does it make sense at all to backport these three patches to
when rkisp1 was a staging driver?

I would be inclined not to backport this.

Regards,

	Hans

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> index 432cb6be55b47..c19fe059c2442 100644
> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
> @@ -848,13 +848,18 @@ struct rkisp1_cif_isp_af_stat {
>  /**
>   * struct rkisp1_cif_isp_hist_stat - statistics histogram data
>   *
> - * @hist_bins: measured bin counters
> + * @hist_bins: measured bin counters. Each bin is a 20 bits unsigned fixed point
> + *	       type. Bits 0-4 are the fractional part and bits 5-19 are the
> + *	       integer part.
>   *
> - * Measurement window divided into 25 sub-windows, set
> - * with ISP_HIST_XXX
> + * The window of the measurements area is divided to 5x5 sub-windows. The
> + * histogram is then computed for each sub-window independently and the final
> + * result is a weighted average of the histogram measurements on all
> + * sub-windows. The window of the measurements area and the weight of each
> + * sub-window are configurable using struct @rkisp1_cif_isp_hst_config.
>   */
>  struct rkisp1_cif_isp_hist_stat {
> -	__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
> +	__u32 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
>  };
>  
>  /**
> 

Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Dafna Hirschfeld]

Am 08.02.21 um 21:46 schrieb Hans Verkuil:
> On 08/02/2021 18:57, Sasha Levin wrote:
>> From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>
>> [ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]
>>
>> Each entry in the array is a 20 bits value composed of 16 bits unsigned
>> integer and 4 bits fractional part. So the type should change to __u32.
>> In addition add a documentation of how the measurements are done.
> 
> Dafna, Helen, does it make sense at all to backport these three patches to
> when rkisp1 was a staging driver?
> 
> I would be inclined not to backport this.

I also don't think it makes sense since this changes the uapi and it is not really a bug fix.


Thanks,
Dafna

> 
> Regards,
> 
> 	Hans
> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>>   drivers/staging/media/rkisp1/uapi/rkisp1-config.h | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> index 432cb6be55b47..c19fe059c2442 100644
>> --- a/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> +++ b/drivers/staging/media/rkisp1/uapi/rkisp1-config.h
>> @@ -848,13 +848,18 @@ struct rkisp1_cif_isp_af_stat {
>>   /**
>>    * struct rkisp1_cif_isp_hist_stat - statistics histogram data
>>    *
>> - * @hist_bins: measured bin counters
>> + * @hist_bins: measured bin counters. Each bin is a 20 bits unsigned fixed point
>> + *	       type. Bits 0-4 are the fractional part and bits 5-19 are the
>> + *	       integer part.
>>    *
>> - * Measurement window divided into 25 sub-windows, set
>> - * with ISP_HIST_XXX
>> + * The window of the measurements area is divided to 5x5 sub-windows. The
>> + * histogram is then computed for each sub-window independently and the final
>> + * result is a weighted average of the histogram measurements on all
>> + * sub-windows. The window of the measurements area and the weight of each
>> + * sub-window are configurable using struct @rkisp1_cif_isp_hst_config.
>>    */
>>   struct rkisp1_cif_isp_hist_stat {
>> -	__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
>> +	__u32 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
>>   };
>>   
>>   /**
>>
> 

Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Greg Kroah-Hartman]

On Tue, Feb 09, 2021 at 01:45:35PM +0100, Dafna Hirschfeld wrote:
> 
> 
> Am 08.02.21 um 21:46 schrieb Hans Verkuil:
> > On 08/02/2021 18:57, Sasha Levin wrote:
> > > From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > 
> > > [ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]
> > > 
> > > Each entry in the array is a 20 bits value composed of 16 bits unsigned
> > > integer and 4 bits fractional part. So the type should change to __u32.
> > > In addition add a documentation of how the measurements are done.
> > 
> > Dafna, Helen, does it make sense at all to backport these three patches to
> > when rkisp1 was a staging driver?
> > 
> > I would be inclined not to backport this.
> 
> I also don't think it makes sense since this changes the uapi and it is not really a bug fix.

Why was it ok to change the uapi in a newer kernel and not an older one?

thanks,

greg k-h

Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Hans Verkuil]

On 09/02/2021 14:02, Greg Kroah-Hartman wrote:
> On Tue, Feb 09, 2021 at 01:45:35PM +0100, Dafna Hirschfeld wrote:
>>
>>
>> Am 08.02.21 um 21:46 schrieb Hans Verkuil:
>>> On 08/02/2021 18:57, Sasha Levin wrote:
>>>> From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>
>>>> [ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]
>>>>
>>>> Each entry in the array is a 20 bits value composed of 16 bits unsigned
>>>> integer and 4 bits fractional part. So the type should change to __u32.
>>>> In addition add a documentation of how the measurements are done.
>>>
>>> Dafna, Helen, does it make sense at all to backport these three patches to
>>> when rkisp1 was a staging driver?
>>>
>>> I would be inclined not to backport this.
>>
>> I also don't think it makes sense since this changes the uapi and it is not really a bug fix.
> 
> Why was it ok to change the uapi in a newer kernel and not an older one?

In the older kernels this was a staging driver and the driver API was not public.
It's debatable whether there is any benefit from trying to backport patches like
this to a staging driver like that.

Also, these backports are incomplete, there are other patches that would need to
be applied to make this work. Applying just these three patches without the other
three (commits 66d81de7ea9d, fc672d806bd7 and ef357e02b6c4) makes it very messy
indeed.

I'd just leave the staging driver in older kernels as-is. Certainly don't just
apply these three patches without the other three commits, that would make it
even worse.

Regards,

	Hans

> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Greg Kroah-Hartman]

On Tue, Feb 09, 2021 at 02:39:41PM +0100, Hans Verkuil wrote:
> On 09/02/2021 14:02, Greg Kroah-Hartman wrote:
> > On Tue, Feb 09, 2021 at 01:45:35PM +0100, Dafna Hirschfeld wrote:
> >>
> >>
> >> Am 08.02.21 um 21:46 schrieb Hans Verkuil:
> >>> On 08/02/2021 18:57, Sasha Levin wrote:
> >>>> From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>>>
> >>>> [ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]
> >>>>
> >>>> Each entry in the array is a 20 bits value composed of 16 bits unsigned
> >>>> integer and 4 bits fractional part. So the type should change to __u32.
> >>>> In addition add a documentation of how the measurements are done.
> >>>
> >>> Dafna, Helen, does it make sense at all to backport these three patches to
> >>> when rkisp1 was a staging driver?
> >>>
> >>> I would be inclined not to backport this.
> >>
> >> I also don't think it makes sense since this changes the uapi and it is not really a bug fix.
> > 
> > Why was it ok to change the uapi in a newer kernel and not an older one?
> 
> In the older kernels this was a staging driver and the driver API was not public.
> It's debatable whether there is any benefit from trying to backport patches like
> this to a staging driver like that.
> 
> Also, these backports are incomplete, there are other patches that would need to
> be applied to make this work. Applying just these three patches without the other
> three (commits 66d81de7ea9d, fc672d806bd7 and ef357e02b6c4) makes it very messy
> indeed.
> 
> I'd just leave the staging driver in older kernels as-is. Certainly don't just
> apply these three patches without the other three commits, that would make it
> even worse.

Fair enough, Sasha, can you drop these?

thanks,

greg k-h

Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Sasha Levin]

On Tue, Feb 09, 2021 at 02:44:19PM +0100, Greg Kroah-Hartman wrote:
>On Tue, Feb 09, 2021 at 02:39:41PM +0100, Hans Verkuil wrote:
>> On 09/02/2021 14:02, Greg Kroah-Hartman wrote:
>> > On Tue, Feb 09, 2021 at 01:45:35PM +0100, Dafna Hirschfeld wrote:
>> >>
>> >>
>> >> Am 08.02.21 um 21:46 schrieb Hans Verkuil:
>> >>> On 08/02/2021 18:57, Sasha Levin wrote:
>> >>>> From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> >>>>
>> >>>> [ Upstream commit 31f190e0ccac8b75d33fdc95a797c526cf9b149e ]
>> >>>>
>> >>>> Each entry in the array is a 20 bits value composed of 16 bits unsigned
>> >>>> integer and 4 bits fractional part. So the type should change to __u32.
>> >>>> In addition add a documentation of how the measurements are done.
>> >>>
>> >>> Dafna, Helen, does it make sense at all to backport these three patches to
>> >>> when rkisp1 was a staging driver?
>> >>>
>> >>> I would be inclined not to backport this.
>> >>
>> >> I also don't think it makes sense since this changes the uapi and it is not really a bug fix.
>> >
>> > Why was it ok to change the uapi in a newer kernel and not an older one?
>>
>> In the older kernels this was a staging driver and the driver API was not public.
>> It's debatable whether there is any benefit from trying to backport patches like
>> this to a staging driver like that.
>>
>> Also, these backports are incomplete, there are other patches that would need to
>> be applied to make this work. Applying just these three patches without the other
>> three (commits 66d81de7ea9d, fc672d806bd7 and ef357e02b6c4) makes it very messy
>> indeed.
>>
>> I'd just leave the staging driver in older kernels as-is. Certainly don't just
>> apply these three patches without the other three commits, that would make it
>> even worse.
>
>Fair enough, Sasha, can you drop these?

Yup.

-- 
Thanks,
Sasha

[PATCH AUTOSEL 5.10 15/36] media: rkisp1: stats: remove a wrong cast to u8

[Sasha Levin]

From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

[ Upstream commit a76f8dc8be471028540df24749e99a3ec0ac7c94 ]

hist_bins is an array of type __u32. Each entry represent
a 20 bit fixed point value as documented inline.
The cast to u8 when setting the values is wrong. Remove it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/rkisp1/rkisp1-stats.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 51c64f75fe29a..8fdf646c4b75b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -253,8 +253,7 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
 	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
 	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
 		pbuf->params.hist.hist_bins[i] =
-			(u8)rkisp1_read(rkisp1,
-					RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
+			rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
 }
 
 static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
-- 
2.27.0

[PATCH AUTOSEL 5.10 16/36] media: rkisp1: stats: mask the hist_bins values

[Sasha Levin]

From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

[ Upstream commit a802a0430b863f03bc01aaea2d2bf6ff464f03e7 ]

hist_bins is an array of type __u32. Each entry represents
a 20 bit value. So mask out the unused bits.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/staging/media/rkisp1/rkisp1-regs.h  | 1 +
 drivers/staging/media/rkisp1/rkisp1-stats.c | 8 +++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-regs.h b/drivers/staging/media/rkisp1/rkisp1-regs.h
index 049f6c3a11df5..5819e58b2e557 100644
--- a/drivers/staging/media/rkisp1/rkisp1-regs.h
+++ b/drivers/staging/media/rkisp1/rkisp1-regs.h
@@ -365,6 +365,7 @@
 #define RKISP1_CIF_ISP_MAX_HIST_PREDIVIDER		0x0000007F
 #define RKISP1_CIF_ISP_HIST_ROW_NUM			5
 #define RKISP1_CIF_ISP_HIST_COLUMN_NUM			5
+#define RKISP1_CIF_ISP_HIST_GET_BIN(x)			((x) & 0x000FFFFF)
 
 /* AUTO FOCUS MEASUREMENT:  ISP_AFM_CTRL */
 #define RKISP1_ISP_AFM_CTRL_ENABLE			BIT(0)
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 8fdf646c4b75b..e52ba9b154e90 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -251,9 +251,11 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
 	unsigned int i;
 
 	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
-	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
-		pbuf->params.hist.hist_bins[i] =
-			rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
+	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++) {
+		u32 reg_val = rkisp1_read(rkisp1, RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
+
+		pbuf->params.hist.hist_bins[i] = RKISP1_CIF_ISP_HIST_GET_BIN(reg_val);
+	}
 }
 
 static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
-- 
2.27.0