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

[Dmitry Baryshkov]

On Mon, 9 Dec 2024 at 11:25, Johan Hovold <johan@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> > Reported-by: Leonard Lausen <leonard@xxxxxxxxx>
> > 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@xxxxxxxxxxxx>
> Link: https://lore.kernel.org/all/b70a4d1d-f98f-4169-942c-cb9006a42b40@xxxxxxxxxxxx/
>
> Reported-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> Link: https://lore.kernel.org/all/ZzyYI8KkWK36FfXf@xxxxxxxxxxxxxxxxxxxx/
>
> > Tested-by: Leonard Lausen <leonard@xxxxxxxxx> # on sc7180 lazor
> > Tested-by: György Kurucz <me@xxxxxxxxxxxx>
> > Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > Tested-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx
>
> Johan



-- 
With best wishes
Dmitry