On 19/01/2021 16:32, Dafna Hirschfeld wrote:
>
>
> 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@xxxxxxxxxxxxx>
>>> ---
>>> 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?
No, this should be documented properly.
Is this the only fractional type that is used in the rkisp1 uapi or are there
others? (Just checking).
>
>>
>> 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.
Clearly something that needs to be fixed as well.
Regards,
Hans
>
> 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];
>>> };
>>>
>>> /**
>>>
>>
