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

[Shankar, Uma]

> -----Original Message-----
> From: Souza, Jose <jose.souza at intel.com>
> Sent: Monday, June 1, 2020 10:13 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 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.

Seems reasonable. This is:
Reviewed-by: Uma Shankar <umashankar at intel.com>

> >
> > > > 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