Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
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 [ 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
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 > > [ 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
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 [ 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 > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
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 > > > > > > [ 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
Am 08.02.21 um 21:46 schrieb Hans Verkuil:
On 08/02/2021 18:57, Sasha Levin wrote:
From: Dafna Hirschfeld
[ 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
Acked-by: Helen Koike
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
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];
};
/**
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
On 08/02/2021 18:57, Sasha Levin wrote:
> From: Dafna Hirschfeld
>
> [ 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
> Acked-by: Helen Koike
> Signed-off-by: Hans Verkuil
> Signed-off-by: Mauro Carvalho Chehab
> Signed-off-by: Sasha Levin
> ---
> 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];
> };
>
> /**
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH AUTOSEL 5.10 14/36] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32
From: Dafna Hirschfeld
[ 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
Acked-by: Helen Koike
Signed-off-by: Hans Verkuil
Signed-off-by: Mauro Carvalho Chehab
Signed-off-by: Sasha Levin
---
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
