diff mbox series

[v7,1/5] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

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

Commit Message

Dafna Hirschfeld Jan. 20, 2021, 4:44 p.m. UTC
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(-)

Comments

Sakari Ailus Jan. 20, 2021, 9:32 p.m. UTC | #1
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
>
Dafna Hirschfeld Jan. 21, 2021, 12:48 p.m. UTC | #2
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
>>
>
Sakari Ailus Jan. 21, 2021, 1:01 p.m. UTC | #3
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.
diff mbox series

Patch

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];
 };
 
 /**