| Message ID | 1381792069-27800-5-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>: > When accessing the display regs for hw state readout or cross check, we > need to make sure the power well is enabled so we can read valid > register state. On the current code (HSW) we already check for the power wells in the HW state readout code: if the power well is disabled, then transcoders A/B/C are disabled. The current code takes care to not touch registers on a disabled power well. > > Likewise, in an actual mode set, we need to take a ref on the > appropriate power well so that the mode set succeeds. From then on, the > power well ref will be tracked by the CRTC enable/disable code. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_dma.c | 2 ++ > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 313a8c9..91c3e6c 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1367,6 +1367,8 @@ static int i915_load_modeset_init(struct drm_device *dev) > i915_disable_vga_mem(dev); > intel_display_power_put(dev, POWER_DOMAIN_VGA); > > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); > + Why is this here? It certainly deserves a comment in the code. > /* Only enable hotplug handling once the fbdev is fully set up. */ > dev_priv->enable_hotplug_processing = true; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 6e4729b..62ee110 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3841,6 +3841,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) > if (intel_crtc->active) > return; > > + intel_display_power_get(dev, POWER_DOMAIN_PIPE(pipe)); > + > intel_crtc->active = true; > > for_each_encoder_on_crtc(dev, crtc, encoder) > @@ -3975,6 +3977,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) > intel_update_watermarks(crtc); > > intel_update_fbc(dev); > + > + if (IS_VALLEYVIEW(dev)) > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(pipe)); So now HSW uses global_resources while VLV uses crtc enable/disable. I really think both platforms should try to do the same thing. By the way, at least on Haswell, if we do an equivalent change we'll have problems, because when you disable the power well at crtc_disable, then everything you did at crtc_mode_set will be undone, and it won't be redone at crtc_enable. When you reenable the power well, the registers go back to their default values, not the values that were previously there. Did you check if VLV behaves the same? > } > > static void i9xx_crtc_off(struct drm_crtc *crtc) > @@ -4134,6 +4139,11 @@ static void intel_connector_check_state(struct intel_connector *connector) > * consider. */ > void intel_connector_dpms(struct drm_connector *connector, int mode) > { > + struct intel_crtc *intel_crtc = to_intel_crtc(connector->encoder->crtc); > + enum intel_display_power_domain domain; > + > + domain = POWER_DOMAIN_PIPE(intel_crtc->pipe); > + > /* All the simple cases only support two dpms states. */ > if (mode != DRM_MODE_DPMS_ON) > mode = DRM_MODE_DPMS_OFF; > @@ -4141,6 +4151,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode) > if (mode == connector->dpms) > return; > > + intel_display_power_get(connector->dev, domain); > connector->dpms = mode; > > /* Only need to change hw state when actually enabled */ > @@ -4148,6 +4159,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode) > intel_encoder_dpms(to_intel_encoder(connector->encoder), mode); > > intel_modeset_check_state(connector->dev); > + intel_display_power_put(connector->dev, domain); > } > > /* Simple connector->get_hw_state implementation for encoders that support only > @@ -9192,6 +9204,15 @@ static int __intel_set_mode(struct drm_crtc *crtc, > for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) > intel_crtc_disable(&intel_crtc->base); > > + /* > + * We take a ref here so the mode set will hit live hw. Once > + * we call the CRTC enable, we can drop our ref since it'll get > + * tracked there from then on. > + */ > + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) > + intel_display_power_get(dev, > + POWER_DOMAIN_PIPE(intel_crtc->pipe)); > + > for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { > if (intel_crtc->base.enabled) > dev_priv->display.crtc_disable(&intel_crtc->base); > @@ -9247,6 +9268,10 @@ done: > } > > out: > + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) > + intel_display_power_put(dev, > + POWER_DOMAIN_PIPE(intel_crtc->pipe)); > + > kfree(pipe_config); > kfree(saved_mode); > return ret; > @@ -10692,6 +10717,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > crtc->base.enabled = crtc->active; > > + if (crtc->active) > + intel_display_power_get(dev, > + POWER_DOMAIN_PIPE(crtc->pipe)); > + What about the panel fitter power domains? Sometimes the panel fitter is the thing that makes you require a power well, even though you're on a pipe that doesn't need it. And on Haswell you also have to take into account TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first doesn't need the power well but the second needs it. > + > DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", > crtc->base.base.id, > crtc->active ? "enabled" : "disabled"); > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 15 Oct 2013 16:54:00 -0300 Paulo Zanoni <przanoni@gmail.com> wrote: > 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>: > > When accessing the display regs for hw state readout or cross check, we > > need to make sure the power well is enabled so we can read valid > > register state. > > On the current code (HSW) we already check for the power wells in the > HW state readout code: if the power well is disabled, then transcoders > A/B/C are disabled. The current code takes care to not touch registers > on a disabled power well. Yeah and we should probably preserve that behavior, for the readout code at least. > > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); > > + > > Why is this here? It certainly deserves a comment in the code. It probably needs to be removed. The refcounting for the initial load is still screwy due to the unconditional enable later on. > So now HSW uses global_resources while VLV uses crtc enable/disable. I > really think both platforms should try to do the same thing. Definitely agreed. > By the way, at least on Haswell, if we do an equivalent change we'll > have problems, because when you disable the power well at > crtc_disable, then everything you did at crtc_mode_set will be undone, > and it won't be redone at crtc_enable. When you reenable the power > well, the registers go back to their default values, not the values > that were previously there. Did you check if VLV behaves the same? No that's taken into account here. In __intel_set_mode we take a private ref on the appropriate power well so that we'll preserve state until we do the first crtc_enable. From then on, the ref is tracked there and we drop the private one in __intel_set_mode > > + if (crtc->active) > > + intel_display_power_get(dev, > > + POWER_DOMAIN_PIPE(crtc->pipe)); > > + > > What about the panel fitter power domains? Sometimes the panel fitter > is the thing that makes you require a power well, even though you're > on a pipe that doesn't need it. > > And on Haswell you also have to take into account > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first > doesn't need the power well but the second needs it. Yeah I'm still not sure how to handle this in generic code. Maybe the power well mapping function Imre added will be enough, but it definitely gets tricky when we look at all the different platforms we have to (and will have to) handle. Thanks,
2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>: > On Tue, 15 Oct 2013 16:54:00 -0300 > Paulo Zanoni <przanoni@gmail.com> wrote: > >> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>: >> > When accessing the display regs for hw state readout or cross check, we >> > need to make sure the power well is enabled so we can read valid >> > register state. >> >> On the current code (HSW) we already check for the power wells in the >> HW state readout code: if the power well is disabled, then transcoders >> A/B/C are disabled. The current code takes care to not touch registers >> on a disabled power well. > > Yeah and we should probably preserve that behavior, for the readout > code at least. > >> > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); >> > + >> >> Why is this here? It certainly deserves a comment in the code. > > It probably needs to be removed. The refcounting for the initial load > is still screwy due to the unconditional enable later on. > >> So now HSW uses global_resources while VLV uses crtc enable/disable. I >> really think both platforms should try to do the same thing. > > Definitely agreed. > >> By the way, at least on Haswell, if we do an equivalent change we'll >> have problems, because when you disable the power well at >> crtc_disable, then everything you did at crtc_mode_set will be undone, >> and it won't be redone at crtc_enable. When you reenable the power >> well, the registers go back to their default values, not the values >> that were previously there. Did you check if VLV behaves the same? > > No that's taken into account here. In __intel_set_mode we take a > private ref on the appropriate power well so that we'll preserve state > until we do the first crtc_enable. From then on, the ref is tracked > there and we drop the private one in __intel_set_mode Yeah, but then after all this is done, at some point we'll call crtc_disable, then we'll put the last ref back and then the power well will be disabled. Then the user moves the mouse and we call crtc_enable, so we enable the power well, all its registers to back to their reset state, then crtc_enable sets some of the registers, but that won't really be enough since no one called crtc_mode_set again, and all the state set by crtc_mode_set (not touched by crtc_enable) is back to the default values. No? What am I missing? > >> > + if (crtc->active) >> > + intel_display_power_get(dev, >> > + POWER_DOMAIN_PIPE(crtc->pipe)); >> > + >> >> What about the panel fitter power domains? Sometimes the panel fitter >> is the thing that makes you require a power well, even though you're >> on a pipe that doesn't need it. >> >> And on Haswell you also have to take into account >> TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first >> doesn't need the power well but the second needs it. > > Yeah I'm still not sure how to handle this in generic code. Maybe the > power well mapping function Imre added will be enough, but it > definitely gets tricky when we look at all the different platforms we > have to (and will have to) handle. > > Thanks, > -- > Jesse Barnes, Intel Open Source Technology Center
On Tue, 15 Oct 2013 17:47:20 -0300 Paulo Zanoni <przanoni@gmail.com> wrote: > 2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>: > > On Tue, 15 Oct 2013 16:54:00 -0300 > > Paulo Zanoni <przanoni@gmail.com> wrote: > > > >> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>: > >> > When accessing the display regs for hw state readout or cross check, we > >> > need to make sure the power well is enabled so we can read valid > >> > register state. > >> > >> On the current code (HSW) we already check for the power wells in the > >> HW state readout code: if the power well is disabled, then transcoders > >> A/B/C are disabled. The current code takes care to not touch registers > >> on a disabled power well. > > > > Yeah and we should probably preserve that behavior, for the readout > > code at least. > > > >> > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); > >> > + > >> > >> Why is this here? It certainly deserves a comment in the code. > > > > It probably needs to be removed. The refcounting for the initial load > > is still screwy due to the unconditional enable later on. > > > >> So now HSW uses global_resources while VLV uses crtc enable/disable. I > >> really think both platforms should try to do the same thing. > > > > Definitely agreed. > > > >> By the way, at least on Haswell, if we do an equivalent change we'll > >> have problems, because when you disable the power well at > >> crtc_disable, then everything you did at crtc_mode_set will be undone, > >> and it won't be redone at crtc_enable. When you reenable the power > >> well, the registers go back to their default values, not the values > >> that were previously there. Did you check if VLV behaves the same? > > > > No that's taken into account here. In __intel_set_mode we take a > > private ref on the appropriate power well so that we'll preserve state > > until we do the first crtc_enable. From then on, the ref is tracked > > there and we drop the private one in __intel_set_mode > > Yeah, but then after all this is done, at some point we'll call > crtc_disable, then we'll put the last ref back and then the power well > will be disabled. Then the user moves the mouse and we call > crtc_enable, so we enable the power well, all its registers to back to > their reset state, then crtc_enable sets some of the registers, but > that won't really be enough since no one called crtc_mode_set again, > and all the state set by crtc_mode_set (not touched by crtc_enable) is > back to the default values. No? What am I missing? That's where the save/restore state of the later patch comes in. We need that if we're going to support DPMS and runtime suspend w/o a mode set.
2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>: > On Tue, 15 Oct 2013 17:47:20 -0300 > Paulo Zanoni <przanoni@gmail.com> wrote: > >> 2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>: >> > On Tue, 15 Oct 2013 16:54:00 -0300 >> > Paulo Zanoni <przanoni@gmail.com> wrote: >> > >> >> 2013/10/14 Jesse Barnes <jbarnes@virtuousgeek.org>: >> >> > When accessing the display regs for hw state readout or cross check, we >> >> > need to make sure the power well is enabled so we can read valid >> >> > register state. >> >> >> >> On the current code (HSW) we already check for the power wells in the >> >> HW state readout code: if the power well is disabled, then transcoders >> >> A/B/C are disabled. The current code takes care to not touch registers >> >> on a disabled power well. >> > >> > Yeah and we should probably preserve that behavior, for the readout >> > code at least. >> > >> >> > + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); >> >> > + >> >> >> >> Why is this here? It certainly deserves a comment in the code. >> > >> > It probably needs to be removed. The refcounting for the initial load >> > is still screwy due to the unconditional enable later on. >> > >> >> So now HSW uses global_resources while VLV uses crtc enable/disable. I >> >> really think both platforms should try to do the same thing. >> > >> > Definitely agreed. >> > >> >> By the way, at least on Haswell, if we do an equivalent change we'll >> >> have problems, because when you disable the power well at >> >> crtc_disable, then everything you did at crtc_mode_set will be undone, >> >> and it won't be redone at crtc_enable. When you reenable the power >> >> well, the registers go back to their default values, not the values >> >> that were previously there. Did you check if VLV behaves the same? >> > >> > No that's taken into account here. In __intel_set_mode we take a >> > private ref on the appropriate power well so that we'll preserve state >> > until we do the first crtc_enable. From then on, the ref is tracked >> > there and we drop the private one in __intel_set_mode >> >> Yeah, but then after all this is done, at some point we'll call >> crtc_disable, then we'll put the last ref back and then the power well >> will be disabled. Then the user moves the mouse and we call >> crtc_enable, so we enable the power well, all its registers to back to >> their reset state, then crtc_enable sets some of the registers, but >> that won't really be enough since no one called crtc_mode_set again, >> and all the state set by crtc_mode_set (not touched by crtc_enable) is >> back to the default values. No? What am I missing? > > That's where the save/restore state of the later patch comes in. We > need that if we're going to support DPMS and runtime suspend w/o a mode > set. Oh... I wasn't even thinking about it since it's on a later patch. But makes sense. But instead of the save/restore thing, shouldn't we just move all the code that touches registers from mode_set to crtc_enable? IMHO it's the shiny solution here. And anything else that we may need to save/restore should be moved to crtc enable/disable and be saved in "struct intel_crtc" every time it is touched. I really think that removing stuff from the save/restore code is the way to go. Also, with my suggestion, crtc_enable will fully implement the "mode set sequence" from BSpec, so people like the Audio guys will have an easier time understanding our code :) > > -- > Jesse Barnes, Intel Open Source Technology Center
On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote: > On Tue, 15 Oct 2013 16:54:00 -0300 > Paulo Zanoni <przanoni@gmail.com> wrote: > [...] > No that's taken into account here. In __intel_set_mode we take a > private ref on the appropriate power well so that we'll preserve state > until we do the first crtc_enable. From then on, the ref is tracked > there and we drop the private one in __intel_set_mode > > > > + if (crtc->active) > > > + intel_display_power_get(dev, > > > + POWER_DOMAIN_PIPE(crtc->pipe)); > > > + > > > > What about the panel fitter power domains? Sometimes the panel fitter > > is the thing that makes you require a power well, even though you're > > on a pipe that doesn't need it. > > > > And on Haswell you also have to take into account > > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first > > doesn't need the power well but the second needs it. > > Yeah I'm still not sure how to handle this in generic code. Maybe the > power well mapping function Imre added will be enough, but it > definitely gets tricky when we look at all the different platforms we > have to (and will have to) handle. Isn't the power domain abstraction a neat idea exactly for the above case? Generic code just asks for the domain it needs and doesn't care how it maps to power wells on the given platform. So for transcoder_edp +pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would enable the power well. You also have the POWER_DOMAIN_PIPE, POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already. --Imre
On Wed, 16 Oct 2013 14:10:13 +0300 Imre Deak <imre.deak@intel.com> wrote: > On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote: > > On Tue, 15 Oct 2013 16:54:00 -0300 > > Paulo Zanoni <przanoni@gmail.com> wrote: > > [...] > > No that's taken into account here. In __intel_set_mode we take a > > private ref on the appropriate power well so that we'll preserve state > > until we do the first crtc_enable. From then on, the ref is tracked > > there and we drop the private one in __intel_set_mode > > > > > > + if (crtc->active) > > > > + intel_display_power_get(dev, > > > > + POWER_DOMAIN_PIPE(crtc->pipe)); > > > > + > > > > > > What about the panel fitter power domains? Sometimes the panel fitter > > > is the thing that makes you require a power well, even though you're > > > on a pipe that doesn't need it. > > > > > > And on Haswell you also have to take into account > > > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first > > > doesn't need the power well but the second needs it. > > > > Yeah I'm still not sure how to handle this in generic code. Maybe the > > power well mapping function Imre added will be enough, but it > > definitely gets tricky when we look at all the different platforms we > > have to (and will have to) handle. > > Isn't the power domain abstraction a neat idea exactly for the above > case? Generic code just asks for the domain it needs and doesn't care > how it maps to power wells on the given platform. So for transcoder_edp > +pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and > POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the > other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would > enable the power well. You also have the POWER_DOMAIN_PIPE, > POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already. Yeah I think it can work. I missed your function that takes a crtc though as well, so we don't end up polluting the generic functions with TRANSCODER references that don't exist on the Atom platforms for example. That's the main thing I'm worried about, since as we get more and more wells I think it'll get easier to get it wrong in the generic code, if we have to use all the required domains for all platforms there.
On Wed, 2013-10-16 at 08:08 -0700, Jesse Barnes wrote: > On Wed, 16 Oct 2013 14:10:13 +0300 > Imre Deak <imre.deak@intel.com> wrote: > > > On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote: > > > On Tue, 15 Oct 2013 16:54:00 -0300 > > > Paulo Zanoni <przanoni@gmail.com> wrote: > > > [...] > > > No that's taken into account here. In __intel_set_mode we take a > > > private ref on the appropriate power well so that we'll preserve state > > > until we do the first crtc_enable. From then on, the ref is tracked > > > there and we drop the private one in __intel_set_mode > > > > > > > > + if (crtc->active) > > > > > + intel_display_power_get(dev, > > > > > + POWER_DOMAIN_PIPE(crtc->pipe)); > > > > > + > > > > > > > > What about the panel fitter power domains? Sometimes the panel fitter > > > > is the thing that makes you require a power well, even though you're > > > > on a pipe that doesn't need it. > > > > > > > > And on Haswell you also have to take into account > > > > TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first > > > > doesn't need the power well but the second needs it. > > > > > > Yeah I'm still not sure how to handle this in generic code. Maybe the > > > power well mapping function Imre added will be enough, but it > > > definitely gets tricky when we look at all the different platforms we > > > have to (and will have to) handle. > > > > Isn't the power domain abstraction a neat idea exactly for the above > > case? Generic code just asks for the domain it needs and doesn't care > > how it maps to power wells on the given platform. So for transcoder_edp > > +pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and > > POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the > > other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would > > enable the power well. You also have the POWER_DOMAIN_PIPE, > > POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already. > > Yeah I think it can work. I missed your function that takes a crtc > though as well, so we don't end up polluting the generic functions with > TRANSCODER references that don't exist on the Atom platforms for > example. That's the main thing I'm worried about, since as we get more > and more wells I think it'll get easier to get it wrong in the generic > code, if we have to use all the required domains for all platforms > there. Afaics, on VLV for example we'd ask for pipe A/B and transcoder A/B power domains, which is still correct. It's true that there the pipe-transcoder connection is fixed, and so we'll always ask for the same pipe/transcoder power domain pair, but I think it's still ok conceptually. So atm the power domains as defined are platform independent, which is great. If we can't avoid adding a platform specific ones in the future, we could still handle those in platform specific code. --Imre
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 313a8c9..91c3e6c 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1367,6 +1367,8 @@ static int i915_load_modeset_init(struct drm_device *dev) i915_disable_vga_mem(dev); intel_display_power_put(dev, POWER_DOMAIN_VGA); + intel_display_power_put(dev, POWER_DOMAIN_PIPE(0)); + /* Only enable hotplug handling once the fbdev is fully set up. */ dev_priv->enable_hotplug_processing = true; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6e4729b..62ee110 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3841,6 +3841,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc) if (intel_crtc->active) return; + intel_display_power_get(dev, POWER_DOMAIN_PIPE(pipe)); + intel_crtc->active = true; for_each_encoder_on_crtc(dev, crtc, encoder) @@ -3975,6 +3977,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc) intel_update_watermarks(crtc); intel_update_fbc(dev); + + if (IS_VALLEYVIEW(dev)) + intel_display_power_put(dev, POWER_DOMAIN_PIPE(pipe)); } static void i9xx_crtc_off(struct drm_crtc *crtc) @@ -4134,6 +4139,11 @@ static void intel_connector_check_state(struct intel_connector *connector) * consider. */ void intel_connector_dpms(struct drm_connector *connector, int mode) { + struct intel_crtc *intel_crtc = to_intel_crtc(connector->encoder->crtc); + enum intel_display_power_domain domain; + + domain = POWER_DOMAIN_PIPE(intel_crtc->pipe); + /* All the simple cases only support two dpms states. */ if (mode != DRM_MODE_DPMS_ON) mode = DRM_MODE_DPMS_OFF; @@ -4141,6 +4151,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode) if (mode == connector->dpms) return; + intel_display_power_get(connector->dev, domain); connector->dpms = mode; /* Only need to change hw state when actually enabled */ @@ -4148,6 +4159,7 @@ void intel_connector_dpms(struct drm_connector *connector, int mode) intel_encoder_dpms(to_intel_encoder(connector->encoder), mode); intel_modeset_check_state(connector->dev); + intel_display_power_put(connector->dev, domain); } /* Simple connector->get_hw_state implementation for encoders that support only @@ -9192,6 +9204,15 @@ static int __intel_set_mode(struct drm_crtc *crtc, for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) intel_crtc_disable(&intel_crtc->base); + /* + * We take a ref here so the mode set will hit live hw. Once + * we call the CRTC enable, we can drop our ref since it'll get + * tracked there from then on. + */ + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) + intel_display_power_get(dev, + POWER_DOMAIN_PIPE(intel_crtc->pipe)); + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) { if (intel_crtc->base.enabled) dev_priv->display.crtc_disable(&intel_crtc->base); @@ -9247,6 +9268,10 @@ done: } out: + for_each_intel_crtc_masked(dev, prepare_pipes, intel_crtc) + intel_display_power_put(dev, + POWER_DOMAIN_PIPE(intel_crtc->pipe)); + kfree(pipe_config); kfree(saved_mode); return ret; @@ -10692,6 +10717,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) crtc->base.enabled = crtc->active; + if (crtc->active) + intel_display_power_get(dev, + POWER_DOMAIN_PIPE(crtc->pipe)); + + DRM_DEBUG_KMS("[CRTC:%d] hw state readout: %s\n", crtc->base.base.id, crtc->active ? "enabled" : "disabled");
When accessing the display regs for hw state readout or cross check, we need to make sure the power well is enabled so we can read valid register state. Likewise, in an actual mode set, we need to take a ref on the appropriate power well so that the mode set succeeds. From then on, the power well ref will be tracked by the CRTC enable/disable code. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_dma.c | 2 ++ drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)