Re: [PATCH 1/1] firmware/dmc/icl: load v1.07 on icelake.

Joonas Lahtinen
2018-09-06T09:50:53Z

Quoting Rodrigo Vivi (2018-09-05 19:42:28)
> On Wed, Sep 05, 2018 at 12:07:43PM +0300, Joonas Lahtinen wrote:
> > Quoting Rodrigo Vivi (2018-09-04 08:27:14)
> > > On Mon, Sep 03, 2018 at 01:00:39PM +0300, Imre Deak wrote:
> > > > On Mon, Aug 27, 2018 at 05:38:44PM -0700, Anusha Srivatsa wrote:
> > > > > Add Support to load DMC on Icelake.
> > > > >
> > > > > While at it, also add support to load the firmware
> > > > > during system resume.
> > > > >
> > > > > v2: load firmware during system resume.(Imre)
> > > > >
> > > > > v3: enable has_csr for icelake.(Jyoti)
> > > > >
> > > > > v4: Only load the firmware in this patch
> > > > >
> > > > > Cc: Jyoti Yadav <jyoti.r.yadav@intel.com>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > >
> > > > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > > >
> > > > Is it ok to push this already now that the ICL 1.07 firmware is in [1]
> > > > or do we have to wait until it propagates to [2]?
> > >
> > > The main motivation behind having drm-firmware is the unpredictability
> > > of linux-firmware.git pull requests acceptance. It may take 1 day or it
> > > may take 2 months.
> > >
> > > So on drm-firmware we at least have it public in a way OSVs
> > > could easisly backport. Although hopefully by the end of 4.20
> > > cycle I believe it will be there on linux-firmware.git already.
> > >
> > > So if fw is already on drm-firmware and passing all tests
> > > we should be able to push the patch to dinq.
> >
> > Was not the decision that we only gate the MODULE_FIRMWARE line until
> > the firmware is in linux-firmware.git?
> >
> > So it should be no harm to support loading firmwares that are available
> > from drm-firmware.git as long as we don't add the MODULE_FIRMWARE which
> > would trigger false positives for distro packagers.
>
> As far as I can remember we had agreed on changing this and adding
> MODULE_FIRMWARE from the beginning.

Is there an e-mail about it? I must have missed such discussion.

> 1. the process gets complex
> 2. we forgot many times to add it afterwards
> 3. module_firmware changes nothing... only the fact that initrd generation
> wont complain if firmware is not there yet.

Yes, but isn't this the only thing we've got complaints of? At least
I've not noticed distros complaining about not having the MODULE_FIRMWARE
(which would omit the firmware from initrd).

So I'd rather stick to the process previously decided on. Assuming pull
request will be accepted in X days is always just an assumption.

Regards, Joonas

> 4. The old issue was that patches were merged without the pull request
> being sent... We fixed that by only accepting patches after pull request
> is sent.
> 5. By the time that all lands to distros linux-firmware.git will have
> the firmware because of 4.
> 6. We have now drm-firmware to mirror what official linux-firmware.git will
> be in few weeks from now.
>
> So I don't see a reason anymore why to keep with complicated process with
> split MODULE_FIRMWARE.
>
> Thanks,
> Rodrigo.
>
> >
> > Regards, Joonas
> >
> > >
> > > Thanks,
> > > Rodrigo.
> > >
> > > >
> > > > [1] https://cgit.freedesktop.org/drm/drm-firmware/
> > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git
> > > >
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_csr.c | 7 +++++++
> > > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++
> > > > > 2 files changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> > > > > index 1ec4f09c61f6..6d9d47322405 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_csr.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > > > > @@ -34,6 +34,9 @@
> > > > > * low-power state and comes back to normal.
> > > > > */
> > > > >
> > > > > +#define I915_CSR_ICL "i915/icl_dmc_ver1_07.bin"
> > > > > +#define ICL_CSR_VERSION_REQUIRED CSR_VERSION(1, 7)
> > > > > +
> > > > > #define I915_CSR_GLK "i915/glk_dmc_ver1_04.bin"
> > > > > MODULE_FIRMWARE(I915_CSR_GLK);
> > > > > #define GLK_CSR_VERSION_REQUIRED CSR_VERSION(1, 4)
> > > > > @@ -301,6 +304,8 @@ static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
> > > > > if (csr->fw_path == i915_modparams.dmc_firmware_path) {
> > > > > /* Bypass version check for firmware override. */
> > > > > required_version = csr->version;
> > > > > + } else if (IS_ICELAKE(dev_priv)) {
> > > > > + required_version = ICL_CSR_VERSION_REQUIRED;
> > > > > } else if (IS_CANNONLAKE(dev_priv)) {
> > > > > required_version = CNL_CSR_VERSION_REQUIRED;
> > > > > } else if (IS_GEMINILAKE(dev_priv)) {
> > > > > @@ -458,6 +463,8 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
> > > > >
> > > > > if (i915_modparams.dmc_firmware_path)
> > > > > csr->fw_path = i915_modparams.dmc_firmware_path;
> > > > > + else if (IS_ICELAKE(dev_priv))
> > > > > + csr->fw_path = I915_CSR_ICL;
> > > > > else if (IS_CANNONLAKE(dev_priv))
> > > > > csr->fw_path = I915_CSR_CNL;
> > > > > else if (IS_GEMINILAKE(dev_priv))
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index 2852395125cd..bd7da068e813 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -3563,6 +3563,9 @@ static void icl_display_core_init(struct drm_i915_private *dev_priv,
> > > > >
> > > > > /* 7. Setup MBUS. */
> > > > > icl_mbus_init(dev_priv);
> > > > > +
> > > > > + if (resume && dev_priv->csr.dmc_payload)
> > > > > + intel_csr_load_program(dev_priv);
> > > > > }
> > > > >
> > > > > static void icl_display_core_uninit(struct drm_i915_private *dev_priv)
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > _______________________________________________
> > > > 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
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx