| Message ID | 20190314134445.19260-1-bparrot@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] drm/atomic: integrate private objects with suspend/resume helpers | expand |
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
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;
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(+)