| Message ID | 1464084653-16684-11-git-send-email-gnuiyl@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 24, 2016 at 06:10:49PM +0800, Liu Ying wrote: > We should disable planes explicitly when suspending. > Especially, this is meaningful for those display controllers which > don't support active planes without relevant CRTCs being enabled. > > Signed-off-by: Liu Ying <gnuiyl@gmail.com> Recommended way is to call drm_atomic_helper_disable_planes_on_crtc in your crtc's ->disable() callback if your hw needs this. This is a general problem (test e.g. dpms), not just an issue in suspend code. Also unsetting the planes from state has a semantic meaning: It unpins the backing storage, which is definitely not what we want for suspend/resume. -Daniel > --- > drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 4befe25..5331d95 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1967,7 +1967,7 @@ commit: > * > * Loops through all connectors, finding those that aren't turned off and then > * turns them off by setting their DPMS mode to OFF and deactivating the CRTC > - * that they are connected to. > + * that they are connected to. The relevant planes are deactivated as well. > * > * This is used for example in suspend/resume to disable all currently active > * functions when suspending. > @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, > drm_for_each_connector(conn, dev) { > struct drm_crtc *crtc = conn->state->crtc; > struct drm_crtc_state *crtc_state; > + struct drm_plane *plane; > > if (!crtc || conn->dpms != DRM_MODE_DPMS_ON) > continue; > @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, > } > > crtc_state->active = false; > + > + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) { > + struct drm_plane_state *plane_state; > + > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) { > + err = PTR_ERR(plane_state); > + goto free; > + } > + > + err = drm_atomic_set_crtc_for_plane(plane_state, NULL); > + if (err != 0) > + goto free; > + drm_atomic_set_fb_for_plane(plane_state, NULL); > + } > } > > err = drm_atomic_commit(state); > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, May 24, 2016 at 7:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, May 24, 2016 at 06:10:49PM +0800, Liu Ying wrote: >> We should disable planes explicitly when suspending. >> Especially, this is meaningful for those display controllers which >> don't support active planes without relevant CRTCs being enabled. >> >> Signed-off-by: Liu Ying <gnuiyl@gmail.com> > > Recommended way is to call drm_atomic_helper_disable_planes_on_crtc in > your crtc's ->disable() callback if your hw needs this. This is a general > problem (test e.g. dpms), not just an issue in suspend code. It looks legacy fbdev unblank operation fails after call that function in ->disable(). I made the change on top of this patch set. If you have any idea, please help out here. Regards, Liu Ying > > Also unsetting the planes from state has a semantic meaning: It unpins the > backing storage, which is definitely not what we want for suspend/resume. > -Daniel > >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index 4befe25..5331d95 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -1967,7 +1967,7 @@ commit: >> * >> * Loops through all connectors, finding those that aren't turned off and then >> * turns them off by setting their DPMS mode to OFF and deactivating the CRTC >> - * that they are connected to. >> + * that they are connected to. The relevant planes are deactivated as well. >> * >> * This is used for example in suspend/resume to disable all currently active >> * functions when suspending. >> @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, >> drm_for_each_connector(conn, dev) { >> struct drm_crtc *crtc = conn->state->crtc; >> struct drm_crtc_state *crtc_state; >> + struct drm_plane *plane; >> >> if (!crtc || conn->dpms != DRM_MODE_DPMS_ON) >> continue; >> @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, >> } >> >> crtc_state->active = false; >> + >> + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) { >> + struct drm_plane_state *plane_state; >> + >> + plane_state = drm_atomic_get_plane_state(state, plane); >> + if (IS_ERR(plane_state)) { >> + err = PTR_ERR(plane_state); >> + goto free; >> + } >> + >> + err = drm_atomic_set_crtc_for_plane(plane_state, NULL); >> + if (err != 0) >> + goto free; >> + drm_atomic_set_fb_for_plane(plane_state, NULL); >> + } >> } >> >> err = drm_atomic_commit(state); >> -- >> 2.7.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, May 25, 2016 at 05:30:05PM +0800, Ying Liu wrote: > On Tue, May 24, 2016 at 7:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, May 24, 2016 at 06:10:49PM +0800, Liu Ying wrote: > >> We should disable planes explicitly when suspending. > >> Especially, this is meaningful for those display controllers which > >> don't support active planes without relevant CRTCs being enabled. > >> > >> Signed-off-by: Liu Ying <gnuiyl@gmail.com> > > > > Recommended way is to call drm_atomic_helper_disable_planes_on_crtc in > > your crtc's ->disable() callback if your hw needs this. This is a general > > problem (test e.g. dpms), not just an issue in suspend code. > > It looks legacy fbdev unblank operation fails after call that function > in ->disable(). I made the change on top of this patch set. > If you have any idea, please help out here. Please explain in detail what fails and how, but your patch here is definitely not the solution. This is something your driver must be able to handle, and which cannot be handled in all callers of ->atomic_commit. -Daniel > > Regards, > Liu Ying > > > > > Also unsetting the planes from state has a semantic meaning: It unpins the > > backing storage, which is definitely not what we want for suspend/resume. > > -Daniel > > > >> --- > >> drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++- > >> 1 file changed, 17 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > >> index 4befe25..5331d95 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -1967,7 +1967,7 @@ commit: > >> * > >> * Loops through all connectors, finding those that aren't turned off and then > >> * turns them off by setting their DPMS mode to OFF and deactivating the CRTC > >> - * that they are connected to. > >> + * that they are connected to. The relevant planes are deactivated as well. > >> * > >> * This is used for example in suspend/resume to disable all currently active > >> * functions when suspending. > >> @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, > >> drm_for_each_connector(conn, dev) { > >> struct drm_crtc *crtc = conn->state->crtc; > >> struct drm_crtc_state *crtc_state; > >> + struct drm_plane *plane; > >> > >> if (!crtc || conn->dpms != DRM_MODE_DPMS_ON) > >> continue; > >> @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, > >> } > >> > >> crtc_state->active = false; > >> + > >> + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) { > >> + struct drm_plane_state *plane_state; > >> + > >> + plane_state = drm_atomic_get_plane_state(state, plane); > >> + if (IS_ERR(plane_state)) { > >> + err = PTR_ERR(plane_state); > >> + goto free; > >> + } > >> + > >> + err = drm_atomic_set_crtc_for_plane(plane_state, NULL); > >> + if (err != 0) > >> + goto free; > >> + drm_atomic_set_fb_for_plane(plane_state, NULL); > >> + } > >> } > >> > >> err = drm_atomic_commit(state); > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 4befe25..5331d95 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1967,7 +1967,7 @@ commit: * * Loops through all connectors, finding those that aren't turned off and then * turns them off by setting their DPMS mode to OFF and deactivating the CRTC - * that they are connected to. + * that they are connected to. The relevant planes are deactivated as well. * * This is used for example in suspend/resume to disable all currently active * functions when suspending. @@ -1997,6 +1997,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, drm_for_each_connector(conn, dev) { struct drm_crtc *crtc = conn->state->crtc; struct drm_crtc_state *crtc_state; + struct drm_plane *plane; if (!crtc || conn->dpms != DRM_MODE_DPMS_ON) continue; @@ -2008,6 +2009,21 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, } crtc_state->active = false; + + drm_for_each_plane_mask(plane, dev, crtc_state->plane_mask) { + struct drm_plane_state *plane_state; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) { + err = PTR_ERR(plane_state); + goto free; + } + + err = drm_atomic_set_crtc_for_plane(plane_state, NULL); + if (err != 0) + goto free; + drm_atomic_set_fb_for_plane(plane_state, NULL); + } } err = drm_atomic_commit(state);
We should disable planes explicitly when suspending. Especially, this is meaningful for those display controllers which don't support active planes without relevant CRTCs being enabled. Signed-off-by: Liu Ying <gnuiyl@gmail.com> --- drivers/gpu/drm/drm_atomic_helper.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)