On Wed, Mar 08, 2023 at 11:58:58AM -0500, Rodrigo Vivi wrote:
> uncore->lock only protects the forcewake domain itself,
> not the register accesses.
>
> uncore's _fw alternatives are for cases where the domains
> are not needed because we are sure that they are already
> awake.
>
> So the move towards the uncore's _fw alternatives seems
> right, however using the uncore-lock to protect the dsparb
> registers seems an abuse of the uncore-lock.
>
> Let's restore the previous individual lock and try to get
> rid of the direct uncore accesses from the display code.
>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/i9xx_wm.c | 13 ++-----------
> drivers/gpu/drm/i915/display/intel_display_core.h | 3 +++
> drivers/gpu/drm/i915/i915_driver.c | 1 +
> 3 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/i9xx_wm.c b/drivers/gpu/drm/i915/display/i9xx_wm.c
> index caef72d38798..8fe0b5c63d3a 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_wm.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_wm.c
> @@ -1771,16 +1771,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>
> trace_vlv_fifo_size(crtc, sprite0_start, sprite1_start, fifo_size);
>
> - /*
> - * uncore.lock serves a double purpose here. It allows us to
> - * use the less expensive I915_{READ,WRITE}_FW() functions, and
> - * it protects the DSPARB registers from getting clobbered by
> - * parallel updates from multiple pipes.
> - *
> - * intel_pipe_update_start() has already disabled interrupts
> - * for us, so a plain spin_lock() is sufficient here.
> - */
I was wondering if we need to preserve the comment about irqs,
but since this is the only place using this lock, and it's never
called from an irq handler a non-irq disabling spinlock will suffice
anyway.
Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> - spin_lock(&uncore->lock);
> + spin_lock(&dev_priv->display.wm.dsparb_lock);
>
> switch (crtc->pipe) {
> case PIPE_A:
> @@ -1840,7 +1831,7 @@ static void vlv_atomic_update_fifo(struct intel_atomic_state *state,
>
> intel_uncore_posting_read_fw(uncore, DSPARB);
>
> - spin_unlock(&uncore->lock);
> + spin_unlock(&dev_priv->display.wm.dsparb_lock);
> }
>
> #undef VLV_FIFO
> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> index fdab7bb93a7d..68c6bfb91dbe 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> @@ -253,6 +253,9 @@ struct intel_wm {
> */
> struct mutex wm_mutex;
>
> + /* protects DSPARB registers on pre-g4x/vlv/chv */
> + spinlock_t dsparb_lock;
> +
> bool ipc_enabled;
> };
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index a53fd339e2cc..c78e36444a12 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -223,6 +223,7 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
> mutex_init(&dev_priv->display.pps.mutex);
> mutex_init(&dev_priv->display.hdcp.comp_mutex);
> spin_lock_init(&dev_priv->display.dkl.phy_lock);
> + spin_lock_init(&dev_priv->display.wm.dsparb_lock);
>
> i915_memcpy_init_early(dev_priv);
> intel_runtime_pm_init_early(&dev_priv->runtime_pm);
> --
> 2.39.2
--
Ville Syrjälä
Intel
