Re: [PATCH] media: rkisp1: uapi: change hist_bins array type from __u16 to __u32

[Dafna Hirschfeld <dafna.hirschfeld@collabora.com>]

Am 19.01.21 um 16:00 schrieb Hans Verkuil:
> On 19/01/2021 15:53, 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.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>> This patch is applied on top of v6 of the patchset
>> "Fix the rkisp1 userspace API for later IP versions"
>>
>>   include/uapi/linux/rkisp1-config.h | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
>> index 57ca3eea985f..47f6b84d7c56 100644
>> --- a/include/uapi/linux/rkisp1-config.h
>> +++ b/include/uapi/linux/rkisp1-config.h
>> @@ -895,7 +895,8 @@ 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 value
>> + *	       composed of a 16-bit unsigned integer and 4 bits fractional part.
> 
> So bits 0-3 are the fractional part and bits 4-19 contain the integer part?
> What goes where should be defined!

Actually I don't know, I just copied the docs in the datasheet.
I can try figure it out. I can meanwhile send a patch without the doc until
we are sure. Is that OK?

> 
> Looking at rkisp1_stats_get_hst_meas() I see this:
> 
>          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);
> 
> Here this is cast to a u8, so how does this work?

This seems to be a bug. I see that this cast is introduced in v12 of the patch
"media: staging: rkisp1: add capture device for statistics".
This cast does not exist in any of the downstream versions.

Thanks,
Dafna

> 
> There is something fishy here...
> 
> Regards,
> 
> 	Hans
> 
>>    *
>>    * The histogram values divided into 16 bins for V10/V11 and 32 bins
>>    * for V12/V13. It is configured within the struct rkisp1_cif_isp_hst_config.
>> @@ -909,7 +910,7 @@ struct rkisp1_cif_isp_af_stat {
>>    * RKISP1_CIF_ISP_HIST_BIN_N_MAX is equal to the maximum of the two.
>>    */
>>   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];
>>   };
>>   
>>   /**
>>
>