Re: [PATCH 10/17] drm: Atomic crtc/connector updates using crtc/plane helper interfaces

[Sean Paul <seanpaul@chromium.org>]

On Sun, Nov 02, 2014 at 02:19:23PM +0100, Daniel Vetter wrote:
> So this is finally the integration of the crtc and plane helper
> interfaces into the atomic helper functions.
>
> In the check function we now have a few steps:
>
> - First we update the output routing and figure out which crtcs need a
>   full mode set. Suitable encoders are selected using ->best_encoder,
>   with the same semantics as the crtc helpers of implicitly disabling
>   all connectors currently using the encoder.
>
> - Then we pull all other connectors into the state update which feed
>   from a crtc which changes. This must be done do catch mode changes
>   and similar updates - atomic updates are differences on top of the
>   current state.
>
> - Then we call all the various ->mode_fixup to compute the adjusted
>   mode. Note that here we have a slight semantic difference compared
>   to the crtc helpers: We have not yet updated the encoder->crtc link
>   when calling the encoder's ->mode_fixup function. But that's a
>   requirement when converting to atomic since we want to prepare the
>   entire state completely contained with the over drm_atomic_state
>   structure. So this must be carefully checked when converting drivers
>   over to atomic helpers.
>
> - Finally we do call the atomic_check functions on planes and crtcs.
>
> The commit function is also quite a beast:
>
> - The only step that can fail is done first, namely pinning the
>   framebuffers. After that we cross the point of no return, an async
>   commit would push all that into the worker thread.
>
> - The disabling of encoders and connectors is a bit tricky, since
>   depending upon the final state we need to select different crtc
>   helper functions.
>
> - Software tracking is a bit clarified compared to the crtc helpers:
>   We commit the software state before starting to touch the hardware,
>   like crtc helpers. But since we just swap them we still have the old
>   state (i.e. the current hw state) around, which is really handy to
>   write simple disable functions. So no more
>   drm_crtc_helper_disable_all_unused_functions kind of fun because
>   we're leaving unused crtcs/encoders behind. Everything gets shut
>   down in-order now, which is one of the key differences of the i915
>   helpers compared to crtc helpers and a really nice additional
>   guarantee.
>
> - Like with the plane helpers the atomic commit function waits for one
>   vblank to pass before calling the framebuffer cleanup function.
>
> Compared to Rob's helper approach there's a bunch of upsides:
>
> - All the interfaces which can fail are called in the ->check hook
>   (i.e. ->best_match and the various ->mode_fixup hooks). This means
>   that drivers can just reuse those functions and don't need to move
>   everything into ->atomic_check callbacks. If drivers have no need
>   for additional constraint checking beyong their existing crtc

s/beyong/beyond/

