Re: [PATCH 6/9] drm/i915: PCH_NOP suspend/resume

From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org, "Mcallister,
	Jeffrey" <jeffrey.mcallister@intel.com>
Subject: Re: [PATCH 6/9] drm/i915: PCH_NOP suspend/resume
Date: Tue, 19 Mar 2013 10:53:21 -0700	[thread overview]
Message-ID: <20130319175320.GB1450@bwidawsk.net> (raw)
In-Reply-To: <CAKMK7uEJmdY7yybyJwsSaHMJe0i+YxvuOxyKbhhZPxB8DJP_Lw@mail.gmail.com>

On Tue, Mar 19, 2013 at 08:51:01AM +0100, Daniel Vetter wrote:
> On Tue, Mar 19, 2013 at 1:51 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Sun, Mar 17, 2013 at 10:28:55PM +0100, Daniel Vetter wrote:
> >> On Wed, Mar 13, 2013 at 11:21:05AM -0700, Ben Widawsky wrote:
> >> > More registers we can't write.
> >> >
> >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_suspend.c | 57 ++++++++++++++++++++++++++-----------
> >> >  1 file changed, 41 insertions(+), 16 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> >> > index c1e02b0..dd5766a 100644
> >> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> >> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> >> > @@ -333,11 +333,19 @@ int i915_save_state(struct drm_device *dev)
> >> >
> >> >     mutex_lock(&dev->struct_mutex);
> >> >
> >> > -   i915_save_display(dev);
> >> > +   if (!HAS_PCH_NOP(dev))
> >> > +           i915_save_display(dev);
> >>
> >> This here looks a bit funny - imo it's better to move this check to the
> >> only two places where we still touch registers in the kms case: lvds & pp
> >> restore.
> >
> > I had something like this originally, except I also can't touch the
> > backlight registers (even though they're not in a bad range).
> >
> > In an earlier patch you asked me to move the check up in the callchain,
> > and I liked that idea. It seems to me here the same logic applies, we
> > never care about any of the display registers. If you feel strongly
> > about it though, I will change it. Please correct me if I misunderstood
> > your request.
> 
> Yes, in general I think it's good to move the checks up in the
> callchains and consolidate them that way. I'm voting for an exception
> here in the register save/restore code since we're slowly moving away
> from it for kms. Moving the PCH_NOP check into the few remaining
> places allows us to easily kill them once those are guarded with if
> (!kms) checks, too. I've just figured that that way we won't have a
> stray PCH_NOP check left behind for code which (eventually) isn't run
> at all in kms mode.
> 
> I don't mind if you leave this as-is, really just a tiny bikeshed.
> -Daniel

I think having the check never hurts, and since I tend to introduce bugs
whenever I change things, I'll punt on this if you don't mind.

> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center