Hi Heiko:
Thanks for you review.
On 11/27/23 23:29, Heiko Stübner wrote:
Hi Andy,
Am Mittwoch, 22. November 2023, 13:55:44 CET schrieb Andy Yan:
From: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
VOP2 on rk3588:
Four video ports:
VP0 Max 4096x2160
VP1 Max 4096x2160
VP2 Max 4096x2160
VP3 Max 2048x1080
4 4K Cluster windows with AFBC/line RGB and AFBC-only YUV support
4 4K Esmart windows with line RGB/YUV support
Signed-off-by: Andy Yan <andy.yan@xxxxxxxxxxxxxx>
---
Changes in v2:
- add rk3588_ prefix for functions which are rk3588 only
- make some calculation as fixed value and keep calculation formula as
comment
- check return value for some cru calculation functions.
- check return value for syscon_regmap_lookup_by_phandle
- add NV20/NV30 for esmart plane
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 381 ++++++++++++++++++-
drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 66 ++++
drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 221 +++++++++++
3 files changed, 660 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 4bcc405bcf11..9eecbe1f71f9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -271,9 +282,12 @@ static bool vop2_cluster_window(const struct vop2_win *win)
static void vop2_cfg_done(struct vop2_video_port *vp)
{
struct vop2 *vop2 = vp->vop2;
+ u32 val;
+
+ val = BIT(vp->id) | (BIT(vp->id) << 16) |
+ RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN;
- regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE,
- BIT(vp->id) | RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN);
+ regmap_set_bits(vop2->map, RK3568_REG_CFG_DONE, val);
I don't fully understand that code:
(1) the write mask is also present on the rk3568, so should this change
be a separate patch with a fixes tag?
The write mask of VP config done on rk356x is missing, that means
you can write the corresponding mask bit, but it has no effect.
I once considered making it a separate patch, I can split it as a separate patch if
you like.
(2) RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN does not contain the part for
the write-mask
#define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN BIT(15)
why is this working then?
Actually this bit has no write-mask bit. 🙂
}
static void vop2_win_disable(struct vop2_win *win)
[...]
@@ -1298,7 +1346,11 @@ static void vop2_plane_atomic_update(struct drm_plane *plane,
vop2_win_write(win, VOP2_WIN_AFBC_ENABLE, 1);
vop2_win_write(win, VOP2_WIN_AFBC_FORMAT, afbc_format);
vop2_win_write(win, VOP2_WIN_AFBC_UV_SWAP, uv_swap);
- vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
+ if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568)
+ vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 0);
+ else
+ vop2_win_write(win, VOP2_WIN_AFBC_AUTO_GATING_EN, 1);
+
I think this at least warrants a comment, what is happening here. Also,
can you already see how future vop2-users are behaving - aka are all new
socs in the "else" part of the conditional, or would a switch-case better
represent future socs?
On rk356x, this bit is auto gating enable, but this function is not work well so
we need to disable this function.
On rk3588, and the following new soc(rk3528/rk3576), this bit is gating disable,
we should write 1 to disable gating when enable a cluster window.
Maybe i add some comments in next version ?
vop2_win_write(win, VOP2_WIN_AFBC_BLOCK_SPLIT_EN, 0);
transform_offset = vop2_afbc_transform_offset(pstate, half_block_en);
vop2_win_write(win, VOP2_WIN_AFBC_HDR_PTR, yrgb_mst);
@@ -1627,9 +1937,17 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
- rk3568_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
+ /*
+ * for drive a high resolution(4KP120, 8K), vop on rk3588/rk3576 need
+ * process multi(1/2/4/8) pixels per cycle, so the dclk feed by the
+ * system cru may be the 1/2 or 1/4 of mode->clock.
+ */
+ clock = vop2_set_intf_mux(vp, rkencoder->crtc_endpoint_id, polflags);
}
+ if (!clock)
+ return;
+
hmm, shouldn't the check for the validity of a mode happen before
atomic_enable is run? So this shouldn't error out in the middle of the
function?
if (vcstate->output_mode == ROCKCHIP_OUT_MODE_AAAA &&
!(vp_data->feature & VOP_FEATURE_OUTPUT_10BIT))
out_mode = ROCKCHIP_OUT_MODE_P888;
Thanks
Heiko
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-rockchip
