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).