Re: [igt-dev] [PATCH i-g-t] tests/kms_dp_dsc: Force a full modeset when we force dsc enable

[Manasi Navare <manasi.d.navare@intel.com>]

On Wed, Apr 03, 2019 at 04:25:10PM +0300, Imre Deak wrote:
> On Tue, Apr 02, 2019 at 12:31:19PM -0700, Manasi Navare wrote:
> > DSC enable gets configured during compute_config and needs
> > a full modeset to force DSC.
> > Sometimes in between the tests, if the initial output is same as the
> > mode being set for DSC then it will not do a full modeset.
> > So we disable the output before forcing a mode with DSC enable.
> > 
> > Fixes: db19bccc1c22 ("test/kms_dp_dsc: Basic KMS test to validate VESA DSC on DP/eDP")
> > Cc: Petri Latvala <petri.latvala@intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > ---
> >  tests/kms_dp_dsc.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/kms_dp_dsc.c b/tests/kms_dp_dsc.c
> > index da93cd74..f3a61029 100644
> > --- a/tests/kms_dp_dsc.c
> > +++ b/tests/kms_dp_dsc.c
> > @@ -174,6 +174,11 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> >  				      &data->fb_test_pattern);
> >  		primary = igt_output_get_plane_type(data->output,
> >  						    DRM_PLANE_TYPE_PRIMARY);
> > +		/* Disable the output first */
> > +		igt_plane_set_fb(primary, NULL);
> 
> I think no need to reset the plane, it's enough to set PIPE_NONE to turn
> off the output.
> 
> > +		igt_output_set_pipe(data->output, PIPE_NONE);
> 
> You are missing here the additional commit needed to turn off the output
> and then set back the output to data->pipe before the commit below. The
> igt_output_set_pipe() call in run_test() won't be needed either.

Oh I looked through all the other kms tests and there we always set the fb for
PRIMARY plane to NULL then set the pipe to NULL.
But I guess if we disable to pipe output, the plane if its NULL or not shouldnt really matter.
So I should just have:

igt_plane_set_fb(primary, NULL);
igt_display_commit(&data->display)

Then set fb to the test pattern and display commit to set output to
the desired mode corerct?


So why is it that I wont need the igt_output_set_pipe(output, pipe) in run_test()?
I see that all the kms tests use this call for_each_valid_output_on_pipe iterator
> 
> > +
> > +		/* Now set the output to the desired mode */
> >  		igt_plane_set_fb(primary, &data->fb_test_pattern);
> >  		igt_display_commit(&data->display);
> >  
> > @@ -187,7 +192,7 @@ static void update_display(data_t *data, enum dsc_test_type test_type)
> >  		clear_dp_dsc_enable(data);
> 
> Instead of this we should restore the original value of
> i915_dsc_fec_support and also make sure we restore it if the test fails
> and aborts somewhere before this call. Take a look at a look at the
> various places changing debugfs entries that call
> igt_install_exit_handler() for an idea how to do this.

So save the original value of i915_dsc_fec_support() and restore that in test_cleanup() and
in the igt_fixture(), have:
kmstest_set_vt_graphics_mode();
igt_install_exit_handler()

Manasi

> 
> >  
> >  		igt_assert_f(enabled,
> > -			     "Default DSC enable failed on Connector: %s Pipe: %s",
> > +			     "\nDefault DSC enable failed on Connector: %s Pipe: %s",
> >  			     data->conn_name,
> >  			     kmstest_pipe_name(data->pipe));
> >  	} else {
> > -- 
> > 2.19.1
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev