| Message ID | 1414934370-11924-11-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
I've applied all the other nits, replies to the more interesting bits below. On Wed, Nov 05, 2014 at 01:53:48PM -0500, Sean Paul wrote: > On Sun, Nov 02, 2014 at 02:19:23PM +0100, Daniel Vetter wrote: > > + 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. Hm, that would mean either a goto (I don't like that for non-exceptional control flow) or duplicating the debug output. I'll go with the latter and frob it a bit. > > + 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 Hm, yeah. I'll rename to num_connectors and add a !!. The idea of returning an int and not just a bool is that drivers might care if there's more than one, i.e. for cloned setups. At least i915 has some special considerations for that in some cases. > > + 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 vblank_get should block on anything, and I'm not sure whether the drm_vblank_count can't just fall over if vblank_get fails (since vblank_get before vblank_count is the pattern drm_irq.c uses iirc). So I'd prefer it this way round just for paranoia. So I think I'll stick with this scheme. The waiting is only done once we've grabbed all the vblank count, and that's the important part to ensure we don't wait for too long. This really is just all part of the "generic code has no idea about the dpms state of a crtc". I'll plan to fix that with the missing dpms on atomic pieces. -Daniel
On Wed, Nov 5, 2014 at 4:44 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > I've applied all the other nits, replies to the more interesting bits > below. > > On Wed, Nov 05, 2014 at 01:53:48PM -0500, Sean Paul wrote: >> On Sun, Nov 02, 2014 at 02:19:23PM +0100, Daniel Vetter wrote: >> > + 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. > > Hm, that would mean either a goto (I don't like that for non-exceptional > control flow) or duplicating the debug output. I'll go with the latter and > frob it a bit. > I think you can just move the debug output above the new_encoder == connector_state->best_encoder check. >> > + 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 > > Hm, yeah. I'll rename to num_connectors and add a !!. The idea of > returning an int and not just a bool is that drivers might care if there's > more than one, i.e. for cloned setups. At least i915 has some special > considerations for that in some cases. > Makes sense, it just jumped out at me as something that might not be future-safe. > >> > + 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 > > vblank_get should block on anything, and I'm not sure whether the > drm_vblank_count can't just fall over if vblank_get fails (since > vblank_get before vblank_count is the pattern drm_irq.c uses iirc). So I'd > prefer it this way round just for paranoia. So I think I'll stick with > this scheme. The waiting is only done once we've grabbed all the vblank > count, and that's the important part to ensure we don't wait for too long. > > This really is just all part of the "generic code has no idea about the > dpms state of a crtc". I'll plan to fix that with the missing dpms on > atomic pieces. Ok, sounds reasonable. The fixup in your tree looks good, feel free to add my R-b when you squash. Sean > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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) { + 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", + 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) { + 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) + return ret; + + has_connectors = drm_atomic_connectors_for_crtc(state, + crtc); + + if (crtc_state->enable != has_connectors) { + 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. + */ +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); + } + + 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 + * 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; };
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 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 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> --- 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(+)