Re: [PATCH 10/14] drm/atomic-helper: Disable planes when suspending

From: Daniel Vetter <daniel@ffwll.ch>
To: Ying Liu <gnuiyl@gmail.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 10/14] drm/atomic-helper: Disable planes when suspending
Date: Wed, 25 May 2016 12:32:22 +0200	[thread overview]
Message-ID: <20160525103222.GI27098@phenom.ffwll.local> (raw)
In-Reply-To: <CAOcKUNWY2EzFukh1Mq3J9E+_2YYXqvwksa4-FxAbKbBWdY6P2g@mail.gmail.com>

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

-- 
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