| Message ID | 20240904120238.3856782-6-andyshrk@163.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | VOP Support for rk3576 | expand |
Hi Andy, On Wed, Sep 04, 2024 at 08:02:32PM +0800, Andy Yan wrote: > From: Andy Yan <andy.yan@rock-chips.com> > > There is a version number hardcoded in the VOP VERSION_INFO > register, and the version number increments sequentially based > on the production order of the SOC. > > So using this version number to distinguish different VOP features > will simplify the code. > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > Changes in v2: > - Introduce vop hardware version > > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++--- > drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 11 +++++++++++ > drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 3 +++ > 3 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > index 9b269f6e576e..871d9bcd1d80 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > @@ -13,6 +13,15 @@ > #include "rockchip_drm_drv.h" > #include "rockchip_drm_vop.h" > > +#define VOP2_VERSION(major, minor, build) ((major) << 24 | (minor) << 16 | (build)) > + > +/* The new SOC VOP version is bigger than the old */ > +#define VOP_VERSION_RK3568 VOP2_VERSION(0x40, 0x15, 0x8023) > +#define VOP_VERSION_RK3588 VOP2_VERSION(0x40, 0x17, 0x6786) > +#define VOP_VERSION_RK3528 VOP2_VERSION(0x50, 0x17, 0x1263) > +#define VOP_VERSION_RK3562 VOP2_VERSION(0x50, 0x17, 0x4350) > +#define VOP_VERSION_RK3576 VOP2_VERSION(0x50, 0x19, 0x9765) What about the RK3566? Does it have the same version code as the RK3568? This new version field replaces the soc_id mechanism we had before to 99%. You keep the soc_id around just for distinguishing between RK3566 and RK3568. It would be nice to fully replace it. I see that the VOP_VERSION_RK* numbers are the same as found in the VOP2_SYS_VERSION_INF registers. On the other hand you never read the value from the register which make the VOP_VERSION_RK* just arbitrary numbers. Wouldn't it be possible to make something up for RK3566, like VOP2_VERSION(0x40, 0x15, 0x8022) to get rid of the soc_id thingy? Sascha
On Thu, Sep 05, 2024 at 04:09:58PM +0800, Andy Yan wrote: > Hi Sascha, > > At 2024-09-05 15:10:56, "Sascha Hauer" <s.hauer@pengutronix.de> wrote: > >Hi Andy, > > > >On Wed, Sep 04, 2024 at 08:02:32PM +0800, Andy Yan wrote: > >> From: Andy Yan <andy.yan@rock-chips.com> > >> > >> There is a version number hardcoded in the VOP VERSION_INFO > >> register, and the version number increments sequentially based > >> on the production order of the SOC. > >> > >> So using this version number to distinguish different VOP features > >> will simplify the code. > >> > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > >> > >> --- > >> > >> Changes in v2: > >> - Introduce vop hardware version > >> > >> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++--- > >> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 11 +++++++++++ > >> drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 3 +++ > >> 3 files changed, 18 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > >> index 9b269f6e576e..871d9bcd1d80 100644 > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > >> @@ -13,6 +13,15 @@ > >> #include "rockchip_drm_drv.h" > >> #include "rockchip_drm_vop.h" > >> > >> +#define VOP2_VERSION(major, minor, build) ((major) << 24 | (minor) << 16 | (build)) > >> + > >> +/* The new SOC VOP version is bigger than the old */ > >> +#define VOP_VERSION_RK3568 VOP2_VERSION(0x40, 0x15, 0x8023) > >> +#define VOP_VERSION_RK3588 VOP2_VERSION(0x40, 0x17, 0x6786) > >> +#define VOP_VERSION_RK3528 VOP2_VERSION(0x50, 0x17, 0x1263) > >> +#define VOP_VERSION_RK3562 VOP2_VERSION(0x50, 0x17, 0x4350) > >> +#define VOP_VERSION_RK3576 VOP2_VERSION(0x50, 0x19, 0x9765) > > > >What about the RK3566? Does it have the same version code as the RK3568? > > > >This new version field replaces the soc_id mechanism we had before to > >99%. You keep the soc_id around just for distinguishing between RK3566 > >and RK3568. It would be nice to fully replace it. > > > >I see that the VOP_VERSION_RK* numbers are the same as found in the > >VOP2_SYS_VERSION_INF registers. On the other hand you never read the > >value from the register which make the VOP_VERSION_RK* just arbitrary > >numbers. Wouldn't it be possible to make something up for RK3566, like > >VOP2_VERSION(0x40, 0x15, 0x8022) to get rid of the soc_id thingy? > Yes，RK3566 and RK3568 share the same VOP IP block， so the version code at VERSION_REGISTER is > the same, the difference between rk3568 and rk33566 are introduced at soc Integration。 > So i would still like to keep the soc_id to handle situation like this。As we always have such cause， one > same IP block， but there are some subtle differences in features across different SOCs. Fine with me. You could leave a comment in the code or commit message that explains why we need both. > I have considered reading the version register directly, but I haven't found a suitable method yet. You could check the expected version from the driver data against the register value, but that would only be an additional sanity check. Not sure if it's worth it. Sascha
On Mon Sep 9, 2024 at 11:13 AM CEST, Sascha Hauer wrote: > On Thu, Sep 05, 2024 at 04:09:58PM +0800, Andy Yan wrote: > > At 2024-09-05 15:10:56, "Sascha Hauer" <s.hauer@pengutronix.de> wrote: > > >On Wed, Sep 04, 2024 at 08:02:32PM +0800, Andy Yan wrote: > > >> From: Andy Yan <andy.yan@rock-chips.com> > > >> > > >> There is a version number hardcoded in the VOP VERSION_INFO > > >> register, and the version number increments sequentially based > > >> on the production order of the SOC. > > >> > > >> So using this version number to distinguish different VOP features > > >> will simplify the code. > > >> > > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > >> > > >> --- > > >> > > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > > >> index 9b269f6e576e..871d9bcd1d80 100644 > > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h > > >> @@ -13,6 +13,15 @@ > > >> #include "rockchip_drm_drv.h" > > >> #include "rockchip_drm_vop.h" > > >> > > >> +#define VOP2_VERSION(major, minor, build) ((major) << 24 | (minor) << 16 | (build)) > > >> + > > >> +/* The new SOC VOP version is bigger than the old */ > > >> +#define VOP_VERSION_RK3568 VOP2_VERSION(0x40, 0x15, 0x8023) > > >> +#define VOP_VERSION_RK3588 VOP2_VERSION(0x40, 0x17, 0x6786) > > >> +#define VOP_VERSION_RK3528 VOP2_VERSION(0x50, 0x17, 0x1263) > > >> +#define VOP_VERSION_RK3562 VOP2_VERSION(0x50, 0x17, 0x4350) > > >> +#define VOP_VERSION_RK3576 VOP2_VERSION(0x50, 0x19, 0x9765) > > > > > >What about the RK3566? Does it have the same version code as the RK3568? > > > > > >This new version field replaces the soc_id mechanism we had before to > > >99%. You keep the soc_id around just for distinguishing between RK3566 > > >and RK3568. It would be nice to fully replace it. > > > > > >I see that the VOP_VERSION_RK* numbers are the same as found in the > > >VOP2_SYS_VERSION_INF registers. On the other hand you never read the > > >value from the register which make the VOP_VERSION_RK* just arbitrary > > >numbers. Wouldn't it be possible to make something up for RK3566, like > > >VOP2_VERSION(0x40, 0x15, 0x8022) to get rid of the soc_id thingy? > > Yes，RK3566 and RK3568 share the same VOP IP block， so the version code at VERSION_REGISTER is > > the same, the difference between rk3568 and rk33566 are introduced at soc Integration。 > > So i would still like to keep the soc_id to handle situation like this。As we always have such cause， one > > same IP block， but there are some subtle differences in features across different SOCs. > > Fine with me. You could leave a comment in the code or commit > message that explains why we need both. Also (or especially?) add that to the commit message of patch 6 of this series. Patch 6's commit message talks about RK3576 while it changes code related to RK3566 and I (too?) thought that not using VOP_VERSION was an oversight, while it turns out to be deliberate. Cheers, Diederik
Hi Sascha， At 2024-09-09 17:13:55, "Sascha Hauer" <s.hauer@pengutronix.de> wrote: >On Thu, Sep 05, 2024 at 04:09:58PM +0800, Andy Yan wrote: >> Hi Sascha, >> >> At 2024-09-05 15:10:56, "Sascha Hauer" <s.hauer@pengutronix.de> wrote: >> >Hi Andy, >> > >> >On Wed, Sep 04, 2024 at 08:02:32PM +0800, Andy Yan wrote: >> >> From: Andy Yan <andy.yan@rock-chips.com> >> >> >> >> There is a version number hardcoded in the VOP VERSION_INFO >> >> register, and the version number increments sequentially based >> >> on the production order of the SOC. >> >> >> >> So using this version number to distinguish different VOP features >> >> will simplify the code. >> >> >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> >> >> >> --- >> >> >> >> Changes in v2: >> >> - Introduce vop hardware version >> >> >> >> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 7 ++++--- >> >> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 11 +++++++++++ >> >> drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 3 +++ >> >> 3 files changed, 18 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h >> >> index 9b269f6e576e..871d9bcd1d80 100644 >> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h >> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h >> >> @@ -13,6 +13,15 @@ >> >> #include "rockchip_drm_drv.h" >> >> #include "rockchip_drm_vop.h" >> >> >> >> +#define VOP2_VERSION(major, minor, build) ((major) << 24 | (minor) << 16 | (build)) >> >> + >> >> +/* The new SOC VOP version is bigger than the old */ >> >> +#define VOP_VERSION_RK3568 VOP2_VERSION(0x40, 0x15, 0x8023) >> >> +#define VOP_VERSION_RK3588 VOP2_VERSION(0x40, 0x17, 0x6786) >> >> +#define VOP_VERSION_RK3528 VOP2_VERSION(0x50, 0x17, 0x1263) >> >> +#define VOP_VERSION_RK3562 VOP2_VERSION(0x50, 0x17, 0x4350) >> >> +#define VOP_VERSION_RK3576 VOP2_VERSION(0x50, 0x19, 0x9765) >> > >> >What about the RK3566? Does it have the same version code as the RK3568? >> > >> >This new version field replaces the soc_id mechanism we had before to >> >99%. You keep the soc_id around just for distinguishing between RK3566 >> >and RK3568. It would be nice to fully replace it. >> > >> >I see that the VOP_VERSION_RK* numbers are the same as found in the >> >VOP2_SYS_VERSION_INF registers. On the other hand you never read the >> >value from the register which make the VOP_VERSION_RK* just arbitrary >> >numbers. Wouldn't it be possible to make something up for RK3566, like >> >VOP2_VERSION(0x40, 0x15, 0x8022) to get rid of the soc_id thingy? >> Yes，RK3566 and RK3568 share the same VOP IP block， so the version code at VERSION_REGISTER is >> the same, the difference between rk3568 and rk33566 are introduced at soc Integration。 >> So i would still like to keep the soc_id to handle situation like this。As we always have such cause， one >> same IP block， but there are some subtle differences in features across different SOCs. > >Fine with me. You could leave a comment in the code or commit >message that explains why we need both. Ok， will add in V3.> >> I have considered reading the version register directly, but I haven't found a suitable method yet. > >You could check the expected version from the driver data against >the register value, but that would only be an additional sanity check. >Not sure if it's worth it. I think we can add a check like that to make sure the version code matchs the real register value，rather than being an arbitrarily created value. >Sascha > >-- >Pengutronix e.K. | | >Steuerwalder Str. 21 | http://www.pengutronix.de/ | >31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > >_______________________________________________ >Linux-rockchip mailing list >Linux-rockchip@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi ， At 2024-09-09 17:36:14, "Diederik de Haas" <didi.debian@cknow.org> wrote: >On Mon Sep 9, 2024 at 11:13 AM CEST, Sascha Hauer wrote: >> On Thu, Sep 05, 2024 at 04:09:58PM +0800, Andy Yan wrote: >> > At 2024-09-05 15:10:56, "Sascha Hauer" <s.hauer@pengutronix.de> wrote: >> > >On Wed, Sep 04, 2024 at 08:02:32PM +0800, Andy Yan wrote: >> > >> From: Andy Yan <andy.yan@rock-chips.com> >> > >> >> > >> There is a version number hardcoded in the VOP VERSION_INFO >> > >> register, and the version number increments sequentially based >> > >> on the production order of the SOC. >> > >> >> > >> So using this version number to distinguish different VOP features >> > >> will simplify the code. >> > >> >> > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> > >> >> > >> --- >> > >> >> > >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h >> > >> index 9b269f6e576e..871d9bcd1d80 100644 >> > >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h >> > >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h >> > >> @@ -13,6 +13,15 @@ >> > >> #include "rockchip_drm_drv.h" >> > >> #include "rockchip_drm_vop.h" >> > >> >> > >> +#define VOP2_VERSION(major, minor, build) ((major) << 24 | (minor) << 16 | (build)) >> > >> + >> > >> +/* The new SOC VOP version is bigger than the old */ >> > >> +#define VOP_VERSION_RK3568 VOP2_VERSION(0x40, 0x15, 0x8023) >> > >> +#define VOP_VERSION_RK3588 VOP2_VERSION(0x40, 0x17, 0x6786) >> > >> +#define VOP_VERSION_RK3528 VOP2_VERSION(0x50, 0x17, 0x1263) >> > >> +#define VOP_VERSION_RK3562 VOP2_VERSION(0x50, 0x17, 0x4350) >> > >> +#define VOP_VERSION_RK3576 VOP2_VERSION(0x50, 0x19, 0x9765) >> > > >> > >What about the RK3566? Does it have the same version code as the RK3568? >> > > >> > >This new version field replaces the soc_id mechanism we had before to >> > >99%. You keep the soc_id around just for distinguishing between RK3566 >> > >and RK3568. It would be nice to fully replace it. >> > > >> > >I see that the VOP_VERSION_RK* numbers are the same as found in the >> > >VOP2_SYS_VERSION_INF registers. On the other hand you never read the >> > >value from the register which make the VOP_VERSION_RK* just arbitrary >> > >numbers. Wouldn't it be possible to make something up for RK3566, like >> > >VOP2_VERSION(0x40, 0x15, 0x8022) to get rid of the soc_id thingy? >> > Yes，RK3566 and RK3568 share the same VOP IP block， so the version code at VERSION_REGISTER is >> > the same, the difference between rk3568 and rk33566 are introduced at soc Integration。 >> > So i would still like to keep the soc_id to handle situation like this。As we always have such cause， one >> > same IP block， but there are some subtle differences in features across different SOCs. >> >> Fine with me. You could leave a comment in the code or commit >> message that explains why we need both. > >Also (or especially?) add that to the commit message of patch 6 of this >series. Patch 6's commit message talks about RK3576 while it changes >code related to RK3566 and I (too?) thought that not using VOP_VERSION >was an oversight, while it turns out to be deliberate. OK， will do in v3.> >Cheers, > Diederik
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c index 6a7982ad3550..f32cfb25063f 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c @@ -354,7 +354,7 @@ static bool vop2_output_uv_swap(u32 bus_format, u32 output_mode) static bool vop2_output_rg_swap(struct vop2 *vop2, u32 bus_format) { - if (vop2->data->soc_id == 3588) { + if (vop2->version == VOP_VERSION_RK3588) { if (bus_format == MEDIA_BUS_FMT_YUV8_1X24 || bus_format == MEDIA_BUS_FMT_YUV10_1X30) return true; @@ -820,7 +820,7 @@ static void vop2_enable(struct vop2 *vop2) if (vop2->data->soc_id == 3566) vop2_writel(vop2, RK3568_OTP_WIN_EN, 1); - if (vop2->data->soc_id == 3588) + if (vop2->version == VOP_VERSION_RK3588) rk3588_vop2_power_domain_enable_all(vop2); vop2_writel(vop2, RK3568_REG_CFG_DONE, RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN); @@ -1231,7 +1231,7 @@ static void vop2_plane_atomic_update(struct drm_plane *plane, * this bit is gating disable, we should write 1 to * disable gating when enable afbc. */ - if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568) + if (vop2->version == VOP_VERSION_RK3568) vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0); else vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1); @@ -2320,6 +2320,7 @@ static int vop2_bind(struct device *dev, struct device *master, void *data) vop2->dev = dev; vop2->data = vop2_data; vop2->ops = vop2_data->ops; + vop2->version = vop2_data->version; vop2->drm = drm; dev_set_drvdata(dev, vop2); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h index 9b269f6e576e..871d9bcd1d80 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h @@ -13,6 +13,15 @@ #include "rockchip_drm_drv.h" #include "rockchip_drm_vop.h" +#define VOP2_VERSION(major, minor, build) ((major) << 24 | (minor) << 16 | (build)) + +/* The new SOC VOP version is bigger than the old */ +#define VOP_VERSION_RK3568 VOP2_VERSION(0x40, 0x15, 0x8023) +#define VOP_VERSION_RK3588 VOP2_VERSION(0x40, 0x17, 0x6786) +#define VOP_VERSION_RK3528 VOP2_VERSION(0x50, 0x17, 0x1263) +#define VOP_VERSION_RK3562 VOP2_VERSION(0x50, 0x17, 0x4350) +#define VOP_VERSION_RK3576 VOP2_VERSION(0x50, 0x19, 0x9765) + #define VOP2_VP_FEATURE_OUTPUT_10BIT BIT(0) #define VOP2_FEATURE_HAS_SYS_GRF BIT(0) @@ -235,6 +244,7 @@ struct vop2_ops { struct vop2_data { u8 nr_vps; u64 feature; + u32 version; const struct vop2_ops *ops; const struct vop2_win_data *win; const struct vop2_video_port_data *vp; @@ -252,6 +262,7 @@ struct vop2_data { }; struct vop2 { + u32 version; struct device *dev; struct drm_device *drm; struct vop2_video_port vps[ROCKCHIP_MAX_CRTC]; diff --git a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c index b44fff0d4cb7..a8a129892977 100644 --- a/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c +++ b/drivers/gpu/drm/rockchip/rockchip_vop2_reg.c @@ -1550,6 +1550,7 @@ static const struct vop2_ops rk3588_vop_ops = { }; static const struct vop2_data rk3566_vop = { + .version = VOP_VERSION_RK3568, .feature = VOP2_FEATURE_HAS_SYS_GRF, .nr_vps = 3, .max_input = { 4096, 2304 }, @@ -1568,6 +1569,7 @@ static const struct vop2_data rk3566_vop = { }; static const struct vop2_data rk3568_vop = { + .version = VOP_VERSION_RK3568, .feature = VOP2_FEATURE_HAS_SYS_GRF, .nr_vps = 3, .max_input = { 4096, 2304 }, @@ -1586,6 +1588,7 @@ static const struct vop2_data rk3568_vop = { }; static const struct vop2_data rk3588_vop = { + .version = VOP_VERSION_RK3588, .feature = VOP2_FEATURE_HAS_SYS_GRF | VOP2_FEATURE_HAS_VO1_GRF | VOP2_FEATURE_HAS_VOP_GRF | VOP2_FEATURE_HAS_SYS_PMU, .nr_vps = 4,