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 18:10:09 +0200	[thread overview]
Message-ID: <20190403161009.GN2665@phenom.ffwll.local> (raw)
In-Reply-To: <20190403143410.GM2665@phenom.ffwll.local>

On Wed, Apr 03, 2019 at 04:34:10PM +0200, Daniel Vetter wrote:
> 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

Correction: I was blind, it's all there in the igt_fixture. Apologies.

> 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

For the test itself I think we should instead drop the dpms off. There's
not much to test really if we suspend with the display off. Or at least we
should check both ways I think, but suspending with display on is a lot
more interesting. And for that case the kernel should resume the display
for us again, not explicit dpms on needed.
-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