Re: [PATCH v3] rockchip/drm: vop2: add support for gamma LUT
From: Andy Yan
Date: Sun Jul 28 2024 - 22:44:12 EST
Hi Piotr,
On 7/27/24 03:49, Piotr Zalewski wrote:
On Friday, July 26th, 2024 at 10:55 AM, Andy Yan <andy.yan@xxxxxxxxxxxxxx> wrote:
Hi Piotr
Hi Andy!
Thanks for your work.
On 7/25/24 06:05, Piotr Zalewski wrote:
Add support for gamma LUT in VOP2 driver. The implementation is based on
the one found in VOP driver and modified to be compatible with VOP2. Blue
and red channels in gamma LUT register write were swapped with respect to
how gamma LUT values are written in VOP. Write of the current video port id
to VOP2_SYS_LUT_PORT_SEL register was added before the write of DSP_LUT_EN
bit. Gamma size is set and drm color management is enabled for each video
port's CRTC except ones which have no associated device. Tested on RK3566
(Pinetab2).
Helped-by: Dragan Simic dsimic@xxxxxxxxxxx
Signed-off-by: Piotr Zalewski pZ010001011111@xxxxxxxxx
---
Notes:
Changes in v3:
- v3 is patch v2 "resend", by mistake the incremental patch were
sent in v2
Changes in v2:
- Apply code styling corrections [1]
- Move gamma LUT write inside the vop2 lock
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@xxxxxxxxxxx/T/#t
[1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@xxxxxxxxxxx/
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9873172e3fd3..37fcf544a5fd 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);
@@ -1482,6 +1491,95 @@ static bool vop2_crtc_mode_fixup(struct drm_crtc *crtc,
return true;
}
+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_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_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 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_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc,
+ struct drm_crtc_state *old_state)
+{
+ struct drm_crtc_state *state = crtc->state;
+ struct vop2_video_port *vp = to_vop2_video_port(crtc);
+ u32 dsp_ctrl;
+ int ret;
+
+ if (!vop2->lut_regs)
+ return;
+
+ if (!state->gamma_lut) {
What's the purpose of checking !state->gamma_lut here,
and you check it again at the end for return.
This makes me very confused.
I understood it this way - since the vop2 lock is unlocked after disabling
gamma LUT, the CRTC state can change while waiting for DSP_LUT_EN bit to
be unset. With the change I sent in response to Daniel's reply, gamma LUT
state modification should now be fully atomic so there shouldn't be a need
for the second state check there anymore (if my logic is incorrect please
explain).
After reading the commit message for adding gamma control for rk3399[0] i understand
what is going on here:
we should run into the if block in two cases:
(1) if the state->gamma_lut is null, we just need to disable dsp_lut and return,
this is why vop1 code check !state->gamma_lut again at the end.
(2) for platform unlinke rk3399(rk3566/8), we also need to disable dsp_lut befor we
write gamma_lut data, for platform like rk3399(rk3588), we don't need do the disable,
this is why vop1 code also has a !VOP_HAS_REG(vop, common, update_gamma_lut) check.
so we also need a similary check here:
(1) if the state->gamma_lut is null, disable dsp lut and return directly.
(1) if the state has a gamma_lut, we shoud dsiable dsp_lut than write gamma lut data on rk3566/8,
buf for rk3588, we should not disable dsp_lut before write gamma
[0]https://lists.infradead.org/pipermail/linux-rockchip/2021-October/028132.html
+ /*
+ * To disable gamma (gamma_lut is null) or to write
+ * an update to the LUT, clear dsp_lut_en.
+ /
+ vop2_lock(vop2);
+
+ vop2_vp_dsp_lut_disable(vp);
+
+ vop2_cfg_done(vp);
+ vop2_unlock(vop2);
+ /
+ * In order to write the LUT to the internal memory,
+ * we need to first make sure the dsp_lut_en bit is cleared.
+ */
+ ret = readx_poll_timeout(vop2_vp_dsp_lut_is_enabled, vp, dsp_ctrl,
+ !dsp_ctrl, 5, 30 * 1000);
+
+ if (ret) {
+ DRM_DEV_ERROR(vop2->dev, "display LUT RAM enable timeout!\n");
+ return;
+ }
+
+ if (!state->gamma_lut)
+ return;
+ }
+
+
+ vop2_lock(vop2);
+ vop2_crtc_write_gamma_lut(vop2, crtc);
+ vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id);
We should select lut_port befor write gamma.
And there is one thing to note here is that:
There is only one gamma for rk3568/rk3566, that means only one
of the three VPs can exclusively use it at a time, so we need
a exclusive check for rk3568/rk3566.
For rk3588 and the new soc, there is no such limination.
The another thing to note is that for rk35588 and other new soc, no need to disable dsp_lut before write it.
Thank you for the clarification! I will change my patch according to these
facts.
+
+ vop2_vp_dsp_lut_enable(vp);
+
+ vop2_cfg_done(vp);
+ 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);
@@ -1925,6 +2023,7 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
const struct vop2_data *vop2_data = vop2->data;
const struct vop2_video_port_data *vp_data = &vop2_data->vp[vp->id];
struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ struct drm_crtc_state *old_state = drm_atomic_get_old_crtc_state(state, crtc);
struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
struct drm_display_mode *mode = &crtc->state->adjusted_mode;
unsigned long clock = mode->crtc_clock * 1000;
@@ -2060,6 +2159,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
drm_crtc_vblank_on(crtc);
vop2_unlock(vop2);
+
+ if (crtc->state->gamma_lut)
+ vop2_crtc_gamma_set(vop2, crtc, old_state);
}
static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
@@ -2070,6 +2172,16 @@ static int vop2_crtc_atomic_check(struct drm_crtc *crtc,
int nplanes = 0;
struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ if (vp->vop2->lut_regs && crtc_state->color_mgmt_changed && crtc_state->gamma_lut) {
+ unsigned int len = drm_color_lut_size(crtc_state->gamma_lut);
+
+ if (len != crtc->gamma_size) {
+ DRM_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n",
+ len, crtc->gamma_size);
+ return -EINVAL;
+ }
+ }
+
drm_atomic_crtc_state_for_each_plane(plane, crtc_state)
nplanes++;
@@ -2459,6 +2571,10 @@ static void vop2_setup_dly_for_windows(struct vop2 *vop2)
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 drm_crtc_state *old_crtc_state = drm_atomic_get_old_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 +2598,9 @@ 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);
+
+ if (crtc_state->color_mgmt_changed && !crtc_state->active_changed)
+ vop2_crtc_gamma_set(vop2, crtc, old_crtc_state);
}
static void vop2_crtc_atomic_flush(struct drm_crtc *crtc,
@@ -2791,6 +2910,14 @@ 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..3a58b73fa876 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)
Best regards, Piotr Zalewski