Hi Piotr, 在 2024-10-28 14:42:28，"Andy Yan" <andys...@163.com> 写道： > >Hi Piotr， > Tested on top of Linux 6.12-rc5 with rk3566-box-demo in a buildroot + > weston environment: > weston --backend=drm-backend.so -i 0 --continue-without-input > weston-simple-egl > > simple-egl will draw a triangle on the desktop. > After the patch applied，the triangle will flicker again and agian。 > So it break some function。 > I've been quite busy lately, and it will take some time before I can > analyze what the specific reason is.
Sorry, it's my fault. After more experiments, I found that the flickering I observed is not related to your patch. This should be a probability-related issue associated with alpha blending，which can be Fixed with patches in my series[0] [0] https://lore.kernel.org/linux-rockchip/75a13dbb.acb8.192289a8005.coremail.andys...@163.com/#r And for the gamma patch， I think there are still some small areas that can be improved. Please see my comment bellow: > > >At 2024-10-27 04:47:42, "Piotr Zalewski" <pz010001011...@proton.me> wrote: >>Hi all, >> >>any comments on this? >> >>On Thursday, October 17th, 2024 at 12:36 AM, Piotr Zalewski >><pz010001011...@proton.me> wrote: >> >>> Add support for gamma LUT in VOP2 driver. The implementation was inspired >>> by one found in VOP1 driver. Blue and red channels in gamma LUT register >>> write were swapped with respect to how gamma LUT values are written in >>> VOP1. Gamma LUT port selection was added before the write of new gamma LUT >>> table. >>> >>> If the current SoC is rk356x, check if no other CRTC has gamma LUT enabled >>> in atomic_check (only one video port can use gamma LUT at a time) and >>> disable gamma LUT before the LUT table write. >>> >>> If the current SoC isn't rk356x, "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, check if no >>> other CRTC has gamma LUT enabled is also not necessary, different register >>> is being used to select gamma LUT port[2] and after setting DSP_LUT_EN bit, >>> GAMMA_UPDATE_EN bit is set[3]. >>> >>> Gamma size is set and drm color management is enabled for each video port's >>> CRTC except ones which have no associated device. >>> >>> Solution was tested on RK3566 (Pinetab2). When using userspace tools >>> which set eg. constant color temperature no issues were noticed. When >>> using userspace tools which adjust eg. color temperature the slight screen >>> flicker is visible probably because of gamma LUT disable. >>> >>> Compare behaviour of eg.: >>> `gammastep -O 3000` >>> >>> To eg.: >>> `gammastep -l 53:23 -t 6000:3000` >>> >>> In latter case color temperature is slowly adjusted at the beginning which >>> causes screen to slighly flicker. Then it adjusts every few seconds which >>> also causes slight screen flicker. >>> >>> [1] >>> https://lists.infradead.org/pipermail/linux-rockchip/2021-October/028132.html >>> [2] >>> https://lore.kernel.org/linux-rockchip/48249708-8c05-40d2-a5d8-23de960c5...@rock-chips.com/ >>> [3] >>> https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3437 >>> >>> Helped-by: Daniel Stone dan...@fooishbar.org >>> >>> Helped-by: Dragan Simic dsi...@manjaro.org >>> >>> Helped-by: Diederik de Haas didi.deb...@cknow.org >>> >>> Helped-by: Andy Yan andy....@rock-chips.com >>> >>> Signed-off-by: Piotr Zalewski pz010001011...@proton.me >>> >>> --- >>> >>> Notes: >>> Changes in v6: >>> - Move gamma lut write to atomic_flush[4]. >>> - In atomic_check if any other than the currently updated CRTC has >>> gamma lut enabled, return -EINVAL [5] (perform a check only if >>> device is rk356x). >>> - Instead of checking for rk3588 to determine seamless gamma >>> update availability check for rk3566/rk3568. >>> - remove null check in vop2_create_crtcs >>> - Move some code to separate functions to increase readability. >>> >>> 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 v5: >>> https://lore.kernel.org/linux-rockchip/20241014222022.571819-4-pz010001011...@proton.me/ >>> Link to v4: >>> https://lore.kernel.org/linux-rockchip/20240815124306.189282-2-pz010001011...@proton.me/ >>> Link to v3: >>> https://lore.kernel.org/linux-rockchip/TkgKVivuaLFLILPY-n3iZo_8KF-daKdqdu-0_e0HP-5Ar_8DALDeNWog2suwWKjX7eomcbGET0KZe7DlzdhK2YM6CbLbeKeFZr-MJzJMtw0=@proton.me/ >>> Link to v2: >>> https://lore.kernel.org/linux-rockchip/Hk03HDb6wSSHWtEFZHUye06HR0-9YzP5nCHx9A8_kHzWSZawDrU1o1pjEGkCOJFoRg0nTB4BWEv6V0XBOjF4-0Mj44lp2TrjaQfnytzp-Pk=@proton.me/ >>> Link to v1: >>> https://lore.kernel.org/linux-rockchip/ZVMxgcrtgHui9fJpnhbN6TSPhofHbbXElh241lImrzzTUl-8WejGpaR8CPzYhBgoqe_xj7N6En8Ny7Z-gsCr0kaFs7apwjYV1MBJJLmLHxs=@proton.me/ >>> >>> [1] >>> https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f...@manjaro.org >>> [2] >>> https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=q9ip7f_hapynbcqp...@mail.gmail.com >>> [3] >>> https://lore.kernel.org/linux-rockchip/7d998e4c-e1d3-4e8b-af76-c5bc83b43...@rock-chips.com >>> [4] >>> https://lore.kernel.org/linux-rockchip/7b45f190.452f.1928e41b746.coremail.andys...@163.com/ >>> [5] >>> https://lore.kernel.org/linux-rockchip/CAPj87rOdQPsuH9qB_ZLfC9S=cO2noNi1mOGW0ZmQ6SHCugb9=w...@mail.gmail.com/ >>> >>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 189 +++++++++++++++++++ >>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 5 + >>> 2 files changed, 194 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c >>> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c >>> index 9873172e3fd3..6018c353e66b 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,65 @@ 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; >>> +} It's better to write like thise to avoid too long one line: u32 val = (vop2_vp_read(vp, RK3568_VP_DSP_CTRL) ; return val & RK3568_VP_DSP_CTRL__DSP_LUT_EN; >>> + >>> +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) op2_vp_dsp_lut_poll_disabled ? >>> +{ >>> + 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 != 3566 && vop2->data->soc_id != 3568); >>> >>> +} >>> + >>> +static bool vop2_gamma_lut_in_use(struct vop2 *vop2, struct >>> vop2_video_port *vp) >>> +{ >>> + const int nr_vps = vop2->data->nr_vps; >>> >>> + int gamma_en_vp_id; We need a blank line after declaration. >>> + for (gamma_en_vp_id = 0; gamma_en_vp_id < nr_vps; gamma_en_vp_id++) >>> + if (vop2_vp_dsp_lut_is_enabled(&vop2->vps[gamma_en_vp_id])) >>> >>> + break; >>> + >>> + return gamma_en_vp_id != nr_vps && gamma_en_vp_id != vp->id; >>> >>> +} >>> + >>> static void vop2_crtc_atomic_disable(struct drm_crtc *crtc, >>> struct drm_atomic_state *state) >>> { >>> @@ -1482,6 +1550,80 @@ 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]; >>> >>> + This blank line seems not needed. >>> + 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_set_gamma_seamless(struct vop2 *vop2, >>> + struct vop2_video_port *vp, >>> + struct drm_crtc *crtc) >>> +{ >>> + vop2_writel(vop2, RK3568_LUT_PORT_SEL, FIELD_PREP( >>> + RK3588_LUT_PORT_SEL__GAMMA_AHB_WRITE_SEL, >>> + vp->id)); >>> >>> + vop2_vp_dsp_lut_enable(vp); >>> + vop2_crtc_write_gamma_lut(vop2, crtc); >>> + vop2_vp_dsp_lut_update_enable(vp); >>> +} >>> + >>> +static void vop2_crtc_atomic_set_gamma_rk356x(struct vop2 *vop2, >>> + struct vop2_video_port *vp, >>> + struct drm_crtc *crtc) >>> +{ >>> + 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 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) >>> >>> + return; >>> + >>> + if (!crtc_state->gamma_lut) { >>> >>> + vop2_vp_dsp_lut_disable(vp); >>> + return; >>> + } >>> + >>> + if (vop2_supports_seamless_gamma_lut_update(vop2)) >>> + vop2_crtc_atomic_set_gamma_seamless(vop2, vp, crtc); >>> + else >>> + vop2_crtc_atomic_set_gamma_rk356x(vop2, vp, crtc); >>> +} >>> + >>> +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 +2199,40 @@ 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; >>> + } >>> + >>> + if (!vop2_supports_seamless_gamma_lut_update(vop2) && >>> vop2_gamma_lut_in_use(vop2, vp)) { >>> + drm_info(vop2->drm, "Gamma LUT can be enabled for only one CRTC at a >>> time\n"); >>> >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int vop2_crtc_atomic_check(struct drm_crtc *crtc, >>> struct drm_atomic_state *state) >>> { >>> @@ -2069,6 +2240,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,6 +2632,7 @@ 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) >>> { >>> @@ -2487,7 +2664,14 @@ static void vop2_crtc_atomic_begin(struct drm_crtc >>> *crtc, >>> static void vop2_crtc_atomic_flush(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; >>> >>> + >>> + // 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); >>> >>> vop2_post_config(crtc); >>> >>> @@ -2790,7 +2974,12 @@ static int vop2_create_crtcs(struct vop2 *vop2) >>> } >>> >>> drm_crtc_helper_add(&vp->crtc, &vop2_crtc_helper_funcs); >>> >>> + if (vop2->lut_regs) { >>> >>> + 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) >>> >>> -- >>> 2.47.0
