Re: (EXT) [PATCH 55/55] media: rkisp1: Fix RSZ_CTRL bits for i.MX8MP

[paul.elder@ideasonboard.com]

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);
> > >  }