[Intel-gfx] [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing

[Daniel Vetter]

On Thu, Nov 13, 2014 at 7:44 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
>> > > I think it's better to expose this as drm properties on the DP
>> > > connector. This has a pile of upsides:
>> > > - We don't need to invent a new parser.
>> > > - Users can play with them to test out different theories.
>> > > - We could even use this to fix broken panels/boards without vbt
>> > > or anything using our plan to set up the initial config with a dt
>> > > firmware blob.
>> > >
>> > > So would be a lot more useful than this private interface.
>> > >
>> > > For the properties themselves I think we should go with explicit
>> > > enumerations with default value "automatic" and then an
>> > > enumeration of all values allowed by DP. For the naming of the
>> > > properties I guess something like DP_link_lanes,
>> > > DP_link_clock, ... would look pretty. The properties should be in
>> > > dev->mode_config (since they're reallly generic) and I think we
>> > > want one function to register them all in one go in the
>> > > drm_dp_helper.c library. Maybe we could even put the values into
>> > > the existing dp source struct so that we don't have to reinvent
>> > > the decode logic every single time.
>> >
>> > I disagree. I do prefer the debugfs thing. Think about all the
>> > examples of people that break their systems by passing
>> > i915.enable_foo=1 or i915.enable_rc6=7 and then open bug reports...
>> > With debug stuff exposed as DP properties, we're going to increase
>> > that problem, and in a way that will make it even harder to detect.
>> > I really really think these things should be exposed only to the
>> > debugfs users, because then if the debugfs file is closed, we can
>> > just ignore all the arguments and keep doing "normal" unaffected
>> > modesets.
>> >
>> > If we don't want the parser, maybe we can make it a binary protocol:
>> > we'lll still have to parse it, but it can get much easier.
>>
>> We already expose piles of funky stuff to users in these properties
>> (e.g. audio on hdmi). And contrary to the module options most of
>> these will just result in black screens if you don't understand what
>> you're doing, so I think the risk is low. There's also a bit of an
>> issue with the current interface as it's not clear which line
>> corresponds to which DP interface. Using properties would solve this.
>> Also these options have a much more direct impact - if you set them
>> and the screen goes dark then the user hopefully realizes that this
>> is something they shouldn't touch.
>>
>> For fbc and rc6 and all those the problem is that nothing really
>> happens, the system just becomes a bit more unstable and randomly
>> crashes. Which users then report.
>>
>> But If you're really concerned about users doing crappy stuff we
>> could add a module option to drm_dp_helper.c which hides these, and
>> then taint the kernel if any user sets them. But I really think this
>> is overkill. -Daniel
>
> Just chatted with Todd about the state of this patchset.  He's going to
> post an update today or tomorrow and summarize the feedback from July
> ad what he's done to address it, add changelogs, and address outstanding
> review feedback from this posting.
>
> I like the idea of exposing some DP stuff like lane count, preemph,
> etc, as new properties, but I don't think it's reasonable to block the
> testing stuff on it, for a few reasons:
>   1) we should think pretty hard about how we want new ABI like
>      this exposed
>   2) the compliance spec changes pretty regularly, so keeping an
>      unstable interface for it in debugfs may make sense anyway
>   3) even if we decide to move the userland test code over to
>      properties in the future, the fact that debugfs is unstable means
>      we can drop it at that time
>
> So I asked Todd to file a JIRA for the properties feature request,
> since it's definitely worth looking at.
>
> Which isn't to say the current interface doesn't have issues, just that
> Todd shouldn't rewrite things again to include a new, stable ABI,
> probably keeping compliance testing and additional DP test coverage out
> of the tree for even longer.
>
> Thoughts, objections?

Ok, so from a really, really high-level perspective the design review
from my side in July had one primary goal and a corollary from that:

- DP validation should exercise the same code paths (where possible)
than what actual customers use in DP mode sets. If we implement
completely separate codepaths (which the first iteration partially
did) then we validate a separate driver and not actually what
customers run. Which is fairly pointless imo.

- Corollary was that this means we should drive the full kernel stack
and so control the testing from userspace using the same interfaces as
any userspace would use for a modeset. And if that interface has gaps,
or our kernel side DP driver has gaps we'd need to fill them. debugfs
should only serve as the testing sideband between the DP tester (sink
device) and the userspace test program. So the kernel just passes
stuff back&forth like test requests (e.g. "pls set this mode on now
and show test pattern") and return test results (e.g. "I've read the
edid now, here's the checksum"). Of course for some test requests we
can directly answer them from the kernel (e.g. "tell me how many dp
aux failures you've seen"). Examples completely made up ;-)

So from that perpective adjusting DP link paramters is either a new
tuning bit, which really should be generally available. Or it
indicates a bug/omission in our DP link training or re-training logic.
In either case I don't think it's really pure test-related sideband
date.

I guess I didn't make this 100% clear in the original design review
what my goal is, or maybe Todd misunderstood things a bit.

Aside from all this (and now with my community hat on) just adding
code to get a sticker (labelled "passed DP validation") which is
separate code and not used by actual users is imo not useful for
merging upstream. But that's just my own opinion, not sure what's
Dave's stance here or whether there's much precendence either way.

So short answer: I still think exposing this with properties is the
right approach, presuming we really need it (and it's not just to
paper over a deficient link training logic in the kernel). I also
think it'll be less code since we can simplify the debugfs option
parser.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch