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

[Dafna Hirschfeld]

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 at 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?

Thanks,
Dafna

>