From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/4] drm/i915: restore cursor and sprite state when forcing a config restore Date: Tue, 26 Mar 2013 17:52:26 +0100 Message-ID: <20130326165226.GW9021@phenom.ffwll.local> References: <1364315146-20542-1-git-send-email-jbarnes@virtuousgeek.org> <1364315146-20542-2-git-send-email-jbarnes@virtuousgeek.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f48.google.com (mail-ee0-f48.google.com [74.125.83.48]) by gabe.freedesktop.org (Postfix) with ESMTP id B3C0AE60F2 for ; Tue, 26 Mar 2013 09:49:37 -0700 (PDT) Received: by mail-ee0-f48.google.com with SMTP id b15so959362eek.7 for ; Tue, 26 Mar 2013 09:49:37 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1364315146-20542-2-git-send-email-jbarnes@virtuousgeek.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Tue, Mar 26, 2013 at 09:25:44AM -0700, Jesse Barnes wrote: > Needed for VT switchless resume. > > Signed-off-by: Jesse Barnes > --- > drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 7307974..093006b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9106,6 +9106,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > struct drm_i915_private *dev_priv = dev->dev_private; > enum pipe pipe; > u32 tmp; > + struct drm_plane *plane; > struct intel_crtc *crtc; > struct intel_encoder *encoder; > struct intel_connector *connector; > @@ -9210,8 +9211,20 @@ setup_pipes: > > if (force_restore) { > for_each_pipe(pipe) { > - intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]); > + struct drm_crtc *crtc = > + dev_priv->pipe_to_crtc_mapping[pipe]; > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + intel_crtc_restore_mode(crtc); I've just noticed that crtc_restore_mode is a bit buggy, since it unconditionally enables the pipe. Which is the wrong thing to do if the pipe has a mode set, but is disabled with dpms off. > + if (intel_crtc->cursor_visible) { > + /* Force update for previously enabled cursor */ > + intel_crtc->cursor_visible = false; > + intel_crtc_update_cursor(&intel_crtc->base, > + true); > + } We already have an unconditional cursor enable call in the various platform_crtc_enable functions. Do we really neeed this? Or should we actually do this ->cursor_visible dance in there instead of here? > } > + list_for_each_entry(plane, &dev->mode_config.plane_list, head) > + intel_plane_restore(plane); Thinking more about this we have the same plane restore issue for dpms. Thanks to my utterly lazy legacy overlay code userspace has to take care of things already, but what about we move that fix here to the right place? Would also be useful for the atomic modeset support, where we want to light up a cursor/plane config right away ... With those things fixed I think the only thing we need to do here is walk all the connectors and do a dpms update. Presuming that the crtc->active tracking is reflecting reality (i.e. all false) it should do the right thing and restore stuff (at least if your crtc disabling doesn't update the dpms state - disabled crtc means implicitly dpms off). Like I've said this has the added benefit that the dpms code is shared with the resume code. Cheers, Your royal pita maintainer ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch