| Message ID | 20210115163829.217131-1-heiko@sntech.de |
|---|---|
| Headers | show |
| Series | Fix the rkisp1 userspace API for later IP versions | expand |
Hi Dafna, Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld: > > Am 15.01.21 um 17:38 schrieb Heiko Stuebner: > > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > > > > Later variants of the rkisp1 block use more entries in some arrays: > > > > RKISP1_CIF_ISP_AE_MEAN_MAX 25 -> 81 > > RKISP1_CIF_ISP_HIST_BIN_N_MAX 16 -> 32 > > RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17 -> 34 > > RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81 > > I see you didn't change the value for that define. In the below patch I find @@ -103,7 +111,9 @@ * Histogram calculation */ /* Last 3 values unused. */ -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28 +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81 +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 so I'm not sure I understand what you mean except this. > The usage of it is a bit more complicated. > It is used in function rkisp1_hst_config. Yeah, though the for-loop iterates over 4*7 entry values, so stays below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases. > Actually the real number of weight values are 25 (5x5) for rk3399, > the last 3 are not used. I think that in order to support both > 5x5 and 9x9 the code in rkisp1-params.c should change. I'll > send a patch fixing it. If you look at my V12-patch [0] the weight handling is done different there and from the registers, it looks like they exchanges that part of the isp. [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/ void rkisp1_hst_config_v12() as a search term [...] > > @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat { > > * > > * @hist_bins: measured bin counters > > * > > - * Measurement window divided into 16 sub-windows, set > > - * with ISP_HIST_XXX > > + * Measurement window divided into 16 sub-windows for V10/V10 > > + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX > > It is actually not windows but histogram bins. Could you change it to: > "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. I've changed this like your suggestions and will give a bit of time for the stuff above. But I guess I can send a v5 some time tomorrow? Thanks for your input Heiko
Am 16.01.21 um 00:52 schrieb Heiko Stübner: > Hi Dafna, > > Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld: >> >> Am 15.01.21 um 17:38 schrieb Heiko Stuebner: >>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> >>> >>> Later variants of the rkisp1 block use more entries in some arrays: >>> >>> RKISP1_CIF_ISP_AE_MEAN_MAX 25 -> 81 >>> RKISP1_CIF_ISP_HIST_BIN_N_MAX 16 -> 32 >>> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17 -> 34 >>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81 >> >> I see you didn't change the value for that define. > > In the below patch I find > > @@ -103,7 +111,9 @@ > * Histogram calculation > */ > /* Last 3 values unused. */ > -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28 > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81 > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 > > so I'm not sure I understand what you mean except this. Opps, I somehow missed that. But now since we have 81 entries, it makes no sense to define it to 28 for V10 and documenting "Last 3 values unused" (see just above the definition). We can set it just to 25, we have 56 (81-25) unused values anyway. > >> The usage of it is a bit more complicated. >> It is used in function rkisp1_hst_config. > > Yeah, though the for-loop iterates over 4*7 entry values, so stays > below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases. > > >> Actually the real number of weight values are 25 (5x5) for rk3399, >> the last 3 are not used. I think that in order to support both >> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll >> send a patch fixing it. > > If you look at my V12-patch [0] the weight handling is done different there > and from the registers, it looks like they exchanges that part of the isp. > > [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/ > void rkisp1_hst_config_v12() as a search term > > [...] Right, there is no need to change the relevant code in rkisp1-param.c when setting the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE to 25. Thanks, Dafna > >>> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat { >>> * >>> * @hist_bins: measured bin counters >>> * >>> - * Measurement window divided into 16 sub-windows, set >>> - * with ISP_HIST_XXX >>> + * Measurement window divided into 16 sub-windows for V10/V10 >>> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX >> >> It is actually not windows but histogram bins. Could you change it to: >> "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. > > I've changed this like your suggestions and will give a bit of time for > the stuff above. But I guess I can send a v5 some time tomorrow? > > > Thanks for your input > Heiko > >
Hi Dafna, Am Samstag, 16. Januar 2021, 10:05:39 CET schrieb Dafna Hirschfeld: > Am 16.01.21 um 00:52 schrieb Heiko Stübner: > > Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld: > >> Am 15.01.21 um 17:38 schrieb Heiko Stuebner: > >>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> > >>> > >>> Later variants of the rkisp1 block use more entries in some arrays: > >>> > >>> RKISP1_CIF_ISP_AE_MEAN_MAX 25 -> 81 > >>> RKISP1_CIF_ISP_HIST_BIN_N_MAX 16 -> 32 > >>> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17 -> 34 > >>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81 > >> > >> I see you didn't change the value for that define. > > > > In the below patch I find > > > > @@ -103,7 +111,9 @@ > > * Histogram calculation > > */ > > /* Last 3 values unused. */ > > -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 > > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28 > > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81 > > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 > > > > so I'm not sure I understand what you mean except this. > > Opps, I somehow missed that. > But now since we have 81 entries, it makes no sense to > define it to 28 for V10 and documenting "Last 3 values unused" (see just above the definition). > We can set it just to 25, we have 56 (81-25) unused values anyway. The rkisp1_hst_config() function iterates over its regs-array and accesses 4 weight values in each step and writing them to the register: static const u32 hist_weight_regs[] = { RKISP1_CIF_ISP_HIST_WEIGHT_00TO30_V10, ... weight0-3 RKISP1_CIF_ISP_HIST_WEIGHT_40TO21_V10, ... weight4-7 RKISP1_CIF_ISP_HIST_WEIGHT_31TO12_V10, ... weight8-11 RKISP1_CIF_ISP_HIST_WEIGHT_22TO03_V10, ... weight12-15 RKISP1_CIF_ISP_HIST_WEIGHT_13TO43_V10, ... weight16-19 RKISP1_CIF_ISP_HIST_WEIGHT_04TO34_V10, ... weight20-23 RKISP1_CIF_ISP_HIST_WEIGHT_44_V10, ... weight24-27 }; and yes the last step only uses 1 value to write to the register. But I guess if we really want to reduce the max value, we should move that last part into a separate write, so that we don't access parts we claim to not access ;-) > >> The usage of it is a bit more complicated. > >> It is used in function rkisp1_hst_config. > > > > Yeah, though the for-loop iterates over 4*7 entry values, so stays > > below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases. > > > > > >> Actually the real number of weight values are 25 (5x5) for rk3399, > >> the last 3 are not used. I think that in order to support both > >> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll > >> send a patch fixing it. > > > > If you look at my V12-patch [0] the weight handling is done different there > > and from the registers, it looks like they exchanges that part of the isp. > > > > [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/ > > void rkisp1_hst_config_v12() as a search term > > > > [...] > > Right, there is no need to change the relevant code in rkisp1-param.c when setting the > RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE to 25. ok, should we add a 4th patch fixing that and moving the last write out of the loop? Heiko > >>> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat { > >>> * > >>> * @hist_bins: measured bin counters > >>> * > >>> - * Measurement window divided into 16 sub-windows, set > >>> - * with ISP_HIST_XXX > >>> + * Measurement window divided into 16 sub-windows for V10/V10 > >>> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX > >> > >> It is actually not windows but histogram bins. Could you change it to: > >> "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. > > > > I've changed this like your suggestions and will give a bit of time for > > the stuff above. But I guess I can send a v5 some time tomorrow? > > > > > > Thanks for your input > > Heiko > > > > >
Am 16.01.21 um 12:57 schrieb Heiko Stübner: > Hi Dafna, > > Am Samstag, 16. Januar 2021, 10:05:39 CET schrieb Dafna Hirschfeld: >> Am 16.01.21 um 00:52 schrieb Heiko Stübner: >>> Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld: >>>> Am 15.01.21 um 17:38 schrieb Heiko Stuebner: >>>>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com> >>>>> >>>>> Later variants of the rkisp1 block use more entries in some arrays: >>>>> >>>>> RKISP1_CIF_ISP_AE_MEAN_MAX 25 -> 81 >>>>> RKISP1_CIF_ISP_HIST_BIN_N_MAX 16 -> 32 >>>>> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES 17 -> 34 >>>>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81 >>>> >>>> I see you didn't change the value for that define. >>> >>> In the below patch I find >>> >>> @@ -103,7 +111,9 @@ >>> * Histogram calculation >>> */ >>> /* Last 3 values unused. */ >>> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 >>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28 >>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81 >>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 >>> >>> so I'm not sure I understand what you mean except this. >> >> Opps, I somehow missed that. >> But now since we have 81 entries, it makes no sense to >> define it to 28 for V10 and documenting "Last 3 values unused" (see just above the definition). >> We can set it just to 25, we have 56 (81-25) unused values anyway. > > The rkisp1_hst_config() function iterates over its regs-array and accesses > 4 weight values in each step and writing them to the register: > > static const u32 hist_weight_regs[] = { > RKISP1_CIF_ISP_HIST_WEIGHT_00TO30_V10, ... weight0-3 > RKISP1_CIF_ISP_HIST_WEIGHT_40TO21_V10, ... weight4-7 > RKISP1_CIF_ISP_HIST_WEIGHT_31TO12_V10, ... weight8-11 > RKISP1_CIF_ISP_HIST_WEIGHT_22TO03_V10, ... weight12-15 > RKISP1_CIF_ISP_HIST_WEIGHT_13TO43_V10, ... weight16-19 > RKISP1_CIF_ISP_HIST_WEIGHT_04TO34_V10, ... weight20-23 > RKISP1_CIF_ISP_HIST_WEIGHT_44_V10, ... weight24-27 > }; > > and yes the last step only uses 1 value to write to the register. > But I guess if we really want to reduce the max value, we should move > that last part into a separate write, so that we don't access parts we claim > to not access ;-) > >>>> The usage of it is a bit more complicated. >>>> It is used in function rkisp1_hst_config. >>> >>> Yeah, though the for-loop iterates over 4*7 entry values, so stays >>> below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases. >>> >>> >>>> Actually the real number of weight values are 25 (5x5) for rk3399, >>>> the last 3 are not used. I think that in order to support both >>>> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll >>>> send a patch fixing it. >>> >>> If you look at my V12-patch [0] the weight handling is done different there >>> and from the registers, it looks like they exchanges that part of the isp. >>> >>> [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/ >>> void rkisp1_hst_config_v12() as a search term >>> >>> [...] >> >> Right, there is no need to change the relevant code in rkisp1-param.c when setting the >> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE to 25. > > ok, should we add a 4th patch fixing that and moving the last write out of the loop? I think changing the related code in the rkisp1-params.c can be done later in a separate patch. I think you can change it to 25 in the same patch that adds V12 support and write in the commit message that because the array size is now 81, there is no point defining the grid size for V10 to 28 and in addition documenting that the last 3 values are not used. Thanks, Dafna > > > Heiko > >>>>> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat { >>>>> * >>>>> * @hist_bins: measured bin counters >>>>> * >>>>> - * Measurement window divided into 16 sub-windows, set >>>>> - * with ISP_HIST_XXX >>>>> + * Measurement window divided into 16 sub-windows for V10/V10 >>>>> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX >>>> >>>> It is actually not windows but histogram bins. Could you change it to: >>>> "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. >>> >>> I've changed this like your suggestions and will give a bit of time for >>> the stuff above. But I guess I can send a v5 some time tomorrow? >>> >>> >>> Thanks for your input >>> Heiko >>> >>> >> > > > >
This NEEDs to go into 5.11 while we can still adapt the uapi during its RC-cycle. When looking into supporting the rkisp1 of the px30 I noticed that some fields got bigger in the uapi, caused by bigger number of samples for example for histograms or gamma values. The rkisp1 was destaged with 5.11-rc1 so we have still time during the 5.11 cycle to fix that without big hassles. This was discussed previously in a mail [0] and a rfc series [1] and this two-part series now splits out the important parts that really should become part of a 5.11-rc and thus the final 5.11. changes since v3: - add patch fixing the original histogram size comment - make comments in uapi more verbose (Hans) - fix wording in admin guide (Hans) - document version <-> soc in uapi as well (easier for people) (Dafna) changes since v2: - actually zero the correct sizes for u16 values (hist-bins) (kernel-test-robot) changes since v1: - drop duplicate isp_ver storage, hw_revision is enough (Dafna) - document multiple maximum sizes in uapi (Hans) - document usage of hw_revision field (Hans) - zero fields transmitted to userspace before adding data (Hans) - use _V10 field sizes when filling fields, as there is only v10 for now changes since rfc: - move rkisp1_version enum into uapo - show version in media-api hw_revision - introduce constants for versions and make max use the biggest Heiko Stuebner (3): media: rockchip: rkisp1: fix comment about number of histogram sub-windows media: rockchip: rkisp1: carry ip version information media: rockchip: rkisp1: extend uapi array sizes Documentation/admin-guide/media/rkisp1.rst | 16 ++++ .../platform/rockchip/rkisp1/rkisp1-dev.c | 21 +++--- .../platform/rockchip/rkisp1/rkisp1-params.c | 2 +- .../platform/rockchip/rkisp1/rkisp1-stats.c | 12 ++- include/uapi/linux/rkisp1-config.h | 74 +++++++++++++++++-- 5 files changed, 106 insertions(+), 19 deletions(-)