| Message ID | 1444085055-27447-4-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Additionally, we'll probably want to start removing SKL workarounds before KBL workarounds. 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. 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
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 >
cc'ing Ben to get his opinion... On Tue, Oct 6, 2015 at 10:43 AM Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote: > 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 >
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@intel.com> > wrote: > > > 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. 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@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.
On Wed, 07 Oct 2015, Ben Widawsky <ben@bwidawsk.net> wrote: > 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@intel.com> >> wrote: >> >> > 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. > > 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@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. INTEL_REVID() is supposed to be a shorthand for getting at ->dev->pdev->revision. I think it's rather surprising that on one platform this would not hold. It's a clever trick, I'll give you that. >> > > 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). It was specifically dropping pre-production SKL workarounds I meant above. You might be interested in having a look at [1] I wrote to address the revid/workaround part of the SKL vs. KBL discussion. With that, I think we could keep INTEL_REVID() as it is, make IS_SKL_REVID() match only SKL, and add IS_KBL_REVID() to match only KBL. As you say, most early SKL workarounds don't impact KBL, so we wouldn't have to touch so many places in the code to add KBL workarounds. I also think that patch, incl. adding separate IS_KBL_REVID, is orthogonal to whether IS_SKYLAKE() should match KBL or not. [1] http://mid.gmane.org/1444131676-12187-3-git-send-email-jani.nikula@intel.com >> > > >> > > 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. I am still confused. I fear you may start thinking that no longer supports my side of the argument. :( BR, Jani.
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) + #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) + #define BXT_REVID_A0 (0x0) #define BXT_REVID_B0 (0x3) #define BXT_REVID_C0 (0x9)
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(-)