- 06 Sep, 2012 40 commits
-
-
Daniel Vetter authored
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). Tested-by:
Jani Nikula <jani.nikula@intel.com> Tested-by:
Ben Widawsky <ben@bwidawsk.net> Tested-by:
Damien Lespiau <damien.lespiau@intel.com> Tested-by:
Rodrigo Vivi <rodrigo.vivi@gmail.com> Acked-by:
Chris Wilson <chris@chris-wilson.co.uk> Tested-by:
Vijay Purushothaman <vijay.a.purushothaman@intel.com> Acked-by:
Paulo Zanoni <paulo.r.zanoni@intel.com> Acked-by:
Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch>
-
Daniel Vetter authored
Now that we have solid modeset state tracking and checking code in place, we can do the Full Monty also after dpms calls. Reviewed-by:
-
Daniel Vetter authored
... let's see whether this catches anything earlier and I can track down a few bugs. v2: Add more checks and also add DRM_DEBUG_KMS output so that it's clear which connector/encoder/crtc is being checked atm. Which proved rather useful for debugging ... v3: Add a WARN in the common encoder dpms function, now that also modeset changes properly update the dpms state ... v4: Properly add a short explanation for each WARN, to avoid the need to correlate dmesg lines with source lines accurately. Suggested by Chris Wilson. v5: Also dump (expected, found) for state checks (or wherever it's not apparent from the test what exactly mismatches with expectations). Again suggested by Chris Wilson. v6: Due to an issue reported by Paulo Zanoni I've noticed that the encoder checking is by far not as strict as it could and should be. Improve this. Reviewed-by:
-
Daniel Vetter authored
Since this only calls crtc helper functions, of which a shocking amount are NULL. Now the curious thing is how the new modeset code worked with this function call still present: Thanks to the hw state readout and the suspend fixes to properly quiescent the register state, nothing is actually enabled at resume (if the bios doesn't set up anything). Which means resume_force_mode doesn't actually do anything and hence nothing blows up at resume time. The other reason things do work is that the fbcon layer has it's own resume notifier callback, which restores the mode. And thanks to the force vt switch at suspend/resume, that then forces X to restore it's own mode. Hence everything still worked (as long as the bios doesn't enable anything). And we can just kill the call to resume_force_mode. The upside of both this patch and the preceeding patch to quiescent the modeset state is that our resume path is much simpler: - We now longer restore bogus register values (which most often would enable the backlight a bit and a few ports), causing flickering. - We now longer call resume_force_mode to restore a mode that the fbcon layer would overwrite right away anyway. Signed-off-by: -
Daniel Vetter authored
We need this to avoid confusing the hw state readout code with the cpt pch plls at resume time: We'd read the new pipe state (which is disabled), but still believe that we have a life pll connected to that pipe (from before the suspend). Hence properly disable pipes to clear out all the residual state. This has the neat side-effect that we don't enable ports prematurely by restoring bogus state from the saved register values. Reviewed-by:
-
Daniel Vetter authored
With this change we can (finally!) rip out a few of the temporary hacks and clean up a few other things: - Kill intel_crtc_prepare_encoders, now unused. - Kill the hacks in the crtc_disable/enable functions to always call the encoder callbacks, we now always call the crtc functions with the right encoder -> crtc links. - Also push down the crtc->enable, encoder and connector dpms state updates. Unfortunately we can't add a WARN in the crtc_disable callbacks to ensure that the crtc is always still enabled when disabling an output pipe - the crtc sanitizer of the hw readout path can hit this when it needs to disable an active pipe without any enabled outputs. - Only call crtc->disable if the pipe is already enabled - again avoids running afoul of the new WARN. v2: Copy&paste our own version of crtc_in_use, too. v3: We need to update the dpms an encoder->connectors_active states, too. v4: I've forgotten to kill the unconditional encoder->disable calls in the crtc_disable functions. v5: Rip out leftover debug printk. v6: Properly clear intel_encoder->connectors_active. This wasn't properly cleared when disabling an encoder because it was no longer on the new connector list, but the crtc was still enabled (i.e. switching the encoder of an active crtc). Reported by Jani Nikula. v7: Don't clobber the encoder->connectors_active state of untouched encoders. Since X likes to first disable all outputs with dpms off before setting a new framebuffer, this hit a few warnings. Reported by Paulo Zanoni. v8: Kill the now stale comment warning that intel_crtc->active is not always updated at the right times. Signed-off-by: -
Daniel Vetter authored
Now that set_mode also disables crtcs and expects it's new configuration in the staged output links we need to adjust the load detect code a bit. Reviewed-by:
-
Daniel Vetter authored
This seems to be the symptom of a few neat bugs, hence be more obnoxious when this fails. Reviewed-by:
-
Daniel Vetter authored
Because that's what it is. Unfortunately we can't rip this out because the fb helper has an incetious relationship with the crtc helper - it likes to call disable_unused_functions, among other things. Reviewed-by:
-
Daniel Vetter authored
This requires a few changes - We still need a noop function for crtc->disable, becuase the fb helper is a bit too intimate with the crtc helper. - We need to clear crtc->fb ourselves in intel_crtc_disable now that we no longer rely on the helper's disable_unused_functions to do that. - We need to split out the sare update code, becuase the crtc code can't call update_dpms any more, it needs to disable the crtc unconditionally. This is because we now keep onto the encoder -> crtc mapping of the (still) active output pipe configuration. - To check that we really disable a crtc that still has encoders, insert a WARN_ON(!enabled) in the crtc disable function. - Lastly, we need to walk over all crtcs to update their enabled state after having called commit_output_state - for all disabled crtcs the crtc helper code has done that for us previously. v2: Update connector dpms and encoder->connectors_active after disabling the crtc, too. v3: Noop-out intel_encoder_disable. Similarly to the crtc disable callback used by the crtc helper code we can't simply remove all these encoder callbacks: The fb helper (which we still use) has a rather incetious relationship with the crtc helper code ... Reviewed-by:
-
Daniel Vetter authored
... using the pipe masks from the previous patch. Well, not quite: - We still need to call the disable_unused_functions helper, until we've moved the call to commit_output_state further down and adjusted intel_crtc_disable a bit. The next patch will do that. - Because we don't support (yet) mode changes on more than one crtc at a time, some of the modeset_pipes checks are a bit hackish - but that only needs fixing once we incorporate global modeset support. Reviewed-by:
-
Daniel Vetter authored
This is definetely a bit more generic than currently required, but if we keep track of all crtcs that need to be disabled/enable (because they loose an encoder or something similar), crtcs that get completely disabled and those that we need to do an actual mode change nicely prepares us for global modeset operations on multiple crtcs. The only big thing missing here would be a global resource allocation step (for e.g. pch plls), which would equally frob these bitmasks if e.g. a crtc only needs a new pll. Or if we need to enable dithering on an another pipe due to bandwidth constrains somewhere. These masks aren't yet put to use in this patch, this will follow in the next one. v2-v5: Fix up the computations for good (hopefully). v6: Fixup a confusion reported by Damien Lespiau: I've conserved the (imo braindead) behaviour of the crtc helper to disable _any_ disconnected outputs if we do a modeset, even when that newly disabled connector isn't connected to the crtc being changed by the modeset. The effect of that is that we could disable an arbitrary number of unrelated crtcs, which I haven't taken into account when writing this code. Fix this up. Reviewed-by:
-
Daniel Vetter authored
- Use the check_cloned helper from the previous patch. - Use encoder->new_crtc to check crtc properties. v2: Kill the double negation with s/!non_cloned/is_cloned, suggested by Jesse Barnes. Reviewed-by:
-
Daniel Vetter authored
The "is this encoder cloned" check will be reused by the lvds encoder, hence exract it. v2: Be a bit more careful about that we need to check the new, staged ouput configuration in the check_non_cloned helper ... v3: Kill the double negation with s/!non_cloned/is_cloned/, suggested by Jesse Barnes. Reviewed-by:
-
Daniel Vetter authored
While at it, adjust a few things: - Only assigng the new mode to crtc->mode right before calling the mode_set callbacks - none of the previous callbacks depend upon this, they all use the mode argument (as they should). - Check encoder->new_crtc instead of the current crtc to check whether the encoder will be used. This prepares for moving the staged output committing further down in the sequence. Follow-on patches will fix up individual ->mode_fixup callbacks (only tv and lvds are affected though). Reviewed-by:
-
Daniel Vetter authored
It's rather pointless to compute crtc->enabled twice right away ;-) The only thing we really have to be careful about is that we frob the dpms state only after a successful modeset and when we've actually haven't just disabled the crtc. Hooray for convoluted interfaces ... Reviewed-by:
-
Daniel Vetter authored
Originally this has been introduced in commit 6eebd6bb Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Mon Nov 28 21:10:05 2011 +0000 drm: Fix lack of CRTC disable for drm_crtc_helper_set_config(.fb=NULL) With the improvements of the output state staging and no longer overwriting crtc->fb before the hw state is updated we can now handle crtc disabling as part of the normal modeset sequence. Reviewed-by:
-
Daniel Vetter authored
Passing in the old fb, having overwritten the current fb, leads to some neatly convoluted code. It's much simpler if we defer the crtc->fb update to the place that updates the hw, in pipe_set_base. This way we also don't need to restore anything in case something fails - we only update crtc->fb once things have succeeded. The real reason for this change is that now we keep the old fb assigned to crtc->fb, which allows us to finally move the crtc disable case into the common low-level set_mode function in the next patch. Also don't clobber crtc->x and crtc->y, we neatly pass these down the callchain already. Unfortunately we can't do the same with crtc->mode, because that one is being used in the mode_set callbacks. v2: Don't restore the drm_crtc object any more on failed modesets, since we've lose an fb reference otherwise. Also (and this is the reason this has been found), this totally confused the modeset state tracking, since it clobbers crtc->enabled. Issue reported by Paulo Zanoni. v3: Rip out the entire crtc saving into struct intel_set_config, not just the restoring part. Reviewed-by:
-
Daniel Vetter authored
This is the core of the new modeset logic. The current code which is based upon the crtc helper code first updates all the link of the new display pipeline and then calls the lower-level set_mode function to execute the required callbacks to get there. The issue with this approach is that for disabling we need to know the _current_ display pipe state, not the new one. Hence we need to stage the new state of the display pipe and only update it once we have disabled the current configuration and before we start to update the hw registers with the new configuration. This patch here just prepares the ground by switching the new output state computation to these staging pointers. To make it clearer, rename the old update_output_state function to stage_output_state. A few peculiarities: - We're also calling the set_mode function at various places to update properties. Hence after a successfule modeset we need to stage the current configuration (for otherwise we might fall back again). This happens automatically because as part of the (successful) modeset we need to copy the staged state to the real one. But for the hw readout code we need to make sure that this happens, too. - Teach the new staged output state computation code the required smarts to handle the disabling of outputs. The current code handles this in a special case, but to better handle global modeset changes covering more than one crtc, we want to do this all in the same low-level modeset code. - The actual modeset code is still a bit ugly and wants to know the new crtc->enabled state a bit early. Follow-on patches will clean that up, for now we have to apply the staged output configuration early, outside of the set_mode functions. - Improve/add comments in stage_output_state. Essentially all that is left to do now is move the disabling code into set_mode and then move the staged state update code also into set_mode, at the right place between disabling things and calling the mode_set callbacks for the new configuration. v2: Disabling a crtc works by passing in a NULL mode or fb, userspace doesn't hand in the list of connectors. We therefore need to detect this case manually and tear down all the output links. v3: Properly update the output staging pointers after having read out the hw state. v4: Simplify the code, add more DRM_DEBUG_KMS output and check a few assumptions with WARN_ON. Essentially all things that I've noticed while debugging issues in other places of the code. v4: Correctly disable the old set of connectors when enabling an already enabled crtc on a new set of crtc. Reported by Paulo Zanoni. Reviewed-by:
-
Daniel Vetter authored
We actually only touch the connector -> encoder and encoder -> crtc linking. So it's enough to just save/restore that. While at it, also switch to kcalloc to allocate these arrays (omission in the commit message spotted by Jesse Barnes). Reviewed-by:
-
Daniel Vetter authored
Because they all are, the ioctl command never calls us with any of these violated. Also drop a equally pointless empty debug message (and also in set_cursor, while we're at it). With all these changes, intel_crtc_set_config is neatly condensed down to it's essence, the actual modeset code (or fb update calling code) v2: The fb helper code is actually stretching ->set_config semantics a bit, it calls it with set->mode == NULL but set->fb != NULL. Reviewed-by:
-
Daniel Vetter authored
Otherwise we'll set_fb complains pretty loudly if we the crtc is off and userspace moves the NULL fb around a bit. Yeah, this actually happens in the wild ... Reviewed-by:
-
Daniel Vetter authored
Yikes! But yeah, we have to do this until someone volunteers to clean up the fb helper and rid it of its incetious relationship with the crtc helper code. Reviewed-by:
-
Daniel Vetter authored
Note that this function already clobbers the mode config state, so we have to clean things up if something fails. Reviewed-by:
-
Daniel Vetter authored
This computes what exactly changed in the modeset configuration, i.e. whether a full modeset is required or only an update of the framebuffer base address or no change at all. In the future we might add more checks for e.g. when only the output mode changed, so that we could do a minimal modeset for outputs that support this. Like the lvds/eDP panels where we only need to update the panel fitter. Reviewed-by:
-
Daniel Vetter authored
At the end this won't be of much use to us, but meanwhile just extract it to get a better overview of what exactly set_config does. Reviewed-by:
-
Daniel Vetter authored
intel_crtc_set_config is an unwidly beast and is in serious need of some function extraction. To facilitate that, introduce a struct to keep track of all the state involved. Atm it doesn't do much more than keep track of all the allocated memory. v2: Apply some bikeshed to intel_set_config_free, as suggested by Jesse Barnes. Reviewed-by:
-
Daniel Vetter authored
Many BIOSen forget to turn on the pipe A after resume (because they actually don't turn on anything), so we have to do that ourselves when sanitizing the hw state. I've discovered this due to the recent addition of a pipe WARN that takes the force quirk into account. v2: Actually try to enable the pipe with a proper configuration instead of simpyl switching it on with whatever random state the bios left it in after resume. v3: Fixup rebase conflict - the load_detect functions have lost their encoder argument. Reviewed-by:
-
Daniel Vetter authored
We now track the connector state in encoder->connectors_active, and because the DP output can't be cloned, that is sufficient to track the link state. Hence use this instead of adding yet another modeset state variable with dubious semantics at driver load and resume time. Also, connectors_active should only ever be set when the encoder is linked to a crtc, hence convert that crtc test into a WARN. v2: Rebase on top of struct intel_dp moving. v3: The rebase accidentally killed the newly-introduced intel_dp->port Noticed by Paulo Zanoni. Reviewed-by:
-
Daniel Vetter authored
Afaict this has been used for two things: - To prevent the crtc enable code from being run twice. We have now intel_crtc->active to track this in a more precise way. - To ensure the code copes correctly with the unknown hw state after boot and resume. Thanks to the hw state readout and sanitize code we have now a better way to handle this. The only thing it still does is complicate our modeset state space. Having outlived its usefullness, let it just die. Reviewed-by:
-
Daniel Vetter authored
Atm we can only check the connector state after a dpms call - while doing modeset with the copy&pasted crtc helper code things are too ill-defined for proper checking. But the idea is very much to call this check from the modeset code, too. v2: Fix dpms check and don't presume that if the hw isn't on that it must not be linked up with an encoder (it could simply be switched off with the dpms state). Reviewed-by:
-
Daniel Vetter authored
... instead of resetting a few things and hoping that this will work out. To properly disable the output pipelines at the initial modeset after resume or boot up we need to have an accurate picture of which outputs are enabled and connected to which crtcs. Otherwise we risk disabling things at the wrong time, which can lead to hangs (or at least royally confused panels), both requiring a walk to the reset button to fix. Hence read out the hw state with the freshly introduce get_hw_state functions and then sanitize it afterwards. For a full modeset readout (which would allow us to avoid the initial modeset at boot up) a few things are still missing: - Reading out the mode from the pipe, especially the dotclock computation is quite some fun. - Reading out the parameters for the stolen memory framebuffer and wrapping it up. - Reading out the pch pll connections - luckily the disable code simply bails out if the crtc doesn't have a pch pll attached (even for configurations that would need one). This patch here turned up tons of smelly stuff around resume: We restore tons of register in seemingly random way (well, not quite, but we're not too careful either), which leaves the hw in a rather ill-defined state: E.g. the port registers are sometimes unconditionally restore (lvds, crt), leaving us with an active encoder/connector but no active pipe connected to it. Luckily the hw state sanitizer detects this madness and fixes things up a bit. v2: When checking whether an encoder with active connectors has a crtc wire up to it, check for both the crtc _and_ it's active state. v3: - Extract intel_sanitize_encoder. - Manually disable active encoders without an active pipe. v4: Correclty fix up the pipe<->plane mapping on machines where we switch pipes/planes. Noticed by Chris Wilson, who also provided the fixup. v5: Spelling fix in a comment, noticed by Paulo Zanoni Reviewed-by:
-
Daniel Vetter authored
Similar to the sdvo code we poke the dvo encoder whether the output is active. Safe that dvo encoders are not standardized, so this requires a new callback into the dvo chip driver. Hence implement that for all 6 dvo drivers. v2: With the newly added ns2501 we now have 6 dvo drivers instead of just 5 ... Acked-by:
-
Daniel Vetter authored
SDVO is the first real special case - we support multiple outputs on the same encoder and the encoder dpms state isn't the same as when just disabling the outputs when the encoder is cloned. Hence we need a real connector get_hw_state function which inquires the sdvo encoder about its active outputs. Reviewed-by:
-
Daniel Vetter authored
Note that even though this connector is cloneable we still can use the exact same test to check whether the connector is on or whether the encoder is enabled - both the dpms code and the encoder disable/enable frob the exact same hw state. For dvo/sdvo outputs, this will be different. Reviewed-by:
-
Daniel Vetter authored
Reviewed-by:
-
Daniel Vetter authored
Reviewed-by:
-
Daniel Vetter authored
Reviewed-by:
-
Daniel Vetter authored
Also add some macros to make the pipe computation a bit easier. v2: I've mixed up the CPT and !CPT PORT_TO_PIPE macro variants ... Reviewed-by:
-
Daniel Vetter authored
It is all glorious if we try really hard to only enable/disable an entire display pipe to ensure that everyting happens in the right order. But if we don't know the output configuration when the driver takes over, this will all be for vain because we'll make the hw angry right on the first modeset - we don't know what outputs/ports are enabled and hence have to disable everything in a rather ad-hoc way. Hence we need to be able to read out the current hw state, so that we can properly tear down the current hw state on the first modeset. Obviously this is also a nice preparation for the fastboot work, where we try to avoid the modeset on driver load if it matches what the hw is currently using. Furthermore we'll be using these functions to cross-check the actual hw state with what we think it should be, to ensure that the modeset state machine actually works as advertised. This patch only contains the interface definitions and a little helper for the simple case where we have a 1:1 encoder to connector mapping. Reviewed-by:
-
