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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
