Allow disabling per-Gen hardware support; add CI testing #1304
Conversation
|
tap tap Is this thing on? |
| @@ -18,6 +18,8 @@ | |||
| # ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR | |||
| # OTHER DEALINGS IN THE SOFTWARE. | |||
|
|
|||
| option(BROKEN "Accept potentially broken builds when disabling generation-specific options" OFF) | |||
There was a problem hiding this comment.
BROKEN=ON, then could enable per-gen build. what's the motivation about this build option, why GENXX = OFF is not enough?
There was a problem hiding this comment.
Currently the per-Gen build options are broken, except for Gen8.
But the per-Gen build options control two separate things: 1. The per-Gen media kernel binaries, and 2. The per-Gen driver code.
The per-Gen driver code cannot be disabled arbitrarily now.
The per-Gen media kernel binaries can be disabled arbitrarily, and they are large and disabling them provides large file size reductions.
The BROKEN option controls whether the per-Gen driver code is affected by the per-Gen config options, allowing us to disable the per-Gen media kernel binaries.
There was a problem hiding this comment.
I understand the desire to have options to reduce driver footprint, but it's not good idea to workaround something which should actually be fixed. Issue with gen10 switch off is a clear issue and needs proper resolution.
I think we should not introduce BROKEN option. Instead, I suggest to start introducing new build configurations (w/ disabled gens) which currently works to make sure we don't regress. For example, -DGEN8=OFF got fixed, let's have this path covered in github actions ci. And add other configs which currently build ok.
There was a problem hiding this comment.
I have some work in progress patches to fix Gen9, but I don't expect to be able to fix disabling the driver code for Gen10, Gen11, or Gen12.
Is someone from Intel going to sign up for that?
We can of course ship my patch as is in ChromeOS to be able to reduce the binary size, but I feel like I've provided a solid and incremental path forward that gets 90% of the size reduction, strictly improves the situation, and doesn't force anyone to drop what they're doing to untangle the currently broken per-Gen driver code disablement.
Alternatively, I'm happy to either
- rename the
BROKENoption if the name is the problem - or I can remove
BROKENentirely and make the per-Gen driver code that is currently required non-optional, leaving theGEN*options to only disable the media kernel binaries. I create theBROKENoption to make the incremental work of fixing the remaining Gen options simpler.
There was a problem hiding this comment.
- rename the BROKEN option if the name is the problem
Having received no feedback for a month, I've speculatively made this change, changing the option name to -DONLY_MEDIA_KERNELS and inverting the value (so that you have to set it to OFF to produce a broken build with -DGEN*=OFF).
|
Anyone want to continue the discussions? |
171dcd0 to
6cc81d6
Compare
6cc81d6 to
60c8489
Compare
|
2 build failure. |
60c8489 to
8ac380b
Compare
|
Okay, rebased onto master, updated for some platform additions, and fixed the two build failures. Looks like I need someone to approve the workflow to run the CI again. |
8ac380b to
650fce2
Compare
More fixes. Should be good to go now. |
650fce2 to
4bb9cb0
Compare
4bb9cb0 to
76b8cc6
Compare
My primary motivation in using these options is to reduce on-disk driver
size. The media kernel binaries needed for video encoding are specific
to each Gen and are also large. Allowing the builder to disable the
media kernel binaries while we work to resolve the compilation problems
involved with disabling per-Gen driver code in the still provides
significant savings.
To do this, I add an ENABLE_REQUIRED_GEN_CODE option that when set to ON
causes CMake to override the GEN options that are known to be needed for
a successful build. The result is that -DGEN*=OFF produces always
produces a working build, and with testing in place (see next patch), we
can iteratively fix the build failures while ensuring that working
configurations do not regress.
By not modifying the per-Gen conditionals in media_gen_flags_linux.cmake
that control the IGFX_*_SUPPORTED preprocessor definitions, we safely
disable the per-Gen media kernel binaries with -DGEN*=OFF.
size(1) shows the savings, with all Gens enabled and disabled for
baseline comparisons:
text data bss dec hex filename
34731583 2127600 9568 36868751 232928f iHD_drv_video.so # -DGEN8=ON -DGEN9=ON -DGEN10=ON -DGEN11=ON -DGEN12=ON
16329447 259344 9760 16598551 fd4617 iHD_drv_video.so # -DGEN8=OFF -DGEN9=OFF -DGEN10=OFF -DGEN11=OFF -DGEN12=OFF
Enabling only a single Gen shows how much of the binary size is
contributed by per-Gen media kernels:
text data bss dec hex filename
19994303 1170608 9664 21174575 143192f iHD_drv_video.so # -DGEN8=OFF -DGEN9=OFF -DGEN10=OFF -DGEN11=OFF -DGEN12=ON
19464071 1079120 9696 20552887 1399cb7 iHD_drv_video.so # -DGEN8=OFF -DGEN9=OFF -DGEN10=OFF -DGEN11=ON -DGEN12=OFF
20239815 396592 9760 20646167 13b0917 iHD_drv_video.so # -DGEN8=OFF -DGEN9=OFF -DGEN10=ON -DGEN11=OFF -DGEN12=OFF
21424711 259344 9760 21693815 14b0577 iHD_drv_video.so # -DGEN8=OFF -DGEN9=ON -DGEN10=OFF -DGEN11=OFF -DGEN12=OFF
18926471 259344 9760 19195575 124e6b7 iHD_drv_video.so # -DGEN8=ON -DGEN9=OFF -DGEN10=OFF -DGEN11=OFF -DGEN12=OFF
That is the media kernels add 4.36 MiB for Gen12, 3.77 MiB for Gen11,
3.86 MiB for Gen10, 4.85 MiB for Gen9, 2.47 MiB for Gen8.
Cannonlake (Gen10) GPUs never shipped. There's no point in shipping 3.86
MiB of media kernels that can never be used.
text data bss dec hex filename
34731583 2127600 9568 36868751 232928f iHD_drv_video.so # -DGEN10=ON
30821215 1990352 9600 32821167 1f4cfaf iHD_drv_video.so # -DGEN10=OFF
Also change the CI workflow from having a build with GEN10=OFF to a
build with GEN10=ON to test the non-default configuration option.
Presumably we'd like to fully remove this code at some point, but I'm
being conservative.
Currently Gen8 is the only Gen that can be fully disabled
(thanks to PR 1302).
Allowing the driver code to be disabled for Gen8 further reduces the
binary size by 277 KiB, or ~11% of the Gen8 media kernels:
text data bss dec hex filename
30821215 1990352 9600 32821167 1f4cfaf iHD_drv_video.so # -DGEN8=ON
28224191 1990384 9632 30224207 1cd2f4f iHD_drv_video.so # -DGEN8=OFF before this patch
27950563 1980440 9600 29940603 1c8db7b iHD_drv_video.so # -DGEN8=OFF after this patch
76b8cc6 to
db13972
Compare
|
Thank you. I'll open further PRs that allow disabling per-Gen driver code as I have time. Hopefully those will not take nearly as long to merge. |
Commit messages contain explanations and data.
tl;dr: This allows the
-DGEN*=OFFoptions to work by ignoring them in cases where we know the build is broken. That allows us to use them to disable the large per-Gen media kernels, which are 4.36 MiB for Gen12, 3.77 MiB for Gen11, 3.86 MiB for Gen10, 4.85 MiB for Gen9, 2.47 MiB for Gen8.CI builds are added so we don't regress. Two follow-on patches disable Gen10 by default (since it never shipped), and (re)disable the Gen8 driver code now that PR 1302 has landed.
cc: @edwarddavidbaker in case you're interested.