Message-ID: <7b45f190.452f.1928e41b746.Coremail.andyshrk@163.com>
Date: Tue, 15 Oct 2024 11:38:44 +0800 (CST)
From: "Andy Yan" <andyshrk@....com>
To: "Piotr Zalewski" <pZ010001011111@...ton.me>
Cc: hjc@...k-chips.com, heiko@...ech.de, andy.yan@...k-chips.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, 
	tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch, 
	dri-devel@...ts.freedesktop.org, 
	linux-arm-kernel@...ts.infradead.org, 
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	skhan@...uxfoundation.org, "Daniel Stone" <daniel@...ishbar.org>, 
	"Dragan Simic" <dsimic@...jaro.org>, 
	"Diederik de Haas" <didi.debian@...ow.org>
Subject: Re:[PATCH v5] rockchip/drm: vop2: add support for gamma LUT


Hi Piotr,

At 2024-10-15 06:30:27, "Piotr Zalewski" <pZ010001011111@...ton.me> wrote:
>Add support for gamma LUT in VOP2 driver. The implementation was inspired
>by one found in VOP1 driver. Blue and red channels in gamma LUT register
>write were swapped with respect to how gamma LUT values are written in
>VOP1. Gamma LUT port selection was added before the write of new gamma LUT 
>table. If the current SoC is RK3588, "seamless" gamma lut update is 
>performed similarly to how it was done in the case of RK3399 in VOP1[1]. In
>seamless update gamma LUT disable before the write isn't necessary, 
>different register is being used to select gamma LUT port[2] and after 
>setting DSP_LUT_EN bit, GAMMA_UPDATE_EN bit is set[3]. Gamma size is set 
>and drm color management is enabled for each video port's CRTC except ones 
>which have no associated device. 
>
>Solution was tested on RK3566 (Pinetab2). When using userspace tools
>which set eg. constant color temperature no issues were noticed. When
>using userspace tools which adjust eg. color temperature the slight screen
>flicker is visible probably because of gamma LUT disable.
>
>Compare behaviour of eg.:
>```
>gammastep -O 3000
>```
>
>To eg.:
>```
>gammastep -l 53:23 -t 6000:3000
>```
>
>In latter case color temperature is slowly adjusted at the beginning which
>causes screen to slighly flicker. Then it adjusts every few seconds which 
>also causes slight screen flicker.
>
>[1] https://lists.infradead.org/pipermail/linux-rockchip/2021-October/028132.html
>[2] https://lore.kernel.org/linux-rockchip/48249708-8c05-40d2-a5d8-23de960c5a77@rock-chips.com/
>[3] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3437
>
>Helped-by: Daniel Stone <daniel@...ishbar.org>
>Helped-by: Dragan Simic <dsimic@...jaro.org>
>Helped-by: Diederik de Haas <didi.debian@...ow.org>
>Helped-by: Andy Yan <andy.yan@...k-chips.com>
>Signed-off-by: Piotr Zalewski <pZ010001011111@...ton.me>
>---
>
>Notes:
>    Changes in v5:
>        - Do not trigger full modeset in case seamless gamma lut update
>		  isn't possible (eg. rk356x case). It was discovered that with 
>		  full modeset, userspace tools which adjust color temperature with
>          high frequency cause screen to go black and reduce overall
>          performance. Instead, revert to previous behaviour of lut update
>		  happening in atomic_begin or (in case there is a modeset) in
>		  atomic_enable. Also, add unrelated to modeset trigger
>		  changes/improvements from v4 on top. Improve code readability
>		  too.
>
>    Changes in v4:
>        - rework the implementation to better utilize DRM atomic updates[2]
>        - handle the RK3588 case[2][3]
>    
>    Changes in v3:
>        - v3 is patch v2 "resend", by mistake the incremental patch was
>        sent in v2
>    
>    Changes in v2:
>        - Apply code styling corrections[1]
>        - Move gamma LUT write inside the vop2 lock
>	
>	Link to v4: https://lore.kernel.org/linux-rockchip/20240815124306.189282-2-pZ010001011111@proton.me/
>    Link to v3: https://lore.kernel.org/linux-rockchip/TkgKVivuaLFLILPY-n3iZo_8KF-daKdqdu-0_e0HP-5Ar_8DALDeNWog2suwWKjX7eomcbGET0KZe7DlzdhK2YM6CbLbeKeFZr-MJzJMtw0=@proton.me/
>    Link to v2: https://lore.kernel.org/linux-rockchip/Hk03HDb6wSSHWtEFZHUye06HR0-9YzP5nCHx9A8_kHzWSZawDrU1o1pjEGkCOJFoRg0nTB4BWEv6V0XBOjF4-0Mj44lp2TrjaQfnytzp-Pk=@proton.me/T/#u
>    Link to v1: https://lore.kernel.org/linux-rockchip/9736eadf6a9d8e97eef919c6b3d88828@manjaro.org/T/#t
>    
>    [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@manjaro.org
>    [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=Q9ip7F_HAPYnbCqPouw@mail.gmail.com
>    [3] https://lore.kernel.org/linux-rockchip/7d998e4c-e1d3-4e8b-af76-c5bc83b43647@rock-chips.com
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 161 +++++++++++++++++++
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |   5 +
> 2 files changed, 166 insertions(+)
>
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>index 9873172e3fd3..a6a2d7df5ecc 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>@@ -278,6 +278,15 @@ static u32 vop2_readl(struct vop2 *vop2, u32 offset)
> 	return val;
> }
> 
>+static u32 vop2_vp_read(struct vop2_video_port *vp, u32 offset)
>+{
>+	u32 val;
>+
>+	regmap_read(vp->vop2->map, vp->data->offset + offset, &val);
>+
>+	return val;
>+}
>+
> static void vop2_win_write(const struct vop2_win *win, unsigned int reg, u32 v)
> {
> 	regmap_field_write(win->reg[reg], v);
>@@ -998,6 +1007,55 @@ static void vop2_disable(struct vop2 *vop2)
> 	clk_disable_unprepare(vop2->hclk);
> }
> 
>+static bool vop2_vp_dsp_lut_is_enabled(struct vop2_video_port *vp)
>+{
>+	return (u32) (vop2_vp_read(vp, RK3568_VP_DSP_CTRL) & RK3568_VP_DSP_CTRL__DSP_LUT_EN) >
>+	    0;
>+}
>+
>+static void vop2_vp_dsp_lut_disable(struct vop2_video_port *vp)
>+{
>+	u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>+
>+	dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN;
>+	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>+}
>+
>+static bool vop2_vp_dsp_lut_poll_disable(struct vop2_video_port *vp)
>+{
>+	u32 dsp_ctrl;
>+	int ret = readx_poll_timeout(vop2_vp_dsp_lut_is_enabled, vp, dsp_ctrl,
>+				!dsp_ctrl, 5, 30 * 1000);
>+	if (ret) {
>+		drm_err(vp->vop2->drm, "display LUT RAM enable timeout!\n");
>+		return false;
>+	}
>+
>+	return true;
>+}
>+
>+static void vop2_vp_dsp_lut_enable(struct vop2_video_port *vp)
>+{
>+	u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>+
>+	dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN;
>+	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>+}
>+
>+static void vop2_vp_dsp_lut_update_enable(struct vop2_video_port *vp)
>+{
>+	u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>+
>+	dsp_ctrl |= RK3588_VP_DSP_CTRL__GAMMA_UPDATE_EN;
>+	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>+}
>+
>+static inline bool vop2_supports_seamless_gamma_lut_update(struct vop2 *vop2)
>+{
>+	return (vop2->data->soc_id == 3588);
>+}
>+
>+
> static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> 				     struct drm_atomic_state *state)
> {
>@@ -1482,6 +1540,66 @@ static bool vop2_crtc_mode_fixup(struct drm_crtc *crtc,
> 	return true;
> }
> 
>+static void vop2_crtc_write_gamma_lut(struct vop2 *vop2, struct drm_crtc *crtc)
>+{
>+	const struct vop2_video_port *vp = to_vop2_video_port(crtc);
>+	const struct vop2_video_port_data *vp_data = &vop2->data->vp[vp->id];
>+
>+	struct drm_color_lut *lut = crtc->state->gamma_lut->data;
>+	unsigned int i, bpc = ilog2(vp_data->gamma_lut_len);
>+	u32 word;
>+
>+	for (i = 0; i < crtc->gamma_size; i++) {
>+		word = (drm_color_lut_extract(lut[i].blue, bpc) << (2 * bpc)) |
>+		    (drm_color_lut_extract(lut[i].green, bpc) << bpc) |
>+		    drm_color_lut_extract(lut[i].red, bpc);
>+
>+		writel(word, vop2->lut_regs + i * 4);
>+	}
>+}
>+
>+static void vop2_crtc_atomic_try_set_gamma(struct vop2 *vop2,

Why we need a  _try_ in the function name ?

>+					struct vop2_video_port *vp,
>+					struct drm_crtc *crtc,
>+					struct drm_crtc_state *crtc_state)
>+{
>+
>+	if (vop2->lut_regs && crtc_state->color_mgmt_changed) {
>+		if (!crtc_state->gamma_lut) {
>+			vop2_vp_dsp_lut_disable(vp);
>+			return;
>+		}
>+
>+		if (vop2_supports_seamless_gamma_lut_update(vop2)) {
I think it's bettery to check for rk3568/rk3566 here,  the newer soc will all follow
rk3588 support seamless gamma lut update.


>+			vop2_writel(vop2, RK3568_LUT_PORT_SEL, FIELD_PREP(
>+				RK3588_LUT_PORT_SEL__GAMMA_AHB_WRITE_SEL,
>+				vp->id));
>+			vop2_crtc_write_gamma_lut(vop2, crtc);
>+			vop2_vp_dsp_lut_enable(vp);
>+			vop2_vp_dsp_lut_update_enable(vp);
>+		} else {

As for rk3566/68, we should do exclusive check here, because there is only
one gamma ,  only one VP can use it at a time. See my comments in V3:

https://patchwork.kernel.org/project/linux-rockchip/patch/TkgKVivuaLFLILPY-n3iZo_8KF-daKdqdu-0_e0HP-5Ar_8DALDeNWog2suwWKjX7eomcbGET0KZe7DlzdhK2YM6CbLbeKeFZr-MJzJMtw0=@proton.me/


>+			vop2_vp_dsp_lut_disable(vp);
>+			vop2_cfg_done(vp);
>+			if (!vop2_vp_dsp_lut_poll_disable(vp))
>+				return;
>+
>+			vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
>+			vop2_crtc_write_gamma_lut(vop2, crtc);
>+			vop2_vp_dsp_lut_enable(vp);
>+		}
>+	}
>+}
>+
>+static inline void vop2_crtc_atomic_try_set_gamma_locked(struct vop2 *vop2,
>+					struct vop2_video_port *vp,
>+					struct drm_crtc *crtc,
>+					struct drm_crtc_state *crtc_state)
>+{
>+	vop2_lock(vop2);
>+	vop2_crtc_atomic_try_set_gamma(vop2, vp, crtc, crtc_state);
>+	vop2_unlock(vop2);
>+}
>+
> static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl)
> {
> 	struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
>@@ -2057,11 +2175,35 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> 
> 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> 
>+	vop2_crtc_atomic_try_set_gamma(vop2, vp, crtc, crtc_state);
>+
> 	drm_crtc_vblank_on(crtc);
> 
> 	vop2_unlock(vop2);
> }
> 
>+static int vop2_crtc_atomic_check_gamma(struct vop2_video_port *vp,
>+					struct drm_crtc *crtc,
>+					struct drm_atomic_state *state,
>+					struct drm_crtc_state *crtc_state)
>+{
>+	struct vop2 *vop2 = vp->vop2;
>+	unsigned int len;
>+
>+	if (!vp->vop2->lut_regs || !crtc_state->color_mgmt_changed ||
>+	    !crtc_state->gamma_lut)
>+		return 0;
>+
>+	len = drm_color_lut_size(crtc_state->gamma_lut);
>+	if (len != crtc->gamma_size) {
>+		drm_dbg(vop2->drm, "Invalid LUT size; got %d, expected %d\n",
>+				      len, crtc->gamma_size);
>+		return -EINVAL;
>+	}
>+
>+	return 0;
>+}
>+
> static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
> 				  struct drm_atomic_state *state)
> {
>@@ -2069,6 +2211,11 @@ static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
> 	struct drm_plane *plane;
> 	int nplanes = 0;
> 	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>+	int ret;
>+
>+	ret = vop2_crtc_atomic_check_gamma(vp, crtc, state, crtc_state);
>+	if (ret)
>+		return ret;
> 
> 	drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
> 		nplanes++;
>@@ -2456,9 +2603,12 @@ static void vop2_setup_dly_for_windows(struct vop2 *vop2)
> 	vop2_writel(vop2, RK3568_SMART_DLY_NUM, sdly);
> }
> 
>+
> static void vop2_crtc_atomic_begin(struct drm_crtc *crtc,
> 				   struct drm_atomic_state *state)
> {
>+	struct drm_crtc_state *crtc_state =
>+		drm_atomic_get_new_crtc_state(state, crtc);
> 	struct vop2_video_port *vp = to_vop2_video_port(crtc);
> 	struct vop2 *vop2 = vp->vop2;
> 	struct drm_plane *plane;
>@@ -2482,6 +2632,11 @@ static void vop2_crtc_atomic_begin(struct drm_crtc *crtc,
> 	vop2_setup_layer_mixer(vp);
> 	vop2_setup_alpha(vp);
> 	vop2_setup_dly_for_windows(vop2);
>+
>+	// NOTE: in case of modeset gamma lut update
>+	// already happened in atomic enable
>+	if (!drm_atomic_crtc_needs_modeset(crtc_state))
>+		vop2_crtc_atomic_try_set_gamma_locked(vop2, vp, crtc, crtc_state);

I would like update gamma at crtc_atomic_flush callback like mediateck, tidss, ast, and
we also following these in our bsp code.

> }
> 
> static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
>@@ -2790,7 +2945,13 @@ static int vop2_create_crtcs(struct vop2 *vop2)
> 		}
> 
> 		drm_crtc_helper_add(&vp->crtc, &vop2_crtc_helper_funcs);
>+		if (vop2->lut_regs && vp->crtc.dev != NULL) {
>+			const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
> 
>+			drm_mode_crtc_set_gamma_size(&vp->crtc, vp_data->gamma_lut_len);
>+			drm_crtc_enable_color_mgmt(&vp->crtc, 0, false,
>+						   vp_data->gamma_lut_len);

It seems that we can keep it in one line,  the default limit  of linux kernel coding style is 100 characters now.


>+		}
> 		init_completion(&vp->dsp_hold_completion);
> 	}
> 
>diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>index 615a16196aff..510dda6f9092 100644
>--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
>@@ -394,6 +394,7 @@ enum dst_factor_mode {
> #define RK3568_REG_CFG_DONE__GLB_CFG_DONE_EN		BIT(15)
> 
> #define RK3568_VP_DSP_CTRL__STANDBY			BIT(31)
>+#define RK3568_VP_DSP_CTRL__DSP_LUT_EN			BIT(28)
> #define RK3568_VP_DSP_CTRL__DITHER_DOWN_MODE		BIT(20)
> #define RK3568_VP_DSP_CTRL__DITHER_DOWN_SEL		GENMASK(19, 18)
> #define RK3568_VP_DSP_CTRL__DITHER_DOWN_EN		BIT(17)
>@@ -408,6 +409,8 @@ enum dst_factor_mode {
> #define RK3568_VP_DSP_CTRL__CORE_DCLK_DIV		BIT(4)
> #define RK3568_VP_DSP_CTRL__OUT_MODE			GENMASK(3, 0)
> 
>+#define RK3588_VP_DSP_CTRL__GAMMA_UPDATE_EN		BIT(22)
>+
> #define RK3588_VP_CLK_CTRL__DCLK_OUT_DIV		GENMASK(3, 2)
> #define RK3588_VP_CLK_CTRL__DCLK_CORE_DIV		GENMASK(1, 0)
> 
>@@ -460,6 +463,8 @@ enum dst_factor_mode {
> #define RK3588_DSP_IF_POL__DP1_PIN_POL			GENMASK(14, 12)
> #define RK3588_DSP_IF_POL__DP0_PIN_POL			GENMASK(10, 8)
> 
>+#define RK3588_LUT_PORT_SEL__GAMMA_AHB_WRITE_SEL	GENMASK(13, 12)
>+
> #define RK3568_VP0_MIPI_CTRL__DCLK_DIV2_PHASE_LOCK	BIT(5)
> #define RK3568_VP0_MIPI_CTRL__DCLK_DIV2			BIT(4)
> 
>-- 
>2.47.0
>
>
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@...ts.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists