Re: [PATCH v2 1/2] drm/i915: Get power refs in encoder->get_power_domains()

[Chris Wilson <chris@chris-wilson.co.uk>]

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@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  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@chris-wilson.co.uk>

Only need to worry about the other the encoders now :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx