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@xxxxxxxxxxxxxx> 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
