[Intel-gfx] [PATCH 5/5] drm/i915: Notify Coarse Power Gating changes to GuC

[Yu Dai]

On 09/10/2015 11:16 PM, Kamble, Sagar A wrote:
> Hi Alex,
>
> Kindly incorporate changes in this patch in your patch at:
> http://lists.freedesktop.org/archives/intel-gfx/2015-September/075668.html
> - [PATCH 5/6] drm/i915/guc: Media domain bit needed when notify GuC rc6
> state
>
> This is because GuC sample forcewake parameters depend on Coarse power
> gating configuration.
>
> Thanks
> Sagar
>
>
>
> On 8/28/2015 1:25 AM, O'Rourke, Tom wrote:
> > On Sun, Aug 23, 2015 at 05:52:51PM +0530, Sagar Arun Kamble wrote:
> >> From: Alex Dai <yu.dai at intel.com>
> >>
> > [TOR:] This commit message is inadequate.  The
> > needed information is in the cover letter but
> > is lacking here.  Please rebase with Alex's
> > previous patch "drm/i915: Notify GuC rc6 state"
> >
> >> Signed-off-by: Alex Dai <yu.dai at intel.com>
> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_guc_submission.c | 18 ++++++++++++++++++
> >>   drivers/gpu/drm/i915/intel_guc.h           |  1 +
> >>   drivers/gpu/drm/i915/intel_pm.c            | 16 ++++++++++++----
> >>   3 files changed, 31 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >> index ec70393..462c679 100644
> >> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >> @@ -877,6 +877,24 @@ int i915_guc_submission_enable(struct drm_device *dev)
> >>   	return 0;
> >>   }
> >>
> >> +void i915_guc_sample_forcewake(struct drm_device *dev, u32 fw_data)
> >> +{
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	struct intel_guc *guc = &dev_priv->guc;
> >> +	struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
> >> +
> >> +	/* Notify GuC about CPG changes. */
> >> +	if (guc_fw->guc_fw_fetch_status == GUC_FIRMWARE_SUCCESS) {

This might not be safe to protect code below. In the case of suspend, 
the guc_fw_fetch_status and guc_fw_load_status both are SUCCESS. 
However, we need to reload fw before any host2guc calls.

> >> +		u32 data[2];
> >> +
> >> +		data[0] = HOST2GUC_ACTION_SAMPLE_FORCEWAKE;
> >> +		data[1] = fw_data;
> >> +
> >> +		if (host2guc_action(guc, data, 2))
> >> +			DRM_ERROR("Unable to notify GuC of CPG change\n");
> >> +	}
> >> +}
> >> +

Please call host2guc_sample_forcewake rather than a host2guc_action.

> >>   void i915_guc_submission_disable(struct drm_device *dev)
> >>   {
> >>   	struct drm_i915_private *dev_priv = dev->dev_private;
> >> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> >> index 4ec2d27..691574d 100644
> >> --- a/drivers/gpu/drm/i915/intel_guc.h
> >> +++ b/drivers/gpu/drm/i915/intel_guc.h
> >> @@ -118,5 +118,6 @@ int i915_guc_submit(struct i915_guc_client *client,
> >>   		    struct drm_i915_gem_request *rq);
> >>   void i915_guc_submission_disable(struct drm_device *dev);
> >>   void i915_guc_submission_fini(struct drm_device *dev);
> >> +void i915_guc_sample_forcewake(struct drm_device *dev, u32 fw_data);
> >>
> >>   #endif
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index c0345d2..4a0483c 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -4616,6 +4616,9 @@ static void gen9_disable_rps(struct drm_device *dev)
> >>   	struct drm_i915_private *dev_priv = dev->dev_private;
> >>
> >>   	I915_WRITE(GEN6_RC_CONTROL, 0);
> >> +
> >> +	i915_guc_sample_forcewake(dev, 0);
> >> +
> >>   	I915_WRITE(GEN9_PG_ENABLE, 0);
> >>   }
> >>
> >> @@ -4804,6 +4807,7 @@ static void gen9_enable_rc6(struct drm_device *dev)
> >>   	struct drm_i915_private *dev_priv = dev->dev_private;
> >>   	struct intel_engine_cs *ring;
> >>   	uint32_t rc6_mask = 0;
> >> +	uint32_t cpg_data = 0;
> >>   	int unused;
> >>
> >>   	/* 1a: Software RC state - RC0 */
> >> @@ -4843,11 +4847,15 @@ static void gen9_enable_rc6(struct drm_device *dev)
> >>   	 * WaRsDisableCoarsePowerGating:skl,bxt - Render/Media PG need to be disabled with RC6.
> >>   	 */
> >>   	if ((IS_BROXTON(dev) && (INTEL_REVID(dev) < BXT_REVID_B0)) ||
> >> -		(IS_SKYLAKE(dev) && (INTEL_REVID(dev) <= SKL_REVID_E0)))
> >> +		(IS_SKYLAKE(dev) && (INTEL_REVID(dev) <= SKL_REVID_E0))) {
> >> +		i915_guc_sample_forcewake(dev, 0);
> >>   		I915_WRITE(GEN9_PG_ENABLE, 0);
> >> -	else
> >> -		I915_WRITE(GEN9_PG_ENABLE, (rc6_mask & GEN6_RC_CTL_RC6_ENABLE) ?
> >> -				(GEN9_RENDER_PG_ENABLE | GEN9_MEDIA_PG_ENABLE) : 0);
> >> +	} else {
> >> +		cpg_data = (rc6_mask & GEN6_RC_CTL_RC6_ENABLE)?
> >> +				(GEN9_RENDER_PG_ENABLE | GEN9_MEDIA_PG_ENABLE):0;
> >> +		i915_guc_sample_forcewake(dev, cpg_data);
> >> +		I915_WRITE(GEN9_PG_ENABLE, cpg_data);
> >> +	}
> >>
> >>   	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> >>
> >> --

Be note that calling guc function here actually happens in another 
thread. The host2guc action will fail if GuC is not loaded. So there is 
a potential racing issue here. I believe the failure will happen during 
suspend / resume. I believe calling i915_guc_sample_forcewake or 
host2guc_sample_forcewake after GuC is initialized is good enough.