Re: [igt-dev] [RFC i-g-t 1/1] i915/pm_backlight: turn on dpms before backlight fade out and fade in

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org>,
	"Taylor, Clinton A" <clinton.a.taylor@intel.com>,
	"Yadav, Jyoti R" <jyoti.r.yadav@intel.com>
Subject: Re: [igt-dev] [RFC i-g-t 1/1] i915/pm_backlight: turn on dpms before backlight fade out and fade in
Date: Wed, 3 Apr 2019 16:34:10 +0200	[thread overview]
Message-ID: <20190403143410.GM2665@phenom.ffwll.local> (raw)
In-Reply-To: <155427749709.24691.14658564444185406872@skylake-alporthouse-com>

On Wed, Apr 03, 2019 at 08:44:57AM +0100, Chris Wilson wrote:
> Quoting Chegondi, Harish (2019-04-03 01:47:09)
> > On Thu, 2019-03-14 at 07:46 +0000, Chris Wilson wrote:
> > > Quoting Harish Chegondi (2019-03-14 06:41:48)
> > > > backlight fade with suspend test turns off dpms which turns off
> > > > the edp backlight and panel. Then it does a runtime suspend,
> > > > system suspend and resume. After resume, it does a fade out and
> > > > fade in of the backlight brightness. From the dmesg logs of the
> > > > ci tests it appears that the test is setting the brightness
> > > > even before the edp panel and backlight are turned on resuilting
> > > > in the brightness values written and read to be different.
> > > > Turn on the dpms which turns on the edp panel and backlight
> > > > before backlight fade out and fade in. With this change the
> > > > fade_with_suspend test passes.
> > 
> > Chris,
> > 
> > My commit message was confusing. I will redo the commit message in the
> > next version of my patch.
> > 
> > > 
> > > But is it legal for the kernel to that? Is the kernel meant to
> > > restore
> > > the previous configuration upon resume or leave it to userspace? What
> > > about for fbcon?
> > 
> > Yes, the kernel is meant to restore the previous configuration upon
> > resume.
> 
> Are you sure?
> 
> We send a hotplug to userspace to tell them to restore the configuration
> as they see fit (because the configuration will often change). If fbcon
> is active, it restores the fbcon screen which can just be the consequence
> of handling its hotplug event.

Bunch of comments from irc discussion:
- kernel is supposed to restore
- but since the test does a dpms off, we do need to do a dpms on
- but that's silly, because defacto that means we test nothing

Worse:
- the test doesn't shut up fbcon, so uncontrolled env
- it also doesn't set up a mode explicitly, so again uncontrolled env

Rodrigo shouldn't have r-b'ed this imo. There's a lot more work needed
here than duct-taping a few more commands on top, I think starting with
a) what exactly are we trying to test here (testing fancy effects isn't
useful in the context of CI, no one is looking)
b) have we set up the right starting conditions to actually run that test

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev