Re: [PATCH 2/2] tests: add kms_edp_vdd_race

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Nov 11, 2013 at 04:54:32PM -0200, Paulo Zanoni wrote:
> 2013/11/11 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Nov 11, 2013 at 04:25:36PM -0200, Paulo Zanoni wrote:
> >> 2013/11/11 Daniel Vetter <daniel@ffwll.ch>:
> >> > On Mon, Nov 11, 2013 at 03:06:10PM -0200, Paulo Zanoni wrote:
> >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >>
> >> >> We recently fixed a bug where it was impossible to do I2C transactions
> >> >> on eDP panels when they were disabled. Now it should be possible to do
> >> >> these transactions when the panel is disabled, but there's a race
> >> >> condition that triggers dmesg errors if we try do do the I2C
> >> >> transactions and set a mode on the panel at the same time. This
> >> >> program should reproduce this bug and check dmesg for errors.
> >> >>
> >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > Like I've said in the previous mail I think the generic dmesg error
> >> > checking should be somewhere generic (and probably in piglit). Otherwise
> >> > the test looks good. And the naming also matches the new convention ;-)
> >>
> >> Then this test will always give a SUCCESS. Not really what I wanted :(
> >
> > It's not the only one. We have tests that only annoy the in-kernel debug
> > features like lockdep, object use-after-free and other stuff. Or all the
> > WARN backtraces from testdisplay. And very often they all "succeed".
> 
> And that's the problem I'm trying to solve. We have a solution, it's
> useful not just for me - you just gave examples of where it would be
> useful too -, yet, IMHO, you still didn't give a good technical reason
> on why you're rejecting it.
> 
> >
> > Checking dmesg in individual tests really doesn't make much sense imo
> 
> Well, IMHO it makes a lot of sense. It's even helping me write code,
> as I already explained.
> 
> 
> > and
> > needs to be somewhere where it's done for _all_ testcases.
> 
> My code is not preventing that. In fact, I think it's helping us get
> to that point.
> 
> 
> > QA already has
> > that in their own testrunner infrastructure, unfortunately that's not
> > shared with developers so we get to invent a new wheel.
> 
> I just proposed these new wheels...

Ok, lazy me finally got around to just doing it. I've sent 2 patches to
the piglit mailing list which enable dmesg checking for igt runs by
default in less than 5 lines of code. For all tests, at the subtest
granularity.

Imo this simplicity a technical reason to do it in piglit ;-)

That leaves us with your use-case of very fine-grained checking of dmesg
errors within a testcase. tbh I'm not really sold on this being that
useful, but I'd be ok with merging the helper code if you convinced it's a
great idea. One thing though which could be improved is the cleanup - imo
it's much simpler to just have an atexit handler for such helpers.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch