| Message ID | 1392674540-10915-11-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Feb 2014 00:02:11 +0200 Imre Deak <imre.deak@intel.com> wrote: > We can read out the pipe HW state only if the required power domain is > on. If not we consider the pipe to be off. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index ce1c00a..e3824f8 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9535,6 +9535,18 @@ check_encoder_state(struct drm_device *dev) > } > } > > +static bool intel_display_get_pipe_config(struct intel_crtc *crtc, > + struct intel_crtc_config *pipe_config) > +{ > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > + > + if (!intel_display_power_enabled(dev_priv, > + POWER_DOMAIN_PIPE(crtc->pipe))) > + return false; > + > + return dev_priv->display.get_pipe_config(crtc, pipe_config); > +} > + > static void > check_crtc_state(struct drm_device *dev) > { > @@ -9572,8 +9584,7 @@ check_crtc_state(struct drm_device *dev) > "crtc's computed enabled state doesn't match tracked enabled state " > "(expected %i, found %i)\n", enabled, crtc->base.enabled); > > - active = dev_priv->display.get_pipe_config(crtc, > - &pipe_config); > + active = intel_display_get_pipe_config(crtc, &pipe_config); > > /* hw state is inconsistent with the pipe A quirk */ > if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) > @@ -11328,8 +11339,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > base.head) { > memset(&crtc->config, 0, sizeof(crtc->config)); > > - crtc->active = dev_priv->display.get_pipe_config(crtc, > - &crtc->config); > + crtc->active = intel_display_get_pipe_config(crtc, > + &crtc->config); > > crtc->base.enabled = crtc->active; > crtc->primary_enabled = crtc->active; Same comment about renaming .get_pipe_config applies here too, but I think you got them all in this case, so that could be done on top. Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
On Thu, Feb 20, 2014 at 11:37:25AM -0800, Jesse Barnes wrote: > On Tue, 18 Feb 2014 00:02:11 +0200 > Imre Deak <imre.deak@intel.com> wrote: > > > We can read out the pipe HW state only if the required power domain is > > on. If not we consider the pipe to be off. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++---- > > 1 file changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index ce1c00a..e3824f8 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9535,6 +9535,18 @@ check_encoder_state(struct drm_device *dev) > > } > > } > > > > +static bool intel_display_get_pipe_config(struct intel_crtc *crtc, > > + struct intel_crtc_config *pipe_config) > > +{ > > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > > + > > + if (!intel_display_power_enabled(dev_priv, > > + POWER_DOMAIN_PIPE(crtc->pipe))) > > + return false; > > + > > + return dev_priv->display.get_pipe_config(crtc, pipe_config); > > +} > > + > > static void > > check_crtc_state(struct drm_device *dev) > > { > > @@ -9572,8 +9584,7 @@ check_crtc_state(struct drm_device *dev) > > "crtc's computed enabled state doesn't match tracked enabled state " > > "(expected %i, found %i)\n", enabled, crtc->base.enabled); > > > > - active = dev_priv->display.get_pipe_config(crtc, > > - &pipe_config); > > + active = intel_display_get_pipe_config(crtc, &pipe_config); > > > > /* hw state is inconsistent with the pipe A quirk */ > > if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) > > @@ -11328,8 +11339,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > base.head) { > > memset(&crtc->config, 0, sizeof(crtc->config)); > > > > - crtc->active = dev_priv->display.get_pipe_config(crtc, > > - &crtc->config); > > + crtc->active = intel_display_get_pipe_config(crtc, > > + &crtc->config); > > > > crtc->base.enabled = crtc->active; > > crtc->primary_enabled = crtc->active; > > Same comment about renaming .get_pipe_config applies here too, but I > think you got them all in this case, so that could be done on top. > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> Same comment here - imo this should be pushed down into callbacks, since they are the only ones precisely aware of which power domains are needed. And by pushing it down we can group the power domain checks closely together with the register readback code, e.g. for fine-grained stuff like pfit or special plls. And we already have some of these checks in the hsw get_pipe_config function. So nack on this approach. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ce1c00a..e3824f8 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9535,6 +9535,18 @@ check_encoder_state(struct drm_device *dev) } } +static bool intel_display_get_pipe_config(struct intel_crtc *crtc, + struct intel_crtc_config *pipe_config) +{ + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + + if (!intel_display_power_enabled(dev_priv, + POWER_DOMAIN_PIPE(crtc->pipe))) + return false; + + return dev_priv->display.get_pipe_config(crtc, pipe_config); +} + static void check_crtc_state(struct drm_device *dev) { @@ -9572,8 +9584,7 @@ check_crtc_state(struct drm_device *dev) "crtc's computed enabled state doesn't match tracked enabled state " "(expected %i, found %i)\n", enabled, crtc->base.enabled); - active = dev_priv->display.get_pipe_config(crtc, - &pipe_config); + active = intel_display_get_pipe_config(crtc, &pipe_config); /* hw state is inconsistent with the pipe A quirk */ if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) @@ -11328,8 +11339,8 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) base.head) { memset(&crtc->config, 0, sizeof(crtc->config)); - crtc->active = dev_priv->display.get_pipe_config(crtc, - &crtc->config); + crtc->active = intel_display_get_pipe_config(crtc, + &crtc->config); crtc->base.enabled = crtc->active; crtc->primary_enabled = crtc->active;
We can read out the pipe HW state only if the required power domain is on. If not we consider the pipe to be off. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)