| Message ID | 20210120164446.1220-2-dafna.hirschfeld@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix the rkisp1 userspace API for later IP versions | expand |
Hi Dafna, Thanks for the update. On Wed, Jan 20, 2021 at 05:44:42PM +0100, Dafna Hirschfeld wrote: > 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. The commit message lines wrap at 74, under 60 is not much. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> Should there be a Fixes: line? The patch is changing UAPI... The file has been recently introduced. Should it go to fixes or to a stable kernel, too? > --- > include/uapi/linux/rkisp1-config.h | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h > index 6e449e784260..f75f8d698fb4 100644 > --- a/include/uapi/linux/rkisp1-config.h > +++ b/include/uapi/linux/rkisp1-config.h > @@ -844,13 +844,17 @@ 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. Please wrap the lines before 80. > */ > 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.17.1 > -- Regards, Sakari Ailus
Am 20.01.21 um 22:32 schrieb Sakari Ailus: > Hi Dafna, > > Thanks for the update. > > On Wed, Jan 20, 2021 at 05:44:42PM +0100, Dafna Hirschfeld wrote: >> 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. > > The commit message lines wrap at 74, under 60 is not much. > >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > Should there be a Fixes: line? The patch is changing UAPI... > > The file has been recently introduced. Should it go to fixes or to a stable > kernel, too? The file was in the staging directory and will be first destaged in the coming 5.11, so I don't think this is necessary. > >> --- >> include/uapi/linux/rkisp1-config.h | 12 ++++++++---- >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h >> index 6e449e784260..f75f8d698fb4 100644 >> --- a/include/uapi/linux/rkisp1-config.h >> +++ b/include/uapi/linux/rkisp1-config.h >> @@ -844,13 +844,17 @@ 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. > > Please wrap the lines before 80. ok, Thanks, Dafna > >> */ >> 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.17.1 >> >
Hi Dafna, On Thu, Jan 21, 2021 at 01:48:58PM +0100, Dafna Hirschfeld wrote: > > > Am 20.01.21 um 22:32 schrieb Sakari Ailus: > > Hi Dafna, > > > > Thanks for the update. > > > > On Wed, Jan 20, 2021 at 05:44:42PM +0100, Dafna Hirschfeld wrote: > > > 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. > > > > The commit message lines wrap at 74, under 60 is not much. > > > > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > > > > Should there be a Fixes: line? The patch is changing UAPI... > > > > The file has been recently introduced. Should it go to fixes or to a stable > > kernel, too? > > The file was in the staging directory and will be first destaged in the coming 5.11, > so I don't think this is necessary. You still probably want to avoid making the uAPI change after 5.11, so the fixes branch would be the right target I presume. -- Sakari Ailus
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h index 6e449e784260..f75f8d698fb4 100644 --- a/include/uapi/linux/rkisp1-config.h +++ b/include/uapi/linux/rkisp1-config.h @@ -844,13 +844,17 @@ 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]; }; /**
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> --- include/uapi/linux/rkisp1-config.h | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)