[igt-dev] [PATCH i-g-t] tests/kms_fbcon_fbt: When restoring fbcon always set mode to text mode

[Souza, Jose]

On Mon, 2020-06-01 at 17:14 +0530, Shankar, Uma wrote:
> > -----Original Message-----
> > From: Souza, Jose <jose.souza at intel.com>
> > Sent: Friday, May 29, 2020 10:21 PM
> > To: Shankar, Uma <uma.shankar at intel.com>; igt-dev at lists.freedesktop.org
> > Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_fbcon_fbt: When restoring fbcon
> > always set mode to text mode
> > 
> > On Fri, 2020-05-29 at 21:19 +0530, Shankar, Uma wrote:
> > > > -----Original Message-----
> > > > From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of
> > > > José Roberto de Souza
> > > > Sent: Friday, May 29, 2020 6:22 AM
> > > > To: igt-dev at lists.freedesktop.org
> > > > Subject: [igt-dev] [PATCH i-g-t] tests/kms_fbcon_fbt: When restoring
> > > > fbcon always set mode to text mode
> > > > 
> > > > If by some reason or tests, this tests is executed and VT mode is
> > > > already in KD_GRAPHICS the call to kmstest_set_vt_graphics_mode()
> > > > will set orig_vt_mode as KD_GRAPHICS and when it was calling
> > > > kmstest_restore_vt_mode() it would set KD_GRAPHICS again not
> > > > returning to fbcon and causing the test to fail.
> > > > 
> > > > As it can be seen here:
> > > > (kms_fbcon_fbt:11004) igt_kms-DEBUG: VT: graphics mode set (mode was
> > 0x1)"
> > > > https://intel-gfx-ci.01.org/tree/drm-tip/drmtip_499/fi-ehl-
> > > > 1/igt at kms_fbcon_fbt@fbc.html
> > > > 
> > > > So here adding a new function to alaways set mode the KD_TEXT when
> > > > we want to restore to fbcon.
> > > > 
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >  lib/igt_kms.c         | 11 +++++++++++
> > > >  lib/igt_kms.h         |  1 +
> > > >  tests/kms_fbcon_fbt.c |  2 +-
> > > >  3 files changed, 13 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c index
> > > > d4cbc1c53..afef59396 100644
> > > > --- a/lib/igt_kms.c
> > > > +++ b/lib/igt_kms.c
> > > > @@ -968,6 +968,17 @@ void kmstest_set_vt_graphics_mode(void)
> > > >  	igt_debug("VT: graphics mode set (mode was 0x%lx)\n", ret);  }
> > > > 
> > > > +/**
> > > > + * kmstest_set_vt_text_mode:
> > > > + *
> > > > + * Sets the controlling VT (if available) into text mode.
> > > > + * Unlikely kmstest_set_vt_graphics_mode() it do not install an igt
> > > > +exit
> > > > + * handler to set the VT back to the previous mode.
> > > > + */
> > > > +void kmstest_set_vt_text_mode(void) {
> > > > +	igt_assert(set_vt_mode(KD_TEXT) >= 0); }
> > > > 
> > > >  static void reset_connectors_at_exit(int sig)  { diff --git
> > > > a/lib/igt_kms.h b/lib/igt_kms.h index adca59ac6..32a0e4cc6 100644
> > > > --- a/lib/igt_kms.h
> > > > +++ b/lib/igt_kms.h
> > > > @@ -93,6 +93,7 @@ void kmstest_dump_mode(drmModeModeInfo *mode);
> > > > int kmstest_get_pipe_from_crtc_id(int fd, int crtc_id);  void
> > > > kmstest_set_vt_graphics_mode(void);
> > > >  void kmstest_restore_vt_mode(void);
> > > > +void kmstest_set_vt_text_mode(void);
> > > > 
> > > >  enum igt_atomic_crtc_properties {
> > > >         IGT_CRTC_BACKGROUND = 0,
> > > > diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c index
> > > > e6dd4353b..e99a1f2f2 100644
> > > > --- a/tests/kms_fbcon_fbt.c
> > > > +++ b/tests/kms_fbcon_fbt.c
> > > > @@ -283,7 +283,7 @@ static void restore_fbcon(struct drm_info *drm)
> > > >  	kmstest_unset_all_crtcs(drm->fd, drm->res);
> > > >  	igt_remove_fb(drm->fd, &drm->fb);
> > > >  	igt_device_drop_master(drm->fd);
> > > > -	kmstest_restore_vt_mode();
> > > > +	kmstest_set_vt_text_mode();
> > > 
> > > Can we level set the state to KD_TEXT in beginning only before we begin our
> > test.
> > > This way test will start with the expected state, and we can then set
> > > KD_GRAPHICS, restore will ensure previous state is set again as
> > > KD_TEXT, and we can just continue with the existing restore logic. Will that
> > cause any issue ?
> > 
> > We could be overwriting the previous state, KD_TEXT is the state we expect in
> > our CI but it do not means that is the state that other users have prior of
> > executing this test, that is why kmstest_restore_vt_mode() don't just set to
> > KD_TEXT.
> 
> But will this not put the mode as KD_TEXT for other user as well who would be expecting it to be
> KD_GRAPHICS. How we ensure that we exit with original state of  KD_GRAPHICS with which
> we started.

This test will restore the state that was set when it was executed, if it is not in the right state is not the job of this test to fix the global
state.
This changes here will make sure state is changed to what we need while executing it, the right fix for the state outside of this test is track
down what test or whatever is causing it and fix it.

> 
> > > Regards,
> > > Uma Shankar
> > > 
> > > >  }
> > > > 
> > > >  static void subtest(struct drm_info *drm, struct feature *feature,
> > > > bool suspend)
> > > > --
> > > > 2.26.2
> > > > 
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev