* [PATCH v1 1/5] media: rkisp1: Add helper function to swap colour channels
2024-07-03 22:25 [PATCH v1 0/5] media: rkisp1: Add support for the companding block Laurent Pinchart
@ 2024-07-03 22:25 ` Laurent Pinchart
2024-07-04 7:53 ` Jacopo Mondi
2024-07-03 22:25 ` [PATCH v1 2/5] media: rkisp1: Add features mask to extensible block handlers Laurent Pinchart
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-03 22:25 UTC (permalink / raw)
To: linux-media; +Cc: Dafna Hirschfeld, Jacopo Mondi, Paul Elder, linux-rockchip
The BLS parameters passed by userspace are specified for named colour
channels (R, Gr, Gb and B), while the hardware registers reference
positions in the 2x2 CFA pattern (A, B, C and D).
The BLS values are swapped based on the CFA pattern when writing to or
reading from registers, using hand-roled switch statements. The logic is
duplicated already, and new code will require similar processing. Move
the swap logic to a shared function, using static data to control the
channels order.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../platform/rockchip/rkisp1/rkisp1-common.c | 15 +++++
.../platform/rockchip/rkisp1/rkisp1-common.h | 3 +
.../platform/rockchip/rkisp1/rkisp1-params.c | 58 ++++---------------
.../platform/rockchip/rkisp1/rkisp1-stats.c | 51 +++++-----------
4 files changed, 44 insertions(+), 83 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
index f956b90a407a..90513d15e7a7 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
@@ -178,3 +178,18 @@ void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
rkisp1_sd_adjust_crop_rect(crop, &crop_bounds);
}
+
+void rkisp1_bls_swap_regs(enum rkisp1_fmt_raw_pat_type pattern,
+ const u32 input[4], u32 output[4])
+{
+ static const unsigned int swap[][4] = {
+ [RKISP1_RAW_RGGB] = { 0, 1, 2, 3 },
+ [RKISP1_RAW_GRBG] = { 1, 0, 3, 2 },
+ [RKISP1_RAW_GBRG] = { 2, 3, 0, 1 },
+ [RKISP1_RAW_BGGR] = { 3, 2, 1, 0 },
+ };
+ unsigned int i;
+
+ for (i = 0; i < 4; ++i)
+ output[i] = input[swap[pattern][i]];
+}
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index c1689c0fa05a..cdf2d30e2bb1 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -645,6 +645,9 @@ void rkisp1_params_post_configure(struct rkisp1_params *params);
*/
void rkisp1_params_disable(struct rkisp1_params *params);
+void rkisp1_bls_swap_regs(enum rkisp1_fmt_raw_pat_type pattern,
+ const u32 input[4], u32 output[4]);
+
/* irq handlers */
irqreturn_t rkisp1_isp_isr(int irq, void *ctx);
irqreturn_t rkisp1_csi_isr(int irq, void *ctx);
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 39a32e98807f..b10cc2701244 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -165,54 +165,20 @@ static void rkisp1_bls_config(struct rkisp1_params *params,
new_control &= RKISP1_CIF_ISP_BLS_ENA;
/* fixed subtraction values */
if (!arg->enable_auto) {
- const struct rkisp1_cif_isp_bls_fixed_val *pval =
- &arg->fixed_val;
+ static const u32 regs[] = {
+ RKISP1_CIF_ISP_BLS_A_FIXED,
+ RKISP1_CIF_ISP_BLS_B_FIXED,
+ RKISP1_CIF_ISP_BLS_C_FIXED,
+ RKISP1_CIF_ISP_BLS_D_FIXED,
+ };
+ u32 swapped[4];
- switch (params->raw_type) {
- case RKISP1_RAW_BGGR:
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
- pval->r);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
- pval->gr);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
- pval->gb);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
- pval->b);
- break;
- case RKISP1_RAW_GBRG:
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
- pval->r);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
- pval->gr);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
- pval->gb);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
- pval->b);
- break;
- case RKISP1_RAW_GRBG:
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
- pval->r);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
- pval->gr);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
- pval->gb);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
- pval->b);
- break;
- case RKISP1_RAW_RGGB:
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
- pval->r);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
- pval->gr);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
- pval->gb);
- rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
- pval->b);
- break;
- default:
- break;
- }
+ rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
+ rkisp1_write(params->rkisp1, swapped[0], arg->fixed_val.r);
+ rkisp1_write(params->rkisp1, swapped[1], arg->fixed_val.gr);
+ rkisp1_write(params->rkisp1, swapped[2], arg->fixed_val.gb);
+ rkisp1_write(params->rkisp1, swapped[3], arg->fixed_val.b);
} else {
if (arg->en_windows & BIT(1)) {
rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_H2_START,
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
index 2795eef91bdd..a502719e916a 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
@@ -304,48 +304,25 @@ static void rkisp1_stats_get_hst_meas_v12(struct rkisp1_stats *stats,
static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
struct rkisp1_stat_buffer *pbuf)
{
+ static const u32 regs[] = {
+ RKISP1_CIF_ISP_BLS_A_MEASURED,
+ RKISP1_CIF_ISP_BLS_B_MEASURED,
+ RKISP1_CIF_ISP_BLS_C_MEASURED,
+ RKISP1_CIF_ISP_BLS_D_MEASURED,
+ };
struct rkisp1_device *rkisp1 = stats->rkisp1;
const struct rkisp1_mbus_info *in_fmt = rkisp1->isp.sink_fmt;
struct rkisp1_cif_isp_bls_meas_val *bls_val;
+ u32 swapped[4];
+
+ rkisp1_bls_swap_regs(in_fmt->bayer_pat, regs, swapped);
bls_val = &pbuf->params.ae.bls_val;
- if (in_fmt->bayer_pat == RKISP1_RAW_BGGR) {
- bls_val->meas_b =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
- bls_val->meas_gb =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
- bls_val->meas_gr =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
- bls_val->meas_r =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
- } else if (in_fmt->bayer_pat == RKISP1_RAW_GBRG) {
- bls_val->meas_gb =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
- bls_val->meas_b =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
- bls_val->meas_r =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
- bls_val->meas_gr =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
- } else if (in_fmt->bayer_pat == RKISP1_RAW_GRBG) {
- bls_val->meas_gr =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
- bls_val->meas_r =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
- bls_val->meas_b =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
- bls_val->meas_gb =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
- } else if (in_fmt->bayer_pat == RKISP1_RAW_RGGB) {
- bls_val->meas_r =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
- bls_val->meas_gr =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
- bls_val->meas_gb =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
- bls_val->meas_b =
- rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
- }
+
+ bls_val->meas_r = rkisp1_read(rkisp1, swapped[0]);
+ bls_val->meas_gr = rkisp1_read(rkisp1, swapped[1]);
+ bls_val->meas_gb = rkisp1_read(rkisp1, swapped[2]);
+ bls_val->meas_b = rkisp1_read(rkisp1, swapped[3]);
}
static const struct rkisp1_stats_ops rkisp1_v10_stats_ops = {
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/5] media: rkisp1: Add helper function to swap colour channels
2024-07-03 22:25 ` [PATCH v1 1/5] media: rkisp1: Add helper function to swap colour channels Laurent Pinchart
@ 2024-07-04 7:53 ` Jacopo Mondi
2024-07-04 8:53 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Jacopo Mondi @ 2024-07-04 7:53 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Dafna Hirschfeld, Jacopo Mondi, Paul Elder,
linux-rockchip
Hi Laurent
On Thu, Jul 04, 2024 at 01:25:29AM GMT, Laurent Pinchart wrote:
> The BLS parameters passed by userspace are specified for named colour
> channels (R, Gr, Gb and B), while the hardware registers reference
> positions in the 2x2 CFA pattern (A, B, C and D).
>
> The BLS values are swapped based on the CFA pattern when writing to or
> reading from registers, using hand-roled switch statements. The logic is
> duplicated already, and new code will require similar processing. Move
> the swap logic to a shared function, using static data to control the
> channels order.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> .../platform/rockchip/rkisp1/rkisp1-common.c | 15 +++++
> .../platform/rockchip/rkisp1/rkisp1-common.h | 3 +
> .../platform/rockchip/rkisp1/rkisp1-params.c | 58 ++++---------------
> .../platform/rockchip/rkisp1/rkisp1-stats.c | 51 +++++-----------
> 4 files changed, 44 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
> index f956b90a407a..90513d15e7a7 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
> @@ -178,3 +178,18 @@ void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
>
> rkisp1_sd_adjust_crop_rect(crop, &crop_bounds);
> }
> +
> +void rkisp1_bls_swap_regs(enum rkisp1_fmt_raw_pat_type pattern,
> + const u32 input[4], u32 output[4])
> +{
> + static const unsigned int swap[][4] = {
Should you declare this as a [4][4] array ?
> + [RKISP1_RAW_RGGB] = { 0, 1, 2, 3 },
> + [RKISP1_RAW_GRBG] = { 1, 0, 3, 2 },
> + [RKISP1_RAW_GBRG] = { 2, 3, 0, 1 },
> + [RKISP1_RAW_BGGR] = { 3, 2, 1, 0 },
> + };
> + unsigned int i;
'i' can be declared inside the for() statement
> +
> + for (i = 0; i < 4; ++i)
> + output[i] = input[swap[pattern][i]];
> +}
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index c1689c0fa05a..cdf2d30e2bb1 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -645,6 +645,9 @@ void rkisp1_params_post_configure(struct rkisp1_params *params);
> */
> void rkisp1_params_disable(struct rkisp1_params *params);
>
> +void rkisp1_bls_swap_regs(enum rkisp1_fmt_raw_pat_type pattern,
> + const u32 input[4], u32 output[4]);
> +
Should this be declared after rkisp1_sd_adjust_crop() to maintain the
same definition ordering as in the C file ?
> /* irq handlers */
> irqreturn_t rkisp1_isp_isr(int irq, void *ctx);
> irqreturn_t rkisp1_csi_isr(int irq, void *ctx);
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 39a32e98807f..b10cc2701244 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -165,54 +165,20 @@ static void rkisp1_bls_config(struct rkisp1_params *params,
> new_control &= RKISP1_CIF_ISP_BLS_ENA;
> /* fixed subtraction values */
> if (!arg->enable_auto) {
> - const struct rkisp1_cif_isp_bls_fixed_val *pval =
> - &arg->fixed_val;
> + static const u32 regs[] = {
> + RKISP1_CIF_ISP_BLS_A_FIXED,
> + RKISP1_CIF_ISP_BLS_B_FIXED,
> + RKISP1_CIF_ISP_BLS_C_FIXED,
> + RKISP1_CIF_ISP_BLS_D_FIXED,
> + };
> + u32 swapped[4];
>
> - switch (params->raw_type) {
> - case RKISP1_RAW_BGGR:
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> - pval->r);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> - pval->gr);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> - pval->gb);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> - pval->b);
> - break;
> - case RKISP1_RAW_GBRG:
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> - pval->r);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> - pval->gr);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> - pval->gb);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> - pval->b);
> - break;
> - case RKISP1_RAW_GRBG:
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> - pval->r);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> - pval->gr);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> - pval->gb);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> - pval->b);
> - break;
> - case RKISP1_RAW_RGGB:
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> - pval->r);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> - pval->gr);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> - pval->gb);
> - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> - pval->b);
> - break;
> - default:
> - break;
> - }
> + rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
Have you considered making rkisp1_bls_swap_regs() shuffle 'regs'
in-place ? Not strictly necessary, just wondering
>
> + rkisp1_write(params->rkisp1, swapped[0], arg->fixed_val.r);
> + rkisp1_write(params->rkisp1, swapped[1], arg->fixed_val.gr);
> + rkisp1_write(params->rkisp1, swapped[2], arg->fixed_val.gb);
> + rkisp1_write(params->rkisp1, swapped[3], arg->fixed_val.b);
> } else {
> if (arg->en_windows & BIT(1)) {
> rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_H2_START,
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> index 2795eef91bdd..a502719e916a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> @@ -304,48 +304,25 @@ static void rkisp1_stats_get_hst_meas_v12(struct rkisp1_stats *stats,
> static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
> struct rkisp1_stat_buffer *pbuf)
> {
> + static const u32 regs[] = {
> + RKISP1_CIF_ISP_BLS_A_MEASURED,
> + RKISP1_CIF_ISP_BLS_B_MEASURED,
> + RKISP1_CIF_ISP_BLS_C_MEASURED,
> + RKISP1_CIF_ISP_BLS_D_MEASURED,
> + };
> struct rkisp1_device *rkisp1 = stats->rkisp1;
> const struct rkisp1_mbus_info *in_fmt = rkisp1->isp.sink_fmt;
> struct rkisp1_cif_isp_bls_meas_val *bls_val;
> + u32 swapped[4];
> +
> + rkisp1_bls_swap_regs(in_fmt->bayer_pat, regs, swapped);
>
> bls_val = &pbuf->params.ae.bls_val;
> - if (in_fmt->bayer_pat == RKISP1_RAW_BGGR) {
> - bls_val->meas_b =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> - bls_val->meas_gb =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> - bls_val->meas_gr =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> - bls_val->meas_r =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> - } else if (in_fmt->bayer_pat == RKISP1_RAW_GBRG) {
> - bls_val->meas_gb =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> - bls_val->meas_b =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> - bls_val->meas_r =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> - bls_val->meas_gr =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> - } else if (in_fmt->bayer_pat == RKISP1_RAW_GRBG) {
> - bls_val->meas_gr =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> - bls_val->meas_r =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> - bls_val->meas_b =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> - bls_val->meas_gb =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> - } else if (in_fmt->bayer_pat == RKISP1_RAW_RGGB) {
> - bls_val->meas_r =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> - bls_val->meas_gr =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> - bls_val->meas_gb =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> - bls_val->meas_b =
> - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> - }
> +
> + bls_val->meas_r = rkisp1_read(rkisp1, swapped[0]);
> + bls_val->meas_gr = rkisp1_read(rkisp1, swapped[1]);
> + bls_val->meas_gb = rkisp1_read(rkisp1, swapped[2]);
> + bls_val->meas_b = rkisp1_read(rkisp1, swapped[3]);
> }
>
> static const struct rkisp1_stats_ops rkisp1_v10_stats_ops = {
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 1/5] media: rkisp1: Add helper function to swap colour channels
2024-07-04 7:53 ` Jacopo Mondi
@ 2024-07-04 8:53 ` Laurent Pinchart
0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-04 8:53 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: linux-media, Dafna Hirschfeld, Paul Elder, linux-rockchip
On Thu, Jul 04, 2024 at 09:53:02AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 04, 2024 at 01:25:29AM GMT, Laurent Pinchart wrote:
> > The BLS parameters passed by userspace are specified for named colour
> > channels (R, Gr, Gb and B), while the hardware registers reference
> > positions in the 2x2 CFA pattern (A, B, C and D).
> >
> > The BLS values are swapped based on the CFA pattern when writing to or
> > reading from registers, using hand-roled switch statements. The logic is
> > duplicated already, and new code will require similar processing. Move
> > the swap logic to a shared function, using static data to control the
> > channels order.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-common.c | 15 +++++
> > .../platform/rockchip/rkisp1/rkisp1-common.h | 3 +
> > .../platform/rockchip/rkisp1/rkisp1-params.c | 58 ++++---------------
> > .../platform/rockchip/rkisp1/rkisp1-stats.c | 51 +++++-----------
> > 4 files changed, 44 insertions(+), 83 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
> > index f956b90a407a..90513d15e7a7 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.c
> > @@ -178,3 +178,18 @@ void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
> >
> > rkisp1_sd_adjust_crop_rect(crop, &crop_bounds);
> > }
> > +
> > +void rkisp1_bls_swap_regs(enum rkisp1_fmt_raw_pat_type pattern,
> > + const u32 input[4], u32 output[4])
> > +{
> > + static const unsigned int swap[][4] = {
>
> Should you declare this as a [4][4] array ?
Ack.
> > + [RKISP1_RAW_RGGB] = { 0, 1, 2, 3 },
> > + [RKISP1_RAW_GRBG] = { 1, 0, 3, 2 },
> > + [RKISP1_RAW_GBRG] = { 2, 3, 0, 1 },
> > + [RKISP1_RAW_BGGR] = { 3, 2, 1, 0 },
> > + };
> > + unsigned int i;
>
> 'i' can be declared inside the for() statement
Backporting could get a bit more annoying, but it's mainline first :-)
I'll switch.
> > +
> > + for (i = 0; i < 4; ++i)
> > + output[i] = input[swap[pattern][i]];
> > +}
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index c1689c0fa05a..cdf2d30e2bb1 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -645,6 +645,9 @@ void rkisp1_params_post_configure(struct rkisp1_params *params);
> > */
> > void rkisp1_params_disable(struct rkisp1_params *params);
> >
> > +void rkisp1_bls_swap_regs(enum rkisp1_fmt_raw_pat_type pattern,
> > + const u32 input[4], u32 output[4]);
> > +
>
> Should this be declared after rkisp1_sd_adjust_crop() to maintain the
> same definition ordering as in the C file ?
OK.
> > /* irq handlers */
> > irqreturn_t rkisp1_isp_isr(int irq, void *ctx);
> > irqreturn_t rkisp1_csi_isr(int irq, void *ctx);
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 39a32e98807f..b10cc2701244 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -165,54 +165,20 @@ static void rkisp1_bls_config(struct rkisp1_params *params,
> > new_control &= RKISP1_CIF_ISP_BLS_ENA;
> > /* fixed subtraction values */
> > if (!arg->enable_auto) {
> > - const struct rkisp1_cif_isp_bls_fixed_val *pval =
> > - &arg->fixed_val;
> > + static const u32 regs[] = {
> > + RKISP1_CIF_ISP_BLS_A_FIXED,
> > + RKISP1_CIF_ISP_BLS_B_FIXED,
> > + RKISP1_CIF_ISP_BLS_C_FIXED,
> > + RKISP1_CIF_ISP_BLS_D_FIXED,
> > + };
> > + u32 swapped[4];
> >
> > - switch (params->raw_type) {
> > - case RKISP1_RAW_BGGR:
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> > - pval->r);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> > - pval->gr);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> > - pval->gb);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> > - pval->b);
> > - break;
> > - case RKISP1_RAW_GBRG:
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> > - pval->r);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> > - pval->gr);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> > - pval->gb);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> > - pval->b);
> > - break;
> > - case RKISP1_RAW_GRBG:
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> > - pval->r);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> > - pval->gr);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> > - pval->gb);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> > - pval->b);
> > - break;
> > - case RKISP1_RAW_RGGB:
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_A_FIXED,
> > - pval->r);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_B_FIXED,
> > - pval->gr);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_C_FIXED,
> > - pval->gb);
> > - rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_D_FIXED,
> > - pval->b);
> > - break;
> > - default:
> > - break;
> > - }
> > + rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
>
> Have you considered making rkisp1_bls_swap_regs() shuffle 'regs'
> in-place ? Not strictly necessary, just wondering
Not really. It could work too, but I'd need to go through a temporary
array in rkisp1_bls_swap_regs(), and the caller would need to initialize
the regs array. In the end I think it would just be more costly.
> > + rkisp1_write(params->rkisp1, swapped[0], arg->fixed_val.r);
> > + rkisp1_write(params->rkisp1, swapped[1], arg->fixed_val.gr);
> > + rkisp1_write(params->rkisp1, swapped[2], arg->fixed_val.gb);
> > + rkisp1_write(params->rkisp1, swapped[3], arg->fixed_val.b);
> > } else {
> > if (arg->en_windows & BIT(1)) {
> > rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_BLS_H2_START,
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> > index 2795eef91bdd..a502719e916a 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> > @@ -304,48 +304,25 @@ static void rkisp1_stats_get_hst_meas_v12(struct rkisp1_stats *stats,
> > static void rkisp1_stats_get_bls_meas(struct rkisp1_stats *stats,
> > struct rkisp1_stat_buffer *pbuf)
> > {
> > + static const u32 regs[] = {
> > + RKISP1_CIF_ISP_BLS_A_MEASURED,
> > + RKISP1_CIF_ISP_BLS_B_MEASURED,
> > + RKISP1_CIF_ISP_BLS_C_MEASURED,
> > + RKISP1_CIF_ISP_BLS_D_MEASURED,
> > + };
> > struct rkisp1_device *rkisp1 = stats->rkisp1;
> > const struct rkisp1_mbus_info *in_fmt = rkisp1->isp.sink_fmt;
> > struct rkisp1_cif_isp_bls_meas_val *bls_val;
> > + u32 swapped[4];
> > +
> > + rkisp1_bls_swap_regs(in_fmt->bayer_pat, regs, swapped);
> >
> > bls_val = &pbuf->params.ae.bls_val;
> > - if (in_fmt->bayer_pat == RKISP1_RAW_BGGR) {
> > - bls_val->meas_b =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> > - bls_val->meas_gb =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> > - bls_val->meas_gr =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> > - bls_val->meas_r =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> > - } else if (in_fmt->bayer_pat == RKISP1_RAW_GBRG) {
> > - bls_val->meas_gb =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> > - bls_val->meas_b =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> > - bls_val->meas_r =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> > - bls_val->meas_gr =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> > - } else if (in_fmt->bayer_pat == RKISP1_RAW_GRBG) {
> > - bls_val->meas_gr =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> > - bls_val->meas_r =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> > - bls_val->meas_b =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> > - bls_val->meas_gb =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> > - } else if (in_fmt->bayer_pat == RKISP1_RAW_RGGB) {
> > - bls_val->meas_r =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_A_MEASURED);
> > - bls_val->meas_gr =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_B_MEASURED);
> > - bls_val->meas_gb =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_C_MEASURED);
> > - bls_val->meas_b =
> > - rkisp1_read(rkisp1, RKISP1_CIF_ISP_BLS_D_MEASURED);
> > - }
> > +
> > + bls_val->meas_r = rkisp1_read(rkisp1, swapped[0]);
> > + bls_val->meas_gr = rkisp1_read(rkisp1, swapped[1]);
> > + bls_val->meas_gb = rkisp1_read(rkisp1, swapped[2]);
> > + bls_val->meas_b = rkisp1_read(rkisp1, swapped[3]);
> > }
> >
> > static const struct rkisp1_stats_ops rkisp1_v10_stats_ops = {
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 2/5] media: rkisp1: Add features mask to extensible block handlers
2024-07-03 22:25 [PATCH v1 0/5] media: rkisp1: Add support for the companding block Laurent Pinchart
2024-07-03 22:25 ` [PATCH v1 1/5] media: rkisp1: Add helper function to swap colour channels Laurent Pinchart
@ 2024-07-03 22:25 ` Laurent Pinchart
2024-07-04 8:49 ` Jacopo Mondi
2024-07-03 22:25 ` [PATCH v1 3/5] media: rkisp1: Add register definitions for the companding block Laurent Pinchart
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-03 22:25 UTC (permalink / raw)
To: linux-media; +Cc: Dafna Hirschfeld, Jacopo Mondi, Paul Elder, linux-rockchip
Future ISP parameter blocks for i.MX8MP-specific features will not
support on Rockchip platforms as they lack the corresponding hardware.
Introduce a features mask in the extensible block handlers to indicate
which device features a block require, and ignore blocks that require
unavailable features.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index b10cc2701244..92312b4dabf6 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1845,6 +1845,7 @@ static const struct rkisp1_ext_params_handler {
size_t size;
rkisp1_block_handler handler;
unsigned int group;
+ unsigned int features;
} rkisp1_ext_params_handlers[] = {
[RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = {
.size = sizeof(struct rkisp1_ext_params_bls_config),
@@ -1956,6 +1957,10 @@ static void rkisp1_ext_params_config(struct rkisp1_params *params,
if (!(block_handler->group & block_group_mask))
continue;
+ if ((block_handler->features & params->rkisp1->info->features) !=
+ params->rkisp1->info->features)
+ continue;
+
block_handler->handler(params, block);
if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE)
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 2/5] media: rkisp1: Add features mask to extensible block handlers
2024-07-03 22:25 ` [PATCH v1 2/5] media: rkisp1: Add features mask to extensible block handlers Laurent Pinchart
@ 2024-07-04 8:49 ` Jacopo Mondi
0 siblings, 0 replies; 15+ messages in thread
From: Jacopo Mondi @ 2024-07-04 8:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Dafna Hirschfeld, Jacopo Mondi, Paul Elder,
linux-rockchip
Hi Laurent
On Thu, Jul 04, 2024 at 01:25:30AM GMT, Laurent Pinchart wrote:
> Future ISP parameter blocks for i.MX8MP-specific features will not
> support on Rockchip platforms as they lack the corresponding hardware.
> Introduce a features mask in the extensible block handlers to indicate
> which device features a block require, and ignore blocks that require
> unavailable features.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index b10cc2701244..92312b4dabf6 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -1845,6 +1845,7 @@ static const struct rkisp1_ext_params_handler {
> size_t size;
> rkisp1_block_handler handler;
> unsigned int group;
> + unsigned int features;
> } rkisp1_ext_params_handlers[] = {
> [RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS] = {
> .size = sizeof(struct rkisp1_ext_params_bls_config),
> @@ -1956,6 +1957,10 @@ static void rkisp1_ext_params_config(struct rkisp1_params *params,
Maybe exapand the comment to also specify the feature has to be
supported by the platform ?
/*
* Make sure the block is supported by the platform and in the
* list of groups to configure.
*/
> if (!(block_handler->group & block_group_mask))
> continue;
>
> + if ((block_handler->features & params->rkisp1->info->features) !=
> + params->rkisp1->info->features)
This should probably be
if ((block_handler->features & params->rkisp1->info->features) !=
block_handler->features)
Thanks
j
> + continue;
> +
> block_handler->handler(params, block);
>
> if (block->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE)
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 3/5] media: rkisp1: Add register definitions for the companding block
2024-07-03 22:25 [PATCH v1 0/5] media: rkisp1: Add support for the companding block Laurent Pinchart
2024-07-03 22:25 ` [PATCH v1 1/5] media: rkisp1: Add helper function to swap colour channels Laurent Pinchart
2024-07-03 22:25 ` [PATCH v1 2/5] media: rkisp1: Add features mask to extensible block handlers Laurent Pinchart
@ 2024-07-03 22:25 ` Laurent Pinchart
2024-07-04 8:56 ` Jacopo Mondi
2024-07-03 22:25 ` [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand Laurent Pinchart
2024-07-03 22:25 ` [PATCH v1 5/5] media: rkisp1: Add support for the companding block Laurent Pinchart
4 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-03 22:25 UTC (permalink / raw)
To: linux-media; +Cc: Dafna Hirschfeld, Jacopo Mondi, Paul Elder, linux-rockchip
From: Paul Elder <paul.elder@ideasonboard.com>
To prepare for adding support for the companding block to the rkisp1
driver for the version of the ISP on the i.MX8MP, add the register
definitions for it, including relevant register field values.
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
.../platform/rockchip/rkisp1/rkisp1-regs.h | 22 +++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
index fccf4c17ee8d..f516c9c1b822 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
@@ -704,6 +704,11 @@
#define RKISP1_CIF_ISP_DPF_SPATIAL_COEFF_MAX 0x1f
#define RKISP1_CIF_ISP_DPF_NLL_COEFF_N_MAX 0x3ff
+#define RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE BIT(0)
+#define RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE BIT(1)
+#define RKISP1_CIF_ISP_COMPAND_CTRL_SOFT_RESET_FLAG BIT(2)
+#define RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE BIT(3)
+
/* =================================================================== */
/* CIF Registers */
/* =================================================================== */
@@ -1394,6 +1399,23 @@
#define RKISP1_CIF_ISP_VSM_DELTA_H (RKISP1_CIF_ISP_VSM_BASE + 0x0000001c)
#define RKISP1_CIF_ISP_VSM_DELTA_V (RKISP1_CIF_ISP_VSM_BASE + 0x00000020)
+#define RKISP1_CIF_ISP_COMPAND_BASE 0x00003200
+#define RKISP1_CIF_ISP_COMPAND_CTRL (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000000)
+#define RKISP1_CIF_ISP_COMPAND_BLS_A_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000004)
+#define RKISP1_CIF_ISP_COMPAND_BLS_B_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000008)
+#define RKISP1_CIF_ISP_COMPAND_BLS_C_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x0000000c)
+#define RKISP1_CIF_ISP_COMPAND_BLS_D_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000010)
+#define RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(n) (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000014 + (n) * 4)
+#define RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(n) (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000040 + (n) * 4)
+#define RKISP1_CIF_ISP_COMPAND_EXPAND_Y_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x0000006c)
+#define RKISP1_CIF_ISP_COMPAND_EXPAND_Y_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000070)
+#define RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000074)
+#define RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000078)
+#define RKISP1_CIF_ISP_COMPAND_EXPAND_X_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x0000007c)
+#define RKISP1_CIF_ISP_COMPAND_EXPAND_X_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000080)
+#define RKISP1_CIF_ISP_COMPAND_COMPRESS_X_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000084)
+#define RKISP1_CIF_ISP_COMPAND_COMPRESS_X_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000088)
+
#define RKISP1_CIF_ISP_CSI0_BASE 0x00007000
#define RKISP1_CIF_ISP_CSI0_CTRL0 (RKISP1_CIF_ISP_CSI0_BASE + 0x00000000)
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 3/5] media: rkisp1: Add register definitions for the companding block
2024-07-03 22:25 ` [PATCH v1 3/5] media: rkisp1: Add register definitions for the companding block Laurent Pinchart
@ 2024-07-04 8:56 ` Jacopo Mondi
0 siblings, 0 replies; 15+ messages in thread
From: Jacopo Mondi @ 2024-07-04 8:56 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Dafna Hirschfeld, Jacopo Mondi, Paul Elder,
linux-rockchip
Hi Laurent
On Thu, Jul 04, 2024 at 01:25:31AM GMT, Laurent Pinchart wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
>
> To prepare for adding support for the companding block to the rkisp1
> driver for the version of the ISP on the i.MX8MP, add the register
> definitions for it, including relevant register field values.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> .../platform/rockchip/rkisp1/rkisp1-regs.h | 22 +++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> index fccf4c17ee8d..f516c9c1b822 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
> @@ -704,6 +704,11 @@
> #define RKISP1_CIF_ISP_DPF_SPATIAL_COEFF_MAX 0x1f
> #define RKISP1_CIF_ISP_DPF_NLL_COEFF_N_MAX 0x3ff
>
The other blocks are preceded by a comment, should you add
/* COMPAND */
here ?
> +#define RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE BIT(0)
> +#define RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE BIT(1)
> +#define RKISP1_CIF_ISP_COMPAND_CTRL_SOFT_RESET_FLAG BIT(2)
> +#define RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE BIT(3)
> +
> /* =================================================================== */
> /* CIF Registers */
> /* =================================================================== */
> @@ -1394,6 +1399,23 @@
> #define RKISP1_CIF_ISP_VSM_DELTA_H (RKISP1_CIF_ISP_VSM_BASE + 0x0000001c)
> #define RKISP1_CIF_ISP_VSM_DELTA_V (RKISP1_CIF_ISP_VSM_BASE + 0x00000020)
>
> +#define RKISP1_CIF_ISP_COMPAND_BASE 0x00003200
> +#define RKISP1_CIF_ISP_COMPAND_CTRL (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000000)
> +#define RKISP1_CIF_ISP_COMPAND_BLS_A_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000004)
> +#define RKISP1_CIF_ISP_COMPAND_BLS_B_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000008)
> +#define RKISP1_CIF_ISP_COMPAND_BLS_C_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x0000000c)
> +#define RKISP1_CIF_ISP_COMPAND_BLS_D_FIXED (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000010)
> +#define RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(n) (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000014 + (n) * 4)
> +#define RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(n) (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000040 + (n) * 4)
> +#define RKISP1_CIF_ISP_COMPAND_EXPAND_Y_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x0000006c)
> +#define RKISP1_CIF_ISP_COMPAND_EXPAND_Y_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000070)
> +#define RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000074)
> +#define RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000078)
> +#define RKISP1_CIF_ISP_COMPAND_EXPAND_X_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x0000007c)
> +#define RKISP1_CIF_ISP_COMPAND_EXPAND_X_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000080)
> +#define RKISP1_CIF_ISP_COMPAND_COMPRESS_X_ADDR (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000084)
> +#define RKISP1_CIF_ISP_COMPAND_COMPRESS_X_WRITE_DATA (RKISP1_CIF_ISP_COMPAND_BASE + 0x00000088)
> +
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Thanks
j
> #define RKISP1_CIF_ISP_CSI0_BASE 0x00007000
> #define RKISP1_CIF_ISP_CSI0_CTRL0 (RKISP1_CIF_ISP_CSI0_BASE + 0x00000000)
>
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand
2024-07-03 22:25 [PATCH v1 0/5] media: rkisp1: Add support for the companding block Laurent Pinchart
` (2 preceding siblings ...)
2024-07-03 22:25 ` [PATCH v1 3/5] media: rkisp1: Add register definitions for the companding block Laurent Pinchart
@ 2024-07-03 22:25 ` Laurent Pinchart
2024-07-04 8:34 ` Jacopo Mondi
2024-07-03 22:25 ` [PATCH v1 5/5] media: rkisp1: Add support for the companding block Laurent Pinchart
4 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-03 22:25 UTC (permalink / raw)
To: linux-media; +Cc: Dafna Hirschfeld, Jacopo Mondi, Paul Elder, linux-rockchip
From: Paul Elder <paul.elder@ideasonboard.com>
Add feature flags for the dedicated black level subtraction hardware
block and for the compand hardware block. The companding feature flag is
added on its own (as opposed to "the absence of BLS") because we will
need it later for when we add support for the companding block.
Skip BLS configuration when the BLS feature flag is unset, as devices
without the dedicated BLS block cannot configure a hardware block that
doesn't exist.
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 4 ++++
drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 9 ++++++---
drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 7 +++++++
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index cdf2d30e2bb1..607e1a024d02 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -116,6 +116,8 @@ enum rkisp1_isp_pad {
* @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
* @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the resizer input
* @RKISP1_FEATURE_DMA_34BIT: The ISP uses 34-bit DMA addresses
+ * @RKISP1_FEATURE_BLS: The ISP has a dedicated BLS block
+ * @RKISP1_FEATURE_COMPAND: The ISP has a companding block
*
* The ISP features are stored in a bitmask in &rkisp1_info.features and allow
* the driver to implement support for features present in some ISP versions
@@ -127,6 +129,8 @@ enum rkisp1_feature {
RKISP1_FEATURE_SELF_PATH = BIT(2),
RKISP1_FEATURE_DUAL_CROP = BIT(3),
RKISP1_FEATURE_DMA_34BIT = BIT(4),
+ RKISP1_FEATURE_BLS = BIT(5),
+ RKISP1_FEATURE_COMPAND = BIT(6),
};
#define rkisp1_has_feature(rkisp1, feature) \
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 0535ce57e862..dd114ab77800 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -509,7 +509,8 @@ static const struct rkisp1_info px30_isp_info = {
.isp_ver = RKISP1_V12,
.features = RKISP1_FEATURE_MIPI_CSI2
| RKISP1_FEATURE_SELF_PATH
- | RKISP1_FEATURE_DUAL_CROP,
+ | RKISP1_FEATURE_DUAL_CROP
+ | RKISP1_FEATURE_BLS,
.max_width = 3264,
.max_height = 2448,
};
@@ -532,7 +533,8 @@ static const struct rkisp1_info rk3399_isp_info = {
.isp_ver = RKISP1_V10,
.features = RKISP1_FEATURE_MIPI_CSI2
| RKISP1_FEATURE_SELF_PATH
- | RKISP1_FEATURE_DUAL_CROP,
+ | RKISP1_FEATURE_DUAL_CROP
+ | RKISP1_FEATURE_BLS,
.max_width = 4416,
.max_height = 3312,
};
@@ -554,7 +556,8 @@ static const struct rkisp1_info imx8mp_isp_info = {
.isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
.isp_ver = RKISP1_V_IMX8MP,
.features = RKISP1_FEATURE_MAIN_STRIDE
- | RKISP1_FEATURE_DMA_34BIT,
+ | RKISP1_FEATURE_DMA_34BIT
+ | RKISP1_FEATURE_COMPAND,
.max_width = 4096,
.max_height = 3072,
};
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 92312b4dabf6..bac9d4972493 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1268,6 +1268,12 @@ rkisp1_isp_isr_other_config(struct rkisp1_params *params,
module_cfg_update = new_params->module_cfg_update;
module_ens = new_params->module_ens;
+ if (!rkisp1_has_feature(params->rkisp1, BLS)) {
+ module_en_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
+ module_cfg_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
+ module_ens &= ~RKISP1_CIF_ISP_MODULE_BLS;
+ }
+
/* update dpc config */
if (module_cfg_update & RKISP1_CIF_ISP_MODULE_DPCC)
rkisp1_dpcc_config(params,
@@ -1851,6 +1857,7 @@ static const struct rkisp1_ext_params_handler {
.size = sizeof(struct rkisp1_ext_params_bls_config),
.handler = rkisp1_ext_params_bls,
.group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
+ .features = RKISP1_FEATURE_BLS,
},
[RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = {
.size = sizeof(struct rkisp1_ext_params_dpcc_config),
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand
2024-07-03 22:25 ` [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand Laurent Pinchart
@ 2024-07-04 8:34 ` Jacopo Mondi
2024-07-04 9:00 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Jacopo Mondi @ 2024-07-04 8:34 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Dafna Hirschfeld, Jacopo Mondi, Paul Elder,
linux-rockchip
Hi Laurent
On Thu, Jul 04, 2024 at 01:25:32AM GMT, Laurent Pinchart wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
>
> Add feature flags for the dedicated black level subtraction hardware
> block and for the compand hardware block. The companding feature flag is
> added on its own (as opposed to "the absence of BLS") because we will
> need it later for when we add support for the companding block.
>
> Skip BLS configuration when the BLS feature flag is unset, as devices
> without the dedicated BLS block cannot configure a hardware block that
> doesn't exist.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 4 ++++
> drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 9 ++++++---
> drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 7 +++++++
> 3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index cdf2d30e2bb1..607e1a024d02 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -116,6 +116,8 @@ enum rkisp1_isp_pad {
> * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
> * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the resizer input
> * @RKISP1_FEATURE_DMA_34BIT: The ISP uses 34-bit DMA addresses
> + * @RKISP1_FEATURE_BLS: The ISP has a dedicated BLS block
> + * @RKISP1_FEATURE_COMPAND: The ISP has a companding block
> *
> * The ISP features are stored in a bitmask in &rkisp1_info.features and allow
> * the driver to implement support for features present in some ISP versions
> @@ -127,6 +129,8 @@ enum rkisp1_feature {
> RKISP1_FEATURE_SELF_PATH = BIT(2),
> RKISP1_FEATURE_DUAL_CROP = BIT(3),
> RKISP1_FEATURE_DMA_34BIT = BIT(4),
> + RKISP1_FEATURE_BLS = BIT(5),
> + RKISP1_FEATURE_COMPAND = BIT(6),
> };
>
> #define rkisp1_has_feature(rkisp1, feature) \
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 0535ce57e862..dd114ab77800 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -509,7 +509,8 @@ static const struct rkisp1_info px30_isp_info = {
> .isp_ver = RKISP1_V12,
> .features = RKISP1_FEATURE_MIPI_CSI2
> | RKISP1_FEATURE_SELF_PATH
> - | RKISP1_FEATURE_DUAL_CROP,
> + | RKISP1_FEATURE_DUAL_CROP
> + | RKISP1_FEATURE_BLS,
Doesn't apply for me on top of [media-stage/master + my ext params
series]
In particular, in media-stage I don't see
> .max_width = 3264,
> .max_height = 2448,
these
> };
> @@ -532,7 +533,8 @@ static const struct rkisp1_info rk3399_isp_info = {
> .isp_ver = RKISP1_V10,
> .features = RKISP1_FEATURE_MIPI_CSI2
> | RKISP1_FEATURE_SELF_PATH
> - | RKISP1_FEATURE_DUAL_CROP,
> + | RKISP1_FEATURE_DUAL_CROP
> + | RKISP1_FEATURE_BLS,
> .max_width = 4416,
> .max_height = 3312,
> };
> @@ -554,7 +556,8 @@ static const struct rkisp1_info imx8mp_isp_info = {
> .isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> .isp_ver = RKISP1_V_IMX8MP,
> .features = RKISP1_FEATURE_MAIN_STRIDE
> - | RKISP1_FEATURE_DMA_34BIT,
> + | RKISP1_FEATURE_DMA_34BIT
> + | RKISP1_FEATURE_COMPAND,
> .max_width = 4096,
> .max_height = 3072,
> };
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 92312b4dabf6..bac9d4972493 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -1268,6 +1268,12 @@ rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> module_cfg_update = new_params->module_cfg_update;
> module_ens = new_params->module_ens;
>
> + if (!rkisp1_has_feature(params->rkisp1, BLS)) {
> + module_en_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
> + module_cfg_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
> + module_ens &= ~RKISP1_CIF_ISP_MODULE_BLS;
> + }
> +
or is it easier to read if you
if (rkisp1_has_feature(params->rkisp1, BLS)) {
/* update bls config */
if (module_cfg_update & RKISP1_CIF_ISP_MODULE_BLS)
rkisp1_bls_config(params,
&new_params->others.bls_config);
if (module_en_update & RKISP1_CIF_ISP_MODULE_BLS) {
if (module_ens & RKISP1_CIF_ISP_MODULE_BLS)
rkisp1_param_set_bits(params,
RKISP1_CIF_ISP_BLS_CTRL,
RKISP1_CIF_ISP_BLS_ENA);
else
rkisp1_param_clear_bits(params,
RKISP1_CIF_ISP_BLS_CTRL,
RKISP1_CIF_ISP_BLS_ENA);
}
}
below ?
> /* update dpc config */
> if (module_cfg_update & RKISP1_CIF_ISP_MODULE_DPCC)
> rkisp1_dpcc_config(params,
> @@ -1851,6 +1857,7 @@ static const struct rkisp1_ext_params_handler {
> .size = sizeof(struct rkisp1_ext_params_bls_config),
> .handler = rkisp1_ext_params_bls,
> .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> + .features = RKISP1_FEATURE_BLS,
> },
> [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = {
> .size = sizeof(struct rkisp1_ext_params_dpcc_config),
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand
2024-07-04 8:34 ` Jacopo Mondi
@ 2024-07-04 9:00 ` Laurent Pinchart
0 siblings, 0 replies; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-04 9:00 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: linux-media, Dafna Hirschfeld, Paul Elder, linux-rockchip
On Thu, Jul 04, 2024 at 10:34:20AM +0200, Jacopo Mondi wrote:
> On Thu, Jul 04, 2024 at 01:25:32AM GMT, Laurent Pinchart wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> >
> > Add feature flags for the dedicated black level subtraction hardware
> > block and for the compand hardware block. The companding feature flag is
> > added on its own (as opposed to "the absence of BLS") because we will
> > need it later for when we add support for the companding block.
> >
> > Skip BLS configuration when the BLS feature flag is unset, as devices
> > without the dedicated BLS block cannot configure a hardware block that
> > doesn't exist.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 4 ++++
> > drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c | 9 ++++++---
> > drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 7 +++++++
> > 3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index cdf2d30e2bb1..607e1a024d02 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -116,6 +116,8 @@ enum rkisp1_isp_pad {
> > * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
> > * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the resizer input
> > * @RKISP1_FEATURE_DMA_34BIT: The ISP uses 34-bit DMA addresses
> > + * @RKISP1_FEATURE_BLS: The ISP has a dedicated BLS block
> > + * @RKISP1_FEATURE_COMPAND: The ISP has a companding block
> > *
> > * The ISP features are stored in a bitmask in &rkisp1_info.features and allow
> > * the driver to implement support for features present in some ISP versions
> > @@ -127,6 +129,8 @@ enum rkisp1_feature {
> > RKISP1_FEATURE_SELF_PATH = BIT(2),
> > RKISP1_FEATURE_DUAL_CROP = BIT(3),
> > RKISP1_FEATURE_DMA_34BIT = BIT(4),
> > + RKISP1_FEATURE_BLS = BIT(5),
> > + RKISP1_FEATURE_COMPAND = BIT(6),
> > };
> >
> > #define rkisp1_has_feature(rkisp1, feature) \
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index 0535ce57e862..dd114ab77800 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -509,7 +509,8 @@ static const struct rkisp1_info px30_isp_info = {
> > .isp_ver = RKISP1_V12,
> > .features = RKISP1_FEATURE_MIPI_CSI2
> > | RKISP1_FEATURE_SELF_PATH
> > - | RKISP1_FEATURE_DUAL_CROP,
> > + | RKISP1_FEATURE_DUAL_CROP
> > + | RKISP1_FEATURE_BLS,
>
> Doesn't apply for me on top of [media-stage/master + my ext params
> series]
>
> In particular, in media-stage I don't see
> > .max_width = 3264,
> > .max_height = 2448,
>
> these
There are a few prerequisites. The cover letter lists the base commit
ID. The fact that I haven't pushed a branch anywhere doesn't help
obviously... I'll make it clearer in v2.
> > };
> > @@ -532,7 +533,8 @@ static const struct rkisp1_info rk3399_isp_info = {
> > .isp_ver = RKISP1_V10,
> > .features = RKISP1_FEATURE_MIPI_CSI2
> > | RKISP1_FEATURE_SELF_PATH
> > - | RKISP1_FEATURE_DUAL_CROP,
> > + | RKISP1_FEATURE_DUAL_CROP
> > + | RKISP1_FEATURE_BLS,
> > .max_width = 4416,
> > .max_height = 3312,
> > };
> > @@ -554,7 +556,8 @@ static const struct rkisp1_info imx8mp_isp_info = {
> > .isr_size = ARRAY_SIZE(imx8mp_isp_isrs),
> > .isp_ver = RKISP1_V_IMX8MP,
> > .features = RKISP1_FEATURE_MAIN_STRIDE
> > - | RKISP1_FEATURE_DMA_34BIT,
> > + | RKISP1_FEATURE_DMA_34BIT
> > + | RKISP1_FEATURE_COMPAND,
> > .max_width = 4096,
> > .max_height = 3072,
> > };
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 92312b4dabf6..bac9d4972493 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -1268,6 +1268,12 @@ rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> > module_cfg_update = new_params->module_cfg_update;
> > module_ens = new_params->module_ens;
> >
> > + if (!rkisp1_has_feature(params->rkisp1, BLS)) {
> > + module_en_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
> > + module_cfg_update &= ~RKISP1_CIF_ISP_MODULE_BLS;
> > + module_ens &= ~RKISP1_CIF_ISP_MODULE_BLS;
> > + }
> > +
>
> or is it easier to read if you
>
> if (rkisp1_has_feature(params->rkisp1, BLS)) {
> /* update bls config */
> if (module_cfg_update & RKISP1_CIF_ISP_MODULE_BLS)
> rkisp1_bls_config(params,
> &new_params->others.bls_config);
>
> if (module_en_update & RKISP1_CIF_ISP_MODULE_BLS) {
> if (module_ens & RKISP1_CIF_ISP_MODULE_BLS)
> rkisp1_param_set_bits(params,
> RKISP1_CIF_ISP_BLS_CTRL,
> RKISP1_CIF_ISP_BLS_ENA);
> else
> rkisp1_param_clear_bits(params,
> RKISP1_CIF_ISP_BLS_CTRL,
> RKISP1_CIF_ISP_BLS_ENA);
> }
> }
>
> below ?
I was considering it. Lower indentation is nice, and I thought that we
could centralize all the feature checks in one place, but I don't mind
much either way. If you have a stronger preference I can change this.
> > /* update dpc config */
> > if (module_cfg_update & RKISP1_CIF_ISP_MODULE_DPCC)
> > rkisp1_dpcc_config(params,
> > @@ -1851,6 +1857,7 @@ static const struct rkisp1_ext_params_handler {
> > .size = sizeof(struct rkisp1_ext_params_bls_config),
> > .handler = rkisp1_ext_params_bls,
> > .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > + .features = RKISP1_FEATURE_BLS,
> > },
> > [RKISP1_EXT_PARAMS_BLOCK_TYPE_DPCC] = {
> > .size = sizeof(struct rkisp1_ext_params_dpcc_config),
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 5/5] media: rkisp1: Add support for the companding block
2024-07-03 22:25 [PATCH v1 0/5] media: rkisp1: Add support for the companding block Laurent Pinchart
` (3 preceding siblings ...)
2024-07-03 22:25 ` [PATCH v1 4/5] media: rkisp1: Add feature flags for BLS and compand Laurent Pinchart
@ 2024-07-03 22:25 ` Laurent Pinchart
2024-07-04 10:40 ` Jacopo Mondi
4 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-03 22:25 UTC (permalink / raw)
To: linux-media; +Cc: Dafna Hirschfeld, Jacopo Mondi, Paul Elder, linux-rockchip
From: Paul Elder <paul.elder@ideasonboard.com>
Add support to the rkisp1 driver for the companding block that exists on
the i.MX8MP version of the ISP. This requires usage of the new
extensible parameters format, and showcases how the format allows for
extensions without breaking backward compatibility.
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v0:
- Drop RKISP1_EXT_PARAM_BUFFER_V2
- Use common structure for compression and expansion curves
- Rename config fields in rkisp1_ext_params_*_config to just config
- Mention block type in structures documentation
- Constify arguments
- Replace __uxx types with uxx
- Use rkisp1_bls_swap_regs() helper in rkisp1_compand_bls_config()
- Use generic feature handling mechanism
---
.../platform/rockchip/rkisp1/rkisp1-params.c | 166 ++++++++++++++++++
include/uapi/linux/rkisp1-config.h | 85 ++++++++-
2 files changed, 250 insertions(+), 1 deletion(-)
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index bac9d4972493..5865d53be9c8 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -57,6 +57,8 @@ union rkisp1_ext_params_config {
struct rkisp1_ext_params_hst_config hst;
struct rkisp1_ext_params_aec_config aec;
struct rkisp1_ext_params_afc_config afc;
+ struct rkisp1_ext_params_compand_bls_config compand_bls;
+ struct rkisp1_ext_params_compand_curve_config compand_curve;
};
enum rkisp1_params_formats {
@@ -1258,6 +1260,92 @@ rkisp1_dpf_strength_config(struct rkisp1_params *params,
rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_DPF_STRENGTH_R, arg->r);
}
+static void rkisp1_compand_write_px_curve(struct rkisp1_params *params,
+ unsigned int addr, const u8 *px_curve)
+{
+ size_t i, j;
+ u32 val;
+
+ /*
+ * The compand curve is specified as a piecewise linear function with
+ * 64 points. X coordinates are stored as a log2 of the displacement
+ * from the previous point, in 5 bits, with 6 values per register. The
+ * last register stores 4 values.
+ */
+ for (i = 0; i < 10; i++) {
+ val = 0;
+ for (j = 0; j < 6; j++)
+ val |= ((px_curve[i * 6 + j] & 0x1f) << (j * 5));
+ rkisp1_write(params->rkisp1, addr + (i * 4), val);
+ }
+
+ val = 0;
+ for (j = 0; j < 4; j++)
+ val |= ((px_curve[60 + j] & 0x1f) << (j * 5));
+ rkisp1_write(params->rkisp1, addr + (i * 4), val);
+}
+
+static void
+rkisp1_compand_write_curve_mem(struct rkisp1_params *params,
+ unsigned int reg_addr, unsigned int reg_data,
+ size_t num_samples, const u32 *curve)
+{
+ size_t i;
+
+ for (i = 0; i < num_samples; i++) {
+ rkisp1_write(params->rkisp1, reg_addr, i);
+ rkisp1_write(params->rkisp1, reg_data, curve[i]);
+ }
+}
+
+static void
+rkisp1_compand_bls_config(struct rkisp1_params *params,
+ const struct rkisp1_cif_isp_compand_bls_config *arg)
+{
+ static const u32 regs[] = {
+ RKISP1_CIF_ISP_COMPAND_BLS_A_FIXED,
+ RKISP1_CIF_ISP_COMPAND_BLS_B_FIXED,
+ RKISP1_CIF_ISP_COMPAND_BLS_C_FIXED,
+ RKISP1_CIF_ISP_COMPAND_BLS_D_FIXED,
+ };
+ u32 swapped[4];
+
+ rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
+
+ rkisp1_write(params->rkisp1, swapped[0], arg->r);
+ rkisp1_write(params->rkisp1, swapped[1], arg->gr);
+ rkisp1_write(params->rkisp1, swapped[2], arg->gb);
+ rkisp1_write(params->rkisp1, swapped[3], arg->b);
+}
+
+static void
+rkisp1_compand_expand_config(struct rkisp1_params *params,
+ const struct rkisp1_cif_isp_compand_curve_config *arg)
+{
+ rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(0),
+ arg->px);
+ rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_Y_ADDR,
+ RKISP1_CIF_ISP_COMPAND_EXPAND_Y_WRITE_DATA,
+ ARRAY_SIZE(arg->y), arg->y);
+ rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_X_ADDR,
+ RKISP1_CIF_ISP_COMPAND_EXPAND_X_WRITE_DATA,
+ ARRAY_SIZE(arg->x), arg->x);
+}
+
+static void
+rkisp1_compand_compress_config(struct rkisp1_params *params,
+ const struct rkisp1_cif_isp_compand_curve_config *arg)
+{
+ rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(0),
+ arg->px);
+ rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_ADDR,
+ RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_WRITE_DATA,
+ ARRAY_SIZE(arg->y), arg->y);
+ rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_X_ADDR,
+ RKISP1_CIF_ISP_COMPAND_COMPRESS_X_WRITE_DATA,
+ ARRAY_SIZE(arg->x), arg->x);
+}
+
static void
rkisp1_isp_isr_other_config(struct rkisp1_params *params,
const struct rkisp1_params_cfg *new_params)
@@ -1844,6 +1932,66 @@ rkisp1_ext_params_afcm(struct rkisp1_params *params,
RKISP1_CIF_ISP_AFM_ENA);
}
+static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
+ const union rkisp1_ext_params_config *block)
+{
+ const struct rkisp1_ext_params_compand_bls_config *bls =
+ &block->compand_bls;
+
+ if (bls->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
+ rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
+ RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
+ return;
+ }
+
+ rkisp1_compand_bls_config(params, &bls->config);
+
+ if (!(params->enabled_blocks &
+ BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS)))
+ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
+ RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
+}
+
+static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
+ const union rkisp1_ext_params_config *block)
+{
+ const struct rkisp1_ext_params_compand_curve_config *curve =
+ &block->compand_curve;
+
+ if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
+ rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
+ RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
+ return;
+ }
+
+ rkisp1_compand_expand_config(params, &curve->config);
+
+ if (!(params->enabled_blocks &
+ BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND)))
+ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
+ RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
+}
+
+static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
+ const union rkisp1_ext_params_config *block)
+{
+ const struct rkisp1_ext_params_compand_curve_config *curve =
+ &block->compand_curve;
+
+ if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
+ rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
+ RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
+ return;
+ }
+
+ rkisp1_compand_compress_config(params, &curve->config);
+
+ if (!(params->enabled_blocks &
+ BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS)))
+ rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
+ RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
+}
+
typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
const union rkisp1_ext_params_config *config);
@@ -1939,6 +2087,24 @@ static const struct rkisp1_ext_params_handler {
.handler = rkisp1_ext_params_afcm,
.group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
},
+ [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS] = {
+ .size = sizeof(struct rkisp1_ext_params_compand_bls_config),
+ .handler = rkisp1_ext_params_compand_bls,
+ .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
+ .features = RKISP1_FEATURE_COMPAND,
+ },
+ [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND] = {
+ .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
+ .handler = rkisp1_ext_params_compand_expand,
+ .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
+ .features = RKISP1_FEATURE_COMPAND,
+ },
+ [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS] = {
+ .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
+ .handler = rkisp1_ext_params_compand_compress,
+ .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
+ .features = RKISP1_FEATURE_COMPAND,
+ },
};
static void rkisp1_ext_params_config(struct rkisp1_params *params,
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 00b09c92cca7..dd962df53af5 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -164,6 +164,11 @@
#define RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS 17
#define RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS 6
+/*
+ * Compand
+ */
+#define RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64
+
/*
* Measurement types
*/
@@ -851,6 +856,39 @@ struct rkisp1_params_cfg {
struct rkisp1_cif_isp_isp_other_cfg others;
};
+/**
+ * struct rkisp1_cif_isp_compand_bls_config - Rockchip ISP1 Companding parameters (BLS)
+ * @r: Fixed subtraction value for Bayer pattern R
+ * @gr: Fixed subtraction value for Bayer pattern Gr
+ * @gb: Fixed subtraction value for Bayer pattern Gb
+ * @b: Fixed subtraction value for Bayer pattern B
+ *
+ * The values will be subtracted from the sensor values. Note that unlike the
+ * dedicated BLS block, the BLS values in the compander are 20-bit unsigned.
+ */
+struct rkisp1_cif_isp_compand_bls_config {
+ __u32 r;
+ __u32 gr;
+ __u32 gb;
+ __u32 b;
+};
+
+/**
+ * struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
+ * parameters (expand and compression curves)
+ * @px: Compand curve x-values. Each value stores the distance from the
+ * previous x-value, expressed as log2 of the distance on 5 bits.
+ * @x: Compand curve x-values. The functionality of these parameters are
+ * unknown to do a lack of hardware documentation, but these are left here
+ * for future compatibility purposes.
+ * @y: Compand curve y-values
+ */
+struct rkisp1_cif_isp_compand_curve_config {
+ __u8 px[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
+ __u32 x[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
+ __u32 y[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
+};
+
/*---------- PART2: Measurement Statistics ------------*/
/**
@@ -1018,6 +1056,9 @@ struct rkisp1_stat_buffer {
* @RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram statistics
* @RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto exposure statistics
* @RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS: Auto-focus statistics
+ * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS: BLS in the compand block
+ * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND: Companding expand curve
+ * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS: Compandding compress curve
*/
enum rkisp1_ext_params_block_type {
RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS,
@@ -1037,6 +1078,9 @@ enum rkisp1_ext_params_block_type {
RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS,
RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS,
RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS,
+ RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS,
+ RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND,
+ RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS,
};
/**
@@ -1384,6 +1428,42 @@ struct rkisp1_ext_params_afc_config {
struct rkisp1_cif_isp_afc_config config;
} __attribute__((aligned(8)));
+/**
+ * struct rkisp1_ext_params_compand_bls_config - RkISP1 extensible params
+ * Compand BLS config
+ *
+ * RkISP1 extensible parameters Companding configuration block (black level
+ * subtraction). Identified by :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS`.
+ *
+ * @header: The RkISP1 extensible parameters header, see
+ * :c:type:`rkisp1_ext_params_block_header`
+ * @config: Companding BLS configuration, see
+ * :c:type:`rkisp1_cif_isp_compand_bls_config`
+ */
+struct rkisp1_ext_params_compand_bls_config {
+ struct rkisp1_ext_params_block_header header;
+ struct rkisp1_cif_isp_compand_bls_config config;
+} __attribute__((aligned(8)));
+
+/**
+ * struct rkisp1_ext_params_compand_curve_config - RkISP1 extensible params
+ * Compand curve config
+ *
+ * RkISP1 extensible parameters Companding configuration block (expand and
+ * compression curves). Identified by
+ * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND`or
+ * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS`.
+ *
+ * @header: The RkISP1 extensible parameters header, see
+ * :c:type:`rkisp1_ext_params_block_header`
+ * @config: Companding curve configuration, see
+ * :c:type:`rkisp1_cif_isp_compand_curve_config`
+ */
+struct rkisp1_ext_params_compand_curve_config {
+ struct rkisp1_ext_params_block_header header;
+ struct rkisp1_cif_isp_compand_curve_config config;
+} __attribute__((aligned(8)));
+
#define RKISP1_EXT_PARAMS_MAX_SIZE \
(sizeof(struct rkisp1_ext_params_bls_config) +\
sizeof(struct rkisp1_ext_params_dpcc_config) +\
@@ -1401,7 +1481,10 @@ struct rkisp1_ext_params_afc_config {
sizeof(struct rkisp1_ext_params_awb_meas_config) +\
sizeof(struct rkisp1_ext_params_hst_config) +\
sizeof(struct rkisp1_ext_params_aec_config) +\
- sizeof(struct rkisp1_ext_params_afc_config))
+ sizeof(struct rkisp1_ext_params_afc_config) +\
+ sizeof(struct rkisp1_ext_params_compand_bls_config) +\
+ sizeof(struct rkisp1_ext_params_compand_curve_config) +\
+ sizeof(struct rkisp1_ext_params_compand_curve_config))
/**
* enum rksip1_ext_param_buffer_version - RkISP1 extensible parameters version
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v1 5/5] media: rkisp1: Add support for the companding block
2024-07-03 22:25 ` [PATCH v1 5/5] media: rkisp1: Add support for the companding block Laurent Pinchart
@ 2024-07-04 10:40 ` Jacopo Mondi
2024-07-04 12:53 ` Laurent Pinchart
0 siblings, 1 reply; 15+ messages in thread
From: Jacopo Mondi @ 2024-07-04 10:40 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Dafna Hirschfeld, Jacopo Mondi, Paul Elder,
linux-rockchip
Hi Laurent
On Thu, Jul 04, 2024 at 01:25:33AM GMT, Laurent Pinchart wrote:
> From: Paul Elder <paul.elder@ideasonboard.com>
>
> Add support to the rkisp1 driver for the companding block that exists on
> the i.MX8MP version of the ISP. This requires usage of the new
> extensible parameters format, and showcases how the format allows for
> extensions without breaking backward compatibility.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v0:
>
> - Drop RKISP1_EXT_PARAM_BUFFER_V2
> - Use common structure for compression and expansion curves
> - Rename config fields in rkisp1_ext_params_*_config to just config
> - Mention block type in structures documentation
> - Constify arguments
> - Replace __uxx types with uxx
> - Use rkisp1_bls_swap_regs() helper in rkisp1_compand_bls_config()
> - Use generic feature handling mechanism
> ---
> .../platform/rockchip/rkisp1/rkisp1-params.c | 166 ++++++++++++++++++
> include/uapi/linux/rkisp1-config.h | 85 ++++++++-
> 2 files changed, 250 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index bac9d4972493..5865d53be9c8 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -57,6 +57,8 @@ union rkisp1_ext_params_config {
> struct rkisp1_ext_params_hst_config hst;
> struct rkisp1_ext_params_aec_config aec;
> struct rkisp1_ext_params_afc_config afc;
> + struct rkisp1_ext_params_compand_bls_config compand_bls;
> + struct rkisp1_ext_params_compand_curve_config compand_curve;
> };
>
> enum rkisp1_params_formats {
> @@ -1258,6 +1260,92 @@ rkisp1_dpf_strength_config(struct rkisp1_params *params,
> rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_DPF_STRENGTH_R, arg->r);
> }
>
> +static void rkisp1_compand_write_px_curve(struct rkisp1_params *params,
> + unsigned int addr, const u8 *px_curve)
> +{
> + size_t i, j;
> + u32 val;
> +
> + /*
> + * The compand curve is specified as a piecewise linear function with
> + * 64 points. X coordinates are stored as a log2 of the displacement
> + * from the previous point, in 5 bits, with 6 values per register. The
> + * last register stores 4 values.
> + */
> + for (i = 0; i < 10; i++) {
> + val = 0;
> + for (j = 0; j < 6; j++)
This loops up to (9 * 6 + 5 = 59) and writes registers up to PX9
This should probably be i < 11 or <= 10 as the companding PX curve has
64 points and 10 registers.
Also, to make sure, I would define the number of PX() registers
entries instead of using the crude '10' and '6' values
> + val |= ((px_curve[i * 6 + j] & 0x1f) << (j * 5));
Can't you just assign val without initializing it to 0 first and
or-ing it later ?
Also, once you make the external loop go up to 11, the last two
iterations will out-of-bound access px[64] and px[65] (px is declared
of size "RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64").
So this probably needs a check:
if (i == 10 && j > 3)
break;
before accessing px_curve[], or maybe declare the for loop as
for (j = 0; j < (i == 10 ? 4 : 6); j++)
> + rkisp1_write(params->rkisp1, addr + (i * 4), val);
Then the parameteric macros
RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(n)
RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(n)
are unused if not for the base address. Should you remove them and
only declare the base address value ?
> + }
> +
> + val = 0;
> + for (j = 0; j < 4; j++)
> + val |= ((px_curve[60 + j] & 0x1f) << (j * 5));
...
ok, I should maybe read the whole function before commenting. I left
the above comments there in case you want to unify the loop.
> + rkisp1_write(params->rkisp1, addr + (i * 4), val);
> +}
> +
> +static void
> +rkisp1_compand_write_curve_mem(struct rkisp1_params *params,
> + unsigned int reg_addr, unsigned int reg_data,
> + size_t num_samples, const u32 *curve)
isn't the number of samples fixed to
RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES ?
> +{
> + size_t i;
why a size and not an unsigned int ?
> +
> + for (i = 0; i < num_samples; i++) {
> + rkisp1_write(params->rkisp1, reg_addr, i);
> + rkisp1_write(params->rkisp1, reg_data, curve[i]);
> + }
> +}
> +
> +static void
> +rkisp1_compand_bls_config(struct rkisp1_params *params,
> + const struct rkisp1_cif_isp_compand_bls_config *arg)
> +{
> + static const u32 regs[] = {
> + RKISP1_CIF_ISP_COMPAND_BLS_A_FIXED,
> + RKISP1_CIF_ISP_COMPAND_BLS_B_FIXED,
> + RKISP1_CIF_ISP_COMPAND_BLS_C_FIXED,
> + RKISP1_CIF_ISP_COMPAND_BLS_D_FIXED,
> + };
> + u32 swapped[4];
> +
> + rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
> +
> + rkisp1_write(params->rkisp1, swapped[0], arg->r);
> + rkisp1_write(params->rkisp1, swapped[1], arg->gr);
> + rkisp1_write(params->rkisp1, swapped[2], arg->gb);
> + rkisp1_write(params->rkisp1, swapped[3], arg->b);
> +}
> +
> +static void
> +rkisp1_compand_expand_config(struct rkisp1_params *params,
> + const struct rkisp1_cif_isp_compand_curve_config *arg)
> +{
> + rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(0),
> + arg->px);
> + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_Y_ADDR,
> + RKISP1_CIF_ISP_COMPAND_EXPAND_Y_WRITE_DATA,
> + ARRAY_SIZE(arg->y), arg->y);
> + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_X_ADDR,
> + RKISP1_CIF_ISP_COMPAND_EXPAND_X_WRITE_DATA,
> + ARRAY_SIZE(arg->x), arg->x);
As the header reports
* @x: Compand curve x-values. The functionality of these parameters are
* unknown to do a lack of hardware documentation, but these are left here
is it safe to write them ?
> +}
> +
> +static void
> +rkisp1_compand_compress_config(struct rkisp1_params *params,
> + const struct rkisp1_cif_isp_compand_curve_config *arg)
> +{
> + rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(0),
> + arg->px);
> + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_ADDR,
> + RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_WRITE_DATA,
> + ARRAY_SIZE(arg->y), arg->y);
> + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_X_ADDR,
> + RKISP1_CIF_ISP_COMPAND_COMPRESS_X_WRITE_DATA,
> + ARRAY_SIZE(arg->x), arg->x);
> +}
> +
> static void
> rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> const struct rkisp1_params_cfg *new_params)
> @@ -1844,6 +1932,66 @@ rkisp1_ext_params_afcm(struct rkisp1_params *params,
> RKISP1_CIF_ISP_AFM_ENA);
> }
>
> +static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
> + const union rkisp1_ext_params_config *block)
nit: I presume going over 80-cols here is intentional
> +{
> + const struct rkisp1_ext_params_compand_bls_config *bls =
> + &block->compand_bls;
> +
> + if (bls->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> + RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> + return;
> + }
> +
> + rkisp1_compand_bls_config(params, &bls->config);
> +
> + if (!(params->enabled_blocks &
> + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS)))
> + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> + RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> +}
> +
> +static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
> + const union rkisp1_ext_params_config *block)
> +{
> + const struct rkisp1_ext_params_compand_curve_config *curve =
> + &block->compand_curve;
> +
> + if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> + RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> + return;
> + }
> +
> + rkisp1_compand_expand_config(params, &curve->config);
> +
> + if (!(params->enabled_blocks &
> + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND)))
> + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> + RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> +}
> +
> +static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
> + const union rkisp1_ext_params_config *block)
> +{
> + const struct rkisp1_ext_params_compand_curve_config *curve =
> + &block->compand_curve;
> +
> + if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> + RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> + return;
> + }
> +
> + rkisp1_compand_compress_config(params, &curve->config);
> +
> + if (!(params->enabled_blocks &
> + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS)))
> + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> + RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> +}
> +
> typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
> const union rkisp1_ext_params_config *config);
>
> @@ -1939,6 +2087,24 @@ static const struct rkisp1_ext_params_handler {
> .handler = rkisp1_ext_params_afcm,
> .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> },
> + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS] = {
> + .size = sizeof(struct rkisp1_ext_params_compand_bls_config),
> + .handler = rkisp1_ext_params_compand_bls,
> + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> + .features = RKISP1_FEATURE_COMPAND,
> + },
> + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND] = {
> + .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
> + .handler = rkisp1_ext_params_compand_expand,
> + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> + .features = RKISP1_FEATURE_COMPAND,
> + },
> + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS] = {
> + .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
> + .handler = rkisp1_ext_params_compand_compress,
> + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> + .features = RKISP1_FEATURE_COMPAND,
> + },
> };
>
> static void rkisp1_ext_params_config(struct rkisp1_params *params,
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 00b09c92cca7..dd962df53af5 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -164,6 +164,11 @@
> #define RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS 17
> #define RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS 6
>
> +/*
> + * Compand
> + */
> +#define RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64
> +
> /*
> * Measurement types
> */
> @@ -851,6 +856,39 @@ struct rkisp1_params_cfg {
> struct rkisp1_cif_isp_isp_other_cfg others;
> };
>
> +/**
> + * struct rkisp1_cif_isp_compand_bls_config - Rockchip ISP1 Companding parameters (BLS)
> + * @r: Fixed subtraction value for Bayer pattern R
> + * @gr: Fixed subtraction value for Bayer pattern Gr
> + * @gb: Fixed subtraction value for Bayer pattern Gb
> + * @b: Fixed subtraction value for Bayer pattern B
> + *
> + * The values will be subtracted from the sensor values. Note that unlike the
> + * dedicated BLS block, the BLS values in the compander are 20-bit unsigned.
I presume it's not worth mentioning this feature is only supported on
specific platforms, right ?
> + */
> +struct rkisp1_cif_isp_compand_bls_config {
> + __u32 r;
> + __u32 gr;
> + __u32 gb;
> + __u32 b;
> +};
> +
> +/**
> + * struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
> + * parameters (expand and compression curves)
Here and below: multi-line comments are aligned differently in the
rest of the file
* struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
* parameters (expand and compression curves)
> + * @px: Compand curve x-values. Each value stores the distance from the
> + * previous x-value, expressed as log2 of the distance on 5 bits.
> + * @x: Compand curve x-values. The functionality of these parameters are
> + * unknown to do a lack of hardware documentation, but these are left here
s/unknown to/unknown due to/
> + * for future compatibility purposes.
Also, the documentation of struct members in the existing code doesn't
use '.' at the end (not totally true, some do, so up to you)
> + * @y: Compand curve y-values
> + */
> +struct rkisp1_cif_isp_compand_curve_config {
> + __u8 px[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> + __u32 x[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> + __u32 y[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> +};
> +
> /*---------- PART2: Measurement Statistics ------------*/
>
> /**
> @@ -1018,6 +1056,9 @@ struct rkisp1_stat_buffer {
> * @RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram statistics
> * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto exposure statistics
> * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS: Auto-focus statistics
> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS: BLS in the compand block
> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND: Companding expand curve
> + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS: Compandding compress curve
s/Compandding/Companding/
> */
> enum rkisp1_ext_params_block_type {
> RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS,
> @@ -1037,6 +1078,9 @@ enum rkisp1_ext_params_block_type {
> RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS,
> RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS,
> RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS,
> + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS,
> + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND,
> + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS,
> };
>
> /**
> @@ -1384,6 +1428,42 @@ struct rkisp1_ext_params_afc_config {
> struct rkisp1_cif_isp_afc_config config;
> } __attribute__((aligned(8)));
>
> +/**
> + * struct rkisp1_ext_params_compand_bls_config - RkISP1 extensible params
> + * Compand BLS config
Here and in other places 'Compand' is spelled with capital 'C'. Is it
intentional ?
> + *
> + * RkISP1 extensible parameters Companding configuration block (black level
> + * subtraction). Identified by :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS`.
> + *
> + * @header: The RkISP1 extensible parameters header, see
> + * :c:type:`rkisp1_ext_params_block_header`
> + * @config: Companding BLS configuration, see
> + * :c:type:`rkisp1_cif_isp_compand_bls_config`
> + */
> +struct rkisp1_ext_params_compand_bls_config {
> + struct rkisp1_ext_params_block_header header;
> + struct rkisp1_cif_isp_compand_bls_config config;
> +} __attribute__((aligned(8)));
> +
> +/**
> + * struct rkisp1_ext_params_compand_curve_config - RkISP1 extensible params
> + * Compand curve config
> + *
> + * RkISP1 extensible parameters Companding configuration block (expand and
> + * compression curves). Identified by
> + * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND`or
> + * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS`.
> + *
> + * @header: The RkISP1 extensible parameters header, see
> + * :c:type:`rkisp1_ext_params_block_header`
> + * @config: Companding curve configuration, see
> + * :c:type:`rkisp1_cif_isp_compand_curve_config`
> + */
> +struct rkisp1_ext_params_compand_curve_config {
> + struct rkisp1_ext_params_block_header header;
> + struct rkisp1_cif_isp_compand_curve_config config;
> +} __attribute__((aligned(8)));
> +
> #define RKISP1_EXT_PARAMS_MAX_SIZE \
> (sizeof(struct rkisp1_ext_params_bls_config) +\
> sizeof(struct rkisp1_ext_params_dpcc_config) +\
> @@ -1401,7 +1481,10 @@ struct rkisp1_ext_params_afc_config {
> sizeof(struct rkisp1_ext_params_awb_meas_config) +\
> sizeof(struct rkisp1_ext_params_hst_config) +\
> sizeof(struct rkisp1_ext_params_aec_config) +\
> - sizeof(struct rkisp1_ext_params_afc_config))
> + sizeof(struct rkisp1_ext_params_afc_config) +\
> + sizeof(struct rkisp1_ext_params_compand_bls_config) +\
> + sizeof(struct rkisp1_ext_params_compand_curve_config) +\
> + sizeof(struct rkisp1_ext_params_compand_curve_config))
Do we need a comment to say why there are two entries of the same
type or not ?
>
> /**
> * enum rksip1_ext_param_buffer_version - RkISP1 extensible parameters version
> --
> Regards,
>
> Laurent Pinchart
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 5/5] media: rkisp1: Add support for the companding block
2024-07-04 10:40 ` Jacopo Mondi
@ 2024-07-04 12:53 ` Laurent Pinchart
2024-07-04 13:18 ` Jacopo Mondi
0 siblings, 1 reply; 15+ messages in thread
From: Laurent Pinchart @ 2024-07-04 12:53 UTC (permalink / raw)
To: Jacopo Mondi; +Cc: linux-media, Dafna Hirschfeld, Paul Elder, linux-rockchip
On Thu, Jul 04, 2024 at 12:40:42PM +0200, Jacopo Mondi wrote:
> Hi Laurent
> On Thu, Jul 04, 2024 at 01:25:33AM GMT, Laurent Pinchart wrote:
> > From: Paul Elder <paul.elder@ideasonboard.com>
> >
> > Add support to the rkisp1 driver for the companding block that exists on
> > the i.MX8MP version of the ISP. This requires usage of the new
> > extensible parameters format, and showcases how the format allows for
> > extensions without breaking backward compatibility.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v0:
> >
> > - Drop RKISP1_EXT_PARAM_BUFFER_V2
> > - Use common structure for compression and expansion curves
> > - Rename config fields in rkisp1_ext_params_*_config to just config
> > - Mention block type in structures documentation
> > - Constify arguments
> > - Replace __uxx types with uxx
> > - Use rkisp1_bls_swap_regs() helper in rkisp1_compand_bls_config()
> > - Use generic feature handling mechanism
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-params.c | 166 ++++++++++++++++++
> > include/uapi/linux/rkisp1-config.h | 85 ++++++++-
> > 2 files changed, 250 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index bac9d4972493..5865d53be9c8 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -57,6 +57,8 @@ union rkisp1_ext_params_config {
> > struct rkisp1_ext_params_hst_config hst;
> > struct rkisp1_ext_params_aec_config aec;
> > struct rkisp1_ext_params_afc_config afc;
> > + struct rkisp1_ext_params_compand_bls_config compand_bls;
> > + struct rkisp1_ext_params_compand_curve_config compand_curve;
> > };
> >
> > enum rkisp1_params_formats {
> > @@ -1258,6 +1260,92 @@ rkisp1_dpf_strength_config(struct rkisp1_params *params,
> > rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_DPF_STRENGTH_R, arg->r);
> > }
> >
> > +static void rkisp1_compand_write_px_curve(struct rkisp1_params *params,
> > + unsigned int addr, const u8 *px_curve)
> > +{
> > + size_t i, j;
> > + u32 val;
> > +
> > + /*
> > + * The compand curve is specified as a piecewise linear function with
> > + * 64 points. X coordinates are stored as a log2 of the displacement
> > + * from the previous point, in 5 bits, with 6 values per register. The
> > + * last register stores 4 values.
> > + */
> > + for (i = 0; i < 10; i++) {
> > + val = 0;
> > + for (j = 0; j < 6; j++)
>
> This loops up to (9 * 6 + 5 = 59) and writes registers up to PX9
> This should probably be i < 11 or <= 10 as the companding PX curve has
> 64 points and 10 registers.
>
> Also, to make sure, I would define the number of PX() registers
> entries instead of using the crude '10' and '6' values
>
> > + val |= ((px_curve[i * 6 + j] & 0x1f) << (j * 5));
>
> Can't you just assign val without initializing it to 0 first and
> or-ing it later ?
I'm not sure to see what you mean here.
> Also, once you make the external loop go up to 11, the last two
> iterations will out-of-bound access px[64] and px[65] (px is declared
> of size "RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64").
>
> So this probably needs a check:
>
> if (i == 10 && j > 3)
> break;
>
> before accessing px_curve[], or maybe declare the for loop as
>
> for (j = 0; j < (i == 10 ? 4 : 6); j++)
>
> > + rkisp1_write(params->rkisp1, addr + (i * 4), val);
>
> Then the parameteric macros
> RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(n)
> RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(n)
>
> are unused if not for the base address. Should you remove them and
> only declare the base address value ?
I think it's useful to document what's available in terms of register
macros, even if not everything is used.
> > + }
> > +
> > + val = 0;
> > + for (j = 0; j < 4; j++)
> > + val |= ((px_curve[60 + j] & 0x1f) << (j * 5));
>
> ...
>
> ok, I should maybe read the whole function before commenting. I left
> the above comments there in case you want to unify the loop.
I'll try to rework the code.
> > + rkisp1_write(params->rkisp1, addr + (i * 4), val);
> > +}
> > +
> > +static void
> > +rkisp1_compand_write_curve_mem(struct rkisp1_params *params,
> > + unsigned int reg_addr, unsigned int reg_data,
> > + size_t num_samples, const u32 *curve)
>
> isn't the number of samples fixed to
> RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES ?
A previous version of the patch had two macros for the expand and
compress curves. Now that it's unified, I can drop the argument.
> > +{
> > + size_t i;
>
> why a size and not an unsigned int ?
The patch is originally from Paul, I don't know. I'll switch to unsigned
int as size_t is 64-bit on 64-bit platforms, which is overkill.
> > +
> > + for (i = 0; i < num_samples; i++) {
> > + rkisp1_write(params->rkisp1, reg_addr, i);
> > + rkisp1_write(params->rkisp1, reg_data, curve[i]);
> > + }
> > +}
> > +
> > +static void
> > +rkisp1_compand_bls_config(struct rkisp1_params *params,
> > + const struct rkisp1_cif_isp_compand_bls_config *arg)
> > +{
> > + static const u32 regs[] = {
> > + RKISP1_CIF_ISP_COMPAND_BLS_A_FIXED,
> > + RKISP1_CIF_ISP_COMPAND_BLS_B_FIXED,
> > + RKISP1_CIF_ISP_COMPAND_BLS_C_FIXED,
> > + RKISP1_CIF_ISP_COMPAND_BLS_D_FIXED,
> > + };
> > + u32 swapped[4];
> > +
> > + rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
> > +
> > + rkisp1_write(params->rkisp1, swapped[0], arg->r);
> > + rkisp1_write(params->rkisp1, swapped[1], arg->gr);
> > + rkisp1_write(params->rkisp1, swapped[2], arg->gb);
> > + rkisp1_write(params->rkisp1, swapped[3], arg->b);
> > +}
> > +
> > +static void
> > +rkisp1_compand_expand_config(struct rkisp1_params *params,
> > + const struct rkisp1_cif_isp_compand_curve_config *arg)
> > +{
> > + rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(0),
> > + arg->px);
> > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_Y_ADDR,
> > + RKISP1_CIF_ISP_COMPAND_EXPAND_Y_WRITE_DATA,
> > + ARRAY_SIZE(arg->y), arg->y);
> > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_X_ADDR,
> > + RKISP1_CIF_ISP_COMPAND_EXPAND_X_WRITE_DATA,
> > + ARRAY_SIZE(arg->x), arg->x);
>
> As the header reports
>
> * @x: Compand curve x-values. The functionality of these parameters are
> * unknown to do a lack of hardware documentation, but these are left here
>
> is it safe to write them ?
Yes. They don't seem to have an effect, but they're written.
> > +}
> > +
> > +static void
> > +rkisp1_compand_compress_config(struct rkisp1_params *params,
> > + const struct rkisp1_cif_isp_compand_curve_config *arg)
> > +{
> > + rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(0),
> > + arg->px);
> > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_ADDR,
> > + RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_WRITE_DATA,
> > + ARRAY_SIZE(arg->y), arg->y);
> > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_X_ADDR,
> > + RKISP1_CIF_ISP_COMPAND_COMPRESS_X_WRITE_DATA,
> > + ARRAY_SIZE(arg->x), arg->x);
> > +}
> > +
> > static void
> > rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> > const struct rkisp1_params_cfg *new_params)
> > @@ -1844,6 +1932,66 @@ rkisp1_ext_params_afcm(struct rkisp1_params *params,
> > RKISP1_CIF_ISP_AFM_ENA);
> > }
> >
> > +static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
> + const union rkisp1_ext_params_config *block)
>
> nit: I presume going over 80-cols here is intentional
I think so :-)
> > +{
> > + const struct rkisp1_ext_params_compand_bls_config *bls =
> > + &block->compand_bls;
> > +
> > + if (bls->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > + RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> > + return;
> > + }
> > +
> > + rkisp1_compand_bls_config(params, &bls->config);
> > +
> > + if (!(params->enabled_blocks &
> > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS)))
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > + RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> > +}
> > +
> > +static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
> > + const union rkisp1_ext_params_config *block)
> > +{
> > + const struct rkisp1_ext_params_compand_curve_config *curve =
> > + &block->compand_curve;
> > +
> > + if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > + RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> > + return;
> > + }
> > +
> > + rkisp1_compand_expand_config(params, &curve->config);
> > +
> > + if (!(params->enabled_blocks &
> > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND)))
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > + RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> > +}
> > +
> > +static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
> > + const union rkisp1_ext_params_config *block)
> > +{
> > + const struct rkisp1_ext_params_compand_curve_config *curve =
> > + &block->compand_curve;
> > +
> > + if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > + RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> > + return;
> > + }
> > +
> > + rkisp1_compand_compress_config(params, &curve->config);
> > +
> > + if (!(params->enabled_blocks &
> > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS)))
> > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > + RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> > +}
> > +
> > typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
> > const union rkisp1_ext_params_config *config);
> >
> > @@ -1939,6 +2087,24 @@ static const struct rkisp1_ext_params_handler {
> > .handler = rkisp1_ext_params_afcm,
> > .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS] = {
> > + .size = sizeof(struct rkisp1_ext_params_compand_bls_config),
> > + .handler = rkisp1_ext_params_compand_bls,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > + .features = RKISP1_FEATURE_COMPAND,
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND] = {
> > + .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
> > + .handler = rkisp1_ext_params_compand_expand,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > + .features = RKISP1_FEATURE_COMPAND,
> > + },
> > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS] = {
> > + .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
> > + .handler = rkisp1_ext_params_compand_compress,
> > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > + .features = RKISP1_FEATURE_COMPAND,
> > + },
> > };
> >
> > static void rkisp1_ext_params_config(struct rkisp1_params *params,
> > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > index 00b09c92cca7..dd962df53af5 100644
> > --- a/include/uapi/linux/rkisp1-config.h
> > +++ b/include/uapi/linux/rkisp1-config.h
> > @@ -164,6 +164,11 @@
> > #define RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS 17
> > #define RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS 6
> >
> > +/*
> > + * Compand
> > + */
> > +#define RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64
> > +
> > /*
> > * Measurement types
> > */
> > @@ -851,6 +856,39 @@ struct rkisp1_params_cfg {
> > struct rkisp1_cif_isp_isp_other_cfg others;
> > };
> >
> > +/**
> > + * struct rkisp1_cif_isp_compand_bls_config - Rockchip ISP1 Companding parameters (BLS)
> > + * @r: Fixed subtraction value for Bayer pattern R
> > + * @gr: Fixed subtraction value for Bayer pattern Gr
> > + * @gb: Fixed subtraction value for Bayer pattern Gb
> > + * @b: Fixed subtraction value for Bayer pattern B
> > + *
> > + * The values will be subtracted from the sensor values. Note that unlike the
> > + * dedicated BLS block, the BLS values in the compander are 20-bit unsigned.
>
> I presume it's not worth mentioning this feature is only supported on
> specific platforms, right ?
We could, but we don't do so for the BLS block. I don't mind either way.
I think it's fairly clear from the driver code, and I would expect
people who want to use this driver to have to read the driver code
anyway.
> > + */
> > +struct rkisp1_cif_isp_compand_bls_config {
> > + __u32 r;
> > + __u32 gr;
> > + __u32 gb;
> > + __u32 b;
> > +};
> > +
> > +/**
> > + * struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
> > + * parameters (expand and compression curves)
>
> Here and below: multi-line comments are aligned differently in the
> rest of the file
We have a mix of all kinds of alignment styles :-( If someone wants to
clean things up, I'll ack a patch.
> * struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
> * parameters (expand and compression curves)
It would need to be
* struct rkisp0_cif_isp_compand_curve_config - Rockchip ISP1 Companding
* parameters (expand and
* compression curves)
which starts looking ridiculous :-)
> > + * @px: Compand curve x-values. Each value stores the distance from the
> > + * previous x-value, expressed as log2 of the distance on 5 bits.
> > + * @x: Compand curve x-values. The functionality of these parameters are
> > + * unknown to do a lack of hardware documentation, but these are left here
>
> s/unknown to/unknown due to/
>
> > + * for future compatibility purposes.
>
> Also, the documentation of struct members in the existing code doesn't
> use '.' at the end (not totally true, some do, so up to you)
I've added one because it's a multi-sentence comment.
> > + * @y: Compand curve y-values
> > + */
> > +struct rkisp1_cif_isp_compand_curve_config {
> > + __u8 px[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> > + __u32 x[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> > + __u32 y[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> > +};
> > +
> > /*---------- PART2: Measurement Statistics ------------*/
> >
> > /**
> > @@ -1018,6 +1056,9 @@ struct rkisp1_stat_buffer {
> > * @RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram statistics
> > * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto exposure statistics
> > * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS: Auto-focus statistics
> > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS: BLS in the compand block
> > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND: Companding expand curve
> > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS: Compandding compress curve
>
> s/Compandding/Companding/
>
> > */
> > enum rkisp1_ext_params_block_type {
> > RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS,
> > @@ -1037,6 +1078,9 @@ enum rkisp1_ext_params_block_type {
> > RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS,
> > RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS,
> > RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS,
> > + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS,
> > + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND,
> > + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS,
> > };
> >
> > /**
> > @@ -1384,6 +1428,42 @@ struct rkisp1_ext_params_afc_config {
> > struct rkisp1_cif_isp_afc_config config;
> > } __attribute__((aligned(8)));
> >
> > +/**
> > + * struct rkisp1_ext_params_compand_bls_config - RkISP1 extensible params
> > + * Compand BLS config
>
> Here and in other places 'Compand' is spelled with capital 'C'. Is it
> intentional ?
I think Paul matched the documentation of other blocks, that write e.g.
'Histogram'.
> > + *
> > + * RkISP1 extensible parameters Companding configuration block (black level
> > + * subtraction). Identified by :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS`.
> > + *
> > + * @header: The RkISP1 extensible parameters header, see
> > + * :c:type:`rkisp1_ext_params_block_header`
> > + * @config: Companding BLS configuration, see
> > + * :c:type:`rkisp1_cif_isp_compand_bls_config`
> > + */
> > +struct rkisp1_ext_params_compand_bls_config {
> > + struct rkisp1_ext_params_block_header header;
> > + struct rkisp1_cif_isp_compand_bls_config config;
> > +} __attribute__((aligned(8)));
> > +
> > +/**
> > + * struct rkisp1_ext_params_compand_curve_config - RkISP1 extensible params
> > + * Compand curve config
> > + *
> > + * RkISP1 extensible parameters Companding configuration block (expand and
> > + * compression curves). Identified by
> > + * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND`or
> > + * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS`.
> > + *
> > + * @header: The RkISP1 extensible parameters header, see
> > + * :c:type:`rkisp1_ext_params_block_header`
> > + * @config: Companding curve configuration, see
> > + * :c:type:`rkisp1_cif_isp_compand_curve_config`
> > + */
> > +struct rkisp1_ext_params_compand_curve_config {
> > + struct rkisp1_ext_params_block_header header;
> > + struct rkisp1_cif_isp_compand_curve_config config;
> > +} __attribute__((aligned(8)));
> > +
> > #define RKISP1_EXT_PARAMS_MAX_SIZE \
> > (sizeof(struct rkisp1_ext_params_bls_config) +\
> > sizeof(struct rkisp1_ext_params_dpcc_config) +\
> > @@ -1401,7 +1481,10 @@ struct rkisp1_ext_params_afc_config {
> > sizeof(struct rkisp1_ext_params_awb_meas_config) +\
> > sizeof(struct rkisp1_ext_params_hst_config) +\
> > sizeof(struct rkisp1_ext_params_aec_config) +\
> > - sizeof(struct rkisp1_ext_params_afc_config))
> > + sizeof(struct rkisp1_ext_params_afc_config) +\
> > + sizeof(struct rkisp1_ext_params_compand_bls_config) +\
> > + sizeof(struct rkisp1_ext_params_compand_curve_config) +\
> > + sizeof(struct rkisp1_ext_params_compand_curve_config))
>
> Do we need a comment to say why there are two entries of the same
> type or not ?
I'll add one.
> >
> > /**
> > * enum rksip1_ext_param_buffer_version - RkISP1 extensible parameters version
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v1 5/5] media: rkisp1: Add support for the companding block
2024-07-04 12:53 ` Laurent Pinchart
@ 2024-07-04 13:18 ` Jacopo Mondi
0 siblings, 0 replies; 15+ messages in thread
From: Jacopo Mondi @ 2024-07-04 13:18 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, linux-media, Dafna Hirschfeld, Paul Elder,
linux-rockchip
Hi Laurent
On Thu, Jul 04, 2024 at 03:53:29PM GMT, Laurent Pinchart wrote:
> On Thu, Jul 04, 2024 at 12:40:42PM +0200, Jacopo Mondi wrote:
> > Hi Laurent
> > On Thu, Jul 04, 2024 at 01:25:33AM GMT, Laurent Pinchart wrote:
> > > From: Paul Elder <paul.elder@ideasonboard.com>
> > >
> > > Add support to the rkisp1 driver for the companding block that exists on
> > > the i.MX8MP version of the ISP. This requires usage of the new
> > > extensible parameters format, and showcases how the format allows for
> > > extensions without breaking backward compatibility.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v0:
> > >
> > > - Drop RKISP1_EXT_PARAM_BUFFER_V2
> > > - Use common structure for compression and expansion curves
> > > - Rename config fields in rkisp1_ext_params_*_config to just config
> > > - Mention block type in structures documentation
> > > - Constify arguments
> > > - Replace __uxx types with uxx
> > > - Use rkisp1_bls_swap_regs() helper in rkisp1_compand_bls_config()
> > > - Use generic feature handling mechanism
> > > ---
> > > .../platform/rockchip/rkisp1/rkisp1-params.c | 166 ++++++++++++++++++
> > > include/uapi/linux/rkisp1-config.h | 85 ++++++++-
> > > 2 files changed, 250 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > index bac9d4972493..5865d53be9c8 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > @@ -57,6 +57,8 @@ union rkisp1_ext_params_config {
> > > struct rkisp1_ext_params_hst_config hst;
> > > struct rkisp1_ext_params_aec_config aec;
> > > struct rkisp1_ext_params_afc_config afc;
> > > + struct rkisp1_ext_params_compand_bls_config compand_bls;
> > > + struct rkisp1_ext_params_compand_curve_config compand_curve;
> > > };
> > >
> > > enum rkisp1_params_formats {
> > > @@ -1258,6 +1260,92 @@ rkisp1_dpf_strength_config(struct rkisp1_params *params,
> > > rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_DPF_STRENGTH_R, arg->r);
> > > }
> > >
> > > +static void rkisp1_compand_write_px_curve(struct rkisp1_params *params,
> > > + unsigned int addr, const u8 *px_curve)
> > > +{
> > > + size_t i, j;
> > > + u32 val;
> > > +
> > > + /*
> > > + * The compand curve is specified as a piecewise linear function with
> > > + * 64 points. X coordinates are stored as a log2 of the displacement
> > > + * from the previous point, in 5 bits, with 6 values per register. The
> > > + * last register stores 4 values.
> > > + */
> > > + for (i = 0; i < 10; i++) {
> > > + val = 0;
> > > + for (j = 0; j < 6; j++)
> >
> > This loops up to (9 * 6 + 5 = 59) and writes registers up to PX9
> > This should probably be i < 11 or <= 10 as the companding PX curve has
> > 64 points and 10 registers.
> >
> > Also, to make sure, I would define the number of PX() registers
> > entries instead of using the crude '10' and '6' values
> >
> > > + val |= ((px_curve[i * 6 + j] & 0x1f) << (j * 5));
> >
> > Can't you just assign val without initializing it to 0 first and
> > or-ing it later ?
>
> I'm not sure to see what you mean here.
Nothing, I mis-read the code :)
>
> > Also, once you make the external loop go up to 11, the last two
> > iterations will out-of-bound access px[64] and px[65] (px is declared
> > of size "RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64").
> >
> > So this probably needs a check:
> >
> > if (i == 10 && j > 3)
> > break;
> >
> > before accessing px_curve[], or maybe declare the for loop as
> >
> > for (j = 0; j < (i == 10 ? 4 : 6); j++)
> >
> > > + rkisp1_write(params->rkisp1, addr + (i * 4), val);
> >
> > Then the parameteric macros
> > RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(n)
> > RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(n)
> >
> > are unused if not for the base address. Should you remove them and
> > only declare the base address value ?
>
> I think it's useful to document what's available in terms of register
> macros, even if not everything is used.
>
> > > + }
> > > +
> > > + val = 0;
> > > + for (j = 0; j < 4; j++)
> > > + val |= ((px_curve[60 + j] & 0x1f) << (j * 5));
> >
> > ...
> >
> > ok, I should maybe read the whole function before commenting. I left
> > the above comments there in case you want to unify the loop.
>
> I'll try to rework the code.
>
> > > + rkisp1_write(params->rkisp1, addr + (i * 4), val);
> > > +}
> > > +
> > > +static void
> > > +rkisp1_compand_write_curve_mem(struct rkisp1_params *params,
> > > + unsigned int reg_addr, unsigned int reg_data,
> > > + size_t num_samples, const u32 *curve)
> >
> > isn't the number of samples fixed to
> > RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES ?
>
> A previous version of the patch had two macros for the expand and
> compress curves. Now that it's unified, I can drop the argument.
>
> > > +{
> > > + size_t i;
> >
> > why a size and not an unsigned int ?
>
> The patch is originally from Paul, I don't know. I'll switch to unsigned
> int as size_t is 64-bit on 64-bit platforms, which is overkill.
>
> > > +
> > > + for (i = 0; i < num_samples; i++) {
> > > + rkisp1_write(params->rkisp1, reg_addr, i);
> > > + rkisp1_write(params->rkisp1, reg_data, curve[i]);
> > > + }
> > > +}
> > > +
> > > +static void
> > > +rkisp1_compand_bls_config(struct rkisp1_params *params,
> > > + const struct rkisp1_cif_isp_compand_bls_config *arg)
> > > +{
> > > + static const u32 regs[] = {
> > > + RKISP1_CIF_ISP_COMPAND_BLS_A_FIXED,
> > > + RKISP1_CIF_ISP_COMPAND_BLS_B_FIXED,
> > > + RKISP1_CIF_ISP_COMPAND_BLS_C_FIXED,
> > > + RKISP1_CIF_ISP_COMPAND_BLS_D_FIXED,
> > > + };
> > > + u32 swapped[4];
> > > +
> > > + rkisp1_bls_swap_regs(params->raw_type, regs, swapped);
> > > +
> > > + rkisp1_write(params->rkisp1, swapped[0], arg->r);
> > > + rkisp1_write(params->rkisp1, swapped[1], arg->gr);
> > > + rkisp1_write(params->rkisp1, swapped[2], arg->gb);
> > > + rkisp1_write(params->rkisp1, swapped[3], arg->b);
> > > +}
> > > +
> > > +static void
> > > +rkisp1_compand_expand_config(struct rkisp1_params *params,
> > > + const struct rkisp1_cif_isp_compand_curve_config *arg)
> > > +{
> > > + rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_EXPAND_PX_N(0),
> > > + arg->px);
> > > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_Y_ADDR,
> > > + RKISP1_CIF_ISP_COMPAND_EXPAND_Y_WRITE_DATA,
> > > + ARRAY_SIZE(arg->y), arg->y);
> > > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_EXPAND_X_ADDR,
> > > + RKISP1_CIF_ISP_COMPAND_EXPAND_X_WRITE_DATA,
> > > + ARRAY_SIZE(arg->x), arg->x);
> >
> > As the header reports
> >
> > * @x: Compand curve x-values. The functionality of these parameters are
> > * unknown to do a lack of hardware documentation, but these are left here
> >
> > is it safe to write them ?
>
> Yes. They don't seem to have an effect, but they're written.
>
> > > +}
> > > +
> > > +static void
> > > +rkisp1_compand_compress_config(struct rkisp1_params *params,
> > > + const struct rkisp1_cif_isp_compand_curve_config *arg)
> > > +{
> > > + rkisp1_compand_write_px_curve(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_PX_N(0),
> > > + arg->px);
> > > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_ADDR,
> > > + RKISP1_CIF_ISP_COMPAND_COMPRESS_Y_WRITE_DATA,
> > > + ARRAY_SIZE(arg->y), arg->y);
> > > + rkisp1_compand_write_curve_mem(params, RKISP1_CIF_ISP_COMPAND_COMPRESS_X_ADDR,
> > > + RKISP1_CIF_ISP_COMPAND_COMPRESS_X_WRITE_DATA,
> > > + ARRAY_SIZE(arg->x), arg->x);
> > > +}
> > > +
> > > static void
> > > rkisp1_isp_isr_other_config(struct rkisp1_params *params,
> > > const struct rkisp1_params_cfg *new_params)
> > > @@ -1844,6 +1932,66 @@ rkisp1_ext_params_afcm(struct rkisp1_params *params,
> > > RKISP1_CIF_ISP_AFM_ENA);
> > > }
> > >
> > > +static void rkisp1_ext_params_compand_bls(struct rkisp1_params *params,
> > + const union rkisp1_ext_params_config *block)
> >
> > nit: I presume going over 80-cols here is intentional
>
> I think so :-)
>
> > > +{
> > > + const struct rkisp1_ext_params_compand_bls_config *bls =
> > > + &block->compand_bls;
> > > +
> > > + if (bls->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > > + RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> > > + return;
> > > + }
> > > +
> > > + rkisp1_compand_bls_config(params, &bls->config);
> > > +
> > > + if (!(params->enabled_blocks &
> > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS)))
> > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > > + RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE);
> > > +}
> > > +
> > > +static void rkisp1_ext_params_compand_expand(struct rkisp1_params *params,
> > > + const union rkisp1_ext_params_config *block)
> > > +{
> > > + const struct rkisp1_ext_params_compand_curve_config *curve =
> > > + &block->compand_curve;
> > > +
> > > + if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > > + RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> > > + return;
> > > + }
> > > +
> > > + rkisp1_compand_expand_config(params, &curve->config);
> > > +
> > > + if (!(params->enabled_blocks &
> > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND)))
> > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > > + RKISP1_CIF_ISP_COMPAND_CTRL_EXPAND_ENABLE);
> > > +}
> > > +
> > > +static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
> > > + const union rkisp1_ext_params_config *block)
> > > +{
> > > + const struct rkisp1_ext_params_compand_curve_config *curve =
> > > + &block->compand_curve;
> > > +
> > > + if (curve->header.enable == RKISP1_EXT_PARAMS_BLOCK_DISABLE) {
> > > + rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > > + RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> > > + return;
> > > + }
> > > +
> > > + rkisp1_compand_compress_config(params, &curve->config);
> > > +
> > > + if (!(params->enabled_blocks &
> > > + BIT(RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS)))
> > > + rkisp1_param_set_bits(params, RKISP1_CIF_ISP_COMPAND_CTRL,
> > > + RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
> > > +}
> > > +
> > > typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
> > > const union rkisp1_ext_params_config *config);
> > >
> > > @@ -1939,6 +2087,24 @@ static const struct rkisp1_ext_params_handler {
> > > .handler = rkisp1_ext_params_afcm,
> > > .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > > },
> > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS] = {
> > > + .size = sizeof(struct rkisp1_ext_params_compand_bls_config),
> > > + .handler = rkisp1_ext_params_compand_bls,
> > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > > + .features = RKISP1_FEATURE_COMPAND,
> > > + },
> > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND] = {
> > > + .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
> > > + .handler = rkisp1_ext_params_compand_expand,
> > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > > + .features = RKISP1_FEATURE_COMPAND,
> > > + },
> > > + [RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS] = {
> > > + .size = sizeof(struct rkisp1_ext_params_compand_curve_config),
> > > + .handler = rkisp1_ext_params_compand_compress,
> > > + .group = RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
> > > + .features = RKISP1_FEATURE_COMPAND,
> > > + },
> > > };
> > >
> > > static void rkisp1_ext_params_config(struct rkisp1_params *params,
> > > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > index 00b09c92cca7..dd962df53af5 100644
> > > --- a/include/uapi/linux/rkisp1-config.h
> > > +++ b/include/uapi/linux/rkisp1-config.h
> > > @@ -164,6 +164,11 @@
> > > #define RKISP1_CIF_ISP_DPF_MAX_NLF_COEFFS 17
> > > #define RKISP1_CIF_ISP_DPF_MAX_SPATIAL_COEFFS 6
> > >
> > > +/*
> > > + * Compand
> > > + */
> > > +#define RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES 64
> > > +
> > > /*
> > > * Measurement types
> > > */
> > > @@ -851,6 +856,39 @@ struct rkisp1_params_cfg {
> > > struct rkisp1_cif_isp_isp_other_cfg others;
> > > };
> > >
> > > +/**
> > > + * struct rkisp1_cif_isp_compand_bls_config - Rockchip ISP1 Companding parameters (BLS)
> > > + * @r: Fixed subtraction value for Bayer pattern R
> > > + * @gr: Fixed subtraction value for Bayer pattern Gr
> > > + * @gb: Fixed subtraction value for Bayer pattern Gb
> > > + * @b: Fixed subtraction value for Bayer pattern B
> > > + *
> > > + * The values will be subtracted from the sensor values. Note that unlike the
> > > + * dedicated BLS block, the BLS values in the compander are 20-bit unsigned.
> >
> > I presume it's not worth mentioning this feature is only supported on
> > specific platforms, right ?
>
> We could, but we don't do so for the BLS block. I don't mind either way.
> I think it's fairly clear from the driver code, and I would expect
> people who want to use this driver to have to read the driver code
> anyway.
>
Yeah, and we should also mention what platforms all other blocks apply
to. Don't bother.
> > > + */
> > > +struct rkisp1_cif_isp_compand_bls_config {
> > > + __u32 r;
> > > + __u32 gr;
> > > + __u32 gb;
> > > + __u32 b;
> > > +};
> > > +
> > > +/**
> > > + * struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
> > > + * parameters (expand and compression curves)
> >
> > Here and below: multi-line comments are aligned differently in the
> > rest of the file
>
> We have a mix of all kinds of alignment styles :-( If someone wants to
> clean things up, I'll ack a patch.
>
> > * struct rkisp1_cif_isp_compand_curve_config - Rockchip ISP1 Companding
> > * parameters (expand and compression curves)
>
> It would need to be
>
> * struct rkisp0_cif_isp_compand_curve_config - Rockchip ISP1 Companding
> * parameters (expand and
> * compression curves)
>
> which starts looking ridiculous :-)
>
I can't disagree
> > > + * @px: Compand curve x-values. Each value stores the distance from the
> > > + * previous x-value, expressed as log2 of the distance on 5 bits.
> > > + * @x: Compand curve x-values. The functionality of these parameters are
> > > + * unknown to do a lack of hardware documentation, but these are left here
> >
> > s/unknown to/unknown due to/
> >
> > > + * for future compatibility purposes.
> >
> > Also, the documentation of struct members in the existing code doesn't
> > use '.' at the end (not totally true, some do, so up to you)
>
> I've added one because it's a multi-sentence comment.
>
> > > + * @y: Compand curve y-values
> > > + */
> > > +struct rkisp1_cif_isp_compand_curve_config {
> > > + __u8 px[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> > > + __u32 x[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> > > + __u32 y[RKISP1_CIF_ISP_COMPAND_MAX_SAMPLES];
> > > +};
> > > +
> > > /*---------- PART2: Measurement Statistics ------------*/
> > >
> > > /**
> > > @@ -1018,6 +1056,9 @@ struct rkisp1_stat_buffer {
> > > * @RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS: Histogram statistics
> > > * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS: Auto exposure statistics
> > > * @RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS: Auto-focus statistics
> > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS: BLS in the compand block
> > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND: Companding expand curve
> > > + * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS: Compandding compress curve
> >
> > s/Compandding/Companding/
> >
> > > */
> > > enum rkisp1_ext_params_block_type {
> > > RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS,
> > > @@ -1037,6 +1078,9 @@ enum rkisp1_ext_params_block_type {
> > > RKISP1_EXT_PARAMS_BLOCK_TYPE_HST_MEAS,
> > > RKISP1_EXT_PARAMS_BLOCK_TYPE_AEC_MEAS,
> > > RKISP1_EXT_PARAMS_BLOCK_TYPE_AFC_MEAS,
> > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS,
> > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND,
> > > + RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS,
> > > };
> > >
> > > /**
> > > @@ -1384,6 +1428,42 @@ struct rkisp1_ext_params_afc_config {
> > > struct rkisp1_cif_isp_afc_config config;
> > > } __attribute__((aligned(8)));
> > >
> > > +/**
> > > + * struct rkisp1_ext_params_compand_bls_config - RkISP1 extensible params
> > > + * Compand BLS config
> >
> > Here and in other places 'Compand' is spelled with capital 'C'. Is it
> > intentional ?
>
> I think Paul matched the documentation of other blocks, that write e.g.
> 'Histogram'.
>
> > > + *
> > > + * RkISP1 extensible parameters Companding configuration block (black level
> > > + * subtraction). Identified by :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS`.
> > > + *
> > > + * @header: The RkISP1 extensible parameters header, see
> > > + * :c:type:`rkisp1_ext_params_block_header`
> > > + * @config: Companding BLS configuration, see
> > > + * :c:type:`rkisp1_cif_isp_compand_bls_config`
> > > + */
> > > +struct rkisp1_ext_params_compand_bls_config {
> > > + struct rkisp1_ext_params_block_header header;
> > > + struct rkisp1_cif_isp_compand_bls_config config;
> > > +} __attribute__((aligned(8)));
> > > +
> > > +/**
> > > + * struct rkisp1_ext_params_compand_curve_config - RkISP1 extensible params
> > > + * Compand curve config
> > > + *
> > > + * RkISP1 extensible parameters Companding configuration block (expand and
> > > + * compression curves). Identified by
> > > + * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND`or
> > > + * :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS`.
> > > + *
> > > + * @header: The RkISP1 extensible parameters header, see
> > > + * :c:type:`rkisp1_ext_params_block_header`
> > > + * @config: Companding curve configuration, see
> > > + * :c:type:`rkisp1_cif_isp_compand_curve_config`
> > > + */
> > > +struct rkisp1_ext_params_compand_curve_config {
> > > + struct rkisp1_ext_params_block_header header;
> > > + struct rkisp1_cif_isp_compand_curve_config config;
> > > +} __attribute__((aligned(8)));
> > > +
> > > #define RKISP1_EXT_PARAMS_MAX_SIZE \
> > > (sizeof(struct rkisp1_ext_params_bls_config) +\
> > > sizeof(struct rkisp1_ext_params_dpcc_config) +\
> > > @@ -1401,7 +1481,10 @@ struct rkisp1_ext_params_afc_config {
> > > sizeof(struct rkisp1_ext_params_awb_meas_config) +\
> > > sizeof(struct rkisp1_ext_params_hst_config) +\
> > > sizeof(struct rkisp1_ext_params_aec_config) +\
> > > - sizeof(struct rkisp1_ext_params_afc_config))
> > > + sizeof(struct rkisp1_ext_params_afc_config) +\
> > > + sizeof(struct rkisp1_ext_params_compand_bls_config) +\
> > > + sizeof(struct rkisp1_ext_params_compand_curve_config) +\
> > > + sizeof(struct rkisp1_ext_params_compand_curve_config))
> >
> > Do we need a comment to say why there are two entries of the same
> > type or not ?
>
> I'll add one.
>
Thanks
j
> > >
> > > /**
> > > * enum rksip1_ext_param_buffer_version - RkISP1 extensible parameters version
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 15+ messages in thread