| Message ID | TkgKVivuaLFLILPY-n3iZo_8KF-daKdqdu-0_e0HP-5Ar_8DALDeNWog2suwWKjX7eomcbGET0KZe7DlzdhK2YM6CbLbeKeFZr-MJzJMtw0=@proton.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] rockchip/drm: vop2: add support for gamma LUT | expand |
Hi Piotr, On Wed, 24 Jul 2024 at 23:06, Piotr Zalewski <pZ010001011111@proton.me> 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). Thanks a lot for doing this! > +static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc, > + struct drm_crtc_state *old_state) > +{ > + [...] > + > + vop2_lock(vop2); > + vop2_crtc_write_gamma_lut(vop2, crtc); > + vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id); > + > + vop2_vp_dsp_lut_enable(vp); > + > + vop2_cfg_done(vp); > + vop2_unlock(vop2); > +} > > @@ -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); > } In the atomic_enable callback, we are already holding the VOP lock, and waiting to set cfg_done etc - we then do it all over again here. This should all be atomic, so that we configure the LUT whilst doing the setup, and then only call cfg_done once, to avoid showing the user intermediate states which only later converge on the desired final state. Cheers, Daniel
On Thursday, July 25th, 2024 at 12:28 PM, Daniel Stone <daniel@fooishbar.org> wrote: > Hi Piotr, Hi, Daniel! Thank you for the review. > > In the atomic_enable callback, we are already holding the VOP lock, > and waiting to set cfg_done etc - we then do it all over again here. > This should all be atomic, so that we configure the LUT whilst doing > the setup, and then only call cfg_done once, to avoid showing the user > intermediate states which only later converge on the desired final > state. > I based my patch on how gamma LUT is handled in VOP. There, in atomic enable, gamma LUT write takes places at the end too, after the mutex was already first-time unlocked. I understand the concept of DRM atomic state updates and what you wrote makes sense. Below is what I came up with to make it fulfill atomicity requirement. Frankly, the code ended up simpler. I tested it on RK3566 (Pinetab2). Let me know what do you think. diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c index 37fcf544a5fd..cba92239dcbc 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c @@ -1497,22 +1497,6 @@ static bool vop2_vp_dsp_lut_is_enabled(struct vop2_video_port *vp) 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); @@ -1532,11 +1516,12 @@ static void vop2_crtc_write_gamma_lut(struct vop2 *vop2, struct drm_crtc *crtc) } static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc, - struct drm_crtc_state *old_state) + struct drm_crtc_state *old_state, + u32* dsp_ctrl) { struct drm_crtc_state *state = crtc->state; struct vop2_video_port *vp = to_vop2_video_port(crtc); - u32 dsp_ctrl; + u32 _dsp_ctrl; int ret; if (!vop2->lut_regs) @@ -1547,37 +1532,27 @@ static void vop2_crtc_gamma_set(struct vop2 *vop2, struct drm_crtc *crtc, * 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; - } + *dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN; if (!state->gamma_lut) return; } + /* + * 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; + } - vop2_lock(vop2); vop2_crtc_write_gamma_lut(vop2, crtc); vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id); - vop2_vp_dsp_lut_enable(vp); - - vop2_cfg_done(vp); - vop2_unlock(vop2); + *dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN; } static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl) @@ -2152,6 +2127,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc, vop2_post_config(crtc); + if (crtc->state->gamma_lut) + vop2_crtc_gamma_set(vop2, crtc, old_state, &dsp_ctrl); + vop2_cfg_done(vp); vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl); @@ -2160,8 +2138,6 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *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, @@ -2599,8 +2575,17 @@ static void vop2_crtc_atomic_begin(struct drm_crtc *crtc, 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); + if (crtc_state->color_mgmt_changed && !crtc_state->active_changed) { + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);; + + vop2_lock(vop2); + + vop2_crtc_gamma_set(vop2, crtc, old_crtc_state, &dsp_ctrl); + + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl); + vop2_cfg_done(vp); + vop2_unlock(vop2); + } } static void vop2_crtc_atomic_flush(struct drm_crtc *crtc, Best regards, Piotr Zalewski
Hi Piotr,
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@manjaro.org>
> Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>
> ---
>
> 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@manjaro.org/T/#t
>
> [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@manjaro.org/
>
> 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.
> + /*
> + * 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.
> +
> + 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)
Hi Piotr, On Thu, 25 Jul 2024 at 20:06, Piotr Zalewski <pZ010001011111@proton.me> wrote: > I based my patch on how gamma LUT is handled in VOP. There, in atomic > enable, gamma LUT write takes places at the end too, after the mutex was > already first-time unlocked. I understand the concept of DRM atomic state > updates and what you wrote makes sense. Yeah, no problem - the old VOP1 code is clearly incorrect here. I'm glad you can improve VOP2. :) > static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl) > @@ -2152,6 +2127,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc, > > vop2_post_config(crtc); > > + if (crtc->state->gamma_lut) > + vop2_crtc_gamma_set(vop2, crtc, old_state, &dsp_ctrl); I think this call should be unconditional, so that we correctly program LUT_DIS if there is no LUT set up during enable. > @@ -2599,8 +2575,17 @@ static void vop2_crtc_atomic_begin(struct drm_crtc *crtc, > 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); > + if (crtc_state->color_mgmt_changed && !crtc_state->active_changed) { > + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);; > + > + vop2_lock(vop2); > + > + vop2_crtc_gamma_set(vop2, crtc, old_crtc_state, &dsp_ctrl); > + > + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl); > + vop2_cfg_done(vp); > + vop2_unlock(vop2); > + } Calling lock/set/write/done/unlock here seems like an anti-pattern; the cfg_done is already written in atomic_flush, so at least that part is not necessary. On platforms like RK3588, it looks like the new LUT can just be written directly from atomic_begin without needing to program DSP_CTRL, take locks, or synchronise against anything, so that should be an easy straight-line path. On platforms like RK3568, it would probably be better to set mode_changed when the colour management configuration changes. That will give you a good point to synchronise the cross-VOP dependencies (i.e. claim or release the shared LUT when it is being enabled/disabled), and also a hint to userspace that it is not going to be a seamless transition as the LUT is disabled, programmed, then re-enabled. I think this would end up in a net reduction of LoC as well as a net reduction of code weirdness. Cheers, Daniel
On Friday, July 26th, 2024 at 10:55 AM, Andy Yan <andy.yan@rock-chips.com> 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@manjaro.org > > Signed-off-by: Piotr Zalewski pZ010001011111@proton.me > > --- > > > > 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@manjaro.org/T/#t > > > > [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@manjaro.org/ > > > > 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). > > + /* > > + * 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
On Friday, July 26th, 2024 at 1:31 PM, Daniel Stone <daniel@fooishbar.org> wrote: > Hi Piotr, Hi Daniel, sorry for delayed response. > > > static void vop2_dither_setup(struct drm_crtc *crtc, u32 *dsp_ctrl) > > @@ -2152,6 +2127,9 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc, > > > > vop2_post_config(crtc); > > > > + if (crtc->state->gamma_lut) > > + vop2_crtc_gamma_set(vop2, crtc, old_state, &dsp_ctrl); > > > I think this call should be unconditional, so that we correctly > program LUT_DIS if there is no LUT set up during enable. > Noted > > @@ -2599,8 +2575,17 @@ static void vop2_crtc_atomic_begin(struct drm_crtc *crtc, > > 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); > > + if (crtc_state->color_mgmt_changed && !crtc_state->active_changed) { > > + u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);; > > + > > + vop2_lock(vop2); > > + > > + vop2_crtc_gamma_set(vop2, crtc, old_crtc_state, &dsp_ctrl); > > + > > + vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl); > > + vop2_cfg_done(vp); > > + vop2_unlock(vop2); > > + } > > > Calling lock/set/write/done/unlock here seems like an anti-pattern; > the cfg_done is already written in atomic_flush, so at least that part > is not necessary. Right sorry for sending such code. I wanted to demonstrate the idea. > On platforms like RK3588, it looks like the new LUT can just be > written directly from atomic_begin without needing to program > DSP_CTRL, take locks, or synchronise against anything, so that should > be an easy straight-line path. > > On platforms like RK3568, it would probably be better to set > mode_changed when the colour management configuration changes. That > will give you a good point to synchronise the cross-VOP dependencies > (i.e. claim or release the shared LUT when it is being > enabled/disabled), and also a hint to userspace that it is not going > to be a seamless transition as the LUT is disabled, programmed, then > re-enabled. > > I think this would end up in a net reduction of LoC as well as a net > reduction of code weirdness. Okay so if I understood you correctly you suggest setting mode_changed in order to trigger full modeset (check->begin->flush->enable) to cleanly handle the RK356x case and for RK3588 just write the LUT in begin and don't do anything more. I tried to do this but couldn't get the thing to work. It seems like setting the mode_changed manually in atomic_check, messes something up with the CRTC states. Namely, the retrieved new state in subsequent atomic_begin call isn't the same state (as a result, flags color_mgmt_changed and mode_changed are both false when they are checked in atomic_begin which stops further gamma LUT reconfiguration). Below is how I reworked this (included only parts which changed). As already mentioned, in atomic check the mode_changed flag is set, then in atomic begin gamma LUT is disabled and DSP_LUT_EN bit unset (or gamma LUT is written directly based on if it's RK356x or not). Then in atomic_flush video port is selected and gamma LUT is written. Gamma LUT is enabled in atomic_enable. Perhaps I'm missing something important, if so please hint what exactly. diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c index 9873172e3fd3..9c5ee2d85a58 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c @@ -2051,6 +2092,13 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc, clk_set_rate(vp->dclk, clock); + /** + * Enable gamma LUT in atomic enable + */ + if (crtc_state->gamma_lut && (vop2->data->soc_id == 3566 || + vop2->data->soc_id == 3568)) + dsp_ctrl |= RK3568_VP_DSP_CTRL__DSP_LUT_EN; + vop2_post_config(crtc); vop2_cfg_done(vp); @@ -2062,6 +2110,39 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc, vop2_unlock(vop2); } +static int vop2_crtc_atomic_check_gamma(struct vop2_video_port *vp, + struct drm_crtc *crtc, + 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_DEBUG_KMS("Invalid LUT size; got %d, expected %d\n", + len, crtc->gamma_size); + return -EINVAL; + } + + if (!crtc_state->mode_changed && (vop2->data->soc_id == 3566 || + vop2->data->soc_id == 3568)) { + int ret; + + crtc_state->mode_changed = true; + + ret = drm_atomic_helper_check_modeset(crtc->dev, + crtc_state->state); + if (ret) + return ret; + } + + return 0; +} + static int vop2_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_state *state) { @@ -2069,6 +2150,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, crtc_state); + if (ret) + return ret; drm_atomic_crtc_state_for_each_plane(plane, crtc_state) nplanes++; @@ -2456,9 +2542,36 @@ static void vop2_setup_dly_for_windows(struct vop2 *vop2) vop2_writel(vop2, RK3568_SMART_DLY_NUM, sdly); } +static void vop2_crtc_atomic_begin_gamma(struct vop2 *vop2, + struct drm_crtc *crtc) { + struct vop2_video_port *vp = to_vop2_video_port(crtc); + int ret; + u32 dsp_ctrl; + + vop2_lock(vop2); + /** + * Gamma lut always has to be disabled in both cases. When gamma + * lut is enabled it needs to be disabled before writing (Applies + * only to RK356x SoCs). + */ + vop2_vp_dsp_lut_disable(vp); + + vop2_cfg_done(vp); + vop2_unlock(vop2); + + 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"); + } +} + 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,13 +2595,50 @@ 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) { + /** + * When the SoC is RK356x gamma lut change always triggers + * mode_changed - proceed with gamma lut disable + */ + if (crtc_state->mode_changed && + (vop2->data->soc_id == 3566 || + vop2->data->soc_id == 3568)) + vop2_crtc_atomic_begin_gamma(vop2, crtc); + /** + * When the Soc isn't RK356x (eg. RK3588) gamma lut can be + * written directly + */ + else if (crtc_state->gamma_lut) + vop2_crtc_write_gamma_lut(vop2, crtc); + } +} + +static void vop2_crtc_atomic_flush_gamma(struct vop2 *vop2, struct vop2_video_port *vp, struct drm_crtc *crtc) +{ + if (vop2->data->soc_id == 3566 || vop2->data->soc_id == 3568) { + vop2_lock(vop2); + + vop2_crtc_write_gamma_lut(vop2, crtc); + vop2_writel(vp->vop2, RK3568_LUT_PORT_SEL, vp->id); + + vop2_unlock(vop2); + } } 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); + /** + * Write gamma LUT in atomic flush + */ + if (crtc_state->mode_changed && crtc_state->gamma_lut) + vop2_crtc_atomic_flush_gamma(vp->vop2, vp, crtc); + vop2_post_config(crtc); vop2_cfg_done(vp);
Hi Piotr, On 7/27/24 03:49, Piotr Zalewski wrote: > > On Friday, July 26th, 2024 at 10:55 AM, Andy Yan <andy.yan@rock-chips.com> 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@manjaro.org >>> Signed-off-by: Piotr Zalewski pZ010001011111@proton.me >>> --- >>> >>> 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@manjaro.org/T/#t >>> >>> [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@manjaro.org/ >>> >>> 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
Hi Andy On Monday, July 29th, 2024 at 4:35 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > > > > + > > > > +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 > Ok I see it. So In my patch it doesn't make sense at all to check it again (forgot about that extra if statement condition there, which I cut out when porting to VOP2). I reworked my patch further for it to handle RK3588 case and to better utilize DRM atomic updates. It's contained in the response to Daniel's review [1]. I experienced some problems so I'm waiting for his response/comment on that. Regarding RK3588, I checked RK3588 TRM v1.0 part2. In its VOP2 section I found: - SYS_CTRL_LUT_PORT_SEL: gamma_ahb_write_sel (seems to represent the same concept as LUT_PORT_SEL in case of RK356x) - VOP2_POST0_DSP_CTRL: gamma_update_en (seems to represent the same concept as in VOP1 in case of RK3399) - I also found dsp_lut_en but I presume it is a bug in documentation. Should RK3588 be handled as RK3399? (gamma LUT can be written directly but gamma_update_en bit has to be set before). What about gamma_ahb_write_sel? [1] https://lkml.org/lkml/2024/7/27/293 Best Regards, Piotr Zalewski
Hi Piotr, On 7/30/24 05:20, Piotr Zalewski wrote: > Hi Andy > > On Monday, July 29th, 2024 at 4:35 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > >>>>> + >>>>> +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 >> > > Ok I see it. So In my patch it doesn't make sense at all to check it again > (forgot about that extra if statement condition there, which I cut out > when porting to VOP2). I reworked my patch further for it to handle RK3588 > case and to better utilize DRM atomic updates. It's contained in the > response to Daniel's review [1]. I experienced some problems so I'm waiting > for his response/comment on that. > > Regarding RK3588, I checked RK3588 TRM v1.0 part2. In its VOP2 section I > found: > - SYS_CTRL_LUT_PORT_SEL: gamma_ahb_write_sel (seems to represent the > same concept as LUT_PORT_SEL in case of RK356x) We should also setting it to she VP id we want write gamma, this is used for selet ahb bus. > - VOP2_POST0_DSP_CTRL: gamma_update_en (seems to represent the same > concept as in VOP1 in case of RK3399) we also need to set it every time we update the gamma lut. > - I also found dsp_lut_en but I presume it is a bug in documentation. No, it is not a bug, we should set it when we enable gamma lut, we just don't need to disable it before we update gamma lut. Here is some code you can take as reference [0] [0]:https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3437 > > Should RK3588 be handled as RK3399? (gamma LUT can be written directly but > gamma_update_en bit has to be set before). What about gamma_ahb_write_sel? > > [1] https://lkml.org/lkml/2024/7/27/293 > > Best Regards, Piotr Zalewski
On Tuesday, July 30th, 2024 at 12:43 PM, Andy Yan <andy.yan@rock-chips.com> wrote: > Hi Piotr, Hi Andy > On 7/30/24 05:20, Piotr Zalewski wrote: > > > > > On Monday, July 29th, 2024 at 4:35 AM, Andy Yan andy.yan@rock-chips.com wrote: > > > > > > > > + > > > > > > +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 > > > > Ok I see it. So In my patch it doesn't make sense at all to check it again > > (forgot about that extra if statement condition there, which I cut out > > when porting to VOP2). I reworked my patch further for it to handle RK3588 > > case and to better utilize DRM atomic updates. It's contained in the > > response to Daniel's review [1]. I experienced some problems so I'm waiting > > for his response/comment on that. > > > > Regarding RK3588, I checked RK3588 TRM v1.0 part2. In its VOP2 section I > > found: > > - SYS_CTRL_LUT_PORT_SEL: gamma_ahb_write_sel (seems to represent the > > same concept as LUT_PORT_SEL in case of RK356x) > > > We should also setting it to she VP id we want write gamma, this is used for selet > ahb bus. > > > - VOP2_POST0_DSP_CTRL: gamma_update_en (seems to represent the same > > concept as in VOP1 in case of RK3399) > > we also need to set it every time we update the gamma lut. > > > - I also found dsp_lut_en but I presume it is a bug in documentation. > > > No, it is not a bug, we should set it when we enable gamma lut, we just don't > need to disable it before we update gamma lut. > > Here is some code you can take as reference [0] > [0]:https://github.com/radxa/kernel/blob/linux-6.1-stan-rkr1/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c#L3437 > Thank you for further clarification. I will include it in the next version of my patch. > > Should RK3588 be handled as RK3399? (gamma LUT can be written directly but > > gamma_update_en bit has to be set before). What about gamma_ahb_write_sel? > > > > [1] https://lkml.org/lkml/2024/7/27/293 > > Best Regards, Piotr Zalewski
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) { + /* + * 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); + + 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)
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@manjaro.org> Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me> --- 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@manjaro.org/T/#t [1] https://lore.kernel.org/linux-rockchip/d019761504b540600d9fc7a585d6f95f@manjaro.org/