>   helper callbacks they don't need to do anything.
>
> - The actual commit operation is properly stage: First we prepare
>   framebuffers, which can potentially still fail (due to memory
>   exhausting). This is important for the async case, where this must
>   be done synchronously to correctly return errors.
>
> - The output configuration changes (done with crtc helper functions)
>   and the plane update (using atomic plane helpers) are correctly
>   interleaved: First we shut down any crtcs that need changing, then
>   we update planes and finally we enable everything again. Hardware
>   without GO bits must be more careful with ordering, which this
>   sequence enables.
>
> - Also for hardware with shared output resources (like display PLLs)
>   we first must shut down the old configuration before we can enable
>   the new one. Otherwise we can hit an impossible intermediate state
>   where there's not enough PLLs (which is the point behind atomic
>   updates).
>
> v2:
> - Ensure that users of ->check update crtc_state->enable correctly.
> - Update the legacy state in crtc/plane structures. Eventually we want
>   to remove that, but for now the drm core still expects this (especially
>   the plane->fb pointer).
>
> v3: A few changes for better async handling:
>
> - Reorder the software side state commit so that it happens all before
>   we touch the hardware. This way async support becomes very easy
>   since we can punt all the actual hw touching to a worker thread. And
>   as long as we synchronize with that thread (flushing or cancelling,
>   depending upon what the driver can handle) before we commit the next
>   software state there's no need for any locking in the worker thread
>   at all. Which greatly simplifies things.
>
>   And as long as we synchronize with all relevant threads we can have
>   a lot of them (e.g. per-crtc for per-crtc updates) running in
>   parallel.
>
> - Expose pre/post plane commit steps separately. We need to expose the
>   actual hw commit step anyway for drivers to be able to implement
>   asynchronous commit workers. But if we expose pre/post and plane
>   commit steps individually we allow drivers to selectively use atomic
>   helpers.
>
> - I've forgotten to call encoder/bridge ->mode_set functions, fix
>   this.
>
> v4: Add debug output and fix a mixup between current and new state
> that resulted in crtcs not getting updated correctly. And in an
> Oops ...
>
> v5:
> - Be kind to driver writers in the vblank wait functions.. if thing
>   aren't working yet, and vblank irq will never come, then let's not
>   block forever.. especially under console-lock.
> - Correctly clear connector_state->best_encoder when disabling.
>   Spotted while trying to understand a report from Rob Clark.
> - Only steal encoder if it actually changed, otherwise hilarity ensues
>   if we steal from the current connector and so set the ->crtc pointer
>   unexpectedly to NULL. Reported by Rob Clark.
> - Bail out in disable_outputs if an output currently doesn't have a
>   best_encoder - this means it's already disabled.
>
> v6: Fixupe kerneldoc as reported by Paulo. And also fix up kerneldoc

s/Fixupe/Fixup/

> in drm_crtc.h.
>
> v7: Take ownership of the atomic state and clean it up with
> drm_atomic_state_free().
>
> v8 Various improvements all over:
> - Polish code comments and kerneldoc.
> - Improve debug output to make sure all failure cases are logged.
> - Treat enabled crtc with no connectors as invalid input from userspace.
> - Don't ignore the return value from mode_fixup().
>
> v9:
> - Improve debug output for crtc_state->mode_changed.
>
> v10:
> - Fixup the vblank waiting code to properly balance the vblank_get/put
>   calls.
> - Better comments when checking/computing crtc->mode_changed
>
> v11: Fixup the encoder stealing logic: We can't look at encoder->crtc
> since that's not in the atomic state structures and might be updated
> asynchronously in and async commit. Instead we need to inspect all the
> connector states and check whether the encoder is currently in used
> and if so, on which crtc.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---

Overall, this looks really good. I just have a couple minor comments.

Also, it seems like my mutt->gmail workflow is mangling the patch's
indentation, sorry 'bout that.



