Re:Re: [PATCH drm-misc-next] rockchip/drm: vop2: don't check color_mgmt_changed in atomic_enable

["Andy Yan" <andyshrk@163.com>]

[-- Attachment #1: Type: text/plain, Size: 4808 bytes --]



Hi Diederik,

At 2025-06-08 20:53:37, "Diederik de Haas" <didi.debian@cknow.org> wrote:
>Hi Andy,
>
>On Sun Jun 8, 2025 at 2:10 PM CEST, Andy Yan wrote:
>> At 2025-06-08 19:08:50, "Diederik de Haas" <didi.debian@cknow.org> wrote:
>>>On Sat Jun 7, 2025 at 5:32 PM CEST, Piotr Zalewski wrote:
>>>> On Thursday, June 5th, 2025 at 10:13 PM, Diederik de Haas <didi.debian@cknow.org> wrote:
>>>>> Since kernel 6.14-rc1 I have the problem that visual output is no longer
>>>>> shown on my PineTab2 and a `git bisect` pointed to this patch/commit
>>>>> as the culprit. What is important to note is that `CONFIG_DRM=m` seems
>>>>> to be required as the problem does not occur with `CONFIG_DRM=y`.
>>>>> 
>>>>> Near the end of my bisect session, something interesting occurred.
>>>>> I was booted into a 'bad' kernel (ie no visual output) and when I
>>>>> started to build my final kernel, I closed the lid of the PineTab2 which
>>>>> made it go into suspend. When my final kernel was built, I opened the
>>>>> lid again, which made it resume, to transfer my final kernel to it.
>>>>> And much to my surprise, I then did have visual output.
>>>>> When I read the (below) commit message of the 'offending' commit, it may
>>>>> not be such a surprise after all.
>>>>> 
>>>>> I did try it on a Quartz64-B (also rk3566) and it did not have any issue
>>>>> (output via HDMI).
>>>>> I don't know what the cause for this issue is, hopefully you do.
>>>>
>>>> I tested and confirmed that this happens with drm=m but also in my case 
>>>> it happened when drm=y. After some testing I found out that at boot modeset
>>>
>>>Interesting that it also happened with drm=y.
>>>As you're more knowledgeable then I am with this, maybe look through
>>>https://lists.sr.ht/~diederik/pine64-discuss/<D9AM2OOLREO0.2JMAI42J06TW0@cknow.org>
>>>
>>>to see if you may spot something relevant?
>>>
>>>> happened twice and at short interval and since this patch allows for gamma 
>>>> LUT update regardless of color_mgmt_changed state this makes DSP CTRL GAMMA 
>>>> LUT EN bit to be unset twice too. It seems that VOP does not like it. I 
>>>
>>>Happy to see you found the cause :-)
>>>Do you happen to know why it was unset twice? That sounds suboptimal.
>>>But (IIUC) setting a bit to a value it already has causing issues,
>>>sounds surprising as well.
>>
>> I have conducted tests on both rk3566-box-demo (with drm set to y) and rk3568-lubancat-2 (with drm set to m), 
>> but I was unable to reproduce this issue. Could you two please share your kernel defconfig and the corresponding kernel startup logs? 
>> Additionally, both of my two boards tested with HDMI output. What kind of display interface does your board use for output?
>
>I wasn't able to reproduce this issue on my PINE64 Quartz-B (rk3566) 
>with HDMI output either, but the problem is present on a PineTab2 [1]
>(also rk3566) which uses a MIPI DSI connection to the display panel.
>
>Kernel config:
>https://paste.sr.ht/~diederik/aa747ed170aa01cc759fbe1ffd9cebe8c887b10b
>
>dmesg kernel 6.14-rc1:
>https://paste.sr.ht/~diederik/733fbf8bb7f6aee8b68cf5a652157d445462c24a
>
>dmesg kernel 6.14-rc1 with Piotr's patch:
>https://paste.sr.ht/~diederik/db1af672cfb611acbfbdf35adb6f170e5c38febc
>
>Both dmesg outputs contain a suspend-resume cycle.
>I'm using a USB Wi-Fi adapter for the wireless connection.
>
>[1] https://wiki.pine64.org/wiki/PineTab2
>
>Happy to provide more info and/or do some tests.

Can you apply the patch in the attachment, reproduce this issue(without Piotr's patch), 
and then provide me with a copy of the kernel log?

Thanks.

>
>Cheers,
>  Diederik
>
>>>> patched vop2_vp_dsp_lut_disable function so that dsp_ctrl is set only if 
>>>> GAMMA LUT EN bit is set. I checked that this also does not break the gamma 
>>>> lut functionality with emphasis on out-of/into suspend behavior.
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> index d0f5fea15e21..7ddf311b38c6 100644
>>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>>> @@ -897,6 +897,9 @@ static void vop2_vp_dsp_lut_disable(struct vop2_video_port *vp)
>>>>  {
>>>>  	u32 dsp_ctrl = vop2_vp_read(vp, RK3568_VP_DSP_CTRL);
>>>>  
>>>> +	if ((dsp_ctrl & RK3568_VP_DSP_CTRL__DSP_LUT_EN) == 0)
>>>> +		return;
>>>> +
>>>>  	dsp_ctrl &= ~RK3568_VP_DSP_CTRL__DSP_LUT_EN;
>>>>  	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
>>>>  }
>>>
>>>I built a kernel with 6.14-rc1 + this patch and can confirm the screen
>>>has output again :-)
>>>
>>>> I will wait with sending a patch because maybe Andy has something to add 
>>>> to this.
>>>
>>>Sounds like a plan. It could be that this issue surfaced an underlaying
>>>issue and if so, fixing that would be even better.

[-- Attachment #2: print_lut_0609_1710.patch --]
[-- Type: application/octet-stream, Size: 1223 bytes --]

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index d0f5fea15e21..d90b345b5b53 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -898,6 +898,7 @@ 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;
+	printk("vop2_vp_dsp_lut_disable dsp_ctrl: 0x%08x\n", dsp_ctrl);
 	vop2_vp_write(vp, RK3568_VP_DSP_CTRL, dsp_ctrl);
 }
 
@@ -1506,7 +1507,7 @@ static void vop2_crtc_atomic_try_set_gamma(struct vop2 *vop2,
 {
 	if (!vop2->lut_regs)
 		return;
-
+	printk("%s  gamma_lut: %px\n", __func__, crtc_state->gamma_lut);
 	if (!crtc_state->gamma_lut) {
 		vop2_vp_dsp_lut_disable(vp);
 		return;
@@ -1643,7 +1644,7 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
 	int ret;
 	struct drm_encoder *encoder;
 
-	drm_dbg(vop2->drm, "Update mode to %dx%d%s%d, type: %d for vp%d\n",
+	drm_info(vop2->drm, "Update mode to %dx%d%s%d, type: %d for vp%d\n",
 		hdisplay, vdisplay, mode->flags & DRM_MODE_FLAG_INTERLACE ? "i" : "p",
 		drm_mode_vrefresh(mode), vcstate->output_type, vp->id);
 

[-- Attachment #3: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip