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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 20/01/2021 12:32, Dafna Hirschfeld wrote:
> 
> 
> Am 20.01.21 um 11:49 schrieb Hans Verkuil:
>> 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@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?
>>>>>>
>>>>>> 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
> 
> Hi, so there is one issue in Heiko's patch, in 2/4 which changes the array size to 25,
> the code in rkisp1-params.c should also change to iterate it only 25 times.
> I talked to Heiko, and we said that I'll send a v7 of his patchset that includes in addition
> to his patches my patches and also that fix to patch 2/4.
> Is that fine or should we keep it in two separated patches?

That's fine. Easier, actually.

Regards,

	Hans

> 
> Thanks,
> Dafna
> 
>>