Re: [PATCH 05/18] drm/amd/display: Use mdelay to avoid crashes

[Alex Deucher <alexdeucher@gmail.com>]

On Wed, Dec 14, 2022 at 4:50 PM Alex Hung <alex.hung@amd.com> wrote:
>
>
>
> On 2022-12-14 13:48, Alex Deucher wrote:
> > On Wed, Dec 14, 2022 at 3:22 PM Aurabindo Pillai
> > <aurabindo.pillai@amd.com> wrote:
> >>
> >> From: Alex Hung <alex.hung@amd.com>
> >>
> >> [Why]
> >> When running IGT kms_bw test with DP monitor, some systems crash from
> >> msleep no matter how long or short the time is.
> >>
> >> [How]
> >> To replace msleep with mdelay.
> >
> > Can you provide a bit more info about the crash?  A lot of platforms
> > don't support delay larger than 2-4ms so this change will generate
> > errors on ARM and possibly other platforms.
> >
> > Alex
>
> The msleep was introduced in eec3303de3378 for non-compliant display
> port monitors but IGT's kms_bw test can cause a recent h/w to hang at
> msleep(60) when calling "igt_remove_fb" in IGT
> (https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_bw.c#L197)
>
> It is possible to workaround this by reversing order of
> igt_remove_fb(&buffer[i]), as the following example:
>
>    igt_create_color_fb with the order buffer[0], buffer[1], buffer[2]
>
> Hangs:
>    igt_remove_fb with the order buffer[0], buffer[1], buffer[2]
>
> No hangs:
>    igt_remove_fb with the reversed order buffer[2], buffer[1], buffer[0]
>
> However, IGT simply exposes the problem and it makes more sense to stop
> the hang from occurring.
>
> I also tried to remove the msleep completely and it also work, but I
> didn't want to break the fix for the original problematic hardware
> configuration.

Why does sleep vs delay make a difference?  Is there some race that we
are not locking against?

Alex

>
> >
> >>
> >> Acked-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> >> Signed-off-by: Alex Hung <alex.hung@amd.com>
> >> Reviewed-by: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com>
> >> ---
> >>   drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> index 913a1fe6b3da..e6251ccadb70 100644
> >> --- a/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> +++ b/drivers/gpu/drm/amd/display/dc/dce110/dce110_hw_sequencer.c
> >> @@ -1215,7 +1215,7 @@ void dce110_blank_stream(struct pipe_ctx *pipe_ctx)
> >>                           * After output is idle pattern some sinks need time to recognize the stream
> >>                           * has changed or they enter protection state and hang.
> >>                           */
> >> -                       msleep(60);
> >> +                       mdelay(60);
> >>                  } else if (pipe_ctx->stream->signal == SIGNAL_TYPE_EDP) {
> >>                          if (!link->dc->config.edp_no_power_sequencing) {
> >>                                  /*
> >> --
> >> 2.39.0
> >>