| Message ID | 1393001548-2883-5-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 21 Feb 2014 13:52:21 -0300 Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Otherwise, when we run intel_modeset_check_state we may already be > runtime suspended, and our state checking code will read registers > while the device is suspended. This can only happen if your > autosuspend_delay_ms is low (not the default 10s). > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 10ec401..c64fb7f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, > struct drm_display_mode *mode, > int x, int y, struct drm_framebuffer *fb) > { > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + intel_runtime_pm_get(dev_priv); > + > ret = __intel_set_mode(crtc, mode, x, y, fb); > > if (ret == 0) > intel_modeset_check_state(crtc->dev); > > + intel_runtime_pm_put(dev_priv); > return ret; > } > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Otherwise, when we run intel_modeset_check_state we may already be > runtime suspended, and our state checking code will read registers > while the device is suspended. This can only happen if your > autosuspend_delay_ms is low (not the default 10s). > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 10ec401..c64fb7f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, > struct drm_display_mode *mode, > int x, int y, struct drm_framebuffer *fb) > { > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + intel_runtime_pm_get(dev_priv); > + > ret = __intel_set_mode(crtc, mode, x, y, fb); > > if (ret == 0) > intel_modeset_check_state(crtc->dev); > > + intel_runtime_pm_put(dev_priv); > return ret; > } Ideally these should be done as part of a power domain get/put as some platforms will need to turn on some power wells too and on that path we do anyway a runtime PM get/put. In the latest VLV power domain support patchset [1] I added the power domain get/put and state check to places I thought necessary. I haven't tested it on HSW but afaics the ones added for the HW state readout code would solve the issue you describe here. --Imre [1] http://www.spinics.net/lists/intel-gfx/msg40344.html
2014-02-24 8:23 GMT-03:00 Imre Deak <imre.deak@intel.com>: > On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Otherwise, when we run intel_modeset_check_state we may already be >> runtime suspended, and our state checking code will read registers >> while the device is suspended. This can only happen if your >> autosuspend_delay_ms is low (not the default 10s). >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 10ec401..c64fb7f 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, >> struct drm_display_mode *mode, >> int x, int y, struct drm_framebuffer *fb) >> { >> + struct drm_device *dev = crtc->dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> int ret; >> >> + intel_runtime_pm_get(dev_priv); >> + >> ret = __intel_set_mode(crtc, mode, x, y, fb); >> >> if (ret == 0) >> intel_modeset_check_state(crtc->dev); >> >> + intel_runtime_pm_put(dev_priv); >> return ret; >> } > > Ideally these should be done as part of a power domain get/put as some > platforms will need to turn on some power wells too and on that path we > do anyway a runtime PM get/put. > > In the latest VLV power domain support patchset [1] I added the power > domain get/put and state check to places I thought necessary. I haven't > tested it on HSW but afaics the ones added for the HW state readout code > would solve the issue you describe here. Yes. I just quickly read the patches, and they seem to try to solve this problem. Due to the reasons you wrote on the first paragraph, I think in the long term we want the power domains solution. But as I mentioned in the cover letter, this series contains bug fixes and maybe we want them on -fixes and even stable Kernels, so maybe we want to merge this patch, then later merge the code that uses power domains, then remove the runitme_pm_get calls and leave just the power domain calls? I'm not sure. > > --Imre > > [1] http://www.spinics.net/lists/intel-gfx/msg40344.html
On Mon, 2014-02-24 at 11:34 -0300, Paulo Zanoni wrote: > 2014-02-24 8:23 GMT-03:00 Imre Deak <imre.deak@intel.com>: > > On Fri, 2014-02-21 at 13:52 -0300, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> Otherwise, when we run intel_modeset_check_state we may already be > >> runtime suspended, and our state checking code will read registers > >> while the device is suspended. This can only happen if your > >> autosuspend_delay_ms is low (not the default 10s). > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 5 +++++ > >> 1 file changed, 5 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 10ec401..c64fb7f 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, > >> struct drm_display_mode *mode, > >> int x, int y, struct drm_framebuffer *fb) > >> { > >> + struct drm_device *dev = crtc->dev; > >> + struct drm_i915_private *dev_priv = dev->dev_private; > >> int ret; > >> > >> + intel_runtime_pm_get(dev_priv); > >> + > >> ret = __intel_set_mode(crtc, mode, x, y, fb); > >> > >> if (ret == 0) > >> intel_modeset_check_state(crtc->dev); > >> > >> + intel_runtime_pm_put(dev_priv); > >> return ret; > >> } > > > > Ideally these should be done as part of a power domain get/put as some > > platforms will need to turn on some power wells too and on that path we > > do anyway a runtime PM get/put. > > > > In the latest VLV power domain support patchset [1] I added the power > > domain get/put and state check to places I thought necessary. I haven't > > tested it on HSW but afaics the ones added for the HW state readout code > > would solve the issue you describe here. > > Yes. I just quickly read the patches, and they seem to try to solve > this problem. Due to the reasons you wrote on the first paragraph, I > think in the long term we want the power domains solution. But as I > mentioned in the cover letter, this series contains bug fixes and > maybe we want them on -fixes and even stable Kernels, so maybe we want > to merge this patch, then later merge the code that uses power > domains, then remove the runitme_pm_get calls and leave just the power > domain calls? I'm not sure. Ok, I can rebase my patchset based on the above. Note that runtime PM was enabled post 3.13, so this fix is needed for -fixes, but not for stable kernels. The same goes for patch 5/11. --Imre
On Fri, Feb 21, 2014 at 01:52:21PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Otherwise, when we run intel_modeset_check_state we may already be > runtime suspended, and our state checking code will read registers > while the device is suspended. This can only happen if your > autosuspend_delay_ms is low (not the default 10s). > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 10ec401..c64fb7f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, > struct drm_display_mode *mode, > int x, int y, struct drm_framebuffer *fb) > { > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > + intel_runtime_pm_get(dev_priv); > + > ret = __intel_set_mode(crtc, mode, x, y, fb); > > if (ret == 0) > intel_modeset_check_state(crtc->dev); > > + intel_runtime_pm_put(dev_priv); I've thought our code has the relevant power domain checks sprinkled all over the get_config/state functions already? If that's not the case I prefer we fix that, similar to my suggestion in reply to Imre's patches of moving the power domain checking into the callbacks. Wrt -fixes: Since the default autosuspend delay will make it impossible to hit this I think we can punt. Rules for late -rc and cc: stable is that it needs to be a real-world problem with a real bug report. And one more: To make pm_pc8 more useful, could we just set the autosuspend delay to 0 while the test is running? Then restore it again with an igt atexit handler. That should help with our coverage and hitting this tiny little issues you've fixed in this series. -Daniel
2014-03-05 10:25 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Feb 21, 2014 at 01:52:21PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> Otherwise, when we run intel_modeset_check_state we may already be >> runtime suspended, and our state checking code will read registers >> while the device is suspended. This can only happen if your >> autosuspend_delay_ms is low (not the default 10s). >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 10ec401..c64fb7f 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, >> struct drm_display_mode *mode, >> int x, int y, struct drm_framebuffer *fb) >> { >> + struct drm_device *dev = crtc->dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> int ret; >> >> + intel_runtime_pm_get(dev_priv); >> + >> ret = __intel_set_mode(crtc, mode, x, y, fb); >> >> if (ret == 0) >> intel_modeset_check_state(crtc->dev); >> >> + intel_runtime_pm_put(dev_priv); > > I've thought our code has the relevant power domain checks sprinkled all > over the get_config/state functions already? If that's not the case I > prefer we fix that, similar to my suggestion in reply to Imre's patches of > moving the power domain checking into the callbacks. > > Wrt -fixes: Since the default autosuspend delay will make it impossible to > hit this I think we can punt. Rules for late -rc and cc: stable is that it > needs to be a real-world problem with a real bug report. > > And one more: To make pm_pc8 more useful, could we just set the > autosuspend delay to 0 while the test is running? Then restore it again > with an igt atexit handler. That should help with our coverage and hitting > this tiny little issues you've fixed in this series. We already set autosuspend_delay_msg to zero when the test is running, we just don't restore the original value later. The problem is that we currently have autosuspend_delay_ms and i915.pc8_timeout as non-zero default, and we don't have support to change i915.pc8_timeout at runtime. If we merge the series that's currently under review (00/23 Merge PC8 with runtime PM, v2), we'll just kill i915.pc8_timeout, so we should be fine, and QA will automatically start running everything with zero timeouts. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 10ec401..c64fb7f 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9746,13 +9746,18 @@ static int intel_set_mode(struct drm_crtc *crtc, struct drm_display_mode *mode, int x, int y, struct drm_framebuffer *fb) { + struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; int ret; + intel_runtime_pm_get(dev_priv); + ret = __intel_set_mode(crtc, mode, x, y, fb); if (ret == 0) intel_modeset_check_state(crtc->dev); + intel_runtime_pm_put(dev_priv); return ret; }