[igt-dev] [PATCH i-g-t] igt/tests: Clear pipes properly in kms_atomic_transition

[Lisovskiy, Stanislav]

On Tue, Jun 30, 2020 at 02:13:33PM +0200, Maarten Lankhorst wrote:
> Op 23-06-2020 om 17:07 schreef Juha-Pekka Heikkila:
> > On 23.6.2020 16.53, Stanislav Lisovskiy wrote:
> >> There is an issue happening from time to time in kms_atomic_transition
> >> (bug #1918). We periodically get assertion that some two outputs
> >> attempt to use same pipe like this:
> >>
> >> "Failed assertion: output->pending_pipe != b->pending_pipe"
> >>
> >> After some investigation came to conclusion that this is happening
> >> because we are calling igt_output_set_pipe(output, PIPE_NONE) only
> >> for connected outputs, which is wrong.
> >> Periodically igt_display_refresh/igt_output_refresh call calls can
> >> update the output state to disconnected. However that doesn't clear
> >> the pipe being assigned(i.e output->pending_pipe).
> >> So this causes assertion to be triggered on next igt_display_refresh
> >> called during commit.
> >>
> >> Bugzilla: https://gitlab.freedesktop.org/drm/intel/-/issues/1918
> >>
> >> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> >> ---
> >>   tests/kms_atomic_transition.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
> >> index 754a4969..a55388cd 100644
> >> --- a/tests/kms_atomic_transition.c
> >> +++ b/tests/kms_atomic_transition.c
> >> @@ -665,8 +665,9 @@ static unsigned set_combinations(igt_display_t *display, unsigned mask, struct i
> >>       enum pipe pipe;
> >>       unsigned event_mask = 0;
> >>   -    for_each_connected_output(display, output)
> >> -        igt_output_set_pipe(output, PIPE_NONE);
> >> +    for_each_pipe(display, pipe)
> >> +        for_each_valid_output_on_pipe(display, pipe, output)
> >> +            igt_output_set_pipe(output, PIPE_NONE);
> >>         for_each_pipe(display, pipe) {
> >>           igt_plane_t *plane = igt_pipe_get_plane_type(&display->pipes[pipe],
> >>
> >
> > Look ok to me.
> >
> > Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com> 
> 
> Nack, for_each_valid_output_on_pipe uses for_each_connected_output, so you keep the bug..
> 
>     for (i = 0; i < display->n_outputs; i++)
>         igt_output_set_pipe(&display->outputs[i], PIPE_NONE); ?
> 
>

Good point, I was so much relying that it just doesn't do that.

Stan