Re: [PATCH v2] drm/i915/dmc: Step away from symbolic links

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Mon, 2016-06-27 at 20:45 +0200, Patrik Jakobsson wrote:
> On Mon, Jun 27, 2016 at 7:12 PM, Vivi, Rodrigo <rodrigo.vivi@intel.co
> m> wrote:
> > 
> > On Mon, 2016-06-27 at 19:51 +0300, Imre Deak wrote:
> > > 
> > > On Mon, 2016-06-27 at 19:32 +0300, Vivi, Rodrigo wrote:
> > > > 
> > > > 
> > > > On Mon, 2016-06-27 at 14:20 +0300, Imre Deak wrote:
> > > > > 
> > > > > 
> > > > > Adding Christophe, he was supposed to make the release after
> > > > > validation.
> > > > Apparently we are almost ready to release and one latest round
> > > > of
> > > > final
> > > > validation was pending.
> > > > 
> > > > Christophe, any news on this front?
> > > > 
> > > > > 
> > > > > 
> > > > >  I don't think it prevents merging this patch though, the
> > > > > result is failure to load the firmware in either case.
> > > > I was going to say that I agree, but I believe Patrik might be
> > > > right.
> > > > Without this patch we load the 1.06 while with this patch we
> > > > start
> > > > loading only the 1.07 that is not available.
> > > > Although 1.06 might have issues the failures would be
> > > > different. So
> > > > or
> > > > we blacklist 1.06 with a separated patch and then merge this
> > > > one or
> > > > we
> > > > release the 1.07 before.
> > > 1.06 is already blacklisted, it has known problems.
> > Oh! So I agree with the first statement. Let's merge this patch ;)
> That was new info for me as well. I don't have commit access so
> anyone
> who can, feel free to merge.

I just did.

Thanks,
Rodrigo.

> 
> -Patrik
> 
> > 
> > 
> > > 
> > > 
> > > --Imre
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > On ma, 2016-06-27 at 12:57 +0200, Patrik Jakobsson wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, Jun 15, 2016 at 12:11:55AM +0000, Vivi, Rodrigo
> > > > > > wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Mon, 2016-05-23 at 10:57 +0200, Patrik Jakobsson
> > > > > > > wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Wed, May 18, 2016 at 01:24:12PM +0300, Mika Kuoppala
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Patrik Jakobsson <patrik.jakobsson@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.
> > > > > > > > > > 
> > > > > > > > > > Certain DMC versions need workarounds in the driver
> > > > > > > > > > which
> > > > > > > > > > forces
> > > > > > > > > > us to
> > > > > > > > > > have a tight dependency between firmware and
> > > > > > > > > > driver. In
> > > > > > > > > > order to
> > > > > > > > > > be able
> > > > > > > > > > to provide a tested and known working configuration
> > > > > > > > > > we
> > > > > > > > > > must
> > > > > > > > > > lock
> > > > > > > > > > down on
> > > > > > > > > > a specific DMC firmware version.
> > > > > > > > > > 
> > > > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > > > > > > > > > Signed-off-by: Patrik Jakobsson
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > > > > > > 
> > > > > > > > > We need ack from Rodrigo and/or whomever is handling
> > > > > > > > > the fw releasing side.
> > > > > > > > > 
> > > > > > > > > -Mika
> > > > > > > > > 
> > > > > > > > As discussed on IRC, Rodrigo is currently away but
> > > > > > > > since he
> > > > > > > > requested
> > > > > > > > this
> > > > > > > > feature we indirectly have his ACK.
> > > > > > > indeed! ;)
> > > > > > I assume we need BXT 1.07 released on 01.org before merging
> > > > > > this.
> > > > > > Any status on
> > > > > > that?
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > -Patrik
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > -Patrik
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > ---
> > > > > > > > > >  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
> > > > > > > > > > _mi
> > > > > > > > > > n_
> > > > > > > > > > ve
> > > > > > > > > > rsion)
> > > > > > > > > > ,
> > > > > > > > > > -                        CSR_VERSION_MINOR(required
> > > > > > > > > > _mi
> > > > > > > > > > n_
> > > > > > > > > > ve
> > > > > > > > > > rsion)
> > > > > > > > > > );
> > > > > > > > > > +                        CSR_VERSION_MAJOR(required
> > > > > > > > > > _ve
> > > > > > > > > > rs
> > > > > > > > > > io
> > > > > > > > > > n),
> > > > > > > > > > +                        CSR_VERSION_MINOR(required
> > > > > > > > > > _ve
> > > > > > > > > > rs
> > > > > > > > > > io
> > > > > > > > > > n));
> > > > > > > > > >                 return NULL;
> > > > > > > > > >         }
> > > > > > > > > > 
> > > > > > > > > > --
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx