Fourth iteration. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Old situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation:
During atomic check: - Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state. - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object. This will return NULL if the object is not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from obj_state->crtc too, this means not having to write some error handling.
During atomic commit: - Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
During both: - Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
When not doing atomic updates: Look at obj->state for now. I have some patches to fix this but I was asked to make it return a const state. This breaks a lot of users though so I skipped that patch in this iteration.
This series will return the correct state regardless of swapping.
Maarten Lankhorst (7): drm/atomic: Add new iterators over all state, v3. drm/atomic: Make add_affected_connectors look at crtc_state. drm/atomic: Use new atomic iterator macros. drm/atomic: Fix atomic helpers to use the new iterator macros. drm/atomic: Add macros to access existing old/new state drm/atomic: Convert get_existing_state callers to get_old/new_state, v2. drm/blend: Use new atomic iterator macros.
drivers/gpu/drm/drm_atomic.c | 39 ++-- drivers/gpu/drm/drm_atomic_helper.c | 377 ++++++++++++++++++++--------------- drivers/gpu/drm/drm_blend.c | 23 +-- drivers/gpu/drm/i915/intel_display.c | 13 +- include/drm/drm_atomic.h | 180 ++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + 6 files changed, 438 insertions(+), 196 deletions(-)
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences.
This requires extra fixups done when committing a state after duplicating, which in general isn't valid but is used by suspend/resume. To handle these, introduce drm_atomic_helper_commit_duplicated_state which performs those fixups before checking & committing the state.
Changes since v1: - Remove nonblock parameter for commit_duplicated_state. Changes since v2: - Use commit_duplicated_state for i915 load detection. - Add WARN_ON(old_state != obj->state) before swapping.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 65 +++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_display.c | 13 +++--- include/drm/drm_atomic.h | 81 ++++++++++++++++++++++++++++++++++-- include/drm/drm_atomic_helper.h | 2 + 5 files changed, 149 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6414bcf7f41b..1c1cbf436717 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -275,6 +275,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, return ERR_PTR(-ENOMEM);
state->crtcs[index].state = crtc_state; + state->crtcs[index].old_state = crtc->state; + state->crtcs[index].new_state = crtc_state; state->crtcs[index].ptr = crtc; crtc_state->state = state;
@@ -691,6 +693,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
state->planes[index].state = plane_state; state->planes[index].ptr = plane; + state->planes[index].old_state = plane->state; + state->planes[index].new_state = plane_state; plane_state->state = state;
DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", @@ -1031,6 +1035,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
drm_connector_reference(connector); state->connectors[index].state = connector_state; + state->connectors[index].old_state = connector->state; + state->connectors[index].new_state = connector_state; state->connectors[index].ptr = connector; connector_state->state = state;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, int i; long ret; struct drm_connector *connector; - struct drm_connector_state *conn_state; + struct drm_connector_state *conn_state, *old_conn_state; struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *crtc_state, *old_crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *plane_state, *old_plane_state; struct drm_crtc_commit *commit;
if (stall) { @@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_connector_in_state(state, connector, conn_state, i) { + for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) { + WARN_ON(connector->state != old_conn_state); + connector->state->state = state; swap(state->connectors[i].state, connector->state); connector->state->state = NULL; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { + WARN_ON(crtc->state != old_crtc_state); + crtc->state->state = state; swap(state->crtcs[i].state, crtc->state); crtc->state->state = NULL; @@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_plane_in_state(state, plane, plane_state, i) { + for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) { + WARN_ON(plane->state != old_plane_state); + plane->state->state = state; swap(state->planes[i].state, plane->state); plane->state->state = NULL; @@ -2471,7 +2477,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); * * See also: * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), - * drm_atomic_helper_resume() + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() */ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) { @@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/** + * drm_atomic_helper_commit_duplicated_state - commit duplicated state + * @state: duplicated atomic state to commit + * @ctx: pointer to acquire_ctx to use for commit. + * + * The state returned by drm_atomic_helper_duplicate_state() and + * drm_atomic_helper_suspend() is partially invalid, and needs to + * be fixed up before commit. + * + * Returns: + * 0 on success or a negative error code on failure. + * + * See also: + * drm_atomic_helper_suspend() + */ +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx) +{ + int i; + struct drm_plane *plane; + struct drm_plane_state *plane_state; + struct drm_connector *connector; + struct drm_connector_state *conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + state->acquire_ctx = ctx; + + for_each_new_plane_in_state(state, plane, plane_state, i) + state->planes[i].old_state = plane->state; + + for_each_new_crtc_in_state(state, crtc, crtc_state, i) + state->crtcs[i].old_state = crtc->state; + + for_each_new_connector_in_state(state, connector, conn_state, i) + state->connectors[i].old_state = connector->state; + + return drm_atomic_commit(state); +} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); + +/** * drm_atomic_helper_resume - subsystem-level resume helper * @dev: DRM device * @state: atomic state to resume to @@ -2534,9 +2581,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, int err;
drm_mode_config_reset(dev); + drm_modeset_lock_all(dev); - state->acquire_ctx = config->acquire_ctx; - err = drm_atomic_commit(state); + err = drm_atomic_helper_commit_duplicated_state(state, config->acquire_ctx); drm_modeset_unlock_all(dev);
return err; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f523256ef77c..74da1c380b32 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3488,7 +3488,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
static int __intel_display_resume(struct drm_device *dev, - struct drm_atomic_state *state) + struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx) { struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; @@ -3512,7 +3513,7 @@ __intel_display_resume(struct drm_device *dev, /* ignore any reset values/BIOS leftovers in the WM registers */ to_intel_atomic_state(state)->skip_intermediate_wm = true;
- ret = drm_atomic_commit(state); + ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
WARN_ON(ret == -EDEADLK); return ret; @@ -3606,7 +3607,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) */ intel_update_primary_planes(dev); } else { - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret); } @@ -3626,7 +3627,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);
- ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret);
@@ -11327,7 +11328,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (!state) return;
- ret = drm_atomic_commit(state); + ret = drm_atomic_helper_commit_duplicated_state(state, ctx); if (ret) DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); drm_atomic_state_put(state); @@ -17210,7 +17211,7 @@ void intel_display_resume(struct drm_device *dev) }
if (!ret) - ret = __intel_display_resume(dev, state); + ret = __intel_display_resume(dev, state, &ctx);
drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index f96220ed4004..6062e7f27325 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -137,12 +137,12 @@ struct drm_crtc_commit {
struct __drm_planes_state { struct drm_plane *ptr; - struct drm_plane_state *state; + struct drm_plane_state *state, *old_state, *new_state; };
struct __drm_crtcs_state { struct drm_crtc *ptr; - struct drm_crtc_state *state; + struct drm_crtc_state *state, *old_state, *new_state; struct drm_crtc_commit *commit; s64 __user *out_fence_ptr; unsigned last_vblank_count; @@ -150,7 +150,7 @@ struct __drm_crtcs_state {
struct __drm_connnectors_state { struct drm_connector *ptr; - struct drm_connector_state *state; + struct drm_connector_state *state, *old_state, *new_state; };
/** @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++) \ for_each_if (connector)
+#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ + (__i)++) \ + for_each_if (connector) + +#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (old_connector_state) = (__state)->connectors[__i].old_state, 1); \ + (__i)++) \ + for_each_if (connector) + +#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->num_connector && \ + ((connector) = (__state)->connectors[__i].ptr, \ + (new_connector_state) = (__state)->connectors[__i].new_state, 1); \ + (__i)++) \ + for_each_if (connector) + #define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_crtc && \ @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++) \ for_each_if (crtc_state)
+#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i)++) \ + for_each_if (crtc) + +#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \ + (__i)++) \ + for_each_if (crtc) + +#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_crtc && \ + ((crtc) = (__state)->crtcs[__i].ptr, \ + (new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \ + (__i)++) \ + for_each_if (crtc) + #define for_each_plane_in_state(__state, plane, plane_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_total_plane && \ @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++) \ for_each_if (plane_state)
+#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i)++) \ + for_each_if (plane) + +#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (old_plane_state) = (__state)->planes[__i].old_state, 1); \ + (__i)++) \ + for_each_if (plane) + +#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \ + for ((__i) = 0; \ + (__i) < (__state)->dev->mode_config.num_total_plane && \ + ((plane) = (__state)->planes[__i].ptr, \ + (new_plane_state) = (__state)->planes[__i].new_state, 1); \ + (__i)++) \ + for_each_if (plane) + /** * drm_atomic_crtc_needs_modeset - compute combined modeset need * @state: &drm_crtc_state for the CRTC diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9afcd3810785..2c4549e98c16 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -105,6 +105,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx); int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state);
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences.
This requires extra fixups done when committing a state after duplicating, which in general isn't valid but is used by suspend/resume. To handle these, introduce drm_atomic_helper_commit_duplicated_state which performs those fixups before checking & committing the state.
Changes since v1:
- Remove nonblock parameter for commit_duplicated_state.
Changes since v2:
- Use commit_duplicated_state for i915 load detection.
- Add WARN_ON(old_state != obj->state) before swapping.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 65 +++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_display.c | 13 +++--- include/drm/drm_atomic.h | 81 +++++++++++++++++++++++++++++++-- include/drm/drm_atomic_helper.h | 2 + 5 files changed, 149 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6414bcf7f41b..1c1cbf436717 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -275,6 +275,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, return ERR_PTR(-ENOMEM);
state->crtcs[index].state = crtc_state;
- state->crtcs[index].old_state = crtc->state;
- state->crtcs[index].new_state = crtc_state; state->crtcs[index].ptr = crtc; crtc_state->state = state;
@@ -691,6 +693,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
state->planes[index].state = plane_state; state->planes[index].ptr = plane;
state->planes[index].old_state = plane->state;
state->planes[index].new_state = plane_state; plane_state->state = state;
DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
@@ -1031,6 +1035,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
drm_connector_reference(connector); state->connectors[index].state = connector_state;
- state->connectors[index].old_state = connector->state;
- state->connectors[index].new_state = connector_state; state->connectors[index].ptr = connector; connector_state->state = state;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, int i; long ret; struct drm_connector *connector;
- struct drm_connector_state *conn_state;
- struct drm_connector_state *conn_state, *old_conn_state; struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_crtc_state *crtc_state, *old_crtc_state; struct drm_plane *plane;
- struct drm_plane_state *plane_state;
struct drm_plane_state *plane_state, *old_plane_state; struct drm_crtc_commit *commit;
if (stall) {
@@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_connector_in_state(state, connector, conn_state, i) {
- for_each_oldnew_connector_in_state(state, connector, old_conn_state,
conn_state, i) {
WARN_ON(connector->state != old_conn_state);- connector->state->state = state; swap(state->connectors[i].state, connector->state); connector->state->state = NULL; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state,
i) {
WARN_ON(crtc->state != old_crtc_state);- crtc->state->state = state; swap(state->crtcs[i].state, crtc->state); crtc->state->state = NULL;
@@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_plane_in_state(state, plane, plane_state, i) {
- for_each_oldnew_plane_in_state(state, plane, old_plane_state,
plane_state, i) {
WARN_ON(plane->state != old_plane_state);- plane->state->state = state; swap(state->planes[i].state, plane->state); plane->state->state = NULL;
@@ -2471,7 +2477,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
- See also:
- drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
- drm_atomic_helper_resume()
*/
- drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) { @@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/**
- drm_atomic_helper_commit_duplicated_state - commit duplicated state
- @state: duplicated atomic state to commit
- @ctx: pointer to acquire_ctx to use for commit.
- The state returned by drm_atomic_helper_duplicate_state() and
- drm_atomic_helper_suspend() is partially invalid, and needs to
- be fixed up before commit.
Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for this function ?
Apart from this the patch looks good to me. I don't like the fact that we now have two sets of states though (state and old_state/new_state). I understand that you'd like to address this as part of a separate patch series, which I'm fine with to avoid delaying this one, but I think we should address this sooner rather than latter, or the API will become very confusing. Yes, that means mass-patching drivers I'm afraid. Could you confirm that this is your plan ?
- Returns:
- 0 on success or a negative error code on failure.
- See also:
- drm_atomic_helper_suspend()
- */
+int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
struct drm_modeset_acquire_ctx*ctx) +{
- int i;
- struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_connector *connector;
- struct drm_connector_state *conn_state;
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- state->acquire_ctx = ctx;
- for_each_new_plane_in_state(state, plane, plane_state, i)
state->planes[i].old_state = plane->state;- for_each_new_crtc_in_state(state, crtc, crtc_state, i)
state->crtcs[i].old_state = crtc->state;- for_each_new_connector_in_state(state, connector, conn_state, i)
state->connectors[i].old_state = connector->state;- return drm_atomic_commit(state);
+} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
+/**
- drm_atomic_helper_resume - subsystem-level resume helper
- @dev: DRM device
- @state: atomic state to resume to
@@ -2534,9 +2581,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, int err;
drm_mode_config_reset(dev);
- drm_modeset_lock_all(dev);
- state->acquire_ctx = config->acquire_ctx;
- err = drm_atomic_commit(state);
- err = drm_atomic_helper_commit_duplicated_state(state,
config->acquire_ctx); drm_modeset_unlock_all(dev);
return err; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f523256ef77c..74da1c380b32 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3488,7 +3488,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
static int __intel_display_resume(struct drm_device *dev,
struct drm_atomic_state *state)
struct drm_atomic_state *state,struct drm_modeset_acquire_ctx *ctx){ struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; @@ -3512,7 +3513,7 @@ __intel_display_resume(struct drm_device *dev, /* ignore any reset values/BIOS leftovers in the WM registers */ to_intel_atomic_state(state)->skip_intermediate_wm = true;
- ret = drm_atomic_commit(state);
ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
WARN_ON(ret == -EDEADLK); return ret;
@@ -3606,7 +3607,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) */ intel_update_primary_planes(dev); } else {
ret = __intel_display_resume(dev, state);
ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with
%i\n", ret);
}@@ -3626,7 +3627,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);
ret = __intel_display_resume(dev, state);
ret = __intel_display_resume(dev, state, ctx);
ret);
@@ -11327,7 +11328,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (!state) return;
- ret = drm_atomic_commit(state);
- ret = drm_atomic_helper_commit_duplicated_state(state, ctx); if (ret) DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); drm_atomic_state_put(state);
@@ -17210,7 +17211,7 @@ void intel_display_resume(struct drm_device *dev) }
if (!ret)
ret = __intel_display_resume(dev, state);
ret = __intel_display_resume(dev, state, &ctx);drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index f96220ed4004..6062e7f27325 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -137,12 +137,12 @@ struct drm_crtc_commit {
struct __drm_planes_state { struct drm_plane *ptr;
- struct drm_plane_state *state;
- struct drm_plane_state *state, *old_state, *new_state;
};
struct __drm_crtcs_state { struct drm_crtc *ptr;
- struct drm_crtc_state *state;
- struct drm_crtc_state *state, *old_state, *new_state; struct drm_crtc_commit *commit; s64 __user *out_fence_ptr; unsigned last_vblank_count;
@@ -150,7 +150,7 @@ struct __drm_crtcs_state {
struct __drm_connnectors_state { struct drm_connector *ptr;
- struct drm_connector_state *state;
- struct drm_connector_state *state, *old_state, *new_state;
};
/** @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++)
\
for_each_if (connector)+#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \
- for ((__i) = 0;
\
(__i) < (__state)->num_connector &&
\
((connector) = (__state)->connectors[__i].ptr,
\
(old_connector_state) = (__state)->connectors[__i].old_state,
\
(new_connector_state) = (__state)->connectors[__i].new_state, 1);
\
(__i)++) \for_each_if (connector)+#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->num_connector &&
\
((connector) = (__state)->connectors[__i].ptr,
\
(old_connector_state) = (__state)->connectors[__i].old_state, 1);
\
(__i)++) \for_each_if (connector)+#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->num_connector &&
\
((connector) = (__state)->connectors[__i].ptr,
\
(new_connector_state) = (__state)->connectors[__i].new_state, 1);
\
(__i)++) \for_each_if (connector)#define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_crtc && \ @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++) \ for_each_if (crtc_state)
+#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \(__i)++) \for_each_if (crtc)+#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i)
\
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \(__i)++) \for_each_if (crtc)+#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i)
\
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \(__i)++) \for_each_if (crtc)#define for_each_plane_in_state(__state, plane, plane_state, __i)
\
for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_total_plane && \ @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++)
\
for_each_if (plane_state)+#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \(__i)++) \for_each_if (plane)+#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1); \(__i)++) \for_each_if (plane)+#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \(__i)++) \for_each_if (plane)/**
- drm_atomic_crtc_needs_modeset - compute combined modeset need
- @state: &drm_crtc_state for the CRTC
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9afcd3810785..2c4549e98c16 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -105,6 +105,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
struct drm_modeset_acquire_ctx*ctx); int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state);
Op 17-01-17 om 00:11 schreef Laurent Pinchart:
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences.
This requires extra fixups done when committing a state after duplicating, which in general isn't valid but is used by suspend/resume. To handle these, introduce drm_atomic_helper_commit_duplicated_state which performs those fixups before checking & committing the state.
Changes since v1:
- Remove nonblock parameter for commit_duplicated_state.
Changes since v2:
- Use commit_duplicated_state for i915 load detection.
- Add WARN_ON(old_state != obj->state) before swapping.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 65 +++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_display.c | 13 +++--- include/drm/drm_atomic.h | 81 +++++++++++++++++++++++++++++++-- include/drm/drm_atomic_helper.h | 2 + 5 files changed, 149 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 6414bcf7f41b..1c1cbf436717 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -275,6 +275,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, return ERR_PTR(-ENOMEM);
state->crtcs[index].state = crtc_state;
- state->crtcs[index].old_state = crtc->state;
- state->crtcs[index].new_state = crtc_state; state->crtcs[index].ptr = crtc; crtc_state->state = state;
@@ -691,6 +693,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
state->planes[index].state = plane_state; state->planes[index].ptr = plane;
state->planes[index].old_state = plane->state;
state->planes[index].new_state = plane_state; plane_state->state = state;
DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
@@ -1031,6 +1035,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
drm_connector_reference(connector); state->connectors[index].state = connector_state;
- state->connectors[index].old_state = connector->state;
- state->connectors[index].new_state = connector_state; state->connectors[index].ptr = connector; connector_state->state = state;
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1971,11 +1971,11 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, int i; long ret; struct drm_connector *connector;
- struct drm_connector_state *conn_state;
- struct drm_connector_state *conn_state, *old_conn_state; struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_crtc_state *crtc_state, *old_crtc_state; struct drm_plane *plane;
- struct drm_plane_state *plane_state;
struct drm_plane_state *plane_state, *old_plane_state; struct drm_crtc_commit *commit;
if (stall) {
@@ -1999,13 +1999,17 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_connector_in_state(state, connector, conn_state, i) {
- for_each_oldnew_connector_in_state(state, connector, old_conn_state,
conn_state, i) {
WARN_ON(connector->state != old_conn_state);- connector->state->state = state; swap(state->connectors[i].state, connector->state); connector->state->state = NULL; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state,
i) {
WARN_ON(crtc->state != old_crtc_state);- crtc->state->state = state; swap(state->crtcs[i].state, crtc->state); crtc->state->state = NULL;
@@ -2020,7 +2024,9 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_plane_in_state(state, plane, plane_state, i) {
- for_each_oldnew_plane_in_state(state, plane, old_plane_state,
plane_state, i) {
WARN_ON(plane->state != old_plane_state);- plane->state->state = state; swap(state->planes[i].state, plane->state); plane->state->state = NULL;
@@ -2471,7 +2477,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all);
- See also:
- drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(),
- drm_atomic_helper_resume()
*/
- drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state()
struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) { @@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/**
- drm_atomic_helper_commit_duplicated_state - commit duplicated state
- @state: duplicated atomic state to commit
- @ctx: pointer to acquire_ctx to use for commit.
- The state returned by drm_atomic_helper_duplicate_state() and
- drm_atomic_helper_suspend() is partially invalid, and needs to
- be fixed up before commit.
Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for this function ?
We would have to fix up other areas that duplicate state too, like i915 suspend and load detect. This means moving it to a helper.
It was my original approach, but Daniel preferred this approach.
Apart from this the patch looks good to me. I don't like the fact that we now have two sets of states though (state and old_state/new_state). I understand that you'd like to address this as part of a separate patch series, which I'm fine with to avoid delaying this one, but I think we should address this sooner rather than latter, or the API will become very confusing. Yes, that means mass-patching drivers I'm afraid. Could you confirm that this is your plan ?
Yes, working on it.
- Returns:
- 0 on success or a negative error code on failure.
- See also:
- drm_atomic_helper_suspend()
- */
+int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
struct drm_modeset_acquire_ctx*ctx) +{
- int i;
- struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_connector *connector;
- struct drm_connector_state *conn_state;
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- state->acquire_ctx = ctx;
- for_each_new_plane_in_state(state, plane, plane_state, i)
state->planes[i].old_state = plane->state;- for_each_new_crtc_in_state(state, crtc, crtc_state, i)
state->crtcs[i].old_state = crtc->state;- for_each_new_connector_in_state(state, connector, conn_state, i)
state->connectors[i].old_state = connector->state;- return drm_atomic_commit(state);
+} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
+/**
- drm_atomic_helper_resume - subsystem-level resume helper
- @dev: DRM device
- @state: atomic state to resume to
@@ -2534,9 +2581,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, int err;
drm_mode_config_reset(dev);
- drm_modeset_lock_all(dev);
- state->acquire_ctx = config->acquire_ctx;
- err = drm_atomic_commit(state);
- err = drm_atomic_helper_commit_duplicated_state(state,
config->acquire_ctx); drm_modeset_unlock_all(dev);
return err; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f523256ef77c..74da1c380b32 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3488,7 +3488,8 @@ static void intel_update_primary_planes(struct drm_device *dev)
static int __intel_display_resume(struct drm_device *dev,
struct drm_atomic_state *state)
struct drm_atomic_state *state,struct drm_modeset_acquire_ctx *ctx){ struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; @@ -3512,7 +3513,7 @@ __intel_display_resume(struct drm_device *dev, /* ignore any reset values/BIOS leftovers in the WM registers */ to_intel_atomic_state(state)->skip_intermediate_wm = true;
- ret = drm_atomic_commit(state);
ret = drm_atomic_helper_commit_duplicated_state(state, ctx);
WARN_ON(ret == -EDEADLK); return ret;
@@ -3606,7 +3607,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) */ intel_update_primary_planes(dev); } else {
ret = __intel_display_resume(dev, state);
ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with%i\n", ret);
}@@ -3626,7 +3627,7 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) dev_priv->display.hpd_irq_setup(dev_priv); spin_unlock_irq(&dev_priv->irq_lock);
ret = __intel_display_resume(dev, state);
ret = __intel_display_resume(dev, state, ctx);ret);
@@ -11327,7 +11328,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, if (!state) return;
- ret = drm_atomic_commit(state);
- ret = drm_atomic_helper_commit_duplicated_state(state, ctx); if (ret) DRM_DEBUG_KMS("Couldn't release load detect pipe: %i\n", ret); drm_atomic_state_put(state);
@@ -17210,7 +17211,7 @@ void intel_display_resume(struct drm_device *dev) }
if (!ret)
ret = __intel_display_resume(dev, state);
ret = __intel_display_resume(dev, state, &ctx);drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index f96220ed4004..6062e7f27325 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -137,12 +137,12 @@ struct drm_crtc_commit {
struct __drm_planes_state { struct drm_plane *ptr;
- struct drm_plane_state *state;
- struct drm_plane_state *state, *old_state, *new_state;
};
struct __drm_crtcs_state { struct drm_crtc *ptr;
- struct drm_crtc_state *state;
- struct drm_crtc_state *state, *old_state, *new_state; struct drm_crtc_commit *commit; s64 __user *out_fence_ptr; unsigned last_vblank_count;
@@ -150,7 +150,7 @@ struct __drm_crtcs_state {
struct __drm_connnectors_state { struct drm_connector *ptr;
- struct drm_connector_state *state;
- struct drm_connector_state *state, *old_state, *new_state;
};
/** @@ -397,6 +397,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++)
\
for_each_if (connector)+#define for_each_oldnew_connector_in_state(__state, connector, old_connector_state, new_connector_state, __i) \
- for ((__i) = 0;
\
(__i) < (__state)->num_connector &&\
((connector) = (__state)->connectors[__i].ptr,\
(old_connector_state) = (__state)->connectors[__i].old_state,\
(new_connector_state) = (__state)->connectors[__i].new_state, 1);\
(__i)++) \for_each_if (connector)+#define for_each_old_connector_in_state(__state, connector, old_connector_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->num_connector &&\
((connector) = (__state)->connectors[__i].ptr,\
(old_connector_state) = (__state)->connectors[__i].old_state, 1);\
(__i)++) \for_each_if (connector)+#define for_each_new_connector_in_state(__state, connector, new_connector_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->num_connector &&\
((connector) = (__state)->connectors[__i].ptr,\
(new_connector_state) = (__state)->connectors[__i].new_state, 1);\
(__i)++) \for_each_if (connector)#define for_each_crtc_in_state(__state, crtc, crtc_state, __i) \ for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_crtc && \ @@ -405,6 +430,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++) \ for_each_if (crtc_state)
+#define for_each_oldnew_crtc_in_state(__state, crtc, old_crtc_state, new_crtc_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \(__i)++) \for_each_if (crtc)+#define for_each_old_crtc_in_state(__state, crtc, old_crtc_state, __i)
\
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(old_crtc_state) = (__state)->crtcs[__i].old_state, 1); \(__i)++) \for_each_if (crtc)+#define for_each_new_crtc_in_state(__state, crtc, new_crtc_state, __i)
\
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_crtc && \((crtc) = (__state)->crtcs[__i].ptr, \(new_crtc_state) = (__state)->crtcs[__i].new_state, 1); \(__i)++) \for_each_if (crtc)#define for_each_plane_in_state(__state, plane, plane_state, __i)
\
for ((__i) = 0; \ (__i) < (__state)->dev->mode_config.num_total_plane && \ @@ -413,6 +463,31 @@ void drm_state_dump(struct drm_device *dev, struct drm_printer *p); (__i)++)
\
for_each_if (plane_state)+#define for_each_oldnew_plane_in_state(__state, plane, old_plane_state, new_plane_state, __i) \ + for ((__i) = 0;
\
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \(__i)++) \for_each_if (plane)+#define for_each_old_plane_in_state(__state, plane, old_plane_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(old_plane_state) = (__state)->planes[__i].old_state, 1); \(__i)++) \for_each_if (plane)+#define for_each_new_plane_in_state(__state, plane, new_plane_state, __i) \
- for ((__i) = 0; \
(__i) < (__state)->dev->mode_config.num_total_plane && \((plane) = (__state)->planes[__i].ptr, \(new_plane_state) = (__state)->planes[__i].new_state, 1); \(__i)++) \for_each_if (plane)/**
- drm_atomic_crtc_needs_modeset - compute combined modeset need
- @state: &drm_crtc_state for the CRTC
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 9afcd3810785..2c4549e98c16 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -105,6 +105,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
struct drm_modeset_acquire_ctx*ctx); int drm_atomic_helper_resume(struct drm_device *dev, struct drm_atomic_state *state);
Hi Maarten,
On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
Op 17-01-17 om 00:11 schreef Laurent Pinchart:
On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences.
This requires extra fixups done when committing a state after duplicating, which in general isn't valid but is used by suspend/resume. To handle these, introduce drm_atomic_helper_commit_duplicated_state which performs those fixups before checking & committing the state.
Changes since v1:
- Remove nonblock parameter for commit_duplicated_state.
Changes since v2:
- Use commit_duplicated_state for i915 load detection.
- Add WARN_ON(old_state != obj->state) before swapping.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 65 +++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_display.c | 13 +++--- include/drm/drm_atomic.h | 81 ++++++++++++++++++++++++++++-- include/drm/drm_atomic_helper.h | 2 + 5 files changed, 149 insertions(+), 18 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26e3419027e..d153e8a72921 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
@@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/**
- drm_atomic_helper_commit_duplicated_state - commit duplicated state
- @state: duplicated atomic state to commit
- @ctx: pointer to acquire_ctx to use for commit.
- The state returned by drm_atomic_helper_duplicate_state() and
- drm_atomic_helper_suspend() is partially invalid, and needs to
- be fixed up before commit.
Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for this function ?
We would have to fix up other areas that duplicate state too, like i915 suspend and load detect. This means moving it to a helper.
It was my original approach, but Daniel preferred this approach.
We have to fix it somewhere, that's for sure. I think it's better to fix it as close as possible to state duplication, instead of carrying a state that we know is invalid and fix it at the last minute. The drawback of this approach is that the window during which the state will be invalid is much larger, which could cause bugs later when new code will try to touch that state.
Daniel, is there a specific reason why you prefer this approach ?
Apart from this the patch looks good to me. I don't like the fact that we now have two sets of states though (state and old_state/new_state). I understand that you'd like to address this as part of a separate patch series, which I'm fine with to avoid delaying this one, but I think we should address this sooner rather than latter, or the API will become very confusing. Yes, that means mass-patching drivers I'm afraid. Could you confirm that this is your plan ?
Yes, working on it.
- Returns:
- 0 on success or a negative error code on failure.
- See also:
- drm_atomic_helper_suspend()
- */
+int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state,
struct drm_modeset_acquire_ctx*ctx) +{
- int i;
- struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_connector *connector;
- struct drm_connector_state *conn_state;
- struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- state->acquire_ctx = ctx;
- for_each_new_plane_in_state(state, plane, plane_state, i)
state->planes[i].old_state = plane->state;- for_each_new_crtc_in_state(state, crtc, crtc_state, i)
state->crtcs[i].old_state = crtc->state;- for_each_new_connector_in_state(state, connector, conn_state, i)
state->connectors[i].old_state = connector->state;- return drm_atomic_commit(state);
+} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state);
[snip]
On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote:
On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
Op 17-01-17 om 00:11 schreef Laurent Pinchart:
On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
@@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/**
- drm_atomic_helper_commit_duplicated_state - commit duplicated state
- @state: duplicated atomic state to commit
- @ctx: pointer to acquire_ctx to use for commit.
- The state returned by drm_atomic_helper_duplicate_state() and
- drm_atomic_helper_suspend() is partially invalid, and needs to
- be fixed up before commit.
Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for this function ?
We would have to fix up other areas that duplicate state too, like i915 suspend and load detect. This means moving it to a helper.
It was my original approach, but Daniel preferred this approach.
We have to fix it somewhere, that's for sure. I think it's better to fix it as close as possible to state duplication, instead of carrying a state that we know is invalid and fix it at the last minute. The drawback of this approach is that the window during which the state will be invalid is much larger, which could cause bugs later when new code will try to touch that state.
Daniel, is there a specific reason why you prefer this approach ?
You can't fix it in suspend? The issue is that on resume, we have reset states (to reflect the hw state of everything being off), so we can only do this on commit. That holds in general for the duplicate, do something nasty, restore state pattern.
And then I think a dedicated helper to commit duplicated state makes sense. The kernel-doc might need a bit of work to explain this better though. I think it should explain what exactly is partially invalid with duplicated states. -Daniel
Hi Daniel,
On Monday 23 Jan 2017 09:48:54 Daniel Vetter wrote:
On Thu, Jan 19, 2017 at 12:56:29AM +0200, Laurent Pinchart wrote:
On Tuesday 17 Jan 2017 08:41:03 Maarten Lankhorst wrote:
Op 17-01-17 om 00:11 schreef Laurent Pinchart:
On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
@@ -2512,6 +2518,47 @@ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) EXPORT_SYMBOL(drm_atomic_helper_suspend);
/**
- drm_atomic_helper_commit_duplicated_state - commit duplicated
state
- @state: duplicated atomic state to commit
- @ctx: pointer to acquire_ctx to use for commit.
- The state returned by drm_atomic_helper_duplicate_state() and
- drm_atomic_helper_suspend() is partially invalid, and needs to
- be fixed up before commit.
Can't you fix the state in drm_atomic_helper_suspend() to avoid the need for this function ?
We would have to fix up other areas that duplicate state too, like i915 suspend and load detect. This means moving it to a helper.
It was my original approach, but Daniel preferred this approach.
We have to fix it somewhere, that's for sure. I think it's better to fix it as close as possible to state duplication, instead of carrying a state that we know is invalid and fix it at the last minute. The drawback of this approach is that the window during which the state will be invalid is much larger, which could cause bugs later when new code will try to touch that state.
Daniel, is there a specific reason why you prefer this approach ?
You can't fix it in suspend? The issue is that on resume, we have reset states (to reflect the hw state of everything being off), so we can only do this on commit. That holds in general for the duplicate, do something nasty, restore state pattern.
Ok, I got it now. You can't fix the state up at suspend time because you need to set the old_state pointers to the state allocated by the .reset() handlers, which are called at resume time.
And then I think a dedicated helper to commit duplicated state makes sense. The kernel-doc might need a bit of work to explain this better though. I think it should explain what exactly is partially invalid with duplicated states.
Yes, the documentation should be clarified, and include the above explanation in some form.
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:38 Maarten Lankhorst wrote:
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences.
This requires extra fixups done when committing a state after duplicating, which in general isn't valid but is used by suspend/resume. To handle these, introduce drm_atomic_helper_commit_duplicated_state which performs those fixups before checking & committing the state.
Changes since v1:
- Remove nonblock parameter for commit_duplicated_state.
Changes since v2:
- Use commit_duplicated_state for i915 load detection.
- Add WARN_ON(old_state != obj->state) before swapping.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 65 +++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_display.c | 13 +++--- include/drm/drm_atomic.h | 81 +++++++++++++++++++++++++++++++-- include/drm/drm_atomic_helper.h | 2 + 5 files changed, 149 insertions(+), 18 deletions(-)
This kills another dereference of connector->state. connector_mask holds all unchanged connectors at least and any changed connectors are already in state anyway.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1c1cbf436717..18cdf2c956c6 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1419,8 +1419,13 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, struct drm_connector *connector; struct drm_connector_state *conn_state; struct drm_connector_list_iter conn_iter; + struct drm_crtc_state *crtc_state; int ret;
+ crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx); if (ret) return ret; @@ -1429,12 +1434,12 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, crtc->base.id, crtc->name, state);
/* - * Changed connectors are already in @state, so only need to look at the - * current configuration. + * Changed connectors are already in @state, so only need to look + * at the connector_mask in crtc_state. */ drm_connector_list_iter_get(state->dev, &conn_iter); drm_for_each_connector_iter(connector, &conn_iter) { - if (connector->state->crtc != crtc) + if (!(crtc_state->connector_mask & (1 << drm_connector_index(connector)))) continue;
conn_state = drm_atomic_get_connector_state(state, connector);
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:39 Maarten Lankhorst wrote:
This kills another dereference of connector->state. connector_mask holds all unchanged connectors at least and any changed connectors are already in state anyway.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1c1cbf436717..18cdf2c956c6 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1419,8 +1419,13 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, struct drm_connector *connector; struct drm_connector_state *conn_state; struct drm_connector_list_iter conn_iter;
struct drm_crtc_state *crtc_state; int ret;
crtc_state = drm_atomic_get_crtc_state(state, crtc);
if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);ret = drm_modeset_lock(&config->connection_mutex, state->acquire_ctx); if (ret) return ret;
@@ -1429,12 +1434,12 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state, crtc->base.id, crtc->name, state);
/*
* Changed connectors are already in @state, so only need to look at
the
* current configuration.
* Changed connectors are already in @state, so only need to look* at the connector_mask in crtc_state.
if (connector->state->crtc != crtc)
if (!(crtc_state->connector_mask & (1 <<drm_connector_index(connector)))) continue;
conn_state = drm_atomic_get_connector_state(state, connector);
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 18cdf2c956c6..7b61e0da9ace 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i) { + for_each_new_plane_in_state(state, plane, plane_state, i) { ret = drm_atomic_plane_check(plane, plane_state); if (ret) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n", @@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { ret = drm_atomic_crtc_check(crtc, crtc_state); if (ret) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check failed\n", @@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) ret = config->funcs->atomic_check(state->dev, state);
if (!state->allow_modeset) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (drm_atomic_crtc_needs_modeset(crtc_state)) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full modeset\n", crtc->base.id, crtc->name); @@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i) + for_each_new_plane_in_state(state, plane, plane_state, i) drm_atomic_plane_print_state(&p, plane_state);
- for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_atomic_crtc_print_state(&p, crtc_state);
- for_each_connector_in_state(state, connector, connector_state, i) + for_each_new_connector_in_state(state, connector, connector_state, i) drm_atomic_connector_print_state(&p, connector_state); }
@@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) return 0;
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { u64 __user *fence_ptr;
fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc); @@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device *dev, return; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { /* * TEST_ONLY and PAGE_FLIP_EVENT are mutually * exclusive, if they weren't, this code should be
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:40 Maarten Lankhorst wrote:
Could we please get a description ? Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 18cdf2c956c6..7b61e0da9ace 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i) {
- for_each_new_plane_in_state(state, plane, plane_state, i) { ret = drm_atomic_plane_check(plane, plane_state); if (ret) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check
failed\n",
@@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) { ret = drm_atomic_crtc_check(crtc, crtc_state); if (ret) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check
failed\n",
@@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) ret = config->funcs->atomic_check(state->dev, state);
if (!state->allow_modeset) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (drm_atomic_crtc_needs_modeset(crtc_state)) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full
modeset\n",
crtc->base.id, crtc->name);@@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i)
- for_each_new_plane_in_state(state, plane, plane_state, i) drm_atomic_plane_print_state(&p, plane_state);
- for_each_crtc_in_state(state, crtc, crtc_state, i)
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_atomic_crtc_print_state(&p, crtc_state);
- for_each_connector_in_state(state, connector, connector_state, i)
- for_each_new_connector_in_state(state, connector, connector_state, i) drm_atomic_connector_print_state(&p, connector_state);
}
@@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) return 0;
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { u64 __user *fence_ptr;
fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
@@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device *dev, return; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) { /*
- TEST_ONLY and PAGE_FLIP_EVENT are mutually
- exclusive, if they weren't, this code should be
On Tue, Jan 17, 2017 at 01:55:50AM +0200, Laurent Pinchart wrote:
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:40 Maarten Lankhorst wrote:
Could we please get a description ? Apart from that,
Typed something small and merged the first 3 patches from this series.
Thanks, Daniel
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 18cdf2c956c6..7b61e0da9ace 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i) {
- for_each_new_plane_in_state(state, plane, plane_state, i) { ret = drm_atomic_plane_check(plane, plane_state); if (ret) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check
failed\n",
@@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) { ret = drm_atomic_crtc_check(crtc, crtc_state); if (ret) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check
failed\n",
@@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) ret = config->funcs->atomic_check(state->dev, state);
if (!state->allow_modeset) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (drm_atomic_crtc_needs_modeset(crtc_state)) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires fullmodeset\n",
crtc->base.id, crtc->name);@@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i)
- for_each_new_plane_in_state(state, plane, plane_state, i) drm_atomic_plane_print_state(&p, plane_state);
- for_each_crtc_in_state(state, crtc, crtc_state, i)
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_atomic_crtc_print_state(&p, crtc_state);
- for_each_connector_in_state(state, connector, connector_state, i)
- for_each_new_connector_in_state(state, connector, connector_state, i) drm_atomic_connector_print_state(&p, connector_state);
}
@@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) return 0;
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { u64 __user *fence_ptr;
fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
@@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device *dev, return; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
- for_each_new_crtc_in_state(state, crtc, crtc_state, i) { /*
- TEST_ONLY and PAGE_FLIP_EVENT are mutually
- exclusive, if they weren't, this code should be
-- Regards,
Laurent Pinchart
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel,
On Tuesday 14 Feb 2017 21:03:07 Daniel Vetter wrote:
On Tue, Jan 17, 2017 at 01:55:50AM +0200, Laurent Pinchart wrote:
On Monday 16 Jan 2017 10:37:40 Maarten Lankhorst wrote:
Could we please get a description ? Apart from that,
Typed something small and merged the first 3 patches from this series.
Thanks. 4 more patches to go. Maarten ? :-)
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 18cdf2c956c6..7b61e0da9ace 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1562,7 +1562,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i) {
for_each_new_plane_in_state(state, plane, plane_state, i) {
ret = drm_atomic_plane_check(plane, plane_state); if (ret) {
DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core checkfailed\n",
@@ -1571,7 +1571,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) }
}
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
ret = drm_atomic_crtc_check(crtc, crtc_state); if (ret) {
DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core checkfailed\n",
@@ -1584,7 +1584,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state) ret = config->funcs->atomic_check(state->dev, state);
if (!state->allow_modeset) {
for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (drm_atomic_crtc_needs_modeset(crtc_state)) {DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full
modeset\n",
crtc->base.id, crtc->name);@@ -1668,13 +1668,13 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
DRM_DEBUG_ATOMIC("checking %p\n", state);
- for_each_plane_in_state(state, plane, plane_state, i)
for_each_new_plane_in_state(state, plane, plane_state, i)
drm_atomic_plane_print_state(&p, plane_state);
- for_each_crtc_in_state(state, crtc, crtc_state, i)
for_each_new_crtc_in_state(state, crtc, crtc_state, i)
drm_atomic_crtc_print_state(&p, crtc_state);
- for_each_connector_in_state(state, connector, connector_state, i)
for_each_new_connector_in_state(state, connector, connector_state, i)
drm_atomic_connector_print_state(&p, connector_state);
}
@@ -1961,7 +1961,7 @@ static int prepare_crtc_signaling(struct drm_device *dev, if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
return 0;
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
u64 __user *fence_ptr;
fence_ptr = get_out_fence_for_crtc(crtc_state->state, crtc);
@@ -2041,7 +2041,7 @@ static void complete_crtc_signaling(struct drm_device *dev, return;
}
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
/*
- TEST_ONLY and PAGE_FLIP_EVENT are mutually
- exclusive, if they weren't, this code should be
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++++----------------- 1 file changed, 152 insertions(+), 141 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -63,14 +63,15 @@ */ static void drm_atomic_helper_plane_changed(struct drm_atomic_state *state, + struct drm_plane_state *old_plane_state, struct drm_plane_state *plane_state, struct drm_plane *plane) { struct drm_crtc_state *crtc_state;
- if (plane->state->crtc) { + if (old_plane_state->crtc) { crtc_state = drm_atomic_get_existing_crtc_state(state, - plane->state->crtc); + old_plane_state->crtc);
if (WARN_ON(!crtc_state)) return; @@ -104,7 +105,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, * part of the state. If the same encoder is assigned to multiple * connectors bail out. */ - for_each_connector_in_state(state, connector, conn_state, i) { + for_each_new_connector_in_state(state, connector, conn_state, i) { const struct drm_connector_helper_funcs *funcs = connector->helper_private; struct drm_encoder *new_encoder;
@@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state, { struct drm_crtc_state *crtc_state; struct drm_connector *connector; - struct drm_connector_state *connector_state; + struct drm_connector_state *old_connector_state, *new_connector_state; int i;
- for_each_connector_in_state(state, connector, connector_state, i) { + for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { struct drm_crtc *encoder_crtc;
- if (connector_state->best_encoder != encoder) + if (new_connector_state->best_encoder != encoder) continue;
- encoder_crtc = connector->state->crtc; + encoder_crtc = old_connector_state->crtc;
DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s], stealing it\n", encoder->base.id, encoder->name, encoder_crtc->base.id, encoder_crtc->name);
- set_best_encoder(state, connector_state, NULL); + set_best_encoder(state, new_connector_state, NULL);
crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc); crtc_state->connectors_changed = true; @@ -272,7 +273,8 @@ steal_encoder(struct drm_atomic_state *state, static int update_connector_routing(struct drm_atomic_state *state, struct drm_connector *connector, - struct drm_connector_state *connector_state) + struct drm_connector_state *old_connector_state, + struct drm_connector_state *new_connector_state) { const struct drm_connector_helper_funcs *funcs; struct drm_encoder *new_encoder; @@ -282,24 +284,24 @@ update_connector_routing(struct drm_atomic_state *state, connector->base.id, connector->name);
- if (connector->state->crtc != connector_state->crtc) { - if (connector->state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, connector->state->crtc); + if (old_connector_state->crtc != new_connector_state->crtc) { + if (old_connector_state->crtc) { + crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc); crtc_state->connectors_changed = true; }
- if (connector_state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc); + if (new_connector_state->crtc) { + crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc); crtc_state->connectors_changed = true; } }
- if (!connector_state->crtc) { + if (!new_connector_state->crtc) { DRM_DEBUG_ATOMIC("Disabling [CONNECTOR:%d:%s]\n", connector->base.id, connector->name);
- set_best_encoder(state, connector_state, NULL); + set_best_encoder(state, new_connector_state, NULL);
return 0; } @@ -308,7 +310,7 @@ update_connector_routing(struct drm_atomic_state *state,
if (funcs->atomic_best_encoder) new_encoder = funcs->atomic_best_encoder(connector, - connector_state); + new_connector_state); else if (funcs->best_encoder) new_encoder = funcs->best_encoder(connector); else @@ -321,33 +323,33 @@ update_connector_routing(struct drm_atomic_state *state, return -EINVAL; }
- if (!drm_encoder_crtc_ok(new_encoder, connector_state->crtc)) { + if (!drm_encoder_crtc_ok(new_encoder, new_connector_state->crtc)) { DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d]\n", new_encoder->base.id, new_encoder->name, - connector_state->crtc->base.id); + new_connector_state->crtc->base.id); return -EINVAL; }
- if (new_encoder == connector_state->best_encoder) { - set_best_encoder(state, connector_state, new_encoder); + if (new_encoder == new_connector_state->best_encoder) { + set_best_encoder(state, new_connector_state, new_encoder);
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] keeps [ENCODER:%d:%s], now on [CRTC:%d:%s]\n", connector->base.id, connector->name, new_encoder->base.id, new_encoder->name, - connector_state->crtc->base.id, - connector_state->crtc->name); + new_connector_state->crtc->base.id, + new_connector_state->crtc->name);
return 0; }
steal_encoder(state, new_encoder);
- set_best_encoder(state, connector_state, new_encoder); + set_best_encoder(state, new_connector_state, new_encoder);
- crtc_state = drm_atomic_get_existing_crtc_state(state, connector_state->crtc); + crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc); crtc_state->connectors_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n", @@ -355,8 +357,8 @@ update_connector_routing(struct drm_atomic_state *state, connector->name, new_encoder->base.id, new_encoder->name, - connector_state->crtc->base.id, - connector_state->crtc->name); + new_connector_state->crtc->base.id, + new_connector_state->crtc->name);
return 0; } @@ -371,7 +373,7 @@ mode_fixup(struct drm_atomic_state *state) int i; bool ret;
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { if (!crtc_state->mode_changed && !crtc_state->connectors_changed) continue; @@ -379,7 +381,7 @@ mode_fixup(struct drm_atomic_state *state) drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode); }
- for_each_connector_in_state(state, connector, conn_state, i) { + for_each_new_connector_in_state(state, connector, conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;
@@ -424,7 +426,7 @@ mode_fixup(struct drm_atomic_state *state) } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
if (!crtc_state->enable) @@ -483,19 +485,19 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_connector *connector; - struct drm_connector_state *connector_state; + struct drm_connector_state *old_connector_state, *new_connector_state; int i, ret;
- for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + if (!drm_mode_equal(&old_crtc_state->mode, &new_crtc_state->mode)) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode changed\n", crtc->base.id, crtc->name); - crtc_state->mode_changed = true; + new_crtc_state->mode_changed = true; }
- if (crtc->state->enable != crtc_state->enable) { + if (old_crtc_state->enable != new_crtc_state->enable) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enable changed\n", crtc->base.id, crtc->name);
@@ -507,8 +509,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, * The other way around is true as well. enable != 0 * iff connectors are attached and a mode is set. */ - crtc_state->mode_changed = true; - crtc_state->connectors_changed = true; + new_crtc_state->mode_changed = true; + new_crtc_state->connectors_changed = true; } }
@@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret) return ret;
- for_each_connector_in_state(state, connector, connector_state, i) { + for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { /* * This only sets crtc->connectors_changed for routing changes, * drivers must set crtc->connectors_changed themselves when * connector properties need to be updated. */ ret = update_connector_routing(state, connector, - connector_state); + old_connector_state, + new_connector_state); if (ret) return ret; } @@ -534,28 +537,28 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, * 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_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { bool has_connectors = - !!crtc_state->connector_mask; + !!new_crtc_state->connector_mask;
/* * We must set ->active_changed after walking connectors for * otherwise an update that only changes active would result in * a full modeset because update_connector_routing force that. */ - if (crtc->state->active != crtc_state->active) { + if (old_crtc_state->active != new_crtc_state->active) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active changed\n", crtc->base.id, crtc->name); - crtc_state->active_changed = true; + new_crtc_state->active_changed = true; }
- if (!drm_atomic_crtc_needs_modeset(crtc_state)) + if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) continue;
DRM_DEBUG_ATOMIC("[CRTC:%d:%s] needs all connectors, enable: %c, active: %c\n", crtc->base.id, crtc->name, - crtc_state->enable ? 'y' : 'n', - crtc_state->active ? 'y' : 'n'); + new_crtc_state->enable ? 'y' : 'n', + new_crtc_state->active ? 'y' : 'n');
ret = drm_atomic_add_affected_connectors(state, crtc); if (ret != 0) @@ -565,7 +568,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret != 0) return ret;
- if (crtc_state->enable != has_connectors) { + if (new_crtc_state->enable != has_connectors) { DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n", crtc->base.id, crtc->name);
@@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *plane_state, *old_plane_state; int i, ret = 0;
- for_each_plane_in_state(state, plane, plane_state, i) { + for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) { const struct drm_plane_helper_funcs *funcs;
funcs = plane->helper_private;
- drm_atomic_helper_plane_changed(state, plane_state, plane); + drm_atomic_helper_plane_changed(state, old_plane_state, plane_state, plane);
if (!funcs || !funcs->atomic_check) continue; @@ -620,7 +623,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev, } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
funcs = crtc->helper_private; @@ -681,12 +684,12 @@ static void disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_connector *connector; - struct drm_connector_state *old_conn_state; + struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; int i;
- for_each_connector_in_state(old_state, connector, old_conn_state, i) { + for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;
@@ -723,7 +726,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
/* Right function depends upon target state. */ if (funcs) { - if (connector->state->crtc && funcs->prepare) + if (new_conn_state->crtc && funcs->prepare) funcs->prepare(encoder); else if (funcs->disable) funcs->disable(encoder); @@ -734,11 +737,11 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) drm_bridge_post_disable(encoder->bridge); }
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
/* Shut down everything that needs a full modeset. */ - if (!drm_atomic_crtc_needs_modeset(crtc->state)) + if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) continue;
if (!old_crtc_state->active) @@ -751,7 +754,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
/* Right function depends upon target state. */ - if (crtc->state->enable && funcs->prepare) + if (new_crtc_state->enable && funcs->prepare) funcs->prepare(crtc); else if (funcs->atomic_disable) funcs->atomic_disable(crtc, old_crtc_state); @@ -780,13 +783,13 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_connector *connector; - struct drm_connector_state *old_conn_state; + struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *crtc_state; int i;
/* clear out existing links and update dpms */ - for_each_connector_in_state(old_state, connector, old_conn_state, i) { + for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) { if (connector->encoder) { WARN_ON(!connector->encoder->crtc);
@@ -794,7 +797,7 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, connector->encoder = NULL; }
- crtc = connector->state->crtc; + crtc = new_conn_state->crtc; if ((!crtc && old_conn_state->crtc) || (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) { struct drm_property *dpms_prop = @@ -811,23 +814,23 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, }
/* set new links */ - for_each_connector_in_state(old_state, connector, old_conn_state, i) { - if (!connector->state->crtc) + for_each_new_connector_in_state(old_state, connector, new_conn_state, i) { + if (!new_conn_state->crtc) continue;
- if (WARN_ON(!connector->state->best_encoder)) + if (WARN_ON(!new_conn_state->best_encoder)) continue;
- connector->encoder = connector->state->best_encoder; - connector->encoder->crtc = connector->state->crtc; + connector->encoder = new_conn_state->best_encoder; + connector->encoder->crtc = new_conn_state->crtc; }
/* set legacy state in the crtc structure */ - for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { struct drm_plane *primary = crtc->primary;
- crtc->mode = crtc->state->mode; - crtc->enabled = crtc->state->enable; + crtc->mode = crtc_state->mode; + crtc->enabled = crtc_state->enable;
if (drm_atomic_get_existing_plane_state(old_state, primary) && primary->state->crtc == crtc) { @@ -835,9 +838,9 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, crtc->y = primary->state->src_y >> 16; }
- if (crtc->state->enable) + if (crtc_state->enable) drm_calc_timestamping_constants(crtc, - &crtc->state->adjusted_mode); + &crtc_state->adjusted_mode); } } EXPORT_SYMBOL(drm_atomic_helper_update_legacy_modeset_state); @@ -846,20 +849,20 @@ static void crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *crtc_state; struct drm_connector *connector; - struct drm_connector_state *old_conn_state; + struct drm_connector_state *conn_state; int i;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
- if (!crtc->state->mode_changed) + if (!crtc_state->mode_changed) continue;
funcs = crtc->helper_private;
- if (crtc->state->enable && funcs->mode_set_nofb) { + if (crtc_state->enable && funcs->mode_set_nofb) { DRM_DEBUG_ATOMIC("modeset on [CRTC:%d:%s]\n", crtc->base.id, crtc->name);
@@ -867,18 +870,18 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) } }
- for_each_connector_in_state(old_state, connector, old_conn_state, i) { + for_each_new_connector_in_state(old_state, connector, conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_crtc_state *new_crtc_state; struct drm_encoder *encoder; struct drm_display_mode *mode, *adjusted_mode;
- if (!connector->state->best_encoder) + if (!conn_state->best_encoder) continue;
- encoder = connector->state->best_encoder; + encoder = conn_state->best_encoder; funcs = encoder->helper_private; - new_crtc_state = connector->state->crtc->state; + new_crtc_state = conn_state->crtc->state; mode = &new_crtc_state->mode; adjusted_mode = &new_crtc_state->adjusted_mode;
@@ -894,7 +897,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state) */ if (funcs && funcs->atomic_mode_set) { funcs->atomic_mode_set(encoder, new_crtc_state, - connector->state); + conn_state); } else if (funcs && funcs->mode_set) { funcs->mode_set(encoder, mode, adjusted_mode); } @@ -946,24 +949,24 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *crtc_state; struct drm_connector *connector; - struct drm_connector_state *old_conn_state; + struct drm_connector_state *conn_state; int i;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
/* Need to filter out CRTCs where only planes change. */ - if (!drm_atomic_crtc_needs_modeset(crtc->state)) + if (!drm_atomic_crtc_needs_modeset(crtc_state)) continue;
- if (!crtc->state->active) + if (!crtc_state->active) continue;
funcs = crtc->helper_private;
- if (crtc->state->enable) { + if (crtc_state->enable) { DRM_DEBUG_ATOMIC("enabling [CRTC:%d:%s]\n", crtc->base.id, crtc->name);
@@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, } }
- for_each_connector_in_state(old_state, connector, old_conn_state, i) { + for_each_new_connector_in_state(old_state, connector, conn_state, i) { const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;
- if (!connector->state->best_encoder) + if (!conn_state->best_encoder) continue;
- if (!connector->state->crtc->state->active || - !drm_atomic_crtc_needs_modeset(connector->state->crtc->state)) + if (!conn_state->crtc->state->active || + !drm_atomic_crtc_needs_modeset(conn_state->crtc->state)) continue;
- encoder = connector->state->best_encoder; + encoder = conn_state->best_encoder; funcs = encoder->helper_private;
DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n", @@ -1038,10 +1041,7 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev, struct drm_plane_state *plane_state; int i, ret;
- for_each_plane_in_state(state, plane, plane_state, i) { - if (!pre_swap) - plane_state = plane->state; - + for_each_new_plane_in_state(state, plane, plane_state, i) { if (!plane_state->fence) continue;
@@ -1080,7 +1080,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; int i, ret; unsigned crtc_mask = 0;
@@ -1091,9 +1091,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, if (old_state->legacy_cursor_update) return;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { - struct drm_crtc_state *new_crtc_state = crtc->state; - + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { if (!new_crtc_state->active || !new_crtc_state->planes_changed) continue;
@@ -1105,7 +1103,7 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev, old_state->crtcs[i].last_vblank_count = drm_crtc_vblank_count(crtc); }
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i) { if (!(crtc_mask & drm_crtc_mask(crtc))) continue;
@@ -1412,11 +1410,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, bool nonblock) { struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state, *crtc_state; struct drm_crtc_commit *commit; int i, ret;
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { commit = kzalloc(sizeof(*commit), GFP_KERNEL); if (!commit) return -ENOMEM; @@ -1437,7 +1435,7 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, /* Drivers only send out events when at least either current or * new CRTC state is active. Complete right away if everything * stays off. */ - if (!crtc->state->active && !crtc_state->active) { + if (!old_crtc_state->active && !crtc_state->active) { complete_all(&commit->flip_done); continue; } @@ -1503,7 +1501,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state) int i; long ret;
- for_each_crtc_in_state(old_state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { spin_lock(&crtc->commit_lock); commit = preceeding_commit(crtc); if (commit) @@ -1554,13 +1552,13 @@ void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state) struct drm_crtc_commit *commit; int i;
- for_each_crtc_in_state(old_state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { commit = old_state->crtcs[i].commit; if (!commit) continue;
/* backend must have consumed any event by now */ - WARN_ON(crtc->state->event); + WARN_ON(crtc_state->event); spin_lock(&crtc->commit_lock); complete_all(&commit->hw_done); spin_unlock(&crtc->commit_lock); @@ -1587,7 +1585,7 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state) int i; long ret;
- for_each_crtc_in_state(old_state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { commit = old_state->crtcs[i].commit; if (WARN_ON(!commit)) continue; @@ -1640,7 +1638,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, struct drm_plane_state *plane_state; int ret, i, j;
- for_each_plane_in_state(state, plane, plane_state, i) { + for_each_new_plane_in_state(state, plane, plane_state, i) { const struct drm_plane_helper_funcs *funcs;
funcs = plane->helper_private; @@ -1655,7 +1653,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev, return 0;
fail: - for_each_plane_in_state(state, plane, plane_state, j) { + for_each_new_plane_in_state(state, plane, plane_state, j) { const struct drm_plane_helper_funcs *funcs;
if (j >= i) @@ -1722,14 +1720,14 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, uint32_t flags) { struct drm_crtc *crtc; - struct drm_crtc_state *old_crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_plane *plane; - struct drm_plane_state *old_plane_state; + struct drm_plane_state *old_plane_state, *new_plane_state; int i; bool active_only = flags & DRM_PLANE_COMMIT_ACTIVE_ONLY; bool no_disable = flags & DRM_PLANE_COMMIT_NO_DISABLE_AFTER_MODESET;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
funcs = crtc->helper_private; @@ -1737,13 +1735,13 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_begin) continue;
- if (active_only && !crtc->state->active) + if (active_only && !new_crtc_state->active) continue;
funcs->atomic_begin(crtc, old_crtc_state); }
- for_each_plane_in_state(old_state, plane, old_plane_state, i) { + for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) { const struct drm_plane_helper_funcs *funcs; bool disabling;
@@ -1762,7 +1760,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, * CRTC to avoid skipping planes being disabled on an * active CRTC. */ - if (!disabling && !plane_crtc_active(plane->state)) + if (!disabling && !plane_crtc_active(new_plane_state)) continue; if (disabling && !plane_crtc_active(old_plane_state)) continue; @@ -1781,12 +1779,12 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, continue;
funcs->atomic_disable(plane, old_plane_state); - } else if (plane->state->crtc || disabling) { + } else if (new_plane_state->crtc || disabling) { funcs->atomic_update(plane, old_plane_state); } }
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs;
funcs = crtc->helper_private; @@ -1794,7 +1792,7 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev, if (!funcs || !funcs->atomic_flush) continue;
- if (active_only && !crtc->state->active) + if (active_only && !new_crtc_state->active) continue;
funcs->atomic_flush(crtc, old_crtc_state); @@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *plane_state, *new_plane_state; int i;
- for_each_plane_in_state(old_state, plane, plane_state, i) { + for_each_oldnew_plane_in_state(old_state, plane, plane_state, new_plane_state, i) { const struct drm_plane_helper_funcs *funcs;
+ /* + * This might be called before swapping when commit is aborted, + * in which case we have to free the new state. + */ + if (plane_state == plane->state) + plane_state = new_plane_state; + funcs = plane->helper_private;
if (funcs->cleanup_fb) @@ -1971,15 +1976,15 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, int i; long ret; struct drm_connector *connector; - struct drm_connector_state *conn_state, *old_conn_state; + struct drm_connector_state *old_conn_state, *new_conn_state; struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state, *old_crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state, *old_plane_state; + struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit;
if (stall) { - for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { spin_lock(&crtc->commit_lock); commit = list_first_entry_or_null(&crtc->commit_list, struct drm_crtc_commit, commit_entry); @@ -1999,20 +2004,24 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_oldnew_connector_in_state(state, connector, old_conn_state, conn_state, i) { + for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) { WARN_ON(connector->state != old_conn_state);
- connector->state->state = state; - swap(state->connectors[i].state, connector->state); - connector->state->state = NULL; + old_conn_state->state = state; + new_conn_state->state = NULL; + + state->connectors[i].state = old_conn_state; + connector->state = new_conn_state; }
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, crtc_state, i) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { WARN_ON(crtc->state != old_crtc_state);
- crtc->state->state = state; - swap(state->crtcs[i].state, crtc->state); - crtc->state->state = NULL; + old_crtc_state->state = state; + new_crtc_state->state = NULL; + + state->crtcs[i].state = old_crtc_state; + crtc->state = new_crtc_state;
if (state->crtcs[i].commit) { spin_lock(&crtc->commit_lock); @@ -2024,12 +2033,14 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, } }
- for_each_oldnew_plane_in_state(state, plane, old_plane_state, plane_state, i) { + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { WARN_ON(plane->state != old_plane_state);
- plane->state->state = state; - swap(state->planes[i].state, plane->state); - plane->state->state = NULL; + old_plane_state->state = state; + new_plane_state->state = NULL; + + state->planes[i].state = old_plane_state; + plane->state = new_plane_state; } } EXPORT_SYMBOL(drm_atomic_helper_swap_state); @@ -2227,7 +2238,7 @@ static int update_output_state(struct drm_atomic_state *state, if (ret) return ret;
- for_each_connector_in_state(state, connector, conn_state, i) { + for_each_new_connector_in_state(state, connector, conn_state, i) { if (conn_state->crtc == set->crtc) { ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); @@ -2249,7 +2260,7 @@ static int update_output_state(struct drm_atomic_state *state, return ret; }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { + for_each_new_crtc_in_state(state, crtc, crtc_state, i) { /* Don't update ->enable for the CRTC in the set_config request, * since a mismatch would indicate a bug in the upper layers. * The actual modeset code later on will catch any
Hi Maarten,
(CC'ing Daniel)
Thank you for the patch.
On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++--------------- 1 file changed, 152 insertions(+), 141 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
@@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state, { struct drm_crtc_state *crtc_state; struct drm_connector *connector;
- struct drm_connector_state *connector_state;
- struct drm_connector_state *old_connector_state, *new_connector_state;
The kernel favours one variable declaration per line, and I think this file does as well, at least mostly (this comment holds for the rest of this patch and the other patches in the series).
int i;
- for_each_connector_in_state(state, connector, connector_state, i) {
- for_each_oldnew_connector_in_state(state, connector,
old_connector_state, new_connector_state, i) {
This is getting quite long, you could wrap the line.
struct drm_crtc *encoder_crtc;
if (connector_state->best_encoder != encoder)
if (new_connector_state->best_encoder != encoder) continue;
encoder_crtc = connector->state->crtc;
encoder_crtc = old_connector_state->crtc;DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s],
stealing it\n",
encoder->base.id, encoder->name, encoder_crtc->base.id, encoder_crtc->name);
set_best_encoder(state, connector_state, NULL);
set_best_encoder(state, new_connector_state, NULL);crtc_state = drm_atomic_get_existing_crtc_state(state,
encoder_crtc);
crtc_state->connectors_changed = true;
[snip]
@@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret) return ret;
- for_each_connector_in_state(state, connector, connector_state, i) {
- for_each_oldnew_connector_in_state(state, connector,
old_connector_state, new_connector_state, i) {
Line wrap here too ?
/* * This only sets crtc->connectors_changed for routing
changes,
* drivers must set crtc->connectors_changed themselves when * connector properties need to be updated. */ ret = update_connector_routing(state, connector,
connector_state);
old_connector_state,new_connector_state);
[snip]
@@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_plane_state *plane_state, *old_plane_state;
In some location you use new_*_state and in others *_state. I believe this should this be standardized to avoid confusion (the code is hard enough to read as it is :-)).
Similarly, you sometimes use *_conn_state and sometimes *_connector_state. That should be standardized as well.
int i, ret = 0;
- for_each_plane_in_state(state, plane, plane_state, i) {
- for_each_oldnew_plane_in_state(state, plane, old_plane_state,
plane_state, i) { const struct drm_plane_helper_funcs *funcs;
funcs = plane->helper_private;
drm_atomic_helper_plane_changed(state, plane_state, plane);
drm_atomic_helper_plane_changed(state, old_plane_state,plane_state, plane);
if (!funcs || !funcs->atomic_check) continue;
[snip]
@@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, } }
- for_each_connector_in_state(old_state, connector, old_conn_state, i) {
- for_each_new_connector_in_state(old_state, connector, conn_state, i) {
Not strictly related to this patch, but I've been wondering how this works, given that we need to loop over connectors in the new state, not the old state. After some investigation I've realized that both the old and new states contain the same list of objects, as we don't keep a copy of the old global atomic state. old_state was the new state before the state swap, and the swap operation sets state->/objects/[*].state to the old state for all objects, but doesn't modify the object pointers. This is possibly more of a question for Daniel, should this be captured in the documentation somewhere (and is my understanding correct) ?
const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;
if (!connector->state->best_encoder)
if (!conn_state->best_encoder) continue;
if (!connector->state->crtc->state->active ||!drm_atomic_crtc_needs_modeset(connector->state->crtc-state))
if (!conn_state->crtc->state->active ||!drm_atomic_crtc_needs_modeset(conn_state->crtc->state)) continue;
encoder = connector->state->best_encoder;
encoder = conn_state->best_encoder;funcs = encoder->helper_private;
DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
[snip]
@@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_plane_state *plane_state, *new_plane_state; int i;
- for_each_plane_in_state(old_state, plane, plane_state, i) {
- for_each_oldnew_plane_in_state(old_state, plane, plane_state,
new_plane_state, i) { const struct drm_plane_helper_funcs *funcs;
/** This might be called before swapping when commit is
aborted,
* in which case we have to free the new state.
Should this be "cleanup the new state" instead of "free the new state" ?
*/if (plane_state == plane->state)plane_state = new_plane_state;funcs = plane->helper_private;
if (funcs->cleanup_fb)
[snip]
Op 17-01-17 om 02:01 schreef Laurent Pinchart:
Hi Maarten,
(CC'ing Daniel)
Thank you for the patch.
On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++++--------------- 1 file changed, 152 insertions(+), 141 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
@@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state, { struct drm_crtc_state *crtc_state; struct drm_connector *connector;
- struct drm_connector_state *connector_state;
- struct drm_connector_state *old_connector_state, *new_connector_state;
The kernel favours one variable declaration per line, and I think this file does as well, at least mostly (this comment holds for the rest of this patch and the other patches in the series).
This is the first I've heard of it..
~/nfs/linux$ git grep 'int i,' | wc -l 8490 ~/nfs/linux$ git grep 'int i;' | wc -l 23636
Based on this, I don't think it's uncommon to have multiple declarations per line.
int i;
- for_each_connector_in_state(state, connector, connector_state, i) {
- for_each_oldnew_connector_in_state(state, connector,
old_connector_state, new_connector_state, i) {
This is getting quite long, you could wrap the line.
Sounds good.
struct drm_crtc *encoder_crtc;
if (connector_state->best_encoder != encoder)
if (new_connector_state->best_encoder != encoder) continue;
encoder_crtc = connector->state->crtc;
encoder_crtc = old_connector_state->crtc;DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s],
stealing it\n",
encoder->base.id, encoder->name, encoder_crtc->base.id, encoder_crtc->name);
set_best_encoder(state, connector_state, NULL);
set_best_encoder(state, new_connector_state, NULL);crtc_state = drm_atomic_get_existing_crtc_state(state,
encoder_crtc);
crtc_state->connectors_changed = true;[snip]
@@ -516,14 +518,15 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, if (ret) return ret;
- for_each_connector_in_state(state, connector, connector_state, i) {
- for_each_oldnew_connector_in_state(state, connector,
old_connector_state, new_connector_state, i) {
Line wrap here too ?
/* * This only sets crtc->connectors_changed for routingchanges,
* drivers must set crtc->connectors_changed themselves when * connector properties need to be updated. */ ret = update_connector_routing(state, connector,
connector_state);
old_connector_state,new_connector_state);[snip]
@@ -599,15 +602,15 @@ drm_atomic_helper_check_planes(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_plane_state *plane_state, *old_plane_state;
In some location you use new_*_state and in others *_state. I believe this should this be standardized to avoid confusion (the code is hard enough to read as it is :-)).
Similarly, you sometimes use *_conn_state and sometimes *_connector_state. That should be standardized as well.
Indeed, at least for those that use both.
int i, ret = 0;
- for_each_plane_in_state(state, plane, plane_state, i) {
- for_each_oldnew_plane_in_state(state, plane, old_plane_state,
plane_state, i) { const struct drm_plane_helper_funcs *funcs;
funcs = plane->helper_private;
drm_atomic_helper_plane_changed(state, plane_state, plane);
drm_atomic_helper_plane_changed(state, old_plane_state,plane_state, plane);
if (!funcs || !funcs->atomic_check) continue;[snip]
@@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, } }
- for_each_connector_in_state(old_state, connector, old_conn_state, i) {
- for_each_new_connector_in_state(old_state, connector, conn_state, i) {
Not strictly related to this patch, but I've been wondering how this works, given that we need to loop over connectors in the new state, not the old state. After some investigation I've realized that both the old and new states contain the same list of objects, as we don't keep a copy of the old global atomic state. old_state was the new state before the state swap, and the swap operation sets state->/objects/[*].state to the old state for all objects, but doesn't modify the object pointers. This is possibly more of a question for Daniel, should this be captured in the documentation somewhere (and is my understanding correct) ?
Yes. But that will go away soon after this patch + all drivers converted.
This is why get_state should not be called during atomic commit, and get_existing_state will be removed.
const struct drm_encoder_helper_funcs *funcs; struct drm_encoder *encoder;
if (!connector->state->best_encoder)
if (!conn_state->best_encoder) continue;
if (!connector->state->crtc->state->active ||!drm_atomic_crtc_needs_modeset(connector->state->crtc-state))
if (!conn_state->crtc->state->active ||!drm_atomic_crtc_needs_modeset(conn_state->crtc->state)) continue;
encoder = connector->state->best_encoder;
encoder = conn_state->best_encoder;funcs = encoder->helper_private;
DRM_DEBUG_ATOMIC("enabling [ENCODER:%d:%s]\n",
[snip]
@@ -1921,12 +1919,19 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_plane_state *plane_state, *new_plane_state; int i;
- for_each_plane_in_state(old_state, plane, plane_state, i) {
- for_each_oldnew_plane_in_state(old_state, plane, plane_state,
new_plane_state, i) { const struct drm_plane_helper_funcs *funcs;
/** This might be called before swapping when commit isaborted,
* in which case we have to free the new state.Should this be "cleanup the new state" instead of "free the new state" ?
Indeed.
*/if (plane_state == plane->state)plane_state = new_plane_state;funcs = plane->helper_private;
if (funcs->cleanup_fb)
[snip]
Hi Maarten,
On Wednesday 18 Jan 2017 15:49:56 Maarten Lankhorst wrote:
Op 17-01-17 om 02:01 schreef Laurent Pinchart:
On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++--------------- 1 file changed, 152 insertions(+), 141 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
@@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state, { struct drm_crtc_state *crtc_state; struct drm_connector *connector;
- struct drm_connector_state *connector_state;
- struct drm_connector_state *old_connector_state, *new_connector_state;
The kernel favours one variable declaration per line, and I think this file does as well, at least mostly (this comment holds for the rest of this patch and the other patches in the series).
This is the first I've heard of it..
~/nfs/linux$ git grep 'int i,' | wc -l 8490 ~/nfs/linux$ git grep 'int i;' | wc -l 23636
Based on this, I don't think it's uncommon to have multiple declarations per line.
It's not uncommon, no. I still think it hinders readability though, especially for long statements like here.
int i;
[snip]
@@ -974,18 +977,18 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, } }
- for_each_connector_in_state(old_state, connector, old_conn_state, i) {
- for_each_new_connector_in_state(old_state, connector, conn_state, i) {
Not strictly related to this patch, but I've been wondering how this works, given that we need to loop over connectors in the new state, not the old state. After some investigation I've realized that both the old and new states contain the same list of objects, as we don't keep a copy of the old global atomic state. old_state was the new state before the state swap, and the swap operation sets state->/objects/[*].state to the old state for all objects, but doesn't modify the object pointers. This is possibly more of a question for Daniel, should this be captured in the documentation somewhere (and is my understanding correct) ?
Yes. But that will go away soon after this patch + all drivers converted.
Will it ? Even with the new helpers we will have code looping over objects from the old state, like here. As the code intends on looping over objects in the new state this is confusing until you realize that the old state always contains the same objects as the new state. As Ville mentioned, drm_atomic_state would be better named drm_atomic_transaction, this would remove the ambiguity.
This is why get_state should not be called during atomic commit, and get_existing_state will be removed.
[snip]
After atomic commit, these macros should be used in place of get_existing_state. Also after commit get_xx_state should no longer be used because it may not have the required locks.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- include/drm/drm_atomic.h | 99 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6062e7f27325..2e6bb7acc837 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -285,6 +285,35 @@ drm_atomic_get_existing_crtc_state(struct drm_atomic_state *state, }
/** + * drm_atomic_get_old_crtc_state - get old crtc state, if it exists + * @state: global atomic state object + * @crtc: crtc to grab + * + * This function returns the old crtc state for the given crtc, or + * NULL if the crtc is not part of the global atomic state. + */ +static inline struct drm_crtc_state * +drm_atomic_get_old_crtc_state(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + return state->crtcs[drm_crtc_index(crtc)].old_state; +} +/** + * drm_atomic_get_new_crtc_state - get new crtc state, if it exists + * @state: global atomic state object + * @crtc: crtc to grab + * + * This function returns the new crtc state for the given crtc, or + * NULL if the crtc is not part of the global atomic state. + */ +static inline struct drm_crtc_state * +drm_atomic_get_new_crtc_state(struct drm_atomic_state *state, + struct drm_crtc *crtc) +{ + return state->crtcs[drm_crtc_index(crtc)].new_state; +} + +/** * drm_atomic_get_existing_plane_state - get plane state, if it exists * @state: global atomic state object * @plane: plane to grab @@ -300,6 +329,36 @@ drm_atomic_get_existing_plane_state(struct drm_atomic_state *state, }
/** + * drm_atomic_get_old_plane_state - get plane state, if it exists + * @state: global atomic state object + * @plane: plane to grab + * + * This function returns the old plane state for the given plane, or + * NULL if the plane is not part of the global atomic state. + */ +static inline struct drm_plane_state * +drm_atomic_get_old_plane_state(struct drm_atomic_state *state, + struct drm_plane *plane) +{ + return state->planes[drm_plane_index(plane)].old_state; +} + +/** + * drm_atomic_get_new_plane_state - get plane state, if it exists + * @state: global atomic state object + * @plane: plane to grab + * + * This function returns the new plane state for the given plane, or + * NULL if the plane is not part of the global atomic state. + */ +static inline struct drm_plane_state * +drm_atomic_get_new_plane_state(struct drm_atomic_state *state, + struct drm_plane *plane) +{ + return state->planes[drm_plane_index(plane)].new_state; +} + +/** * drm_atomic_get_existing_connector_state - get connector state, if it exists * @state: global atomic state object * @connector: connector to grab @@ -320,6 +379,46 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, }
/** + * drm_atomic_get_old_connector_state - get connector state, if it exists + * @state: global atomic state object + * @connector: connector to grab + * + * This function returns the old connector state for the given connector, + * or NULL if the connector is not part of the global atomic state. + */ +static inline struct drm_connector_state * +drm_atomic_get_old_connector_state(struct drm_atomic_state *state, + struct drm_connector *connector) +{ + int index = drm_connector_index(connector); + + if (index >= state->num_connector) + return NULL; + + return state->connectors[index].old_state; +} + +/** + * drm_atomic_get_new_connector_state - get connector state, if it exists + * @state: global atomic state object + * @connector: connector to grab + * + * This function returns the new connector state for the given connector, + * or NULL if the connector is not part of the global atomic state. + */ +static inline struct drm_connector_state * +drm_atomic_get_new_connector_state(struct drm_atomic_state *state, + struct drm_connector *connector) +{ + int index = drm_connector_index(connector); + + if (index >= state->num_connector) + return NULL; + + return state->connectors[index].new_state; +} + +/** * __drm_atomic_get_current_plane_state - get current plane state * @state: global atomic state object * @plane: plane to grab
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:42 Maarten Lankhorst wrote:
After atomic commit, these macros should be used in place of get_existing_state. Also after commit get_xx_state should no longer be used because it may not have the required locks.
Should this be captured in the functions' documentation ? In particular, should the drm_atomic_get_existing_*_state() functions be marked as deprecated ?
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
include/drm/drm_atomic.h | 99 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 99 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 6062e7f27325..2e6bb7acc837 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -285,6 +285,35 @@ drm_atomic_get_existing_crtc_state(struct drm_atomic_state *state, }
/**
- drm_atomic_get_old_crtc_state - get old crtc state, if it exists
- @state: global atomic state object
- @crtc: crtc to grab
- This function returns the old crtc state for the given crtc, or
- NULL if the crtc is not part of the global atomic state.
- */
+static inline struct drm_crtc_state * +drm_atomic_get_old_crtc_state(struct drm_atomic_state *state,
struct drm_crtc *crtc)+{
- return state->crtcs[drm_crtc_index(crtc)].old_state;
+} +/**
- drm_atomic_get_new_crtc_state - get new crtc state, if it exists
- @state: global atomic state object
- @crtc: crtc to grab
- This function returns the new crtc state for the given crtc, or
- NULL if the crtc is not part of the global atomic state.
- */
+static inline struct drm_crtc_state * +drm_atomic_get_new_crtc_state(struct drm_atomic_state *state,
struct drm_crtc *crtc)+{
- return state->crtcs[drm_crtc_index(crtc)].new_state;
+}
+/**
- drm_atomic_get_existing_plane_state - get plane state, if it exists
- @state: global atomic state object
- @plane: plane to grab
@@ -300,6 +329,36 @@ drm_atomic_get_existing_plane_state(struct drm_atomic_state *state, }
/**
- drm_atomic_get_old_plane_state - get plane state, if it exists
- @state: global atomic state object
- @plane: plane to grab
- This function returns the old plane state for the given plane, or
- NULL if the plane is not part of the global atomic state.
- */
+static inline struct drm_plane_state * +drm_atomic_get_old_plane_state(struct drm_atomic_state *state,
struct drm_plane *plane)+{
- return state->planes[drm_plane_index(plane)].old_state;
+}
+/**
- drm_atomic_get_new_plane_state - get plane state, if it exists
- @state: global atomic state object
- @plane: plane to grab
- This function returns the new plane state for the given plane, or
- NULL if the plane is not part of the global atomic state.
- */
+static inline struct drm_plane_state * +drm_atomic_get_new_plane_state(struct drm_atomic_state *state,
struct drm_plane *plane)+{
- return state->planes[drm_plane_index(plane)].new_state;
+}
+/**
- drm_atomic_get_existing_connector_state - get connector state, if it
exists * @state: global atomic state object
- @connector: connector to grab
@@ -320,6 +379,46 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, }
/**
- drm_atomic_get_old_connector_state - get connector state, if it exists
- @state: global atomic state object
- @connector: connector to grab
- This function returns the old connector state for the given connector,
- or NULL if the connector is not part of the global atomic state.
- */
+static inline struct drm_connector_state * +drm_atomic_get_old_connector_state(struct drm_atomic_state *state,
struct drm_connector *connector)+{
- int index = drm_connector_index(connector);
- if (index >= state->num_connector)
return NULL;- return state->connectors[index].old_state;
+}
+/**
- drm_atomic_get_new_connector_state - get connector state, if it exists
- @state: global atomic state object
- @connector: connector to grab
- This function returns the new connector state for the given connector,
- or NULL if the connector is not part of the global atomic state.
- */
+static inline struct drm_connector_state * +drm_atomic_get_new_connector_state(struct drm_atomic_state *state,
struct drm_connector *connector)+{
- int index = drm_connector_index(connector);
- if (index >= state->num_connector)
return NULL;- return state->connectors[index].new_state;
+}
+/**
- __drm_atomic_get_current_plane_state - get current plane state
- @state: global atomic state object
- @plane: plane to grab
This is a straightforward conversion that converts all the users of get_existing_state in atomic core to use get_old_state or get_new_state
Changes since v1: - Fix using the wrong state in drm_atomic_helper_update_legacy_modeset_state.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_atomic.c | 6 +++--- drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++++-------------------- drivers/gpu/drm/drm_blend.c | 3 +-- 3 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7b61e0da9ace..6428e9469607 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1362,8 +1362,8 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state, return 0;
if (conn_state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(conn_state->state, - conn_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(conn_state->state, + conn_state->crtc);
crtc_state->connector_mask &= ~(1 << drm_connector_index(conn_state->connector)); @@ -1480,7 +1480,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state, { struct drm_plane *plane;
- WARN_ON(!drm_atomic_get_existing_crtc_state(state, crtc)); + WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
drm_for_each_plane_mask(plane, state->dev, crtc->state->plane_mask) { struct drm_plane_state *plane_state = diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -70,8 +70,7 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, struct drm_crtc_state *crtc_state;
if (old_plane_state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, - old_plane_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, old_plane_state->crtc);
if (WARN_ON(!crtc_state)) return; @@ -80,8 +79,7 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state, }
if (plane_state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, - plane_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
if (WARN_ON(!crtc_state)) return; @@ -150,7 +148,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, drm_for_each_connector_iter(connector, &conn_iter) { struct drm_crtc_state *crtc_state;
- if (drm_atomic_get_existing_connector_state(state, connector)) + if (drm_atomic_get_new_connector_state(state, connector)) continue;
encoder = connector->state->best_encoder; @@ -178,7 +176,7 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state, conn_state->crtc->base.id, conn_state->crtc->name, connector->base.id, connector->name);
- crtc_state = drm_atomic_get_existing_crtc_state(state, conn_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
ret = drm_atomic_set_crtc_for_connector(conn_state, NULL); if (ret) @@ -219,7 +217,7 @@ set_best_encoder(struct drm_atomic_state *state, */ WARN_ON(!crtc && encoder != conn_state->best_encoder); if (crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
crtc_state->encoder_mask &= ~(1 << drm_encoder_index(conn_state->best_encoder)); @@ -230,7 +228,7 @@ set_best_encoder(struct drm_atomic_state *state, crtc = conn_state->crtc; WARN_ON(!crtc); if (crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
crtc_state->encoder_mask |= 1 << drm_encoder_index(encoder); @@ -263,7 +261,7 @@ steal_encoder(struct drm_atomic_state *state,
set_best_encoder(state, new_connector_state, NULL);
- crtc_state = drm_atomic_get_existing_crtc_state(state, encoder_crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, encoder_crtc); crtc_state->connectors_changed = true;
return; @@ -286,12 +284,12 @@ update_connector_routing(struct drm_atomic_state *state,
if (old_connector_state->crtc != new_connector_state->crtc) { if (old_connector_state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, old_connector_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, old_connector_state->crtc); crtc_state->connectors_changed = true; }
if (new_connector_state->crtc) { - crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc); crtc_state->connectors_changed = true; } } @@ -349,7 +347,7 @@ update_connector_routing(struct drm_atomic_state *state,
set_best_encoder(state, new_connector_state, new_encoder);
- crtc_state = drm_atomic_get_existing_crtc_state(state, new_connector_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc); crtc_state->connectors_changed = true;
DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n", @@ -390,8 +388,7 @@ mode_fixup(struct drm_atomic_state *state) if (!conn_state->crtc || !conn_state->best_encoder) continue;
- crtc_state = drm_atomic_get_existing_crtc_state(state, - conn_state->crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
/* * Each encoder has at most one connector (since we always steal @@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (!old_conn_state->crtc) continue;
- old_crtc_state = drm_atomic_get_existing_crtc_state(old_state, - old_conn_state->crtc); + old_crtc_state = drm_atomic_get_new_crtc_state(old_state, old_conn_state->crtc);
if (!old_crtc_state->active || !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state)) @@ -828,14 +824,15 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev, /* set legacy state in the crtc structure */ for_each_new_crtc_in_state(old_state, crtc, crtc_state, i) { struct drm_plane *primary = crtc->primary; + struct drm_plane_state *plane_state;
crtc->mode = crtc_state->mode; crtc->enabled = crtc_state->enable;
- if (drm_atomic_get_existing_plane_state(old_state, primary) && - primary->state->crtc == crtc) { - crtc->x = primary->state->src_x >> 16; - crtc->y = primary->state->src_y >> 16; + plane_state = drm_atomic_get_new_plane_state(old_state, primary); + if (plane_state && plane_state->crtc == crtc) { + crtc->x = plane_state->src_x >> 16; + crtc->y = plane_state->src_y >> 16; }
if (crtc_state->enable) @@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
drm_for_each_plane_mask(plane, crtc->dev, plane_mask) { struct drm_plane_state *old_plane_state = - drm_atomic_get_existing_plane_state(old_state, plane); + drm_atomic_get_old_plane_state(old_state, plane); const struct drm_plane_helper_funcs *plane_funcs;
plane_funcs = plane->helper_private; diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index 1f2412c7ccfd..5c45d0852608 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -388,8 +388,7 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, if (!crtc) continue; if (plane->state->zpos != plane_state->zpos) { - crtc_state = - drm_atomic_get_existing_crtc_state(state, crtc); + crtc_state = drm_atomic_get_new_crtc_state(state, crtc); crtc_state->zpos_changed = true; } }
Hi Maarten,
Thank you for the patch.
I don't think you need the "v2" at the end of the subject line.
On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
This is a straightforward conversion that converts all the users of get_existing_state in atomic core to use get_old_state or get_new_state
Changes since v1:
- Fix using the wrong state in
drm_atomic_helper_update_legacy_modeset_state.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++--- drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++------------------ drivers/gpu/drm/drm_blend.c | 3 +-- 3 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7b61e0da9ace..6428e9469607 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c
[snip]
@@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
drm_for_each_plane_mask(plane, crtc->dev, plane_mask) { struct drm_plane_state *old_plane_state =
drm_atomic_get_existing_plane_state(old_state, plane);
drm_atomic_get_old_plane_state(old_state, plane);
I believe this change to be correct, but given that the function is called after state swap, didn't it use the new state before this patch ?
const struct drm_plane_helper_funcs *plane_funcs; plane_funcs = plane->helper_private;
Op 17-01-17 om 02:12 schreef Laurent Pinchart:
Hi Maarten,
Thank you for the patch.
I don't think you need the "v2" at the end of the subject line.
On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
This is a straightforward conversion that converts all the users of get_existing_state in atomic core to use get_old_state or get_new_state
Changes since v1:
- Fix using the wrong state in
drm_atomic_helper_update_legacy_modeset_state.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++--- drivers/gpu/drm/drm_atomic_helper.c | 39 +++++++++++++++------------------ drivers/gpu/drm/drm_blend.c | 3 +-- 3 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7b61e0da9ace..6428e9469607 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c
[snip]
@@ -1835,7 +1832,7 @@ drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_state)
drm_for_each_plane_mask(plane, crtc->dev, plane_mask) { struct drm_plane_state *old_plane_state =
drm_atomic_get_existing_plane_state(old_state, plane);
drm_atomic_get_old_plane_state(old_state, plane);I believe this change to be correct, but given that the function is called after state swap, didn't it use the new state before this patch ?
get_existing_state returns the old state after swap.
But the call to drm_atomic_plane_disabling is confusing to me. I'm changing the function to pass old and new plane state, which should make it easier to understand.
Cheers, ~Maarten
Hi Maarten,
One more thing.
On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
This is a straightforward conversion that converts all the users of get_existing_state in atomic core to use get_old_state or get_new_state
Changes since v1:
- Fix using the wrong state in
drm_atomic_helper_update_legacy_modeset_state.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++--- drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++-------------------- drivers/gpu/drm/drm_blend.c | 3 +-- 3 files changed, 22 insertions(+), 26 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
@@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (!old_conn_state->crtc) continue;
old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
old_conn_state->crtc);
old_crtc_state = drm_atomic_get_new_crtc_state(old_state,old_conn_state->crtc);
This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right thing as old_state->crtcs[*].state was set to the old state by the state swap operation.
On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses drm_atomic_get_new_plane_state() the same way you do here, even though it operates after state swap, and your changelog specifically mentions that you've fixed that in v2. It's getting too late to properly wrap my head around this, I'll let you check which option is correct (but I reserve the right to challenge it if your explanation isn't convincing enough ;-)).
if (!old_crtc_state->active || !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-state))
Op 17-01-17 om 02:27 schreef Laurent Pinchart:
Hi Maarten,
One more thing.
On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
This is a straightforward conversion that converts all the users of get_existing_state in atomic core to use get_old_state or get_new_state
Changes since v1:
- Fix using the wrong state in
drm_atomic_helper_update_legacy_modeset_state.
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_atomic.c | 6 +++--- drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++-------------------- drivers/gpu/drm/drm_blend.c | 3 +-- 3 files changed, 22 insertions(+), 26 deletions(-)
[snip]
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c
[snip]
@@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (!old_conn_state->crtc) continue;
old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,old_conn_state->crtc);
old_crtc_state = drm_atomic_get_new_crtc_state(old_state,old_conn_state->crtc);
This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right thing as old_state->crtcs[*].state was set to the old state by the state swap operation.
This is wrong, even. Fixed in next version. At least looking at the code made it obvious. :)
On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses drm_atomic_get_new_plane_state() the same way you do here, even though it operates after state swap, and your changelog specifically mentions that you've fixed that in v2. It's getting too late to properly wrap my head around this, I'll let you check which option is correct (but I reserve the right to challenge it if your explanation isn't convincing enough ;-)).
update_legacy_modeset_state was looking at primary->state (new state) before, but was using get_existing_state to check whether it's part of the commit, so it's valid to look at plane->state.
This was fixed to get the new state, so it makes sense there.
if (!old_crtc_state->active || !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-state))
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com --- drivers/gpu/drm/drm_blend.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index 5c45d0852608..78cf9f6cae08 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -378,26 +378,26 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *old_plane_state, *new_plane_state; int i, ret = 0;
- for_each_plane_in_state(state, plane, plane_state, i) { - crtc = plane_state->crtc; + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { + crtc = new_plane_state->crtc; if (!crtc) continue; - if (plane->state->zpos != plane_state->zpos) { - crtc_state = drm_atomic_get_new_crtc_state(state, crtc); - crtc_state->zpos_changed = true; + if (old_plane_state->zpos != new_plane_state->zpos) { + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc); + new_crtc_state->zpos_changed = true; } }
- for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (crtc_state->plane_mask != crtc->state->plane_mask || - crtc_state->zpos_changed) { + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { + if (old_crtc_state->plane_mask != new_crtc_state->plane_mask || + new_crtc_state->zpos_changed) { ret = drm_atomic_helper_crtc_normalize_zpos(crtc, - crtc_state); + new_crtc_state); if (ret) return ret; }
Hi Maarten,
Thank you for the patch.
On Monday 16 Jan 2017 10:37:44 Maarten Lankhorst wrote:
Could we please get a commit message ?
Apart from that,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/drm_blend.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index 5c45d0852608..78cf9f6cae08 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -378,26 +378,26 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state) { struct drm_crtc *crtc;
- struct drm_crtc_state *crtc_state;
- struct drm_crtc_state *old_crtc_state, *new_crtc_state; struct drm_plane *plane;
- struct drm_plane_state *plane_state;
- struct drm_plane_state *old_plane_state, *new_plane_state; int i, ret = 0;
- for_each_plane_in_state(state, plane, plane_state, i) {
crtc = plane_state->crtc;
- for_each_oldnew_plane_in_state(state, plane, old_plane_state,
new_plane_state, i) {
crtc = new_plane_state->crtc;
if (plane->state->zpos != plane_state->zpos) {crtc_state = drm_atomic_get_new_crtc_state(state,
crtc);
crtc_state->zpos_changed = true;
if (old_plane_state->zpos != new_plane_state->zpos) {new_crtc_state = drm_atomic_get_new_crtc_state(state,
crtc);
new_crtc_state->zpos_changed = true;
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (crtc_state->plane_mask != crtc->state->plane_mask ||crtc_state->zpos_changed) {
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
new_crtc_state, i) {
if (old_crtc_state->plane_mask != new_crtc_state->plane_mask||
new_crtc_state->zpos_changed) { ret = drm_atomic_helper_crtc_normalize_zpos(crtc,
crtc_state);
new_crtc_state);
if (ret) return ret; }
Hi Maarten,
Thank you for the patches.
On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
Fourth iteration. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Old situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation:
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object. This will return NULL if the object is not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from obj_state->crtc too, this means not having to write some error handling.
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or
drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
That will make driver code quite complicated :-/ I wonder if that's really a good solution. Maybe we should maintain obj->state only for the duration of the commit as a way to simplify drivers, and set it to NULL at all other times to avoid misusing it ? I know this contradicts the comment I've made in my review of patch 1/7, you can now choose which one to ignore :-)
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as
needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
When not doing atomic updates: Look at obj->state for now. I have some patches to fix this but I was asked to make it return a const state. This breaks a lot of users though so I skipped that patch in this iteration.
This series will return the correct state regardless of swapping.
Maarten Lankhorst (7): drm/atomic: Add new iterators over all state, v3. drm/atomic: Make add_affected_connectors look at crtc_state. drm/atomic: Use new atomic iterator macros. drm/atomic: Fix atomic helpers to use the new iterator macros. drm/atomic: Add macros to access existing old/new state drm/atomic: Convert get_existing_state callers to get_old/new_state, v2. drm/blend: Use new atomic iterator macros.
drivers/gpu/drm/drm_atomic.c | 39 ++-- drivers/gpu/drm/drm_atomic_helper.c | 377 ++++++++++++++++++-------------- drivers/gpu/drm/drm_blend.c | 23 +-- drivers/gpu/drm/i915/intel_display.c | 13 +- include/drm/drm_atomic.h | 180 ++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + 6 files changed, 438 insertions(+), 196 deletions(-)
On Tue, Jan 17, 2017 at 02:45:32AM +0200, Laurent Pinchart wrote:
Hi Maarten,
Thank you for the patches.
On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
Fourth iteration. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Old situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation:
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object. This will return NULL if the object is not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from obj_state->crtc too, this means not having to write some error handling.
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or
drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
That will make driver code quite complicated :-/ I wonder if that's really a good solution. Maybe we should maintain obj->state only for the duration of the commit as a way to simplify drivers, and set it to NULL at all other times to avoid misusing it ? I know this contradicts the comment I've made in my review of patch 1/7, you can now choose which one to ignore :-)
We still need a state pointer to track the current logical state all the time (ignoring nonblocking commits or other async magic). We might do that in obj->_state or something to discourage use, but it needs to be there. -Daniel
Hi Maarten,
One more thing.
On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
Fourth iteration. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Old situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation:
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object. This will return NULL if the object is not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from obj_state->crtc too, this means not having to write some error handling.
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or
drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
There are four calls to the drm_atomic_get_existing_*_state() functions left in the DRM core after this series. There's also one call to drm_atomic_get_plane_state() in drm_blend.c. Could you please fix that ?
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as
needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
When not doing atomic updates: Look at obj->state for now. I have some patches to fix this but I was asked to make it return a const state. This breaks a lot of users though so I skipped that patch in this iteration.
This series will return the correct state regardless of swapping.
Maarten Lankhorst (7): drm/atomic: Add new iterators over all state, v3. drm/atomic: Make add_affected_connectors look at crtc_state. drm/atomic: Use new atomic iterator macros. drm/atomic: Fix atomic helpers to use the new iterator macros. drm/atomic: Add macros to access existing old/new state drm/atomic: Convert get_existing_state callers to get_old/new_state, v2. drm/blend: Use new atomic iterator macros.
drivers/gpu/drm/drm_atomic.c | 39 ++-- drivers/gpu/drm/drm_atomic_helper.c | 377 +++++++++++++++++------------- drivers/gpu/drm/drm_blend.c | 23 +-- drivers/gpu/drm/i915/intel_display.c | 13 +- include/drm/drm_atomic.h | 180 ++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + 6 files changed, 438 insertions(+), 196 deletions(-)
Hi Maarten,
Do you plan to post a v4 ? I have two drivers that depends on this series for fence support, and I'd like to get that merged in v4.12.
On Monday 16 Jan 2017 10:37:37 Maarten Lankhorst wrote:
Fourth iteration. Instead of trying to convert all drivers straight away, implement all macros that are required to get state working.
Old situation: Use obj->state, which can refer to old or new state. Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state. Use for_each_obj_in_state, which refers to new or old state.
New situation:
During atomic check:
- Use drm_atomic_get_obj_state to add a object to the atomic state, or get the new state.
- Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state, without adding the object. This will return NULL if the object is not part of the state. For planes and connectors the relevant crtc_state is added, so this will work to get the crtc_state from obj_state->crtc too, this means not having to write some error handling.
During atomic commit:
- Do not use drm_atomic_get_obj_state, obj->state or
drm_atomic_get_(existing_)obj_state any more, replace with drm_atomic_get_old/new_obj_state calls as required.
During both:
- Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as
needed. oldnew will be renamed to for_each_obj_in_state after all callers are converted to the new api.
When not doing atomic updates: Look at obj->state for now. I have some patches to fix this but I was asked to make it return a const state. This breaks a lot of users though so I skipped that patch in this iteration.
This series will return the correct state regardless of swapping.
Maarten Lankhorst (7): drm/atomic: Add new iterators over all state, v3. drm/atomic: Make add_affected_connectors look at crtc_state. drm/atomic: Use new atomic iterator macros. drm/atomic: Fix atomic helpers to use the new iterator macros. drm/atomic: Add macros to access existing old/new state drm/atomic: Convert get_existing_state callers to get_old/new_state, v2. drm/blend: Use new atomic iterator macros.
drivers/gpu/drm/drm_atomic.c | 39 ++-- drivers/gpu/drm/drm_atomic_helper.c | 377 ++++++++++++++++++-------------- drivers/gpu/drm/drm_blend.c | 23 +-- drivers/gpu/drm/i915/intel_display.c | 13 +- include/drm/drm_atomic.h | 180 ++++++++++++++++- include/drm/drm_atomic_helper.h | 2 + 6 files changed, 438 insertions(+), 196 deletions(-)
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Maarten Lankhorst