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

During my investigation, I already tried removing DPMS off and verified
that the system suspend is turning off the backlight and the system
resume is properly turning on the backlight. I captured the test
changes and the observation in the fdo:

https://bugs.freedesktop.org/show_bug.cgi?id=107820#c6

I will send out this patch.

Thanks for your feedback.

-Harish.