Re: [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader

From: Yu Dai <yu.dai@intel.com>
To: "O'Rourke, Tom" <Tom.O'Rourke@intel.com>,
	Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 04/13 v4] drm/i915: GuC-specific firmware loader
Date: Mon, 20 Jul 2015 09:18:03 -0700	[thread overview]
Message-ID: <55AD1F3B.9040206@intel.com> (raw)
In-Reply-To: <20150718003538.GA81395@torourke-desk1>

On 07/17/2015 05:35 PM, O'Rourke, Tom wrote:
> On Thu, Jul 09, 2015 at 07:29:05PM +0100, Dave Gordon wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > +static u32 get_core_family(struct drm_i915_private *dev_priv)
> > +{
> > +	switch (INTEL_INFO(dev_priv)->gen) {
> > +	case 8:
> > +		return GFXCORE_FAMILY_GEN8;
> [TOR:] Should Gen 8 case be included here if only Gen 9 is supported?

Yes, we can remove this even Gen8 is capable but it is not supported by 
these patch series anyway.

> > +
> > +
> > +	/* Set MMIO/WA for GuC init */
> > +	I915_WRITE(DRBMISC1, DOORBELL_ENABLE);
> [TOR:] Should this DOORBELL_ENABLE be dropped?  A note in
> the BSpec indicates this is not needed, but also it should
> be harmless.

Per response from firmware team / BSpec, we can remove this line.

> > +
> > +	/* Enable MIA caching. GuC clock gating is disabled. */
> > +	I915_WRITE(GUC_SHIM_CONTROL, GUC_SHIM_CONTROL_VALUE);
> [TOR:] Should guc clock gating be enabled?  A note in the
> BSpec indicates this should be disabled for certain
> pre-production steppings; this note may not apply to later
> steppings.  Normally, the driver would enable guc clock
> gating (bit 15, GUC_ENABLE_MIA_CLOCK_GATING).

There was a hang issue in GuC if clock gating is enabled. This has be 
resolved for a while. We should enable this bit.

> > +
> > +	/* WaC6DisallowByGfxPause*/
> > +	I915_WRITE(GEN6_GFXPAUSE, 0x30FFF);
> > +
> > +	if (IS_SKYLAKE(dev))
> > +		I915_WRITE(GEN9_GT_PM_CONFIG, GEN8_GT_DOORBELL_ENABLE);
> > +	else
> > +		I915_WRITE(GEN8_GT_PM_CONFIG, GEN8_GT_DOORBELL_ENABLE);
> [TOR:] Would a comment be helpful here?  This line is correct
> for Broxton (Gen 9 and not Skylake) but the constants are
> reused from Gen 8.
>
> > +
>

BXT is Gen9 LP, which is using same mmio register as Gen8 for this case. 
My suggestion:
s/GEN8_GT_DOORBELL_ENABLE/GT_DOORBELL_ENABLE/g
And, add definition below. Use it here to avoid confuse.
#define GEN9LP_GT_PM_CONFIG 0x138140

s/I915_WRITE(GEN8_GT_PM_CONFIG, GEN8_GT_DOORBELL_ENABLE);
/I915_WRITE(GEN9LP_GT_PM_CONFIG, GT_DOORBELL_ENABLE);/g

Thanks,
Alex
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx