Sunday, August 19, 2012

New Modeset Code

drm/i915.ko is gearing up to gain a new modeset code, to finally move away from the crtc helper code (in drm/drm_crtc_helper.c) used by (up to now) all kms drivers. As a quick reference I'll detail the motivation and design of the new code a bit here (mostly stitched together from patchbomb announcements and commits introducing the new concepts).

The crtc helper code has the fundamental assumption that encoders and crtcs can be enabled/disabled in any order, as long as we take care of depencies (which means that enabled encoders need an enabled crtc to feed them data, essentially).

Our hw works differently. We already have tons of ugly cases where crtc code enables encoder hw (or encoder->mode_set enables stuff that should only be enabled in enocder->commit) to work around these issues. But on the disable side we can't pull off similar tricks - there we actually need to rework the modeset sequence that controls all this. And this is also the real motivation why I've finally undertaken this rewrite: eDP on my shiny new Ivybridge Ultrabook is broken, and it's broken due to the wrong disable sequence ...

The new code introduces a few interfaces and concepts:
  • Add new encoder->enable/disable functions which are directly called from the crtc->enable/disable function. This ensures that the encoder's can be enabled/disabled at a very specific in the modeset sequence, controlled by our platform specific code (instead of the crtc helper code calling them at a time it deems convenient).
  • Rework the dpms code - our code has mostly 1:1 connector:encoder mappings and does support cloning on only a few encoders, so we can simplify things quite a bit.
  • Also only ever disable/enable the entire output pipeline. This ensures that we obey the right sequence of enabling/disabling things, trying to be clever here mostly just complicates the code and results in bugs. For cloneable encoders this requires a bit of special handling to ensure that outputs can still be disabled individually, but it simplifies the common case.
  • Add infrastructure to read out the current hw state. No amount of careful ordering will help us if we brick the hw on the initial modeset setup. Which could happen if we just randomly disable things, oblivious to the state set up by the bios. Hence we need to be able to read that out. As a benefit, we grow a few generic functions useful to cross-check our modeset code with actual hw state.
With all this in place, we can copy&paste the crtc helper code into the drm/i915 driver and start to rework it:
  • As detailed above, the new code only disables/enables an entire output pipe. As a preparation for global mode-changes (e.g. reassigning shared resources) it keeps track of which pipes need to be touched by a set of bitmasks.
  • To ensure that we correctly disable the current display pipes, we need to know the currently active connector/encoder/crtc linking. The old crtc helper simply overwrote these links with the new setup, the new code stages the new links in ->new_* pointers. Those get commited to the real linking pointers once the old output configuration has been torn down, before the ->mode_set callbacks are called.
  • Finally the code adds tons of self-consistency checks by employing the new hw state readout functions to cross-check the actual hw state with what the datastructure think it should be. These checks are done both after every modeset and after the hw state has been read out and sanitized at boot/resume time. All these checks greatly helped in tracking down regressions and bugs in the new code.
With this new basis, a lot of cleanups and improvements to the code are now possible (besides the DP fixes that ultimately made me write this), but not yet done:
  • I think we should create struct intel_mode and use it as the adjusted mode everywhere to store little pieces like needs_tvclock, pipe dithering values or dp link parameters. That would still be a layering violation, but at least we wouldn't need to recompute these kinds of things in intel_display.c. Especially the port bpc computation needed for selecting the pipe bpc and dithering settings in intel_display.c is rather gross.
  • In a related rework we could implement ->mode_valid in terms of ->mode_fixup in a generic way - I've hunted down too many bugs where ->mode_valid did the right thing, but ->mode_fixup didn't. Or vice versa, resulting in funny bugs for user-supplied modes.
  • Ditch the idea to rework the hdp handling in the common crtc helper code and just move things to i915.ko. Which would rid us of the ->detect crtc helper dependencies.
  • LVDS wire pair and pll enabling is all done in the crtc->mode_set function currently. We should be able to move this to the crtc_enable callbacks (or in the case of the LVDS wire pair enabling, into some encoder callback).
Last, but not least, this new code should also help in enabling a few neat features: The hw state readout code prepares (but there are still big pieces missing) for fastboot, i.e. avoiding the inital modeset at boot-up and just taking over the configuration left behind by the bios. We also should be able to extend the configuration checks in the beginning of the modeset sequence and make better decisions about shared resources (which is the entire point behind the atomic/global modeset ioctl).

