From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 09/19] drm/i915: check port power domain when reading the encoder hw state Date: Mon, 24 Feb 2014 14:53:11 +0200 Message-ID: <1393246391.13131.72.camel@intelbox> References: <1392674540-10915-1-git-send-email-imre.deak@intel.com> <1392674540-10915-10-git-send-email-imre.deak@intel.com> <20140220113620.72cae64f@jbarnes-desktop> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0084874496==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 4D57BFAA32 for ; Mon, 24 Feb 2014 04:53:37 -0800 (PST) In-Reply-To: <20140220113620.72cae64f@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0084874496== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-wU1sqYbKHFppjt7g9B5o" --=-wU1sqYbKHFppjt7g9B5o Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2014-02-20 at 11:36 -0800, Jesse Barnes wrote: > On Tue, 18 Feb 2014 00:02:10 +0200 > Imre Deak wrote: >=20 > > 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. > >=20 > > Signed-off-by: Imre Deak > > --- > > 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(-) > >=20 > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/in= tel_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 inte= l_connector *intel_connector) > > enum transcoder cpu_transcoder; > > uint32_t tmp; > > =20 > > - if (!intel_encoder->get_hw_state(intel_encoder, &pipe)) > > + if (!intel_encoder_get_hw_state(intel_encoder, &pipe)) > > return false; > > =20 > > if (port =3D=3D PORT_A) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/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 in= tel_connector *connector) > > WARN(!encoder->connectors_active, > > "encoder->connectors_active not set\n"); > > =20 > > - encoder_enabled =3D encoder->get_hw_state(encoder, &pipe); > > + encoder_enabled =3D 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_co= nnector *connector) > > enum pipe pipe =3D 0; > > struct intel_encoder *encoder =3D connector->encoder; > > =20 > > - return encoder->get_hw_state(encoder, &pipe); > > + return intel_encoder_get_hw_state(encoder, &pipe); > > } > > =20 > > static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe= pipe, > > @@ -9464,6 +9464,19 @@ check_connector_state(struct drm_device *dev) > > } > > } > > =20 > > +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 =3D intel_encoder->base.dev->dev_pr= ivate; > > + > > + power_domain =3D 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 s= tate " > > "(expected %i, found %i)\n", active, encoder->connectors_active= ); > > =20 > > - active =3D encoder->get_hw_state(encoder, &pipe); > > + active =3D intel_encoder_get_hw_state(encoder, &pipe); > > WARN(active !=3D 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 !=3D &crtc->base) > > continue; > > - if (encoder->get_hw_state(encoder, &pipe)) > > + if (intel_encoder_get_hw_state(encoder, &pipe)) > > encoder->get_config(encoder, &pipe_config); > > } > > =20 > > @@ -11350,7 +11363,7 @@ static void intel_modeset_readout_hw_state(stru= ct drm_device *dev) > > base.head) { > > pipe =3D 0; > > =20 > > - if (encoder->get_hw_state(encoder, &pipe)) { > > + if (intel_encoder_get_hw_state(encoder, &pipe)) { > > crtc =3D to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > encoder->base.crtc =3D &crtc->base; > > encoder->get_config(encoder, &crtc->config); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_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_encode= r *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 pi= pe *pipe); > > =20 > > /* intel_dp.c */ > > void intel_dp_init(struct drm_device *dev, int output_reg, enum port p= ort); >=20 > 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 --=-wU1sqYbKHFppjt7g9B5o Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJTC0C3AAoJEORIIAnNuWDFGT4H/0NloVW9m+B4zuvinOE4DmSF tImxzshDhScxT2CphARfhplJfN+5PgLBpM0zNRsCzMDNMzMeMF9q7I/sw5Hhd0Al 6J9ldUWq7wh4tj5vEcxtMdhdQtW0uNrDUtviU89lVXyw/G4mQIjJP2P1/9z2JQug f6FvnrvaLXYJFi3Up7vxd7FUzMEUxteoodzZZOs8RCLPLGNpqziU09THHYIDFHV0 KFN6hZYYRcApGEf0RDkkV+JV9c2J9qXdADbdUnLvhnCYGeu5kYCPGpKdefUz5+4T yu7OD32cZ3QwUNZxJECh07BhgXF6A3AhW4YP68W+31Mvp+lFX3VtF8A3uP3j1ck= =zXW5 -----END PGP SIGNATURE----- --=-wU1sqYbKHFppjt7g9B5o-- --===============0084874496== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0084874496==--