| Message ID | 1467710712-32431-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Nak. I don't intend to update the symbolic links on linux-firmware.git repository anymore so if we receive a new minor version update we are not going to load. I was the one advocating in the favor for the symbolic link flexibility but I lost the discussions for the stability and validation etc. So let's just move away from symbolic link for good. On Tue, 2016-07-05 at 12:25 +0300, Mika Kuoppala wrote: > We need the ability to explicitly load only a specified firmware > version. As the firmware blob contains the version, we use > that to filter out the ones we don't want. The version encoded > into the firmware name is superfluous and we should allow user > to point into specific firmware through a symlink, and only do > filtering based on the version stamp included in the blob. > This allows user to conveniently point to a firmware blob and > still gives us the control of what we decided to run on. > > This is partial revert of > 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") > > Fixes: 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") > References: https://bugs.freedesktop.org/show_bug.cgi?id=96800 > Reported-by: Mike Lothian <mike@fireburn.co.uk> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_csr.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > b/drivers/gpu/drm/i915/intel_csr.c > index ea047cd46b71..77d01a0b64b4 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_01.bin" > +#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin" > MODULE_FIRMWARE(I915_CSR_KBL); > #define KBL_CSR_VERSION_REQUIRED CSR_VERSION(1, 1) > > -#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin" > +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin" > MODULE_FIRMWARE(I915_CSR_SKL); > #define SKL_CSR_VERSION_REQUIRED CSR_VERSION(1, 26) > > -#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin" > +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin" > MODULE_FIRMWARE(I915_CSR_BXT); > #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) >
"Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > Nak. > > I don't intend to update the symbolic links on linux-firmware.git > repository anymore so if we receive a new minor version update we are > not going to load. > > I was the one advocating in the favor for the symbolic link flexibility > but I lost the discussions for the stability and validation etc. > And I was one advocating in favor of getting rid of symlink. But the filename versioning is superfluous as the contents has the version info which we can solely rely to not run something we dont want. So I am not sure what we lose in stability and validation front with the strict version check. -Mika > So let's just move away from symbolic link for good. > > > On Tue, 2016-07-05 at 12:25 +0300, Mika Kuoppala wrote: >> We need the ability to explicitly load only a specified firmware >> version. As the firmware blob contains the version, we use >> that to filter out the ones we don't want. The version encoded >> into the firmware name is superfluous and we should allow user >> to point into specific firmware through a symlink, and only do >> filtering based on the version stamp included in the blob. >> This allows user to conveniently point to a firmware blob and >> still gives us the control of what we decided to run on. >> >> This is partial revert of >> 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") >> >> Fixes: 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") >> References: https://bugs.freedesktop.org/show_bug.cgi?id=96800 >> Reported-by: Mike Lothian <mike@fireburn.co.uk> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> drivers/gpu/drm/i915/intel_csr.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_csr.c >> b/drivers/gpu/drm/i915/intel_csr.c >> index ea047cd46b71..77d01a0b64b4 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_01.bin" >> +#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin" >> MODULE_FIRMWARE(I915_CSR_KBL); >> #define KBL_CSR_VERSION_REQUIRED CSR_VERSION(1, 1) >> >> -#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin" >> +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin" >> MODULE_FIRMWARE(I915_CSR_SKL); >> #define SKL_CSR_VERSION_REQUIRED CSR_VERSION(1, 26) >> >> -#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin" >> +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin" >> MODULE_FIRMWARE(I915_CSR_BXT); >> #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) >>
On Thu, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > "Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > > > > > Nak. > > > > I don't intend to update the symbolic links on linux-firmware.git > > repository anymore so if we receive a new minor version update we > > are > > not going to load. > > > > I was the one advocating in the favor for the symbolic link > > flexibility > > but I lost the discussions for the stability and validation etc. > > > And I was one advocating in favor of getting rid of symlink. oh true! indeed funny! :) > But the > filename versioning is superfluous as the contents has the version > info > which we can solely rely to not run something we dont want. indeed. > > So I am not sure what we lose in stability and validation front > with the strict version check. But I'm not sure what we gain though. Is the reason https://bugs.freedesktop.org/show_bug.cgi?id=96800 ? If the reason is that CONFIG_EXTRA_FIRMWARE variable if we increase the major version we also cause an issue anyways. But also, we don't know the version the users using the CONFIG_EXTRA_FIRMWARE is compiling and what exact version of the firmware that source in that specific moment is requiring. How would we be sure the users has the symbolic link pointing to the right version that code is requiring? > -Mika > > > > > So let's just move away from symbolic link for good. > > > > > > On Tue, 2016-07-05 at 12:25 +0300, Mika Kuoppala wrote: > > > > > > We need the ability to explicitly load only a specified firmware > > > version. As the firmware blob contains the version, we use > > > that to filter out the ones we don't want. The version encoded > > > into the firmware name is superfluous and we should allow user > > > to point into specific firmware through a symlink, and only do > > > filtering based on the version stamp included in the blob. > > > This allows user to conveniently point to a firmware blob and > > > still gives us the control of what we decided to run on. > > > > > > This is partial revert of > > > 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") > > > > > > Fixes: 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic > > > links") > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=96800 > > > Reported-by: Mike Lothian <mike@fireburn.co.uk> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_csr.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > > b/drivers/gpu/drm/i915/intel_csr.c > > > index ea047cd46b71..77d01a0b64b4 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_01.bin" > > > +#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin" > > > MODULE_FIRMWARE(I915_CSR_KBL); > > > #define KBL_CSR_VERSION_REQUIRED CSR_VERSION(1, 1) > > > > > > -#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin" > > > +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin" > > > MODULE_FIRMWARE(I915_CSR_SKL); > > > #define SKL_CSR_VERSION_REQUIRED CSR_VERSION(1, 26) > > > > > > -#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin" > > > +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin" > > > MODULE_FIRMWARE(I915_CSR_BXT); > > > #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) > > >
On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > "Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > > > Nak. > > > > I don't intend to update the symbolic links on linux-firmware.git > > repository anymore so if we receive a new minor version update we > > are > > not going to load. > > > > I was the one advocating in the favor for the symbolic link > > flexibility > > but I lost the discussions for the stability and validation etc. > > > > And I was one advocating in favor of getting rid of symlink. But the > filename versioning is superfluous as the contents has the version > info > which we can solely rely to not run something we dont want. > > So I am not sure what we lose in stability and validation front > with the strict version check. Bisection is more cumbersome with a symlink. --Imre > -Mika > > > So let's just move away from symbolic link for good. > > > > > > On Tue, 2016-07-05 at 12:25 +0300, Mika Kuoppala wrote: > > > We need the ability to explicitly load only a specified firmware > > > version. As the firmware blob contains the version, we use > > > that to filter out the ones we don't want. The version encoded > > > into the firmware name is superfluous and we should allow user > > > to point into specific firmware through a symlink, and only do > > > filtering based on the version stamp included in the blob. > > > This allows user to conveniently point to a firmware blob and > > > still gives us the control of what we decided to run on. > > > > > > This is partial revert of > > > 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") > > > > > > Fixes: 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic > > > links") > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=96800 > > > Reported-by: Mike Lothian <mike@fireburn.co.uk> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_csr.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_csr.c > > > b/drivers/gpu/drm/i915/intel_csr.c > > > index ea047cd46b71..77d01a0b64b4 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_01.bin" > > > +#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin" > > > MODULE_FIRMWARE(I915_CSR_KBL); > > > #define KBL_CSR_VERSION_REQUIRED CSR_VERSION(1, 1) > > > > > > -#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin" > > > +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin" > > > MODULE_FIRMWARE(I915_CSR_SKL); > > > #define SKL_CSR_VERSION_REQUIRED CSR_VERSION(1, 26) > > > > > > -#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin" > > > +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin" > > > MODULE_FIRMWARE(I915_CSR_BXT); > > > #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7) > > >
On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > "Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > > > > > Nak. > > > > > > I don't intend to update the symbolic links on linux-firmware.git > > > repository anymore so if we receive a new minor version update we > > > are > > > not going to load. > > > > > > I was the one advocating in the favor for the symbolic link > > > flexibility > > > but I lost the discussions for the stability and validation etc. > > > > > > > And I was one advocating in favor of getting rid of symlink. But the > > filename versioning is superfluous as the contents has the version > > info > > which we can solely rely to not run something we dont want. > > > > So I am not sure what we lose in stability and validation front > > with the strict version check. > > Bisection is more cumbersome with a symlink. Did you miss a without there? Because when bisecting the kernel it's harder without the symlink as the build breaks otherwise and the runtime is not bisectable either. -Chris
On ma, 2016-07-11 at 13:39 +0100, chris@chris-wilson.co.uk wrote: > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > "Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > > > > > > > Nak. > > > > > > > > I don't intend to update the symbolic links on linux- > > > > firmware.git > > > > repository anymore so if we receive a new minor version update > > > > we > > > > are > > > > not going to load. > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > flexibility > > > > but I lost the discussions for the stability and validation > > > > etc. > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. But > > > the > > > filename versioning is superfluous as the contents has the > > > version > > > info > > > which we can solely rely to not run something we dont want. > > > > > > So I am not sure what we lose in stability and validation front > > > with the strict version check. > > > > Bisection is more cumbersome with a symlink. > > Did you miss a without there? Because when bisecting the kernel it's > harder without the symlink as the build breaks otherwise and the > runtime > is not bisectable either. No, I meant when bisecting we also want to load the proper fw version for each bisect commit point. So using exact filenames we'll automatically load the proper firmware file for a given commit, whereas with symlinks we have to adjust the symlink at each bisect step. You need to have all the required FW versions for the bisect range to be present on the filesystem of course. --Imre
On Mon, Jul 11, 2016 at 03:45:21PM +0300, Imre Deak wrote: > On ma, 2016-07-11 at 13:39 +0100, chris@chris-wilson.co.uk wrote: > > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > > "Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > > > > > > > > > Nak. > > > > > > > > > > I don't intend to update the symbolic links on linux- > > > > > firmware.git > > > > > repository anymore so if we receive a new minor version update > > > > > we > > > > > are > > > > > not going to load. > > > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > > flexibility > > > > > but I lost the discussions for the stability and validation > > > > > etc. > > > > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. But > > > > the > > > > filename versioning is superfluous as the contents has the > > > > version > > > > info > > > > which we can solely rely to not run something we dont want. > > > > > > > > So I am not sure what we lose in stability and validation front > > > > with the strict version check. > > > > > > Bisection is more cumbersome with a symlink. > > > > Did you miss a without there? Because when bisecting the kernel it's > > harder without the symlink as the build breaks otherwise and the > > runtime > > is not bisectable either. > > No, I meant when bisecting we also want to load the proper fw version > for each bisect commit point. So using exact filenames we'll > automatically load the proper firmware file for a given commit, whereas > with symlinks we have to adjust the symlink at each bisect step. You mean you have to adjust the kernel build at each point. That is the root of the problem as we do not have deferred firmware loading. And then you get random changes in the firmare whilst bisecting the kernel. -Chris
On ma, 2016-07-11 at 13:55 +0100, chris@chris-wilson.co.uk wrote: > On Mon, Jul 11, 2016 at 03:45:21PM +0300, Imre Deak wrote: > > On ma, 2016-07-11 at 13:39 +0100, chris@chris-wilson.co.uk wrote: > > > On Mon, Jul 11, 2016 at 02:23:48PM +0300, Imre Deak wrote: > > > > On to, 2016-07-07 at 17:57 +0300, Mika Kuoppala wrote: > > > > > "Vivi, Rodrigo" <rodrigo.vivi@intel.com> writes: > > > > > > > > > > > Nak. > > > > > > > > > > > > I don't intend to update the symbolic links on linux- > > > > > > firmware.git > > > > > > repository anymore so if we receive a new minor version > > > > > > update > > > > > > we > > > > > > are > > > > > > not going to load. > > > > > > > > > > > > I was the one advocating in the favor for the symbolic link > > > > > > flexibility > > > > > > but I lost the discussions for the stability and validation > > > > > > etc. > > > > > > > > > > > > > > > > And I was one advocating in favor of getting rid of symlink. > > > > > But > > > > > the > > > > > filename versioning is superfluous as the contents has the > > > > > version > > > > > info > > > > > which we can solely rely to not run something we dont want. > > > > > > > > > > So I am not sure what we lose in stability and validation > > > > > front > > > > > with the strict version check. > > > > > > > > Bisection is more cumbersome with a symlink. > > > > > > Did you miss a without there? Because when bisecting the kernel > > > it's > > > harder without the symlink as the build breaks otherwise and the > > > runtime > > > is not bisectable either. > > > > No, I meant when bisecting we also want to load the proper fw > > version > > for each bisect commit point. So using exact filenames we'll > > automatically load the proper firmware file for a given commit, > > whereas > > with symlinks we have to adjust the symlink at each bisect step. > > You mean you have to adjust the kernel build at each point. That is > the > root of the problem as we do not have deferred firmware loading. Yes. Looking at the bug now: it's true that using CONFIG_EXTRA_FIRMWARE you have to update this config option at each bisect point, but you would have to anyway update the symlink in that case too. > And then you get random changes in the firmare whilst bisecting the > kernel. What do you mean random? During bisecting we want to load the firmware version that was used with a particular commit. With a symlink pointing to the wrong firmware file for a given commit, we'd fail loading the firmware due to the version check and hide/introduce bugs for that commit. --Imre
On Mon, Jul 11, 2016 at 04:24:50PM +0300, Imre Deak wrote: > On ma, 2016-07-11 at 13:55 +0100, chris@chris-wilson.co.uk wrote: > > And then you get random changes in the firmare whilst bisecting the > > kernel. > > What do you mean random? During bisecting we want to load the firmware > version that was used with a particular commit. With a symlink pointing > to the wrong firmware file for a given commit, we'd fail loading the > firmware due to the version check and hide/introduce bugs for that > commit. No. You want to be changing exactly one variable, which means leaving the firmware constant. The firmware should be side-ways compatible for everything with the same minor version (thus resolvable from the same symlink), right? -Chris
On ma, 2016-07-11 at 14:50 +0100, chris@chris-wilson.co.uk wrote: > On Mon, Jul 11, 2016 at 04:24:50PM +0300, Imre Deak wrote: > > On ma, 2016-07-11 at 13:55 +0100, chris@chris-wilson.co.uk wrote: > > > And then you get random changes in the firmare whilst bisecting > > > the > > > kernel. > > > > What do you mean random? During bisecting we want to load the > > firmware > > version that was used with a particular commit. With a symlink > > pointing > > to the wrong firmware file for a given commit, we'd fail loading > > the > > firmware due to the version check and hide/introduce bugs for that > > commit. > > No. You want to be changing exactly one variable, which means leaving > the firmware constant. Hm, not sure. When looking for a working snapshot you also want to consider bugs introduced by the firmware itself. This is in a way the exact reason why we want stricter control on the firmware version and introduced a white list. This also means that loading a firmware version other than what the driver allows (at a given commit) won't work anyway. > The firmware should be side-ways compatible for > everything with the same minor version (thus resolvable from the same > symlink), right? From the same major version I guess it should, but the reason things don't work that way is why we introduced version white listing. --Imre
Versioning dependencies isn't exactly a new problem outside the kernel, so please allow me to share my experience below. Thanks to Rodrigo for glancing over this email and preventing me from writing anything too stupid or that was said already. >> The firmware should be side-ways compatible for >> everything with the same minor version (thus resolvable from the same >> symlink), right? > > From the same major version I guess it should, but the reason things > don't work that way is why we introduced version white listing. In an ideal world you look only at the major version of your dependencies since they mark ABI changes. This is what the firmware symbolic links do. Back to the real world, some minor versions are so bad that you really don't want to inflict them on your users. So you go and blacklist at the hardest level: in the source code itself. Fine. However keep in mind blacklisting should stay exceptional since leaking system configuration into source code means taking away from users control they normally have. You need a pretty strong reason for this like some very obscure bugs that can't be Googled easily or some "less than alpha" version. Please keep in mind it's a hack/workaround: blacklisting shouldn't never become a normal policy. On the other hand, never WHITE-list since it blocks users from *UPGRADING* the dependency. If you're tempted to white-list the most current version then just blacklist all versions older than today and job done. White-listing is making a very bold and most likely wrong statement about the future: that no future revision of the dependency will ever be a pure bugfix and fully compatible with the current revision. Please don't try to abstract away the independent versioning of the firmware and make it look like firmware and kernel are developed, versioned, released and validated on the same cycles and by the same team. Granted: it's important to publish which versions were validated together. However most software projects don't embed their validation results in their source code as a default policy, there are better ways to document things like this. >> No. You want to be changing exactly one variable, which means leaving >> the firmware constant. Yes! Constant at least for a range large enough for bisections to be practical (ABI changes happen too). > Hm, not sure. When looking for a working snapshot you also want to > consider bugs introduced by the firmware itself. This is in a way the > exact reason why we want stricter control on the firmware version and > introduced a white list. This also means that loading a firmware > version other than what the driver allows (at a given commit) won't > work anyway. Please trust and respect your users. If someone is technical and motivated enough to bisect the kernel and/or the firmware then please don't get in her way and trust that she will: 1. try the latest releases first, and 2. more generally have some clue about what she's doing. Or at least learn quickly enough. Again feel free to blacklist the known broken past but don't block the unknown future, thanks! Marc
Marc kicked me to the other side of the fence. I'm now cheering for the symbolic links back again. We cannot block users to use some new firmware version if they like/want/need. Also, also as Chris highlighted the hardcoded version doesn't help the bisect but make it unlikely. I don't believe any users out there will save and install all firmware versions when bisecting. This is not a new problem and we can use the linux libraries as an example here. like libmeh.so.2.1, libmeh.so.2 link and libmeh.so link. We don't hardcode all userspace libraries in the userspace side for the graphics stack and we do not validate all possible combinations of libdrm, mesa, ddx, libva, etc... Why should we need this with firmware? Also I'd like to take this libmeh.so example here and go even further with the symbolic link fixing https://bugs.freedesktop.org/show_bug.cgi?id=96800 in the right way: - providing a firmware_platform.bin that is a symbolic link to the lastest firmware_platform_<abi version>.bin (our soname) that is a symbolic link to firmware_platform_<abi version>_<release number>.bin Our driver should also search for the firmware_platform.bin and have a check for the minimal version like userspace libraries does. In any case our drm_debug should have the information of the version loaded and version expected, in case users find issues that leads us to blame the firmware versions. Thoughts? On Tue, 2016-07-19 at 14:58 -0700, Herbert, Marc wrote: > Versioning dependencies isn't exactly a new problem outside the > kernel, > so please allow me to share my experience below. Thanks to Rodrigo > for > glancing over this email and preventing me from writing anything too > stupid or that was said already. > > > > > > > > > > The firmware should be side-ways compatible for > > > everything with the same minor version (thus resolvable from the > > > same > > > symlink), right? > > From the same major version I guess it should, but the reason > > things > > don't work that way is why we introduced version white listing. > In an ideal world you look only at the major version of your > dependencies > since they mark ABI changes. This is what the firmware symbolic links > do. > > Back to the real world, some minor versions are so bad that you > really don't want to inflict them on your users. So you go and > blacklist at > the hardest level: in the source code itself. Fine. > However keep in mind blacklisting should stay exceptional since > leaking > system configuration into source code means taking away from users > control > they normally have. You need a pretty strong reason for this like > some very > obscure bugs that can't be Googled easily or some "less than alpha" > version. > Please keep in mind it's a hack/workaround: blacklisting shouldn't > never become a normal policy. > > On the other hand, never WHITE-list since it blocks users from > *UPGRADING* > the dependency. > > If you're tempted to white-list the most current version then just > blacklist > all versions older than today and job done. White-listing is making a > very bold > and most likely wrong statement about the future: that no future > revision > of the dependency will ever be a pure bugfix and fully compatible > with the > current revision. > > Please don't try to abstract away the independent versioning of the > firmware and make it look like firmware and kernel are developed, > versioned, > released and validated on the same cycles and by the same team. > Granted: it's important to publish which versions > were validated together. However most software projects don't embed > their validation results in their source code as a default policy, > there are better ways to document things like this. > > > > > > > > > No. You want to be changing exactly one variable, which means > > > leaving > > > the firmware constant. > Yes! Constant at least for a range large enough for bisections to be > practical (ABI changes happen too). > > > > > Hm, not sure. When looking for a working snapshot you also want to > > consider bugs introduced by the firmware itself. This is in a way > > the > > exact reason why we want stricter control on the firmware version > > and > > introduced a white list. This also means that loading a firmware > > version other than what the driver allows (at a given commit) won't > > work anyway. > Please trust and respect your users. If someone is technical and > motivated > enough to bisect the kernel and/or the firmware then please don't get > in her way and trust that she will: 1. try the latest releases first, > and 2. more generally have some clue about what she's doing. Or at > least learn quickly enough. > > Again feel free to blacklist the known broken past but don't block > the > unknown future, thanks! > > Marc >
If you look back we did have both no-black listing (only required a proper major version) and black listing before we ended up deciding that we'll use white listing. The reasons for this were too many obscure firmware issues, undocumented interoperability details between firmware and the driver and as a result what we saw the only viable way to validate driver functionality and deal with bug reports. --Imre On ke, 2016-07-20 at 01:39 +0300, Vivi, Rodrigo wrote: > Marc kicked me to the other side of the fence. I'm now cheering for > the > symbolic links back again. > > We cannot block users to use some new firmware version if they > like/want/need. > > Also, also as Chris highlighted the hardcoded version doesn't help > the > bisect but make it unlikely. I don't believe any users out there will > save and install all firmware versions when bisecting. > > This is not a new problem and we can use the linux libraries as an > example here. like libmeh.so.2.1, libmeh.so.2 link and libmeh.so > link. > > We don't hardcode all userspace libraries in the userspace side for > the graphics stack and we do not validate all possible combinations > of libdrm, mesa, ddx, libva, etc... Why should we need this with > firmware? > > Also I'd like to take this libmeh.so example here and go even further > with the symbolic link fixing https://bugs.freedesktop.org/show_bug.c > gi?id=96800 in the right way: > > - providing a firmware_platform.bin that is a symbolic link to the > lastest firmware_platform_<abi version>.bin (our soname) that is a > symbolic link to firmware_platform_<abi version>_<release > number>.bin > > Our driver should also search for the firmware_platform.bin and have > a > check for the minimal version like userspace libraries does. > > In any case our drm_debug should have the information of the version > loaded and version expected, in case users find issues that leads us > to > blame the firmware versions. > > Thoughts? > > > On Tue, 2016-07-19 at 14:58 -0700, Herbert, Marc wrote: > > Versioning dependencies isn't exactly a new problem outside the > > kernel, > > so please allow me to share my experience below. Thanks to Rodrigo > > for > > glancing over this email and preventing me from writing anything > > too > > stupid or that was said already. > > > > > > > > > > > > > > > The firmware should be side-ways compatible for > > > > everything with the same minor version (thus resolvable from > > > > the > > > > same > > > > symlink), right? > > > From the same major version I guess it should, but the reason > > > things > > > don't work that way is why we introduced version white listing. > > In an ideal world you look only at the major version of your > > dependencies > > since they mark ABI changes. This is what the firmware symbolic > > links > > do. > > > > Back to the real world, some minor versions are so bad that you > > really don't want to inflict them on your users. So you go and > > blacklist at > > the hardest level: in the source code itself. Fine. > > However keep in mind blacklisting should stay exceptional since > > leaking > > system configuration into source code means taking away from users > > control > > they normally have. You need a pretty strong reason for this like > > some very > > obscure bugs that can't be Googled easily or some "less than alpha" > > version. > > Please keep in mind it's a hack/workaround: blacklisting shouldn't > > never become a normal policy. > > > > On the other hand, never WHITE-list since it blocks users from > > *UPGRADING* > > the dependency. > > > > If you're tempted to white-list the most current version then just > > blacklist > > all versions older than today and job done. White-listing is making > > a > > very bold > > and most likely wrong statement about the future: that no future > > revision > > of the dependency will ever be a pure bugfix and fully compatible > > with the > > current revision. > > > > Please don't try to abstract away the independent versioning of the > > firmware and make it look like firmware and kernel are developed, > > versioned, > > released and validated on the same cycles and by the same team. > > Granted: it's important to publish which versions > > were validated together. However most software projects don't embed > > their validation results in their source code as a default policy, > > there are better ways to document things like this. > > > > > > > > > > > > > No. You want to be changing exactly one variable, which means > > > > leaving > > > > the firmware constant. > > Yes! Constant at least for a range large enough for bisections to > > be > > practical (ABI changes happen too). > > > > > > > > Hm, not sure. When looking for a working snapshot you also want > > > to > > > consider bugs introduced by the firmware itself. This is in a way > > > the > > > exact reason why we want stricter control on the firmware version > > > and > > > introduced a white list. This also means that loading a firmware > > > version other than what the driver allows (at a given commit) > > > won't > > > work anyway. > > Please trust and respect your users. If someone is technical and > > motivated > > enough to bisect the kernel and/or the firmware then please don't > > get > > in her way and trust that she will: 1. try the latest releases > > first, > > and 2. more generally have some clue about what she's doing. Or at > > least learn quickly enough. > > > > Again feel free to blacklist the known broken past but don't block > > the > > unknown future, thanks! > > > > Marc
On Wed, 20 Jul 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: > We don't hardcode all userspace libraries in the userspace side for > the graphics stack and we do not validate all possible combinations of > libdrm, mesa, ddx, libva, etc... Why should we need this with > firmware? Because the firmware blob is more like a binary kernel module that works with a specific kernel version than an open source userspace component written on top of a stable ABI. You do not know what the firmware does, nor what the future versions of it will do. The kernel provides an ABI with a strict no regressions policy for its users. The firmware has no such guarantees, and it is expected to go hand in hand with the operating system versions it has been validated against. And as I've explained numerous times, we do not have the resources to validate all kernel releases against all firmware releases. BR, Jani.
So, what should we do in cases like this missed 1.23? Close the bug as wontfix? We are blocking users from upgrade the component, or worst, like in this case where 1.23 was causing bugs we are removing at all and preventing the user to have the extra power savings with another stable version of the firmware. -----Original Message----- From: Nikula, Jani Sent: Monday, August 01, 2016 6:25 AM To: Vivi, Rodrigo; Herbert, Marc; Deak, Imre; chris@chris-wilson.co.uk Cc: intel-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/i915/dmc: Accept symbolic link in firmware name On Wed, 20 Jul 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: > We don't hardcode all userspace libraries in the userspace side for > the graphics stack and we do not validate all possible combinations of > libdrm, mesa, ddx, libva, etc... Why should we need this with > firmware? Because the firmware blob is more like a binary kernel module that works with a specific kernel version than an open source userspace component written on top of a stable ABI. You do not know what the firmware does, nor what the future versions of it will do. The kernel provides an ABI with a strict no regressions policy for its users. The firmware has no such guarantees, and it is expected to go hand in hand with the operating system versions it has been validated against. And as I've explained numerous times, we do not have the resources to validate all kernel releases against all firmware releases. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
On Wed, Aug 03, 2016 at 06:00:14AM +0000, Vivi, Rodrigo wrote: > So, what should we do in cases like this missed 1.23? > Close the bug as wontfix? > > We are blocking users from upgrade the component, or worst, like in this > case where 1.23 was causing bugs we are removing at all and preventing > the user to have the extra power savings with another stable version of > the firmware. Restore the 1.23 version to the linux firmware repo imo. It's a regression, best way to fix it is by (partial) revert. -Daniel > > -----Original Message----- > From: Nikula, Jani > Sent: Monday, August 01, 2016 6:25 AM > To: Vivi, Rodrigo; Herbert, Marc; Deak, Imre; chris@chris-wilson.co.uk > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH] drm/i915/dmc: Accept symbolic link in firmware name > > On Wed, 20 Jul 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: > > We don't hardcode all userspace libraries in the userspace side for > > the graphics stack and we do not validate all possible combinations of > > libdrm, mesa, ddx, libva, etc... Why should we need this with > > firmware? > > Because the firmware blob is more like a binary kernel module that works with a specific kernel version than an open source userspace component written on top of a stable ABI. > > You do not know what the firmware does, nor what the future versions of it will do. The kernel provides an ABI with a strict no regressions policy for its users. The firmware has no such guarantees, and it is expected to go hand in hand with the operating system versions it has been validated against. And as I've explained numerous times, we do not have the resources to validate all kernel releases against all firmware releases. > > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 03 Aug 2016, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Aug 03, 2016 at 06:00:14AM +0000, Vivi, Rodrigo wrote: >> So, what should we do in cases like this missed 1.23? >> Close the bug as wontfix? >> >> We are blocking users from upgrade the component, or worst, like in this >> case where 1.23 was causing bugs we are removing at all and preventing >> the user to have the extra power savings with another stable version of >> the firmware. > > Restore the 1.23 version to the linux firmware repo imo. It's a > regression, best way to fix it is by (partial) revert. Yes, if there's a kernel out there that requires firmware 1.23, you need to keep that version of the firmware in linux-firmware. This should be obvious. If you manage the firmware version bumps in kernel commits properly, you can backport the firmware version bumps to appropriate stable kernels, *after* you've ensured it doesn't cause other problems. I don't think we should encourage users to upgrade firmware on their own. It'll just blow up the firmware/kernel combinations and make it harder for us to manage the bugs. For testing, we could add a module parameter to have the driver request a specific version of the firmware. _unsafe, of course. Also, I never argued we should only accept *one* version. I believe we could try one, and if it's not there, try another. Both tested versions against that specific kernel. But this should be limited to not blow up the combinations. BR, Jani. > -Daniel > >> >> -----Original Message----- >> From: Nikula, Jani >> Sent: Monday, August 01, 2016 6:25 AM >> To: Vivi, Rodrigo; Herbert, Marc; Deak, Imre; chris@chris-wilson.co.uk >> Cc: intel-gfx@lists.freedesktop.org >> Subject: Re: [PATCH] drm/i915/dmc: Accept symbolic link in firmware name >> >> On Wed, 20 Jul 2016, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: >> > We don't hardcode all userspace libraries in the userspace side for >> > the graphics stack and we do not validate all possible combinations of >> > libdrm, mesa, ddx, libva, etc... Why should we need this with >> > firmware? >> >> Because the firmware blob is more like a binary kernel module that works with a specific kernel version than an open source userspace component written on top of a stable ABI. >> >> You do not know what the firmware does, nor what the future versions of it will do. The kernel provides an ABI with a strict no regressions policy for its users. The firmware has no such guarantees, and it is expected to go hand in hand with the operating system versions it has been validated against. And as I've explained numerous times, we do not have the resources to validate all kernel releases against all firmware releases. >> >> >> BR, >> Jani. >> >> >> -- >> Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index ea047cd46b71..77d01a0b64b4 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_01.bin" +#define I915_CSR_KBL "i915/kbl_dmc_ver1.bin" MODULE_FIRMWARE(I915_CSR_KBL); #define KBL_CSR_VERSION_REQUIRED CSR_VERSION(1, 1) -#define I915_CSR_SKL "i915/skl_dmc_ver1_26.bin" +#define I915_CSR_SKL "i915/skl_dmc_ver1.bin" MODULE_FIRMWARE(I915_CSR_SKL); #define SKL_CSR_VERSION_REQUIRED CSR_VERSION(1, 26) -#define I915_CSR_BXT "i915/bxt_dmc_ver1_07.bin" +#define I915_CSR_BXT "i915/bxt_dmc_ver1.bin" MODULE_FIRMWARE(I915_CSR_BXT); #define BXT_CSR_VERSION_REQUIRED CSR_VERSION(1, 7)
We need the ability to explicitly load only a specified firmware version. As the firmware blob contains the version, we use that to filter out the ones we don't want. The version encoded into the firmware name is superfluous and we should allow user to point into specific firmware through a symlink, and only do filtering based on the version stamp included in the blob. This allows user to conveniently point to a firmware blob and still gives us the control of what we decided to run on. This is partial revert of 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") Fixes: 4aa7fb9c3c4f ("drm/i915/dmc: Step away from symbolic links") References: https://bugs.freedesktop.org/show_bug.cgi?id=96800 Reported-by: Mike Lothian <mike@fireburn.co.uk> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_csr.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)