Re: [igt-dev] [PATCH i-g-t 2/2] lib/psr: Add support to new modified i915_edp_psr_status output

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Fri, 2018-10-05 at 15:59 -0700, Souza, Jose wrote:
> On Fri, 2018-10-05 at 15:42 -0700, Dhinakaran Pandiyan wrote:
> > On Thu, 2018-10-04 at 13:59 -0700, José Roberto de Souza wrote:
> > > The kernel patch 'drm/i915: Refactor PSR status debugfs' 
> > 
> > Was this sent to the list already? I might have missed it.
> > 
> > > changed the
> > > output of i915_edp_psr_status, so adding support to the new
> > > output
> > > here while keeping the support to the old one for a while.
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  lib/igt_psr.c | 9 ++++++---
> > >  1 file changed, 6 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index f36496fd..a48c9232 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -32,8 +32,10 @@ static bool psr_active(int debugfs_fd, bool
> > > check_active)
> > >  
> > >  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
> > >  				sizeof(buf));
> > > -	active = strstr(buf, "HW Enabled & Active bit: yes\n") &&
> > > -		(strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
> > > +
> > > +	active = (strstr(buf, "HW Enabled & Active bit: yes\n") ||
> > > +		  strstr(buf, "Source PSR ctl: enabled")) &&
> > > +		 (strstr(buf, "SRDENT") || strstr(buf, "SLEEP"));
> > >  	return check_active ? active : !active;
> > >  }
> > >  
> > > @@ -136,5 +138,6 @@ bool psr_supported(int debugfs_fd)
> > >  
> > >  	igt_debugfs_simple_read(debugfs_fd, "i915_edp_psr_status", buf,
> > >  				sizeof(buf));
> > > -	return strstr(buf, "Sink_Support: yes\n");
> > > +	return strstr(buf, "Sink_Support: yes\n") ||
> > > +	       strstr(buf, "Sink support: yes");
> > 
> > This results in unnecessary commits. I think what can do is
> 
> You don't want to keep support for both for a while? Someone recently
> sent a patch fixing PSR test for old kernels I think is worthy
> keeping
> both at least for 1 or 2 kernel releases.
> 
> > 1) send unrelated kernel fixes that pass CI and merge them.
> > 2) send new debugfs changes and corresponding IGT changes to the
> > list
> > for review.
> > 3) merge igt patches after kernel debugfs changes are reviewed.
> > 4) rerun tests on the reviewed kernel patches and merge them.
> 
> I'm okay in doing this way but it will just waste CI time running a
> test that we know that will fail.
The downside is we have confusing commits in git history. And debugfs
is not kernel ABI from what I understand. In any case, it is better to
not modify tests until the corresponding kernel patches are finalized. 

For instance, these changes - "Sink_Support" -> "Sink support" and "HW
Enabled & Active bit" -> "Source PSR ctl" aren't on the intel-gfx list
yet.



> 
> Could you then merge patch 1 to 4 maybe 7 too from 
> https://patchwork.freedesktop.org/series/50526/ in kernel? 
> 
> Also the first one in this series is not related.
Done, thanks for the patches.

-DK
> 
> > 
> > 
> > 
> > 
> > >  }

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev