| Message ID | 20190326074057.27833-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Device id consolidation | expand |
On 26/03/2019 07:40, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Series removes device id checks from i915_drv.h macros and consolidates them to > i915_pciids.h as the main "database", while making intel_device_info.c reference > the former, expanding the existing concept of a platform mask by a few low bits > reserved for sub-platform mask. > > This has a two-fold positive effect of firstly consolidating the list of device > ids to one location, and secondly removing the if-ladders from every > IS_<platfrom>_<subplatform> call site. > > Maintenance burden is not completely removed but should be improved. One case in > point is that I have found some disagreements between device id listed in > i915_pciids.h and i915_drv.h. > > At the same time platform mask code is generalized to an array of u32 to > accomodate the addition of EHL and avoid spilling into u64 which would cause a > small code size increase. Downside is that any platforms on the u32 boundary, > like currently ICL and EHL, lose the benefit of optimizing the "IS_ICELAKE || > IS_ELKHARTLAKE" checks into a single conditional, although at the moment there > aren't any such call-sites. > > Before vs after for the whole series: > > text data bss dec hex filename > 1891093 43903 7424 1942420 1da394 i915.ko.0 > 1890434 43903 7424 1941761 1da101 i915.ko.1 > > add/remove: 12/3 grow/shrink: 92/121 up/down: 1974/-1769 (205) > ... > Total: Before=1286293, After=1286498, chg +0.02% After patch 4 v8: text data bss dec hex filename 1904423 43891 7424 1955738 1dd79a i915.ko.0 1903354 43891 7424 1954669 1dd36d i915.ko.1 add/remove: 8/3 grow/shrink: 94/124 up/down: 1623/-1889 (-266) ... Total: Before=1293823, After=1293557, chg -0.02% Regards, Tvrtko > Tvrtko Ursulin (4): > drm/i915: Split Pineview device info into desktop and mobile > drm/i915: Remove redundant device id from IS_IRONLAKE_M macro > drm/i915: Split some PCI ids into separate groups > drm/i915: Introduce concept of a sub-platform > > arch/x86/kernel/early-quirks.c | 3 +- > drivers/gpu/drm/i915/i915_drv.c | 8 +- > drivers/gpu/drm/i915/i915_drv.h | 133 +++++++++++------ > drivers/gpu/drm/i915/i915_gpu_error.c | 3 + > drivers/gpu/drm/i915/i915_pci.c | 14 +- > drivers/gpu/drm/i915/intel_device_info.c | 145 ++++++++++++++++++ > drivers/gpu/drm/i915/intel_device_info.h | 27 +++- > drivers/gpu/drm/i915/intel_pm.c | 4 +- > include/drm/i915_pciids.h | 179 ++++++++++++++++------- > 9 files changed, 414 insertions(+), 102 deletions(-) >
Quoting Tvrtko Ursulin (2019-03-28 09:23:24) > > On 26/03/2019 07:40, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > Series removes device id checks from i915_drv.h macros and consolidates them to > > i915_pciids.h as the main "database", while making intel_device_info.c reference > > the former, expanding the existing concept of a platform mask by a few low bits > > reserved for sub-platform mask. > > > > This has a two-fold positive effect of firstly consolidating the list of device > > ids to one location, and secondly removing the if-ladders from every > > IS_<platfrom>_<subplatform> call site. > > > > Maintenance burden is not completely removed but should be improved. One case in > > point is that I have found some disagreements between device id listed in > > i915_pciids.h and i915_drv.h. > > > > At the same time platform mask code is generalized to an array of u32 to > > accomodate the addition of EHL and avoid spilling into u64 which would cause a > > small code size increase. Downside is that any platforms on the u32 boundary, > > like currently ICL and EHL, lose the benefit of optimizing the "IS_ICELAKE || > > IS_ELKHARTLAKE" checks into a single conditional, although at the moment there > > aren't any such call-sites. > > > > Before vs after for the whole series: > > > > text data bss dec hex filename > > 1891093 43903 7424 1942420 1da394 i915.ko.0 > > 1890434 43903 7424 1941761 1da101 i915.ko.1 > > > > add/remove: 12/3 grow/shrink: 92/121 up/down: 1974/-1769 (205) > > ... > > Total: Before=1286293, After=1286498, chg +0.02% > > After patch 4 v8: > > text data bss dec hex filename > 1904423 43891 7424 1955738 1dd79a i915.ko.0 > 1903354 43891 7424 1954669 1dd36d i915.ko.1 > > add/remove: 8/3 grow/shrink: 94/124 up/down: 1623/-1889 (-266) > ... > Total: Before=1293823, After=1293557, chg -0.02% The series is an improvement, both for the reader and for the compiler, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On 28/03/2019 09:39, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-03-28 09:23:24) >> >> On 26/03/2019 07:40, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Series removes device id checks from i915_drv.h macros and consolidates them to >>> i915_pciids.h as the main "database", while making intel_device_info.c reference >>> the former, expanding the existing concept of a platform mask by a few low bits >>> reserved for sub-platform mask. >>> >>> This has a two-fold positive effect of firstly consolidating the list of device >>> ids to one location, and secondly removing the if-ladders from every >>> IS_<platfrom>_<subplatform> call site. >>> >>> Maintenance burden is not completely removed but should be improved. One case in >>> point is that I have found some disagreements between device id listed in >>> i915_pciids.h and i915_drv.h. >>> >>> At the same time platform mask code is generalized to an array of u32 to >>> accomodate the addition of EHL and avoid spilling into u64 which would cause a >>> small code size increase. Downside is that any platforms on the u32 boundary, >>> like currently ICL and EHL, lose the benefit of optimizing the "IS_ICELAKE || >>> IS_ELKHARTLAKE" checks into a single conditional, although at the moment there >>> aren't any such call-sites. >>> >>> Before vs after for the whole series: >>> >>> text data bss dec hex filename >>> 1891093 43903 7424 1942420 1da394 i915.ko.0 >>> 1890434 43903 7424 1941761 1da101 i915.ko.1 >>> >>> add/remove: 12/3 grow/shrink: 92/121 up/down: 1974/-1769 (205) >>> ... >>> Total: Before=1286293, After=1286498, chg +0.02% >> >> After patch 4 v8: >> >> text data bss dec hex filename >> 1904423 43891 7424 1955738 1dd79a i915.ko.0 >> 1903354 43891 7424 1954669 1dd36d i915.ko.1 >> >> add/remove: 8/3 grow/shrink: 94/124 up/down: 1623/-1889 (-266) >> ... >> Total: Before=1293823, After=1293557, chg -0.02% > > The series is an improvement, both for the reader and for the compiler, > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks. Jani, Lucas? Passable? Still some objections? Regards, Tvrtko
On 27/03/2019 18:38, Patchwork wrote: > == Series Details == > > Series: Device id consolidation (rev2) > URL : https://patchwork.freedesktop.org/series/58561/ > State : success > > == Summary == > > CI Bug Log - changes from CI_DRM_5827 -> Patchwork_12616 > ==================================================== > > Summary > ------- > > **SUCCESS** > > No regressions found. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/58561/revisions/2/mbox/ > > Known issues > ------------ > > Here are the changes found in Patchwork_12616 that come from known issues: > > ### IGT changes ### > > #### Issues hit #### > > * igt@amdgpu/amd_basic@userptr: > - fi-kbl-8809g: PASS -> DMESG-WARN [fdo#108965] > > * igt@gem_exec_store@basic-bsd2: > - fi-hsw-4770: NOTRUN -> SKIP [fdo#109271] +41 > > * igt@i915_selftest@live_execlists: > - fi-apl-guc: NOTRUN -> INCOMPLETE [fdo#103927] / [fdo#109720] > > * igt@kms_force_connector_basic@force-load-detect: > - fi-skl-6700k2: NOTRUN -> SKIP [fdo#109271] +26 > > * igt@kms_frontbuffer_tracking@basic: > - fi-icl-u2: PASS -> FAIL [fdo#103167] > > * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence: > - fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362] > > * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: > - fi-apl-guc: NOTRUN -> DMESG-WARN [fdo#108529] / [fdo#108566] > > * igt@kms_psr@primary_page_flip: > - fi-apl-guc: NOTRUN -> SKIP [fdo#109271] +50 > > > #### Possible fixes #### > > * igt@kms_chamelium@common-hpd-after-suspend: > - fi-skl-6700k2: INCOMPLETE [fdo#104108] -> PASS > > * igt@kms_pipe_crc_basic@hang-read-crc-pipe-b: > - fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS > > * igt@kms_pipe_crc_basic@read-crc-pipe-b: > - fi-byt-clapper: FAIL [fdo#107362] -> PASS > > > [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 > [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191 > [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927 > [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108 > [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362 > [fdo#108529]: https://bugs.freedesktop.org/show_bug.cgi?id=108529 > [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566 > [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965 > [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 > [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720 > > > Participating hosts (39 -> 35) > ------------------------------ > > Additional (2): fi-hsw-4770 fi-apl-guc > Missing (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-bdw-samus fi-kbl-r > > > Build changes > ------------- > > * Linux: CI_DRM_5827 -> Patchwork_12616 > > CI_DRM_5827: d90542b50bfba088d53e8f520c05080e5ca764e7 @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4909: 7df3eeb4f3360cd2b511c31acc1c52bd7ce6587f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_12616: 4f6f1609d5d18d208431f910a3b1e2d28cd623fa @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > 4f6f1609d5d1 drm/i915: Introduce concept of a sub-platform > 8551b470ae66 drm/i915: Split some PCI ids into separate groups > 871661fc5d65 drm/i915: Remove redundant device id from IS_IRONLAKE_M macro > 41661f3ceb00 drm/i915: Split Pineview device info into desktop and mobile Pushed, thanks for the feedback and review. If there will be any problems with Clang I am counting on people familiar with the problem space suggesting workarounds. Regards, Tvrtko
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Series removes device id checks from i915_drv.h macros and consolidates them to i915_pciids.h as the main "database", while making intel_device_info.c reference the former, expanding the existing concept of a platform mask by a few low bits reserved for sub-platform mask. This has a two-fold positive effect of firstly consolidating the list of device ids to one location, and secondly removing the if-ladders from every IS_<platfrom>_<subplatform> call site. Maintenance burden is not completely removed but should be improved. One case in point is that I have found some disagreements between device id listed in i915_pciids.h and i915_drv.h. At the same time platform mask code is generalized to an array of u32 to accomodate the addition of EHL and avoid spilling into u64 which would cause a small code size increase. Downside is that any platforms on the u32 boundary, like currently ICL and EHL, lose the benefit of optimizing the "IS_ICELAKE || IS_ELKHARTLAKE" checks into a single conditional, although at the moment there aren't any such call-sites. Before vs after for the whole series: text data bss dec hex filename 1891093 43903 7424 1942420 1da394 i915.ko.0 1890434 43903 7424 1941761 1da101 i915.ko.1 add/remove: 12/3 grow/shrink: 92/121 up/down: 1974/-1769 (205) ... Total: Before=1286293, After=1286498, chg +0.02% Tvrtko Ursulin (4): drm/i915: Split Pineview device info into desktop and mobile drm/i915: Remove redundant device id from IS_IRONLAKE_M macro drm/i915: Split some PCI ids into separate groups drm/i915: Introduce concept of a sub-platform arch/x86/kernel/early-quirks.c | 3 +- drivers/gpu/drm/i915/i915_drv.c | 8 +- drivers/gpu/drm/i915/i915_drv.h | 133 +++++++++++------ drivers/gpu/drm/i915/i915_gpu_error.c | 3 + drivers/gpu/drm/i915/i915_pci.c | 14 +- drivers/gpu/drm/i915/intel_device_info.c | 145 ++++++++++++++++++ drivers/gpu/drm/i915/intel_device_info.h | 27 +++- drivers/gpu/drm/i915/intel_pm.c | 4 +- include/drm/i915_pciids.h | 179 ++++++++++++++++------- 9 files changed, 414 insertions(+), 102 deletions(-)