Re: [PATCH 10/36] drm/i915: Replace pcu_lock with sb_lock

From: Sagar Arun Kamble <sagar.a.kamble@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Cc: praveen.paneri@intel.com
Subject: Re: [PATCH 10/36] drm/i915: Replace pcu_lock with sb_lock
Date: Thu, 15 Mar 2018 17:36:57 +0530	[thread overview]
Message-ID: <9ed93813-636c-3a0b-fe7c-a1392289468e@intel.com> (raw)
In-Reply-To: <20180314093748.8541-10-chris@chris-wilson.co.uk>


On 3/14/2018 3:07 PM, Chris Wilson wrote:
> We now have two locks for sideband access. The general one covering
> sideband access across all generation, sb_lock, and a specific one
> covering sideband access via the punit on vlv/chv. After lifting the
> sb_lock around the punit into the callers, the pcu_lock is now redudant
> and can be separated from its other use to regulate RPS (essentially
> giving RPS a lock all of its own).
>
> v2: Extract a couple of minor bug fixes.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c     |  47 ++++--------
>   drivers/gpu/drm/i915/i915_drv.h         |  10 +--
>   drivers/gpu/drm/i915/i915_irq.c         |   4 +-
>   drivers/gpu/drm/i915/i915_sysfs.c       |  32 +++-----
>   drivers/gpu/drm/i915/intel_cdclk.c      |  28 -------
>   drivers/gpu/drm/i915/intel_display.c    |   6 --
>   drivers/gpu/drm/i915/intel_hdcp.c       |   2 -
>   drivers/gpu/drm/i915/intel_pm.c         | 127 +++++++++++++++-----------------
>   drivers/gpu/drm/i915/intel_runtime_pm.c |   8 --
>   drivers/gpu/drm/i915/intel_sideband.c   |   4 -
>   10 files changed, 93 insertions(+), 175 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ebce80f29087..0db75e8ce494 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1074,8 +1074,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>   	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>   		u32 rpmodectl, freq_sts;
>   
> -		mutex_lock(&dev_priv->pcu_lock);
> -
>   		rpmodectl = I915_READ(GEN6_RP_CONTROL);
>   		seq_printf(m, "Video Turbo Mode: %s\n",
>   			   yesno(rpmodectl & GEN6_RP_MEDIA_TURBO));
> @@ -1110,7 +1108,6 @@ static int i915_frequency_info(struct seq_file *m, void *unused)
>   		seq_printf(m,
>   			   "efficient (RPe) frequency: %d MHz\n",
>   			   intel_gpu_freq(dev_priv, rps->efficient_freq));
> -		mutex_unlock(&dev_priv->pcu_lock);
>   	} else if (INTEL_GEN(dev_priv) >= 6) {
>   		u32 rp_state_limits;
>   		u32 gt_perf_status;
> @@ -1525,12 +1522,9 @@ static int gen6_drpc_info(struct seq_file *m)
>   		gen9_powergate_status = I915_READ(GEN9_PWRGT_DOMAIN_STATUS);
>   	}
>   
> -	if (INTEL_GEN(dev_priv) <= 7) {
> -		mutex_lock(&dev_priv->pcu_lock);
> +	if (INTEL_GEN(dev_priv) <= 7)
>   		sandybridge_pcode_read(dev_priv, GEN6_PCODE_READ_RC6VIDS,
>   				       &rc6vids);
> -		mutex_unlock(&dev_priv->pcu_lock);
> -	}
>   
>   	seq_printf(m, "RC1e Enabled: %s\n",
>   		   yesno(rcctl1 & GEN6_RC_CTL_RC1e_ENABLE));
> @@ -1801,17 +1795,10 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>   	struct intel_rps *rps = &dev_priv->gt_pm.rps;
>   	unsigned int max_gpu_freq, min_gpu_freq;
>   	int gpu_freq, ia_freq;
> -	int ret;
>   
>   	if (!HAS_LLC(dev_priv))
>   		return -ENODEV;
>   
> -	intel_runtime_pm_get(dev_priv);
> -
> -	ret = mutex_lock_interruptible(&dev_priv->pcu_lock);
> -	if (ret)
> -		goto out;
> -
>   	min_gpu_freq = rps->min_freq;
>   	max_gpu_freq = rps->max_freq;
>   	if (IS_GEN9_BC(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> @@ -1822,6 +1809,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>   
>   	seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
>   
> +	intel_runtime_pm_get(dev_priv);
>   	for (gpu_freq = min_gpu_freq; gpu_freq <= max_gpu_freq; gpu_freq++) {
>   		ia_freq = gpu_freq;
>   		sandybridge_pcode_read(dev_priv,
> @@ -1835,12 +1823,9 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>   			   ((ia_freq >> 0) & 0xff) * 100,
>   			   ((ia_freq >> 8) & 0xff) * 100);
>   	}
> -
> -	mutex_unlock(&dev_priv->pcu_lock);
> -
> -out:
>   	intel_runtime_pm_put(dev_priv);
> -	return ret;
> +
> +	return 0;
>   }
>   
>   static int i915_opregion(struct seq_file *m, void *unused)
> @@ -4174,7 +4159,7 @@ i915_max_freq_set(void *data, u64 val)
>   
>   	DRM_DEBUG_DRIVER("Manually setting max freq to %llu\n", val);
>   
> -	ret = mutex_lock_interruptible(&dev_priv->pcu_lock);
> +	ret = mutex_lock_interruptible(&rps->lock);
>   	if (ret)
>   		return ret;
>   
> @@ -4187,8 +4172,8 @@ i915_max_freq_set(void *data, u64 val)
>   	hw_min = rps->min_freq;
>   
>   	if (val < hw_min || val > hw_max || val < rps->min_freq_softlimit) {
> -		mutex_unlock(&dev_priv->pcu_lock);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto unlock;
>   	}
>   
>   	rps->max_freq_softlimit = val;
> @@ -4196,9 +4181,9 @@ i915_max_freq_set(void *data, u64 val)
>   	if (intel_set_rps(dev_priv, val))
>   		DRM_DEBUG_DRIVER("failed to update RPS to new softlimit\n");
>   
> -	mutex_unlock(&dev_priv->pcu_lock);
> -
> -	return 0;
> +unlock:
> +	mutex_unlock(&rps->lock);
> +	return ret;
>   }
>   
>   DEFINE_SIMPLE_ATTRIBUTE(i915_max_freq_fops,
> @@ -4230,7 +4215,7 @@ i915_min_freq_set(void *data, u64 val)
>   
>   	DRM_DEBUG_DRIVER("Manually setting min freq to %llu\n", val);
>   
> -	ret = mutex_lock_interruptible(&dev_priv->pcu_lock);
> +	ret = mutex_lock_interruptible(&rps->lock);
>   	if (ret)
>   		return ret;
>   
> @@ -4244,8 +4229,8 @@ i915_min_freq_set(void *data, u64 val)
>   
>   	if (val < hw_min ||
>   	    val > hw_max || val > rps->max_freq_softlimit) {
> -		mutex_unlock(&dev_priv->pcu_lock);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto unlock;
>   	}
>   
>   	rps->min_freq_softlimit = val;
> @@ -4253,9 +4238,9 @@ i915_min_freq_set(void *data, u64 val)
>   	if (intel_set_rps(dev_priv, val))
>   		DRM_DEBUG_DRIVER("failed to update RPS to new softlimit\n");
>   
> -	mutex_unlock(&dev_priv->pcu_lock);
> -
> -	return 0;
> +unlock:
> +	mutex_unlock(&rps->lock);
> +	return ret;
>   }
>   
>   DEFINE_SIMPLE_ATTRIBUTE(i915_min_freq_fops,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 67cf0fe533f8..1f246d2a4e84 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -735,6 +735,8 @@ struct intel_rps_ei {
>   };
>   
>   struct intel_rps {
> +	struct mutex lock;
> +
I think this lock can now become part of struct intel_gt_pm.
>   	/*
>   	 * work, interrupts_enabled and pm_iir are protected by
>   	 * dev_priv->irq_lock
> @@ -1783,14 +1785,6 @@ struct drm_i915_private {
>   	/* Cannot be determined by PCIID. You must always read a register. */
>   	u32 edram_cap;
>   
> -	/*
> -	 * Protects RPS/RC6 register access and PCU communication.
> -	 * Must be taken after struct_mutex if nested. Note that
> -	 * this lock may be held for long periods of time when
> -	 * talking to hw - so only take it when talking to hw!
> -	 */
> -	struct mutex pcu_lock;
> -
>   	/* gen6+ GT PM state */
>   	struct intel_gen6_power_mgmt gt_pm;
>   
...
> -int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> -				    u32 mbox, u32 val,
> -				    int fast_timeout_us, int slow_timeout_ms)
> +static int __sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> +					     u32 mbox, u32 val,
> +					     int fast_timeout_us,
> +					     int slow_timeout_ms)
>   {
>   	int status;
>   
> -	WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
> -
lockdep_assert is missed here.

With this change, patch looks good to me.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>   	/* GEN6_PCODE_* are outside of the forcewake domain, we can
>   	 * use te fw I915_READ variants to reduce the amount of work
>   	 * required when reading/writing.
>   	 */
>   
> -	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY) {
> -		DRM_DEBUG_DRIVER("warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps\n",
> -				 val, mbox, __builtin_return_address(0));
> +	if (I915_READ_FW(GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY)
>   		return -EAGAIN;
> -	}
>   
>   	I915_WRITE_FW(GEN6_PCODE_DATA, val);
>   	I915_WRITE_FW(GEN6_PCODE_DATA1, 0);
> @@ -9290,11 +9273,8 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
>   	if (__intel_wait_for_register_fw(dev_priv,
>   					 GEN6_PCODE_MAILBOX, GEN6_PCODE_READY, 0,
>   					 fast_timeout_us, slow_timeout_ms,
> -					 NULL)) {
> -		DRM_ERROR("timeout waiting for pcode write of 0x%08x to mbox %x to finish for %ps\n",
> -			  val, mbox, __builtin_return_address(0));
> +					 NULL))
>   		return -ETIMEDOUT;
> -	}
>   
>   	I915_WRITE_FW(GEN6_PCODE_DATA, 0);
>   
> @@ -9303,13 +9283,28 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
>   	else
>   		status = gen6_check_mailbox_status(dev_priv);
>   
> +	return status;
> +}
> +
> +int sandybridge_pcode_write_timeout(struct drm_i915_private *dev_priv,
> +				    u32 mbox, u32 val,
> +				    int fast_timeout_us,
> +				    int slow_timeout_ms)
> +{
> +	int status;
> +
> +	mutex_lock(&dev_priv->sb_lock);
> +	status = __sandybridge_pcode_write_timeout(dev_priv, mbox, val,
> +						   fast_timeout_us,
> +						   slow_timeout_ms);
> +	mutex_unlock(&dev_priv->sb_lock);
> +
>   	if (status) {
>   		DRM_DEBUG_DRIVER("warning: pcode (write of 0x%08x to mbox %x) mailbox access failed for %ps: %d\n",
>   				 val, mbox, __builtin_return_address(0), status);
> -		return status;
>   	}
>   
> -	return 0;
> +	return status;
>   }
>   
>   static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
> @@ -9318,7 +9313,7 @@ static bool skl_pcode_try_request(struct drm_i915_private *dev_priv, u32 mbox,
>   {
>   	u32 val = request;
>   
> -	*status = sandybridge_pcode_read(dev_priv, mbox, &val);
> +	*status = __sandybridge_pcode_read(dev_priv, mbox, &val);
>   
>   	return *status || ((val & reply_mask) == reply);
>   }
> @@ -9348,7 +9343,7 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
>   	u32 status;
>   	int ret;
>   
> -	WARN_ON(!mutex_is_locked(&dev_priv->pcu_lock));
> +	mutex_lock(&dev_priv->sb_lock);
>   
>   #define COND skl_pcode_try_request(dev_priv, mbox, request, reply_mask, reply, \
>   				   &status)
> @@ -9384,6 +9379,7 @@ int skl_pcode_request(struct drm_i915_private *dev_priv, u32 mbox, u32 request,
>   	preempt_enable();
>   
>   out:
> +	mutex_unlock(&dev_priv->sb_lock);
>   	return ret ? ret : status;
>   #undef COND
>   }
> @@ -9453,8 +9449,7 @@ int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
>   
>   void intel_pm_setup(struct drm_i915_private *dev_priv)
>   {
> -	mutex_init(&dev_priv->pcu_lock);
> -
> +	mutex_init(&dev_priv->gt_pm.rps.lock);
>   	atomic_set(&dev_priv->gt_pm.rps.num_waiters, 0);
>   
>   	dev_priv->runtime_pm.suspended = false;
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 069b6a30468f..2cc64f0fda57 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -815,7 +815,6 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>   	state = enable ? PUNIT_PWRGT_PWR_ON(power_well_id) :
>   			 PUNIT_PWRGT_PWR_GATE(power_well_id);
>   
> -	mutex_lock(&dev_priv->pcu_lock);
>   	vlv_punit_get(dev_priv);
>   
>   #define COND \
> @@ -838,7 +837,6 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
>   
>   out:
>   	vlv_punit_put(dev_priv);
> -	mutex_unlock(&dev_priv->pcu_lock);
>   }
>   
>   static void vlv_power_well_enable(struct drm_i915_private *dev_priv,
> @@ -865,7 +863,6 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
>   	mask = PUNIT_PWRGT_MASK(power_well_id);
>   	ctrl = PUNIT_PWRGT_PWR_ON(power_well_id);
>   
> -	mutex_lock(&dev_priv->pcu_lock);
>   	vlv_punit_get(dev_priv);
>   
>   	state = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_STATUS) & mask;
> @@ -886,7 +883,6 @@ static bool vlv_power_well_enabled(struct drm_i915_private *dev_priv,
>   	WARN_ON(ctrl != state);
>   
>   	vlv_punit_put(dev_priv);
> -	mutex_unlock(&dev_priv->pcu_lock);
>   
>   	return enabled;
>   }
> @@ -1398,7 +1394,6 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>   	bool enabled;
>   	u32 state, ctrl;
>   
> -	mutex_lock(&dev_priv->pcu_lock);
>   	vlv_punit_get(dev_priv);
>   
>   	state = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ) & DP_SSS_MASK(pipe);
> @@ -1417,7 +1412,6 @@ static bool chv_pipe_power_well_enabled(struct drm_i915_private *dev_priv,
>   	WARN_ON(ctrl << 16 != state);
>   
>   	vlv_punit_put(dev_priv);
> -	mutex_unlock(&dev_priv->pcu_lock);
>   
>   	return enabled;
>   }
> @@ -1432,7 +1426,6 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>   
>   	state = enable ? DP_SSS_PWR_ON(pipe) : DP_SSS_PWR_GATE(pipe);
>   
> -	mutex_lock(&dev_priv->pcu_lock);
>   	vlv_punit_get(dev_priv);
>   
>   #define COND \
> @@ -1455,7 +1448,6 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
>   
>   out:
>   	vlv_punit_put(dev_priv);
> -	mutex_unlock(&dev_priv->pcu_lock);
>   }
>   
>   static void chv_pipe_power_well_enable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> index dc3b491b4d00..2d4e48e9e1d5 100644
> --- a/drivers/gpu/drm/i915/intel_sideband.c
> +++ b/drivers/gpu/drm/i915/intel_sideband.c
> @@ -142,8 +142,6 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>   {
>   	u32 val = 0;
>   
> -	lockdep_assert_held(&dev_priv->pcu_lock);
> -
>   	vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
>   			SB_CRRDDA_NP, addr, &val);
>   
> @@ -152,8 +150,6 @@ u32 vlv_punit_read(struct drm_i915_private *dev_priv, u32 addr)
>   
>   int vlv_punit_write(struct drm_i915_private *dev_priv, u32 addr, u32 val)
>   {
> -	lockdep_assert_held(&dev_priv->pcu_lock);
> -
>   	return vlv_sideband_rw(dev_priv, PCI_DEVFN(0, 0), IOSF_PORT_PUNIT,
>   			       SB_CRWRDA_NP, addr, &val);
>   }

-- 
Thanks,
Sagar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx