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

[Rodrigo Vivi]

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 at chris-wilson.co.uk>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at 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
>