<div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Thu, Apr 6, 2017 at 12:34 AM Maarten Lankhorst <<a href="mailto:maarten.lankhorst@linux.intel.com">maarten.lankhorst@linux.intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Op 06-04-17 om 03:27 schreef Pandiyan, Dhinakaran: > On Wed, 2017-04-05 at 12:06 +0200, Daniel Vetter wrote: >> On Wed, Apr 05, 2017 at 10:41:24AM +0200, Maarten Lankhorst wrote: >>> The connector atomic check function may be called multiple times, >>> or not at all. It's mostly useful for implementing properties but if you >>> call check_modeset twice it can be used for other modeset related checks >>> as well. >>> >>> Signed-off-by: Maarten Lankhorst <<a href="mailto:maarten.lankhorst@linux.intel.com" class="gmail_msg" target="_blank">maarten.lankhorst@linux.intel.com</a>> >>> --- >>> drivers/gpu/drm/drm_atomic_helper.c | 25 +++++++++++++++++---- >>> include/drm/drm_modeset_helper_vtables.h | 37 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 58 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >>> index c3994b4d5f32..d550e79e8347 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -459,10 +459,20 @@ mode_fixup(struct drm_atomic_state *state) >>> * >>> * Check the state object to see if the requested state is physically possible. >>> * This does all the crtc and connector related computations for an atomic >>> - * update and adds any additional connectors needed for full modesets and calls >>> - * down into &drm_crtc_helper_funcs.mode_fixup and >>> - * &drm_encoder_helper_funcs.mode_fixup or >>> - * &drm_encoder_helper_funcs.atomic_check functions of the driver backend. >>> + * update and adds any additional connectors needed for full modesets. It calls >>> + * the various per-object callbacks in the follow order: >>> + * >>> + * 1. &drm_connector_helper_funcs.atomic_best_encoder for determining the new encoder. >>> + * 2. &drm_connector_helper_funcs.atomic_check to validate the connector state. >>> + * 3. If it's determined a modeset is needed then all connectors on the affected crtc >>> + * crtc are added. > Typo - 'crtc' typed twice. > > update_output_state which is called before _helper_check_modeset() also > adds all affected connectors. Why is that not considered when you say > affected connectors are added in Step 3? Is that because > update_output_state() is in the legacy modeset path? Yes. update_output_state is just a legacy function. When a crtc is cloned, a atomic update may only disable 1 connector and leave the other one enabled. In this case there's a modeset but all connectors are not checked yet. Similar for changing crtc_state->active, which may also result in a modeset with no connectors added yet. > >>> + * 4. &drm_bridge_funcs.mode_fixup is called on all encoder bridges. >>> + * 5. &drm_encoder_helper_funcs.atomic_check is called to validate any encoder state. >>> + * This function is only called when the encoder will be part of a configured crtc, >>> + * it must not be used for implementing connector property validation. >>> + * If this function is NULL, &drm_atomic_encoder_helper_funcs.mode_fixup is called >>> + * instead. >>> + * 6. &drm_crtc_helper_funcs.mode_fixup is called last, to fix up the mode with crtc constraints. >> Line too I think. And maybe insert empty lines between each item, to make >> them stand out more. Shouldn't affect rendering, but better to recheck. >> >>> * >>> * &drm_crtc_state.mode_changed is set when the input mode is changed. >>> * &drm_crtc_state.connectors_changed is set when a connector is added or >>> @@ -522,6 +532,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >>> return ret; >>> >>> for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { >>> + const struct drm_connector_helper_funcs *funcs = connector->helper_private; >>> + >>> /* >>> * This only sets crtc->connectors_changed for routing changes, >>> * drivers must set crtc->connectors_changed themselves when >>> @@ -539,6 +551,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, >>> new_connector_state->link_status) >>> new_crtc_state->connectors_changed = true; >>> } >>> + >>> + if (funcs->atomic_check) >>> + ret = funcs->atomic_check(connector, new_connector_state); >>> + if (ret) >>> + return ret; >>> } >>> >>> /* >>> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h >>> index b304950b402d..7b5dd909f189 100644 >>> --- a/include/drm/drm_modeset_helper_vtables.h >>> +++ b/include/drm/drm_modeset_helper_vtables.h >>> @@ -860,6 +860,43 @@ struct drm_connector_helper_funcs { >>> */ >>> struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector, >>> struct drm_connector_state *connector_state); >>> + >>> + /** >>> + * @atomic_check: >>> + * >>> + * Drivers should check connector properties in this >>> + * hook. This function is called from &drm_atomic_helper_check_modeset. >>> + * >>> + * This function may not be called at all on a modeset or connector >>> + * change, so it should only be used to validate properties. > I did not understand this part - "function may not be called at all on a > modeset or connector change". Why is that? I meant moving one connector to a different crtc, if there are 2 connectors on one of the crtc the check function for the other one may not be called the first time around. I guess that falls under modeset, so that part may be killed. </blockquote><div> </div>With the "crtc crtc" typo fixed and this change, Reviewed-by: Dhinakaran Pandiyan <<a href="mailto:dhinakaran.pandiyan@intel.com">dhinakaran.pandiyan@intel.com</a>></div><div class="gmail_quote">-DK</div><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> >>> + * If it's required to validate all connectors from there, call >>> + * &drm_atomic_helper_check_modeset twice. This will result in > s/twice/again ? Indeed. >>> + * atomic_check possibly being called twice as well, which the callback > &drm_connector_helper_funcs.atomic_check? > > > I had to read this a couple of times to understand, but I guess, that's > mostly me trying to piece things together. Yes. _______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail_msg" target="_blank">Intel-gfx@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" class="gmail_msg" target="_blank">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div></div>