Re: [Intel-gfx] [PATCH] drm/i915: Inherit Kabylake platform features from Skylake

[Chris Wilson]

Quoting Rodrigo Vivi (2017-09-28 18:13:27)
> On Thu, Sep 28, 2017 at 05:00:53PM +, Chris Wilson wrote:
> > Quoting Rodrigo Vivi (2017-09-28 17:33:48)
> > > On Thu, Sep 28, 2017 at 02:59:13PM +, 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 
> > > > Cc: Rodrigo Vivi 
> > > > Cc: Paulo Zanoni 
> > > > Cc: Mika Kuoppala 
> > > > ---
> > > >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Inherit Kabylake platform features from Skylake

[Rodrigo Vivi]

On Thu, Sep 28, 2017 at 05:00:53PM +, Chris Wilson wrote:
> Quoting Rodrigo Vivi (2017-09-28 17:33:48)
> > On Thu, Sep 28, 2017 at 02:59:13PM +, 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 
> > > Cc: Rodrigo Vivi 
> > > Cc: Paulo Zanoni 
> > > Cc: Mika Kuoppala 
> > > ---
> > >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Inherit Kabylake platform features from Skylake

[Chris Wilson]

Quoting Rodrigo Vivi (2017-09-28 17:33:48)
> On Thu, Sep 28, 2017 at 02:59:13PM +, 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 
> > Cc: Rodrigo Vivi 
> > Cc: Paulo Zanoni 
> > Cc: Mika Kuoppala 
> > ---
> >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Inherit Kabylake platform features from Skylake

[Rodrigo Vivi]

On Thu, Sep 28, 2017 at 02:59:13PM +, 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 
> Cc: Rodrigo Vivi 
> Cc: Paulo Zanoni 
> Cc: Mika Kuoppala 
> ---
>  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
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx