On Thu, Jul 05, 2018 at 03:26:54PM +0300, Imre Deak wrote:
> We can simplify the encoder's get_power_domains() hook by calling it
> only if the encoder is active. That way the hook can return its power
> domains unconditionally without checking the active state by calling
> encoder::get_hw_state(). This get_hw_state() query is in fact
> redundant since it's already done by intel_modeset_readout_hw_state()
> setting the encoder's crtc or leaving it NULL accordingly. Let's use
> this fact to decide if the encoder is active.
>
> While at it clarify the comment in intel_ddi_get_power_domains() about
> primary vs. fake MST encoders and make sure we never do an incorrect
> encoder->dig_port cast for fake MST encoders.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 19 +++++++------------
> drivers/gpu/drm/i915/intel_display.c | 11 +++++------
> 2 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0319825b725b..6b74eee5a3ac 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2005,24 +2005,19 @@ intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
> struct intel_crtc_state *crtc_state)
> {
> - struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> - enum pipe pipe;
> + struct intel_digital_port *dig_port;
> u64 domains;
>
> - if (!intel_ddi_get_hw_state(encoder, &pipe))
> - return 0;
> -
> - domains = BIT_ULL(dig_port->ddi_io_power_domain);
> - if (!crtc_state)
> - return domains;
> -
> /*
> * TODO: Add support for MST encoders. Atm, the following should never
> - * happen since this function will be called only for the primary
> - * encoder which doesn't have its own pipe/crtc_state.
> + * happen since fake-MST encoders don't set their get_power_domains()
> + * hook.
> */
> if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST)))
> - return domains;
> + return 0;
> +
> + dig_port = enc_to_dig_port(&encoder->base);
> + domains = BIT_ULL(dig_port->ddi_io_power_domain);
>
> /* AUX power is only needed for (e)DP mode, not for HDMI. */
> if (intel_crtc_has_dp_encoder(crtc_state)) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 681e0710a467..a61dbdf2f612 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15705,14 +15705,13 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv)
> continue;
>
> /*
> - * For MST and inactive encoders we don't have a crtc state.
> - * FIXME: no need to call get_power_domains in such cases, it
> - * will always return 0.
> + * MST-primary and inactive encoders don't have a crtc state
> + * and neither of these require any power domain references.
> */
> - crtc_state = encoder->base.crtc ?
> - to_intel_crtc_state(encoder->base.crtc->state) :
> - NULL;
> + if (!encoder->base.crtc)
> + continue;
>
> + crtc_state = to_intel_crtc_state(encoder->base.crtc->state);
> get_domains = encoder->get_power_domains(encoder, crtc_state);
> for_each_power_domain(domain, get_domains)
> intel_display_power_get(dev_priv, domain);
> --
> 2.13.2
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