>  drivers/gpu/drm/drm_atomic_helper.c | 692 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_crtc_helper.c   |   1 +
>  include/drm/drm_atomic_helper.h     |   8 +
>  include/drm/drm_crtc.h              |  10 +
>  4 files changed, 711 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 55a8eb2678b0..887e1971c915 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -29,6 +29,7 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_atomic_helper.h>
>
>  static void
>  drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> @@ -57,6 +58,327 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>   }
>  }
>
> +static struct drm_crtc *
> +get_current_crtc_for_encoder(struct drm_device *dev,
> +     struct drm_encoder *encoder)
> +{
> + struct drm_mode_config *config = &dev->mode_config;
> + struct drm_connector *connector;
> +
> + WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
> +
> + list_for_each_entry(connector, &config->connector_list, head) {
> + if (connector->state->best_encoder != encoder)
> + continue;
> +
> + return connector->state->crtc;
> + }
> +
> + return NULL;
> +}
> +
> +static int
> +steal_encoder(struct drm_atomic_state *state,
> +      struct drm_encoder *encoder,
> +      struct drm_crtc *encoder_crtc)
> +{
> + struct drm_mode_config *config = &state->dev->mode_config;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector *connector;
> + struct drm_connector_state *connector_state;
> +
> + /*
> + * We can only steal an encoder coming from a connector, which means we
> + * must already hold the connection_mutex.
> + */
> + WARN_ON(!drm_modeset_is_locked(&config->connection_mutex));
> +
> + DRM_DEBUG_KMS("[ENCODER:%d:%s] in use on [CRTC:%d], stealing it\n",
> +      encoder->base.id, encoder->name,
> +      encoder_crtc->base.id);
> +
> + crtc_state = drm_atomic_get_crtc_state(state, encoder_crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
> + crtc_state->mode_changed = true;
> +
> + list_for_each_entry(connector, &config->connector_list, head) {
> + if (connector->state->best_encoder != encoder)
> + continue;
> +
> + DRM_DEBUG_KMS("Stealing encoder from [CONNECTOR:%d:%s]\n",
> +      connector->base.id,
> +      connector->name);
> +
> + connector_state = drm_atomic_get_connector_state(state,
> + connector);
> + if (IS_ERR(connector_state))
> + return PTR_ERR(connector_state);
> +
> + connector_state->crtc = NULL;
> + connector_state->best_encoder = NULL;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +update_connector_routing(struct drm_atomic_state *state, int conn_idx)
> +{
> + struct drm_connector_helper_funcs *funcs;
> + struct drm_encoder *new_encoder;
> + struct drm_connector *connector;
> + struct drm_connector_state *connector_state;
> + struct drm_crtc_state *crtc_state;
> + int idx, ret;
> +
> + connector = state->connectors[conn_idx];
> + connector_state = state->connector_states[conn_idx];
> +
> + if (!connector)
> + return 0;
> +
> + DRM_DEBUG_KMS("Updating routing for [CONNECTOR:%d:%s]\n",
> + connector->base.id,
> + connector->name);
> +
> + if (connector->state->crtc != connector_state->crtc) {
> + if (connector->state->crtc) {
> + idx = drm_crtc_index(connector->state->crtc);
> +
> + crtc_state = state->crtc_states[idx];
> + crtc_state->mode_changed = true;
> + }
> +
> + if (connector_state->crtc) {
> + idx = drm_crtc_index(connector_state->crtc);
> +
> + crtc_state = state->crtc_states[idx];
> + crtc_state->mode_changed = true;
> + }
> + }
> +
> + if (!connector_state->crtc) {
> + DRM_DEBUG_KMS("Disabling [CONNECTOR:%d:%s]\n",
> + connector->base.id,
> + connector->name);
> +
> + connector_state->best_encoder = NULL;
> +
> + return 0;
> + }
> +
> + funcs = connector->helper_private;
> + new_encoder = funcs->best_encoder(connector);
> +
> + if (!new_encoder) {
> + DRM_DEBUG_KMS("No suitable encoder found for [CONNECTOR:%d:%s]\n",
> +      connector->base.id,
> +      connector->name);
> + return -EINVAL;
> + }
> +
> + if (new_encoder != connector_state->best_encoder) {

nit: If you just returned early when the encoder doesn't change, you can save
indentation and line breaks.

> + struct drm_crtc *encoder_crtc;
> +
> + encoder_crtc = get_current_crtc_for_encoder(state->dev,
> +    new_encoder);
> +
> + if (encoder_crtc) {
> + ret = steal_encoder(state, new_encoder, encoder_crtc);
> + if (ret) {
> + DRM_DEBUG_KMS("Encoder stealing failed for [CONNECTOR:%d:%s]\n",
> +      connector->base.id,
> +      connector->name);
> + return ret;
> + }
> + }
> +
> + connector_state->best_encoder = new_encoder;
> + idx = drm_crtc_index(connector_state->crtc);
> +
> + crtc_state = state->crtc_states[idx];
> + crtc_state->mode_changed = true;
> + }
> +
> + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d]\n",
> +      connector->base.id,
> +      connector->name,
> +      new_encoder->base.id,
> +      new_encoder->name,
> +      connector_state->crtc->base.id);
> +
> + return 0;
> +}
> +
> +static int
> +mode_fixup(struct drm_atomic_state *state)
> +{
> + int ncrtcs = state->dev->mode_config.num_crtc;
> + int nconnectors = state->dev->mode_config.num_connector;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector_state *conn_state;
> + int i;
> + bool ret;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + crtc_state = state->crtc_states[i];
> +
> + if (!crtc_state || !crtc_state->mode_changed)
> + continue;
> +
> + drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
> + }
> +
> + for (i = 0; i < nconnectors; i++) {
> + struct drm_encoder_helper_funcs *funcs;
> + struct drm_encoder *encoder;
> +
> + conn_state = state->connector_states[i];
> +
> + if (!conn_state)
> + continue;
> +
> + WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
> +
> + if (!conn_state->crtc || !conn_state->best_encoder)
> + continue;
> +
> + crtc_state =
> + state->crtc_states[drm_crtc_index(conn_state->crtc)];
> +
> + /*
> + * Each encoder has at most one connector (since we always steal
> + * it away), so we won't call ->mode_fixup twice.
> + */
> + encoder = conn_state->best_encoder;
> + funcs = encoder->helper_private;
> +
> + if (encoder->bridge && encoder->bridge->funcs->mode_fixup) {
> + ret = encoder->bridge->funcs->mode_fixup(
> + encoder->bridge, &crtc_state->mode,
> + &crtc_state->adjusted_mode);
> + if (!ret) {
> + DRM_DEBUG_KMS("Bridge fixup failed\n");
> + return -EINVAL;
> + }
> + }
> +
> +
> + ret = funcs->mode_fixup(encoder, &crtc_state->mode,
> + &crtc_state->adjusted_mode);
> + if (!ret) {
> + DRM_DEBUG_KMS("[ENCODER:%d:%s] fixup failed\n",
> +      encoder->base.id, encoder->name);
> + return -EINVAL;
> + }
> + }
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc;
> +
> + crtc_state = state->crtc_states[i];
> + crtc = state->crtcs[i];
> +
> + if (!crtc_state || !crtc_state->mode_changed)
> + continue;
> +
> + funcs = crtc->helper_private;
> + ret = funcs->mode_fixup(crtc, &crtc_state->mode,
> + &crtc_state->adjusted_mode);
> + if (!ret) {
> + DRM_DEBUG_KMS("[CRTC:%d] fixup failed\n",
> +      crtc->base.id);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int
> +drm_atomic_helper_check_prepare(struct drm_device *dev,
> + struct drm_atomic_state *state)
> +{
> + int ncrtcs = dev->mode_config.num_crtc;
> + int nconnectors = dev->mode_config.num_connector;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + int i, ret;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + crtc = state->crtcs[i];
> + crtc_state = state->crtc_states[i];
> +
> + if (!crtc)
> + continue;
> +
> + if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {
> + DRM_DEBUG_KMS("[CRTC:%d] mode changed\n",
> +      crtc->base.id);
> + crtc_state->mode_changed = true;
> + }
> +
> + if (crtc->state->enable != crtc_state->enable) {
> + DRM_DEBUG_KMS("[CRTC:%d] state changed\n",

nit: s/state/enable/

> +      crtc->base.id);
> + crtc_state->mode_changed = true;
> + }
> + }
> +
> + for (i = 0; i < nconnectors; i++) {
> + /*
> + * This only sets crtc->mode_changed for routing changes,
> + * drivers must set crtc->mode_changed themselves when connector
> + * properties need to be updated.
> + */
> + ret = update_connector_routing(state, i);
> + if (ret)
> + return ret;
> + }
> +
> + /*
> + * After all the routing has been prepared we need to add in any
> + * connector which is itself unchanged, but who's crtc changes it's
> + * configuration. This must be done before calling mode_fixup in case a
> + * crtc only changed its mode but has the same set of connectors.
> + */
> + for (i = 0; i < ncrtcs; i++) {
> +
> + crtc = state->crtcs[i];
> + crtc_state = state->crtc_states[i];
> +
> + if (!crtc)
> + continue;
> +
> + if (crtc_state->mode_changed) {

nit: Flipping this check and moving it up into the !crtc if above saves a bit of
indentation

> + bool has_connectors;
> +
> + DRM_DEBUG_KMS("[CRTC:%d] needs full modeset, enable: %c\n",
> +      crtc->base.id,
> +      crtc_state->enable ? 'y' : 'n');
> +
> + ret = drm_atomic_add_affected_connectors(state, crtc);
> + if (ret < 0)

I think if (ret != 0) is more appropriate here

> + return ret;
> +
> + has_connectors = drm_atomic_connectors_for_crtc(state,
> + crtc);
> +
> + if (crtc_state->enable != has_connectors) {

This makes me a little nervous. Even though has_connectors is a bool,
drm_atomic_connectors_for_crtc returns an int, and this seems like something
that someone might "fix" in the future.

[PATCH] drm/atomic: Use proper type for drm_atomic_connectors_for_crtc


> + DRM_DEBUG_KMS("[CRTC:%d] enabled/connectors mismatch\n",
> +      crtc->base.id);
> +
> + return -EINVAL;
> + }
> + }
> + }
> +
> + return mode_fixup(state);
> +}
> +
>  /**
>   * drm_atomic_helper_check - validate state object
>   * @dev: DRM device
> @@ -78,6 +400,10 @@ int drm_atomic_helper_check(struct drm_device *dev,
>   int ncrtcs = dev->mode_config.num_crtc;
>   int i, ret = 0;
>
> + ret = drm_atomic_helper_check_prepare(dev, state);
> + if (ret)
> + return ret;
> +
>   for (i = 0; i < nplanes; i++) {
>   struct drm_plane_helper_funcs *funcs;
>   struct drm_plane *plane = state->planes[i];
> @@ -125,6 +451,372 @@ int drm_atomic_helper_check(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_check);
>
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> + int ncrtcs = old_state->dev->mode_config.num_crtc;
> + int nconnectors = old_state->dev->mode_config.num_connector;
> + int i;
> +
> + for (i = 0; i < nconnectors; i++) {
> + struct drm_connector_state *old_conn_state;
> + struct drm_connector *connector;
> + struct drm_encoder_helper_funcs *funcs;
> + struct drm_encoder *encoder;
> +
> + old_conn_state = old_state->connector_states[i];
> + connector = old_state->connectors[i];
> +
> + /* Shut down everything that's in the changeset and currently
> + * still on. So need to check the old, saved state. */
> + if (!old_conn_state || !old_conn_state->crtc)
> + continue;
> +
> + encoder = connector->state->best_encoder;
> +
> + if (!encoder)
> + continue;
> +
> + funcs = encoder->helper_private;
> +
> + /*
> + * Each encoder has at most one connector (since we always steal
> + * it away), so we won't call call disable hooks twice.
> + */
> + if (encoder->bridge)
> + encoder->bridge->funcs->disable(encoder->bridge);
> +
> + /* Right function depends upon target state. */
> + if (connector->state->crtc)
> + funcs->prepare(encoder);
> + else if (funcs->disable)
> + funcs->disable(encoder);
> + else
> + funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +
> + if (encoder->bridge)
> + encoder->bridge->funcs->post_disable(encoder->bridge);
> + }
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc;
> +
> + crtc = old_state->crtcs[i];
> +
> + /* Shut down everything that needs a full modeset. */
> + if (!crtc || !crtc->state->mode_changed)
> + continue;
> +
> + funcs = crtc->helper_private;
> +
> + /* Right function depends upon target state. */
> + if (crtc->state->enable)
> + funcs->prepare(crtc);
> + else if (funcs->disable)
> + funcs->disable(crtc);
> + else
> + funcs->dpms(crtc, DRM_MODE_DPMS_OFF);
> + }
> +}
> +
> +static void
> +set_routing_links(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> + int nconnectors = dev->mode_config.num_connector;
> + int ncrtcs = old_state->dev->mode_config.num_crtc;
> + int i;
> +
> + /* clear out existing links */
> + for (i = 0; i < nconnectors; i++) {
> + struct drm_connector *connector;
> +
> + connector = old_state->connectors[i];
> +
> + if (!connector || !connector->encoder)
> + continue;
> +
> + WARN_ON(!connector->encoder->crtc);
> +
> + connector->encoder->crtc = NULL;
> + connector->encoder = NULL;
> + }
> +
> + /* set new links */
> + for (i = 0; i < nconnectors; i++) {
> + struct drm_connector *connector;
> +
> + connector = old_state->connectors[i];
> +
> + if (!connector || !connector->state->crtc)
> + continue;
> +
> + if (WARN_ON(!connector->state->best_encoder))
> + continue;
> +
> + connector->encoder = connector->state->best_encoder;
> + connector->encoder->crtc = connector->state->crtc;
> + }
> +
> + /* set legacy state in the crtc structure */
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc *crtc;
> +
> + crtc = old_state->crtcs[i];
> +
> + if (!crtc)
> + continue;
> +
> + crtc->mode = crtc->state->mode;
> + crtc->enabled = crtc->state->enable;
> + crtc->x = crtc->primary->state->src_x >> 16;
> + crtc->y = crtc->primary->state->src_y >> 16;
> + }
> +}
> +
> +static void
> +crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> + int ncrtcs = old_state->dev->mode_config.num_crtc;
> + int nconnectors = old_state->dev->mode_config.num_connector;
> + int i;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc;
> +
> + crtc = old_state->crtcs[i];
> +
> + if (!crtc || !crtc->state->mode_changed)
> + continue;
> +
> + funcs = crtc->helper_private;
> +
> + if (crtc->state->enable)
> + funcs->mode_set_nofb(crtc);
> + }
> +
> + for (i = 0; i < nconnectors; i++) {
> + struct drm_connector *connector;
> + struct drm_crtc_state *new_crtc_state;
> + struct drm_encoder_helper_funcs *funcs;
> + struct drm_encoder *encoder;
> + struct drm_display_mode *mode, *adjusted_mode;
> +
> + connector = old_state->connectors[i];
> +
> + if (!connector || !connector->state->best_encoder)
> + continue;
> +
> + encoder = connector->state->best_encoder;
> + funcs = encoder->helper_private;
> + new_crtc_state = connector->state->crtc->state;
> + mode = &new_crtc_state->mode;
> + adjusted_mode = &new_crtc_state->adjusted_mode;
> +
> + /*
> + * Each encoder has at most one connector (since we always steal
> + * it away), so we won't call call mode_set hooks twice.
> + */
> + funcs->mode_set(encoder, mode, adjusted_mode);
> +
> + if (encoder->bridge && encoder->bridge->funcs->mode_set)
> + encoder->bridge->funcs->mode_set(encoder->bridge,
> + mode, adjusted_mode);
> + }
> +}
> +
> +/**
> + * drm_atomic_helper_commit_pre_planes - modeset commit before plane updates
> + * @dev: DRM device
> + * @state: atomic state
> + *
> + * This function commits the modeset changes that need to be committed before
> + * updating planes. It shuts down all the outputs that need to be shut down and
> + * prepares them (if requried) with the new mode.

s/requried/required/

> + */
> +void drm_atomic_helper_commit_pre_planes(struct drm_device *dev,
> + struct drm_atomic_state *state)
> +{
> + disable_outputs(dev, state);
> + set_routing_links(dev, state);
> + crtc_set_mode(dev, state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_pre_planes);
> +
> +/**
> + * drm_atomic_helper_commit_post_planes - modeset commit after plane updates
> + * @dev: DRM device
> + * @old_state: atomic state object with old state structures
> + *
> + * This function commits the modeset changes that need to be committed after
> + * updating planes: It enables all the outputs with the new configuration which
> + * had to be turned off for the update.
> + */
> +void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
> +  struct drm_atomic_state *old_state)
> +{
> + int ncrtcs = old_state->dev->mode_config.num_crtc;
> + int nconnectors = old_state->dev->mode_config.num_connector;
> + int i;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + struct drm_crtc_helper_funcs *funcs;
> + struct drm_crtc *crtc;
> +
> + crtc = old_state->crtcs[i];
> +
> + /* Need to filter out CRTCs where only planes change. */
> + if (!crtc || !crtc->state->mode_changed)
> + continue;
> +
> + funcs = crtc->helper_private;
> +
> + if (crtc->state->enable)
> + funcs->commit(crtc);
> + }
> +
> + for (i = 0; i < nconnectors; i++) {
> + struct drm_connector *connector;
> + struct drm_encoder_helper_funcs *funcs;
> + struct drm_encoder *encoder;
> +
> + connector = old_state->connectors[i];
> +
> + if (!connector || !connector->state->best_encoder)
> + continue;
> +
> + encoder = connector->state->best_encoder;
> + funcs = encoder->helper_private;
> +
> + /*
> + * Each encoder has at most one connector (since we always steal
> + * it away), so we won't call call enable hooks twice.
> + */
> + if (encoder->bridge)
> + encoder->bridge->funcs->pre_enable(encoder->bridge);
> +
> + funcs->commit(encoder);
> +
> + if (encoder->bridge)
> + encoder->bridge->funcs->enable(encoder->bridge);
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_post_planes);
> +
> +static void
> +wait_for_vblanks(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> + int ncrtcs = old_state->dev->mode_config.num_crtc;
> + int i, ret;
> +
> + for (i = 0; i < ncrtcs; i++) {
> + crtc = old_state->crtcs[i];
> + old_crtc_state = old_state->crtc_states[i];
> +
> + if (!crtc)
> + continue;
> +
> + /* No one cares about the old state, so abuse it for tracking
> + * and store whether we hold a vblank reference (and should do a
> + * vblank wait) in the ->enable boolean. */
> + old_crtc_state->enable = false;
> +
> + if (!crtc->state->enable)
> + continue;
> +
> + ret = drm_crtc_vblank_get(crtc);
> + if (ret != 0)
> + continue;
> +
> + old_crtc_state->enable = true;
> + old_crtc_state->last_vblank_count = drm_vblank_count(dev, i);

I think you should collect last_vblank_count before drm_crtc_vblank_get

> + }
> +
> + for (i = 0; i < ncrtcs; i++) {
> + crtc = old_state->crtcs[i];
> + old_crtc_state = old_state->crtc_states[i];
> +
> + if (!crtc || !old_crtc_state->enable)
> + continue;
> +
> + ret = wait_event_timeout(dev->vblank[i].queue,
> + old_crtc_state->last_vblank_count !=
> + drm_vblank_count(dev, i),
> + msecs_to_jiffies(50));
> +
> + drm_crtc_vblank_put(crtc);
> + }
> +}
> +
> +/**
> + * drm_atomic_helper_commit - commit validated state object
> + * @dev: DRM device
> + * @state: the driver state object
> + * @async: asynchronous commit
> + *
> + * This function commits a with drm_atomic_helper_check() pre-validated state
> + * object. This can still fail when e.g. the framebuffer reservation fails. For
> + * now this doesn't implement asynchronous commits.
> + *
> + * RETURNS
> + * Zero for success or -errno.
> + */
> +int drm_atomic_helper_commit(struct drm_device *dev,
> +     struct drm_atomic_state *state,
> +     bool async)
> +{
> + int ret;
> +
> + if (async)
> + return -EBUSY;
> +
> + ret = drm_atomic_helper_prepare_planes(dev, state);
> + if (ret)
> + return ret;
> +
> + /*
> + * This is the point of no return - everything below never fails except
> + * when the hw goes bonghits. Which means we can commit the new state on
> + * the software side now.
> + */
> +
> + drm_atomic_helper_swap_state(dev, state);
> +
> + /*
> + * Everything below can be run asynchronously withou the need to grab

s/withou/without/

> + * any modeset locks at all under one conditions: It must be guaranteed
> + * that the asynchronous work has either been cancelled (if the driver
> + * supports it, which at least requires that the framebuffers get
> + * cleaned up with drm_atomic_helper_cleanup_planes()) or completed
> + * before the new state gets committed on the software side with
> + * drm_atomic_helper_swap_state().
> + *
> + * This scheme allows new atomic state updates to be prepared and
> + * checked in parallel to the asynchronous completion of the previous
> + * update. Which is important since compositors need to figure out the
> + * composition of the next frame right after having submitted the
> + * current layout.
> + */
> +
> + drm_atomic_helper_commit_pre_planes(dev, state);
> +
> + drm_atomic_helper_commit_planes(dev, state);
> +
> + drm_atomic_helper_commit_post_planes(dev, state);
> +
> + wait_for_vblanks(dev, state);
> +
> + drm_atomic_helper_cleanup_planes(dev, state);
> +
> + drm_atomic_state_free(state);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit);
> +
>  /**
>   * drm_atomic_helper_prepare_planes - prepare plane resources after commit
>   * @dev: DRM device
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c
> index 95ecbb131053..46728a8ac622 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -927,6 +927,7 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
>
>   crtc_state->enable = true;
>   crtc_state->planes_changed = true;
> + crtc_state->mode_changed = true;
>   drm_mode_copy(&crtc_state->mode, mode);
>   drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
>
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 79938c62e7ad..9781ce739e10 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -30,6 +30,14 @@
>
>  int drm_atomic_helper_check(struct drm_device *dev,
>      struct drm_atomic_state *state);
> +int drm_atomic_helper_commit(struct drm_device *dev,
> +     struct drm_atomic_state *state,
> +     bool async);
> +
> +void drm_atomic_helper_commit_pre_planes(struct drm_device *dev,
> + struct drm_atomic_state *state);
> +void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
> +  struct drm_atomic_state *old_state);
>
>  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>       struct drm_atomic_state *state);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 77ff8992a3b7..ddff25eb34d4 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -229,6 +229,9 @@ struct drm_atomic_state;
>  /**
>   * struct drm_crtc_state - mutable crtc state
>   * @enable: whether the CRTC should be enabled, gates all other state
> + * @mode_changed: for use by helpers and drivers when computing state updates
> + * @last_vblank_count: for helpers and drivers to capture the vblank of the
> + * update to ensure framebuffer cleanup isn't done too early
>   * @planes_changed: for use by helpers and drivers when computing state updates
>   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
>   * @mode: current mode timings
> @@ -241,6 +244,10 @@ struct drm_crtc_state {
>
>   /* computed state bits used by helpers and drivers */
>   bool planes_changed : 1;
> + bool mode_changed : 1;
> +
> + /* last_vblank_count: for vblank waits before cleanup */
> + u32 last_vblank_count;
>
>   /* adjusted_mode: for use by helpers and drivers */
>   struct drm_display_mode adjusted_mode;
> @@ -426,11 +433,14 @@ struct drm_crtc {
>  /**
>   * struct drm_connector_state - mutable connector state
>   * @crtc: crtc to connect connector to, NULL if disabled
> + * @best_encoder: can be used by helpers and drivers to select the encoder
>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_connector_state {
>   struct drm_crtc *crtc;
>
> + struct drm_encoder *best_encoder;
> +
>   struct drm_atomic_state *state;
>  };
>
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel