| Message ID | 20210119145341.29686-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: rkisp1: uapi: change hist_bins array type from __u16 to __u32 | expand |
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. 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]; >>> }; >>> >>> /** >>> >>
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. Thanks, Dafna > >> >>> >>> 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]; >>>> }; >>>> >>>> /** >>>> >>> >
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 > > Thanks, > Dafna > > >> >>> >>>> >>>> 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]; >>>>> }; >>>>> >>>>> /** >>>>> >>>> >>
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 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. 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. Thanks, Dafna > >> >> Thanks, >> Dafna >> >> >>> >>>> >>>>> >>>>> 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]; >>>>>> }; >>>>>> >>>>>> /** >>>>>> >>>>> >>> >
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
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? Thanks, Dafna >
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 > >>
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. * * 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]; }; /**
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(-)