Hi Andy and Diederik
>
> The root case for the problem is now clear。
>
> Most of the registers in VOP need to write the cfd_done register(call
> vop2_cfg_done)
> after you have configured the registers. Then, they will take effect only
> when the VSYNC event occurs(It doesn't take effect immediately after you
> finish writing.).
> This also includes all the VP_DSP_CTRL registers.
>
> see the code:
>
> vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>
> vop2_crtc_atomic_try_set_gamma(vop2, vp, crtc, crtc_state);
> -->
>
> static void vop2_vp_dsp_lut_disable(struct vop2_video_port vp)
> {
> u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>
>
> When we read this register, we are reading the actual effective value,
> not the one(dsp_ctrl) that was just written in before (it has not yet taken
> effect)
>
> So when we continue to write about this register here, we overwrite the actual
> value we originally intended to put in.
>
>
> dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN;
> vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
> }
>
> I think the correct solution should be similar to the Windows-related
> registry settings.
> All the registers related to Video Ports should be set as non-volatile, see:
> /
> * The window registers are only updated when config done is written.
> * Until that they read back the old value. As we read-modify-write
> * these registers mark them as non-volatile. This makes sure we read
> * the new values from the regmap register cache.
> */
> static const struct regmap_range vop2_nonvolatile_range[] = {
> regmap_reg_range(0x1000, 0x23ff),
> };
>
> static const struct regmap_access_table vop2_volatile_table = {
> .no_ranges = vop2_nonvolatile_range,
> .n_no_ranges = ARRAY_SIZE(vop2_nonvolatile_range),
> };
>
I'm wondering that perhaps making a separate code path for gamma
LUT enable in atomic enable would fix it? IIUC we set config.
earlier in atomic_enable and write dsp_ctrl + cfg_done with proper
configuration but since there wasn't a vsync the read of dsp ctrl
in lut disable is not correct (or we cannot be sure it is correct).
Putting try set gamma LUT before first dsp ctrl write and make it
use the dsp_ctrl value so far written in atomic enable could also
fix the issue because we wouldn't read dsp ctrl value from the
register.
> Actually, there is another question. I still haven't figured out why
> this problem doesn't occur when compiling rockchipdrm=y .
>
I applied your patch and run a test on both versions. This is what
i got with drm=m, so the same result as diederik.
```
[ 8.138482] vop2_crtc_atomic_try_set_gamma gamma_lut: 0000000000000000
[ 8.139093] vop2_vp_dsp_lut_disable dsp_ctrl: 0x0000000f
```
This is what i got with drm=y. So it seems with drm=y read value in
the register _is_ what was written earlier in atomic enable i.e value is
vaild? Maybe it is just "unfortunate" timing?
```
[ 6.646991] vop2_crtc_atomic_try_set_gamma gamma_lut: 0000000000000000
[ 6.647594] vop2_vp_dsp_lut_disable dsp_ctrl: 0x00010000
```
I will have time to spend more on this over the weekend.
Best regards, Piotr Zalewski
