Quoting Imre Deak (2019-04-07 13:46:55)
> Push getting the reference for the encoders' power domains into the
> encoder get_power_domains() hook instead of doing this from the caller.
> This way the encoder can store away the corresponding wakerefs.
>
> This fixes the DSI encoder disabling, which didn't release these
> power references it acquired during HW state readout.
>
> Note that longtime ownership for the corresponding wakerefs can be thus
> acquired / released in two ways. Nevertheless there is always only one
> owner for them:
>
> After HW readout (booting/system resume):
> - encoder->get_power_domains() acquires
> - encoder->disable*() releases
>
> After a modeset (calling intel_atomic_commit()):
> - encoder->enable*() acquires
> - encoder->disable*() releases
>
> * can be any of the encoder enable/disable hooks.
>
> v2:
> - Check that the DSI io_wakerefs are unset both during encoder HW
> readout and enabling. (Chris)
>
> Fixes: 0e6e0be4c9523 ("drm/i915: Markup paired operations on display power domains")
> Cc: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/icl_dsi.c | 40 ++++++++++++++++++------------------
> drivers/gpu/drm/i915/intel_ddi.c | 17 ++++++++-------
> drivers/gpu/drm/i915/intel_display.c | 6 +-----
> drivers/gpu/drm/i915/intel_drv.h | 10 +++++----
> 4 files changed, 35 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> index b67ffaa283dc..586cf136546f 100644
> --- a/drivers/gpu/drm/i915/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/icl_dsi.c
> @@ -323,6 +323,21 @@ static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
> }
> }
>
> +static void get_dsi_io_power_domains(struct drm_i915_private *dev_priv,
> + struct intel_dsi *intel_dsi)
> +{
> + enum port port;
> +
> + for_each_dsi_port(port, intel_dsi->ports) {
> + WARN_ON(intel_dsi->io_wakeref[port]);
> + intel_dsi->io_wakeref[port] =
> + intel_display_power_get(dev_priv,
> + port == PORT_A ?
> + POWER_DOMAIN_PORT_DDI_A_IO :
> + POWER_DOMAIN_PORT_DDI_B_IO);
> + }
Ok, that looks much more convincing that get_power_domain is just a
temporary reference. The warn will make sure we don't loose track of an
old wakeref by overwriting it with a new one.
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Only need to worry about the other the encoders now :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
