| Message ID | 1392674540-10915-10-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 18 Feb 2014 00:02:10 +0200 Imre Deak <imre.deak@intel.com> wrote: > Since the encoder is tied to its port, we need to make sure the power > domain for that port is on before reading out the encoder HW state. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++----- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 2643d3b..f95bc3a 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1110,7 +1110,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) > enum transcoder cpu_transcoder; > uint32_t tmp; > > - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) > + if (!intel_encoder_get_hw_state(intel_encoder, &pipe)) > return false; > > if (port == PORT_A) > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 7ef06fa..ce1c00a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4519,7 +4519,7 @@ static void intel_connector_check_state(struct intel_connector *connector) > WARN(!encoder->connectors_active, > "encoder->connectors_active not set\n"); > > - encoder_enabled = encoder->get_hw_state(encoder, &pipe); > + encoder_enabled = intel_encoder_get_hw_state(encoder, &pipe); > WARN(!encoder_enabled, "encoder not enabled\n"); > if (WARN_ON(!encoder->base.crtc)) > return; > @@ -4561,7 +4561,7 @@ bool intel_connector_get_hw_state(struct intel_connector *connector) > enum pipe pipe = 0; > struct intel_encoder *encoder = connector->encoder; > > - return encoder->get_hw_state(encoder, &pipe); > + return intel_encoder_get_hw_state(encoder, &pipe); > } > > static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > @@ -9464,6 +9464,19 @@ check_connector_state(struct drm_device *dev) > } > } > > +bool intel_encoder_get_hw_state(struct intel_encoder *intel_encoder, > + enum pipe *pipe) > +{ > + enum intel_display_power_domain power_domain; > + struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; > + > + power_domain = intel_display_port_power_domain(intel_encoder); > + if (!intel_display_power_enabled(dev_priv, power_domain)) > + return false; > + > + return intel_encoder->get_hw_state(intel_encoder, pipe); > +} > + > static void > check_encoder_state(struct drm_device *dev) > { > @@ -9504,7 +9517,7 @@ check_encoder_state(struct drm_device *dev) > "encoder's computed active state doesn't match tracked active state " > "(expected %i, found %i)\n", active, encoder->connectors_active); > > - active = encoder->get_hw_state(encoder, &pipe); > + active = intel_encoder_get_hw_state(encoder, &pipe); > WARN(active != encoder->connectors_active, > "encoder's hw state doesn't match sw tracking " > "(expected %i, found %i)\n", > @@ -9571,7 +9584,7 @@ check_crtc_state(struct drm_device *dev) > enum pipe pipe; > if (encoder->base.crtc != &crtc->base) > continue; > - if (encoder->get_hw_state(encoder, &pipe)) > + if (intel_encoder_get_hw_state(encoder, &pipe)) > encoder->get_config(encoder, &pipe_config); > } > > @@ -11350,7 +11363,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > base.head) { > pipe = 0; > > - if (encoder->get_hw_state(encoder, &pipe)) { > + if (intel_encoder_get_hw_state(encoder, &pipe)) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > encoder->base.crtc = &crtc->base; > encoder->get_config(encoder, &crtc->config); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index e31eb1e..afc01a4 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -738,6 +738,7 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder); > int valleyview_get_vco(struct drm_i915_private *dev_priv); > void intel_mode_from_pipe_config(struct drm_display_mode *mode, > struct intel_crtc_config *pipe_config); > +bool intel_encoder_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); > > /* intel_dp.c */ > void intel_dp_init(struct drm_device *dev, int output_reg, enum port port); I think there are a few more places to switch over, e.g. connector_check_state and connector_get_hw_state? Maybe we should rename the fn pointer field to include a __ in the front so the compiler will catch them all and we'll know they're not to be called directly.
On Thu, 2014-02-20 at 11:36 -0800, Jesse Barnes wrote: > On Tue, 18 Feb 2014 00:02:10 +0200 > Imre Deak <imre.deak@intel.com> wrote: > > > Since the encoder is tied to its port, we need to make sure the power > > domain for that port is on before reading out the encoder HW state. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 2 +- > > drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++----- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 3 files changed, 20 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 2643d3b..f95bc3a 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1110,7 +1110,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) > > enum transcoder cpu_transcoder; > > uint32_t tmp; > > > > - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) > > + if (!intel_encoder_get_hw_state(intel_encoder, &pipe)) > > return false; > > > > if (port == PORT_A) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 7ef06fa..ce1c00a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4519,7 +4519,7 @@ static void intel_connector_check_state(struct intel_connector *connector) > > WARN(!encoder->connectors_active, > > "encoder->connectors_active not set\n"); > > > > - encoder_enabled = encoder->get_hw_state(encoder, &pipe); > > + encoder_enabled = intel_encoder_get_hw_state(encoder, &pipe); > > WARN(!encoder_enabled, "encoder not enabled\n"); > > if (WARN_ON(!encoder->base.crtc)) > > return; > > @@ -4561,7 +4561,7 @@ bool intel_connector_get_hw_state(struct intel_connector *connector) > > enum pipe pipe = 0; > > struct intel_encoder *encoder = connector->encoder; > > > > - return encoder->get_hw_state(encoder, &pipe); > > + return intel_encoder_get_hw_state(encoder, &pipe); > > } > > > > static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, > > @@ -9464,6 +9464,19 @@ check_connector_state(struct drm_device *dev) > > } > > } > > > > +bool intel_encoder_get_hw_state(struct intel_encoder *intel_encoder, > > + enum pipe *pipe) > > +{ > > + enum intel_display_power_domain power_domain; > > + struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; > > + > > + power_domain = intel_display_port_power_domain(intel_encoder); > > + if (!intel_display_power_enabled(dev_priv, power_domain)) > > + return false; > > + > > + return intel_encoder->get_hw_state(intel_encoder, pipe); > > +} > > + > > static void > > check_encoder_state(struct drm_device *dev) > > { > > @@ -9504,7 +9517,7 @@ check_encoder_state(struct drm_device *dev) > > "encoder's computed active state doesn't match tracked active state " > > "(expected %i, found %i)\n", active, encoder->connectors_active); > > > > - active = encoder->get_hw_state(encoder, &pipe); > > + active = intel_encoder_get_hw_state(encoder, &pipe); > > WARN(active != encoder->connectors_active, > > "encoder's hw state doesn't match sw tracking " > > "(expected %i, found %i)\n", > > @@ -9571,7 +9584,7 @@ check_crtc_state(struct drm_device *dev) > > enum pipe pipe; > > if (encoder->base.crtc != &crtc->base) > > continue; > > - if (encoder->get_hw_state(encoder, &pipe)) > > + if (intel_encoder_get_hw_state(encoder, &pipe)) > > encoder->get_config(encoder, &pipe_config); > > } > > > > @@ -11350,7 +11363,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > base.head) { > > pipe = 0; > > > > - if (encoder->get_hw_state(encoder, &pipe)) { > > + if (intel_encoder_get_hw_state(encoder, &pipe)) { > > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > encoder->base.crtc = &crtc->base; > > encoder->get_config(encoder, &crtc->config); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index e31eb1e..afc01a4 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -738,6 +738,7 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder); > > int valleyview_get_vco(struct drm_i915_private *dev_priv); > > void intel_mode_from_pipe_config(struct drm_display_mode *mode, > > struct intel_crtc_config *pipe_config); > > +bool intel_encoder_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); > > > > /* intel_dp.c */ > > void intel_dp_init(struct drm_device *dev, int output_reg, enum port port); > > I think there are a few more places to switch over, e.g. > connector_check_state and connector_get_hw_state? I haven't added a high level power domain check for those, because they only call the encoder check-state and get-hw-state handler internally, and those have power domain checks in place already. I also postponed adding power domain checks for the PLL HW readout/state checking code, since the development of that part is still in a bit of a flux. But atm, it's only relevant on PCH platforms where we don't need to care about power wells. So I'd add the checks for this when things have settled. > Maybe we should > rename the fn pointer field to include a __ in the front so the > compiler will catch them all and we'll know they're not to be called > directly. Hm, that'd mean some extra churn. I agree with your argument about the compile time check it's a very nice guarantee for reviewers. I don't have a strong opinion about the second argument. Some existing handlers are also not ok to call in some contexts, so to keep things unified we'd also have to prefix those. For now I'd just punt on this .. --Imre
On Tue, Feb 18, 2014 at 12:02:10AM +0200, Imre Deak wrote: > Since the encoder is tied to its port, we need to make sure the power > domain for that port is on before reading out the encoder HW state. > > Signed-off-by: Imre Deak <imre.deak@intel.com> > +bool intel_encoder_get_hw_state(struct intel_encoder *intel_encoder, > + enum pipe *pipe) > +{ > + enum intel_display_power_domain power_domain; > + struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; > + > + power_domain = intel_display_port_power_domain(intel_encoder); > + if (!intel_display_power_enabled(dev_priv, power_domain)) > + return false; > + > + return intel_encoder->get_hw_state(intel_encoder, pipe); > +} Nope, these kinds of checks should be pushed down into the encoder/platform specific callbacks imo. Like I've said in my previous reply I'm not a fan of the port_power_domain function, knowledge about this should be pushed out from generic code into encoder callbacks. hw engineers are create enough imo that this will hurt us in the end if we don't have full flexibility in the encoder specific code. Same goes with any other relevant power well checks, imo they should be as close to the actual hw readout code as possible (e.g. pfit, pll, ...). For example see the platform specific power well check in haswell_get_pipe_config. So please replaced this patch with one which sprinkles the relevant checks all over the place. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 2643d3b..f95bc3a 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1110,7 +1110,7 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) enum transcoder cpu_transcoder; uint32_t tmp; - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) + if (!intel_encoder_get_hw_state(intel_encoder, &pipe)) return false; if (port == PORT_A) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7ef06fa..ce1c00a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4519,7 +4519,7 @@ static void intel_connector_check_state(struct intel_connector *connector) WARN(!encoder->connectors_active, "encoder->connectors_active not set\n"); - encoder_enabled = encoder->get_hw_state(encoder, &pipe); + encoder_enabled = intel_encoder_get_hw_state(encoder, &pipe); WARN(!encoder_enabled, "encoder not enabled\n"); if (WARN_ON(!encoder->base.crtc)) return; @@ -4561,7 +4561,7 @@ bool intel_connector_get_hw_state(struct intel_connector *connector) enum pipe pipe = 0; struct intel_encoder *encoder = connector->encoder; - return encoder->get_hw_state(encoder, &pipe); + return intel_encoder_get_hw_state(encoder, &pipe); } static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe, @@ -9464,6 +9464,19 @@ check_connector_state(struct drm_device *dev) } } +bool intel_encoder_get_hw_state(struct intel_encoder *intel_encoder, + enum pipe *pipe) +{ + enum intel_display_power_domain power_domain; + struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; + + power_domain = intel_display_port_power_domain(intel_encoder); + if (!intel_display_power_enabled(dev_priv, power_domain)) + return false; + + return intel_encoder->get_hw_state(intel_encoder, pipe); +} + static void check_encoder_state(struct drm_device *dev) { @@ -9504,7 +9517,7 @@ check_encoder_state(struct drm_device *dev) "encoder's computed active state doesn't match tracked active state " "(expected %i, found %i)\n", active, encoder->connectors_active); - active = encoder->get_hw_state(encoder, &pipe); + active = intel_encoder_get_hw_state(encoder, &pipe); WARN(active != encoder->connectors_active, "encoder's hw state doesn't match sw tracking " "(expected %i, found %i)\n", @@ -9571,7 +9584,7 @@ check_crtc_state(struct drm_device *dev) enum pipe pipe; if (encoder->base.crtc != &crtc->base) continue; - if (encoder->get_hw_state(encoder, &pipe)) + if (intel_encoder_get_hw_state(encoder, &pipe)) encoder->get_config(encoder, &pipe_config); } @@ -11350,7 +11363,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) base.head) { pipe = 0; - if (encoder->get_hw_state(encoder, &pipe)) { + if (intel_encoder_get_hw_state(encoder, &pipe)) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); encoder->base.crtc = &crtc->base; encoder->get_config(encoder, &crtc->config); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index e31eb1e..afc01a4 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -738,6 +738,7 @@ intel_display_port_power_domain(struct intel_encoder *intel_encoder); int valleyview_get_vco(struct drm_i915_private *dev_priv); void intel_mode_from_pipe_config(struct drm_display_mode *mode, struct intel_crtc_config *pipe_config); +bool intel_encoder_get_hw_state(struct intel_encoder *encoder, enum pipe *pipe); /* intel_dp.c */ void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
Since the encoder is tied to its port, we need to make sure the power domain for that port is on before reading out the encoder HW state. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 23 ++++++++++++++++++----- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 20 insertions(+), 6 deletions(-)