| Message ID | 1363198868-21787-7-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) { > /* Interrupt state */ > - if (HAS_PCH_SPLIT(dev)) { > + if (HAS_PCH_NOP(dev)) { > + dev_priv->regfile.saveDEIER = I915_READ(DEIER); > + dev_priv->regfile.saveDEIMR = I915_READ(DEIMR); > + dev_priv->regfile.saveGTIER = I915_READ(GTIER); > + dev_priv->regfile.saveGTIMR = I915_READ(GTIMR); > + dev_priv->regfile.saveMCHBAR_RENDER_STANDBY = > + I915_READ(RSTDBYCTL); > + } else if (HAS_PCH_SPLIT(dev)) { > dev_priv->regfile.saveDEIER = I915_READ(DEIER); > dev_priv->regfile.saveDEIMR = I915_READ(DEIMR); > dev_priv->regfile.saveGTIER = I915_READ(GTIER); > @@ -361,13 +369,18 @@ int i915_save_state(struct drm_device *dev) > /* Memory Arbitration state */ > dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE); > > - /* Scratch space */ > - for (i = 0; i < 16; i++) { > - dev_priv->regfile.saveSWF0[i] = I915_READ(SWF00 + (i << 2)); > - dev_priv->regfile.saveSWF1[i] = I915_READ(SWF10 + (i << 2)); > + if (!HAS_PCH_NOP(dev)) { > + /* Scratch space */ > + for (i = 0; i < 16; i++) { > + dev_priv->regfile.saveSWF0[i] = > + I915_READ(SWF00 + (i << 2)); > + dev_priv->regfile.saveSWF1[i] = > + I915_READ(SWF10 + (i << 2)); > + } > + for (i = 0; i < 3; i++) > + dev_priv->regfile.saveSWF2[i] = > + I915_READ(SWF30 + (i << 2)); Blergh, I hate those registers, and I have no idea where we actually need to restore them for kms. Can you please also add a big "XXX: Do we really need this for kms?" comment in the scratch space block? > } > - for (i = 0; i < 3; i++) > - dev_priv->regfile.saveSWF2[i] = I915_READ(SWF30 + (i << 2)); > > mutex_unlock(&dev->struct_mutex); > > @@ -383,11 +396,17 @@ int i915_restore_state(struct drm_device *dev) > > mutex_lock(&dev->struct_mutex); > > - i915_restore_display(dev); > + if (!HAS_PCH_NOP(dev)) > + i915_restore_display(dev); > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) { > /* Interrupt state */ > - if (HAS_PCH_SPLIT(dev)) { > + if (HAS_PCH_NOP(dev)) { > + I915_WRITE(DEIER, dev_priv->regfile.saveDEIER); > + I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR); > + I915_WRITE(GTIER, dev_priv->regfile.saveGTIER); > + I915_WRITE(GTIMR, dev_priv->regfile.saveGTIMR); > + } else if (HAS_PCH_SPLIT(dev)) { > I915_WRITE(DEIER, dev_priv->regfile.saveDEIER); > I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR); > I915_WRITE(GTIER, dev_priv->regfile.saveGTIER); > @@ -407,16 +426,22 @@ int i915_restore_state(struct drm_device *dev) > /* Memory arbitration state */ > I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000); > > - for (i = 0; i < 16; i++) { > - I915_WRITE(SWF00 + (i << 2), dev_priv->regfile.saveSWF0[i]); > - I915_WRITE(SWF10 + (i << 2), dev_priv->regfile.saveSWF1[i]); > + if (!HAS_PCH_NOP(dev)) { > + for (i = 0; i < 16; i++) { > + I915_WRITE(SWF00 + (i << 2), > + dev_priv->regfile.saveSWF0[i]); > + I915_WRITE(SWF10 + (i << 2), > + dev_priv->regfile.saveSWF1[i]); > + } > + for (i = 0; i < 3; i++) > + I915_WRITE(SWF30 + (i << 2), > + dev_priv->regfile.saveSWF2[i]); > } > - for (i = 0; i < 3; i++) > - I915_WRITE(SWF30 + (i << 2), dev_priv->regfile.saveSWF2[i]); > > mutex_unlock(&dev->struct_mutex); > > - intel_i2c_reset(dev); > + if (!HAS_PCH_NOP(dev)) > + intel_i2c_reset(dev); > > return 0; > } > -- > 1.8.1.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. > > > > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) { > > /* Interrupt state */ > > - if (HAS_PCH_SPLIT(dev)) { > > + if (HAS_PCH_NOP(dev)) { > > + dev_priv->regfile.saveDEIER = I915_READ(DEIER); > > + dev_priv->regfile.saveDEIMR = I915_READ(DEIMR); > > + dev_priv->regfile.saveGTIER = I915_READ(GTIER); > > + dev_priv->regfile.saveGTIMR = I915_READ(GTIMR); > > + dev_priv->regfile.saveMCHBAR_RENDER_STANDBY = > > + I915_READ(RSTDBYCTL); > > + } else if (HAS_PCH_SPLIT(dev)) { > > dev_priv->regfile.saveDEIER = I915_READ(DEIER); > > dev_priv->regfile.saveDEIMR = I915_READ(DEIMR); > > dev_priv->regfile.saveGTIER = I915_READ(GTIER); > > @@ -361,13 +369,18 @@ int i915_save_state(struct drm_device *dev) > > /* Memory Arbitration state */ > > dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE); > > > > - /* Scratch space */ > > - for (i = 0; i < 16; i++) { > > - dev_priv->regfile.saveSWF0[i] = I915_READ(SWF00 + (i << 2)); > > - dev_priv->regfile.saveSWF1[i] = I915_READ(SWF10 + (i << 2)); > > + if (!HAS_PCH_NOP(dev)) { > > + /* Scratch space */ > > + for (i = 0; i < 16; i++) { > > + dev_priv->regfile.saveSWF0[i] = > > + I915_READ(SWF00 + (i << 2)); > > + dev_priv->regfile.saveSWF1[i] = > > + I915_READ(SWF10 + (i << 2)); > > + } > > + for (i = 0; i < 3; i++) > > + dev_priv->regfile.saveSWF2[i] = > > + I915_READ(SWF30 + (i << 2)); > > Blergh, I hate those registers, and I have no idea where we actually need > to restore them for kms. Can you please also add a big "XXX: Do we really > need this for kms?" comment in the scratch space block? > > > } > > - for (i = 0; i < 3; i++) > > - dev_priv->regfile.saveSWF2[i] = I915_READ(SWF30 + (i << 2)); > > > > mutex_unlock(&dev->struct_mutex); > > > > @@ -383,11 +396,17 @@ int i915_restore_state(struct drm_device *dev) > > > > mutex_lock(&dev->struct_mutex); > > > > - i915_restore_display(dev); > > + if (!HAS_PCH_NOP(dev)) > > + i915_restore_display(dev); > > > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) { > > /* Interrupt state */ > > - if (HAS_PCH_SPLIT(dev)) { > > + if (HAS_PCH_NOP(dev)) { > > + I915_WRITE(DEIER, dev_priv->regfile.saveDEIER); > > + I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR); > > + I915_WRITE(GTIER, dev_priv->regfile.saveGTIER); > > + I915_WRITE(GTIMR, dev_priv->regfile.saveGTIMR); > > + } else if (HAS_PCH_SPLIT(dev)) { > > I915_WRITE(DEIER, dev_priv->regfile.saveDEIER); > > I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR); > > I915_WRITE(GTIER, dev_priv->regfile.saveGTIER); > > @@ -407,16 +426,22 @@ int i915_restore_state(struct drm_device *dev) > > /* Memory arbitration state */ > > I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000); > > > > - for (i = 0; i < 16; i++) { > > - I915_WRITE(SWF00 + (i << 2), dev_priv->regfile.saveSWF0[i]); > > - I915_WRITE(SWF10 + (i << 2), dev_priv->regfile.saveSWF1[i]); > > + if (!HAS_PCH_NOP(dev)) { > > + for (i = 0; i < 16; i++) { > > + I915_WRITE(SWF00 + (i << 2), > > + dev_priv->regfile.saveSWF0[i]); > > + I915_WRITE(SWF10 + (i << 2), > > + dev_priv->regfile.saveSWF1[i]); > > + } > > + for (i = 0; i < 3; i++) > > + I915_WRITE(SWF30 + (i << 2), > > + dev_priv->regfile.saveSWF2[i]); > > } > > - for (i = 0; i < 3; i++) > > - I915_WRITE(SWF30 + (i << 2), dev_priv->regfile.saveSWF2[i]); > > > > mutex_unlock(&dev->struct_mutex); > > > > - intel_i2c_reset(dev); > > + if (!HAS_PCH_NOP(dev)) > > + intel_i2c_reset(dev); > > > > return 0; > > } > > -- > > 1.8.1.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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
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
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); if (!drm_core_check_feature(dev, DRIVER_MODESET)) { /* Interrupt state */ - if (HAS_PCH_SPLIT(dev)) { + if (HAS_PCH_NOP(dev)) { + dev_priv->regfile.saveDEIER = I915_READ(DEIER); + dev_priv->regfile.saveDEIMR = I915_READ(DEIMR); + dev_priv->regfile.saveGTIER = I915_READ(GTIER); + dev_priv->regfile.saveGTIMR = I915_READ(GTIMR); + dev_priv->regfile.saveMCHBAR_RENDER_STANDBY = + I915_READ(RSTDBYCTL); + } else if (HAS_PCH_SPLIT(dev)) { dev_priv->regfile.saveDEIER = I915_READ(DEIER); dev_priv->regfile.saveDEIMR = I915_READ(DEIMR); dev_priv->regfile.saveGTIER = I915_READ(GTIER); @@ -361,13 +369,18 @@ int i915_save_state(struct drm_device *dev) /* Memory Arbitration state */ dev_priv->regfile.saveMI_ARB_STATE = I915_READ(MI_ARB_STATE); - /* Scratch space */ - for (i = 0; i < 16; i++) { - dev_priv->regfile.saveSWF0[i] = I915_READ(SWF00 + (i << 2)); - dev_priv->regfile.saveSWF1[i] = I915_READ(SWF10 + (i << 2)); + if (!HAS_PCH_NOP(dev)) { + /* Scratch space */ + for (i = 0; i < 16; i++) { + dev_priv->regfile.saveSWF0[i] = + I915_READ(SWF00 + (i << 2)); + dev_priv->regfile.saveSWF1[i] = + I915_READ(SWF10 + (i << 2)); + } + for (i = 0; i < 3; i++) + dev_priv->regfile.saveSWF2[i] = + I915_READ(SWF30 + (i << 2)); } - for (i = 0; i < 3; i++) - dev_priv->regfile.saveSWF2[i] = I915_READ(SWF30 + (i << 2)); mutex_unlock(&dev->struct_mutex); @@ -383,11 +396,17 @@ int i915_restore_state(struct drm_device *dev) mutex_lock(&dev->struct_mutex); - i915_restore_display(dev); + if (!HAS_PCH_NOP(dev)) + i915_restore_display(dev); if (!drm_core_check_feature(dev, DRIVER_MODESET)) { /* Interrupt state */ - if (HAS_PCH_SPLIT(dev)) { + if (HAS_PCH_NOP(dev)) { + I915_WRITE(DEIER, dev_priv->regfile.saveDEIER); + I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR); + I915_WRITE(GTIER, dev_priv->regfile.saveGTIER); + I915_WRITE(GTIMR, dev_priv->regfile.saveGTIMR); + } else if (HAS_PCH_SPLIT(dev)) { I915_WRITE(DEIER, dev_priv->regfile.saveDEIER); I915_WRITE(DEIMR, dev_priv->regfile.saveDEIMR); I915_WRITE(GTIER, dev_priv->regfile.saveGTIER); @@ -407,16 +426,22 @@ int i915_restore_state(struct drm_device *dev) /* Memory arbitration state */ I915_WRITE(MI_ARB_STATE, dev_priv->regfile.saveMI_ARB_STATE | 0xffff0000); - for (i = 0; i < 16; i++) { - I915_WRITE(SWF00 + (i << 2), dev_priv->regfile.saveSWF0[i]); - I915_WRITE(SWF10 + (i << 2), dev_priv->regfile.saveSWF1[i]); + if (!HAS_PCH_NOP(dev)) { + for (i = 0; i < 16; i++) { + I915_WRITE(SWF00 + (i << 2), + dev_priv->regfile.saveSWF0[i]); + I915_WRITE(SWF10 + (i << 2), + dev_priv->regfile.saveSWF1[i]); + } + for (i = 0; i < 3; i++) + I915_WRITE(SWF30 + (i << 2), + dev_priv->regfile.saveSWF2[i]); } - for (i = 0; i < 3; i++) - I915_WRITE(SWF30 + (i << 2), dev_priv->regfile.saveSWF2[i]); mutex_unlock(&dev->struct_mutex); - intel_i2c_reset(dev); + if (!HAS_PCH_NOP(dev)) + intel_i2c_reset(dev); return 0; }
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(-)