During suspend/resume process all connectors are explicitly disabled and then reenabled. However resume fails because of the connector_status check:
[ 1185.831970] [dpu error]connector not connected 3
It doesn't make sense to check for the Writeback connected status (and other drivers don't perform such check), so drop the check.
Fixes: 71174f362d67 ("drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c") Cc: stable@vger.kernel.org Reported-by: Leonard Lausen leonard@lausen.nl Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/57 Tested-by: Leonard Lausen leonard@lausen.nl # on sc7180 lazor Tested-by: György Kurucz me@kuruczgy.com Reviewed-by: Johan Hovold johan+linaro@kernel.org Tested-by: Johan Hovold johan+linaro@kernel.org Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- Leonard Lausen reported an issue with suspend/resume of the sc7180 devices. Fix the WB atomic check, which caused the issue. --- Changes in v3: - Rebased on top of msm-fixes - Link to v2: https://lore.kernel.org/r/20240802-dpu-fix-wb-v2-0-7eac9eb8e895@linaro.org
Changes in v2: - Reworked the writeback to just drop the connector->status check. - Expanded commit message for the debugging patch. - Link to v1: https://lore.kernel.org/r/20240709-dpu-fix-wb-v1-0-448348bfd4cb@linaro.org --- drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c index 16f144cbc0c986ee266412223d9e605b01f9fb8c..8ff496082902b1ee713e806140f39b4730ed256a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c @@ -42,9 +42,6 @@ static int dpu_wb_conn_atomic_check(struct drm_connector *connector, if (!conn_state || !conn_state->connector) { DPU_ERROR("invalid connector state\n"); return -EINVAL; - } else if (conn_state->connector->status != connector_status_connected) { - DPU_ERROR("connector not connected %d\n", conn_state->connector->status); - return -EINVAL; }
crtc = conn_state->crtc;
--- base-commit: 86313a9cd152330c634b25d826a281c6a002eb77 change-id: 20240709-dpu-fix-wb-6cd57e3eb182
Best regards,
Dmitry,
Looks like you just silently ignored my reviewed feedback, yet included my conditional reviewed-by tag. Repeating below.
On Sun, Dec 08, 2024 at 07:29:11PM +0200, Dmitry Baryshkov wrote:
During suspend/resume process all connectors are explicitly disabled and then reenabled. However resume fails because of the connector_status check:
[ 1185.831970] [dpu error]connector not connected 3
Please also include the follow-on resume error. I'm seeing:
[dpu error]connector not connected 3 [drm:drm_mode_config_helper_resume [drm_kms_helper]] *ERROR* Failed to resume (-22)
and say something about that this can prevent *displays* from being enabled on resume in *some* setups (preferably with an explanation why if you have one).
It doesn't make sense to check for the Writeback connected status (and other drivers don't perform such check), so drop the check.
Fixes: 71174f362d67 ("drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c")
I noticed that the implementation had this status check also before 71174f362d67 ("drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c").
Why did this not cause any trouble back then? Or is this not the right Fixes tag?
Cc: stable@vger.kernel.org Reported-by: Leonard Lausen leonard@lausen.nl Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/57
Please include mine an György's reports here too.
Since this has dragged on for many months now, more people have run into this issue and have reported this to you. Giving them credit for this is the least you can do especially since you failed to include the corresponding details about how this manifests itself to users in the commit message:
Reported-by: György Kurucz me@kuruczgy.com Link: https://lore.kernel.org/all/b70a4d1d-f98f-4169-942c-cb9006a42b40@kuruczgy.co...
Reported-by: Johan Hovold johan+linaro@kernel.org Link: https://lore.kernel.org/all/ZzyYI8KkWK36FfXf@hovoldconsulting.com/
Tested-by: Leonard Lausen leonard@lausen.nl # on sc7180 lazor Tested-by: György Kurucz me@kuruczgy.com Reviewed-by: Johan Hovold johan+linaro@kernel.org Tested-by: Johan Hovold johan+linaro@kernel.org Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
Leonard Lausen reported an issue with suspend/resume of the sc7180 devices. Fix the WB atomic check, which caused the issue.
Changes in v3:
- Rebased on top of msm-fixes
- Link to v2: https://lore.kernel.org/r/20240802-dpu-fix-wb-v2-0-7eac9eb8e895@linaro.org
Johan
On Mon, 9 Dec 2024 at 11:25, Johan Hovold johan@kernel.org wrote:
Dmitry,
Looks like you just silently ignored my reviewed feedback, yet included my conditional reviewed-by tag. Repeating below.
Excuse me. I'll expand the commit message.
On Sun, Dec 08, 2024 at 07:29:11PM +0200, Dmitry Baryshkov wrote:
During suspend/resume process all connectors are explicitly disabled and then reenabled. However resume fails because of the connector_status check:
[ 1185.831970] [dpu error]connector not connected 3
Please also include the follow-on resume error. I'm seeing:
[dpu error]connector not connected 3 [drm:drm_mode_config_helper_resume [drm_kms_helper]] *ERROR* Failed to resume (-22)and say something about that this can prevent *displays* from being enabled on resume in *some* setups (preferably with an explanation why if you have one).
It doesn't make sense to check for the Writeback connected status (and other drivers don't perform such check), so drop the check.
Fixes: 71174f362d67 ("drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c")
I noticed that the implementation had this status check also before 71174f362d67 ("drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c").
Why did this not cause any trouble back then? Or is this not the right Fixes tag?
If I remember correctly, the encoder's atomic_check() is called only if the corresponding connector is a part of the new state, if there is a connected CRTC, etc, while the connector's atomic_check() is called both for old and new connectors.
Cc: stable@vger.kernel.org Reported-by: Leonard Lausen leonard@lausen.nl Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/57
Please include mine an György's reports here too.
Since this has dragged on for many months now, more people have run into this issue and have reported this to you. Giving them credit for this is the least you can do especially since you failed to include the corresponding details about how this manifests itself to users in the commit message:
Reported-by: György Kurucz me@kuruczgy.com Link: https://lore.kernel.org/all/b70a4d1d-f98f-4169-942c-cb9006a42b40@kuruczgy.co...
Reported-by: Johan Hovold johan+linaro@kernel.org Link: https://lore.kernel.org/all/ZzyYI8KkWK36FfXf@hovoldconsulting.com/
Tested-by: Leonard Lausen leonard@lausen.nl # on sc7180 lazor Tested-by: György Kurucz me@kuruczgy.com Reviewed-by: Johan Hovold johan+linaro@kernel.org Tested-by: Johan Hovold johan+linaro@kernel.org Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org
Leonard Lausen reported an issue with suspend/resume of the sc7180 devices. Fix the WB atomic check, which caused the issue.
Changes in v3:
- Rebased on top of msm-fixes
- Link to v2: https://lore.kernel.org/r/20240802-dpu-fix-wb-v2-0-7eac9eb8e895@linaro.org
Johan
On Mon, Dec 09, 2024 at 12:00:07PM +0200, Dmitry Baryshkov wrote: On Mon, 9 Dec 2024 at 11:25, Johan Hovold johan@kernel.org wrote:
I noticed that the implementation had this status check also before 71174f362d67 ("drm/msm/dpu: move writeback's atomic_check to dpu_writeback.c").
Why did this not cause any trouble back then? Or is this not the right Fixes tag?
If I remember correctly, the encoder's atomic_check() is called only if the corresponding connector is a part of the new state, if there is a connected CRTC, etc, while the connector's atomic_check() is called both for old and new connectors.
Thanks for the explanation.
Johan
linux-stable-mirror@lists.linaro.org
-
Dmitry Baryshkov -
Johan Hovold