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

Wed Apr 3 20:49:44 UTC 2019

On Wed, Apr 03, 2019 at 11:34:05PM +0300, Imre Deak wrote:
> On Wed, Apr 03, 2019 at 01:17:17PM -0700, Manasi Navare wrote:
> > On Wed, Apr 03, 2019 at 10:33:41PM +0300, Imre Deak wrote:
> > > On Wed, Apr 03, 2019 at 11:20:58AM -0700, Manasi Navare wrote:
> > > > 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 at intel.com>
> > > > > > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > > > > Cc: Imre Deak <imre.deak at intel.com>
> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare at 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);
> > > 
> > > You need to set PIPE_NONE before the commit and then the actual pipe
> > > before the rest of the sequence, like:
> > > 
> > > 	igt_debug("DSC is supported on %s\n", data->conn_name);
> > > 
> > > 	igt_output_set_pipe(data->output, PIPE_NONE);
> > > 	igt_display_commit(&data->display);
> > > 
> > > 	force_dp_dsc_enable(...);
> > > 
> > > 	igt_output_set_pipe(data->output, data->pipe);
> > > 	igt_create_pattern_fb(...);
> > > 	primary = ...
> > > 	igt_plane_set_fb(...);
> > > 	igt_display_commit(&data->display);
> > 
> > Yea, got this part.
> > Testing is in progress.
> > 
> > > 
> > > > 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
> > > 
> > > Because you will anyway set it to PIPE_NONE right afterwards in
> > > update_display().
> > 
> > Yup got it.
> > 
> > > 
> > > > > > +
> > > > > > +		/* 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()
> > 
> > I think this part will require lot of changes in the debugfs code in
> > the kernel and also the IGT, since now I just write a 1 to the node to
> > force dsc enable and then a 0 to clear it and in the kernel we accept
> > kstr_to_bool to get a value in this node to set force_dsc_enable() to
> > true or false.  But now basically the sysfs_write in IGT will need to
> > write the entire original file that was read from
> > i915_dsc_fec_support() and in the kernel we will need to accept the
> > whole file and then parse the DSC_enabled field to take its value to
> > use it for force_dsc_enable.
> > 
> > Is all this what you are suggesting?
> 
> Hm, no way to read back the actual debugfs value, that's weird. Then
> yes, I'd suggest adding a line to i915_dsc_fec_support_show() printing
> there intel_dp->force_dsc_en too.

Currently we have inh i915_dsc_fec_support_show:
DSC_enabled:
DSC_sink_support:
FEC_sink_support:

And this node we open as a read only first and then as write node and force_dsc_enable()
from IGT just writes a 1 to this, so you are saying that in the kernel _write() node,
I accept this and write to force_dsc_enable entry in this file instead of rewriting the whole file?
So in IGT also I will have to parse the file and write 1 for force_dsc_enable entry?

I still cant figure out how the restore() function work, we write the stored_values and write
for each of the entries in the file? And then in the kernel debugfs_write, get these and write back
to each entry?

Are there any existing examples in kernel debugfs and IGT where we do this?

Manasi

> 
> > And the exit_handler() that we install in that we call this test_cleanup()?
> 
> The exit handler will be called automatically when the process exits for
> whatever reason.
> 
> > 
> > Manasi
> > 
> > > 
> > > Save the original value in force_dp_dsc_enable() and restore it in a new
> > > restore_dp_dsc_enable() which you call instead of clear_dp_dsc_enable()
> > > in update_display().
> > > 
> > > > and in the igt_fixture(), have:
> > > > kmstest_set_vt_graphics_mode();
> > > > igt_install_exit_handler()
> > > 
> > > The exit handler should be installed only after saving the original
> > > value, so the handler will have the proper value to restore. Save/install
> > > the exit handler only once even if force_dp_dsc_enable() is called
> > > multiple times.
> > > 
> > > > 
> > > > 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
> > > > > > 