From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH i-g-t] tests: add pc8 Date: Tue, 6 Aug 2013 22:18:14 +0200 Message-ID: <20130806201814.GI22035@phenom.ffwll.local> References: <1375130907-4054-1-git-send-email-przanoni@gmail.com> <1375131207-4162-1-git-send-email-przanoni@gmail.com> <20130805060551.GL22035@phenom.ffwll.local> <20130805153515.GJ22035@phenom.ffwll.local> <20130806201102.GH22035@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f52.google.com (mail-wg0-f52.google.com [74.125.82.52]) by gabe.freedesktop.org (Postfix) with ESMTP id 993B3E6F63 for ; Tue, 6 Aug 2013 13:18:09 -0700 (PDT) Received: by mail-wg0-f52.google.com with SMTP id b13so767192wgh.31 for ; Tue, 06 Aug 2013 13:18:08 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130806201102.GH22035@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org Mabye I should actually cc Damien when I poke him ... -Daniel On Tue, Aug 06, 2013 at 10:11:02PM +0200, Daniel Vetter wrote: > On Tue, Aug 06, 2013 at 04:33:53PM -0300, Paulo Zanoni wrote: > > 2013/8/5 Daniel Vetter : > > > On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote: > > >> 2013/8/5 Daniel Vetter : > > >> > On Mon, Jul 29, 2013 at 05:53:27PM -0300, Paulo Zanoni wrote: > > >> The problem is: let's imagine there's some important register that we > > >> initialize when starting the driver, but then we don't touch it > > >> anymore. And this register is one of the registers that get reset when > > >> we enter PC8, but our code doesn't fix it after leaving PC8. So if you > > >> boot your machine, do something to allow PC8+ and then disallow it, > > >> the register will go back to the "reset" state (which isn't guaranteed > > >> to be 0x0, especially on display registers). Then you run tests/pc8: > > >> it reads the register (which contains the "reset" value instead of the > > >> real value set by our driver when it got loaded, because we already > > >> entered PC8+ once), enters PC8+, leaves PC8+, reads the register > > >> again, notices the value is the same and then gives us a "PASS". On > > >> the other hand, if you didn't reach PC8+ before running tests/pc8, > > >> then it would have noticed the value of the register changes. > > >> > > >> In other words: if some register gets initialized by our driver to a > > >> non-default value, but this value gets lost after we enter/leave PC8+, > > >> we will *only* be able to notice the difference when comparing a state > > >> where we *never* entered PC8+ against a state where we "already > > >> entered PC8+ at least once", because after the first time you > > >> enter/leave PC8+ you'll already have reset the register, so you'll be > > >> comparing bugged state against bugged state. > > > > > > Ok, I see your point. But imo igt testcases shouldn't (at least with the > > > testcases run by default) have such detailed knowledge of what the kernel > > > actually does with the registers. So a depency like "this test needs to be > > > run after a fresh boot when we've never entered pc8+ yet" isn't good since > > > it'll make the testcase fragile. > > > > > > Instead the test should check whether everything still works after we've > > > been in pc8+ for a bit. One exception could be w/a bits if we have a > > > common igt testcase to check for all of them. Then we could just rerun > > > that testcase. > > > > > > But if e.g. the swizzling settings get lost over pc8+ it's better to add > > > an explicit functional swizzle test here instead of checking registers. > > > > So instead of doing this, why not just make sure the very first i-g-t > > test gets us to PC8+, ant then run the full i-g-t suite after that? > > This should cover all the stuff that breaks due to us not restoring > > registers properly. Then we only have to worry about the things that > > run while PC8 is enabled, we can do that in another test. > > It would be nice if we could do this (e.g. also for re-running igt after > gpu hangs or after a suspend/resume). But I think atm QA runs testcases > essentially in a random order (or not in one we can control) so doing that > is pretty hard. > > Since the major usecase here would be to sanity-check some registers (for > w/a and sane settings, e.g. the ddi transalation buffer settings) and we > don't yet really have a good integration of the existing wa setting > checker tool I think we can just punt here. I'll add a note to my igt > wishlist about this. And of course pls keep the special test mode in your > testcase so that developers could manually check that we correctly restore > registers. > > Imo a big issue with making the w/a checker work is that it'll be a giant > pain to keep it in sync with the kernel sources. But maybe we can extend > the existing w/a sourcecode comment grabber in a clever way. Damien, any > ideas? > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch