[igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Anshuman Gupta]

Currently IGT is lacking IS_DGFX() macro support.
There are some power features like D3Cold are only
supported on discrete card. So IGT test/tools specific
to D3Cold requires to consume IS_DGFX().
Adding a is_dgfx field in intel_device_info and initializing
it for DG1. All future discrete platform would require to
initialize this field.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 lib/intel_chipset.h     | 2 ++
 lib/intel_device_info.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
index db75a829f..07fdd213e 100644
--- a/lib/intel_chipset.h
+++ b/lib/intel_chipset.h
@@ -42,6 +42,7 @@ struct intel_device_info {
 	unsigned gt; /* 0 if unknown */
 	bool has_4tile : 1;
 	bool has_flatccs;
+	bool is_dgfx :1;
 	bool is_mobile : 1;
 	bool is_whitney : 1;
 	bool is_almador : 1;
@@ -208,6 +209,7 @@ void intel_check_pch(void);
 
 #define IS_MOBILE(devid)	(intel_get_device_info(devid)->is_mobile)
 #define IS_965(devid)		AT_LEAST_GEN(devid, 4)
+#define IS_DGFX(devid)	(intel_get_device_info(devid)->is_dgfx)
 
 #define HAS_BSD_RING(devid)	AT_LEAST_GEN(devid, 5)
 #define HAS_BLT_RING(devid)	AT_LEAST_GEN(devid, 6)
diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c
index e55841df5..b735f16a6 100644
--- a/lib/intel_device_info.c
+++ b/lib/intel_device_info.c
@@ -386,6 +386,7 @@ static const struct intel_device_info intel_rocketlake_info = {
 static const struct intel_device_info intel_dg1_info = {
 	.graphics_ver = 12,
 	.display_ver = 12,
+	.is_dgfx = true,
 	.is_dg1 = true,
 	.codename = "dg1"
 };
@@ -394,6 +395,7 @@ static const struct intel_device_info intel_dg2_info = {
 	.graphics_ver = 12,
 	.display_ver = 13,
 	.has_4tile = true,
+	.is_dgfx = true,
 	.is_dg2 = true,
 	.codename = "dg2",
 	.has_flatccs = true,
-- 
2.26.2

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Kamil Konieczny]

Hi Anshuman,

On 2022-04-22 at 18:29:12 +0530, Anshuman Gupta wrote:
> Currently IGT is lacking IS_DGFX() macro support.

imho anything new is lacking ... I would rewrite this into
something like in subject and move that add macro info to
place where you describe what you added.

> There are some power features like D3Cold are only
> supported on discrete card. So IGT test/tools specific

s/So//

> to D3Cold requires to consume IS_DGFX().

s/requires to consume IS_DGFX()./requires to know this./

> Adding a is_dgfx field in intel_device_info and initializing

s/a is_dgfx/an is_dgfx/

Here you can write about new macro.

> it for DG1. All future discrete platform would require to
-------- ^

You are also adding it for DG2, so write DG1/DG2 here.

Regards,
Kamil

> initialize this field.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  lib/intel_chipset.h     | 2 ++
>  lib/intel_device_info.c | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/lib/intel_chipset.h b/lib/intel_chipset.h
> index db75a829f..07fdd213e 100644
> --- a/lib/intel_chipset.h
> +++ b/lib/intel_chipset.h
> @@ -42,6 +42,7 @@ struct intel_device_info {
>  	unsigned gt; /* 0 if unknown */
>  	bool has_4tile : 1;
>  	bool has_flatccs;
> +	bool is_dgfx :1;
>  	bool is_mobile : 1;
>  	bool is_whitney : 1;
>  	bool is_almador : 1;
> @@ -208,6 +209,7 @@ void intel_check_pch(void);
>  
>  #define IS_MOBILE(devid)	(intel_get_device_info(devid)->is_mobile)
>  #define IS_965(devid)		AT_LEAST_GEN(devid, 4)
> +#define IS_DGFX(devid)	(intel_get_device_info(devid)->is_dgfx)
>  
>  #define HAS_BSD_RING(devid)	AT_LEAST_GEN(devid, 5)
>  #define HAS_BLT_RING(devid)	AT_LEAST_GEN(devid, 6)
> diff --git a/lib/intel_device_info.c b/lib/intel_device_info.c
> index e55841df5..b735f16a6 100644
> --- a/lib/intel_device_info.c
> +++ b/lib/intel_device_info.c
> @@ -386,6 +386,7 @@ static const struct intel_device_info intel_rocketlake_info = {
>  static const struct intel_device_info intel_dg1_info = {
>  	.graphics_ver = 12,
>  	.display_ver = 12,
> +	.is_dgfx = true,
>  	.is_dg1 = true,
>  	.codename = "dg1"
>  };
> @@ -394,6 +395,7 @@ static const struct intel_device_info intel_dg2_info = {
>  	.graphics_ver = 12,
>  	.display_ver = 13,
>  	.has_4tile = true,
> +	.is_dgfx = true,
>  	.is_dg2 = true,
>  	.codename = "dg2",
>  	.has_flatccs = true,
> -- 
> 2.26.2
> 

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Dixit, Ashutosh]

On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
>
> Currently IGT is lacking IS_DGFX() macro support.
> There are some power features like D3Cold are only
> supported on discrete card. So IGT test/tools specific
> to D3Cold requires to consume IS_DGFX().
> Adding a is_dgfx field in intel_device_info and initializing
> it for DG1. All future discrete platform would require to
> initialize this field.

#define IS_DGFX(fd) gem_has_lmem(fd) ?

gem_has_lmem() is already pretty widely used so maybe we should not
introduce another way to achieve the same goal?

[igt-dev] ✗ GitLab.Pipeline: warning for lib/intel_device_info: Add IS_DGFX() support

[Patchwork]

== Series Details ==

Series: lib/intel_device_info: Add IS_DGFX() support
URL   : https://patchwork.freedesktop.org/series/102994/
State : warning

== Summary ==

Pipeline status: FAILED.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/567044 for the overview.

test:ninja-test-arm64 has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/21639402):
  Ok:                   22
  Expected Fail:         3
  Fail:                289
  Unexpected Pass:       0
  Skipped:               0
  Timeout:               0
  
  Full log written to /builds/gfx-ci/igt-ci-tags/build/meson-logs/testlog.txt
  section_end:1650646012:step_script
  section_start:1650646012:upload_artifacts_on_failure
  Uploading artifacts for failed job
  Uploading artifacts...
  build: found 1725 matching files and directories   
  Uploading artifacts as "archive" to coordinator... 201 Created  id=21639402 responseStatus=201 Created token=nCAus7wc
  section_end:1650646023:upload_artifacts_on_failure
  section_start:1650646023:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1650646023:cleanup_file_variables
  ERROR: Job failed: exit code 1

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/567044

[igt-dev] ✗ Fi.CI.BAT: failure for lib/intel_device_info: Add IS_DGFX() support

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 10782 bytes --]

== Series Details ==

Series: lib/intel_device_info: Add IS_DGFX() support
URL   : https://patchwork.freedesktop.org/series/102994/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11543 -> IGTPW_6984
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_6984 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_6984, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/index.html

Participating hosts (45 -> 45)
------------------------------

  Additional (2): fi-hsw-4770 bat-adlm-1 
  Missing    (2): fi-bsw-cyan fi-skl-6700k2 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_6984:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@module-reload:
    - bat-dg1-5:          [PASS][1] -> [SKIP][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/bat-dg1-5/igt@i915_pm_rpm@module-reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/bat-dg1-5/igt@i915_pm_rpm@module-reload.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - {bat-adlm-1}:       NOTRUN -> [DMESG-WARN][3]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/bat-adlm-1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  
Known issues
------------

  Here are the changes found in IGTPW_6984 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-hsw-4770:        NOTRUN -> [SKIP][4] ([fdo#109271]) +9 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-hsw-4770/igt@gem_huc_copy@huc-copy.html
    - fi-rkl-11600:       NOTRUN -> [SKIP][5] ([i915#2190])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][6] ([i915#4613]) +3 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@gem_lmem_swapping@basic.html

  * igt@gem_tiled_pread_basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][7] ([i915#3282])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-hsw-4770:        NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#3012])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-hsw-4770/igt@i915_pm_backlight@basic-brightness.html
    - fi-rkl-11600:       NOTRUN -> [SKIP][9] ([i915#3012])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_selftest@live@gt_engines:
    - fi-rkl-guc:         [PASS][10] -> [INCOMPLETE][11] ([i915#4418])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/fi-rkl-guc/igt@i915_selftest@live@gt_engines.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-guc/igt@i915_selftest@live@gt_engines.html

  * igt@i915_selftest@live@hangcheck:
    - fi-snb-2600:        [PASS][12] -> [INCOMPLETE][13] ([i915#3921])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/fi-snb-2600/igt@i915_selftest@live@hangcheck.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-snb-2600/igt@i915_selftest@live@hangcheck.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-hsw-4770:        NOTRUN -> [SKIP][14] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-hsw-4770/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-rkl-11600:       NOTRUN -> [SKIP][15] ([fdo#111827]) +8 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][16] ([i915#4070] / [i915#4103]) +1 similar issue
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-rkl-11600:       NOTRUN -> [SKIP][17] ([fdo#109285] / [i915#4098])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-hsw-4770:        NOTRUN -> [SKIP][18] ([fdo#109271] / [i915#533])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-hsw-4770/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html
    - fi-rkl-11600:       NOTRUN -> [SKIP][19] ([i915#4070] / [i915#533])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  * igt@kms_psr@primary_mmap_gtt:
    - fi-rkl-11600:       NOTRUN -> [SKIP][20] ([i915#1072]) +3 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@kms_psr@primary_mmap_gtt.html
    - fi-hsw-4770:        NOTRUN -> [SKIP][21] ([fdo#109271] / [i915#1072]) +3 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-hsw-4770/igt@kms_psr@primary_mmap_gtt.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-rkl-11600:       NOTRUN -> [SKIP][22] ([i915#3555] / [i915#4098])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-userptr:
    - fi-rkl-11600:       NOTRUN -> [SKIP][23] ([i915#3301] / [i915#3708])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@prime_vgem@basic-userptr.html

  * igt@prime_vgem@basic-write:
    - fi-rkl-11600:       NOTRUN -> [SKIP][24] ([i915#3291] / [i915#3708]) +2 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@prime_vgem@basic-write.html

  * igt@runner@aborted:
    - fi-bdw-5557u:       NOTRUN -> [FAIL][25] ([i915#4312])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-bdw-5557u/igt@runner@aborted.html
    - fi-rkl-guc:         NOTRUN -> [FAIL][26] ([i915#4312])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-guc/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@dmabuf@all@dma_fence_chain:
    - bat-dg1-5:          [INCOMPLETE][27] ([i915#4154]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/bat-dg1-5/igt@dmabuf@all@dma_fence_chain.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/bat-dg1-5/igt@dmabuf@all@dma_fence_chain.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-rkl-11600:       [INCOMPLETE][29] ([i915#5127]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@kms_busy@basic@modeset:
    - bat-adlp-4:         [DMESG-WARN][31] ([i915#3576]) -> [PASS][32] +1 similar issue
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/bat-adlp-4/igt@kms_busy@basic@modeset.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/bat-adlp-4/igt@kms_busy@basic@modeset.html

  
#### Warnings ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-5:          [DMESG-FAIL][33] ([i915#4494] / [i915#4957]) -> [DMESG-FAIL][34] ([i915#4957])
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11543/bat-dg1-5/igt@i915_selftest@live@hangcheck.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/bat-dg1-5/igt@i915_selftest@live@hangcheck.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3921]: https://gitlab.freedesktop.org/drm/intel/issues/3921
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4154]: https://gitlab.freedesktop.org/drm/intel/issues/4154
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4418]: https://gitlab.freedesktop.org/drm/intel/issues/4418
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5127]: https://gitlab.freedesktop.org/drm/intel/issues/5127
  [i915#5270]: https://gitlab.freedesktop.org/drm/intel/issues/5270
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5338]: https://gitlab.freedesktop.org/drm/intel/issues/5338
  [i915#5356]: https://gitlab.freedesktop.org/drm/intel/issues/5356
  [i915#5552]: https://gitlab.freedesktop.org/drm/intel/issues/5552


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_6449 -> IGTPW_6984

  CI-20190529: 20190529
  CI_DRM_11543: df51a64607842dbdc675b82f3390deefef9e562f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_6984: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/index.html
  IGT_6449: 704da775abb83faa9324a665fe2992ab90f4ab03 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6984/index.html

[-- Attachment #2: Type: text/html, Size: 12504 bytes --]

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Gupta, Anshuman]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> Sent: Friday, April 22, 2022 9:38 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support
> 
> On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
> >
> > Currently IGT is lacking IS_DGFX() macro support.
> > There are some power features like D3Cold are only supported on
> > discrete card. So IGT test/tools specific to D3Cold requires to
> > consume IS_DGFX().
> > Adding a is_dgfx field in intel_device_info and initializing it for
> > DG1. All future discrete platform would require to initialize this
> > field.
> 
> #define IS_DGFX(fd) gem_has_lmem(fd) ?
> 
> gem_has_lmem() is already pretty widely used so maybe we should not
> introduce another way to achieve the same goal?
Before introducing this , I thought on using gem_has_lmem.
But I was not sure in case every discrete platform mandatory to have lmem region.
If it is guaranteed that every discrete platform will have lmem memory region ?
I will  drop  this patch.
Thanks,
Anshuman Gupta.

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Dixit, Ashutosh]

On Sun, 24 Apr 2022 22:24:06 -0700, Gupta, Anshuman wrote:
>
> > -----Original Message-----
> > From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> > Sent: Friday, April 22, 2022 9:38 PM
> > To: Gupta, Anshuman <anshuman.gupta@intel.com>
> > Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support
> >
> > On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
> > >
> > > Currently IGT is lacking IS_DGFX() macro support.
> > > There are some power features like D3Cold are only supported on
> > > discrete card. So IGT test/tools specific to D3Cold requires to
> > > consume IS_DGFX().
> > > Adding a is_dgfx field in intel_device_info and initializing it for
> > > DG1. All future discrete platform would require to initialize this
> > > field.
> >
> > #define IS_DGFX(fd) gem_has_lmem(fd) ?
> >
> > gem_has_lmem() is already pretty widely used so maybe we should not
> > introduce another way to achieve the same goal?
>
> Before introducing this , I thought on using gem_has_lmem.
> But I was not sure in case every discrete platform mandatory to have lmem region.
> If it is guaranteed that every discrete platform will have lmem memory region ?
> I will  drop  this patch.

So not sure what will happen in the future but till now and in the
forseeable future all dGfx platforms have LMEM, so gem_has_lmem seems fine.

I think if we introduce IS_DGFX() as in this patch, we should have an
additional patch to convert all tests using gem_has_lmem() to IS_DGFX() so
that we don't have multiple ways of tests deciding if something should run
for dGfx.

Or maybe both are also ok? So lmem related tests should use gem_has_lmem()
and non LMEM related tests should use IS_DGFX()?

So if we want to merge this let's look at other tests which are presently
using gem_has_lmem() and see if any need to be converted over to
IS_DGFX(). Or to keep things simple just use gem_has_lmem() for now?

Thanks.

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Gupta, Anshuman]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> Sent: Monday, April 25, 2022 11:14 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support
> 
> On Sun, 24 Apr 2022 22:24:06 -0700, Gupta, Anshuman wrote:
> >
> > > -----Original Message-----
> > > From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> > > Sent: Friday, April 22, 2022 9:38 PM
> > > To: Gupta, Anshuman <anshuman.gupta@intel.com>
> > > Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo
> > > <rodrigo.vivi@intel.com>
> > > Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add
> > > IS_DGFX() support
> > >
> > > On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
> > > >
> > > > Currently IGT is lacking IS_DGFX() macro support.
> > > > There are some power features like D3Cold are only supported on
> > > > discrete card. So IGT test/tools specific to D3Cold requires to
> > > > consume IS_DGFX().
> > > > Adding a is_dgfx field in intel_device_info and initializing it
> > > > for DG1. All future discrete platform would require to initialize
> > > > this field.
> > >
> > > #define IS_DGFX(fd) gem_has_lmem(fd) ?
> > >
> > > gem_has_lmem() is already pretty widely used so maybe we should not
> > > introduce another way to achieve the same goal?
> >
> > Before introducing this , I thought on using gem_has_lmem.
> > But I was not sure in case every discrete platform mandatory to have lmem
> region.
> > If it is guaranteed that every discrete platform will have lmem memory region
> ?
> > I will  drop  this patch.
> 
> So not sure what will happen in the future but till now and in the forseeable
> future all dGfx platforms have LMEM, so gem_has_lmem seems fine.
It seems better to carry with gem_has_lmem(), as IS_DGFX() requires to populate device info fields.
This patch spined off from series https://patchwork.freedesktop.org/series/102780/
I will use gem_has_lmem() instead of IS_DGFX().
@rodrigo what is your opinion about it  ?
> 
> I think if we introduce IS_DGFX() as in this patch, we should have an additional
> patch to convert all tests using gem_has_lmem() to IS_DGFX() so that we don't
> have multiple ways of tests deciding if something should run for dGfx.
> 
> Or maybe both are also ok? So lmem related tests should use gem_has_lmem()
> and non LMEM related tests should use IS_DGFX()?
Actually, both of these methods even are not sufficient, if needed prior to open the drm_fd.
Ex. igt_device_find_first_i915_discrete_card() checks the igpu b:d:f, which breaks on one of real time issue.
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/94
Thanks,
Anshuman Gupta.
 
> 
> So if we want to merge this let's look at other tests which are presently using
> gem_has_lmem() and see if any need to be converted over to IS_DGFX(). Or to
> keep things simple just use gem_has_lmem() for now?
> 
> Thanks.

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Dixit, Ashutosh]

On Sun, 24 Apr 2022 23:10:23 -0700, Gupta, Anshuman wrote:
>
> > > > On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
> > > > >
> > > > > Currently IGT is lacking IS_DGFX() macro support.
> > > > > There are some power features like D3Cold are only supported on
> > > > > discrete card. So IGT test/tools specific to D3Cold requires to
> > > > > consume IS_DGFX().
> > > > > Adding a is_dgfx field in intel_device_info and initializing it
> > > > > for DG1. All future discrete platform would require to initialize
> > > > > this field.
> > > >
> > > > #define IS_DGFX(fd) gem_has_lmem(fd) ?
> > > >
> > > > gem_has_lmem() is already pretty widely used so maybe we should not
> > > > introduce another way to achieve the same goal?
> > >
> > > Before introducing this , I thought on using gem_has_lmem.
> > > But I was not sure in case every discrete platform mandatory to have lmem
> > region.
> > > If it is guaranteed that every discrete platform will have lmem memory region
> > ?
> > > I will  drop  this patch.
> >
> > So not sure what will happen in the future but till now and in the forseeable
> > future all dGfx platforms have LMEM, so gem_has_lmem seems fine.
> It seems better to carry with gem_has_lmem(), as IS_DGFX() requires to populate device info fields.
> This patch spined off from series https://patchwork.freedesktop.org/series/102780/
> I will use gem_has_lmem() instead of IS_DGFX().
> @rodrigo what is your opinion about it  ?
> >
> > I think if we introduce IS_DGFX() as in this patch, we should have an additional
> > patch to convert all tests using gem_has_lmem() to IS_DGFX() so that we don't
> > have multiple ways of tests deciding if something should run for dGfx.
> >
> > Or maybe both are also ok? So lmem related tests should use gem_has_lmem()
> > and non LMEM related tests should use IS_DGFX()?
>
> Actually, both of these methods even are not sufficient, if needed prior
> to open the drm_fd.  Ex. igt_device_find_first_i915_discrete_card()
> checks the igpu b:d:f, which breaks on one of real time issue.
> https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/issues/94

In that case maybe such API's which attempt to do this and don't work in
*all* cases should be deleted and something new provided? Anyway this seems
to be a different issue to what we were discussing? Thanks.

>
> >
> > So if we want to merge this let's look at other tests which are presently using
> > gem_has_lmem() and see if any need to be converted over to IS_DGFX(). Or to
> > keep things simple just use gem_has_lmem() for now?
> >
> > Thanks.

Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support

[Petri Latvala]

On Sun, Apr 24, 2022 at 10:44:14PM -0700, Dixit, Ashutosh wrote:
> On Sun, 24 Apr 2022 22:24:06 -0700, Gupta, Anshuman wrote:
> >
> > > -----Original Message-----
> > > From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> > > Sent: Friday, April 22, 2022 9:38 PM
> > > To: Gupta, Anshuman <anshuman.gupta@intel.com>
> > > Cc: igt-dev@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_device_info: Add IS_DGFX() support
> > >
> > > On Fri, 22 Apr 2022 05:59:12 -0700, Anshuman Gupta wrote:
> > > >
> > > > Currently IGT is lacking IS_DGFX() macro support.
> > > > There are some power features like D3Cold are only supported on
> > > > discrete card. So IGT test/tools specific to D3Cold requires to
> > > > consume IS_DGFX().
> > > > Adding a is_dgfx field in intel_device_info and initializing it for
> > > > DG1. All future discrete platform would require to initialize this
> > > > field.
> > >
> > > #define IS_DGFX(fd) gem_has_lmem(fd) ?
> > >
> > > gem_has_lmem() is already pretty widely used so maybe we should not
> > > introduce another way to achieve the same goal?
> >
> > Before introducing this , I thought on using gem_has_lmem.
> > But I was not sure in case every discrete platform mandatory to have lmem region.
> > If it is guaranteed that every discrete platform will have lmem memory region ?
> > I will  drop  this patch.
> 
> So not sure what will happen in the future but till now and in the
> forseeable future all dGfx platforms have LMEM, so gem_has_lmem seems fine.
> 
> I think if we introduce IS_DGFX() as in this patch, we should have an
> additional patch to convert all tests using gem_has_lmem() to IS_DGFX() so
> that we don't have multiple ways of tests deciding if something should run
> for dGfx.

The difference between checking for device info data and runtime data
is finding out whether something is expected to work on some kernel
versions or not. Runtime data should always be preferred when it's
available. A stupid example from history: DG1 enablement phase when
lmem access wasn't there yet in the kernel. gem_has_lmem() false,
still dGPU. IS_DGFX() doesn't tell that lmem interfaces are usable for
this device, it just tells that this device is supposed to have
gem_has_lmem() = true.

Best practice is to appease the human reader of the test code: Instead
of something like

if (IS_DGFX(i915))

use

if (i915_can_do_d3cold(i915))

where the function is

bool i915_can_do_d3cold(int i915) {
  /* explain why only DGFX, and all DGFX */
  return IS_DGFX(i915);
}

In my opinion nothing should care directly whether a device is DGFX or
not, but specific requirements can be expressed with IS_DGFX just fine
if there's not a good way to ask the kernel if the capability is
present.

If multiple different requirement checks end up being just "return
IS_DGFX(i915)" it doesn't matter, we're still better off for the case
in the future when someone figures out a better way to control whether
the device can do x. Easier to find all the spots that case about x,
but not y.


-- 
Petri Latvala