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

[Imre Deak]

On Wed, Apr 03, 2019 at 04:11:49PM -0700, Manasi Navare wrote:
> On Thu, Apr 04, 2019 at 02:02:42AM +0300, Imre Deak wrote:
> > On Wed, Apr 03, 2019 at 03:48:24PM -0700, Manasi Navare wrote:
> > > On Thu, Apr 04, 2019 at 12:42:33AM +0300, Imre Deak wrote:
> > > > On Wed, Apr 03, 2019 at 01:49:44PM -0700, Manasi Navare wrote:
> > > > > [...]
> > > > > Currently we have inh i915_dsc_fec_support_show:
> > > > > DSC_enabled:
> > > > > DSC_sink_support:
> > > > > FEC_sink_support:
> > > > 
> > > > Yes, and after the kernel change you'll have an additional
> > > >   
> > > >   Force DSC enable: yes/no
> > > > 
> > > > line here.
> > > > 
> > > > > 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?
> > > > 
> > > > I'm not sure what you mean. Currently "1" or "0" is written to the
> > > > debugfs file which will set intel_dp->force_dsc_enable. This doesn't
> > > > need to be changed at all, no need to rewrite the whole file.
> > > > 
> > > > > So in IGT also I will have to parse the file and write 1 for
> > > > > force_dsc_enable entry?
> > > > 
> > > > You can get the original value of it similarly to how it's done already
> > > > for 'DSC_Sink_Support', by reading the file and doing
> > > > 
> > > > 	strstr(buf, "Force DSC enable: yes");
> > > > 
> > > > on the content.
> > > > 
> > > > > 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?
> > > > 
> > > > Only force_dsc_enable need to be restored, the only value that you
> > > > change by writing to the debugfs file. All the rest are calculated
> > > > values that you don't need to care about.
> > > 
> > > But now that I rething then, force_dsc_enable is always 0, it s a variable
> > > that we use to force DSC only if IGT asks to, so for test we force it to 1
> > > and then after the test always clear it since the original value is always 0.
> > > So why do we need a separate restore() for just force_dsc_enable if the value
> > > is always 0.
> > 
> > It's not necessarily 0. Before the test starts someone could've written
> > 1 to that file.
> 
> Hmm, so now the use of this force_dsc_enable in kernel is only for
> this specific IGT and the kernel doesnt touch this. But because in the
> future the kernel might set it to true, we want to make sure that the
> IGT doesnt change the kernel behaviour after the test is run, is my
> understanding correct?  So basically adding that now for future
> proofing it?

No, anybody running tests manually could set it and expect it to be
preserved.

> Also on the igt_install_exit_handler(), should the test_cleanup be now
> part of this handler?

No need for that, any required modeset specific cleanup will be handled
automatically.

> Manasi
> 
> > 
> > > 
> > > Manasi
> > > 
> > > > 
> > > > --Imre