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

[Ben Widawsky]

On Tue, Oct 06, 2015 at 08:51:13PM +0000, Rodrigo Vivi wrote:
> cc'ing Ben to get his opinion...
> 

Of course anything is possible wrt the delta of KBL features vs SKL. With the
knowledge we have, we can make a pretty educated guess that there will be no
changes, and with an equally high level of confidence say that if there are
changes, they will be very minor and self contained.

I am in favor of this minimalistic patch myself. I think both the result, and
reduced amount of churn make this patch favorable to the requested alternative.
Some finer comments below.

> On Tue, Oct 6, 2015 at 10:43 AM Vivi, Rodrigo <rodrigo.vivi at intel.com>
> wrote:
> 
> > On Tue, 2015-10-06 at 12:24 +0300, Jani Nikula wrote:
> > > On Tue, 06 Oct 2015, Rodrigo Vivi <rodrigo.vivi at intel.com> wrote:
> > > > Kabylake is gen 9.5 derivated from Skylake H0 stepping.

Don't call it 9.5... some people don't like that... Just say it's a SKL
derivative.

> > > >
> > > > So we don't need pre-production Skylake workaround and also
> > > > firmware loading will use SKL H0 offsets.

In fact we know some of these workarounds to be harmful.

> > > >
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at 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...
> >

Jani, I do understand your distaste with this patch. However, I think this is a
very reasonable, and more importantly, readable wart in the code. It's very
obvious what and why the macro does what it does.

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

I don't see a reason for this. Maybe you have thought of something I haven't?
We're not using any of the early SKL workarounds on KBL as a result of this
patch, so it shouldn't matter.

> > 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.
> >

I'm very much in favor of killing pre-production support (not that anyone
asked).

> > >
> > > 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.
> >

Maybe amend the comment to say that the actual PCI header has revid 0 (or
whatever it was). With that, it's pretty clear - and yes, it is a value which
can and should be used to compare with INTEL_REVID, but as stated above, the
comparison isn't needed, and if/when it is, we can/should revisit the more
intrusive change you're suggesting.