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

[Manasi Navare]

On Thu, Apr 04, 2019 at 02:15:06AM +0300, Imre Deak wrote:
> 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.

So just call igt_install_exit_handler() without pasisng any function pointer to do
just the default cleanups?

Manasi

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