During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com --- drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc; + struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev); @@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
+ drm_for_each_privobj(privobj, dev) { + struct drm_private_state *priv_state; + + priv_state = drm_atomic_get_private_obj_state(state, privobj); + if (IS_ERR(priv_state)) { + err = PTR_ERR(priv_state); + goto free; + } + } + drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state; @@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state; + struct drm_private_obj *privobj; + struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
+ for_each_new_private_obj_in_state(state, privobj, new_priv_state, i) + state->private_objs[i].old_state = privobj->state; + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com
drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc;
struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev);
@@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
- drm_for_each_privobj(privobj, dev) {
struct drm_private_state *priv_state;priv_state = drm_atomic_get_private_obj_state(state, privobj);if (IS_ERR(priv_state)) {err = PTR_ERR(priv_state);goto free;}- }
- drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state;
@@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;
struct drm_private_obj *privobj;
struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
state->private_objs[i].old_state = privobj->state;for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
Random order between crtc vs. plane vs. connector vs. priv is tickling my ocd nerve a bit.
Otherwise looks sensible to me. Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Ville Syrjälä ville.syrjala@linux.intel.com wrote on Thu [2019-Mar-14 17:31:29 +0200]:
On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com
drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc;
struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev);
@@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
- drm_for_each_privobj(privobj, dev) {
struct drm_private_state *priv_state;priv_state = drm_atomic_get_private_obj_state(state, privobj);if (IS_ERR(priv_state)) {err = PTR_ERR(priv_state);goto free;}- }
- drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state;
@@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;
struct drm_private_obj *privobj;
struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
state->private_objs[i].old_state = privobj->state;for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
Random order between crtc vs. plane vs. connector vs. priv is tickling my ocd nerve a bit.
These loops are independent from each other. And even without this patch the order is different between drm_atomic_helper_duplicate_state() and drm_atomic_helper_commit_duplicated_state(). :)
Benoit
Otherwise looks sensible to me. Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
-- 2.17.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel
Hi Benoit,
Thank you for the patch.
On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com
This looks good to me. I was actually wondering if private object state was properly handled by the suspend/resume helpers no later than yesterday. Seems you read my mind :-)
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc;
struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev);
@@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
- drm_for_each_privobj(privobj, dev) {
struct drm_private_state *priv_state;priv_state = drm_atomic_get_private_obj_state(state, privobj);if (IS_ERR(priv_state)) {err = PTR_ERR(priv_state);goto free;}- }
- drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state;
@@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;
struct drm_private_obj *privobj;
struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
state->private_objs[i].old_state = privobj->state;for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com
Why do you need this? We're doing a full atomic_check, and your atomic_check should be pulling in any private state objects and recomputing their states. This smells like papering over a driver bug.
The duplicate helpers should only need to duplicate the uapi-relevant states, i.e. connector/crtc/planes. -Daniel
drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc;
struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev);
@@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
- drm_for_each_privobj(privobj, dev) {
struct drm_private_state *priv_state;priv_state = drm_atomic_get_private_obj_state(state, privobj);if (IS_ERR(priv_state)) {err = PTR_ERR(priv_state);goto free;}- }
- drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state;
@@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;
struct drm_private_obj *privobj;
struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
state->private_objs[i].old_state = privobj->state;for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
-- 2.17.1
Daniel Vetter daniel@ffwll.ch wrote on Fri [2019-Mar-15 11:50:57 +0100]:
On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com
Why do you need this? We're doing a full atomic_check, and your atomic_check should be pulling in any private state objects and recomputing their states. This smells like papering over a driver bug.
I have not seen any atomic_check called during the suspend duplicate helper. And you are correct normally the private_object state in my case would get pulled in during a plane atomic_check but that does not happen here in this step.
Why wouldn't we save all the "state" artifacts so that the whole atomic state is consistent?
Benoit
The duplicate helpers should only need to duplicate the uapi-relevant states, i.e. connector/crtc/planes. -Daniel
drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc;
struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev);
@@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
- drm_for_each_privobj(privobj, dev) {
struct drm_private_state *priv_state;priv_state = drm_atomic_get_private_obj_state(state, privobj);if (IS_ERR(priv_state)) {err = PTR_ERR(priv_state);goto free;}- }
- drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state;
@@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;
struct drm_private_obj *privobj;
struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
state->private_objs[i].old_state = privobj->state;for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
-- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Fri, Mar 15, 2019 at 06:56:07AM -0500, Benoit Parrot wrote:
Daniel Vetter daniel@ffwll.ch wrote on Fri [2019-Mar-15 11:50:57 +0100]:
On Thu, Mar 14, 2019 at 08:44:45AM -0500, Benoit Parrot wrote:
During a suspend cycle the atomic state is saved to be used during the restore cycle.
However the current state duplication logic does not duplicate private objects. This leads to state inconsistencies at resume time.
With private objects modeset lock now integrated, we can make sure that private object state are properly saved and restored.
Signed-off-by: Benoit Parrot bparrot@ti.com
Why do you need this? We're doing a full atomic_check, and your atomic_check should be pulling in any private state objects and recomputing their states. This smells like papering over a driver bug.
I have not seen any atomic_check called during the suspend duplicate helper. And you are correct normally the private_object state in my case would get pulled in during a plane atomic_check but that does not happen here in this step.
Yeah duplicating in suspend won't call atomic_check. Restoring the duplicated state on resume will (or there's going to be lots of broken drivers).
Why wouldn't we save all the "state" artifacts so that the whole atomic state is consistent?
It's not needed, and might possible paper over driver bugs ...
The duplicate helpers should only need to duplicate the uapi-relevant states, i.e. connector/crtc/planes.
... because of this reason. If your driver can't recompute the private state from just the duplicated uapi-relevant states it's broken. Because essentially the exact same thing might happen through the atomic ioctl from userspace.
Hence my question: Why do you need this? -Daniel
-Daniel
drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 540a77a2ade9..b108021cc092 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3189,6 +3189,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, struct drm_connector_list_iter conn_iter; struct drm_plane *plane; struct drm_crtc *crtc;
struct drm_private_obj *privobj; int err = 0;
state = drm_atomic_state_alloc(dev);
@@ -3218,6 +3219,16 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev, } }
- drm_for_each_privobj(privobj, dev) {
struct drm_private_state *priv_state;priv_state = drm_atomic_get_private_obj_state(state, privobj);if (IS_ERR(priv_state)) {err = PTR_ERR(priv_state);goto free;}- }
- drm_connector_list_iter_begin(dev, &conn_iter); drm_for_each_connector_iter(conn, &conn_iter) { struct drm_connector_state *conn_state;
@@ -3325,12 +3336,17 @@ int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_connector_state *new_conn_state; struct drm_crtc *crtc; struct drm_crtc_state *new_crtc_state;
struct drm_private_obj *privobj;
struct drm_private_state *new_priv_state;
state->acquire_ctx = ctx;
for_each_new_plane_in_state(state, plane, new_plane_state, i) state->planes[i].old_state = plane->state;
for_each_new_private_obj_in_state(state, privobj, new_priv_state, i)
state->private_objs[i].old_state = privobj->state;for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) state->crtcs[i].old_state = crtc->state;
-- 2.17.1
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Benoit Parrot -
Daniel Vetter -
Laurent Pinchart -
Ville Syrjälä