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

[Dave Airlie]

On 14 November 2014 06:44, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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.

Don't expose DP stuff in properties, I don't want users controlling
the parameters of the DP link in any way. I can't see any use
in userspace for controlling this stuff. So I'm happier with debugfs,
to avoid making an ABI we hate later.

Yes I do prefer we make DP validation go via the same paths,
but some parts of DP validation require things userspace shouldn't
be allowed setup, and for those we should have bypasses, everything
else should be done via normal channels.

Dave.