17 comments:

  1. Thank you for destroying all existing porting efforts.

    ReplyDelete
  2. Great!
    When can we expect a FreeBSD version of that driver? Will you port it?

    ReplyDelete
  3. Oh year. It took about 18 month until the "old" Intel GPU stuff was ported from Linux to FreeBSD. And now, just in time for the first FreeBSD release supporting Westmere to Ivy Bridge, the next sweeping changes come in. I understand that GPU programing often requires massive changes due to fast moving hardware development and so on, but why is it necessary that everytime when this happens the already working stuff is broken again and again? Often including the API to xf86-video-intel. The world doesn't end with Linux!

    What about official support for Intel GPUs under FreeBSD? Of course FreeBSD is not as big as Linux but has nevertheless a strong userbase investing a lot of money into hardware. And good supported Intel GPUs are a strong argument for buying Intel hardware... Last but not least support for a second platform can bring more stability into a codebase by exhibit bugs and and other problems.

    ReplyDelete
  4. Thanks for doing all this great work on the GPU drivers.
    Are you considering also to deliver drivers for other platforms than linux? The different BSD-Platforms (OpenBSD, NetBSD nad FreeBSD are also in urgent need of working drivers.

    ReplyDelete
  5. I concur with everyone who already commented on this, the *BSDs are really, well... screwed by such seemingly arbitrary rewriting. It already took 18 months to port the exisiting code to FreeBSD, and with this rewrite.... I just hope it's not that difficult to port to different operating systems.

    ReplyDelete
  6. Would be nice to see some effort in supporting *BSD too. I like Intel CPUs a lot, but your GPU is dead in the water with the advent of such actions.

    ReplyDelete
  7. I would advise the BSD guys to bug Intel about funding support, seriously. Flood their website with complaints. I think the BSD effort is doomed without at least one internal developer, and slowing down Linux progress for the sake of BSD simply is not an option.

    ReplyDelete
  8. Well, let me clarify a bit first: This patch set is not a complete rewrite of the modeset code, all the low-level hw-frobbing parts are still all the same. The code mostly just reworks the interfaces a bit and makes the logic that drives the modeset sequence intel-specific. And we need that, because with the current generic framework we flat-out can't fix a few bugs.

    So this patch series isn't more work for porting than any other patch series, and if you can't keep up with the development pace of drm/i915 with porting, you have a serious problem no matter what.

    Now I don't mind keeping the *bsd folks in mind a bit more if I actually get something back from such an effort. But right now the only feedback I've seen outside from Linux is from Solaris people, and Intel itself is pouring the money into that.

    I haven't heard any useful feedback from any *bsd folks in years (wrt the intel driver), and as long as it's just bitching&moaning I just keep on presuming that graphics on *bsd is dead and won't bother.

    </rant>

    ReplyDelete
  9. Hello danvet,

    Thanks for clearing up the issue a bit, it doesn't sound as bad as probably many of us thought.
    I fully agree with and understand you on the point of getting no feedback, I would most probably do the same, really.
    I'm no developer of any *BSD, so my question is: can I as an ordinary user give you any useful feedback? That might sound stupid, but I really want to help you if it makes you mind the *BSDs a bit more, as you said.

    Sincerely,

    D.Rom

    ReplyDelete
  10. Hi Dave! As a user there's not much you can do here - since we're no experts on the *BSD kernel code we direct the occasional user away to bsd-specific support channels. Developers popping up and discussion things would be much more interesting, but the last guy was Owen Ainsworth who work on a OpenBSD port a few years back. E.g. I've only learned of the FreeBSD porting effort recently (because a user asked about it) and afaik the guy who did all the porting never showed up anywhere. That kind of disconnect with the *BSD community is what I'm missing here first of all ...

    ReplyDelete
  11. Thanks for your response, Daniel!
    It's really a shame that there is such a disconnect between developers like you and developers of the *BSDs. And I think something should change in that respect.
    Afaik, the guy who did all the porting of the modesetting code is Konstantin Belousov(kib@freebsd.org).
    Maybe I can achieve something if I just contact everbody on the FreeBSD mailing list and do something like a 'call for more cooperation'. That may be in vain, but at least it could start a discussion which may drive things further.
    In the end, I think everybody can just gain something out of it, and maybe they will see it the same way.

    ReplyDelete
  12. Actually I've just noticed that Konstantin _did_ pop up a few times on the intel-gfx mailing list. So it looks like things are working, I've simply missed the (little) bsd signal in all the linux noise ;-)

    I guess that only leaves the issue that bsd simply has too few people working on low-level graphics drivers to sustain a porting effort. I can't really fix that ...

    ReplyDelete
  13. Glad to hear that he did :)

    Yep, manpower is not a strong point of BSD development, and I understand that you can't do much about it.
    Still, thanks for talking with me, much appreciated :)

    ReplyDelete
  14. Nevermind bitching bsd folks - porting to exotic OS is not your job.
    But if those efforts would make bugs like https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-intel/+bug/1052000 disappear than it's totally justified.

    ReplyDelete
    Replies
    1. Unfortunately that bug is a different issue - the BIOS doesn't set up the eDP panel in this case, and we can't set it up without the BIOS' help. I've worked on some patches which will be merged into 3.8, but on the UX31A they can only enable the backlight, the panel is still non-functional. But on some other machines it now does work. It seems to be something timing-related though, since on very rare occasions I could get it to correctly light up.

      The eternal frustration which is called (e)DP ...

      Delete
    2. I dare to disagree - with latest bios and ubuntu's X stack I can actually use external eDP panel (see the bug update). The strange thing is that Xorg only show part of main screen (it has better resolution) on external monitor. I mean I can put windows into the part of the screen and even make them "full-screen" but the expected behavior is to have 2 separate distinct screens - not the one embedded into another in some twisted way.

      If it were bios issue I presume that external panel would be completely non-functional but it actually shows stuff. Of course it shows wrong not what user would expect and corrupts main screen (it suddenly has different background wallpaper for example) - but it does work.

      I'm no specialist in xorg internals but I would be delighted to help with testing :)

      Delete
    3. So it's just me with the unlucky panel :(

      For the default configuration that's just an issue with your login manager and desktop enviroment - if you can set it up with xrandr. Luckily wrestling with those things is not my turf ...

      Delete