[Intel-gfx] [PATCH 6/9] drm/i915: PCH_NOP suspend/resume

[Ben Widawsky]

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