| Message ID | 1346236444-31640-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 29 Aug 2012 12:34:04 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > 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 on 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. > > 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. Might not need to clean it up here though, since the only time that should matter is during the first mode set after a power on or resume when a superfluous CRTC might be enabled. And with the new hw state readout, we can fix that up. You choose though. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Wed, Sep 5, 2012 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Wed, 29 Aug 2012 12:34:04 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > >> 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 on 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. >> >> 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. > > Might not need to clean it up here though, since the only time that > should matter is during the first mode set after a power on or resume > when a superfluous CRTC might be enabled. And with the new hw state > readout, we can fix that up. > > You choose though. If your comment is just about the v6 fixup, that's definitely need here. The failure mode is that we unplug a connector, then the window systems disables _all_ connectors since it wants to reassign them (or change the fb or whatever, atomic modeset would fix this better). On the first modeset we then disable crtc 0, but crtc 1 gets disabled, too (since the connector is now disconnected). Since I didn't consider things, the state tracking would get out of synced with reality and not enable crtc 0 again on the new framebuffer (worked though when retrying). If the comment is just about changing the behaviour, that's definitely planned, but as a follow-up cleanup - it changes the abi behaviour, so we need to have it in a separate patch for revertability. Or do you mean something totally different? -Daniel
On Wed, 5 Sep 2012 21:38:27 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Wed, Sep 5, 2012 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Wed, 29 Aug 2012 12:34:04 +0200 > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > >> 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 on 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. > >> > >> 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. > > > > Might not need to clean it up here though, since the only time that > > should matter is during the first mode set after a power on or resume > > when a superfluous CRTC might be enabled. And with the new hw state > > readout, we can fix that up. > > > > You choose though. > > If your comment is just about the v6 fixup, that's definitely need > here. The failure mode is that we unplug a connector, then the window > systems disables _all_ connectors since it wants to reassign them (or > change the fb or whatever, atomic modeset would fix this better). On > the first modeset we then disable crtc 0, but crtc 1 gets disabled, > too (since the connector is now disconnected). Since I didn't consider > things, the state tracking would get out of synced with reality and > not enable crtc 0 again on the new framebuffer (worked though when > retrying). > > If the comment is just about changing the behaviour, that's definitely > planned, but as a follow-up cleanup - it changes the abi behaviour, so > we need to have it in a separate patch for revertability. > > Or do you mean something totally different? No that covers it, thanks for both explanations.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c59569e..8f71957 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6706,6 +6706,98 @@ fail: return ERR_PTR(-EINVAL); } +/* Computes which crtcs are affected and sets the relevant bits in the mask. For + * simplicity we use the crtc's pipe number (because it's easier to obtain). */ +static void +intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes, + unsigned *prepare_pipes, unsigned *disable_pipes) +{ + struct intel_crtc *intel_crtc; + struct drm_device *dev = crtc->dev; + struct intel_encoder *encoder; + struct intel_connector *connector; + struct drm_crtc *tmp_crtc; + + *disable_pipes = *modeset_pipes = *prepare_pipes = 0; + + /* Check which crtcs have changed outputs connected to them, these need + * to be part of the prepare_pipes mask. We don't (yet) support global + * modeset across multiple crtcs, so modeset_pipes will only have one + * bit set at most. */ + list_for_each_entry(connector, &dev->mode_config.connector_list, + base.head) { + if (connector->base.encoder == &connector->new_encoder->base) + continue; + + if (connector->base.encoder) { + tmp_crtc = connector->base.encoder->crtc; + + *prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe; + } + + if (connector->new_encoder) + *prepare_pipes |= + 1 << connector->new_encoder->new_crtc->pipe; + } + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, + base.head) { + if (encoder->base.crtc == &encoder->new_crtc->base) + continue; + + if (encoder->base.crtc) { + tmp_crtc = encoder->base.crtc; + + *prepare_pipes |= 1 << to_intel_crtc(tmp_crtc)->pipe; + } + + if (encoder->new_crtc) + *prepare_pipes |= 1 << encoder->new_crtc->pipe; + } + + /* Check for any pipes that will be fully disabled ... */ + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, + base.head) { + bool used = false; + + /* Don't try to disable disabled crtcs. */ + if (!intel_crtc->base.enabled) + continue; + + list_for_each_entry(encoder, &dev->mode_config.encoder_list, + base.head) { + if (encoder->new_crtc == intel_crtc) + used = true; + } + + if (!used) + *disable_pipes |= 1 << intel_crtc->pipe; + } + + + /* set_mode is also used to update properties on life display pipes. */ + intel_crtc = to_intel_crtc(crtc); + if (crtc->enabled) + *prepare_pipes |= 1 << intel_crtc->pipe; + + /* We only support modeset on one single crtc, hence we need to do that + * only for the passed in crtc iff we change anything else than just + * disable crtcs. + * + * This is actually not true, to be fully compatible with the old crtc + * helper we automatically disable _any_ output (i.e. doesn't need to be + * connected to the crtc we're modesetting on) if it's disconnected. + * Which is a rather nutty api (since changed the output configuration + * without userspace's explicit request can lead to confusion), but + * alas. Hence we currently need to modeset on all pipes we prepare. */ + if (*prepare_pipes) + *modeset_pipes = *prepare_pipes; + + /* ... and mask these out. */ + *modeset_pipes &= ~(*disable_pipes); + *prepare_pipes &= ~(*disable_pipes); +} + bool intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y, struct drm_framebuffer *fb) @@ -6715,8 +6807,12 @@ bool intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; struct drm_encoder_helper_funcs *encoder_funcs; struct drm_encoder *encoder; + unsigned disable_pipe, prepare_pipes, modeset_pipes; bool ret = true; + intel_modeset_affected_pipes(crtc, &modeset_pipes, + &prepare_pipes, &disable_pipe); + intel_modeset_commit_output_state(dev); crtc->enabled = drm_helper_crtc_in_use(crtc);
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 on 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. 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. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 96 ++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+)