On 20/01/2021 11:37, Dafna Hirschfeld wrote:
>
>
> Am 20.01.21 um 10:58 schrieb Hans Verkuil:
>> On 19/01/2021 18:47, Dafna Hirschfeld wrote:
>>>
>>>
>>> Am 19.01.21 um 17:32 schrieb Hans Verkuil:
>>>> 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.
>>>
>>> I see that indeed bits 0-3 are the fractional part.
>>> The measurements are taken over 25 sub-windows and it is possible to
>>> give a weight to each window. If I set all weights to 1 then I expect to get
>>> an integer value and indeed in that case I see that the last 4 bits are always 0.
>>> If I set the weights to other than 1 I get the last 4 bits not only 0.
>>>
>>> The UAPI is generally not very good documented mainly because there is currently
>>> no open source userspace that uses it.
>>>
>>>>
>>>> Is this the only fractional type that is used in the rkisp1 uapi or are there
>>>> others? (Just checking).
>>>
>>> There are other places as well.
>>> I see that there are some values in rkisp1_cif_isp_lsc_config that are fractional
>>> and not documented.
>>> The two other fractional types in the uapi are already documented.
>>>
>>> I can add a patch that extends the documentation of all the fields in the uapi using the datasheet.
>>> But I think such a patch can be added after de-staging.
>>
>> You need to make changes anyway in the uAPI for 5.11 (including fixing the
>> bad u8 cast), so let's do this right.
>>
>> Regards,
>>
>> Hans
>
> So just to make sure I understood correctly, in addition to Heiko Stuebner's patchset.
> I should a add another patchset that includes:
>
> 1. changing the hist_bins type to __u32,
> 2. removing the u8 cast in rkisp1-stats.c
> 3. documenting all the fields in the uapi
Well, documenting the fractional types at least. Or are you talking about other
fields as well?
>
> I don't know what is the policy for fixes among the kernel management,
> I am worried that if we are too late or the changes are too big they might reject it.
rc4 was released last weekend, so if it is possible to post these fixes today/tomorrow
then there is enough time. I'd like to post a PR for these and other fixes ideally no
later than Friday.
The documentation patch can slip to 5.12 if really necessary, but I would prefer
to have that as part of the 5.11 fixes as well.
>
> Also, documenting thoroughly can only be done if there is userspace app or at least unit tests
> that test each component since the datasheet lack some details as in this case.
Do what you can. If some things need more time, then it is OK to postpone that
part to 5.12.
Regards,
Hans
