Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on Skylake H0.

From: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
To: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"jani.nikula@linux.intel.com" <jani.nikula@linux.intel.com>
Subject: Re: [PATCH 3/5] drm/i915/kbl: Kabylake A0 is based on	Skylake H0.
Date: Tue, 6 Oct 2015 17:43:44 +0000	[thread overview]
Message-ID: <1444153498.22060.72.camel@intel.com> (raw)
In-Reply-To: <8737xo9wor.fsf@intel.com>

On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
> On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > Kabylake is gen 9.5 derivated from Skylake H0 stepping.
> > 
> > So we don't need pre-production Skylake workaround and also
> > firmware loading will use SKL H0 offsets.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 7374a0d..580c005 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2436,7 +2436,6 @@ struct drm_i915_cmd_table {
> >  })
> >  #define INTEL_INFO(p) 	(&__I915__(p)->info)
> >  #define INTEL_DEVID(p)	(INTEL_INFO(p)->device_id)
> > -#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision)
> >  
> >  #define IS_I830(dev)		(INTEL_DEVID(dev) == 0x3577)
> >  #define IS_845G(dev)		(INTEL_DEVID(dev) == 0x2562)
> > @@ -2508,6 +2507,9 @@ struct drm_i915_cmd_table {
> >  
> >  #define IS_PRELIMINARY_HW(intel_info) ((intel_info)
> > ->is_preliminary)
> >  
> > +#define INTEL_REVID(p)	(__I915__(p)->dev->pdev->revision + 
> > \
> > +			 IS_KABYLAKE(p) ? 7 : 0)
> > +
> 
> I am not fond of this at all. It will be really confusing that
> ->revision is different from INTEL_REVID when checking the 
> workarounds,
> and that you'll be using SKL_REVID_* to match KBL revision
> ids. 

this is exactly one of the reasons why I did this sum in this way so
they never match...

> Additionally, we'll probably want to start removing SKL workarounds
> before KBL workarounds.

I believe this is another discussion... On HSW BDW I remember I was
removing old Wa as it was no longer needed, but on SKL I saw this REVID
and I believed the idea was to let them there since some devs might be
using preliminary platforms yet for other reasons... I don't see a
problem of letting the old W/a there.

> 
> Others may disagree, but I'd like KBL revid checks be different from
> SKL.
> 
> >  #define SKL_REVID_A0		(0x0)
> >  #define SKL_REVID_B0		(0x1)
> >  #define SKL_REVID_C0		(0x2)
> > @@ -2515,6 +2517,9 @@ struct drm_i915_cmd_table {
> >  #define SKL_REVID_E0		(0x4)
> >  #define SKL_REVID_F0		(0x5)
> >  
> > +/* KBL A0 is based on SKL H0 */
> > +#define KBL_REVID_A0		(0x7)
> 
> You can't compare this against INTEL_REVID() now can you...? Or is 
> this
> not the one in the spec? Confused already.

Yes, this is confusing indeed. It seems that we have many levels of
steppings (according to platform guys) and this platform stepping
returning 0 is our KBL A0, but this correspond to our internal gpu
stepping H0 (same going to skl h0).

Like dmc firmware loading for instance we need to load the firmware for
stepping 7.

So yes, this definition matches BSPec KBL A0.

> 
> BR,
> Jani.
> 
> > +
> >  #define BXT_REVID_A0		(0x0)
> >  #define BXT_REVID_B0		(0x3)
> >  #define BXT_REVID_C0		(0x9)
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx