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


Hi Piotr,

在 2024-10-28 14:42:28，"Andy Yan" <andyshrk@....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.andyshrk@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" <pZ010001011111@...ton.me> wrote:
>>Hi all,
>>
>>any comments on this?
>>
>>On Thursday, October 17th, 2024 at 12:36 AM, Piotr Zalewski <pZ010001011111@...ton.me> wrote:
>>
>>> Add support for gamma LUT in VOP2 driver. The implementation was inspired
>>> by one found in VOP1 driver. Blue and red channels in gamma LUT register
>>> write were swapped with respect to how gamma LUT values are written in
>>> VOP1. Gamma LUT port selection was added before the write of new gamma LUT
>>> table.
>>> 
>>> If the current SoC is 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-23de960c5a77@rock-chips.com/
>>> [3] https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3437
>>> 
>>> Helped-by: Daniel Stone daniel@...ishbar.org
>>> 
>>> Helped-by: Dragan Simic dsimic@...jaro.org
>>> 
>>> Helped-by: Diederik de Haas didi.debian@...ow.org
>>> 
>>> Helped-by: Andy Yan andy.yan@...k-chips.com
>>> 
>>> Signed-off-by: Piotr Zalewski pZ010001011111@...ton.me
>>> 
>>> ---
>>> 
>>> Notes:
>>> Changes in 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-pZ010001011111@proton.me/
>>> Link to v4: https://lore.kernel.org/linux-rockchip/20240815124306.189282-2-pZ010001011111@proton.me/
>>> Link to v3: https://lore.kernel.org/linux-rockchip/TkgKVivuaLFLILPY-n3iZo_8KF-daKdqdu-0_e0HP-5Ar_8DALDeNWog2suwWKjX7eomcbGET0KZe7DlzdhK2YM6CbLbeKeFZr-MJzJMtw0=@proton.me/
>>> Link to v2: https://lore.kernel.org/linux-rockchip/Hk03HDb6wSSHWtEFZHUye06HR0-9YzP5nCHx9A8_kHzWSZawDrU1o1pjEGkCOJFoRg0nTB4BWEv6V0XBOjF4-0Mj44lp2TrjaQfnytzp-Pk=@proton.me/
>>> Link to v1: https://lore.kernel.org/linux-rockchip/ZVMxgcrtgHui9fJpnhbN6TSPhofHbbXElh241lImrzzTUl-8WejGpaR8CPzYhBgoqe_xj7N6En8Ny7Z-gsCr0kaFs7apwjYV1MBJJLmLHxs=@proton.me/
>>> 
>>> [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@manjaro.org
>>> [2] https://lore.kernel.org/linux-rockchip/CAPj87rOM=j0zmuWL9frGKV1xzPbJrk=Q9ip7F_HAPYnbCqPouw@mail.gmail.com
>>> [3] https://lore.kernel.org/linux-rockchip/7d998e4c-e1d3-4e8b-af76-c5bc83b43647@rock-chips.com
>>> [4] https://lore.kernel.org/linux-rockchip/7b45f190.452f.1928e41b746.Coremail.andyshrk@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

Powered by blists - more mailing lists