[Intel-gfx] [PATCH] drm/i915/dmc: Step away from symbolic links

[Mika Kuoppala]

Jani Nikula <jani.nikula at linux.intel.com> writes:

> [ text/plain ]
> On Tue, 10 May 2016, Patrik Jakobsson <patrik.jakobsson at linux.intel.com> wrote:
>> On Tue, May 10, 2016 at 03:52:02PM +0300, Mika Kuoppala wrote:
>>> Patrik Jakobsson <patrik.jakobsson at linux.intel.com> writes:
>>> 
>>> > [ text/plain ]
>>> > Load specific firmware versions for the DMC instead of using symbolic
>>> > links. The currently recommended versions are: SKL 1.26, KBL 1.01 and
>>> > BXT 1.07.
>>> >
>>> 
>>> We should augment the commit message to answer the 'why' part.
>>> 
>>> Otherwise, looks good.
>>
>> Yes I agree. Where did this discussion take place? I don't believe I was
>> part of it. Rodrigo told me we've reached consensus on the decision but
>> that's all I know.
>
> This captures most of it I think
> http://mid.gmane.org/87fuu6i0zb.fsf@intel.com
>

Pretty much. In essence we want to offer a combination
with somekind of proof (read tested) that it works. And lock
that combination. Think about it as a colored seal paint on screw
heads on some products: these were tested and delivered together.
You can unscrew and dissassemble the parts still with minimal effort
and mix the pieces, but they may or may not work as planned if you do so.

And if you broke the seal paint and get a black screen, you know
who to blame.

-Mika

>
> BR,
> Jani.
>
>
>>
>> -Patrik
>>
>>> 
>>> -Mika
>>> 
>>> 
>>> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> > Cc: Imre Deak <imre.deak at intel.com>
>>> > Cc: Mika Kuoppala <mika.kuoppala at linux.intel.com>
>>> > Signed-off-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
>>> > ---
>>> >  drivers/gpu/drm/i915/intel_csr.c | 29 ++++++++++++++---------------
>>> >  1 file changed, 14 insertions(+), 15 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
>>> > index 2b3b428..ea047cd 100644
>>> > --- a/drivers/gpu/drm/i915/intel_csr.c
>>> > +++ b/drivers/gpu/drm/i915/intel_csr.c
>>> > @@ -41,15 +41,15 @@
>>> >   * be moved to FW_FAILED.
>>> >   */
>>> >  
>>> > -#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin"
>>> > +#define I915_CSR_KBL "i915/kbl_dmc_ver1_01.bin"
>>> >  MODULE_FIRMWARE(I915_CSR_KBL);
>>> >  #define KBL_CSR_VERSION_REQUIRED	CSR_VERSION(1, 1)
>>> >  
>>> > -#define I915_CSR_SKL "i915/skl_dmc_ver1.bin"
>>> > +#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin"
>>> >  MODULE_FIRMWARE(I915_CSR_SKL);
>>> > -#define SKL_CSR_VERSION_REQUIRED	CSR_VERSION(1, 23)
>>> > +#define SKL_CSR_VERSION_REQUIRED	CSR_VERSION(1, 26)
>>> >  
>>> > -#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin"
>>> > +#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin"
>>> >  MODULE_FIRMWARE(I915_CSR_BXT);
>>> >  #define BXT_CSR_VERSION_REQUIRED	CSR_VERSION(1, 7)
>>> >  
>>> > @@ -286,7 +286,7 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>>> >  	uint32_t dmc_offset = CSR_DEFAULT_FW_OFFSET, readcount = 0, nbytes;
>>> >  	uint32_t i;
>>> >  	uint32_t *dmc_payload;
>>> > -	uint32_t required_min_version;
>>> > +	uint32_t required_version;
>>> >  
>>> >  	if (!fw)
>>> >  		return NULL;
>>> > @@ -303,24 +303,23 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
>>> >  	csr->version = css_header->version;
>>> >  
>>> >  	if (IS_KABYLAKE(dev_priv)) {
>>> > -		required_min_version = KBL_CSR_VERSION_REQUIRED;
>>> > +		required_version = KBL_CSR_VERSION_REQUIRED;
>>> >  	} else if (IS_SKYLAKE(dev_priv)) {
>>> > -		required_min_version = SKL_CSR_VERSION_REQUIRED;
>>> > +		required_version = SKL_CSR_VERSION_REQUIRED;
>>> >  	} else if (IS_BROXTON(dev_priv)) {
>>> > -		required_min_version = BXT_CSR_VERSION_REQUIRED;
>>> > +		required_version = BXT_CSR_VERSION_REQUIRED;
>>> >  	} else {
>>> >  		MISSING_CASE(INTEL_REVID(dev_priv));
>>> > -		required_min_version = 0;
>>> > +		required_version = 0;
>>> >  	}
>>> >  
>>> > -	if (csr->version < required_min_version) {
>>> > -		DRM_INFO("Refusing to load old DMC firmware v%u.%u,"
>>> > -			 " please upgrade to v%u.%u or later"
>>> > -			   " [" FIRMWARE_URL "].\n",
>>> > +	if (csr->version != required_version) {
>>> > +		DRM_INFO("Refusing to load DMC firmware v%u.%u,"
>>> > +			 " please use v%u.%u [" FIRMWARE_URL "].\n",
>>> >  			 CSR_VERSION_MAJOR(csr->version),
>>> >  			 CSR_VERSION_MINOR(csr->version),
>>> > -			 CSR_VERSION_MAJOR(required_min_version),
>>> > -			 CSR_VERSION_MINOR(required_min_version));
>>> > +			 CSR_VERSION_MAJOR(required_version),
>>> > +			 CSR_VERSION_MINOR(required_version));
>>> >  		return NULL;
>>> >  	}
>>> >  
>>> > -- 
>>> > 2.5.0
>
> -- 
> Jani Nikula, Intel Open Source Technology Center