SC7180: Color Transform Matrix breaks after suspend

changed the description

We have asked our QA team to recreate this bug to debug further.

Thanks

Abhinav

Our QA team could not recreate this with the sc7180 chromebook with 5.15 kernel as most of our internal software builds are still using that kernel for sc7180.

So perhaps something got broken between 5.15 and 6.x kernels.

We will have to check with the latest kernels what got broken now.

Thanks

Abhinav

could be a difference between suspend-to-idle vs suspend-to-ram (or whatever "deep suspend" means)? IIRC on CrOS s2idle is used, at least for arm64, but distros may be using suspend-to-ram instead (because apparently s2idle draws a lot more power on x86?)

Hi Rob

For debugging this, is it possible to switch CrOS to s2ram with any config? Would like to see if we can repro this with s2ram on CrOS.

Thanks

Abhinav

I think:

echo deep > /sys/power/mem_sleep

assigned to @jenatali and unassigned @abhinavk

assigned to @abhinavk and unassigned @jenatali

Hey, I can take a look at this

Thank you @jesszhan @abhinavk @robclark. I was able to confirm that the issue also happens with s2idle. However, it only happens if an external display is connected. If no external display is connected, Color Transform Matrix does NOT break after deep suspend nor after s2idle suspend.

Disconnecting the external display after the suspend, unsuspend cycle does not restore Color Transform Matrix functionality. The only way to restore the functionality without reboot appears to be the trick described in the Overview: "One trick to get the CTM to be applied again is to disable the display (close laptop screen) while an external monitor is connected, then re-enable the display (open the laptop screen again) and disconnect the external monitor."

changed title from SC7180: Color Transform Matrix breaks after deep suspend to SC7180: Color Transform Matrix breaks after suspend

@jesszhan @abhinavk @robclark the external monitor issue may be more complicated. But I've also noticed a problem for the case without any external monitors. In that case, CTM is simply no longer applied after suspend / wake-up cycle. Only if user modifies CTM setting, CTM starts being applied again. As example, see below log output. CTM is set to blob=79 before the suspend, which applies a night light effect. After suspend and resume, CTM blob remains unchanged but the night light effect disappears, meaning that the driver/msm no longer applies CTM. Upon changing the CTM in GUI Settings, it is being applied again.

I presume that ChromeOS works around this kernel bug by always re-applying / modifying CTM settings after system wakup @robclark?

