| Message ID | 20170928145913.2778-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 28, 2017 at 02:59:13PM +0000, Chris Wilson wrote: > I recently tried to update the gen9 feature matrix and to my unpleasant > surprise found that Kabylake still acted like Broadwell and didn't > enable the feature. This is because kbl/cfl are inheriting their > defaults from Broadwell and not Skylake. Hmm... I should've considered this would happen someday sooner than later... My Bad... > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_pci.c | 21 +++++---------------- > 1 file changed, 5 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index da60866b6628..01d4b569b2cc 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -495,13 +495,9 @@ static const struct intel_device_info intel_geminilake_info __initconst = { > }; > > #define KBL_PLATFORM \ > - BDW_FEATURES, \ Note that BDW has _FEATURES and _PLATFORM. The idea is that _FEATURES would be he place to get inherited while platform was for that specific one and shouldn't be imported by a different platform. so we wouldn't need to overwrite any specific value like .platform. And we would have a placeholder for the specific values and features that we want only on that particular platform and never propagated to newer ones. But yeah... that is lame, fragile, non documented and caused confusion. Sorry. > - .gen = 9, \ > + SKL_PLATFORM, \ > .platform = INTEL_KABYLAKE, \ > - .has_csr = 1, \ > - .has_guc = 1, \ > - .has_ipc = 1, \ > - .ddb_size = 896 > + .has_ipc = 1 > > static const struct intel_device_info intel_kabylake_gt1_info __initconst = { > KBL_PLATFORM, > @@ -520,13 +516,8 @@ static const struct intel_device_info intel_kabylake_gt3_info __initconst = { > }; > > #define CFL_PLATFORM \ > - BDW_FEATURES, \ > - .gen = 9, \ > - .platform = INTEL_COFFEELAKE, \ > - .has_csr = 1, \ > - .has_guc = 1, \ > - .has_ipc = 1, \ > - .ddb_size = 896 > + KBL_PLATFORM, \ > + .platform = INTEL_COFFEELAKE > > static const struct intel_device_info intel_coffeelake_gt1_info __initconst = { > CFL_PLATFORM, > @@ -545,14 +536,12 @@ static const struct intel_device_info intel_coffeelake_gt3_info __initconst = { > }; > > static const struct intel_device_info intel_cannonlake_gt2_info __initconst = { > - BDW_FEATURES, > + SKL_PLATFORM, my OCD doesn't like to read cannonlake as skylake platform... So maybe we should just kill "_PLATFORM" and rename everything back to "FEATURES" and on this case also it would be better for CFL to inherit KBL one and CNL inherit CFL one and on. The downside is that we again don't have a place for specific features that we don't want to propagate. Thanks for bringing this up, Rodrigo. > .is_alpha_support = 1, > .platform = INTEL_CANNONLAKE, > .gen = 10, > .gt = 2, > .ddb_size = 1024, > - .has_csr = 1, > - .has_ipc = 1, > .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 } > }; > > -- > 2.14.1 >
Quoting Rodrigo Vivi (2017-09-28 17:33:48) > On Thu, Sep 28, 2017 at 02:59:13PM +0000, Chris Wilson wrote: > > I recently tried to update the gen9 feature matrix and to my unpleasant > > surprise found that Kabylake still acted like Broadwell and didn't > > enable the feature. This is because kbl/cfl are inheriting their > > defaults from Broadwell and not Skylake. > > Hmm... I should've considered this would happen someday sooner than later... > My Bad... > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_pci.c | 21 +++++---------------- > > 1 file changed, 5 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > index da60866b6628..01d4b569b2cc 100644 > > --- a/drivers/gpu/drm/i915/i915_pci.c > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > @@ -495,13 +495,9 @@ static const struct intel_device_info intel_geminilake_info __initconst = { > > }; > > > > #define KBL_PLATFORM \ > > - BDW_FEATURES, \ > > Note that BDW has _FEATURES and _PLATFORM. > The idea is that _FEATURES would be he place to get inherited > while platform was for that specific one and shouldn't be imported by a different platform. > so we wouldn't need to overwrite any specific value like .platform. > And we would have a placeholder for the specific values and features that we want only > on that particular platform and never propagated to newer ones. > > But yeah... that is lame, fragile, non documented and caused confusion. Sorry. I honestly don't mind, just my expectation was that cfl/kbl == skl + constant so that I could enable a feature in one gen and expect it to forward propagate (and I prefer intel_device_info for its debug/error reporting and opportunity for its fine granularity). I don't think you want to mix the platform name with features then, i.e. GEN8_FEATURES, GEN8_LP_FEATURES GEN9_FEATURES, GEN9_LP_FEATURES #define KBL_PLATFORM GEN9_FEATURES, ... > > static const struct intel_device_info intel_cannonlake_gt2_info __initconst = { > > - BDW_FEATURES, > > + SKL_PLATFORM, > > my OCD doesn't like to read cannonlake as skylake platform... We don't appear to support Cannonlake as a whole ;) Just a couple of CI devices? > So maybe we should just kill "_PLATFORM" and rename everything back to "FEATURES" > and on this case also it would be better for CFL to inherit KBL one and CNL inherit CFL one and on. The FEATURES / PLATFORM split is fine, it can even extend to multiple inheritance (for as long as the last-one-wins rule works). -Chris
On Thu, Sep 28, 2017 at 05:00:53PM +0000, Chris Wilson wrote: > Quoting Rodrigo Vivi (2017-09-28 17:33:48) > > On Thu, Sep 28, 2017 at 02:59:13PM +0000, Chris Wilson wrote: > > > I recently tried to update the gen9 feature matrix and to my unpleasant > > > surprise found that Kabylake still acted like Broadwell and didn't > > > enable the feature. This is because kbl/cfl are inheriting their > > > defaults from Broadwell and not Skylake. > > > > Hmm... I should've considered this would happen someday sooner than later... > > My Bad... > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_pci.c | 21 +++++---------------- > > > 1 file changed, 5 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > > index da60866b6628..01d4b569b2cc 100644 > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > @@ -495,13 +495,9 @@ static const struct intel_device_info intel_geminilake_info __initconst = { > > > }; > > > > > > #define KBL_PLATFORM \ > > > - BDW_FEATURES, \ > > > > Note that BDW has _FEATURES and _PLATFORM. > > The idea is that _FEATURES would be he place to get inherited > > while platform was for that specific one and shouldn't be imported by a different platform. > > so we wouldn't need to overwrite any specific value like .platform. > > And we would have a placeholder for the specific values and features that we want only > > on that particular platform and never propagated to newer ones. > > > > But yeah... that is lame, fragile, non documented and caused confusion. Sorry. > > I honestly don't mind, just my expectation was that cfl/kbl == skl + > constant so that I could enable a feature in one gen and expect it to > forward propagate (and I prefer intel_device_info for its debug/error > reporting and opportunity for its fine granularity). > > I don't think you want to mix the platform name with features then, i.e. > GEN8_FEATURES, GEN8_LP_FEATURES > GEN9_FEATURES, GEN9_LP_FEATURES > > #define KBL_PLATFORM GEN9_FEATURES, ... Good idea. Only doubt is how to rename HSW_FEATURES GEN7_5_FEATURES ? GEN75_FEATURES ? or just leave HSW_FEATURES as the exception? > > > > static const struct intel_device_info intel_cannonlake_gt2_info __initconst = { > > > - BDW_FEATURES, > > > + SKL_PLATFORM, > > > > my OCD doesn't like to read cannonlake as skylake platform... > > We don't appear to support Cannonlake as a whole ;) > Just a couple of CI devices? > > > So maybe we should just kill "_PLATFORM" and rename everything back to "FEATURES" > > and on this case also it would be better for CFL to inherit KBL one and CNL inherit CFL one and on. > > The FEATURES / PLATFORM split is fine, it can even extend to multiple > inheritance (for as long as the last-one-wins rule works). > -Chris
Quoting Rodrigo Vivi (2017-09-28 18:13:27) > On Thu, Sep 28, 2017 at 05:00:53PM +0000, Chris Wilson wrote: > > Quoting Rodrigo Vivi (2017-09-28 17:33:48) > > > On Thu, Sep 28, 2017 at 02:59:13PM +0000, Chris Wilson wrote: > > > > I recently tried to update the gen9 feature matrix and to my unpleasant > > > > surprise found that Kabylake still acted like Broadwell and didn't > > > > enable the feature. This is because kbl/cfl are inheriting their > > > > defaults from Broadwell and not Skylake. > > > > > > Hmm... I should've considered this would happen someday sooner than later... > > > My Bad... > > > > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_pci.c | 21 +++++---------------- > > > > 1 file changed, 5 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > > > > index da60866b6628..01d4b569b2cc 100644 > > > > --- a/drivers/gpu/drm/i915/i915_pci.c > > > > +++ b/drivers/gpu/drm/i915/i915_pci.c > > > > @@ -495,13 +495,9 @@ static const struct intel_device_info intel_geminilake_info __initconst = { > > > > }; > > > > > > > > #define KBL_PLATFORM \ > > > > - BDW_FEATURES, \ > > > > > > Note that BDW has _FEATURES and _PLATFORM. > > > The idea is that _FEATURES would be he place to get inherited > > > while platform was for that specific one and shouldn't be imported by a different platform. > > > so we wouldn't need to overwrite any specific value like .platform. > > > And we would have a placeholder for the specific values and features that we want only > > > on that particular platform and never propagated to newer ones. > > > > > > But yeah... that is lame, fragile, non documented and caused confusion. Sorry. > > > > I honestly don't mind, just my expectation was that cfl/kbl == skl + > > constant so that I could enable a feature in one gen and expect it to > > forward propagate (and I prefer intel_device_info for its debug/error > > reporting and opportunity for its fine granularity). > > > > I don't think you want to mix the platform name with features then, i.e. > > GEN8_FEATURES, GEN8_LP_FEATURES > > GEN9_FEATURES, GEN9_LP_FEATURES > > > > #define KBL_PLATFORM GEN9_FEATURES, ... > > Good idea. > Only doubt is how to rename HSW_FEATURES > GEN7_5_FEATURES ? > GEN75_FEATURES ? > or just leave HSW_FEATURES as the exception? G75_FEATURES, looking back at g33 and g4x where there have been substantial changes mid-generation. (Makes the notion of generation quite weak, but it's loose terminology for our convenience anyway considering the different update cycles for the different blocks). -Chris
diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index da60866b6628..01d4b569b2cc 100644 --- a/drivers/gpu/drm/i915/i915_pci.c +++ b/drivers/gpu/drm/i915/i915_pci.c @@ -495,13 +495,9 @@ static const struct intel_device_info intel_geminilake_info __initconst = { }; #define KBL_PLATFORM \ - BDW_FEATURES, \ - .gen = 9, \ + SKL_PLATFORM, \ .platform = INTEL_KABYLAKE, \ - .has_csr = 1, \ - .has_guc = 1, \ - .has_ipc = 1, \ - .ddb_size = 896 + .has_ipc = 1 static const struct intel_device_info intel_kabylake_gt1_info __initconst = { KBL_PLATFORM, @@ -520,13 +516,8 @@ static const struct intel_device_info intel_kabylake_gt3_info __initconst = { }; #define CFL_PLATFORM \ - BDW_FEATURES, \ - .gen = 9, \ - .platform = INTEL_COFFEELAKE, \ - .has_csr = 1, \ - .has_guc = 1, \ - .has_ipc = 1, \ - .ddb_size = 896 + KBL_PLATFORM, \ + .platform = INTEL_COFFEELAKE static const struct intel_device_info intel_coffeelake_gt1_info __initconst = { CFL_PLATFORM, @@ -545,14 +536,12 @@ static const struct intel_device_info intel_coffeelake_gt3_info __initconst = { }; static const struct intel_device_info intel_cannonlake_gt2_info __initconst = { - BDW_FEATURES, + SKL_PLATFORM, .is_alpha_support = 1, .platform = INTEL_CANNONLAKE, .gen = 10, .gt = 2, .ddb_size = 1024, - .has_csr = 1, - .has_ipc = 1, .color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 } };
I recently tried to update the gen9 feature matrix and to my unpleasant surprise found that Kabylake still acted like Broadwell and didn't enable the feature. This is because kbl/cfl are inheriting their defaults from Broadwell and not Skylake. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/i915_pci.c | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-)