[v3] drm/msm/dpu1: don't choke on disabling the writeback connector

Commit Message

Dmitry Baryshkov Dec. 8, 2024, 5:29 p.m. UTC

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: [e-mail]
Reported-by: Leonard Lausen <[e-mail]>
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/57
Tested-by: Leonard Lausen <[e-mail]> # on sc7180 lazor
Tested-by: György Kurucz <[e-mail]>
Reviewed-by: Johan Hovold <[e-mail]>
Tested-by: Johan Hovold <[e-mail]>
Signed-off-by: Dmitry Baryshkov <[e-mail]>
---
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/[e-mail]

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/[e-mail]
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 3 ---
 1 file changed, 3 deletions(-)


---
base-commit: 86313a9cd152330c634b25d826a281c6a002eb77
change-id: 20240709-dpu-fix-wb-6cd57e3eb182

Best regards,

Comments

Johan Hovold Dec. 9, 2024, 9:25 a.m. UTC | #1

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: [e-mail]
> Reported-by: Leonard Lausen <[e-mail]>
> 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 <[e-mail]>
Link: https://lore.kernel.org/all/[e-mail]/

Reported-by: Johan Hovold <[e-mail]>
Link: https://lore.kernel.org/all/[e-mail]/

> Tested-by: Leonard Lausen <[e-mail]> # on sc7180 lazor
> Tested-by: György Kurucz <[e-mail]>
> Reviewed-by: Johan Hovold <[e-mail]>
> Tested-by: Johan Hovold <[e-mail]>
> Signed-off-by: Dmitry Baryshkov <[e-mail]>
> ---
> 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/[e-mail]

Johan

Dmitry Baryshkov Dec. 9, 2024, 10 a.m. UTC | #2

On Mon, 9 Dec 2024 at 11:25, Johan Hovold <[e-mail]> 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: [e-mail]
> > Reported-by: Leonard Lausen <[e-mail]>
> > 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 <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/
>
> Reported-by: Johan Hovold <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/
>
> > Tested-by: Leonard Lausen <[e-mail]> # on sc7180 lazor
> > Tested-by: György Kurucz <[e-mail]>
> > Reviewed-by: Johan Hovold <[e-mail]>
> > Tested-by: Johan Hovold <[e-mail]>
> > Signed-off-by: Dmitry Baryshkov <[e-mail]>
> > ---
> > 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/[e-mail]
>
> Johan

Johan Hovold Dec. 9, 2024, 10:16 a.m. UTC | #3

On Mon, Dec 09, 2024 at 12:00:07PM +0200, Dmitry Baryshkov wrote:
On Mon, 9 Dec 2024 at 11:25, Johan Hovold <[e-mail]> 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

Patch

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;