Re: [igt-dev] [PATCH i-g-t v4 1/4] test/psr: Skip PSR2 tests when panel resolution is bigger than PSR2 HW supports

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jan 24, 2019 at 04:09:35PM -0800, Souza, Jose wrote:
> On Thu, 2019-01-24 at 14:06 -0800, Rodrigo Vivi wrote:
> > On Thu, Jan 24, 2019 at 11:02:51AM -0800, Dhinakaran Pandiyan wrote:
> > > On Thu, 2019-01-24 at 10:20 -0800, Souza, Jose wrote:
> > > > On Wed, 2019-01-23 at 22:39 -0800, Rodrigo Vivi wrote:
> > > > > On Wed, Jan 23, 2019 at 06:25:07PM -0800, Dhinakaran Pandiyan
> > > > > wrote:
> > > > > > On Wed, 2019-01-23 at 15:56 -0800, José Roberto de Souza
> > > > > > wrote:
> > > > > > > If resolution that will be used in PSR tests is bigger than
> > > > > > > the
> > > > > > > resolution that PSR2 HW supports, PSR2 will not be enabled
> > > > > > > so
> > > > > > > those
> > > > > > > tests should be skiped. Most of eDP panels only supports
> > > > > > > one
> > > > > > > resolution that is why we don't even try to use other
> > > > > > > resolution.
> > > > > > > 
> > > > > > > BSpec: 7713
> > > > > > 
> > > > > > I don't like the fact that we are re-implementing driver code
> > > > > > in
> > > > > > the
> > > > > > test to avoid false-positives. We should really replace the
> > > > > > panel
> > > > > > on
> > > > > > the WHL machine with a panel that is expected to work. 
> > > > > > 
> > > > > 
> > > > > I fully agree. The debugfs should be auto-suficient.
> > > > > 
> > > > > In the past it was the idea of Source OK. if sink support yes
> > > > > and
> > > > > source ok
> > > That had a problem too, "Source OK" was set after driver completed
> > > all
> > > the checks and just before enabling PSR2. In effect, it started
> > > becoming a duplicate of the "Enabled" field and we weren't able to
> > > catch if the driver checks before psr_enable() were wrong.
> > 
> > yeap, this is true indeed...
> > 
> > But we need to find a way where the debugfs tells us if
> > conditions are not match and enable is not expected to
> > avoid having to reimplement all the conditions twice.
> 
> I could add a reason field to debugfs and like we have for FBC(fbc-
> >no_fbc_reason)
> 
> What do you think?

well, I always disliked the no_fbc_reason, but it is better than duplicating
the code and I don't have better idea ;)

> 
> > 
> > > 
> > > > > and we tried to enabled but it is not getting active,
> > > > > than we have a bug somewhere.
> > > > > 
> > > > > I don't know the current status of the psr debugfs anymore, but
> > > > > I believe the test case should infer this information from
> > > > > there
> > > > > somehow instead of re-implement the driver code here.
> > > > > 
> > > > > > But, as a temporary measure to make CI happy and enable PSR2.
> > > > > 
> > > > > why is this only affecting PSR2 case?
> > > > 
> > > > PSR2 has a maximum resolution suported and the only resolution
> > > > that
> > > > CI
> > > > eDP panel supports is bigger than that.
> > > > 
> > > > > > Reviewed-by: Dhinakaran Pandiyan <
> > > > > > dhinakaran.pandiyan@intel.com>
> > > > > > 
> > > > > > Rodrigo, what's your opinion on this?
> > > > > > 
> > > > > > -DK
> > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > ---
> > > > > > >  tests/kms_psr.c | 21 +++++++++++++++++++--
> > > > > > >  1 file changed, 19 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tests/kms_psr.c b/tests/kms_psr.c
> > > > > > > index 3e16a6bf..4792e158 100644
> > > > > > > --- a/tests/kms_psr.c
> > > > > > > +++ b/tests/kms_psr.c
> > > > > > > @@ -445,13 +445,30 @@ int main(int argc, char *argv[])
> > > > > > >  		igt_require_f(sink_support(&data, PSR_MODE_1),
> > > > > > >  			      "Sink does not support PSR\n");
> > > > > > >  
> > > > > > > -		data.supports_psr2 = sink_support(&data,
> > > > > > > PSR_MODE_2);
> > > > > > > -
> > > > > > >  		data.bufmgr =
> > > > > > > drm_intel_bufmgr_gem_init(data.drm_fd,
> > > > > > > 4096);
> > > > > > >  		igt_assert(data.bufmgr);
> > > > > > >  		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
> > > > > > >  
> > > > > > >  		display_init(&data);
> > > > > > > +
> > > > > > > +		data.supports_psr2 = sink_support(&data,
> > > > > > > PSR_MODE_2);
> > > > > > > +		if (data.supports_psr2) {
> > > > > > > +			uint32_t devid =
> > > > > > > intel_get_drm_devid(data.drm_fd);
> > > > > > > +			uint16_t max_h = 0, max_v = 0;
> > > > > > > +			int gen = intel_gen(devid);
> > > > > > > +
> > > > > > > +			if (gen >= 10 || IS_GEMINILAKE(devid))
> > > > > > > {
> > > > > > > +				max_h = 4096;
> > > > > > > +				max_v = 2304;
> > > > > > > +			} else if (gen == 9) {
> > > > > > > +				max_h = 3640;
> > > > > > > +				max_v = 2304;
> > > > > > > +			}
> > > > > > > +
> > > > > > > +			if (data.mode->hdisplay > max_h ||
> > > > > > > +			    data.mode->vdisplay > max_v)
> > > > > > > +				data.supports_psr2 = false;
> > > > > > > +		}
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	for (data.op_psr_mode = PSR_MODE_1; data.op_psr_mode <=
> > > > > > > PSR_MODE_2;
> > > 
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev


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