| Message ID | 20220614191127.3420492-56-paul.elder@ideasonboard.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rkisp1: Cleanups and add support for i.MX8MP | expand |
Hello Paul, thanks for the patch. Am Dienstag, 14. Juni 2022, 21:11:27 CEST schrieb Paul Elder: > The ISP that is integrated in the i.MX8MP uses different bits in the > MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration > compared to the on in the RK3399. In addition, it adds a new bit for > enabling crop. Add new definitions for these bits for i.MX8MP devices, > and update where they are set. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ > .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index > 34f4fe09c88d..24ad2ccec2a3 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > @@ -168,6 +168,10 @@ > #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) > #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) > > +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) > + Does it make sense to move this kind of information into struct rkisp1_info? This way you can skip the if (isp_ver == ...) thing. Best regards, Alexander > /* RSZ_CROP_[XY]_DIR */ > #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << > 0) > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index > 08bf3aa8088f..29a31b18a082 100644 > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct > rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, > RKISP1_CIF_RSZ_CTRL); > > if (when == RKISP1_SHADOW_REGS_ASYNC) > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; > + else > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > else > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; > + else > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); > }
Hi Alexander, On Thu, Jun 16, 2022 at 10:05:06AM +0200, Alexander Stein wrote: > Am Dienstag, 14. Juni 2022, 21:11:27 CEST schrieb Paul Elder: > > The ISP that is integrated in the i.MX8MP uses different bits in the > > MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration > > compared to the on in the RK3399. In addition, it adds a new bit for > > enabling crop. Add new definitions for these bits for i.MX8MP devices, > > and update where they are set. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ > > .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index > > 34f4fe09c88d..24ad2ccec2a3 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > @@ -168,6 +168,10 @@ > > #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) > > #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) > > > > +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) > > + > > Does it make sense to move this kind of information into struct rkisp1_info? > This way you can skip the if (isp_ver == ...) thing. Good question. Paul, what do you think ? If it doesn't get moved to the structure, I think I'd condition it by the RKISP1_FEATURE_RSZ_CROP feature bit instead of a version check, as it seems closely related. I'm actually leaning towards the latter. > > /* RSZ_CROP_[XY]_DIR */ > > #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << 0) > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index > > 08bf3aa8088f..29a31b18a082 100644 > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct > > rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, > > RKISP1_CIF_RSZ_CTRL); > > > > if (when == RKISP1_SHADOW_REGS_ASYNC) > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; > > + else > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > else > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; > > + else > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > > > rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); > > }
Hi Paul, Ping On Sat, Jun 18, 2022 at 02:03:19AM +0300, Laurent Pinchart wrote: > On Thu, Jun 16, 2022 at 10:05:06AM +0200, Alexander Stein wrote: > > Am Dienstag, 14. Juni 2022, 21:11:27 CEST schrieb Paul Elder: > > > The ISP that is integrated in the i.MX8MP uses different bits in the > > > MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration > > > compared to the on in the RK3399. In addition, it adds a new bit for > > > enabling crop. Add new definitions for these bits for i.MX8MP devices, > > > and update where they are set. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ > > > .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index > > > 34f4fe09c88d..24ad2ccec2a3 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > > @@ -168,6 +168,10 @@ > > > #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) > > > #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) > > > > > > +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) > > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) > > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) > > > + > > > > Does it make sense to move this kind of information into struct rkisp1_info? > > This way you can skip the if (isp_ver == ...) thing. > > Good question. Paul, what do you think ? If it doesn't get moved to the > structure, I think I'd condition it by the RKISP1_FEATURE_RSZ_CROP > feature bit instead of a version check, as it seems closely related. I'm > actually leaning towards the latter. > > > > /* RSZ_CROP_[XY]_DIR */ > > > #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << 0) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index > > > 08bf3aa8088f..29a31b18a082 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > > @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct > > > rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, > > > RKISP1_CIF_RSZ_CTRL); > > > > > > if (when == RKISP1_SHADOW_REGS_ASYNC) > > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; > > > + else > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > > else > > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; > > > + else > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > > > > > rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); > > > }
Hi Laurent, On Sat, Jun 18, 2022 at 02:03:17AM +0300, Laurent Pinchart wrote: > Hi Alexander, > > On Thu, Jun 16, 2022 at 10:05:06AM +0200, Alexander Stein wrote: > > Am Dienstag, 14. Juni 2022, 21:11:27 CEST schrieb Paul Elder: > > > The ISP that is integrated in the i.MX8MP uses different bits in the > > > MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration > > > compared to the on in the RK3399. In addition, it adds a new bit for > > > enabling crop. Add new definitions for these bits for i.MX8MP devices, > > > and update where they are set. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ > > > .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- > > > 2 files changed, 12 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index > > > 34f4fe09c88d..24ad2ccec2a3 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h > > > @@ -168,6 +168,10 @@ > > > #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) > > > #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) > > > > > > +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) > > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) > > > +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) > > > + > > > > Does it make sense to move this kind of information into struct rkisp1_info? > > This way you can skip the if (isp_ver == ...) thing. > > Good question. Paul, what do you think ? If it doesn't get moved to the > structure, I think I'd condition it by the RKISP1_FEATURE_RSZ_CROP > feature bit instead of a version check, as it seems closely related. I'm > actually leaning towards the latter. Yeah I think the latter too. Paul > > > > /* RSZ_CROP_[XY]_DIR */ > > > #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << 0) > > > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > > b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index > > > 08bf3aa8088f..29a31b18a082 100644 > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c > > > @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct > > > rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, > > > RKISP1_CIF_RSZ_CTRL); > > > > > > if (when == RKISP1_SHADOW_REGS_ASYNC) > > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; > > > + else > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; > > > else > > > - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > > + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; > > > + else > > > + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; > > > > > > rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); > > > }
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h index 34f4fe09c88d..24ad2ccec2a3 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h @@ -168,6 +168,10 @@ #define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO BIT(9) #define RKISP1_CIF_RSZ_SCALER_FACTOR BIT(16) +#define RKISP1_CIF_RSZ_CTRL_CROP_ENABLE_IMX BIT(8) +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX BIT(9) +#define RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX BIT(10) + /* RSZ_CROP_[XY]_DIR */ #define RKISP1_CIF_RSZ_CROP_XY_DIR(start, end) ((end) << 16 | (start) << 0) diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c index 08bf3aa8088f..29a31b18a082 100644 --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c @@ -209,9 +209,15 @@ static void rkisp1_rsz_update_shadow(struct rkisp1_resizer *rsz, u32 ctrl_cfg = rkisp1_rsz_read(rsz, RKISP1_CIF_RSZ_CTRL); if (when == RKISP1_SHADOW_REGS_ASYNC) - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO_IMX; + else + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_AUTO; else - ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; + if (rsz->rkisp1->info->isp_ver == IMX8MP_V10) + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD_IMX; + else + ctrl_cfg |= RKISP1_CIF_RSZ_CTRL_CFG_UPD; rkisp1_rsz_write(rsz, RKISP1_CIF_RSZ_CTRL, ctrl_cfg); }
The ISP that is integrated in the i.MX8MP uses different bits in the MRSZ_CTRL and SRSZ_CTRL registers for updating the configuration compared to the on in the RK3399. In addition, it adds a new bit for enabling crop. Add new definitions for these bits for i.MX8MP devices, and update where they are set. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h | 4 ++++ .../media/platform/rockchip/rkisp1/rkisp1-resizer.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 2 deletions(-)