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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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? <[email protected]>
>
> --
> 2.17.1
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel
Ville Syrj?l? <[email protected]> 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 <[email protected]>
> > ---
> > 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? <[email protected]>
>
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > 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 <[email protected]>
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 <[email protected]>
> ---
> 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;
>
--
Regards,
Laurent Pinchart
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 <[email protected]>
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
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Daniel Vetter <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
> >
> > 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch