[v5] rockchip/drm: vop2: add support for gamma LUT

Commit Message

Piotr Zalewski Oct. 14, 2024, 10:30 p.m. UTC

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/[e-mail]/
[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 <[e-mail]>
Helped-by: Dragan Simic <[e-mail]>
Helped-by: Diederik de Haas <[e-mail]>
Helped-by: Andy Yan <[e-mail]>
Signed-off-by: Piotr Zalewski <[e-mail]>
---

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/[e-mail]/
    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/[e-mail]/T/#t
    
    [1] https://lore.kernel.org/linux-rockchip/[e-mail]
    [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=[e-mail]
    [3] https://lore.kernel.org/linux-rockchip/[e-mail]

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 161 +++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |   5 +
 2 files changed, 166 insertions(+)

Comments

Andy Yan Oct. 15, 2024, 3:38 a.m. UTC | #1

Hi Piotr,

At 2024-10-15 06:30:27, "Piotr Zalewski" <[e-mail]> 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/[e-mail]/
>[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 <[e-mail]>
>Helped-by: Dragan Simic <[e-mail]>
>Helped-by: Diederik de Haas <[e-mail]>
>Helped-by: Andy Yan <[e-mail]>
>Signed-off-by: Piotr Zalewski <[e-mail]>
>---
>
>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/[e-mail]/
>    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/[e-mail]/T/#t
>    
>    [1] https://lore.kernel.org/linux-rockchip/[e-mail]
>    [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=[e-mail]
>    [3] https://lore.kernel.org/linux-rockchip/[e-mail]
>
> 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
>[e-mail]
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Piotr Zalewski Oct. 15, 2024, 11:55 a.m. UTC | #2

On Tuesday, October 15th, 2024 at 5:38 AM, Andy Yan <[e-mail]> wrote:

> Hi Piotr,
> 
> At 2024-10-15 06:30:27, "Piotr Zalewski" [e-mail] 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/[e-mail]/
> > [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 [e-mail]
> > Helped-by: Dragan Simic [e-mail]
> > Helped-by: Diederik de Haas [e-mail]
> > Helped-by: Andy Yan [e-mail]
> > Signed-off-by: Piotr Zalewski [e-mail]
> > ---
> > 
> > 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/[e-mail]/
> > 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/[e-mail]/T/#t
> > 
> > [1] https://lore.kernel.org/linux-rockchip/[e-mail]
> > [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=[e-mail]
> > [3] https://lore.kernel.org/linux-rockchip/[e-mail]
> > 
> > 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 ?

Since there is an if condition which checks for lut regs and color mgmt 
changed flag directly in the function, 'try' in the function name 
increases readability by suggesting that.

> > + 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.

I will change in the next version.

> > + 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:

What do you mean exactly by exclusive check in this case.It's true that
gamma LUT is shared across video ports in rk356x but, if I correctly
understand, this doesn't forbid to reprogram LUT port sel and allow other
VP to use gamma LUT.

> 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.

I will move it to atomic_flush in the next version.

> > }
> > 
> > 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.

Thanks. I didn't know, I will amend it.

> > + }
> > 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
> > [e-mail]
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

Andy Yan Oct. 15, 2024, 12:22 p.m. UTC | #3

Hi Piotr,

At 2024-10-15 19:55:44, "Piotr Zalewski" <[e-mail]> wrote:
>On Tuesday, October 15th, 2024 at 5:38 AM, Andy Yan <[e-mail]> wrote:
>
>> Hi Piotr,
>> 
>> At 2024-10-15 06:30:27, "Piotr Zalewski" [e-mail] 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/[e-mail]/
>> > [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 [e-mail]
>> > Helped-by: Dragan Simic [e-mail]
>> > Helped-by: Diederik de Haas [e-mail]
>> > Helped-by: Andy Yan [e-mail]
>> > Signed-off-by: Piotr Zalewski [e-mail]
>> > ---
>> > 
>> > 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/[e-mail]/
>> > 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/[e-mail]/T/#t
>> > 
>> > [1] https://lore.kernel.org/linux-rockchip/[e-mail]
>> > [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=[e-mail]
>> > [3] https://lore.kernel.org/linux-rockchip/[e-mail]
>> > 
>> > 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 ?
>
>Since there is an if condition which checks for lut regs and color mgmt 
>changed flag directly in the function, 'try' in the function name 
>increases readability by suggesting that.
>
>> > + 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.
>
>I will change in the next version.
>
>> > + 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:
>
>What do you mean exactly by exclusive check in this case.It's true that
>gamma LUT is shared across video ports in rk356x but, if I correctly
>understand, this doesn't forbid to reprogram LUT port sel and allow other
>VP to use gamma LUT.

Yes, we can reprogram LUT port sel,  but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
want reprogram LUT port sel form VPx to VPy.

>
>> 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.
>
>I will move it to atomic_flush in the next version.
>
>> > }
>> > 
>> > 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.
>
>Thanks. I didn't know, I will amend it.

See bdc48fa11e46("checkpatch/coding-style: deprecate 80-column warning")

>
>> > + }
>> > 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
>> > [e-mail]
>> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>_______________________________________________
>Linux-rockchip mailing list
>[e-mail]
>http://lists.infradead.org/mailman/listinfo/linux-rockchip

Piotr Zalewski Oct. 15, 2024, 8:13 p.m. UTC | #4

Hi Andy

On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan <[e-mail]> wrote:

> > > > + 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.
> > 
> > I will change in the next version.
> > 
> > > > + 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:
> > 
> > What do you mean exactly by exclusive check in this case.It's true that
> > gamma LUT is shared across video ports in rk356x but, if I correctly
> > understand, this doesn't forbid to reprogram LUT port sel and allow other
> > VP to use gamma LUT.
> 
> 
> Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
> want reprogram LUT port sel form VPx to VPy.
> 

Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
being set, instead of disabling the LUT before the gamma LUT write for the
current CRTC's video port, active video port is selected. Selection is 
based on if DSP LUT EN bit is set for particular video port. eg:
```
static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
{
	struct vop2_video_port *vp;
	int i;
	for (i = 0; i < vop2->data->nr_vps; i++) {
		vp = &vop2->vps[i];

		if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
			return vp;
		}
	}
	return NULL;
}

(...)

struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);

if (active_vp) {
	vop2_vp_dsp_lut_disable(active_vp);
	vop2_cfg_done(active_vp);
	if (!vop2_vp_dsp_lut_poll_disable(active_vp))
		return;
}

vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
vop2_crtc_write_gamma_lut(vop2, crtc);
vop2_vp_dsp_lut_enable(vp);
```


> > > > 
> > > > 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.
> > 
> > Thanks. I didn't know, I will amend it.
> 
> 
> See bdc48fa11e46("checkpatch/coding-style: deprecate 80-column warning")
> 

Interesting.

Best regards, Piotr Zalewski

Andy Yan Oct. 16, 2024, 1:10 a.m. UTC | #5

Hi Piotr,

At 2024-10-16 04:13:40, "Piotr Zalewski" <[e-mail]> wrote:
>Hi Andy
>
>On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan <[e-mail]> wrote:
>
>> > > > + 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.
>> > 
>> > I will change in the next version.
>> > 
>> > > > + 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:
>> > 
>> > What do you mean exactly by exclusive check in this case.It's true that
>> > gamma LUT is shared across video ports in rk356x but, if I correctly
>> > understand, this doesn't forbid to reprogram LUT port sel and allow other
>> > VP to use gamma LUT.
>> 
>> 
>> Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
>> want reprogram LUT port sel form VPx to VPy.
>> 
>
>Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
>being set, instead of disabling the LUT before the gamma LUT write for the
>current CRTC's video port, active video port is selected. Selection is 
>based on if DSP LUT EN bit is set for particular video port. eg:

If the userspace want to set gamma for CRTCx,  then that is indeed where they want to set the
gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.

I think there are two options：
（1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
  (2)  disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.

[1]https://github.com/armbian/linux-rockchip/blob/rk3576-6.1-dev-2024_04_19/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3666


>```
>static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
>{
>	struct vop2_video_port *vp;
>	int i;
>	for (i = 0; i < vop2->data->nr_vps; i++) {
>		vp = &vop2->vps[i];
>
>		if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
>			return vp;
>		}
>	}
>	return NULL;
>}
>
>(...)
>
>struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);
>
>if (active_vp) {
>	vop2_vp_dsp_lut_disable(active_vp);
>	vop2_cfg_done(active_vp);
>	if (!vop2_vp_dsp_lut_poll_disable(active_vp))
>		return;
>}
>
>vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
>vop2_crtc_write_gamma_lut(vop2, crtc);
>vop2_vp_dsp_lut_enable(vp);
>```
>
>
>> > > > 
>> > > > 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.
>> > 
>> > Thanks. I didn't know, I will amend it.
>> 
>> 
>> See bdc48fa11e46("checkpatch/coding-style: deprecate 80-column warning")
>> 
>
>Interesting.
>
>Best regards, Piotr Zalewski

Andy Yan Oct. 16, 2024, 1:26 a.m. UTC | #6

Hi Piotr,

At 2024-10-15 06:30:27, "Piotr Zalewski" <[e-mail]> 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/[e-mail]/
>[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 <[e-mail]>
>Helped-by: Dragan Simic <[e-mail]>
>Helped-by: Diederik de Haas <[e-mail]>
>Helped-by: Andy Yan <[e-mail]>
>Signed-off-by: Piotr Zalewski <[e-mail]>
>---
>
>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/[e-mail]/
>    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/[e-mail]/T/#t
>    
>    [1] https://lore.kernel.org/linux-rockchip/[e-mail]
>    [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=[e-mail]
>    [3] https://lore.kernel.org/linux-rockchip/[e-mail]
>
> 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,
>+					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)) {
>+			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 {
>+			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);
> }
> 
> 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) {


There is no need to check vp->crtc.dev here,  set has been set in drm_crtc_init_with_planes .

Piotr Zalewski Oct. 16, 2024, 7:41 a.m. UTC | #7

Hi Andy,

On Wednesday, October 16th, 2024 at 3:10 AM, Andy Yan <[e-mail]> wrote:

> Hi Piotr,
> 
> At 2024-10-16 04:13:40, "Piotr Zalewski" [e-mail] wrote:
> 
> > Hi Andy
> > 
> > On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan [e-mail] wrote:
> > 
> > > > > > + 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.
> > > > 
> > > > I will change in the next version.
> > > > 
> > > > > > + 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:
> > > > 
> > > > What do you mean exactly by exclusive check in this case.It's true that
> > > > gamma LUT is shared across video ports in rk356x but, if I correctly
> > > > understand, this doesn't forbid to reprogram LUT port sel and allow other
> > > > VP to use gamma LUT.
> > > 
> > > Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
> > > want reprogram LUT port sel form VPx to VPy.
> > 
> > Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
> > being set, instead of disabling the LUT before the gamma LUT write for the
> > current CRTC's video port, active video port is selected. Selection is
> > based on if DSP LUT EN bit is set for particular video port. eg:
> 
> 
> If the userspace want to set gamma for CRTCx, then that is indeed where they want to set the
> gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.

Hello maybe there is some misunderstanding, here is full version of my rework
```

> I think there are two options：
> （1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
> (2) disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.
> 
> [1]https://github.com/armbian/linux-rockchip/blob/rk3576-6.1-dev-2024_04_19/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3666

Hmm ok in my solution I supposedo

> > ```
> > static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
> > {
> > struct vop2_video_port *vp;
> > int i;
> > for (i = 0; i < vop2->data->nr_vps; i++) {
> > vp = &vop2->vps[i];
> > 
> > if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
> > return vp;
> > }
> > }
> > return NULL;
> > }
> > 
> > (...)
> > 
> > struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);
> > 
> > if (active_vp) {
> > vop2_vp_dsp_lut_disable(active_vp);
> > vop2_cfg_done(active_vp);
> > if (!vop2_vp_dsp_lut_poll_disable(active_vp))
> > return;
> > }
> > 
> > vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
> > vop2_crtc_write_gamma_lut(vop2, crtc);
> > vop2_vp_dsp_lut_enable(vp);
> > ```

Best Regards, Piotr Zalewski

Piotr Zalewski Oct. 16, 2024, 7:43 a.m. UTC | #8

Hi Andy, Ignore the message I'm answering to. I accidentally sent it prematurely. 

On Wednesday, October 16th, 2024 at 9:41 AM, Piotr Zalewski <[e-mail]> wrote:

> Hi Andy,
> 
> On Wednesday, October 16th, 2024 at 3:10 AM, Andy Yan [e-mail] wrote:
> 
> > Hi Piotr,
> > 
> > At 2024-10-16 04:13:40, "Piotr Zalewski" [e-mail] wrote:
> > 
> > > Hi Andy
> > > 
> > > On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan [e-mail] wrote:
> > > 
> > > > > > > + 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.
> > > > > 
> > > > > I will change in the next version.
> > > > > 
> > > > > > > + 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:
> > > > > 
> > > > > What do you mean exactly by exclusive check in this case.It's true that
> > > > > gamma LUT is shared across video ports in rk356x but, if I correctly
> > > > > understand, this doesn't forbid to reprogram LUT port sel and allow other
> > > > > VP to use gamma LUT.
> > > > 
> > > > Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
> > > > want reprogram LUT port sel form VPx to VPy.
> > > 
> > > Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
> > > being set, instead of disabling the LUT before the gamma LUT write for the
> > > current CRTC's video port, active video port is selected. Selection is
> > > based on if DSP LUT EN bit is set for particular video port. eg:
> > 
> > If the userspace want to set gamma for CRTCx, then that is indeed where they want to set the
> > gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.
> 
> 
> Hello maybe there is some misunderstanding, here is full version of my rework
> ```
> 
> > I think there are two options：
> > （1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
> > (2) disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.
> > 
> > [1]https://github.com/armbian/linux-rockchip/blob/rk3576-6.1-dev-2024_04_19/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3666
> 
> 
> Hmm ok in my solution I supposedo
> 
> > > ```
> > > static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
> > > {
> > > struct vop2_video_port *vp;
> > > int i;
> > > for (i = 0; i < vop2->data->nr_vps; i++) {
> > > vp = &vop2->vps[i];
> > > 
> > > if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
> > > return vp;
> > > }
> > > }
> > > return NULL;
> > > }
> > > 
> > > (...)
> > > 
> > > struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);
> > > 
> > > if (active_vp) {
> > > vop2_vp_dsp_lut_disable(active_vp);
> > > vop2_cfg_done(active_vp);
> > > if (!vop2_vp_dsp_lut_poll_disable(active_vp))
> > > return;
> > > }
> > > 
> > > vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
> > > vop2_crtc_write_gamma_lut(vop2, crtc);
> > > vop2_vp_dsp_lut_enable(vp);
> > > ```
> 
> 
> Best Regards, Piotr Zalewski

Piotr Zalewski Oct. 16, 2024, 9:16 a.m. UTC | #9

Hi Andy,

On Wednesday, October 16th, 2024 at 3:10 AM, Andy Yan <[e-mail]> wrote:

> Hi Piotr,
> 
> At 2024-10-16 04:13:40, "Piotr Zalewski" [e-mail] wrote:
> 
> > Hi Andy
> > 
> > On Tuesday, October 15th, 2024 at 2:22 PM, Andy Yan [e-mail] wrote:
> > 
> > > > > > + 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.
> > > > 
> > > > I will change in the next version.
> > > > 
> > > > > > + 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:
> > > > 
> > > > What do you mean exactly by exclusive check in this case.It's true that
> > > > gamma LUT is shared across video ports in rk356x but, if I correctly
> > > > understand, this doesn't forbid to reprogram LUT port sel and allow other
> > > > VP to use gamma LUT.
> > > 
> > > Yes, we can reprogram LUT port sel, but we need to make sure the the dsp_lut_en bit in VPx is cleared if we
> > > want reprogram LUT port sel form VPx to VPy.
> > 
> > Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
> > being set, instead of disabling the LUT before the gamma LUT write for the
> > current CRTC's video port, active video port is selected. Selection is
> > based on if DSP LUT EN bit is set for particular video port. eg:
> 
> 
> If the userspace want to set gamma for CRTCx, then that is indeed where they want to set the
> gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.
> 
> I think there are two options：
> （1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
> (2) disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.
> 
> [1]https://github.com/armbian/linux-rockchip/blob/rk3576-6.1-dev-2024_04_19/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3666

When I think about it, userspace tools set gamma LUT for all
active CRTCs with gamma LUT support[2] so essentially when there is only 
one which can be active implicitly (like in the case of rk356x) in BSP code 
you provided[1], it will go with the first one and in the second approach 
it will end up setting second one in the end. I think this 
solution[1] is better if assumed that first CRTC is always "main" one.

I will rework it to[1] and test it. (Have to check if hdmi out on pt2 works).

[2] https://gitlab.com/chinstrap/gammastep/-/blob/master/src/gamma-drm.c?ref_type=heads#L261

> > ```
> > static struct vop2_video_port *vop2_vp_dsp_lut_get_active_vp(struct vop2 *vop2)
> > {
> > struct vop2_video_port *vp;
> > int i;
> > for (i = 0; i < vop2->data->nr_vps; i++) {
> > vp = &vop2->vps[i];
> > 
> > if (vp->crtc.dev != NULL && vop2_vp_dsp_lut_is_enabled(vp)) {
> > return vp;
> > }
> > }
> > return NULL;
> > }
> > 
> > (...)
> > 
> > struct vop2_video_port *active_vp = vop2_vp_dsp_lut_get_active_vp(vop2);
> > 
> > if (active_vp) {
> > vop2_vp_dsp_lut_disable(active_vp);
> > vop2_cfg_done(active_vp);
> > if (!vop2_vp_dsp_lut_poll_disable(active_vp))
> > return;
> > }
> > 
> > vop2_writel(vop2, RK3568_LUT_PORT_SEL, vp->id);
> > vop2_crtc_write_gamma_lut(vop2, crtc);
> > vop2_vp_dsp_lut_enable(vp);
> > ```

Best regards, Piotr Zalewski

Diederik de Haas Oct. 16, 2024, 9:20 a.m. UTC | #10

On Wed Oct 16, 2024 at 11:16 AM CEST, Piotr Zalewski wrote:
> I will rework it to[1] and test it. (Have to check if hdmi out on pt2 works).

Last time I tried it, hdmi out did work.

Piotr Zalewski Oct. 16, 2024, 9:23 a.m. UTC | #11

Hi Andy, 

> > 
> > 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) {
> 
> 
> 
> There is no need to check vp->crtc.dev here, set has been set in drm_crtc_init_with_planes .

I tested it and this check must be here because trying to enable color mgmt 
on CRTC with no associated device, causes null ptr dereference later on in 
drm_crtc_enable_color_mgmt (unless something changed in recent kernel 
versions, last time I tested was 6.10? or even 6.8).

Best regards, Piotr Zalewski

Piotr Zalewski Oct. 16, 2024, 9:24 a.m. UTC | #12

Hi Diederik

On Wednesday, October 16th, 2024 at 11:20 AM, Diederik de Haas <[e-mail]> wrote:

> On Wed Oct 16, 2024 at 11:16 AM CEST, Piotr Zalewski wrote:
> 
> > I will rework it to[1] and test it. (Have to check if hdmi out on pt2 works).
> 
> 
> Last time I tried it, hdmi out did work.

Nice, thanks for the info :)

Daniel Stone Oct. 16, 2024, 12:27 p.m. UTC | #13

Hi all,

On Wed, 16 Oct 2024 at 02:11, Andy Yan <[e-mail]> wrote:
> At 2024-10-16 04:13:40, "Piotr Zalewski" <[e-mail]> wrote:
> >Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
> >being set, instead of disabling the LUT before the gamma LUT write for the
> >current CRTC's video port, active video port is selected. Selection is
> >based on if DSP LUT EN bit is set for particular video port. eg:
>
> If the userspace want to set gamma for CRTCx,  then that is indeed where they want to set the
> gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.
>
> I think there are two options：
> （1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
>   (2)  disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.

1 is the only solution that can work. Silently changing the colour
properties of a separate CRTC is not OK, since this can lead to
displaying incorrect content.

Cheers,
Daniel

Piotr Zalewski Oct. 16, 2024, 7:19 p.m. UTC | #14

Hi Daniel,

On Wednesday, October 16th, 2024 at 2:27 PM, Daniel Stone <[e-mail]> wrote:

> Hi all,
>
> On Wed, 16 Oct 2024 at 02:11, Andy Yan [e-mail] wrote:
>
> > At 2024-10-16 04:13:40, "Piotr Zalewski" [e-mail] wrote:
> >
> > > Ok I get it now. Is such rework correct? - when gamma LUT for rk356x is
> > > being set, instead of disabling the LUT before the gamma LUT write for the
> > > current CRTC's video port, active video port is selected. Selection is
> > > based on if DSP LUT EN bit is set for particular video port. eg:
> >
> > If the userspace want to set gamma for CRTCx, then that is indeed where they want to set the
> > gamma on。The driver silently sets the gamma on another CRTC, which is not what the user wants.
> >
> > I think there are two options：
> > （1）return a error if gamma is enable on other CRTC， this is what we done in our BSP code[1]
> > (2) disable the dsp_lut on privious CRTC, then switch to the current CRTC which userspace wants.
>
>
> 1 is the only solution that can work. Silently changing the colour
> properties of a separate CRTC is not OK, since this can lead to
> displaying incorrect content.

Ok right kernel keeps track of the state and sees gamma as enabled even if 
dsp lut en bit was cleared.

Would it be better to check if gamma is already enabled on another CRTC in 
atomic_check rather than atomic_begin/atomic_flush (and silently fail) like
in[1]?

[1] https://github.com/armbian/linux-rockchip/blob/rk3576-6.1-dev-2024_04_19/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3666

Best regards, Piotr Zalewski

Daniel Stone Oct. 16, 2024, 8:17 p.m. UTC | #15

Hi Piotr,

On Wed, 16 Oct 2024 at 20:19, Piotr Zalewski <[e-mail]> wrote:
> On Wednesday, October 16th, 2024 at 2:27 PM, Daniel Stone <[e-mail]> wrote:
> > 1 is the only solution that can work. Silently changing the colour
> > properties of a separate CRTC is not OK, since this can lead to
> > displaying incorrect content.
>
> Ok right kernel keeps track of the state and sees gamma as enabled even if
> dsp lut en bit was cleared.
>
> Would it be better to check if gamma is already enabled on another CRTC in
> atomic_check rather than atomic_begin/atomic_flush (and silently fail) like
> in[1]?

Yes please, that's exactly it. If userspace requests something that
can't be done, then fail the request.

Cheers,
Daniel

Piotr Zalewski Oct. 16, 2024, 10:04 p.m. UTC | #16

Hi Andy

On Wednesday, October 16th, 2024 at 11:23 AM, Piotr Zalewski <[e-mail]> wrote:

> > > 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) {
> > 
> > There is no need to check vp->crtc.dev here, set has been set in drm_crtc_init_with_planes .
> 
> 
> I tested it and this check must be here because trying to enable color mgmt
> on CRTC with no associated device, causes null ptr dereference later on in
> drm_crtc_enable_color_mgmt (unless something changed in recent kernel
> versions, last time I tested was 6.10? or even 6.8).

Just tested it without the null check and it works. Next version will be without it.

Best regards, Piotr Zalewski

Patch

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,
+					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)) {
+			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 {
+			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);
 }
 
 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);
+		}
 		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)