% ./drm_info | grep -B 11 CTM
drmModeGetResources: Operation not supported
Failed to retrieve information from /dev/dri/card0
│   ├───CRTC 0
│   │   ├───Object ID: 65
│   │   ├───Legacy info
│   │   │   ├───Mode: 1920x1080@60.00 preferred driver phsync nvsync
│   │   │   └───Gamma size: 0
│   │   └───Properties
│   │       ├───"ACTIVE" (atomic): range [0, 1] = 1
│   │       ├───"MODE_ID" (atomic): blob = 74
│   │       │   └───1920x1080@60.00 preferred driver phsync nvsync
│   │       ├───"OUT_FENCE_PTR" (atomic): range [0, UINT64_MAX] = 0
│   │       ├───"VRR_ENABLED": range [0, 1] = 0
│   │       └───"CTM": blob = 79
│   └───CRTC 1
│       ├───Object ID: 66
│       ├───Legacy info
│       │   └───Gamma size: 0
│       └───Properties
│           ├───"ACTIVE" (atomic): range [0, 1] = 0
│           ├───"MODE_ID" (atomic): blob = 0
│           ├───"OUT_FENCE_PTR" (atomic): range [0, UINT64_MAX] = 0
│           ├───"VRR_ENABLED": range [0, 1] = 0
│           └───"CTM": blob = 85
% cat /sys/power/mem_sleep
[s2idle] deep
% sudo systemctl suspend
% ./drm_info | grep -B 11 CTM
drmModeGetResources: Operation not supported
Failed to retrieve information from /dev/dri/card0
│   ├───CRTC 0
│   │   ├───Object ID: 65
│   │   ├───Legacy info
│   │   │   ├───Mode: 1920x1080@60.00 preferred driver phsync nvsync
│   │   │   └───Gamma size: 0
│   │   └───Properties
│   │       ├───"ACTIVE" (atomic): range [0, 1] = 1
│   │       ├───"MODE_ID" (atomic): blob = 74
│   │       │   └───1920x1080@60.00 preferred driver phsync nvsync
│   │       ├───"OUT_FENCE_PTR" (atomic): range [0, UINT64_MAX] = 0
│   │       ├───"VRR_ENABLED": range [0, 1] = 0
│   │       └───"CTM": blob = 79
│   └───CRTC 1
│       ├───Object ID: 66
│       ├───Legacy info
│       │   └───Gamma size: 0
│       └───Properties
│           ├───"ACTIVE" (atomic): range [0, 1] = 0
│           ├───"MODE_ID" (atomic): blob = 0
│           ├───"OUT_FENCE_PTR" (atomic): range [0, UINT64_MAX] = 0
│           ├───"VRR_ENABLED": range [0, 1] = 0
│           └───"CTM": blob = 85
% # Change CTM value in GUI
% ./drm_info | grep -B 11 CTM                                                                                                                                    ~/src/drm_info/build master
drmModeGetResources: Operation not supported
Failed to retrieve information from /dev/dri/card0
│   ├───CRTC 0
│   │   ├───Object ID: 65
│   │   ├───Legacy info
│   │   │   ├───Mode: 1920x1080@60.00 preferred driver phsync nvsync
│   │   │   └───Gamma size: 0
│   │   └───Properties
│   │       ├───"ACTIVE" (atomic): range [0, 1] = 1
│   │       ├───"MODE_ID" (atomic): blob = 74
│   │       │   └───1920x1080@60.00 preferred driver phsync nvsync
│   │       ├───"OUT_FENCE_PTR" (atomic): range [0, UINT64_MAX] = 0
│   │       ├───"VRR_ENABLED": range [0, 1] = 0
│   │       └───"CTM": blob = 75
│   └───CRTC 1
│       ├───Object ID: 66
│       ├───Legacy info
│       │   └───Gamma size: 0
│       └───Properties
│           ├───"ACTIVE" (atomic): range [0, 1] = 0
│           ├───"MODE_ID" (atomic): blob = 0
│           ├───"OUT_FENCE_PTR" (atomic): range [0, UINT64_MAX] = 0
│           ├───"VRR_ENABLED": range [0, 1] = 0
│           └───"CTM": blob = 85

I presume that ChromeOS works around this kernel bug by always re-applying / modifying CTM settings after system wakup @robclark?

Probably? Not sure I'd call it a work-around but it could be why we don't see this issue.

@leezu could you please check, if you remove the if (!state->color_mgmt_changed) return; from _dpu_crtc_setup_cp_blocks, will the CTM be applied after resume?

Hi @leezu

From your comment #23 (comment 1699796) , it seems like you are not running CrOS. Hence we could not replicate your problem with our QA team.

Which compositor are you trying with?

Thanks

Abhinav

Hi @abhinavk,

I'm using KDE KWin as Compositor built from https://invent.kde.org/plasma/kwin/-/merge_requests/3296 The merge request should be released as part of upcoming KDE Plasma 5.26

Thank you

Hi @lumag,

thank you! Rebuilding with below patch resolves the issue for the single monitor setting. I do not have the external display handy before the second week of January, so I will report on the effect in the external display later.

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..269eff899901 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -747,10 +747,6 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
        struct dpu_hw_dspp *dspp;
        int i;
 
-
-       if (!state->color_mgmt_changed)
-               return;
-
        for (i = 0; i < cstate->num_mixers; i++) {
                ctl = mixer[i].lm_ctl;
                dspp = mixer[i].hw_dspp;

@abhinavk @robclark short summary of my understanding of the issue. The driver expects that the hw_dspp state is corectly cached, thus the DSPP is reprogrammed only if there was a change in one of color management properties. However during suspend/resume the hardware state of DSPP is lost. As there was no change in props, corresponding flag is not set, so DSPP is not reprogrammed.

I think the easiest way to fix the issue is to force color_mgmt_changed flag in the suspended state before calling resume functions. WDYT?

If you agree with my analisys, could you please send corresponding patch?

Hi @lumag

Yes, that seems to be the case. I will push a change after testing on my chromebook.

Thanks

Abhinav

Hi @leezu

I have finished setting up my chromebook here. Will figure out a way to replicate this bug and post a fix in a week from now.

Thanks

Abhinav

Hey @lumag @robclark ,

I was able to recreate the issue on SC7180, but I had some doubts about the proposed fix. Should it be up to drivers to dictate what the state of a property should be after a resume? And aren't there some cases where CTM shouldn't be applied after a resume?

For example, if we end up resuming with a different state where CTM wasn't previously applied, wouldn't reapplying CTM after resume be wrong?

FWIW, I looked for examples of this from other drivers and wasn't able to find examples other drivers implementing a similar fix.

Thanks,

Jessica Zhang

The atomic state should be applied (if the state is lost by the hw) across s/r.. it is an implementation detail of the hw whether the state needs to be reprogrammed after resume. Ie. if it is still showing up in /sys/kernel/debug/dri/0/state then the hw state should reflect that.

@robclark @lumag

Yes, agreed, we need to restore the state.

I was thinking a bit more about it. Rather than touching the color_mgmt_changed flag which is controlled by UAPI, I feel that below change should be better because we will be programming the CP blocks even if active_changed.

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..36d09ba8f757 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -748,7 +748,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
        int i;


-       if (!state->color_mgmt_changed)
+       if (!state->color_mgmt_changed && !state->active_changed)
                return;

        for (i = 0; i < cstate->num_mixers; i++) {

What do you think of this?

Thanks

Abhinav

Just to note. We are reaquiring the DSPPs in dpu_encoder_virt_atomic_check if drm_atomic_crtc_needs_modeset(). Thus even if color_mgmt (or active) wasn't changed the backing DSPP unit might change. I think we'd better use drm_atomic_crtc_needs_modeset() here in addition to color_mgmt_changed.

BTW: what is the cost of DSPP reprogramming flush? Is it something noticeable?

Hi @lumag

Alright, lets go with drm_atomic_crtc_needs_modeset().

Regarding your second question, why would there be two flush programmings?

So the first commit after resume was the one which was marking color_mgmt_changed as true for Chrome OS.

Now, the same first commit will reprogram the CP blocks thanks to the drm_atomic_crtc_needs_modeset() change.

Even if we add the condition, it will happen only once.

Thanks

Abhinav

@abhinavk I was asking to understand if the if (color_management_changed) condition makes any sense at all. In other words, if we remove it and measure power consumption, can we notice the difference?

@lumag We need to investigate the power consumption angle but its not just the flush. The CTM matrix also gets reprogrammed every frame if we do that. Dont think its a good idea.

Meanwhile, @leezu can you please check if below change helps for this issue?

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321283ff..c745471c1901 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -748,7 +748,7 @@ static void _dpu_crtc_setup_cp_blocks(struct drm_crtc *crtc)
        int i;


-       if (!state->color_mgmt_changed)
+       if (!state->color_mgmt_changed && !drm_atomic_crtc_needs_modeset(state))
                return;

        for (i = 0; i < cstate->num_mixers; i++) {

Thanks

Abhinav

Hi @abhinavk, I applied your new patch to v6.1.6 and confirmed it also works.

Hi @leezu , thanks for confirming. I've posted the fix here: https://patchwork.freedesktop.org/series/112983/

mentioned in commit lumag/msm@97c35007

mentioned in commit lumag/msm@7efd4edc

closed with merge request !36 (merged)

mentioned in merge request !78 (merged)

SC7180: Color Transform Matrix breaks after suspend

Designs

Child items ...

Activity

Admin message

Admin message

SC7180: Color Transform Matrix breaks after suspend

Activity