[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/display/adlp: Disable underrun recovery

[Patchwork]

== Series Details ==

Series: drm/i915/display/adlp: Disable underrun recovery
URL   : https://patchwork.freedesktop.org/series/96548/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10836 -> Patchwork_21514


Summary
---

  **SUCCESS**

  No regressions found.

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

Participating hosts (37 -> 33)
--

  Additional (1): fi-tgl-1115g4 
  Missing(5): fi-bxt-dsi bat-dg1-6 fi-tgl-u2 fi-bsw-cyan bat-adlp-4 

Known issues


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

### IGT changes ###

 Issues hit 

  * igt@amdgpu/amd_basic@cs-gfx:
- fi-rkl-guc: NOTRUN -> [SKIP][1] ([fdo#109315]) +17 similar issues
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-rkl-guc/igt@amdgpu/amd_ba...@cs-gfx.html

  * igt@amdgpu/amd_basic@query-info:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][2] ([fdo#109315])
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@amdgpu/amd_ba...@query-info.html

  * igt@amdgpu/amd_cs_nop@nop-gfx0:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][3] ([fdo#109315] / [i915#2575]) +16 
similar issues
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@amdgpu/amd_cs_...@nop-gfx0.html

  * igt@gem_huc_copy@huc-copy:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][4] ([i915#2190])
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@gem_huc_c...@huc-copy.html

  * igt@i915_pm_backlight@basic-brightness:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][5] ([i915#1155])
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@i915_pm_backli...@basic-brightness.html

  * igt@kms_chamelium@common-hpd-after-suspend:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][6] ([fdo#111827]) +8 similar issues
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@kms_chamel...@common-hpd-after-suspend.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][7] ([i915#4103]) +1 similar issue
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][8] ([fdo#109285])
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@kms_force_connector_ba...@force-load-detect.html

  * igt@kms_psr@primary_mmap_gtt:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][9] ([i915#1072]) +3 similar issues
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@kms_psr@primary_mmap_gtt.html

  * igt@prime_vgem@basic-userptr:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][10] ([i915#3301])
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-tgl-1115g4/igt@prime_v...@basic-userptr.html

  
 Possible fixes 

  * igt@i915_selftest@live@gt_engines:
- fi-rkl-guc: [INCOMPLETE][11] -> [PASS][12]
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10836/fi-rkl-guc/igt@i915_selftest@live@gt_engines.html
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-rkl-guc/igt@i915_selftest@live@gt_engines.html

  * igt@kms_frontbuffer_tracking@basic:
- fi-cml-u2:  [DMESG-WARN][13] ([i915#4269]) -> [PASS][14]
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10836/fi-cml-u2/igt@kms_frontbuffer_track...@basic.html
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-cml-u2/igt@kms_frontbuffer_track...@basic.html
- {fi-hsw-gt1}:   [DMESG-WARN][15] ([i915#4290]) -> [PASS][16]
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10836/fi-hsw-gt1/igt@kms_frontbuffer_track...@basic.html
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21514/fi-hsw-gt1/igt@kms_frontbuffer_track...@basic.html

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

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [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#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4269]: https://gitlab.freedesktop.org/drm/intel/issues/4269
  

Re: [Intel-gfx] [PATCH v9 10/10] drm: use DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES bitmap to tracefs

[Jason Baron]

On 10/27/21 12:36 AM, Jim Cromie wrote:
> Use new macro to create a sysfs control bitmap knob to control
> print-to-trace in: /sys/module/drm/parameters/trace
> 
> todo: reconsider this api, ie a single macro expecting both debug &
> trace terms (2 each), followed by a single description and the
> bitmap-spec::
> 
> Good: declares bitmap once for both interfaces
> 
> Bad: arg-type/count handling (expecting 4 args) is ugly,
>  especially preceding the bitmap-init var-args.
> 

Hi Jim,

I agree having the bitmap declared twice seems redundant. But I like having 
fewer args and not necessarily combining the trace/log variants of
DEBUG_CATEGORIES. hmmm...what if the DEFINE_DYNAMIC_DEBUG_CATEGORIES() took a 
pointer to the array of struct dyndbg_bitdesc map[] directly as the
final argument instead of the __VA_ARGS__? Then, we could just declare the map 
once?

Thanks,

-Jason

> Signed-off-by: Jim Cromie 
> ---
>  drivers/gpu/drm/drm_print.c | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> index ce662d0f339b..7b49fbc5e21d 100644
> --- a/drivers/gpu/drm/drm_print.c
> +++ b/drivers/gpu/drm/drm_print.c
> @@ -73,6 +73,25 @@ DEFINE_DYNAMIC_DEBUG_CATEGORIES(debug, __drm_debug,
>   [7] = { DRM_DBG_CAT_LEASE },
>   [8] = { DRM_DBG_CAT_DP },
>   [9] = { DRM_DBG_CAT_DRMRES });
> +
> +#ifdef CONFIG_TRACING
> +unsigned long __drm_trace;
> +EXPORT_SYMBOL(__drm_trace);
> +DEFINE_DYNAMIC_DEBUG_TRACE_CATEGORIES(trace, __drm_trace,
> +   DRM_DEBUG_DESC,
> +   [0] = { DRM_DBG_CAT_CORE },
> +   [1] = { DRM_DBG_CAT_DRIVER },
> +   [2] = { DRM_DBG_CAT_KMS },
> +   [3] = { DRM_DBG_CAT_PRIME },
> +   [4] = { DRM_DBG_CAT_ATOMIC },
> +   [5] = { DRM_DBG_CAT_VBL },
> +   [6] = { DRM_DBG_CAT_STATE },
> +   [7] = { DRM_DBG_CAT_LEASE },
> +   [8] = { DRM_DBG_CAT_DP },
> +   [9] = { DRM_DBG_CAT_DRMRES });
> +
> +struct trace_array *trace_arr;
> +#endif
>  #endif
>  
>  void __drm_puts_coredump(struct drm_printer *p, const char *str)
> 

[Intel-gfx] [PATCH] drm/i915/display/adlp: Disable underrun recovery

[José Roberto de Souza]

It was also defeatured for ADL-P and other platforms.

BSpec: 55424
Signed-off-by: José Roberto de Souza 
---
 drivers/gpu/drm/i915/display/intel_display.c | 39 
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
b/drivers/gpu/drm/i915/display/intel_display.c
index 29392dfc46c8d..64406408ba590 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -956,26 +956,6 @@ void intel_display_finish_reset(struct drm_i915_private 
*dev_priv)
clear_bit_unlock(I915_RESET_MODESET, _priv->gt.reset.flags);
 }
 
-static bool underrun_recovery_supported(const struct intel_crtc_state 
*crtc_state)
-{
-   if (crtc_state->pch_pfit.enabled &&
-   (crtc_state->pipe_src_w > drm_rect_width(_state->pch_pfit.dst) 
||
-crtc_state->pipe_src_h > 
drm_rect_height(_state->pch_pfit.dst) ||
-crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420))
-   return false;
-
-   if (crtc_state->dsc.compression_enable)
-   return false;
-
-   if (crtc_state->has_psr2)
-   return false;
-
-   if (crtc_state->splitter.enable)
-   return false;
-
-   return true;
-}
-
 static void icl_set_pipe_chicken(const struct intel_crtc_state *crtc_state)
 {
struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -999,19 +979,14 @@ static void icl_set_pipe_chicken(const struct 
intel_crtc_state *crtc_state)
 */
tmp |= PIXEL_ROUNDING_TRUNC_FB_PASSTHRU;
 
-   if (IS_DG2(dev_priv)) {
-   /*
-* Underrun recovery must always be disabled on DG2.  However
-* the chicken bit meaning is inverted compared to other
-* platforms.
-*/
+   /*
+* Underrun recovery must always be disabled on display 13+.
+* DG2 chicken bit meaning is inverted compared to other platforms.
+*/
+   if (IS_DG2(dev_priv))
tmp &= ~UNDERRUN_RECOVERY_ENABLE_DG2;
-   } else if (DISPLAY_VER(dev_priv) >= 13) {
-   if (underrun_recovery_supported(crtc_state))
-   tmp &= ~UNDERRUN_RECOVERY_DISABLE_ADLP;
-   else
-   tmp |= UNDERRUN_RECOVERY_DISABLE_ADLP;
-   }
+   else if (DISPLAY_VER(dev_priv) >= 13)
+   tmp |= UNDERRUN_RECOVERY_DISABLE_ADLP;
 
intel_de_write(dev_priv, PIPE_CHICKEN(pipe), tmp);
 }
-- 
2.33.1

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/pmu: Fix synchronization of PMU callback with reset

[Patchwork]

== Series Details ==

Series: drm/i915/pmu: Fix synchronization of PMU callback with reset
URL   : https://patchwork.freedesktop.org/series/96543/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10834_full -> Patchwork_21513_full


Summary
---

  **FAILURE**

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

  

Participating hosts (10 -> 10)
--

  No changes in participating hosts

Possible new issues
---

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

### IGT changes ###

 Possible regressions 

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
- shard-kbl:  [PASS][1] -> [INCOMPLETE][2]
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-kbl1/igt@kms_cursor_...@pipe-c-cursor-suspend.html
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-kbl4/igt@kms_cursor_...@pipe-c-cursor-suspend.html

  
Known issues


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

### IGT changes ###

 Issues hit 

  * igt@gem_exec_fair@basic-deadline:
- shard-skl:  NOTRUN -> [FAIL][3] ([i915#2846])
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-skl4/igt@gem_exec_f...@basic-deadline.html

  * igt@gem_exec_fair@basic-none@vcs0:
- shard-apl:  [PASS][4] -> [FAIL][5] ([i915#2842])
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl8/igt@gem_exec_fair@basic-n...@vcs0.html
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-apl7/igt@gem_exec_fair@basic-n...@vcs0.html
- shard-tglb: NOTRUN -> [FAIL][6] ([i915#2842]) +5 similar issues
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-tglb2/igt@gem_exec_fair@basic-n...@vcs0.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
- shard-tglb: [PASS][7] -> [FAIL][8] ([i915#2842])
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-tglb2/igt@gem_exec_fair@basic-pace-sh...@rcs0.html
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-tglb8/igt@gem_exec_fair@basic-pace-sh...@rcs0.html

  * igt@gem_exec_fair@basic-pace-solo@rcs0:
- shard-glk:  [PASS][9] -> [FAIL][10] ([i915#2842]) +1 similar issue
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-glk7/igt@gem_exec_fair@basic-pace-s...@rcs0.html
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-glk1/igt@gem_exec_fair@basic-pace-s...@rcs0.html

  * igt@gem_exec_fair@basic-pace@vecs0:
- shard-kbl:  [PASS][11] -> [FAIL][12] ([i915#2842]) +2 similar 
issues
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-kbl4/igt@gem_exec_fair@basic-p...@vecs0.html
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-kbl4/igt@gem_exec_fair@basic-p...@vecs0.html

  * igt@gem_exec_whisper@basic-contexts-priority:
- shard-glk:  [PASS][13] -> [DMESG-WARN][14] ([i915#118])
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-glk1/igt@gem_exec_whis...@basic-contexts-priority.html
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-glk2/igt@gem_exec_whis...@basic-contexts-priority.html

  * igt@gem_pread@exhaustion:
- shard-skl:  NOTRUN -> [WARN][15] ([i915#2658])
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-skl6/igt@gem_pr...@exhaustion.html

  * igt@gem_pxp@protected-raw-src-copy-not-readible:
- shard-skl:  NOTRUN -> ([SKIP][16], [SKIP][17]) ([fdo#109271]) +6 
similar issues
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-skl4/igt@gem_...@protected-raw-src-copy-not-readible.html
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-skl3/igt@gem_...@protected-raw-src-copy-not-readible.html

  * igt@gem_pxp@regular-baseline-src-copy-readible:
- shard-kbl:  NOTRUN -> [SKIP][18] ([fdo#109271]) +95 similar issues
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-kbl3/igt@gem_...@regular-baseline-src-copy-readible.html

  * igt@gem_userptr_blits@coherency-sync:
- shard-tglb: NOTRUN -> [SKIP][19] ([fdo#110542])
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-tglb6/igt@gem_userptr_bl...@coherency-sync.html

  * igt@gem_userptr_blits@input-checking:
- shard-skl:  NOTRUN -> ([DMESG-WARN][20], [DMESG-WARN][21]) 
([i915#3002])
   [20]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/shard-skl3/igt@gem_userptr_bl...@input-checking.html

Re: [Intel-gfx] [PATCH 3/3] drm/i915/guc/slpc: Update boost sysfs hooks for SLPC

[Dixit, Ashutosh]

On Mon, 01 Nov 2021 18:26:08 -0700, Vinay Belgaumkar wrote:
>
> Add a helper to sort through the SLPC/RPS paths of get/set methods.
> Boost frequency will be modified as long as it is within the constraints
> of RP0 and if it is different from the existing one. We will set min
> freq to boost only if there is at least one active waiter.
>
> v2: Add num_boosts to guc_slpc_info and changes for worker function
> v3: Review comments (Ashutosh)

Reviewed-by: Ashutosh Dixit 

Re: [Intel-gfx] [PATCH 3/3] drm/i915/guc/slpc: Update boost sysfs hooks for SLPC

[Dixit, Ashutosh]

On Mon, 01 Nov 2021 13:28:14 -0700, Dixit, Ashutosh wrote:
>
> On Sun, 31 Oct 2021 21:39:37 -0700, Belgaumkar, Vinay wrote:
> >
> > +static int set_boost_freq(struct intel_rps *rps, u32 val)
>
> Since this is legacy rps code path maybe change function name to
> rps_set_boost_freq?

Not being able to find v3 of this patch so giving a R-b on v2 but the R-b
applies to v3:

Reviewed-by: Ashutosh Dixit 

Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc/slpc: Add waitboost functionality for SLPC

[Dixit, Ashutosh]

On Mon, 01 Nov 2021 18:26:07 -0700, Vinay Belgaumkar wrote:
>
> Add helper in RPS code for handling SLPC and non-SLPC paths.
> When boost is requested in the SLPC path, we can ask GuC to ramp
> up the frequency req by setting the minimum frequency to boost freq.
> Reset freq back to the min softlimit when there are no more waiters.
>
> v2: Schedule a worker thread which can boost freq from within
> an interrupt context as well.
>
> v3: No need to check against requested freq before scheduling boost
> work (Ashutosh)

Reviewed-by: Ashutosh Dixit 

Re: [Intel-gfx] [PATCH 1/3] drm/i915/guc/slpc: Define and initialize boost frequency

[Dixit, Ashutosh]

On Mon, 01 Nov 2021 18:26:06 -0700, Vinay Belgaumkar wrote:
>
> Define helpers and struct members required to record boost info.
> Boost frequency is initialized to RP0 at SLPC init. Also define num_waiters
> which can track the pending boost requests.
>
> Boost will be done by scheduling a worker thread. This will avoid
> the need to make H2G calls inside an interrupt context. Initialize the
> worker function during SLPC init as well. Had to move intel_guc_slpc_init
> a few lines below to accomodate this.
>
> v2: Add a workqueue to handle waitboost
> v3: Code review comments (Ashutosh)

Reviewed-by: Ashutosh Dixit 

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table

[John Harrison]

On 11/3/2021 14:38, Jordan Justen wrote:

John Harrison  writes:


On 11/1/2021 08:39, Jordan Justen wrote:

 writes:


From: Rodrigo Vivi 

GuC contains a consolidated table with a bunch of information about the
current device.

Previously, this information was spread and hardcoded to all the components
including GuC, i915 and various UMDs. The goal here is to consolidate
the data into GuC in a way that all interested components can grab the
very latest and synchronized information using a simple query.

As per most of the other queries, this one can be called twice.
Once with item.length=0 to determine the exact buffer size, then
allocate the user memory and call it again for to retrieve the
table data. For example:
struct drm_i915_query_item item = {
  .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
};
query.items_ptr = (int64_t) 
query.num_items = 1;

ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));

if (item.length <= 0)
  return -ENOENT;

data = malloc(item.length);
item.data_ptr = (int64_t) 
ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));

// Parse the data as appropriate...

The returned array is a simple and flexible KLV (Key/Length/Value)
formatted table. For example, it could be just:
enum device_attr {
   ATTR_SOME_VALUE = 0,
   ATTR_SOME_MASK  = 1,
};

static const u32 hwconfig[] = {
ATTR_SOME_VALUE,
1, // Value Length in DWords
8, // Value

ATTR_SOME_MASK,
3,
0x00, 0x, 0xFF00,
};

Seems simple enough, so why doesn't i915 define the format of the
returned hwconfig blob in i915_drm.h?

Because the definition is nothing to do with i915. This table comes from
the hardware spec. It is not defined by the KMD and it is not currently
used by the KMD. So there is no reason for the KMD to be creating
structures for it in the same way that the KMD does not document,
define, struct, etc. every other feature of the hardware that the UMDs
might use.

So, i915 wants to wash it's hands completely of the format? There is
obviously a difference between hardware features and a blob coming from
closed source software. (Which i915 just happens to be passing along.)
The hardware is a lot more difficult to change...
Actually, no. The table is not "coming from closed source software". The 
table is defined by hardware specs. It is a table of hardware specific 
values. It is not being invented by the GuC just for fun or as a way to 
subvert the universe into the realms of closed source software. As per 
KMD, GuC is merely passing the table through. The table is only 
supported on newer hardware platforms and all GuC does is provide a 
mechanism for the KMD to retrieve it because the KMD cannot access it 
directly. The table contents are defined by hardware architects same as 
all the other aspects of the hardware.




It seems like these details should be dropped from the i915 patch commit
message since i915 wants nothing to do with it.

Sure. Can remove comments.



I would think it'd be preferable for i915 to stand behind the basic blob
format as is (even if the keys/values can't be defined), and make a new
query item if the closed source software changes the format.
Close source software is not allowed to change the format because closed 
source software has no say in defining the format. The format is 
officially defined as being fixed in the spec. New key values can be 
added to the key enumeration but existing values cannot be deprecated 
and re-purposed. The table must be stable across all OSs and all 
platforms. No software can arbitrarily decide to change it.




Of course, it'd be even better if i915 could define some keys/values as
well. (Or if a spec could be released to help document / tie down the
format.)

See the corresponding IGT test that details all the currently defined keys.





struct drm_i915_hwconfig {
uint32_t key;
uint32_t length;
uint32_t values[];
};

It sounds like the kernel depends on the closed source guc being loaded
to return this information. Is that right? Will i915 also become
dependent on some of this data such that it won't be able to initialize
without the firmware being loaded?

At the moment, the KMD does not use the table at all. We merely provide
a mechanism for the UMDs to retrieve it from the hardware.

In terms of future direction, that is something you need to take up with
the hardware architects.


Why do you keep saying hardware, when only software is involved?
See above - because the table is defined by hardware. No software, 
closed or open, has any say in the specification of the table.


John.





The attribute ids are defined in a hardware spec.

Which spec?

Unfortunately, it is not one that is currently public. We are pushing
the relevant people to get it included in the public bspec / HRM. It is
a slow process :(.


Right. I take it no progress has been made in 

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix synchronization of PMU callback with reset

[Patchwork]

== Series Details ==

Series: drm/i915/pmu: Fix synchronization of PMU callback with reset
URL   : https://patchwork.freedesktop.org/series/96543/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10834 -> Patchwork_21513


Summary
---

  **SUCCESS**

  No regressions found.

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

Participating hosts (37 -> 33)
--

  Additional (2): fi-glk-dsi fi-tgl-1115g4 
  Missing(6): fi-kbl-soraka bat-dg1-6 fi-tgl-u2 fi-bsw-cyan fi-icl-u2 
bat-adlp-4 

Known issues


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

### IGT changes ###

 Issues hit 

  * igt@amdgpu/amd_basic@query-info:
- fi-bsw-kefka:   NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-bsw-kefka/igt@amdgpu/amd_ba...@query-info.html
- fi-tgl-1115g4:  NOTRUN -> [SKIP][2] ([fdo#109315])
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@amdgpu/amd_ba...@query-info.html

  * igt@amdgpu/amd_cs_nop@nop-gfx0:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][3] ([fdo#109315] / [i915#2575]) +16 
similar issues
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@amdgpu/amd_cs_...@nop-gfx0.html

  * igt@gem_huc_copy@huc-copy:
- fi-glk-dsi: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-glk-dsi/igt@gem_huc_c...@huc-copy.html
- fi-tgl-1115g4:  NOTRUN -> [SKIP][5] ([i915#2190])
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@gem_huc_c...@huc-copy.html

  * igt@i915_pm_backlight@basic-brightness:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][6] ([i915#1155])
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@i915_pm_backli...@basic-brightness.html

  * igt@kms_chamelium@common-hpd-after-suspend:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][7] ([fdo#111827]) +8 similar issues
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@kms_chamel...@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
- fi-glk-dsi: NOTRUN -> [SKIP][8] ([fdo#109271] / [fdo#111827]) +8 
similar issues
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-glk-dsi/igt@kms_chamel...@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][9] ([i915#4103]) +1 similar issue
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][10] ([fdo#109285])
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@kms_force_connector_ba...@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
- fi-glk-dsi: NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#533])
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-glk-dsi/igt@kms_pipe_crc_ba...@compare-crc-sanitycheck-pipe-d.html

  * igt@kms_psr@primary_mmap_gtt:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][12] ([i915#1072]) +3 similar issues
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@kms_psr@primary_mmap_gtt.html

  * igt@kms_psr@primary_page_flip:
- fi-glk-dsi: NOTRUN -> [SKIP][13] ([fdo#109271]) +30 similar issues
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-glk-dsi/igt@kms_psr@primary_page_flip.html

  * igt@prime_vgem@basic-userptr:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][14] ([i915#3301])
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-tgl-1115g4/igt@prime_v...@basic-userptr.html

  * igt@runner@aborted:
- fi-bdw-5557u:   NOTRUN -> [FAIL][15] ([i915#1602] / [i915#2426] / 
[i915#4312])
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-bdw-5557u/igt@run...@aborted.html

  
 Possible fixes 

  * igt@i915_selftest@live@execlists:
- fi-bsw-kefka:   [INCOMPLETE][16] ([i915#2940]) -> [PASS][17]
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/fi-bsw-kefka/igt@i915_selftest@l...@execlists.html
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-bsw-kefka/igt@i915_selftest@l...@execlists.html

  * igt@i915_selftest@live@gt_heartbeat:
- fi-bxt-dsi: [DMESG-FAIL][18] ([i915#541]) -> [PASS][19]
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21513/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html

  
  {name}: This element 

[Intel-gfx] [PATCH] drm/i915/pmu: Fix synchronization of PMU callback with reset

[Umesh Nerlige Ramappa]

Since the PMU callback runs in irq context, it synchronizes with gt
reset using the reset count. We could run into a case where the PMU
callback could read the reset count before it is updated. This has a
potential of corrupting the busyness stats.

In addition to the reset count, check if the reset bit is set before
capturing busyness.

In addition save the previous stats only if you intend to update them.

Signed-off-by: Umesh Nerlige Ramappa 
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 5cc49c0b3889..d83ade77ca07 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -1183,6 +1183,7 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs 
*engine, ktime_t *now)
u64 total, gt_stamp_saved;
unsigned long flags;
u32 reset_count;
+   bool in_reset;
 
spin_lock_irqsave(>timestamp.lock, flags);
 
@@ -1191,7 +1192,9 @@ static ktime_t guc_engine_busyness(struct intel_engine_cs 
*engine, ktime_t *now)
 * engine busyness from GuC, so we just use the driver stored
 * copy of busyness. Synchronize with gt reset using reset_count.
 */
-   reset_count = i915_reset_count(gpu_error);
+   rcu_read_lock();
+   in_reset = test_bit(I915_RESET_BACKOFF, >reset.flags);
+   rcu_read_unlock();
 
*now = ktime_get();
 
@@ -1201,9 +1204,10 @@ static ktime_t guc_engine_busyness(struct 
intel_engine_cs *engine, ktime_t *now)
 * start_gt_clk is derived from GuC state. To get a consistent
 * view of activity, we query the GuC state only if gt is awake.
 */
-   stats_saved = *stats;
-   gt_stamp_saved = guc->timestamp.gt_stamp;
-   if (intel_gt_pm_get_if_awake(gt)) {
+   if (intel_gt_pm_get_if_awake(gt) && !in_reset) {
+   stats_saved = *stats;
+   gt_stamp_saved = guc->timestamp.gt_stamp;
+   reset_count = i915_reset_count(gpu_error);
guc_update_engine_gt_clks(engine);
guc_update_pm_timestamp(guc, engine, now);
intel_gt_pm_put_async(gt);
-- 
2.20.1

[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/perf: Detect offset in context image for OA ctx control reg (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: Detect offset in context image for OA ctx control reg 
(rev2)
URL   : https://patchwork.freedesktop.org/series/96507/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10834_full -> Patchwork_21512_full


Summary
---

  **FAILURE**

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

  

Participating hosts (10 -> 10)
--

  No changes in participating hosts

Possible new issues
---

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

### IGT changes ###

 Possible regressions 

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
- shard-kbl:  [PASS][1] -> [INCOMPLETE][2]
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-kbl1/igt@kms_cursor_...@pipe-c-cursor-suspend.html
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/shard-kbl4/igt@kms_cursor_...@pipe-c-cursor-suspend.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
- shard-apl:  [PASS][3] -> [INCOMPLETE][4]
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl8/igt@kms_vbl...@pipe-a-ts-continuation-suspend.html
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/shard-apl2/igt@kms_vbl...@pipe-a-ts-continuation-suspend.html

  
 Warnings 

  * igt@kms_fbcon_fbt@fbc-suspend:
- shard-snb:  [SKIP][5] ([fdo#109271]) -> [INCOMPLETE][6]
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-snb7/igt@kms_fbcon_...@fbc-suspend.html
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/shard-snb2/igt@kms_fbcon_...@fbc-suspend.html

  
Known issues


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

### CI changes ###

 Issues hit 

  * boot:
- shard-apl:  ([PASS][7], [PASS][8], [PASS][9], [PASS][10], 
[PASS][11], [PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], 
[PASS][17], [PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], 
[PASS][23], [PASS][24], [PASS][25], [PASS][26], [PASS][27], [PASS][28], 
[PASS][29], [PASS][30], [PASS][31]) -> ([PASS][32], [FAIL][33], [PASS][34], 
[PASS][35], [PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], 
[PASS][41], [PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], 
[PASS][47], [PASS][48], [PASS][49], [PASS][50], [PASS][51], [PASS][52], 
[PASS][53], [PASS][54], [PASS][55], [PASS][56]) ([i915#4386])
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl8/boot.html
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl8/boot.html
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl8/boot.html
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl8/boot.html
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl7/boot.html
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl7/boot.html
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl7/boot.html
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl6/boot.html
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl6/boot.html
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl6/boot.html
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl6/boot.html
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl4/boot.html
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl4/boot.html
   [20]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl4/boot.html
   [21]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl3/boot.html
   [22]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl3/boot.html
   [23]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl3/boot.html
   [24]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl2/boot.html
   [25]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl2/boot.html
   [26]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl2/boot.html
   [27]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl2/boot.html
   [28]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl1/boot.html
   [29]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl1/boot.html
   [30]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl1/boot.html
   [31]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/shard-apl1/boot.html
   [32]: 

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/uapi: Add query for hwconfig table

[Jordan Justen]

John Harrison  writes:

> On 11/1/2021 08:39, Jordan Justen wrote:
>>  writes:
>>
>>> From: Rodrigo Vivi 
>>>
>>> GuC contains a consolidated table with a bunch of information about the
>>> current device.
>>>
>>> Previously, this information was spread and hardcoded to all the components
>>> including GuC, i915 and various UMDs. The goal here is to consolidate
>>> the data into GuC in a way that all interested components can grab the
>>> very latest and synchronized information using a simple query.
>>>
>>> As per most of the other queries, this one can be called twice.
>>> Once with item.length=0 to determine the exact buffer size, then
>>> allocate the user memory and call it again for to retrieve the
>>> table data. For example:
>>>struct drm_i915_query_item item = {
>>>  .query_id = DRM_I915_QUERY_HWCONCFIG_TABLE;
>>>};
>>>query.items_ptr = (int64_t) 
>>>query.num_items = 1;
>>>
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>if (item.length <= 0)
>>>  return -ENOENT;
>>>
>>>data = malloc(item.length);
>>>item.data_ptr = (int64_t) 
>>>ioctl(fd, DRM_IOCTL_I915_QUERY, query, sizeof(query));
>>>
>>>// Parse the data as appropriate...
>>>
>>> The returned array is a simple and flexible KLV (Key/Length/Value)
>>> formatted table. For example, it could be just:
>>>enum device_attr {
>>>   ATTR_SOME_VALUE = 0,
>>>   ATTR_SOME_MASK  = 1,
>>>};
>>>
>>>static const u32 hwconfig[] = {
>>>ATTR_SOME_VALUE,
>>>1, // Value Length in DWords
>>>8, // Value
>>>
>>>ATTR_SOME_MASK,
>>>3,
>>>0x00, 0x, 0xFF00,
>>>};
>> Seems simple enough, so why doesn't i915 define the format of the
>> returned hwconfig blob in i915_drm.h?
> Because the definition is nothing to do with i915. This table comes from 
> the hardware spec. It is not defined by the KMD and it is not currently 
> used by the KMD. So there is no reason for the KMD to be creating 
> structures for it in the same way that the KMD does not document, 
> define, struct, etc. every other feature of the hardware that the UMDs 
> might use.

So, i915 wants to wash it's hands completely of the format? There is
obviously a difference between hardware features and a blob coming from
closed source software. (Which i915 just happens to be passing along.)
The hardware is a lot more difficult to change...

It seems like these details should be dropped from the i915 patch commit
message since i915 wants nothing to do with it.

I would think it'd be preferable for i915 to stand behind the basic blob
format as is (even if the keys/values can't be defined), and make a new
query item if the closed source software changes the format.

Of course, it'd be even better if i915 could define some keys/values as
well. (Or if a spec could be released to help document / tie down the
format.)

>>
>> struct drm_i915_hwconfig {
>>  uint32_t key;
>>  uint32_t length;
>>  uint32_t values[];
>> };
>>
>> It sounds like the kernel depends on the closed source guc being loaded
>> to return this information. Is that right? Will i915 also become
>> dependent on some of this data such that it won't be able to initialize
>> without the firmware being loaded?
> At the moment, the KMD does not use the table at all. We merely provide 
> a mechanism for the UMDs to retrieve it from the hardware.
>
> In terms of future direction, that is something you need to take up with 
> the hardware architects.
>

Why do you keep saying hardware, when only software is involved?

>
>>> The attribute ids are defined in a hardware spec.
>> Which spec?
>
> Unfortunately, it is not one that is currently public. We are pushing 
> the relevant people to get it included in the public bspec / HRM. It is 
> a slow process :(.
>

Right. I take it no progress has been made in the 1.5 months since you
posted this, so it'll probably finally be documented 6~12 months after
hardware is available? :)

-Jordan

Re: [Intel-gfx] [PATCH 2/3] drm/i915/dg2: Add initial gt/ctx/engine workarounds

[Srivatsa, Anusha]

> -Original Message-
> From: Roper, Matthew D 
> Sent: Tuesday, November 2, 2021 3:25 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org; Roper, Matthew D
> ; Srivatsa, Anusha
> 
> Subject: [PATCH 2/3] drm/i915/dg2: Add initial gt/ctx/engine workarounds
> 
> Bspec: 54077,68173,54833
> Cc: Anusha Srivatsa 
> Signed-off-by: Matt Roper 

Reviewed-by: Anusha Srivatsa 

> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 278 +++-
>  drivers/gpu/drm/i915/i915_reg.h |  94 +--
>  drivers/gpu/drm/i915/intel_pm.c |  21 +-
>  3 files changed, 372 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 4aaa210fc003..37fd541a9719 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -644,6 +644,42 @@ static void dg1_ctx_workarounds_init(struct
> intel_engine_cs *engine,
>DG1_HZ_READ_SUPPRESSION_OPTIMIZATION_DISABLE);
>  }
> 
> +static void dg2_ctx_workarounds_init(struct intel_engine_cs *engine,
> +  struct i915_wa_list *wal)
> +{
> + gen12_ctx_gt_tuning_init(engine, wal);
> +
> + /* Wa_16011186671:dg2_g11 */
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0)) {
> + wa_masked_dis(wal, VFLSKPD,
> DIS_MULT_MISS_RD_SQUASH);
> + wa_masked_en(wal, VFLSKPD, DIS_OVER_FETCH_CACHE);
> + }
> +
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_B0)) {
> + /* Wa_14010469329:dg2_g10 */
> + wa_masked_en(wal, GEN11_COMMON_SLICE_CHICKEN3,
> +  XEHP_DUAL_SIMD8_SEQ_MERGE_DISABLE);
> +
> + /*
> +  * Wa_22010465075:dg2_g10
> +  * Wa_22010613112:dg2_g10
> +  * Wa_14010698770:dg2_g10
> +  */
> + wa_masked_en(wal, GEN11_COMMON_SLICE_CHICKEN3,
> +  GEN12_DISABLE_CPS_AWARE_COLOR_PIPE);
> + }
> +
> + /* Wa_16013271637:dg2 */
> + wa_masked_en(wal, SLICE_COMMON_ECO_CHICKEN1,
> +  MSC_MSAA_REODER_BUF_BYPASS_DISABLE);
> +
> + /* Wa_22012532006:dg2 */
> + if (IS_DG2_GRAPHICS_STEP(engine->i915, G10, STEP_A0, STEP_C0) ||
> + IS_DG2_GRAPHICS_STEP(engine->i915, G11, STEP_A0, STEP_B0))
> + wa_masked_en(wal, GEN9_HALF_SLICE_CHICKEN7,
> +
> DG2_DISABLE_ROUND_ENABLE_ALLOW_FOR_SSLA);
> +}
> +
>  static void fakewa_disable_nestedbb_mode(struct intel_engine_cs *engine,
>struct i915_wa_list *wal)
>  {
> @@ -730,7 +766,9 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs
> *engine,
>   if (engine->class != RENDER_CLASS)
>   goto done;
> 
> - if (IS_XEHPSDV(i915))
> + if (IS_DG2(i915))
> + dg2_ctx_workarounds_init(engine, wal);
> + else if (IS_XEHPSDV(i915))
>   ; /* noop; none at this time */
>   else if (IS_DG1(i915))
>   dg1_ctx_workarounds_init(engine, wal); @@ -1343,12
> +1381,117 @@ xehpsdv_gt_workarounds_init(struct intel_gt *gt, struct
> i915_wa_list *wal)
>   GLOBAL_INVALIDATION_MODE);
>  }
> 
> +static void
> +dg2_gt_workarounds_init(struct intel_gt *gt, struct i915_wa_list *wal)
> +{
> + struct intel_engine_cs *engine;
> + int id;
> +
> + xehp_init_mcr(gt, wal);
> +
> + /* Wa_14011060649:dg2 */
> + wa_14011060649(gt, wal);
> +
> + /*
> +  * Although there are per-engine instances of these registers,
> +  * they technically exist outside the engine itself and are not
> +  * impacted by engine resets.  Furthermore, they're part of the
> +  * GuC blacklist so trying to treat them as engine workarounds
> +  * will result in GuC initialization failure and a wedged GPU.
> +  */
> + for_each_engine(engine, gt, id) {
> + if (engine->class != VIDEO_DECODE_CLASS)
> + continue;
> +
> + /* Wa_16010515920:dg2_g10 */
> + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0,
> STEP_B0))
> + wa_write_or(wal, VDBOX_CGCTL3F18(engine-
> >mmio_base),
> + ALNUNIT_CLKGATE_DIS);
> + }
> +
> + if (IS_DG2_G10(gt->i915)) {
> + /* Wa_22010523718:dg2 */
> + wa_write_or(wal, UNSLICE_UNIT_LEVEL_CLKGATE,
> + CG3DDISCFEG_CLKGATE_DIS);
> +
> + /* Wa_14011006942:dg2 */
> + wa_write_or(wal, SUBSLICE_UNIT_LEVEL_CLKGATE,
> + DSS_ROUTER_CLKGATE_DIS);
> + }
> +
> + if (IS_DG2_GRAPHICS_STEP(gt->i915, G10, STEP_A0, STEP_B0)) {
> + /* Wa_14010680813:dg2_g10 */
> + wa_write_or(wal, GEN12_GAMSTLB_CTRL,
> CONTROL_BLOCK_CLKGATE_DIS |
> + EGRESS_BLOCK_CLKGATE_DIS |
> TAG_BLOCK_CLKGATE_DIS);
> +
> + /* 

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Detect offset in context image for OA ctx control reg (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: Detect offset in context image for OA ctx control reg 
(rev2)
URL   : https://patchwork.freedesktop.org/series/96507/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10834 -> Patchwork_21512


Summary
---

  **SUCCESS**

  No regressions found.

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

Participating hosts (37 -> 35)
--

  Additional (2): fi-glk-dsi fi-tgl-1115g4 
  Missing(4): fi-bsw-cyan bat-adlp-4 bat-dg1-6 fi-tgl-u2 

Known issues


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

### IGT changes ###

 Issues hit 

  * igt@amdgpu/amd_basic@query-info:
- fi-bsw-kefka:   NOTRUN -> [SKIP][1] ([fdo#109271]) +17 similar issues
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-bsw-kefka/igt@amdgpu/amd_ba...@query-info.html
- fi-tgl-1115g4:  NOTRUN -> [SKIP][2] ([fdo#109315])
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@amdgpu/amd_ba...@query-info.html

  * igt@amdgpu/amd_cs_nop@nop-gfx0:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][3] ([fdo#109315] / [i915#2575]) +16 
similar issues
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@amdgpu/amd_cs_...@nop-gfx0.html

  * igt@gem_huc_copy@huc-copy:
- fi-glk-dsi: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#2190])
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-glk-dsi/igt@gem_huc_c...@huc-copy.html
- fi-tgl-1115g4:  NOTRUN -> [SKIP][5] ([i915#2190])
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@gem_huc_c...@huc-copy.html

  * igt@i915_pm_backlight@basic-brightness:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][6] ([i915#1155])
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@i915_pm_backli...@basic-brightness.html

  * igt@kms_chamelium@common-hpd-after-suspend:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][7] ([fdo#111827]) +8 similar issues
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@kms_chamel...@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-hpd-fast:
- fi-glk-dsi: NOTRUN -> [SKIP][8] ([fdo#109271] / [fdo#111827]) +8 
similar issues
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-glk-dsi/igt@kms_chamel...@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][9] ([i915#4103]) +1 similar issue
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][10] ([fdo#109285])
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@kms_force_connector_ba...@force-load-detect.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
- fi-glk-dsi: NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#533])
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-glk-dsi/igt@kms_pipe_crc_ba...@compare-crc-sanitycheck-pipe-d.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
- fi-bdw-5557u:   [PASS][12] -> [INCOMPLETE][13] ([i915#146])
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/fi-bdw-5557u/igt@kms_pipe_crc_ba...@suspend-read-crc-pipe-a.html
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-bdw-5557u/igt@kms_pipe_crc_ba...@suspend-read-crc-pipe-a.html

  * igt@kms_psr@primary_mmap_gtt:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][14] ([i915#1072]) +3 similar issues
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@kms_psr@primary_mmap_gtt.html

  * igt@kms_psr@primary_page_flip:
- fi-glk-dsi: NOTRUN -> [SKIP][15] ([fdo#109271]) +30 similar issues
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-glk-dsi/igt@kms_psr@primary_page_flip.html

  * igt@prime_vgem@basic-userptr:
- fi-tgl-1115g4:  NOTRUN -> [SKIP][16] ([i915#3301])
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-tgl-1115g4/igt@prime_v...@basic-userptr.html

  
 Possible fixes 

  * igt@i915_selftest@live@execlists:
- fi-bsw-kefka:   [INCOMPLETE][17] ([i915#2940]) -> [PASS][18]
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10834/fi-bsw-kefka/igt@i915_selftest@l...@execlists.html
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21512/fi-bsw-kefka/igt@i915_selftest@l...@execlists.html

  * igt@i915_selftest@live@gt_heartbeat:
- fi-bxt-dsi: [DMESG-FAIL][19] ([i915#541]) -> [PASS][20]
   [19]: 

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: Detect offset in context image for OA ctx control reg (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/perf: Detect offset in context image for OA ctx control reg 
(rev2)
URL   : https://patchwork.freedesktop.org/series/96507/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d73e6124b5f9 drm/i915/perf: Detect offset in context image for OA ctx control 
reg
-:60: WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: 
netdev]_alert([subsystem]dev, ... then dev_alert(dev, ... then pr_alert(...  to 
printk(KERN_ALERT ...
#60: FILE: drivers/gpu/drm/i915/selftests/i915_perf.c:189:
+   printk(KERN_ALERT "OA: offset %08x for %08x\n", offset, reg);

-:62: WARNING:PREFER_PR_LEVEL: Prefer [subsystem eg: 
netdev]_alert([subsystem]dev, ... then dev_alert(dev, ... then pr_alert(...  to 
printk(KERN_ALERT ...
#62: FILE: drivers/gpu/drm/i915/selftests/i915_perf.c:191:
+   printk(KERN_ALERT "OA: offset not found for %08x\n", reg);

total: 0 errors, 2 warnings, 0 checks, 138 lines checked

[Intel-gfx] [PATCH] drm/i915/perf: Detect offset in context image for OA ctx control reg

[Umesh Nerlige Ramappa]

Not for review. Just trying out a selftest on CI machines.

Signed-off-by: Umesh Nerlige Ramappa 
---
 drivers/gpu/drm/i915/selftests/i915_perf.c | 114 -
 1 file changed, 113 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/selftests/i915_perf.c 
b/drivers/gpu/drm/i915/selftests/i915_perf.c
index 9e9a6cb1d9e5..67b230c867c6 100644
--- a/drivers/gpu/drm/i915/selftests/i915_perf.c
+++ b/drivers/gpu/drm/i915/selftests/i915_perf.c
@@ -8,6 +8,7 @@
 
 #include "gem/i915_gem_pm.h"
 #include "gt/intel_gt.h"
+#include "gt/intel_lrc_reg.h"
 
 #include "i915_selftest.h"
 
@@ -152,6 +153,111 @@ static int live_sanitycheck(void *arg)
return 0;
 }
 
+#define MI_OPCODE(x) (((x) >> 23) & 0x3f)
+#define MI_LRI_CMD(x) (MI_OPCODE(x) == MI_OPCODE(MI_INSTR(0x22, 0)))
+#define MI_LRI_LEN(x) (((x) & 0xff) + 1)
+static int __find_reg_in_lri(u32 *state, u32 reg, u32 *offset)
+{
+   u32 idx = *offset;
+   u32 len = MI_LRI_LEN(state[idx]) + idx;
+
+   idx++;
+   for (; idx < len; idx += 2)
+   if (state[idx] == reg)
+   break;
+
+   *offset = idx;
+   return state[idx] == reg;
+}
+
+static void __context_image_offset(struct intel_context *ce, u32 reg)
+{
+   u32 *state = ce->lrc_reg_state;
+   u32 len = (ce->engine->context_size - PAGE_SIZE) / 4;
+   u32 offset;
+
+   for (offset = 0; offset < len; ) {
+   if (MI_LRI_CMD(state[offset])) {
+   if (__find_reg_in_lri(state, reg, ))
+   break;
+   } else {
+   offset++;
+   }
+   }
+
+   if (offset < len)
+   printk(KERN_ALERT "OA: offset %08x for %08x\n", offset, reg);
+   else
+   printk(KERN_ALERT "OA: offset not found for %08x\n", reg);
+}
+
+static int __live_lrc_state(struct intel_engine_cs *engine,
+   struct i915_vma *scratch)
+{
+   struct intel_context *ce;
+   struct i915_gem_ww_ctx ww;
+   i915_reg_t reg;
+   int err;
+
+   ce = intel_context_create(engine);
+   if (IS_ERR(ce))
+   return PTR_ERR(ce);
+
+   i915_gem_ww_ctx_init(, false);
+retry:
+   err = i915_gem_object_lock(scratch->obj, );
+   if (!err)
+   err = intel_context_pin_ww(ce, );
+   if (err)
+   goto err_put;
+
+   reg = GEN12_OACTXCONTROL;
+   __context_image_offset(ce, i915_mmio_reg_offset(reg));
+
+   intel_context_unpin(ce);
+err_put:
+   if (err == -EDEADLK) {
+   err = i915_gem_ww_ctx_backoff();
+   if (!err)
+   goto retry;
+   }
+   i915_gem_ww_ctx_fini();
+   intel_context_put(ce);
+   return err;
+}
+
+static int __live_oactxctrl_offset(void *arg)
+{
+   struct drm_i915_private *i915 = arg;
+   struct intel_gt *gt = >gt;
+   struct intel_engine_cs *engine;
+   struct i915_vma *scratch;
+   enum intel_engine_id id;
+   int err = 0;
+
+   if (!HAS_LOGICAL_RING_CONTEXTS(i915))
+   return 0;
+
+   scratch = __vm_create_scratch_for_read_pinned(>ggtt->vm, PAGE_SIZE);
+   if (IS_ERR(scratch))
+   return PTR_ERR(scratch);
+
+   for_each_engine(engine, gt, id) {
+   if (engine->class != RENDER_CLASS)
+   continue;
+
+   err = __live_lrc_state(engine, scratch);
+   if (err)
+   break;
+   }
+
+   if (igt_flush_test(gt->i915))
+   err = -EIO;
+
+   i915_vma_unpin_and_release(, 0);
+   return err;
+}
+
 static int write_timestamp(struct i915_request *rq, int slot)
 {
u32 *cs;
@@ -188,7 +294,7 @@ static ktime_t poll_status(struct i915_request *rq, int 
slot)
return ktime_get();
 }
 
-static int live_noa_delay(void *arg)
+static int __live_noa_delay(void *arg)
 {
struct drm_i915_private *i915 = arg;
struct i915_perf_stream *stream;
@@ -280,6 +386,12 @@ static int live_noa_delay(void *arg)
return err;
 }
 
+static int live_noa_delay(void *arg)
+{
+   __live_oactxctrl_offset(arg);
+   return __live_noa_delay(arg);
+}
+
 static int live_noa_gpr(void *arg)
 {
struct drm_i915_private *i915 = arg;
-- 
2.20.1

Re: [Intel-gfx] [PATCH 8/8] drm/amdgpu: add drm buddy support to amdgpu

[Matthew Auld]

On 25/10/2021 14:00, Arunpravin wrote:

- Remove drm_mm references and replace with drm buddy functionalities
- Add res cursor support for drm buddy

Signed-off-by: Arunpravin 





+   spin_lock(>lock);
+   r = drm_buddy_alloc(mm, (uint64_t)place->fpfn << PAGE_SHIFT,
+   (uint64_t)lpfn << PAGE_SHIFT,
+   (uint64_t)n_pages << PAGE_SHIFT,
+min_page_size, >blocks,
+node->flags);



Is spinlock + GFP_KERNEL allowed?


+   spin_unlock(>lock);
+
+   if (unlikely(r))
+   goto error_free_blocks;
+
pages_left -= pages;
++i;
  
  		if (pages > pages_left)

pages = pages_left;
}
-   spin_unlock(>lock);
+
+   /* Free unused pages for contiguous allocation */
+   if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
+   uint64_t actual_size = (uint64_t)node->base.num_pages << 
PAGE_SHIFT;
+
+   r = drm_buddy_free_unused_pages(mm,
+   actual_size,
+   >blocks);


Needs some locking.

Re: [Intel-gfx] [PATCH 6/8] drm/i915: add free_unused_pages support to i915

[Matthew Auld]

On 25/10/2021 14:00, Arunpravin wrote:

add drm_buddy_free_unused_pages() support on
contiguous allocation

Signed-off-by: Arunpravin 
---
  drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 8 
  1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c 
b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index 963468228392..162947af8e04 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -98,6 +98,14 @@ static int i915_ttm_buddy_man_alloc(struct 
ttm_resource_manager *man,
if (unlikely(err))
goto err_free_blocks;
  
+	if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {

+   err = drm_buddy_free_unused_pages(mm, (uint64_t)n_pages << 
PAGE_SHIFT,
+  _res->blocks);
+
+   if (unlikely(err))
+   goto err_free_blocks;


That needs some locking, mark_free/mark_split are all globally visible. 
Some concurrent user might steal the block, or worse.



+   }
+
*res = _res->base;
return 0;
  

Re: [Intel-gfx] [PATCH 5/8] drm: Implement method to free unused pages

[Matthew Auld]

On 25/10/2021 14:00, Arunpravin wrote:

On contiguous allocation, we round up the size
to the *next* power of 2, implement a function
to free the unused pages after the newly allocate block.

Signed-off-by: Arunpravin 


Ideally this gets added with some user, so we can see it in action? 
Maybe squash the next patch here?



---
  drivers/gpu/drm/drm_buddy.c | 103 
  include/drm/drm_buddy.h |   4 ++
  2 files changed, 107 insertions(+)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 9d3547bcc5da..0da8510736eb 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -284,6 +284,109 @@ static inline bool contains(u64 s1, u64 e1, u64 s2, u64 
e2)
return s1 <= s2 && e1 >= e2;
  }
  
+/**

+ * drm_buddy_free_unused_pages - free unused pages
+ *
+ * @mm: DRM buddy manager
+ * @actual_size: original size requested
+ * @blocks: output list head to add allocated blocks
+ *
+ * For contiguous allocation, we round up the size to the nearest
+ * power of two value, drivers consume *actual* size, so remaining
+ * portions are unused and it can be freed.
+ *
+ * Returns:
+ * 0 on success, error code on failure.
+ */
+int drm_buddy_free_unused_pages(struct drm_buddy_mm *mm,


drm_buddy_block_trim?


+   u64 actual_size,


new_size?


+   struct list_head *blocks)
+{
+   struct drm_buddy_block *block;
+   struct drm_buddy_block *buddy;
+   u64 actual_start;
+   u64 actual_end;
+   LIST_HEAD(dfs);
+   u64 count = 0;
+   int err;
+
+   if (!list_is_singular(blocks))
+   return -EINVAL;
+
+   block = list_first_entry_or_null(blocks,
+struct drm_buddy_block,
+link);
+
+   if (!block)
+   return -EINVAL;


list_is_singular() already ensures that I guess?



+
+   if (actual_size > drm_buddy_block_size(mm, block))
+   return -EINVAL;
+
+   if (actual_size == drm_buddy_block_size(mm, block))
+   return 0;


Probably need to check the alignment of the actual_size, and also check 
that it is non-zero?



+
+   list_del(>link);
+
+   actual_start = drm_buddy_block_offset(block);
+   actual_end = actual_start + actual_size - 1;
+
+   if (drm_buddy_block_is_allocated(block))


That should rather be a programmer error.


+   mark_free(mm, block);
+
+   list_add(>tmp_link, );
+
+   while (1) {
+   block = list_first_entry_or_null(,
+struct drm_buddy_block,
+tmp_link);
+
+   if (!block)
+   break;
+
+   list_del(>tmp_link);
+
+   if (count == actual_size)
+   return 0;



Check for overlaps somewhere here to avoid needless searching and splitting?


+
+   if (contains(actual_start, actual_end, 
drm_buddy_block_offset(block),
+   (drm_buddy_block_offset(block) + 
drm_buddy_block_size(mm, block) - 1))) {


Could maybe record the start/end for better readability?


+   BUG_ON(!drm_buddy_block_is_free(block));
+
+   /* Allocate only required blocks */
+   mark_allocated(block);
+   mm->avail -= drm_buddy_block_size(mm, block);
+   list_add_tail(>link, blocks);
+   count += drm_buddy_block_size(mm, block);
+   continue;
+   }
+
+   if (drm_buddy_block_order(block) == 0)
+   continue;


Should be impossible with overlaps check added.


+
+   if (!drm_buddy_block_is_split(block)) {


That should always be true.


+   err = split_block(mm, block);
+
+   if (unlikely(err))
+   goto err_undo;
+   }
+
+   list_add(>right->tmp_link, );
+   list_add(>left->tmp_link, );
+   }
+
+   return -ENOSPC;



Would it make sense to factor out part of the alloc_range for this? It 
looks roughly the same.



+
+err_undo:
+   buddy = get_buddy(block);
+   if (buddy &&
+   (drm_buddy_block_is_free(block) &&
+drm_buddy_block_is_free(buddy)))
+   __drm_buddy_free(mm, block);
+   return err;



Where do we add the block back to the original list? Did we not just 
leak it?




+}
+EXPORT_SYMBOL(drm_buddy_free_unused_pages);
+
  static struct drm_buddy_block *
  alloc_range(struct drm_buddy_mm *mm,
u64 start, u64 end,
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index cd8021d2d6e7..1dfc80c88e1f 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@ -145,6 +145,10 @@ int drm_buddy_alloc(struct drm_buddy_mm 

Re: [Intel-gfx] [PATCH 3/8] drm: implement top-down allocation method

[Matthew Auld]

On 25/10/2021 14:00, Arunpravin wrote:

Implemented a function which walk through the order list,
compares the offset and returns the maximum offset block,
this method is unpredictable in obtaining the high range
address blocks which depends on allocation and deallocation.
for instance, if driver requests address at a low specific
range, allocator traverses from the root block and splits
the larger blocks until it reaches the specific block and
in the process of splitting, lower orders in the freelist
are occupied with low range address blocks and for the
subsequent TOPDOWN memory request we may return the low
range blocks.To overcome this issue, we may go with the
below approach.

The other approach, sorting each order list entries in
ascending order and compares the last entry of each
order list in the freelist and return the max block.
This creates sorting overhead on every drm_buddy_free()
request and split up of larger blocks for a single page
request.

Signed-off-by: Arunpravin 
---
  drivers/gpu/drm/drm_buddy.c | 42 +++--
  include/drm/drm_buddy.h |  1 +
  2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 406e3d521903..9d3547bcc5da 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -362,6 +362,27 @@ alloc_range(struct drm_buddy_mm *mm,
return ERR_PTR(err);
  }
  
+static struct drm_buddy_block *

+get_maxblock(struct list_head *head)
+{
+   struct drm_buddy_block *max_block = NULL, *node;
+
+   max_block = list_first_entry_or_null(head,
+struct drm_buddy_block,
+link);
+
+   if (!max_block)
+   return NULL;
+
+   list_for_each_entry(node, head, link) {
+   if (drm_buddy_block_offset(node) >
+   drm_buddy_block_offset(max_block))


Alignment.


+   max_block = node;
+   }


I suppose there will be pathological cases where this will unnecessarily 
steal the mappable portion? But in practice maybe this is good enough?



+
+   return max_block;
+}
+
  static struct drm_buddy_block *
  alloc_from_freelist(struct drm_buddy_mm *mm,
unsigned int order,
@@ -372,13 +393,22 @@ alloc_from_freelist(struct drm_buddy_mm *mm,
int err;
  
  	for (i = order; i <= mm->max_order; ++i) {

-   if (!list_empty(>free_list[i])) {
-   block = list_first_entry_or_null(>free_list[i],
-struct drm_buddy_block,
-link);
+   if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
+   if (!list_empty(>free_list[i])) {


AFAIK no need to keep checking list_empty(), below also.


+   block = get_maxblock(>free_list[i]);
  
-			if (block)

-   break;
+   if (block)
+   break;
+   }
+   } else {
+   if (!list_empty(>free_list[i])) {
+   block = 
list_first_entry_or_null(>free_list[i],
+struct 
drm_buddy_block,
+link);
+
+   if (block)
+   break;
+   }
}
}
  
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h

index c7bb5509a7ad..cd8021d2d6e7 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@ -28,6 +28,7 @@
  })
  
  #define DRM_BUDDY_RANGE_ALLOCATION (1 << 0)

+#define DRM_BUDDY_TOPDOWN_ALLOCATION (1 << 1)
  
  struct drm_buddy_block {

  #define DRM_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[John Harrison]

On 11/3/2021 02:36, Petri Latvala wrote:

On Tue, Nov 02, 2021 at 06:45:38PM -0700, John Harrison wrote:

On 11/2/2021 16:34, Matthew Brost wrote:

On Thu, Oct 21, 2021 at 04:40:40PM -0700, john.c.harri...@intel.com wrote:

From: John Harrison 

Some of the capture tests were using explicit contexts, some not. Some
were poking the per engine pre-emption timeout, some not. This would
lead to sporadic failures due to random timeouts, contexts being
banned depending upon how many subtests were run and/or how many
engines a given platform has, and other such failures.

So, update all tests to be conistent.

Signed-off-by: John Harrison 
---
   tests/i915/gem_exec_capture.c | 80 +--
   1 file changed, 58 insertions(+), 22 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index c85c198f7..e373d24ed 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset 
*obj_offsets, int obj_count,
return blobs;
   }
+static void configure_hangs(int fd, const struct intel_execution_engine2 *e, 
int ctxt_id)
+{
+   /* Ensure fast hang detection */
+   gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 
250);
+   gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 
500);

#define for 250, 500?

Is there any point? There is no special reason for the values other than
small enough to be fast and long enough to not be too small to be usable. So
there isn't really any particular name to give them beyond
'SHORT_PREEMPT_TIMEOUT' or some such. And the whole point of the helper
function is that the values are programmed in one place only and not used
anywhere else. So there is no worry about repetition of magic numbers.

In about one year everyone has forgotten this explanation and will
wonder if it's related to some in-kernel behaviour or if there's some
other reason these values have been chosen.

So at least a comment why the values are these, please.
There is a comment already. Not sure what more can be added that is 
meaningful other than changing it to "Ensure fast hang detection by 
picking some random numbers out of the air that seem to be vaguely 
plausible".


John.

Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Split pre-skl primary plane update into noarm+arm pair

[Lisovskiy, Stanislav]

On Thu, Oct 21, 2021 at 12:27:57AM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Chop i9xx_plane_update() into two halves. Fist half becomes
> the _noarm() variant, second part the _arm() variant.
> 
> Fortunately I have already previously grouped the register
> writes into roughtly the correct order, so the split looks
> surprisingly clean.
> 
> One slightly surprising fact was that the CHV pipe B PRIMPOS/SIZE
> registers are self arming unlike their pre-ctg DSPPOS/SIZE
> counterparts. In fact all the new CHV pipe B registers are
> self arming.
> 
> Also we must remind ourselves that i830/i845 are a bit borked
> in that all of their plane registers are self-arming.
> 
> I didn't do any i915_update_info measurements for this one
> alone. I'll get total numbers with the corrsponding sprite
> plane changes.

Reviewed-by: Stanislav Lisovskiy 

> 
> v2: Don't break my precious i830/i845
> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/i9xx_plane.c | 85 ---
>  1 file changed, 61 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c 
> b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index 93163f9100a8..66aa79abe71c 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -418,32 +418,13 @@ static int i9xx_plane_min_cdclk(const struct 
> intel_crtc_state *crtc_state,
>   return DIV_ROUND_UP(pixel_rate * num, den);
>  }
>  
> -/* TODO: split into noarm+arm pair */
> -static void i9xx_plane_update_arm(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state,
> -   const struct intel_plane_state *plane_state)
> +static void i9xx_plane_update_noarm(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> - u32 linear_offset;
> - int x = plane_state->view.color_plane[0].x;
> - int y = plane_state->view.color_plane[0].y;
> - int crtc_x = plane_state->uapi.dst.x1;
> - int crtc_y = plane_state->uapi.dst.y1;
> - int crtc_w = drm_rect_width(_state->uapi.dst);
> - int crtc_h = drm_rect_height(_state->uapi.dst);
>   unsigned long irqflags;
> - u32 dspaddr_offset;
> - u32 dspcntr;
> -
> - dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
> -
> - linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> -
> - if (DISPLAY_VER(dev_priv) >= 4)
> - dspaddr_offset = plane_state->view.color_plane[0].offset;
> - else
> - dspaddr_offset = linear_offset;
>  
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
> @@ -451,6 +432,11 @@ static void i9xx_plane_update_arm(struct intel_plane 
> *plane,
> plane_state->view.color_plane[0].stride);
>  
>   if (DISPLAY_VER(dev_priv) < 4) {
> + int crtc_x = plane_state->uapi.dst.x1;
> + int crtc_y = plane_state->uapi.dst.y1;
> + int crtc_w = drm_rect_width(_state->uapi.dst);
> + int crtc_h = drm_rect_height(_state->uapi.dst);
> +
>   /*
>* PLANE_A doesn't actually have a full window
>* generator but let's assume we still need to
> @@ -460,7 +446,39 @@ static void i9xx_plane_update_arm(struct intel_plane 
> *plane,
> (crtc_y << 16) | crtc_x);
>   intel_de_write_fw(dev_priv, DSPSIZE(i9xx_plane),
> ((crtc_h - 1) << 16) | (crtc_w - 1));
> - } else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> + }
> +
> + spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
> +}
> +
> +static void i9xx_plane_update_arm(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> + int x = plane_state->view.color_plane[0].x;
> + int y = plane_state->view.color_plane[0].y;
> + u32 dspcntr, dspaddr_offset, linear_offset;
> + unsigned long irqflags;
> +
> + dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
> +
> + linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> +
> + if (DISPLAY_VER(dev_priv) >= 4)
> + dspaddr_offset = plane_state->view.color_plane[0].offset;
> + else
> + dspaddr_offset = linear_offset;
> +
> + spin_lock_irqsave(_priv->uncore.lock, irqflags);
> +
> + if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> + int crtc_x = 

Re: [Intel-gfx] [PATCH 3/9] drm/i915: Fix up the sprite namespacing

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:24PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Give all sprite exclusive functions/etc. a proper namespace.

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_sprite.c | 106 ++--
>  1 file changed, 53 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
> b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 08116f41da26..1daa3360cf02 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -118,7 +118,7 @@ static void i9xx_plane_linear_gamma(u16 gamma[8])
>  }
>  
>  static void
> -chv_update_csc(const struct intel_plane_state *plane_state)
> +chv_sprite_update_csc(const struct intel_plane_state *plane_state)
>  {
>   struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> @@ -190,7 +190,7 @@ chv_update_csc(const struct intel_plane_state 
> *plane_state)
>  #define COS_0 1
>  
>  static void
> -vlv_update_clrc(const struct intel_plane_state *plane_state)
> +vlv_sprite_update_clrc(const struct intel_plane_state *plane_state)
>  {
>   struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> @@ -393,7 +393,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state 
> *crtc_state,
>   return sprctl;
>  }
>  
> -static void vlv_update_gamma(const struct intel_plane_state *plane_state)
> +static void vlv_sprite_update_gamma(const struct intel_plane_state 
> *plane_state)
>  {
>   struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> @@ -417,9 +417,9 @@ static void vlv_update_gamma(const struct 
> intel_plane_state *plane_state)
>  }
>  
>  static void
> -vlv_update_plane(struct intel_plane *plane,
> -  const struct intel_crtc_state *crtc_state,
> -  const struct intel_plane_state *plane_state)
> +vlv_sprite_update(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum pipe pipe = plane->pipe;
> @@ -455,7 +455,7 @@ vlv_update_plane(struct intel_plane *plane,
>   intel_de_write_fw(dev_priv, SPCONSTALPHA(pipe, plane_id), 0);
>  
>   if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
> - chv_update_csc(plane_state);
> + chv_sprite_update_csc(plane_state);
>  
>   if (key->flags) {
>   intel_de_write_fw(dev_priv, SPKEYMINVAL(pipe, plane_id),
> @@ -478,15 +478,15 @@ vlv_update_plane(struct intel_plane *plane,
>   intel_de_write_fw(dev_priv, SPSURF(pipe, plane_id),
> intel_plane_ggtt_offset(plane_state) + 
> sprsurf_offset);
>  
> - vlv_update_clrc(plane_state);
> - vlv_update_gamma(plane_state);
> + vlv_sprite_update_clrc(plane_state);
> + vlv_sprite_update_gamma(plane_state);
>  
>   spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
>  }
>  
>  static void
> -vlv_disable_plane(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state)
> +vlv_sprite_disable(struct intel_plane *plane,
> +const struct intel_crtc_state *crtc_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum pipe pipe = plane->pipe;
> @@ -502,8 +502,8 @@ vlv_disable_plane(struct intel_plane *plane,
>  }
>  
>  static bool
> -vlv_plane_get_hw_state(struct intel_plane *plane,
> -enum pipe *pipe)
> +vlv_sprite_get_hw_state(struct intel_plane *plane,
> + enum pipe *pipe)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum intel_display_power_domain power_domain;
> @@ -805,7 +805,7 @@ static void ivb_sprite_linear_gamma(const struct 
> intel_plane_state *plane_state,
>   i++;
>  }
>  
> -static void ivb_update_gamma(const struct intel_plane_state *plane_state)
> +static void ivb_sprite_update_gamma(const struct intel_plane_state 
> *plane_state)
>  {
>   struct intel_plane *plane = to_intel_plane(plane_state->uapi.plane);
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> @@ -835,9 +835,9 @@ static void ivb_update_gamma(const struct 
> intel_plane_state *plane_state)
>  }
>  
>  static void
> -ivb_update_plane(struct intel_plane *plane,
> -  const struct intel_crtc_state *crtc_state,
> -  const struct intel_plane_state *plane_state)
> +ivb_sprite_update(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
>  {
>   struct 

Re: [Intel-gfx] [PATCH 4/9] drm/i915: Split update_plane() into update_noarm() + update_arm()

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:25PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> The amount of plane registers we have to write has been steadily
> increasing, putting more pressure on the vblank evasion mechanism
> and forcing us to increase its time budget. Let's try to take some
> of the pressure off by splitting plane updates into two parts:
> 1) write all non-self arming plane registers, ie. the registers
>where the write actually does nothing until a separate arming
>register is also written which will cause the hardware to latch
>the new register values at the next start of vblank
> 2) write all self arming plane registers, ie. registers which always
>just latch at the next start of vblank, and registers which also
>arm other registers to do so
> 
> Here we just provide the mechanism, but don't actually implement
> the split on any platform yet. so everything stays now in the _arm()
> hooks. Subsequently we can move a whole bunch of stuff into the
> _noarm() part, especially in more modern platforms where the number
> of registers we have to write is also the greatest. On older
> platforms this is less beneficial probably, but no real reason
> to deviate from a common behaviour.
> 
> And let's sprinkle some TODOs around the areas that will need
> adapting.

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/i9xx_plane.c | 15 ++--
>  .../gpu/drm/i915/display/intel_atomic_plane.c | 88 ++-
>  .../gpu/drm/i915/display/intel_atomic_plane.h | 23 +++--
>  drivers/gpu/drm/i915/display/intel_cursor.c   | 44 +-
>  drivers/gpu/drm/i915/display/intel_display.c  | 12 +--
>  .../drm/i915/display/intel_display_types.h| 12 ++-
>  drivers/gpu/drm/i915/display/intel_sprite.c   | 44 +-
>  .../drm/i915/display/skl_universal_plane.c| 15 ++--
>  drivers/gpu/drm/i915/i915_trace.h | 33 ++-
>  9 files changed, 192 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c 
> b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index b1439ba78f67..93163f9100a8 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -418,9 +418,10 @@ static int i9xx_plane_min_cdclk(const struct 
> intel_crtc_state *crtc_state,
>   return DIV_ROUND_UP(pixel_rate * num, den);
>  }
>  
> -static void i9xx_update_plane(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state,
> -   const struct intel_plane_state *plane_state)
> +/* TODO: split into noarm+arm pair */
> +static void i9xx_plane_update_arm(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> @@ -493,8 +494,8 @@ static void i9xx_update_plane(struct intel_plane *plane,
>   spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
>  }
>  
> -static void i9xx_disable_plane(struct intel_plane *plane,
> -const struct intel_crtc_state *crtc_state)
> +static void i9xx_plane_disable_arm(struct intel_plane *plane,
> +const struct intel_crtc_state *crtc_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> @@ -851,8 +852,8 @@ intel_primary_plane_create(struct drm_i915_private 
> *dev_priv, enum pipe pipe)
>   plane->max_stride = ilk_primary_max_stride;
>   }
>  
> - plane->update_plane = i9xx_update_plane;
> - plane->disable_plane = i9xx_disable_plane;
> + plane->update_arm = i9xx_plane_update_arm;
> + plane->disable_arm = i9xx_plane_disable_arm;
>   plane->get_hw_state = i9xx_plane_get_hw_state;
>   plane->check_plane = i9xx_plane_check;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c 
> b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 0be8c00e3db9..ae21770fc321 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -469,31 +469,72 @@ skl_next_plane_to_commit(struct intel_atomic_state 
> *state,
>   return NULL;
>  }
>  
> -void intel_update_plane(struct intel_plane *plane,
> - const struct intel_crtc_state *crtc_state,
> - const struct intel_plane_state *plane_state)
> +void intel_plane_update_noarm(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
>  {
>   struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> - 

Re: [Intel-gfx] [PATCH 6/9] drm/i915: Split pre-skl primary plane update into noarm+arm pair

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:27PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Chop i9xx_plane_update() into two halves. Fist half becomes
> the _noarm() variant, second part the _arm() variant.
> 
> Fortunately I have already previously grouped the register
> writes into roughtly the correct order, so the split looks
> surprisingly clean.
> 
> One slightly surprising fact was that the CHV pipe B PRIMPOS/SIZE
> registers are self arming unlike their pre-ctg DSPPOS/SIZE
> counterparts. In fact all the new CHV pipe B registers are
> self arming.
> 
> I didn't do any i915_update_info measurements for this one
> alone. I'll get total numbers with the corrsponding sprite
> plane changes.

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/i9xx_plane.c | 61 +++
>  1 file changed, 40 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c 
> b/drivers/gpu/drm/i915/display/i9xx_plane.c
> index 93163f9100a8..9dfd0a53e0ee 100644
> --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> @@ -418,23 +418,49 @@ static int i9xx_plane_min_cdclk(const struct 
> intel_crtc_state *crtc_state,
>   return DIV_ROUND_UP(pixel_rate * num, den);
>  }
>  
> -/* TODO: split into noarm+arm pair */
> +static void i9xx_plane_update_noarm(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(_priv->uncore.lock, irqflags);
> +
> + intel_de_write_fw(dev_priv, DSPSTRIDE(i9xx_plane),
> +   plane_state->view.color_plane[0].stride);
> +
> + if (DISPLAY_VER(dev_priv) < 4) {
> + int crtc_x = plane_state->uapi.dst.x1;
> + int crtc_y = plane_state->uapi.dst.y1;
> + int crtc_w = drm_rect_width(_state->uapi.dst);
> + int crtc_h = drm_rect_height(_state->uapi.dst);
> +
> + /*
> +  * PLANE_A doesn't actually have a full window
> +  * generator but let's assume we still need to
> +  * program whatever is there.
> +  */
> + intel_de_write_fw(dev_priv, DSPPOS(i9xx_plane),
> +   (crtc_y << 16) | crtc_x);
> + intel_de_write_fw(dev_priv, DSPSIZE(i9xx_plane),
> +   ((crtc_h - 1) << 16) | (crtc_w - 1));
> + }
> +
> + spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
> +}
> +
>  static void i9xx_plane_update_arm(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> - u32 linear_offset;
>   int x = plane_state->view.color_plane[0].x;
>   int y = plane_state->view.color_plane[0].y;
> - int crtc_x = plane_state->uapi.dst.x1;
> - int crtc_y = plane_state->uapi.dst.y1;
> - int crtc_w = drm_rect_width(_state->uapi.dst);
> - int crtc_h = drm_rect_height(_state->uapi.dst);
> + u32 dspcntr, dspaddr_offset, linear_offset;
>   unsigned long irqflags;
> - u32 dspaddr_offset;
> - u32 dspcntr;
>  
>   dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
>  
> @@ -447,20 +473,12 @@ static void i9xx_plane_update_arm(struct intel_plane 
> *plane,
>  
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
> - intel_de_write_fw(dev_priv, DSPSTRIDE(i9xx_plane),
> -   plane_state->view.color_plane[0].stride);
> + if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
> + int crtc_x = plane_state->uapi.dst.x1;
> + int crtc_y = plane_state->uapi.dst.y1;
> + int crtc_w = drm_rect_width(_state->uapi.dst);
> + int crtc_h = drm_rect_height(_state->uapi.dst);
>  
> - if (DISPLAY_VER(dev_priv) < 4) {
> - /*
> -  * PLANE_A doesn't actually have a full window
> -  * generator but let's assume we still need to
> -  * program whatever is there.
> -  */
> - intel_de_write_fw(dev_priv, DSPPOS(i9xx_plane),
> -   (crtc_y << 16) | crtc_x);
> - intel_de_write_fw(dev_priv, DSPSIZE(i9xx_plane),
> -   ((crtc_h - 1) << 16) | (crtc_w - 1));
> - } else if (IS_CHERRYVIEW(dev_priv) && i9xx_plane == PLANE_B) {
>   intel_de_write_fw(dev_priv, PRIMPOS(i9xx_plane),
> (crtc_y << 16) 

Re: [Intel-gfx] [PATCH 5/9] drm/i915: Split skl+ plane update into noarm+arm pair

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:26PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Chop skl_program_plane() into two halves. Fist half becomes
> the _noarm() variant, second part the _arm() variant.
> 
> Fortunately I have already previously grouped the register
> writes into roughtly the correct order, so the split looks
> surprisingly clean.
> 
> A few notable oddities I did not realize were self arming
> are AUX_DIST and COLOR_CTL.
> 
> i915_update_info doesn't look too terrible on my cfl running
> kms_atomic_transition --r plane-all-transition --extended:
> w/o patch   w/ patch
> Updates: 2178   Updates: 2018
>|   |
>1us |   1us |
>|   |
>4us |   4us |*
>|*  |**
>   16us |**16us |***
>|***|
>   66us |  66us |
>|   |
>  262us | 262us |
>|   |
>1ms |   1ms |
>|   |
>4ms |   4ms |
>|   |
>   17ms |  17ms |
>|   |
> Min update: 8332ns  Min update: 6164ns
> Max update: 48758ns Max update: 31808ns
> Average update: 19959ns Average update: 13159ns
> Overruns > 100us: 0 Overruns > 100us: 0
> 
> And with lockdep enabled:
> w/o patch   w/ patch
> Updates: 2177 Updates: 2172
>| |
>1us | 1us |
>| |
>4us | 4us |
>|***  |*
>   16us |**  16us |**
>|***  |*
>   66us |66us |
>| |
>  262us |   262us |
>| |
>1ms | 1ms |
>| |
>4ms | 4ms |
>| |
>   17ms |17ms |
>| |
> Min update: 12645ns   Min update: 9980ns
> Max update: 50153ns   Max update: 33533ns
> Average update: 25337ns   Average update: 18245ns
> Overruns > 250us: 0   Overruns > 250us: 0
> 
> TODO: On icl+ everything seems to be armed by PLANE_SURF, so we
>   can optimize this even further on modern platforms. But I
>   think there's a bit of refactoring to be done first to
>   figure out the best way to go about it (eg. just reusing
>   the current skl+ functions, or doing a lower level split).
> 
> TODO: Split scaler programming as well, but IIRC the scaler
>   has some oddball double buffering behaviour on some
>   platforms, so needs proper reverse engineering
>

Reviewed-by: Stanislav Lisovskiy 
 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  .../drm/i915/display/skl_universal_plane.c| 113 +++---
>  1 file changed, 72 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 74f3870d39b1..2a822e1e465e 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1050,60 +1050,32 @@ static void icl_plane_csc_load_black(struct 
> intel_plane *plane)
>  }
>  
>  static void
> -skl_program_plane(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state,
> -   const struct intel_plane_state *plane_state,
> -   int color_plane)
> +skl_program_plane_noarm(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state,
> + int color_plane)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum plane_id plane_id = plane->id;
>   enum pipe pipe = plane->pipe;
> - const struct drm_intel_sprite_colorkey *key = _state->ckey;
>   u32 stride = skl_plane_stride(plane_state, color_plane);
>   const struct drm_framebuffer *fb = plane_state->hw.fb;
> - int aux_plane = skl_main_to_aux_plane(fb, color_plane);
>   int crtc_x = plane_state->uapi.dst.x1;
>   int crtc_y = plane_state->uapi.dst.y1;
> - u32 x 

Re: [Intel-gfx] [PATCH 7/9] drm/i915: Split g4x+ sprite plane update into noarm+arm pair

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:28PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Chop g4x_sprite_update() into two halves. Fist half becomes
> the _noarm() variant, second part the _arm() variant.
> 
> Fortunately I have already previously grouped the register
> writes into roughtly the correct order, so the split looks
> surprisingly clean.
> 
> Not much of a change in i915_update_info on these older
> platforms that don't have so many planes or registers to
> begin with. Here are the numbers from snb (totally unpatched
> vs. both primary plane and sprite patched applied) running
> kms_atomic_transition --r plane-all-transition --extended:
> w/o patch   w/ patch
> Updates: 5404 Updates: 5405
>| |
>1us |**   1us |**
>|*|*
>4us |***  4us |***
>|**   |**
>   16us |**  16us |**
>| |
>   66us |66us |
>| |
>  262us |   262us |
>| |
>1ms | 1ms |
>| |
>4ms | 4ms |
>| |
>   17ms |17ms |
>| |
> Min update: 1400nsMin update: 1307ns
> Max update: 19809ns   Max update: 20194ns
> Average update: 6957nsAverage update: 6432ns
> Overruns > 100us: 0   Overruns > 100us: 0
> 
> But there seems to be a slight improvement with
> lockdep enabled:
> w/o patch   w/ patch
> Updates: 17612Updates: 16364
>| |
>1us | 1us |
>|**   |**
>4us |**   4us |**
>| |*
>   16us |*   16us |
>|***  |*
>   66us |66us |
>| |
>  262us |   262us |
>| |
>1ms | 1ms |
>| |
>4ms | 4ms |
>| |
>   17ms |17ms |
>| |
> Min update: 3141nsMin update: 3562ns
> Max update: 126450ns  Max update: 73354ns
> Average update: 16373ns   Average update: 15153ns
> Overruns > 250us: 0   Overruns > 250us: 0

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_sprite.c | 41 ++---
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
> b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 9c36c1492b33..03e3bf890ce9 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -1165,28 +1165,21 @@ static void ilk_sprite_update_gamma(const struct 
> intel_plane_state *plane_state)
>  }
>  
>  static void
> -g4x_sprite_update_arm(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state,
> -   const struct intel_plane_state *plane_state)
> +g4x_sprite_update_noarm(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum pipe pipe = plane->pipe;
> - u32 dvssurf_offset = plane_state->view.color_plane[0].offset;
> - u32 linear_offset;
> - const struct drm_intel_sprite_colorkey *key = _state->ckey;
>   int crtc_x = plane_state->uapi.dst.x1;
>   int crtc_y = plane_state->uapi.dst.y1;
>   u32 crtc_w = drm_rect_width(_state->uapi.dst);
>   u32 crtc_h = drm_rect_height(_state->uapi.dst);
> - u32 x = plane_state->view.color_plane[0].x;
> - u32 y = plane_state->view.color_plane[0].y;
>   u32 src_w = drm_rect_width(_state->uapi.src) >> 16;
>   u32 src_h = drm_rect_height(_state->uapi.src) >> 16;
> - u32 dvscntr, dvsscale = 0;
> + u32 dvsscale = 0;
>   unsigned long irqflags;
>  
> - dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
> -
>   /* Sizes are 0 based */
>   src_w--;
>   src_h--;
> @@ -1196,8 +1189,6 @@ 

Re: [Intel-gfx] [PATCH i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[John Harrison]

On 11/3/2021 07:00, Tvrtko Ursulin wrote:

On 22/10/2021 00:40, john.c.harri...@intel.com wrote:

From: John Harrison 

The sysfs file read helper does not actually report any errors if a
realloc fails. It just silently returns a 'valid' but truncated
buffer. This then leads to the decode of the buffer failing in random
ways. So, add a check for ENOMEM being generated during the read.

Signed-off-by: John Harrison 
---
  tests/i915/gem_exec_capture.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tests/i915/gem_exec_capture.c 
b/tests/i915/gem_exec_capture.c

index e373d24ed..8997125ee 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -131,9 +131,11 @@ static int check_error_state(int dir, struct 
offset *obj_offsets, int obj_count,

  char *error, *str;
  int blobs = 0;
  +    errno = 0;
  error = igt_sysfs_get(dir, "error");
  igt_sysfs_set(dir, "error", "Begone!");
  igt_assert(error);
+    igt_assert(errno != ENOMEM);


igt_sysfs_get:

len = 64;
...
    newbuf = realloc(buf, 2*len);

Maybe the problem is doubling goes out of hand. How big are your 
buffers? Perhaps you could improve the library function instead to 
grow less aggressively.
The buffers are generally ending at 2GB in size with the capture being 
about 1.8GB (on the particular system I happen to be testing on).


I considered various options such as doubling until a given size and 
then just incrementing by fixed amounts. But where do you draw the line? 
1MB, 128MB, 1GB, 128GB? If the final result needs to be 128GB (which you 
cannot know until you have finished reading and resizing) and you are 
allocating in 1MB chunks then it is going to take a very long time to 
get there. I ended up leaving it as a straight double on the grounds 
that it is the best compromise between overallocation and taking 
ridiculous numbers of steps.






And at the same time perhaps the bug is this:

    if (igt_debug_on(!newbuf))
    break;
...
    return buf;

So failures to grow the buffer are ignored, while failure to allocate 
the initial one are not. Perhaps both should return NULL and then 
callers would not be surprised.


Or you think someone relies on this current odd behaviour?

As per the commit description, this is exactly the problem. However, I 
do not know for certain this is not intentional behaviour and something 
somewhere is relying on it. And I really do not have the time to audit 
this. The vast majority of uses are reading teeny tiny files and don't 
care but who knows what might not be in some particular 
test/config/platform/etc. The fact that it is explicitly saying 
'igt_debug_on' means that someone must have made a conscious decision to 
not assert. It's not like they just forgot to check for null being 
returned. Which implies it is intentional and required.


John.



Regards,

Tvrtko


  igt_debug("%s\n", error);
    /* render ring --- user = 0x d000 */

Re: [Intel-gfx] [PATCH 8/9] drm/i915: Split ivb+ sprite plane update into noarm+arm pair

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:29PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Chop ivb_sprite_update() into two halves. Fist half becomes
> the _noarm() variant, second part the _arm() variant.
> 
> Fortunately I have already previously grouped the register
> writes into roughtly the correct order, so the split looks
> surprisingly clean.
> 
> Didn't bother with i915_update_info numbers for this one.
> I expect the results to be pretty much identical to the snb
> numbers from the corresponding g4x+ sprite modification.

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_sprite.c | 42 ++---
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
> b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 03e3bf890ce9..4e5f95aebeca 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -835,30 +835,22 @@ static void ivb_sprite_update_gamma(const struct 
> intel_plane_state *plane_state)
>   i++;
>  }
>  
> -/* TODO: split into noarm+arm pair */
>  static void
> -ivb_sprite_update_arm(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state,
> -   const struct intel_plane_state *plane_state)
> +ivb_sprite_update_noarm(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum pipe pipe = plane->pipe;
> - u32 sprsurf_offset = plane_state->view.color_plane[0].offset;
> - u32 linear_offset;
> - const struct drm_intel_sprite_colorkey *key = _state->ckey;
>   int crtc_x = plane_state->uapi.dst.x1;
>   int crtc_y = plane_state->uapi.dst.y1;
>   u32 crtc_w = drm_rect_width(_state->uapi.dst);
>   u32 crtc_h = drm_rect_height(_state->uapi.dst);
> - u32 x = plane_state->view.color_plane[0].x;
> - u32 y = plane_state->view.color_plane[0].y;
>   u32 src_w = drm_rect_width(_state->uapi.src) >> 16;
>   u32 src_h = drm_rect_height(_state->uapi.src) >> 16;
> - u32 sprctl, sprscale = 0;
> + u32 sprscale = 0;
>   unsigned long irqflags;
>  
> - sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> -
>   /* Sizes are 0 based */
>   src_w--;
>   src_h--;
> @@ -868,8 +860,6 @@ ivb_sprite_update_arm(struct intel_plane *plane,
>   if (crtc_w != src_w || crtc_h != src_h)
>   sprscale = SPRITE_SCALE_ENABLE | (src_w << 16) | src_h;
>  
> - linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> -
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
>   intel_de_write_fw(dev_priv, SPRSTRIDE(pipe),
> @@ -879,6 +869,29 @@ ivb_sprite_update_arm(struct intel_plane *plane,
>   if (IS_IVYBRIDGE(dev_priv))
>   intel_de_write_fw(dev_priv, SPRSCALE(pipe), sprscale);
>  
> + spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
> +}
> +
> +static void
> +ivb_sprite_update_arm(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum pipe pipe = plane->pipe;
> + const struct drm_intel_sprite_colorkey *key = _state->ckey;
> + u32 sprsurf_offset = plane_state->view.color_plane[0].offset;
> + u32 x = plane_state->view.color_plane[0].x;
> + u32 y = plane_state->view.color_plane[0].y;
> + u32 sprctl, linear_offset;
> + unsigned long irqflags;
> +
> + sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> +
> + linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> +
> + spin_lock_irqsave(_priv->uncore.lock, irqflags);
> +
>   if (key->flags) {
>   intel_de_write_fw(dev_priv, SPRKEYVAL(pipe), key->min_value);
>   intel_de_write_fw(dev_priv, SPRKEYMSK(pipe),
> @@ -1796,6 +1809,7 @@ intel_sprite_plane_create(struct drm_i915_private 
> *dev_priv,
>  
>   plane_funcs = _sprite_funcs;
>   } else if (DISPLAY_VER(dev_priv) >= 7) {
> + plane->update_noarm = ivb_sprite_update_noarm;
>   plane->update_arm = ivb_sprite_update_arm;
>   plane->disable_arm = ivb_sprite_disable_arm;
>   plane->get_hw_state = ivb_sprite_get_hw_state;
> -- 
> 2.32.0
> 

Re: [Intel-gfx] [PATCH 9/9] drm/i915: Split vlv/chv sprite plane update into noarm+arm pair

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:30PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Chop vlv_sprite_update() into two halves. Fist half becomes
> the _noarm() variant, second part the _arm() variant.
> 
> Fortunately I have already previously grouped the register
> writes into roughtly the correct order, so the split looks
> surprisingly clean.
> 
> Looks like most of the hardware logic wa scopied from the
> pre-ctg sprite C, so SPSTRIDE/POS/SIZE are armed by SPSURF,
> while the rest are self arming. SPCONSTALPHA is the one
> entirely new register that didn't exist in the old sprite C,
> and looks like that one is self arming. The CHV pipe B CSC
> is also self arming, like the rest of the CHV pipe B
> additions.
> 
> I didn't have time to capture i915_update_info numbers for
> these, but since all the other platforms generally showed
> improvements, and crucially no regression, I am fairly
> confident this should behave similarly.

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Stanislav Lisovskiy 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_sprite.c | 45 ++---
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c 
> b/drivers/gpu/drm/i915/display/intel_sprite.c
> index 4e5f95aebeca..fc6ecb41a40e 100644
> --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> @@ -416,35 +416,24 @@ static void vlv_sprite_update_gamma(const struct 
> intel_plane_state *plane_state)
> gamma[i] << 16 | gamma[i] << 8 | gamma[i]);
>  }
>  
> -/* TODO: split into noarm+arm pair */
>  static void
> -vlv_sprite_update_arm(struct intel_plane *plane,
> -   const struct intel_crtc_state *crtc_state,
> -   const struct intel_plane_state *plane_state)
> +vlv_sprite_update_noarm(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>   enum pipe pipe = plane->pipe;
>   enum plane_id plane_id = plane->id;
> - u32 sprsurf_offset = plane_state->view.color_plane[0].offset;
> - u32 linear_offset;
> - const struct drm_intel_sprite_colorkey *key = _state->ckey;
>   int crtc_x = plane_state->uapi.dst.x1;
>   int crtc_y = plane_state->uapi.dst.y1;
>   u32 crtc_w = drm_rect_width(_state->uapi.dst);
>   u32 crtc_h = drm_rect_height(_state->uapi.dst);
> - u32 x = plane_state->view.color_plane[0].x;
> - u32 y = plane_state->view.color_plane[0].y;
>   unsigned long irqflags;
> - u32 sprctl;
> -
> - sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
>  
>   /* Sizes are 0 based */
>   crtc_w--;
>   crtc_h--;
>  
> - linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> -
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
>   intel_de_write_fw(dev_priv, SPSTRIDE(pipe, plane_id),
> @@ -453,7 +442,30 @@ vlv_sprite_update_arm(struct intel_plane *plane,
> (crtc_y << 16) | crtc_x);
>   intel_de_write_fw(dev_priv, SPSIZE(pipe, plane_id),
> (crtc_h << 16) | crtc_w);
> - intel_de_write_fw(dev_priv, SPCONSTALPHA(pipe, plane_id), 0);
> +
> + spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
> +}
> +
> +static void
> +vlv_sprite_update_arm(struct intel_plane *plane,
> +   const struct intel_crtc_state *crtc_state,
> +   const struct intel_plane_state *plane_state)
> +{
> + struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> + enum pipe pipe = plane->pipe;
> + enum plane_id plane_id = plane->id;
> + const struct drm_intel_sprite_colorkey *key = _state->ckey;
> + u32 sprsurf_offset = plane_state->view.color_plane[0].offset;
> + u32 x = plane_state->view.color_plane[0].x;
> + u32 y = plane_state->view.color_plane[0].y;
> + u32 sprctl, linear_offset;
> + unsigned long irqflags;
> +
> + sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
> +
> + linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> +
> + spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
>   if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_B)
>   chv_sprite_update_csc(plane_state);
> @@ -467,6 +479,8 @@ vlv_sprite_update_arm(struct intel_plane *plane,
> key->max_value);
>   }
>  
> + intel_de_write_fw(dev_priv, SPCONSTALPHA(pipe, plane_id), 0);
> +
>   intel_de_write_fw(dev_priv, SPLINOFF(pipe, plane_id), linear_offset);
>   intel_de_write_fw(dev_priv, SPTILEOFF(pipe, plane_id), (y << 16) | x);
>  
> @@ -1791,6 +1805,7 @@ intel_sprite_plane_create(struct drm_i915_private 
> *dev_priv,
>   return plane;
>  
>   if (IS_VALLEYVIEW(dev_priv) || 

Re: [Intel-gfx] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[John Harrison]

On 11/3/2021 06:50, Tvrtko Ursulin wrote:

On 22/10/2021 00:40, john.c.harri...@intel.com wrote:

From: John Harrison 

The 'many' test ended with an 'assert(count)', presumably meaning to
ensure that some objects were actually captured. However, 'count' is
the number of objects created not how many were captured. Plus, there
is already a 'require(count > 1)' at the start and count is invarient
so the final assert is basically pointless.

General concensus appears to be that the test should not fail
irrespective of how many blobs are captured as low memory situations
could cause the capture to be abbreviated. So just remove the
pointless assert completely.


Hm the test appears to be using intel_get_avail_ram_mb() to size the 
working set. Suggesting problems with low memory situations should not 
apply unless bugs. In which case would a better fix be improving the 
sizing logic and changing the assert to igt_assert(blobs)?
After fixing the sysfs read code to cope with large files, I don't ever 
see abbreviated captures any more. However, other reviewers objected to 
asserting anything at all about the final count (whether full size, zero 
or whatever) on the grounds that low memory issues *might* still occur. 
And some in quite blunt language as I recall. If you think different, 
feel free to start your own patch set.


John.



Regards,

Tvrtko


Signed-off-by: John Harrison 
---
  tests/i915/gem_exec_capture.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/tests/i915/gem_exec_capture.c 
b/tests/i915/gem_exec_capture.c

index 7e0a8b8ad..53649cdb2 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, 
unsigned int flags)

  }
  igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
   blobs, size >> 12, count);
-    igt_assert(count);
    free(error);
  free(offsets);

Re: [Intel-gfx] [PATCH v2 2/8] drm: improve drm_buddy_alloc function

[Matthew Auld]

On 25/10/2021 14:00, Arunpravin wrote:

- Make drm_buddy_alloc a single function to handle
   range allocation and non-range allocation demands

- Implemented a new function alloc_range() which allocates
   the requested power-of-two block comply with range limitations

- Moved order computation and memory alignment logic from
   i915 driver to drm buddy

V2:
merged below changes to keep the build unbroken
- drm_buddy_alloc_range() becomes obsolete and may be removed
- enable ttm range allocation (fpfn / lpfn) support in i915 driver
- apply enhanced drm_buddy_alloc() function to i915 driver

Signed-off-by: Arunpravin 
---
  drivers/gpu/drm/drm_buddy.c   | 265 +++---
  drivers/gpu/drm/i915/i915_ttm_buddy_manager.c |  67 ++---
  drivers/gpu/drm/i915/i915_ttm_buddy_manager.h |   2 +
  include/drm/drm_buddy.h   |  22 +-
  4 files changed, 207 insertions(+), 149 deletions(-)

diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 39eb1d224bec..406e3d521903 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -274,63 +274,6 @@ void drm_buddy_free_list(struct drm_buddy_mm *mm, struct 
list_head *objects)
  }
  EXPORT_SYMBOL(drm_buddy_free_list);
  
-/**

- * drm_buddy_alloc - allocate power-of-two blocks
- *
- * @mm: DRM buddy manager to allocate from
- * @order: size of the allocation
- *
- * The order value here translates to:
- *
- * 0 = 2^0 * mm->chunk_size
- * 1 = 2^1 * mm->chunk_size
- * 2 = 2^2 * mm->chunk_size
- *
- * Returns:
- * allocated ptr to the _buddy_block on success
- */
-struct drm_buddy_block *
-drm_buddy_alloc(struct drm_buddy_mm *mm, unsigned int order)
-{
-   struct drm_buddy_block *block = NULL;
-   unsigned int i;
-   int err;
-
-   for (i = order; i <= mm->max_order; ++i) {
-   block = list_first_entry_or_null(>free_list[i],
-struct drm_buddy_block,
-link);
-   if (block)
-   break;
-   }
-
-   if (!block)
-   return ERR_PTR(-ENOSPC);
-
-   BUG_ON(!drm_buddy_block_is_free(block));
-
-   while (i != order) {
-   err = split_block(mm, block);
-   if (unlikely(err))
-   goto out_free;
-
-   /* Go low */
-   block = block->left;
-   i--;
-   }
-
-   mark_allocated(block);
-   mm->avail -= drm_buddy_block_size(mm, block);
-   kmemleak_update_trace(block);
-   return block;
-
-out_free:
-   if (i != order)
-   __drm_buddy_free(mm, block);
-   return ERR_PTR(err);
-}
-EXPORT_SYMBOL(drm_buddy_alloc);
-
  static inline bool overlaps(u64 s1, u64 e1, u64 s2, u64 e2)
  {
return s1 <= e2 && e1 >= s2;
@@ -341,52 +284,22 @@ static inline bool contains(u64 s1, u64 e1, u64 s2, u64 
e2)
return s1 <= s2 && e1 >= e2;
  }
  
-/**

- * drm_buddy_alloc_range - allocate range
- *
- * @mm: DRM buddy manager to allocate from
- * @blocks: output list head to add allocated blocks
- * @start: start of the allowed range for this block
- * @size: size of the allocation
- *
- * Intended for pre-allocating portions of the address space, for example to
- * reserve a block for the initial framebuffer or similar, hence the 
expectation
- * here is that drm_buddy_alloc() is still the main vehicle for
- * allocations, so if that's not the case then the drm_mm range allocator is
- * probably a much better fit, and so you should probably go use that instead.
- *
- * Note that it's safe to chain together multiple alloc_ranges
- * with the same blocks list
- *
- * Returns:
- * 0 on success, error code on failure.
- */
-int drm_buddy_alloc_range(struct drm_buddy_mm *mm,
- struct list_head *blocks,
- u64 start, u64 size)
+static struct drm_buddy_block *
+alloc_range(struct drm_buddy_mm *mm,
+   u64 start, u64 end,
+   unsigned int order)
  {
struct drm_buddy_block *block;
struct drm_buddy_block *buddy;
-   LIST_HEAD(allocated);
LIST_HEAD(dfs);
-   u64 end;
int err;
int i;
  
-	if (size < mm->chunk_size)

-   return -EINVAL;
-
-   if (!IS_ALIGNED(size | start, mm->chunk_size))
-   return -EINVAL;
-
-   if (range_overflows(start, size, mm->size))
-   return -EINVAL;
+   end = end - 1;
  
  	for (i = 0; i < mm->n_roots; ++i)

list_add_tail(>roots[i]->tmp_link, );
  
-	end = start + size - 1;

-
do {
u64 block_start;
u64 block_end;
@@ -394,31 +307,32 @@ int drm_buddy_alloc_range(struct drm_buddy_mm *mm,
block = list_first_entry_or_null(,
 struct drm_buddy_block,
 tmp_link);
+


No need for the newline.


 

Re: [Intel-gfx] [PATCH 2/9] drm/i915: Fix async flip with decryption and/or DPT

[Lisovskiy, Stanislav]

On Mon, Oct 18, 2021 at 02:50:23PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> We're currently forgetting to set the PLANE_SURF_DECRYPT
> flag in the async flip path. So if the hardware were to
> latch that bit despite this being an async flip we'd start
> scanning out garbage. And if it doesn't latch it then I
> guess we'd just end up with a weird register value that
> doesn't actually match the hardware state, which isn't
> great for anyone starting at register dumps.
> 
> Similarly the async flip path also forgets to call
> skl_surf_address() which means the DPT address space to
> GGTT address space downshift is not being applied to
> the offset. Which means we are pointing PLANE_SURF
> at some random location in GGTT instead of the correct
> DPT page.
> 
> So let's fix two birds with one stone and extract the
> PLANE_SURF calculation from skl_program_plane() into
> a small helper and use it in the async flip path as well.

Reviewed-by: Stanislav Lisovskiy 

> 
> Cc: Anshuman Gupta 
> Cc: Daniele Ceraolo Spurio 
> Cc: Juston Li 
> Cc: Rodrigo Vivi 
> Cc: Uma Shankar 
> Cc: Karthik B S 
> Signed-off-by: Ville Syrjälä 
> ---
>  .../drm/i915/display/skl_universal_plane.c| 30 ---
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c 
> b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 7444b88829ea..e2f024449149 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1011,6 +1011,20 @@ static u32 skl_surf_address(const struct 
> intel_plane_state *plane_state,
>   }
>  }
>  
> +static u32 skl_plane_surf(const struct intel_plane_state *plane_state,
> +   int color_plane)
> +{
> + u32 plane_surf;
> +
> + plane_surf = intel_plane_ggtt_offset(plane_state) +
> + skl_surf_address(plane_state, color_plane);
> +
> + if (plane_state->decrypt)
> + plane_surf |= PLANE_SURF_DECRYPT;
> +
> + return plane_surf;
> +}
> +
>  static void icl_plane_csc_load_black(struct intel_plane *plane)
>  {
>   struct drm_i915_private *i915 = to_i915(plane->base.dev);
> @@ -1045,7 +1059,6 @@ skl_program_plane(struct intel_plane *plane,
>   enum plane_id plane_id = plane->id;
>   enum pipe pipe = plane->pipe;
>   const struct drm_intel_sprite_colorkey *key = _state->ckey;
> - u32 surf_addr = skl_surf_address(plane_state, color_plane);
>   u32 stride = skl_plane_stride(plane_state, color_plane);
>   const struct drm_framebuffer *fb = plane_state->hw.fb;
>   int aux_plane = skl_main_to_aux_plane(fb, color_plane);
> @@ -1058,7 +1071,7 @@ skl_program_plane(struct intel_plane *plane,
>   u8 alpha = plane_state->hw.alpha >> 8;
>   u32 plane_color_ctl = 0, aux_dist = 0;
>   unsigned long irqflags;
> - u32 keymsk, keymax, plane_surf;
> + u32 keymsk, keymax;
>   u32 plane_ctl = plane_state->ctl;
>  
>   plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> @@ -1084,16 +1097,13 @@ skl_program_plane(struct intel_plane *plane,
>   }
>  
>   if (aux_plane) {
> - aux_dist = skl_surf_address(plane_state, aux_plane) - surf_addr;
> + aux_dist = skl_surf_address(plane_state, aux_plane) -
> + skl_surf_address(plane_state, color_plane);
>  
>   if (DISPLAY_VER(dev_priv) < 12)
>   aux_dist |= skl_plane_stride(plane_state, aux_plane);
>   }
>  
> - plane_surf = intel_plane_ggtt_offset(plane_state) + surf_addr;
> - if (plane_state->decrypt)
> - plane_surf |= PLANE_SURF_DECRYPT;
> -
>   spin_lock_irqsave(_priv->uncore.lock, irqflags);
>  
>   /*
> @@ -1157,7 +1167,8 @@ skl_program_plane(struct intel_plane *plane,
>* the control register just before the surface register.
>*/
>   intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
> - intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id), plane_surf);
> + intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> +   skl_plane_surf(plane_state, color_plane));
>  
>   spin_unlock_irqrestore(_priv->uncore.lock, irqflags);
>  }
> @@ -1172,7 +1183,6 @@ skl_plane_async_flip(struct intel_plane *plane,
>   unsigned long irqflags;
>   enum plane_id plane_id = plane->id;
>   enum pipe pipe = plane->pipe;
> - u32 surf_addr = plane_state->view.color_plane[0].offset;
>   u32 plane_ctl = plane_state->ctl;
>  
>   plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> @@ -1184,7 +1194,7 @@ skl_plane_async_flip(struct intel_plane *plane,
>  
>   intel_de_write_fw(dev_priv, PLANE_CTL(pipe, plane_id), plane_ctl);
>   intel_de_write_fw(dev_priv, PLANE_SURF(pipe, plane_id),
> -   intel_plane_ggtt_offset(plane_state) + surf_addr);
> +   skl_plane_surf(plane_state, 0));
>  
>   

Re: [Intel-gfx] [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Ville Syrjälä]

On Wed, Nov 03, 2021 at 01:02:11PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct 
> > drm_connector *connector,
> > u32 max_tmds_clock = hf_vsdb[5] * 5000;
> > struct drm_scdc *scdc = >scdc;
> >  
> > -   if (max_tmds_clock > 34) {
> > +   if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > display->max_tmds_clock = max_tmds_clock;
> > DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
> > display->max_tmds_clock);
> > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
> > b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > index d2e61f6c6e08..0666203d52b7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> > @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder 
> > *encoder,
> > if (scdc->scrambling.low_rates)
> > pipe_config->hdmi_scrambling = true;
> >  
> > -   if (pipe_config->port_clock > 34) {
> > +   if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
> > pipe_config->hdmi_scrambling = true;
> > pipe_config->hdmi_high_tmds_clock_ratio = true;
> > }
> 
> All of that is HDMI 2.0 stuff. So this just makes it all super
> confusing IMO. Nak.

So reading throgh HDMI 1.4 again it does specify 340 MHz as some kind
of upper limit for the physical cable. But nowhere else is that number
really mentioned AFAICS. HDMI 2.0 does talk quite a bit about the 340
Mcsc limit in various places.

I wonder what people would think of a couple of helpers like:
- drm_hdmi_{can,must}_use_scrambling()
- drm_hdmi_is_high_tmds_clock_ratio()
or something along those lines? At least with those the code would
read decently and I wouldn't have to wonder what this HDMI 1.4 TMDS
clock limit really is.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH v2 i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[Matthew Brost]

On Wed, Nov 03, 2021 at 10:04:45AM -0700, john.c.harri...@intel.com wrote:
> From: John Harrison 
> 
> Some of the capture tests were using explicit contexts, some not. Some
> were poking the per engine pre-emption timeout, some not. This would
> lead to sporadic failures due to random timeouts, contexts being
> banned depending upon how many subtests were run and/or how many
> engines a given platform has, and other such failures.
> 
> So, update all tests to be conistent.
> 
> Signed-off-by: John Harrison 

Reviewed-by: Matthew Brost 

> ---
>  tests/i915/gem_exec_capture.c | 79 +--
>  1 file changed, 57 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index c85c198f7..11c348d3b 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset 
> *obj_offsets, int obj_count,
>   return blobs;
>  }
>  
> +static void configure_hangs(int fd, const struct intel_execution_engine2 *e, 
> int ctxt_id)
> +{
> + /* Ensure fast hang detection */
> + gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 
> 250);
> + gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 
> 500);
> +
> + /* Allow engine based resets and disable banning */
> + igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
> +}
> +
>  static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t 
> *ctx,
> -unsigned ring, uint32_t target, uint64_t target_size)
> +const struct intel_execution_engine2 *e,
> +uint32_t target, uint64_t target_size)
>  {
>   const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
>   struct drm_i915_gem_exec_object2 obj[4];
> @@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
> const intel_ctx_t *ctx,
>   struct offset offset;
>   int i;
>  
> + configure_hangs(fd, e, ctx->id);
> +
>   memset(obj, 0, sizeof(obj));
>   obj[SCRATCH].handle = gem_create(fd, 4096);
>   obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
> @@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
> const intel_ctx_t *ctx,
>   memset(, 0, sizeof(execbuf));
>   execbuf.buffers_ptr = (uintptr_t)obj;
>   execbuf.buffer_count = ARRAY_SIZE(obj);
> - execbuf.flags = ring;
> + execbuf.flags = e->flags;
>   if (gen > 3 && gen < 6)
>   execbuf.flags |= I915_EXEC_SECURE;
>   execbuf.rsvd1 = ctx->id;
> @@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
> const intel_ctx_t *ctx,
>   gem_close(fd, obj[SCRATCH].handle);
>  }
>  
> -static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
> +static void capture(int fd, int dir, const intel_ctx_t *ctx,
> + const struct intel_execution_engine2 *e)
>  {
>   uint32_t handle;
>   uint64_t ahnd;
> @@ -335,7 +349,7 @@ static void capture(int fd, int dir, const intel_ctx_t 
> *ctx, unsigned ring)
>   handle = gem_create(fd, obj_size);
>   ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> - __capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
> + __capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
>  
>   gem_close(fd, handle);
>   put_ahnd(ahnd);
> @@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
>  }
>  
>  static struct offset *
> -__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> -   unsigned int size, int count,
> -   unsigned int flags)
> +__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> +const struct intel_execution_engine2 *e,
> +unsigned int size, int count, unsigned int flags)
>  #define INCREMENTAL 0x1
>  #define ASYNC 0x2
>  {
> @@ -369,6 +383,8 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>   struct offset *offsets;
>   int i;
>  
> + configure_hangs(fd, e, ctx->id);
> +
>   offsets = calloc(count, sizeof(*offsets));
>   igt_assert(offsets);
>  
> @@ -470,9 +486,10 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>   memset(, 0, sizeof(execbuf));
>   execbuf.buffers_ptr = (uintptr_t)obj;
>   execbuf.buffer_count = count + 2;
> - execbuf.flags = ring;
> + execbuf.flags = e->flags;
>   if (gen > 3 && gen < 6)
>   execbuf.flags |= I915_EXEC_SECURE;
> + execbuf.rsvd1 = ctx->id;
>  
>   igt_assert(!READ_ONCE(*seqno));
>   gem_execbuf(fd, );
> @@ -503,12 +520,27 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned 
> ring,
>   return offsets;
>  }
>  
> +#define find_first_available_engine(fd, ctx, e) \
> + do { \
> + ctx = intel_ctx_create_all_physical(fd); \
> + igt_assert(ctx); \
> + for_each_ctx_engine(fd, ctx, e) \
> + for_each_if(gem_class_can_store_dword(fd, 

[Intel-gfx] [PATCH v2 i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[John . C . Harrison]

From: John Harrison 

Some of the capture tests were using explicit contexts, some not. Some
were poking the per engine pre-emption timeout, some not. This would
lead to sporadic failures due to random timeouts, contexts being
banned depending upon how many subtests were run and/or how many
engines a given platform has, and other such failures.

So, update all tests to be conistent.

Signed-off-by: John Harrison 
---
 tests/i915/gem_exec_capture.c | 79 +--
 1 file changed, 57 insertions(+), 22 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index c85c198f7..11c348d3b 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset 
*obj_offsets, int obj_count,
return blobs;
 }
 
+static void configure_hangs(int fd, const struct intel_execution_engine2 *e, 
int ctxt_id)
+{
+   /* Ensure fast hang detection */
+   gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 
250);
+   gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 
500);
+
+   /* Allow engine based resets and disable banning */
+   igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
+}
+
 static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
-  unsigned ring, uint32_t target, uint64_t target_size)
+  const struct intel_execution_engine2 *e,
+  uint32_t target, uint64_t target_size)
 {
const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
struct drm_i915_gem_exec_object2 obj[4];
@@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
const intel_ctx_t *ctx,
struct offset offset;
int i;
 
+   configure_hangs(fd, e, ctx->id);
+
memset(obj, 0, sizeof(obj));
obj[SCRATCH].handle = gem_create(fd, 4096);
obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
@@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
const intel_ctx_t *ctx,
memset(, 0, sizeof(execbuf));
execbuf.buffers_ptr = (uintptr_t)obj;
execbuf.buffer_count = ARRAY_SIZE(obj);
-   execbuf.flags = ring;
+   execbuf.flags = e->flags;
if (gen > 3 && gen < 6)
execbuf.flags |= I915_EXEC_SECURE;
execbuf.rsvd1 = ctx->id;
@@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
const intel_ctx_t *ctx,
gem_close(fd, obj[SCRATCH].handle);
 }
 
-static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
+static void capture(int fd, int dir, const intel_ctx_t *ctx,
+   const struct intel_execution_engine2 *e)
 {
uint32_t handle;
uint64_t ahnd;
@@ -335,7 +349,7 @@ static void capture(int fd, int dir, const intel_ctx_t 
*ctx, unsigned ring)
handle = gem_create(fd, obj_size);
ahnd = get_reloc_ahnd(fd, ctx->id);
 
-   __capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
+   __capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
 
gem_close(fd, handle);
put_ahnd(ahnd);
@@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
 }
 
 static struct offset *
-__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
- unsigned int size, int count,
- unsigned int flags)
+__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
+  const struct intel_execution_engine2 *e,
+  unsigned int size, int count, unsigned int flags)
 #define INCREMENTAL 0x1
 #define ASYNC 0x2
 {
@@ -369,6 +383,8 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
struct offset *offsets;
int i;
 
+   configure_hangs(fd, e, ctx->id);
+
offsets = calloc(count, sizeof(*offsets));
igt_assert(offsets);
 
@@ -470,9 +486,10 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
memset(, 0, sizeof(execbuf));
execbuf.buffers_ptr = (uintptr_t)obj;
execbuf.buffer_count = count + 2;
-   execbuf.flags = ring;
+   execbuf.flags = e->flags;
if (gen > 3 && gen < 6)
execbuf.flags |= I915_EXEC_SECURE;
+   execbuf.rsvd1 = ctx->id;
 
igt_assert(!READ_ONCE(*seqno));
gem_execbuf(fd, );
@@ -503,12 +520,27 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
return offsets;
 }
 
+#define find_first_available_engine(fd, ctx, e) \
+   do { \
+   ctx = intel_ctx_create_all_physical(fd); \
+   igt_assert(ctx); \
+   for_each_ctx_engine(fd, ctx, e) \
+   for_each_if(gem_class_can_store_dword(fd, e->class)) \
+   break; \
+   igt_assert(e); \
+   configure_hangs(fd, e, ctx->id); \
+   } while(0)
+
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
 {
+   const struct 

[Intel-gfx] [PATCH v2 i-g-t 8/8] tests/i915/gem_exec_capture: Update to support GuC based resets

[John . C . Harrison]

From: John Harrison 

When GuC submission is enabled, GuC itself manages hang detection and
recovery. Therefore, any test that relies on being able to trigger an
engine reset in the driver will fail. Full GT resets can still be
triggered by the driver. However, in that situation detecting the
specific context that caused a hang is not possible as the driver has
no information about what is actually running on the hardware at any
given time. Plus of course, there was no context that caused the hang
because the hang was triggered manually, so it's basically a bogus
mechanism in the first place!

Update the capture test to cause a reset via a the hangcheck mechanism
by submitting a hanging batch and waiting. That way it is guaranteed to
be testing the correct reset code paths for the current platform,
whether that is GuC enabled or not.

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 tests/i915/gem_exec_capture.c | 65 ---
 1 file changed, 53 insertions(+), 12 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index e924d0a30..143d97ad4 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -23,6 +23,7 @@
 
 #include 
 #include 
+#include 
 
 #include "i915/gem.h"
 #include "i915/gem_create.h"
@@ -31,6 +32,8 @@
 #include "igt_rand.h"
 #include "igt_sysfs.h"
 
+#define MAX_RESET_TIME 600
+
 IGT_TEST_DESCRIPTION("Check that we capture the user specified objects on a 
hang");
 
 struct offset {
@@ -213,7 +216,29 @@ static void configure_hangs(int fd, const struct 
intel_execution_engine2 *e, int
gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 
500);
 
/* Allow engine based resets and disable banning */
-   igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
+   igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE | 
HANG_WANT_ENGINE_RESET);
+}
+
+static bool fence_busy(int fence)
+{
+   return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;
+}
+
+static void wait_to_die(int fence_out)
+{
+   struct timeval before, after, delta;
+
+   /* Wait for a reset to occur */
+   gettimeofday(, NULL);
+   while (fence_busy(fence_out)) {
+   gettimeofday(, NULL);
+   timersub(, , );
+   igt_assert(delta.tv_sec < MAX_RESET_TIME);
+   sched_yield();
+   }
+   gettimeofday(, NULL);
+   timersub(, , );
+   igt_info("Target died after %ld.%06lds\n", delta.tv_sec, delta.tv_usec);
 }
 
 static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
@@ -230,7 +255,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
const intel_ctx_t *ctx,
struct drm_i915_gem_execbuffer2 execbuf;
uint32_t *batch, *seqno;
struct offset offset;
-   int i;
+   int i, fence_out;
 
configure_hangs(fd, e, ctx->id);
 
@@ -315,18 +340,25 @@ static void __capture1(int fd, int dir, uint64_t ahnd, 
const intel_ctx_t *ctx,
execbuf.flags = e->flags;
if (gen > 3 && gen < 6)
execbuf.flags |= I915_EXEC_SECURE;
+   execbuf.flags |= I915_EXEC_FENCE_OUT;
execbuf.rsvd1 = ctx->id;
+   execbuf.rsvd2 = ~0UL;
 
igt_assert(!READ_ONCE(*seqno));
-   gem_execbuf(fd, );
+   gem_execbuf_wr(fd, );
+
+   fence_out = execbuf.rsvd2 >> 32;
+   igt_assert(fence_out >= 0);
 
/* Wait for the request to start */
while (READ_ONCE(*seqno) != 0xc0ffee)
igt_assert(gem_bo_busy(fd, obj[SCRATCH].handle));
munmap(seqno, 4096);
 
+   /* Wait for a reset to occur */
+   wait_to_die(fence_out);
+
/* Check that only the buffer we marked is reported in the error */
-   igt_force_gpu_reset(fd);
memset(, 0, sizeof(offset));
offset.addr = obj[CAPTURE].offset;
igt_assert_eq(check_error_state(dir, , 1, target_size, false), 
1);
@@ -373,7 +405,8 @@ static int cmp(const void *A, const void *B)
 static struct offset *
 __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
   const struct intel_execution_engine2 *e,
-  unsigned int size, int count, unsigned int flags)
+  unsigned int size, int count,
+  unsigned int flags, int *_fence_out)
 #define INCREMENTAL 0x1
 #define ASYNC 0x2
 {
@@ -383,7 +416,7 @@ __captureN(int fd, int dir, uint64_t ahnd, const 
intel_ctx_t *ctx,
struct drm_i915_gem_execbuffer2 execbuf;
uint32_t *batch, *seqno;
struct offset *offsets;
-   int i;
+   int i, fence_out;
 
configure_hangs(fd, e, ctx->id);
 
@@ -491,10 +524,17 @@ __captureN(int fd, int dir, uint64_t ahnd, const 
intel_ctx_t *ctx,
execbuf.flags = e->flags;
if (gen > 3 && gen < 6)
execbuf.flags |= I915_EXEC_SECURE;
+   execbuf.flags |= I915_EXEC_FENCE_OUT;
execbuf.rsvd1 = ctx->id;
+   execbuf.rsvd2 = ~0UL;
 

[Intel-gfx] [PATCH v2 i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[John . C . Harrison]

From: John Harrison 

The sysfs file read helper does not actually report any errors if a
realloc fails. It just silently returns a 'valid' but truncated
buffer. This then leads to the decode of the buffer failing in random
ways. So, add a check for ENOMEM being generated during the read.

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 tests/i915/gem_exec_capture.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 11c348d3b..e924d0a30 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -131,9 +131,11 @@ static int check_error_state(int dir, struct offset 
*obj_offsets, int obj_count,
char *error, *str;
int blobs = 0;
 
+   errno = 0;
error = igt_sysfs_get(dir, "error");
igt_sysfs_set(dir, "error", "Begone!");
igt_assert(error);
+   igt_assert(errno != ENOMEM);
igt_debug("%s\n", error);
 
/* render ring --- user = 0x d000 */
-- 
2.25.1

[Intel-gfx] [PATCH v2 i-g-t 7/8] lib/igt_gt: Allow per engine reset testing

[John . C . Harrison]

From: John Harrison 

With GuC submission, engine resets are handled entirely within GuC
rather than within i915. Traditionally, IGT has disallowed engine
based resets becuase they don't send the uevent which IGT uses to
check for unexpected resets. However, it is important to be able to
test all reset mechanisms that can be used, so allow engine based
resets to be enabled.

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 lib/igt_gt.c | 44 +---
 lib/igt_gt.h |  1 +
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a0ba04cc1..7c7df95ee 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -56,23 +56,28 @@
  * engines.
  */
 
+static int reset_query_once = -1;
+
 static bool has_gpu_reset(int fd)
 {
-   static int once = -1;
-   if (once < 0) {
-   struct drm_i915_getparam gp;
-   int val = 0;
-
-   memset(, 0, sizeof(gp));
-   gp.param = 35; /* HAS_GPU_RESET */
-   gp.value = 
-
-   if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, ))
-   once = intel_gen(intel_get_drm_devid(fd)) >= 5;
-   else
-   once = val > 0;
+   if (reset_query_once < 0) {
+   reset_query_once = gem_gpu_reset_type(fd);
+
+   /* Very old kernels did not support the query */
+   if (reset_query_once == -1)
+   reset_query_once =
+ (intel_gen(intel_get_drm_devid(fd)) >= 5) ? 1 : 0;
}
-   return once;
+
+   return reset_query_once > 0;
+}
+
+static bool has_engine_reset(int fd)
+{
+   if (reset_query_once < 0)
+   has_gpu_reset(fd);
+
+   return reset_query_once > 1;
 }
 
 static void eat_error_state(int dev)
@@ -176,7 +181,11 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned 
flags)
igt_skip("hang injection disabled by user [IGT_HANG=0]\n");
gem_context_require_bannable(fd);
 
-   allow_reset = 1;
+   if (flags & HANG_WANT_ENGINE_RESET)
+   allow_reset = 2;
+   else
+   allow_reset = 1;
+
if ((flags & HANG_ALLOW_CAPTURE) == 0) {
param.param = I915_CONTEXT_PARAM_NO_ERROR_CAPTURE;
param.value = 1;
@@ -187,11 +196,16 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned 
flags)
__gem_context_set_param(fd, );
allow_reset = INT_MAX; /* any reset method */
}
+
igt_require(igt_params_set(fd, "reset", "%d", allow_reset));
+   reset_query_once = -1;  /* Re-query after changing param */
 
if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
igt_require(has_gpu_reset(fd));
 
+   if (flags & HANG_WANT_ENGINE_RESET)
+   igt_require(has_engine_reset(fd));
+
ban = context_get_ban(fd, ctx);
if ((flags & HANG_ALLOW_BAN) == 0)
context_set_ban(fd, ctx, 0);
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index ceb044b86..c5059817b 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -51,6 +51,7 @@ igt_hang_t igt_hang_ctx_with_ahnd(int fd, uint64_t ahnd, 
uint32_t ctx, int ring,
 
 #define HANG_ALLOW_BAN 1
 #define HANG_ALLOW_CAPTURE 2
+#define HANG_WANT_ENGINE_RESET 4
 
 igt_hang_t igt_hang_ring(int fd, int ring);
 igt_hang_t igt_hang_ring_with_ahnd(int fd, int ring, uint64_t ahnd);
-- 
2.25.1

[Intel-gfx] [PATCH v2 i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[John . C . Harrison]

From: John Harrison 

The 'many' test ended with an 'assert(count)', presumably meaning to
ensure that some objects were actually captured. However, 'count' is
the number of objects created not how many were captured. Plus, there
is already a 'require(count > 1)' at the start and count is invarient
so the final assert is basically pointless.

General concensus appears to be that the test should not fail
irrespective of how many blobs are captured as low memory situations
could cause the capture to be abbreviated. So just remove the
pointless assert completely.

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 tests/i915/gem_exec_capture.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 7e0a8b8ad..53649cdb2 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, unsigned 
int flags)
}
igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
 blobs, size >> 12, count);
-   igt_assert(count);
 
free(error);
free(offsets);
-- 
2.25.1

[Intel-gfx] [PATCH v2 i-g-t 2/8] tests/i915/gem_exec_capture: Cope with larger page sizes

[John . C . Harrison]

From: John Harrison 

At some point, larger than 4KB page sizes were added to the i915
driver. This included adding an informational line to the buffer
entries in error capture logs. However, the error capture test was not
updated to skip this string, thus it would silently abort processing.

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 tests/i915/gem_exec_capture.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 53649cdb2..47ca64dd6 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned 
int flags)
addr |= strtoul(str + 1, , 16);
igt_assert(*str++ == '\n');
 
+   /* gtt_page_sizes = 0x0001 */
+   if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
+   str += 19 + 8;
+   igt_assert(*str++ == '\n');
+   }
+
if (!(*str == ':' || *str == '~'))
continue;
 
-- 
2.25.1

[Intel-gfx] [PATCH v2 i-g-t 3/8] tests/i915/gem_exec_capture: Make the error decode a common helper

[John . C . Harrison]

From: John Harrison 

The decode of the error capture contents was happening in two
different sub-tests with two very different pieces of code. One being
much more extensive than the other (actually decodes and verifies the
contents of the captured buffers rather than just the address). So,
move the code into a common helper function and use that in both
places.

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 tests/i915/gem_exec_capture.c | 344 +-
 1 file changed, 170 insertions(+), 174 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 47ca64dd6..c85c198f7 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -33,32 +33,175 @@
 
 IGT_TEST_DESCRIPTION("Check that we capture the user specified objects on a 
hang");
 
-static void check_error_state(int dir, struct drm_i915_gem_exec_object2 *obj)
+struct offset {
+   uint64_t addr;
+   unsigned long idx;
+   bool found;
+};
+
+static unsigned long zlib_inflate(uint32_t **ptr, unsigned long len)
+{
+   struct z_stream_s zstream;
+   void *out;
+
+   memset(, 0, sizeof(zstream));
+
+   zstream.next_in = (unsigned char *)*ptr;
+   zstream.avail_in = 4*len;
+
+   if (inflateInit() != Z_OK)
+   return 0;
+
+   out = malloc(128*4096); /* approximate obj size */
+   zstream.next_out = out;
+   zstream.avail_out = 128*4096;
+
+   do {
+   switch (inflate(, Z_SYNC_FLUSH)) {
+   case Z_STREAM_END:
+   goto end;
+   case Z_OK:
+   break;
+   default:
+   inflateEnd();
+   return 0;
+   }
+
+   if (zstream.avail_out)
+   break;
+
+   out = realloc(out, 2*zstream.total_out);
+   if (out == NULL) {
+   inflateEnd();
+   return 0;
+   }
+
+   zstream.next_out = (unsigned char *)out + zstream.total_out;
+   zstream.avail_out = zstream.total_out;
+   } while (1);
+end:
+   inflateEnd();
+   free(*ptr);
+   *ptr = out;
+   return zstream.total_out / 4;
+}
+
+static unsigned long
+ascii85_decode(char *in, uint32_t **out, bool inflate, char **end)
+{
+   unsigned long len = 0, size = 1024;
+
+   *out = realloc(*out, sizeof(uint32_t)*size);
+   if (*out == NULL)
+   return 0;
+
+   while (*in >= '!' && *in <= 'z') {
+   uint32_t v = 0;
+
+   if (len == size) {
+   size *= 2;
+   *out = realloc(*out, sizeof(uint32_t)*size);
+   if (*out == NULL)
+   return 0;
+   }
+
+   if (*in == 'z') {
+   in++;
+   } else {
+   v += in[0] - 33; v *= 85;
+   v += in[1] - 33; v *= 85;
+   v += in[2] - 33; v *= 85;
+   v += in[3] - 33; v *= 85;
+   v += in[4] - 33;
+   in += 5;
+   }
+   (*out)[len++] = v;
+   }
+   *end = in;
+
+   if (!inflate)
+   return len;
+
+   return zlib_inflate(out, len);
+}
+
+static int check_error_state(int dir, struct offset *obj_offsets, int 
obj_count,
+uint64_t obj_size, bool incremental)
 {
char *error, *str;
-   bool found = false;
+   int blobs = 0;
 
error = igt_sysfs_get(dir, "error");
igt_sysfs_set(dir, "error", "Begone!");
-
igt_assert(error);
igt_debug("%s\n", error);
 
/* render ring --- user = 0x d000 */
-   for (str = error; (str = strstr(str, "--- user = ")); str++) {
+   for (str = error; (str = strstr(str, "--- user = ")); ) {
+   uint32_t *data = NULL;
uint64_t addr;
-   uint32_t hi, lo;
+   unsigned long i, sz;
+   unsigned long start;
+   unsigned long end;
 
-   igt_assert(sscanf(str, "--- user = 0x%x %x", , ) == 2);
-   addr = hi;
+   if (strncmp(str, "--- user = 0x", 13))
+   break;
+   str += 13;
+   addr = strtoul(str, , 16);
addr <<= 32;
-   addr |= lo;
-   igt_assert_eq_u64(addr, obj->offset);
-   found = true;
+   addr |= strtoul(str + 1, , 16);
+   igt_assert(*str++ == '\n');
+
+   start = 0;
+   end = obj_count;
+   while (end > start) {
+   i = (end - start) / 2 + start;
+   if (obj_offsets[i].addr < addr)
+   start = i + 1;
+   else if 

[Intel-gfx] [PATCH v2 i-g-t 0/8] Fixes for gem_exec_capture

[John . C . Harrison]

From: John Harrison 

Fix a bunch of issues with gem_exec_capture with the ultimate aim of
making it pass on GuC enabled platforms.

v2: Abstract the 'find first available engine' block into a helper
(review feedback from Matthew B). Note that for unknown reasons, this
helper does not work as a function. After wasting far too long trying
to debug out why the engine mask in the execbuf was *sometimes*
invalid, just making it a macro instead worked fine. Seems like maybe
the for_each_ctx_engine macro creates a local stack variable that is
silently required to remain in scope for the resulting ctx to be valid?

Signed-off-by: John Harrison 


John Harrison (8):
  tests/i915/gem_exec_capture: Remove pointless assert
  tests/i915/gem_exec_capture: Cope with larger page sizes
  tests/i915/gem_exec_capture: Make the error decode a common helper
  tests/i915/gem_exec_capture: Use contexts and engines properly
  tests/i915/gem_exec_capture: Check for memory allocation failure
  lib/igt_sysfs: Support large files
  lib/igt_gt: Allow per engine reset testing
  tests/i915/gem_exec_capture: Update to support GuC based resets

 lib/igt_gt.c  |  44 ++--
 lib/igt_gt.h  |   1 +
 lib/igt_sysfs.c   |  17 +-
 tests/i915/gem_exec_capture.c | 469 --
 4 files changed, 315 insertions(+), 216 deletions(-)

-- 
2.25.1

[Intel-gfx] [PATCH v2 i-g-t 6/8] lib/igt_sysfs: Support large files

[John . C . Harrison]

From: John Harrison 

The syfs helper functions were all using basic 'int' data types for
sizs, offsets, etc. when reading from sysfs. This works fine for
little files, but not for large error capture logs (which can be
gigabytes in sizes).

Signed-off-by: John Harrison 
Reviewed-by: Matthew Brost 
---
 lib/igt_sysfs.c | 17 +++--
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 6919ac361..ee75e3ef1 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -53,9 +53,11 @@
  * provides basic support for like igt_sysfs_open().
  */
 
-static int readN(int fd, char *buf, int len)
+static ssize_t readN(int fd, char *buf, size_t len)
 {
-   int ret, total = 0;
+   ssize_t ret;
+   size_t total = 0;
+
do {
ret = read(fd, buf + total, len - total);
if (ret < 0)
@@ -69,9 +71,11 @@ static int readN(int fd, char *buf, int len)
return total ?: ret;
 }
 
-static int writeN(int fd, const char *buf, int len)
+static ssize_t writeN(int fd, const char *buf, size_t len)
 {
-   int ret, total = 0;
+   ssize_t ret;
+   size_t total = 0;
+
do {
ret = write(fd, buf + total, len - total);
if (ret < 0)
@@ -218,8 +222,9 @@ bool igt_sysfs_set(int dir, const char *attr, const char 
*value)
 char *igt_sysfs_get(int dir, const char *attr)
 {
char *buf;
-   int len, offset, rem;
-   int ret, fd;
+   size_t len, offset, rem;
+   ssize_t ret;
+   int fd;
 
fd = openat(dir, attr, O_RDONLY);
if (igt_debug_on(fd < 0))
-- 
2.25.1

Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: fixup dma_fence_wait usage

[Vudum, Lakshminarayana]

We have a bug for the regression  and I have updated the filter to include ICL 
and re-reported.
https://gitlab.freedesktop.org/drm/intel/-/issues/2370
igt@kms_cursor_legacy@cursor-vs-flip-toggle - fail - Test assertion failure 
function cursor_vs_flip, Failed assertion: fail_count == 0, Failed to meet 
cursor update expectations

Lakshmi.

-Original Message-
From: Auld, Matthew  
Sent: Wednesday, November 3, 2021 12:46 AM
To: intel-gfx@lists.freedesktop.org; Vudum, Lakshminarayana 

Subject: Re: ✗ Fi.CI.IGT: failure for drm/i915: fixup dma_fence_wait usage

On 02/11/2021 18:48, Patchwork wrote:
> *Patch Details*
> *Series:* drm/i915: fixup dma_fence_wait usage
> *URL:*https://patchwork.freedesktop.org/series/96504/ 
> 
> *State:*  failure
> *Details:*
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/index.html
> 
> 
> 
>   CI Bug Log - changes from CI_DRM_10828_full -> Patchwork_21505_full
> 
> 
> Summary
> 
> *FAILURE*
> 
> Serious unknown changes coming with Patchwork_21505_full absolutely 
> need to be verified manually.
> 
> If you think the reported changes have nothing to do with the changes 
> introduced in Patchwork_21505_full, please notify your bug team to 
> allow them to document this new failure mode, which will reduce false 
> positives in CI.
> 
> 
> Participating hosts (10 -> 10)
> 
> No changes in participating hosts
> 
> 
> Possible new issues
> 
> Here are the unknown changes that may have been introduced in
> Patchwork_21505_full:
> 
> 
>   IGT changes
> 
> 
> Possible regressions
> 
>   * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
>   o shard-iclb: PASS
> 
> 
> -> FAIL
> 
>  igt@kms_cursor_leg...@cursor-vs-flip-toggle.html>
> 


False positive. Failure looks unrelated.


> 
> Known issues
> 
> Here are the changes found in Patchwork_21505_full that come from 
> known
> issues:
> 
> 
>   CI changes
> 
> 
> Issues hit
> 
>   * boot:
>   o shard-glk: (PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> ,
> PASS
> 
> )
> -> (PASS
> 
> 

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: fixup dma_fence_wait usage

[Patchwork]

== Series Details ==

Series: drm/i915: fixup dma_fence_wait usage
URL   : https://patchwork.freedesktop.org/series/96504/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10828_full -> Patchwork_21505_full


Summary
---

  **SUCCESS**

  No regressions found.

  

Participating hosts (10 -> 10)
--

  No changes in participating hosts

Known issues


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

### CI changes ###

 Issues hit 

  * boot:
- shard-glk:  ([PASS][1], [PASS][2], [PASS][3], [PASS][4], 
[PASS][5], [PASS][6], [PASS][7], [PASS][8], [PASS][9], [PASS][10], [PASS][11], 
[PASS][12], [PASS][13], [PASS][14], [PASS][15], [PASS][16], [PASS][17], 
[PASS][18], [PASS][19], [PASS][20], [PASS][21], [PASS][22], [PASS][23], 
[PASS][24], [PASS][25]) -> ([PASS][26], [PASS][27], [PASS][28], [PASS][29], 
[PASS][30], [PASS][31], [PASS][32], [PASS][33], [PASS][34], [PASS][35], 
[PASS][36], [PASS][37], [PASS][38], [PASS][39], [PASS][40], [PASS][41], 
[PASS][42], [PASS][43], [PASS][44], [PASS][45], [PASS][46], [FAIL][47], 
[PASS][48], [PASS][49], [PASS][50]) ([i915#4392])
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk1/boot.html
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk1/boot.html
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk2/boot.html
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk2/boot.html
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk2/boot.html
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk3/boot.html
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk3/boot.html
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk4/boot.html
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk4/boot.html
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk4/boot.html
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk5/boot.html
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk5/boot.html
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk6/boot.html
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk6/boot.html
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk6/boot.html
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk7/boot.html
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk7/boot.html
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk7/boot.html
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk8/boot.html
   [20]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk8/boot.html
   [21]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk8/boot.html
   [22]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk9/boot.html
   [23]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk9/boot.html
   [24]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk9/boot.html
   [25]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10828/shard-glk9/boot.html
   [26]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk9/boot.html
   [27]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk9/boot.html
   [28]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk9/boot.html
   [29]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk8/boot.html
   [30]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk8/boot.html
   [31]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk8/boot.html
   [32]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk7/boot.html
   [33]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk7/boot.html
   [34]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk6/boot.html
   [35]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk6/boot.html
   [36]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk5/boot.html
   [37]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk5/boot.html
   [38]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk4/boot.html
   [39]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk4/boot.html
   [40]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk4/boot.html
   [41]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk3/boot.html
   [42]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk3/boot.html
   [43]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/shard-glk3/boot.html
   [44]: 

Re: [Intel-gfx] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic

[Javier Martinez Canillas]

Hello Thomas,

On 11/3/21 14:01, Thomas Zimmermann wrote:

[snip]

>>
>>
>> Javier Martinez Canillas (5):
>>drm/i915: Fix comment about modeset parameters
>>drm: Move nomodeset kernel parameter handler to the DRM subsystem
>>drm: Rename vgacon_text_force() function to drm_modeset_disabled()
>>drm: Add a drm_drv_enabled() helper function
>>drm: Use drm_drv_enabled() instead of drm_modeset_disabled()
> 
> There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And 
> I'd put patch (4+5) first, so you have the drivers out of the way. After 
> that patch (2+3) should only modify drm_drv_enabled().
>

Sure, I'm happy with less patches.

Thanks for your feedback.
 
> Best regards
> Thomas
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Intel-gfx] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()

[Javier Martinez Canillas]

On 11/3/21 13:57, Thomas Zimmermann wrote:

[snip]

>>   
>> -if (vgacon_text_force()) {
>> +if (drm_modeset_disabled()) {
>>  DRM_ERROR("amdgpu kernel modesetting disabled.\n");
> 
> Please remove all such error messages from drivers. 
> drm_modeset_disabled() should print a unified message instead.
>

Agreed.

>> -static bool vgacon_text_mode_force;
>> +static bool drm_nomodeset;
>>   
>> -bool vgacon_text_force(void)
>> +bool drm_modeset_disabled(void)
> 
> I suggest to rename this function to drm_check_modeset() and have it 
> return a negative errno code on failure. This gives maximum flexibility 
> and reduces errors in drivers. Right now the drivers return something 
> like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.
>

Good idea. I'll do it in v2 as well.
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

[Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915: Failsafe migration blits (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915: Failsafe migration blits (rev5)
URL   : https://patchwork.freedesktop.org/series/95617/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10832_full -> Patchwork_21511_full


Summary
---

  **SUCCESS**

  No regressions found.

  

Participating hosts (10 -> 11)
--

  Additional (1): shard-rkl 

Possible new issues
---

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

### IGT changes ###

 Suppressed 

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

  * igt@kms_flip_tiling@flip-change-tiling:
- {shard-rkl}:NOTRUN -> [SKIP][1] +48 similar issues
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-rkl-2/igt@kms_flip_til...@flip-change-tiling.html

  
Known issues


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

### IGT changes ###

 Issues hit 

  * igt@gem_create@create-massive:
- shard-tglb: NOTRUN -> [DMESG-WARN][2] ([i915#3002])
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-tglb2/igt@gem_cre...@create-massive.html
- shard-apl:  NOTRUN -> [DMESG-WARN][3] ([i915#3002])
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-apl2/igt@gem_cre...@create-massive.html

  * igt@gem_ctx_isolation@preservation-s3@bcs0:
- shard-tglb: [PASS][4] -> [INCOMPLETE][5] ([i915#456]) +2 similar 
issues
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10832/shard-tglb3/igt@gem_ctx_isolation@preservation...@bcs0.html
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-tglb7/igt@gem_ctx_isolation@preservation...@bcs0.html

  * igt@gem_exec_fair@basic-deadline:
- shard-apl:  NOTRUN -> [FAIL][6] ([i915#2846])
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-apl4/igt@gem_exec_f...@basic-deadline.html

  * igt@gem_exec_fair@basic-none-vip@rcs0:
- shard-iclb: [PASS][7] -> [FAIL][8] ([i915#2842])
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10832/shard-iclb7/igt@gem_exec_fair@basic-none-...@rcs0.html
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-iclb6/igt@gem_exec_fair@basic-none-...@rcs0.html

  * igt@gem_exec_fair@basic-pace@vcs1:
- shard-kbl:  [PASS][9] -> [FAIL][10] ([i915#2842])
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10832/shard-kbl3/igt@gem_exec_fair@basic-p...@vcs1.html
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-kbl3/igt@gem_exec_fair@basic-p...@vcs1.html

  * igt@gem_exec_params@secure-non-root:
- shard-tglb: NOTRUN -> [SKIP][11] ([fdo#112283])
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-tglb5/igt@gem_exec_par...@secure-non-root.html

  * igt@gem_huc_copy@huc-copy:
- shard-apl:  NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#2190])
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-apl4/igt@gem_huc_c...@huc-copy.html

  * igt@gem_pxp@create-regular-context-1:
- shard-tglb: NOTRUN -> [SKIP][13] ([i915#4270]) +1 similar issue
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-tglb5/igt@gem_...@create-regular-context-1.html

  * igt@gem_render_copy@x-tiled-to-vebox-yf-tiled:
- shard-kbl:  NOTRUN -> [SKIP][14] ([fdo#109271]) +112 similar 
issues
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-kbl3/igt@gem_render_c...@x-tiled-to-vebox-yf-tiled.html

  * igt@gem_softpin@evict-snoop:
- shard-tglb: NOTRUN -> [SKIP][15] ([fdo#109312])
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-tglb5/igt@gem_soft...@evict-snoop.html

  * igt@gem_userptr_blits@unsync-unmap-after-close:
- shard-tglb: NOTRUN -> [SKIP][16] ([i915#3297]) +1 similar issue
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-tglb5/igt@gem_userptr_bl...@unsync-unmap-after-close.html

  * igt@gem_workarounds@suspend-resume:
- shard-apl:  [PASS][17] -> [DMESG-WARN][18] ([i915#180]) +2 
similar issues
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10832/shard-apl6/igt@gem_workarou...@suspend-resume.html
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-apl4/igt@gem_workarou...@suspend-resume.html

  * igt@gen7_exec_parse@batch-without-end:
- shard-iclb: NOTRUN -> [SKIP][19] ([fdo#109289])
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/shard-iclb8/igt@gen7_exec_pa...@batch-without-end.html

  * igt@gen9_exec_parse@bb-large:
- shard-tglb: NOTRUN -> [SKIP][20] ([i915#2856])
   [20]: 

Re: [Intel-gfx] [PATCH i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[Tvrtko Ursulin]

On 22/10/2021 00:40, john.c.harri...@intel.com wrote:

From: John Harrison 

The sysfs file read helper does not actually report any errors if a
realloc fails. It just silently returns a 'valid' but truncated
buffer. This then leads to the decode of the buffer failing in random
ways. So, add a check for ENOMEM being generated during the read.

Signed-off-by: John Harrison 
---
  tests/i915/gem_exec_capture.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index e373d24ed..8997125ee 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -131,9 +131,11 @@ static int check_error_state(int dir, struct offset 
*obj_offsets, int obj_count,
char *error, *str;
int blobs = 0;
  
+	errno = 0;

error = igt_sysfs_get(dir, "error");
igt_sysfs_set(dir, "error", "Begone!");
igt_assert(error);
+   igt_assert(errno != ENOMEM);


igt_sysfs_get:

len = 64;
...
newbuf = realloc(buf, 2*len);

Maybe the problem is doubling goes out of hand. How big are your 
buffers? Perhaps you could improve the library function instead to grow 
less aggressively.


And at the same time perhaps the bug is this:

if (igt_debug_on(!newbuf))
break;
...
return buf;

So failures to grow the buffer are ignored, while failure to allocate 
the initial one are not. Perhaps both should return NULL and then 
callers would not be surprised.


Or you think someone relies on this current odd behaviour?

Regards,

Tvrtko


igt_debug("%s\n", error);
  
  	/* render ring --- user = 0x d000 */

Re: [Intel-gfx] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[Tvrtko Ursulin]

On 22/10/2021 00:40, john.c.harri...@intel.com wrote:

From: John Harrison 

The 'many' test ended with an 'assert(count)', presumably meaning to
ensure that some objects were actually captured. However, 'count' is
the number of objects created not how many were captured. Plus, there
is already a 'require(count > 1)' at the start and count is invarient
so the final assert is basically pointless.

General concensus appears to be that the test should not fail
irrespective of how many blobs are captured as low memory situations
could cause the capture to be abbreviated. So just remove the
pointless assert completely.


Hm the test appears to be using intel_get_avail_ram_mb() to size the 
working set. Suggesting problems with low memory situations should not 
apply unless bugs. In which case would a better fix be improving the 
sizing logic and changing the assert to igt_assert(blobs)?


Regards,

Tvrtko


Signed-off-by: John Harrison 
---
  tests/i915/gem_exec_capture.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 7e0a8b8ad..53649cdb2 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, unsigned 
int flags)
}
igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
 blobs, size >> 12, count);
-   igt_assert(count);
  
  	free(error);

free(offsets);

Re: [Intel-gfx] [RESEND PATCH 1/5] drm/i915: Fix comment about modeset parameters

[Jani Nikula]

On Wed, 03 Nov 2021, Javier Martinez Canillas  wrote:
> The comment mentions that the KMS is enabled by default unless either the
> i915.modeset module parameter or vga_text_mode_force boot option are used.
>
> But the latter does not exist and instead the nomodeset option was meant.
>
> Signed-off-by: Javier Martinez Canillas 

Thanks for the patch. I've picked this up to drm-intel-next as a
non-functional change independent from the rest of the series.

BR,
Jani.

> ---
>
>  drivers/gpu/drm/i915/i915_module.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_module.c 
> b/drivers/gpu/drm/i915/i915_module.c
> index ab2295dd4500..c7507266aa83 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -24,8 +24,8 @@ static int i915_check_nomodeset(void)
>  
>   /*
>* Enable KMS by default, unless explicitly overriden by
> -  * either the i915.modeset prarameter or by the
> -  * vga_text_mode_force boot option.
> +  * either the i915.modeset parameter or by the
> +  * nomodeset boot option.
>*/
>  
>   if (i915_modparams.modeset == 0)

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Javier Martinez Canillas]

Hello Jani,

On 11/3/21 13:56, Jani Nikula wrote:

[snip]

>>  
>> +obj-y += drm_nomodeset.o
> 
> This is a subtle functional change. With this, you'll always have
> __setup("nomodeset", text_mode) builtin and the parameter available. And
> using nomodeset will print out the pr_warn() splat from text_mode(). But
> removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
> leads to vgacon_text_force() always returning false.
>

Yes, that's what I decided at the end to make it unconditional. That
way the same behaviour is preserved (even when only DRM drivers are
using the exported symbol).
 
> To not make functional changes, this should be:
> 
> obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>

Right, that should work.

> Now, going with the cleanup in this series, maybe we should make the
> functional change, and break the connection to CONFIG_VGA_CONSOLE
> altogether, also in the header?
> 
> (Maybe we'll also need a proxy drm kconfig option to only have
> drm_modeset.o builtin when CONFIG_DRM != n.)
>

See my other email. I believe the issue is drivers/gpu/drm always
being included even when CONFIG_DRM is not set.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Javier Martinez Canillas]

Hello Thomas,

On 11/3/21 13:41, Thomas Zimmermann wrote:
> Hi
> 
> Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:
>> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
>> but the exported vgacon_text_force() symbol is only used by DRM drivers.
>>
>> It makes much more sense for the parameter logic to be in the subsystem
>> of the drivers that are making use of it. Let's move that to DRM.
>>
>> Suggested-by: Daniel Vetter 
>> Signed-off-by: Javier Martinez Canillas 
>> ---
>>
>>   drivers/gpu/drm/Makefile|  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
>>   drivers/gpu/drm/ast/ast_drv.c   |  1 -
>>   drivers/gpu/drm/drm_nomodeset.c | 26 +
>>   drivers/gpu/drm/i915/i915_module.c  |  2 --
>>   drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
>>   drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
>>   drivers/gpu/drm/qxl/qxl_drv.c   |  1 -
>>   drivers/gpu/drm/radeon/radeon_drv.c |  1 -
>>   drivers/gpu/drm/tiny/bochs.c|  1 -
>>   drivers/gpu/drm/tiny/cirrus.c   |  1 -
>>   drivers/gpu/drm/vboxvideo/vbox_drv.c|  1 -
>>   drivers/gpu/drm/virtio/virtgpu_drv.c|  1 -
>>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  1 -
>>   drivers/video/console/vgacon.c  | 21 
>>   include/drm/drm_mode_config.h   |  6 ++
>>   include/linux/console.h |  6 --
>>   17 files changed, 35 insertions(+), 41 deletions(-)
>>   create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 1c41156deb5f..0e2d60ea93ca 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o 
>> drm_privacy_screen_x86.
>>   
>>   obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>>   
>> +obj-y += drm_nomodeset.o
> 
> Repeating my other comment, should this rather be protected by a 
> separate config symbol that is selected by CONFIG_DRM?
>

I actually thought about that and my opinion is that obj-y reflects
what we really want here or do you envision this getting disabled
in some cases ?

Probably the problem is Kbuild descending into the drivers/gpu dir
even when CONFIG_DRM is not set. Maybe what we want is something
like the following instead?

diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index 835c88318cec..ef12ee05ba6e 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -3,6 +3,7 @@
 # taken to initialize them in the correct order. Link order is the only way
 # to ensure this currently.
 obj-$(CONFIG_TEGRA_HOST1X) += host1x/
-obj-y  += drm/ vga/
+obj-$(CONFIG_DRM)  += drm/
+obj-y  += vga/
 obj-$(CONFIG_IMX_IPUV3_CORE)   += ipu-v3/
 obj-$(CONFIG_TRACE_GPU_MEM)+= trace/

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Intel-gfx] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic

[Thomas Zimmermann]

Hi

Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:

[ resend with all relevant people as Cc now, sorry to others for the spam ]

There is a lot of historical baggage on this parameter. It's defined in
the vgacon driver as a "nomodeset" parameter, but it's handler function is
called text_mode() that sets a variable named vgacon_text_mode_force whose
value is queried with a function named vgacon_text_force().

All this implies that it's about forcing text mode for VGA, yet it is not
used in neither vgacon nor other console driver. The only users for these
are DRM drivers, that check for the vgacon_text_force() return value to
determine whether the driver could be loaded or not.

That makes it quite confusing to read the code, because the variables and
function names don't reflect what they actually do and also are not in the
same subsystem as the drivers that make use of them.

This patch-set attempts to cleanup the code by moving the nomodseset param
to the DRM subsystem and do some renaming to make their intention clearer.

There is also another aspect that could be improved, and is the fact that
drivers are checking for the nomodeset being set as an indication if have
to be loaded.

But there may be other reasons why this could be the case, so it is better
to encapsulate the logic in a separate function to make clear what's about.

Patch #1 is just a trivial fix for a comment that isn't referring to the
correct kernel parameter.

Patch #2 moves the nomodeset logic to the DRM subsystem.

Patch #3 renames the vgacon_text_force() function and accompaning logic as
drm_modeset_disabled(), which is what this function is really about.

Patch #4 adds a drm_drv_enabled() function that could be used by drivers
to check if could be enabled.

Patch #5 uses the drm_drv_enabled() function to check this instead of just
checking if nomodeset has been set.


Javier Martinez Canillas (5):
   drm/i915: Fix comment about modeset parameters
   drm: Move nomodeset kernel parameter handler to the DRM subsystem
   drm: Rename vgacon_text_force() function to drm_modeset_disabled()
   drm: Add a drm_drv_enabled() helper function
   drm: Use drm_drv_enabled() instead of drm_modeset_disabled()


There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And 
I'd put patch (4+5) first, so you have the drivers out of the way. After 
that patch (2+3) should only modify drm_drv_enabled().


Best regards
Thomas



  drivers/gpu/drm/Makefile|  2 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 ++---
  drivers/gpu/drm/ast/ast_drv.c   |  3 +--
  drivers/gpu/drm/drm_drv.c   | 21 
  drivers/gpu/drm/drm_nomodeset.c | 26 +
  drivers/gpu/drm/i915/i915_module.c  | 10 +-
  drivers/gpu/drm/mgag200/mgag200_drv.c   |  3 +--
  drivers/gpu/drm/nouveau/nouveau_drm.c   |  3 +--
  drivers/gpu/drm/qxl/qxl_drv.c   |  3 +--
  drivers/gpu/drm/radeon/radeon_drv.c |  3 +--
  drivers/gpu/drm/tiny/bochs.c|  3 +--
  drivers/gpu/drm/tiny/cirrus.c   |  3 +--
  drivers/gpu/drm/vboxvideo/vbox_drv.c|  5 +
  drivers/gpu/drm/virtio/virtgpu_drv.c|  3 +--
  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  3 +--
  drivers/video/console/vgacon.c  | 21 
  include/drm/drm_drv.h   |  1 +
  include/drm/drm_mode_config.h   |  6 ++
  include/linux/console.h |  6 --
  19 files changed, 73 insertions(+), 57 deletions(-)
  create mode 100644 drivers/gpu/drm/drm_nomodeset.c



--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev


OpenPGP_signature
Description: OpenPGP digital signature

Re: [Intel-gfx] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()

[Thomas Zimmermann]

Hi

Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:

This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.

But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.

Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.

Suggested-by: Suggested-by: Daniel Vetter 
Signed-off-by: Javier Martinez Canillas 
---

  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
  drivers/gpu/drm/ast/ast_drv.c   |  2 +-
  drivers/gpu/drm/drm_nomodeset.c | 16 
  drivers/gpu/drm/i915/i915_module.c  |  2 +-
  drivers/gpu/drm/mgag200/mgag200_drv.c   |  2 +-
  drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
  drivers/gpu/drm/qxl/qxl_drv.c   |  2 +-
  drivers/gpu/drm/radeon/radeon_drv.c |  2 +-
  drivers/gpu/drm/tiny/bochs.c|  2 +-
  drivers/gpu/drm/tiny/cirrus.c   |  2 +-
  drivers/gpu/drm/vboxvideo/vbox_drv.c|  4 +---
  drivers/gpu/drm/virtio/virtgpu_drv.c|  2 +-
  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  2 +-
  include/drm/drm_mode_config.h   |  4 ++--
  14 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
  {
int r;
  
-	if (vgacon_text_force()) {

+   if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");


Please remove all such error messages from drivers. 
drm_modeset_disabled() should print a unified message instead.




return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
  
  static int __init ast_init(void)

  {
-   if (vgacon_text_force() && ast_modeset == -1)
+   if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
  
  	if (ast_modeset == 0)

diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
  #include 
  #include 
  
-static bool vgacon_text_mode_force;

+static bool drm_nomodeset;
  
-bool vgacon_text_force(void)

+bool drm_modeset_disabled(void)


I suggest to rename this function to drm_check_modeset() and have it 
return a negative errno code on failure. This gives maximum flexibility 
and reduces errors in drivers. Right now the drivers return something 
like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.



  {
-   return vgacon_text_mode_force;
+   return drm_nomodeset;
  }
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
  
-static int __init text_mode(char *str)

+static int __init disable_modeset(char *str)
  {
-   vgacon_text_mode_force = true;
+   drm_nomodeset = true;
  
  	pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");

pr_warn("Any video related functionality will be severely degraded, and you 
may not even be able to suspend the system properly\n");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
  }
  
-/* force text mode - used by kernel modesetting */

-__setup("nomodeset", text_mode);
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c 
b/drivers/gpu/drm/i915/i915_module.c
index 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
  
-	if (vgacon_text_force() && i915_modparams.modeset == -1)

+   if (drm_modeset_disabled() && i915_modparams.modeset == -1)
use_kms = false;
  
  	if (!use_kms) {

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c 
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
  
  static int __init mgag200_init(void)

  {
-   if (vgacon_text_force() && mgag200_modeset == -1)
+   if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
  
  	if (mgag200_modeset == 0)

diff --git 

Re: [Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Jani Nikula]

On Wed, 03 Nov 2021, Javier Martinez Canillas  wrote:
> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
> but the exported vgacon_text_force() symbol is only used by DRM drivers.
>
> It makes much more sense for the parameter logic to be in the subsystem
> of the drivers that are making use of it. Let's move that to DRM.
>
> Suggested-by: Daniel Vetter 
> Signed-off-by: Javier Martinez Canillas 
> ---
>
>  drivers/gpu/drm/Makefile|  2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
>  drivers/gpu/drm/ast/ast_drv.c   |  1 -
>  drivers/gpu/drm/drm_nomodeset.c | 26 +
>  drivers/gpu/drm/i915/i915_module.c  |  2 --
>  drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
>  drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
>  drivers/gpu/drm/qxl/qxl_drv.c   |  1 -
>  drivers/gpu/drm/radeon/radeon_drv.c |  1 -
>  drivers/gpu/drm/tiny/bochs.c|  1 -
>  drivers/gpu/drm/tiny/cirrus.c   |  1 -
>  drivers/gpu/drm/vboxvideo/vbox_drv.c|  1 -
>  drivers/gpu/drm/virtio/virtgpu_drv.c|  1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  1 -
>  drivers/video/console/vgacon.c  | 21 
>  include/drm/drm_mode_config.h   |  6 ++
>  include/linux/console.h |  6 --
>  17 files changed, 35 insertions(+), 41 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c41156deb5f..0e2d60ea93ca 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o 
> drm_privacy_screen_x86.
>  
>  obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>  
> +obj-y += drm_nomodeset.o

This is a subtle functional change. With this, you'll always have
__setup("nomodeset", text_mode) builtin and the parameter available. And
using nomodeset will print out the pr_warn() splat from text_mode(). But
removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
leads to vgacon_text_force() always returning false.

To not make functional changes, this should be:

obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o

Now, going with the cleanup in this series, maybe we should make the
functional change, and break the connection to CONFIG_VGA_CONSOLE
altogether, also in the header?

(Maybe we'll also need a proxy drm kconfig option to only have
drm_modeset.o builtin when CONFIG_DRM != n.)

> +
>  drm_cma_helper-y := drm_gem_cma_helper.o
>  obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index c718fb5f3f8a..2680a2aaa877 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -31,7 +31,6 @@
>  #include "amdgpu_drv.h"
>  
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> @@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
>   int r;
>  
>   if (vgacon_text_force()) {
> - DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> + DRM_ERROR("amdgpu kernel modesetting disabled.\n");
>   return -EINVAL;
>   }
>  
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 86d5cd7b6318..048be607b182 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -26,7 +26,6 @@
>   * Authors: Dave Airlie 
>   */
>  
> -#include 
>  #include 
>  #include 
>  
> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
> new file mode 100644
> index ..1ac9a8d5a8fe
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_nomodeset.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include 
> +#include 
> +
> +static bool vgacon_text_mode_force;
> +
> +bool vgacon_text_force(void)
> +{
> + return vgacon_text_mode_force;
> +}
> +EXPORT_SYMBOL(vgacon_text_force);
> +
> +static int __init text_mode(char *str)
> +{
> + vgacon_text_mode_force = true;
> +
> + pr_warn("You have booted with nomodeset. This means your GPU drivers 
> are DISABLED\n");
> + pr_warn("Any video related functionality will be severely degraded, and 
> you may not even be able to suspend the system properly\n");
> + pr_warn("Unless you actually understand what nomodeset does, you should 
> reboot without enabling it\n");
> +
> + return 1;
> +}
> +
> +/* force text mode - used by kernel modesetting */
> +__setup("nomodeset", text_mode);
> diff --git a/drivers/gpu/drm/i915/i915_module.c 
> b/drivers/gpu/drm/i915/i915_module.c
> index c7507266aa83..14a59226519d 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -4,8 +4,6 @@
>   * Copyright © 2021 Intel Corporation
>   */
>  
> -#include 
> -
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_object.h"
>  #include 

Re: [Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Thomas Zimmermann]

Hi

Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:

The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.

It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it. Let's move that to DRM.

Suggested-by: Daniel Vetter 
Signed-off-by: Javier Martinez Canillas 
---

  drivers/gpu/drm/Makefile|  2 ++
  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
  drivers/gpu/drm/ast/ast_drv.c   |  1 -
  drivers/gpu/drm/drm_nomodeset.c | 26 +
  drivers/gpu/drm/i915/i915_module.c  |  2 --
  drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
  drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
  drivers/gpu/drm/qxl/qxl_drv.c   |  1 -
  drivers/gpu/drm/radeon/radeon_drv.c |  1 -
  drivers/gpu/drm/tiny/bochs.c|  1 -
  drivers/gpu/drm/tiny/cirrus.c   |  1 -
  drivers/gpu/drm/vboxvideo/vbox_drv.c|  1 -
  drivers/gpu/drm/virtio/virtgpu_drv.c|  1 -
  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  1 -
  drivers/video/console/vgacon.c  | 21 
  include/drm/drm_mode_config.h   |  6 ++
  include/linux/console.h |  6 --
  17 files changed, 35 insertions(+), 41 deletions(-)
  create mode 100644 drivers/gpu/drm/drm_nomodeset.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..0e2d60ea93ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o 
drm_privacy_screen_x86.
  
  obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
  
+obj-y += drm_nomodeset.o


Repeating my other comment, should this rather be protected by a 
separate config symbol that is selected by CONFIG_DRM?


Best regards
Thomas


+
  drm_cma_helper-y := drm_gem_cma_helper.o
  obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
  
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

index c718fb5f3f8a..2680a2aaa877 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
  #include "amdgpu_drv.h"
  
  #include 

-#include 
  #include 
  #include 
  #include 
@@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
int r;
  
  	if (vgacon_text_force()) {

-   DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+   DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
  
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c

index 86d5cd7b6318..048be607b182 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
   * Authors: Dave Airlie 
   */
  
-#include 

  #include 
  #include 
  
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c

new file mode 100644
index ..1ac9a8d5a8fe
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+
+static bool vgacon_text_mode_force;
+
+bool vgacon_text_force(void)
+{
+   return vgacon_text_mode_force;
+}
+EXPORT_SYMBOL(vgacon_text_force);
+
+static int __init text_mode(char *str)
+{
+   vgacon_text_mode_force = true;
+
+   pr_warn("You have booted with nomodeset. This means your GPU drivers are 
DISABLED\n");
+   pr_warn("Any video related functionality will be severely degraded, and you 
may not even be able to suspend the system properly\n");
+   pr_warn("Unless you actually understand what nomodeset does, you should 
reboot without enabling it\n");
+
+   return 1;
+}
+
+/* force text mode - used by kernel modesetting */
+__setup("nomodeset", text_mode);
diff --git a/drivers/gpu/drm/i915/i915_module.c 
b/drivers/gpu/drm/i915/i915_module.c
index c7507266aa83..14a59226519d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
   * Copyright © 2021 Intel Corporation
   */
  
-#include 

-
  #include "gem/i915_gem_context.h"
  #include "gem/i915_gem_object.h"
  #include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c 
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..685e766db6a4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -6,7 +6,6 @@
   *  Dave Airlie
   */
  
-#include 

  #include 
  #include 
  #include 
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..029997f50d1a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,7 +22,6 @@
   * Authors: Ben Skeggs
   */
  
-#include 

  #include 
  #include 
  #include 
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Failsafe migration blits (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915: Failsafe migration blits (rev5)
URL   : https://patchwork.freedesktop.org/series/95617/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_10832 -> Patchwork_21511


Summary
---

  **SUCCESS**

  No regressions found.

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

Participating hosts (36 -> 36)
--

  Additional (3): fi-icl-u2 fi-elk-e7500 fi-tgl-u2 
  Missing(3): fi-bsw-cyan bat-dg1-6 bat-adlp-4 

Known issues


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

### IGT changes ###

 Issues hit 

  * igt@amdgpu/amd_basic@cs-compute:
- fi-elk-e7500:   NOTRUN -> [SKIP][1] ([fdo#109271]) +49 similar issues
   [1]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-elk-e7500/igt@amdgpu/amd_ba...@cs-compute.html

  * igt@amdgpu/amd_basic@semaphore:
- fi-bdw-5557u:   NOTRUN -> [SKIP][2] ([fdo#109271]) +27 similar issues
   [2]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-bdw-5557u/igt@amdgpu/amd_ba...@semaphore.html

  * igt@amdgpu/amd_cs_nop@fork-gfx0:
- fi-icl-u2:  NOTRUN -> [SKIP][3] ([fdo#109315]) +17 similar issues
   [3]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-icl-u2/igt@amdgpu/amd_cs_...@fork-gfx0.html

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
- fi-snb-2600:NOTRUN -> [SKIP][4] ([fdo#109271]) +17 similar issues
   [4]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-snb-2600/igt@amdgpu/amd_cs_...@sync-fork-compute0.html

  * igt@gem_huc_copy@huc-copy:
- fi-tgl-u2:  NOTRUN -> [SKIP][5] ([i915#2190])
   [5]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-tgl-u2/igt@gem_huc_c...@huc-copy.html
- fi-icl-u2:  NOTRUN -> [SKIP][6] ([i915#2190])
   [6]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-icl-u2/igt@gem_huc_c...@huc-copy.html

  * igt@kms_chamelium@dp-crc-fast:
- fi-bdw-5557u:   NOTRUN -> [SKIP][7] ([fdo#109271] / [fdo#111827]) +8 
similar issues
   [7]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-bdw-5557u/igt@kms_chamel...@dp-crc-fast.html

  * igt@kms_chamelium@dp-hpd-fast:
- fi-tgl-u2:  NOTRUN -> [SKIP][8] ([fdo#109284] / [fdo#111827]) +8 
similar issues
   [8]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-tgl-u2/igt@kms_chamel...@dp-hpd-fast.html

  * igt@kms_chamelium@hdmi-hpd-fast:
- fi-icl-u2:  NOTRUN -> [SKIP][9] ([fdo#111827]) +8 similar issues
   [9]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-icl-u2/igt@kms_chamel...@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-tgl-u2:  NOTRUN -> [SKIP][10] ([i915#4103]) +1 similar issue
   [10]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-tgl-u2/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
- fi-icl-u2:  NOTRUN -> [SKIP][11] ([fdo#109278]) +2 similar issues
   [11]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-icl-u2/igt@kms_cursor_leg...@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
- fi-tgl-u2:  NOTRUN -> [SKIP][12] ([fdo#109285])
   [12]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-tgl-u2/igt@kms_force_connector_ba...@force-load-detect.html
- fi-icl-u2:  NOTRUN -> [SKIP][13] ([fdo#109285])
   [13]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-icl-u2/igt@kms_force_connector_ba...@force-load-detect.html

  * igt@prime_vgem@basic-userptr:
- fi-icl-u2:  NOTRUN -> [SKIP][14] ([i915#3301])
   [14]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-icl-u2/igt@prime_v...@basic-userptr.html
- fi-tgl-u2:  NOTRUN -> [SKIP][15] ([i915#3301])
   [15]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-tgl-u2/igt@prime_v...@basic-userptr.html

  
 Possible fixes 

  * igt@debugfs_test@read_all_entries:
- fi-kbl-soraka:  [DMESG-WARN][16] ([i915#1982] / [i915#262]) -> 
[PASS][17]
   [16]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10832/fi-kbl-soraka/igt@debugfs_test@read_all_entries.html
   [17]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-kbl-soraka/igt@debugfs_test@read_all_entries.html

  * igt@gem_exec_suspend@basic-s3:
- fi-bdw-5557u:   [INCOMPLETE][18] ([i915#146]) -> [PASS][19]
   [18]: 
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10832/fi-bdw-5557u/igt@gem_exec_susp...@basic-s3.html
   [19]: 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21511/fi-bdw-5557u/igt@gem_exec_susp...@basic-s3.html

  * igt@i915_selftest@live@hangcheck:
- fi-snb-2600:[INCOMPLETE][20] ([i915#3921]) -> [PASS][21]
   [20]: 

[Intel-gfx] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Javier Martinez Canillas]

The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
but the exported vgacon_text_force() symbol is only used by DRM drivers.

It makes much more sense for the parameter logic to be in the subsystem
of the drivers that are making use of it. Let's move that to DRM.

Suggested-by: Daniel Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/Makefile|  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
 drivers/gpu/drm/ast/ast_drv.c   |  1 -
 drivers/gpu/drm/drm_nomodeset.c | 26 +
 drivers/gpu/drm/i915/i915_module.c  |  2 --
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
 drivers/gpu/drm/qxl/qxl_drv.c   |  1 -
 drivers/gpu/drm/radeon/radeon_drv.c |  1 -
 drivers/gpu/drm/tiny/bochs.c|  1 -
 drivers/gpu/drm/tiny/cirrus.c   |  1 -
 drivers/gpu/drm/vboxvideo/vbox_drv.c|  1 -
 drivers/gpu/drm/virtio/virtgpu_drv.c|  1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  1 -
 drivers/video/console/vgacon.c  | 21 
 include/drm/drm_mode_config.h   |  6 ++
 include/linux/console.h |  6 --
 17 files changed, 35 insertions(+), 41 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_nomodeset.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..0e2d60ea93ca 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o 
drm_privacy_screen_x86.
 
 obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
 
+obj-y += drm_nomodeset.o
+
 drm_cma_helper-y := drm_gem_cma_helper.o
 obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..2680a2aaa877 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -31,7 +31,6 @@
 #include "amdgpu_drv.h"
 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
int r;
 
if (vgacon_text_force()) {
-   DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+   DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
 
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..048be607b182 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
  * Authors: Dave Airlie 
  */
 
-#include 
 #include 
 #include 
 
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..1ac9a8d5a8fe
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+
+static bool vgacon_text_mode_force;
+
+bool vgacon_text_force(void)
+{
+   return vgacon_text_mode_force;
+}
+EXPORT_SYMBOL(vgacon_text_force);
+
+static int __init text_mode(char *str)
+{
+   vgacon_text_mode_force = true;
+
+   pr_warn("You have booted with nomodeset. This means your GPU drivers 
are DISABLED\n");
+   pr_warn("Any video related functionality will be severely degraded, and 
you may not even be able to suspend the system properly\n");
+   pr_warn("Unless you actually understand what nomodeset does, you should 
reboot without enabling it\n");
+
+   return 1;
+}
+
+/* force text mode - used by kernel modesetting */
+__setup("nomodeset", text_mode);
diff --git a/drivers/gpu/drm/i915/i915_module.c 
b/drivers/gpu/drm/i915/i915_module.c
index c7507266aa83..14a59226519d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -4,8 +4,6 @@
  * Copyright © 2021 Intel Corporation
  */
 
-#include 
-
 #include "gem/i915_gem_context.h"
 #include "gem/i915_gem_object.h"
 #include "i915_active.h"
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c 
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..685e766db6a4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -6,7 +6,6 @@
  *  Dave Airlie
  */
 
-#include 
 #include 
 #include 
 #include 
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..029997f50d1a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -22,7 +22,6 @@
  * Authors: Ben Skeggs
  */
 
-#include 
 #include 
 #include 
 #include 
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index fc47b0deb021..3cd6bd9f059d 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -29,7 +29,6 @@
 
 #include "qxl_drv.h"
 
-#include 
 #include 
 #include 
 #include 
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c 

[Intel-gfx] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()

[Javier Martinez Canillas]

This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.

But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.

Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.

Suggested-by: Suggested-by: Daniel Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 drivers/gpu/drm/ast/ast_drv.c   |  2 +-
 drivers/gpu/drm/drm_nomodeset.c | 16 
 drivers/gpu/drm/i915/i915_module.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
 drivers/gpu/drm/qxl/qxl_drv.c   |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.c |  2 +-
 drivers/gpu/drm/tiny/bochs.c|  2 +-
 drivers/gpu/drm/tiny/cirrus.c   |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c|  4 +---
 drivers/gpu/drm/virtio/virtgpu_drv.c|  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  2 +-
 include/drm/drm_mode_config.h   |  4 ++--
 14 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
 {
int r;
 
-   if (vgacon_text_force()) {
+   if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
 
 static int __init ast_init(void)
 {
-   if (vgacon_text_force() && ast_modeset == -1)
+   if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
 
if (ast_modeset == 0)
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
 #include 
 #include 
 
-static bool vgacon_text_mode_force;
+static bool drm_nomodeset;
 
-bool vgacon_text_force(void)
+bool drm_modeset_disabled(void)
 {
-   return vgacon_text_mode_force;
+   return drm_nomodeset;
 }
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
 
-static int __init text_mode(char *str)
+static int __init disable_modeset(char *str)
 {
-   vgacon_text_mode_force = true;
+   drm_nomodeset = true;
 
pr_warn("You have booted with nomodeset. This means your GPU drivers 
are DISABLED\n");
pr_warn("Any video related functionality will be severely degraded, and 
you may not even be able to suspend the system properly\n");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
 }
 
-/* force text mode - used by kernel modesetting */
-__setup("nomodeset", text_mode);
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git a/drivers/gpu/drm/i915/i915_module.c 
b/drivers/gpu/drm/i915/i915_module.c
index 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
 
-   if (vgacon_text_force() && i915_modparams.modeset == -1)
+   if (drm_modeset_disabled() && i915_modparams.modeset == -1)
use_kms = false;
 
if (!use_kms) {
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c 
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
 
 static int __init mgag200_init(void)
 {
-   if (vgacon_text_force() && mgag200_modeset == -1)
+   if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
 
if (mgag200_modeset == 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 029997f50d1a..903d0e626954 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1321,7 +1321,7 @@ nouveau_drm_init(void)
nouveau_display_options();
 
if (nouveau_modeset == -1) {
-   if (vgacon_text_force())
+   if (drm_modeset_disabled())
nouveau_modeset = 0;
}
 
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c 

[Intel-gfx] [RESEND PATCH 1/5] drm/i915: Fix comment about modeset parameters

[Javier Martinez Canillas]

The comment mentions that the KMS is enabled by default unless either the
i915.modeset module parameter or vga_text_mode_force boot option are used.

But the latter does not exist and instead the nomodeset option was meant.

Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/i915/i915_module.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_module.c 
b/drivers/gpu/drm/i915/i915_module.c
index ab2295dd4500..c7507266aa83 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -24,8 +24,8 @@ static int i915_check_nomodeset(void)
 
/*
 * Enable KMS by default, unless explicitly overriden by
-* either the i915.modeset prarameter or by the
-* vga_text_mode_force boot option.
+* either the i915.modeset parameter or by the
+* nomodeset boot option.
 */
 
if (i915_modparams.modeset == 0)
-- 
2.33.1

[Intel-gfx] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic

[Javier Martinez Canillas]

[ resend with all relevant people as Cc now, sorry to others for the spam ]

There is a lot of historical baggage on this parameter. It's defined in
the vgacon driver as a "nomodeset" parameter, but it's handler function is
called text_mode() that sets a variable named vgacon_text_mode_force whose
value is queried with a function named vgacon_text_force().

All this implies that it's about forcing text mode for VGA, yet it is not
used in neither vgacon nor other console driver. The only users for these
are DRM drivers, that check for the vgacon_text_force() return value to
determine whether the driver could be loaded or not.

That makes it quite confusing to read the code, because the variables and
function names don't reflect what they actually do and also are not in the
same subsystem as the drivers that make use of them.

This patch-set attempts to cleanup the code by moving the nomodseset param
to the DRM subsystem and do some renaming to make their intention clearer.

There is also another aspect that could be improved, and is the fact that
drivers are checking for the nomodeset being set as an indication if have
to be loaded.

But there may be other reasons why this could be the case, so it is better
to encapsulate the logic in a separate function to make clear what's about.

Patch #1 is just a trivial fix for a comment that isn't referring to the
correct kernel parameter.

Patch #2 moves the nomodeset logic to the DRM subsystem.

Patch #3 renames the vgacon_text_force() function and accompaning logic as
drm_modeset_disabled(), which is what this function is really about.

Patch #4 adds a drm_drv_enabled() function that could be used by drivers
to check if could be enabled.

Patch #5 uses the drm_drv_enabled() function to check this instead of just
checking if nomodeset has been set.


Javier Martinez Canillas (5):
  drm/i915: Fix comment about modeset parameters
  drm: Move nomodeset kernel parameter handler to the DRM subsystem
  drm: Rename vgacon_text_force() function to drm_modeset_disabled()
  drm: Add a drm_drv_enabled() helper function
  drm: Use drm_drv_enabled() instead of drm_modeset_disabled()

 drivers/gpu/drm/Makefile|  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  5 ++---
 drivers/gpu/drm/ast/ast_drv.c   |  3 +--
 drivers/gpu/drm/drm_drv.c   | 21 
 drivers/gpu/drm/drm_nomodeset.c | 26 +
 drivers/gpu/drm/i915/i915_module.c  | 10 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  3 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  3 +--
 drivers/gpu/drm/qxl/qxl_drv.c   |  3 +--
 drivers/gpu/drm/radeon/radeon_drv.c |  3 +--
 drivers/gpu/drm/tiny/bochs.c|  3 +--
 drivers/gpu/drm/tiny/cirrus.c   |  3 +--
 drivers/gpu/drm/vboxvideo/vbox_drv.c|  5 +
 drivers/gpu/drm/virtio/virtgpu_drv.c|  3 +--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  3 +--
 drivers/video/console/vgacon.c  | 21 
 include/drm/drm_drv.h   |  1 +
 include/drm/drm_mode_config.h   |  6 ++
 include/linux/console.h |  6 --
 19 files changed, 73 insertions(+), 57 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_nomodeset.c

-- 
2.33.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Failsafe migration blits (rev5)

[Patchwork]

== Series Details ==

Series: drm/i915: Failsafe migration blits (rev5)
URL   : https://patchwork.freedesktop.org/series/95617/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
b899c699aca8 drm/i915/ttm: Reorganize the ttm move code
-:511: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does 
MAINTAINERS need updating?
#511: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 807 lines checked
fec65a76f733 drm/i915/ttm: Failsafe migration blits

Re: [Intel-gfx] [PATCH 2/2] drm/i915/dp: For PCON TMDS mode set only the relavant bits in config DPCD

[Shankar, Uma]

> -Original Message-
> From: Nautiyal, Ankit K 
> Sent: Friday, October 29, 2021 11:32 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Sharma, Swati2 ; Shankar, Uma
> 
> Subject: [PATCH 2/2] drm/i915/dp: For PCON TMDS mode set only the relavant 
> bits
> in config DPCD
> 
> Currently we reset the whole PCON linkConfig DPCD to set the TMDS mode.
> This also resets the Source control bit and HDMI link enable bit and goes to
> autonomous mode of operation, which is seen to spoil the PCONs internal state.
> 
> This patch avoids resetting the PCON link config register and sets only TMDS 
> mode
> bit, Source control bit in the configuration DPCD.
> It then enables the HDMI Link Enable bit.
> 
> Signed-off-by: Ankit Nautiyal 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 25 -
>  1 file changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index f5fd106e555c..3df35079580a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2287,6 +2287,29 @@ static bool intel_dp_is_hdmi_2_1_sink(struct intel_dp
> *intel_dp)
>   return false;
>  }
> 
> +static
> +int intel_dp_pcon_set_tmds_mode(struct intel_dp *intel_dp) {
> + int ret;
> + u8 buf = 0;
> +
> + /* Set PCON source control mode and TMDS */
> + buf |= DP_PCON_ENABLE_SOURCE_CTL_MODE;
> + buf &= ~DP_PCON_ENABLE_MAX_FRL_BW;
> +

I don't see setting this to TMDS, we should be still in FRL mode as bit5 is not 
being reset here.
Am I missing something ?

Regards,
Uma Shankar
> + ret = drm_dp_dpcd_writeb(_dp->aux,
> DP_PCON_HDMI_LINK_CONFIG_1, buf);
> + if (ret < 0)
> + return ret;
> +
> + /* Set HDMI LINK ENABLE */
> + buf |= DP_PCON_ENABLE_HDMI_LINK;
> + ret = drm_dp_dpcd_writeb(_dp->aux,
> DP_PCON_HDMI_LINK_CONFIG_1, buf);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
>  void intel_dp_check_frl_training(struct intel_dp *intel_dp)  {
>   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); @@ -2305,7
> +2328,7 @@ void intel_dp_check_frl_training(struct intel_dp *intel_dp)
>   int ret, mode;
> 
>   drm_dbg(_priv->drm, "Couldn't set FRL mode, continuing with
> TMDS mode\n");
> - ret = drm_dp_pcon_reset_frl_config(_dp->aux);
> + ret = intel_dp_pcon_set_tmds_mode(intel_dp);
>   mode = drm_dp_pcon_hdmi_link_mode(_dp->aux, NULL);
> 
>   if (ret < 0 || mode != DP_PCON_HDMI_MODE_TMDS)
> --
> 2.25.1

[Intel-gfx] [PATCH v4 2/2] drm/i915/ttm: Failsafe migration blits

[Thomas Hellström]

If the initial fill blit or copy blit of an object fails, the old
content of the data might be exposed and read as soon as either CPU- or
GPU PTEs are set up to point at the pages.

Intercept the blit fence with an async callback that checks the
blit fence for errors and if there are errors performs an async cpu blit
instead. If there is a failure to allocate the async dma_fence_work,
allocate it on the stack and sync wait for the blit to complete.

Add selftests that simulate gpu blit failures and failure to allocate
the async dma_fence_work.

A previous version of this pach used dma_fence_work, now that's
opencoded which adds more code but might lower the latency
somewhat in the common non-error case.

v3:
- Style fixes (Matthew Auld)
v4:
- Use "#if IS_ENABLED()" instead of #ifdef (Matthew Auld)

Signed-off-by: Thomas Hellström 
Reviewed-by: Matthew Auld 
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c  | 322 +++---
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h  |   5 +
 .../drm/i915/gem/selftests/i915_gem_migrate.c |  24 +-
 3 files changed, 295 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c 
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index 0ed6b7f2b95f..1747e9ff97e7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -18,6 +18,29 @@
 #include "gt/intel_gt.h"
 #include "gt/intel_migrate.h"
 
+/**
+ * DOC: Selftest failure modes for failsafe migration:
+ *
+ * For fail_gpu_migration, the gpu blit scheduled is always a clear blit
+ * rather than a copy blit, and then we force the failure paths as if
+ * the blit fence returned an error.
+ *
+ * For fail_work_allocation we fail the kmalloc of the async worker, we
+ * sync the gpu blit. If it then fails, or fail_gpu_migration is set to
+ * true, then a memcpy operation is performed sync.
+ */
+#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
+static bool fail_gpu_migration;
+static bool fail_work_allocation;
+
+void i915_ttm_migrate_set_failure_modes(bool gpu_migration,
+   bool work_allocation)
+{
+   fail_gpu_migration = gpu_migration;
+   fail_work_allocation = work_allocation;
+}
+#endif
+
 static enum i915_cache_level
 i915_ttm_cache_level(struct drm_i915_private *i915, struct ttm_resource *res,
 struct ttm_tt *ttm)
@@ -129,11 +152,11 @@ int i915_ttm_move_notify(struct ttm_buffer_object *bo)
return 0;
 }
 
-static int i915_ttm_accel_move(struct ttm_buffer_object *bo,
-  bool clear,
-  struct ttm_resource *dst_mem,
-  struct ttm_tt *dst_ttm,
-  struct sg_table *dst_st)
+static struct dma_fence *i915_ttm_accel_move(struct ttm_buffer_object *bo,
+bool clear,
+struct ttm_resource *dst_mem,
+struct ttm_tt *dst_ttm,
+struct sg_table *dst_st)
 {
struct drm_i915_private *i915 = container_of(bo->bdev, typeof(*i915),
 bdev);
@@ -144,30 +167,29 @@ static int i915_ttm_accel_move(struct ttm_buffer_object 
*bo,
int ret;
 
if (!i915->gt.migrate.context || intel_gt_is_wedged(>gt))
-   return -EINVAL;
+   return ERR_PTR(-EINVAL);
+
+   /* With fail_gpu_migration, we always perform a GPU clear. */
+   if (I915_SELFTEST_ONLY(fail_gpu_migration))
+   clear = true;
 
dst_level = i915_ttm_cache_level(i915, dst_mem, dst_ttm);
if (clear) {
-   if (bo->type == ttm_bo_type_kernel)
-   return -EINVAL;
+   if (bo->type == ttm_bo_type_kernel &&
+   !I915_SELFTEST_ONLY(fail_gpu_migration))
+   return ERR_PTR(-EINVAL);
 
intel_engine_pm_get(i915->gt.migrate.context->engine);
ret = intel_context_migrate_clear(i915->gt.migrate.context, 
NULL,
  dst_st->sgl, dst_level,
  
i915_ttm_gtt_binds_lmem(dst_mem),
  0, );
-
-   if (!ret && rq) {
-   i915_request_wait(rq, 0, MAX_SCHEDULE_TIMEOUT);
-   i915_request_put(rq);
-   }
-   intel_engine_pm_put(i915->gt.migrate.context->engine);
} else {
struct i915_refct_sgt *src_rsgt =
i915_ttm_resource_get_st(obj, bo->resource);
 
if (IS_ERR(src_rsgt))
-   return PTR_ERR(src_rsgt);
+   return ERR_CAST(src_rsgt);
 
src_level = i915_ttm_cache_level(i915, bo->resource, src_ttm);

[Intel-gfx] [PATCH v4 1/2] drm/i915/ttm: Reorganize the ttm move code

[Thomas Hellström]

We are about to introduce failsafe- and asynchronous migration and
ttm moves.
This will add complexity and code to the TTM move code so it makes sense
to split it out to a separate file to make the i915 TTM code easer to
digest.
Split the i915 TTM move code out and since we will have to change the name
of the gpu_binds_iomem() and cpu_maps_iomem() functions anyway,
we alter the name of gpu_binds_iomem() to i915_ttm_gtt_binds_lmem() which
is more reflecting what it is used for.
With this we also add some more documentation. Otherwise there should be
no functional change.

Signed-off-by: Thomas Hellström 
Reviewed-by: Matthew Auld 
---
 drivers/gpu/drm/i915/Makefile|   1 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c  | 328 +++
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h  |  35 ++
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 308 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h |  38 +++
 5 files changed, 430 insertions(+), 280 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 467872cca027..7d0d0b814670 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -154,6 +154,7 @@ gem-y += \
gem/i915_gem_throttle.o \
gem/i915_gem_tiling.o \
gem/i915_gem_ttm.o \
+   gem/i915_gem_ttm_move.o \
gem/i915_gem_ttm_pm.o \
gem/i915_gem_userptr.o \
gem/i915_gem_wait.o \
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c 
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 6a05369e2705..6369fb9b2455 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -14,13 +14,9 @@
 #include "gem/i915_gem_object.h"
 #include "gem/i915_gem_region.h"
 #include "gem/i915_gem_ttm.h"
+#include "gem/i915_gem_ttm_move.h"
 #include "gem/i915_gem_ttm_pm.h"
 
-
-#include "gt/intel_engine_pm.h"
-#include "gt/intel_gt.h"
-#include "gt/intel_migrate.h"
-
 #define I915_TTM_PRIO_PURGE 0
 #define I915_TTM_PRIO_NO_PAGES  1
 #define I915_TTM_PRIO_HAS_PAGES 2
@@ -108,28 +104,6 @@ static int i915_ttm_err_to_gem(int err)
return err;
 }
 
-static bool gpu_binds_iomem(struct ttm_resource *mem)
-{
-   return mem->mem_type != TTM_PL_SYSTEM;
-}
-
-static bool cpu_maps_iomem(struct ttm_resource *mem)
-{
-   /* Once / if we support GGTT, this is also false for cached ttm_tts */
-   return mem->mem_type != TTM_PL_SYSTEM;
-}
-
-static enum i915_cache_level
-i915_ttm_cache_level(struct drm_i915_private *i915, struct ttm_resource *res,
-struct ttm_tt *ttm)
-{
-   return ((HAS_LLC(i915) || HAS_SNOOP(i915)) && !gpu_binds_iomem(res) &&
-   ttm->caching == ttm_cached) ? I915_CACHE_LLC :
-   I915_CACHE_NONE;
-}
-
-static void i915_ttm_adjust_lru(struct drm_i915_gem_object *obj);
-
 static enum ttm_caching
 i915_ttm_select_tt_caching(const struct drm_i915_gem_object *obj)
 {
@@ -370,23 +344,14 @@ static void i915_ttm_evict_flags(struct ttm_buffer_object 
*bo,
*placement = i915_sys_placement;
 }
 
-static int i915_ttm_move_notify(struct ttm_buffer_object *bo)
-{
-   struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
-   int ret;
-
-   ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
-   if (ret)
-   return ret;
-
-   ret = __i915_gem_object_put_pages(obj);
-   if (ret)
-   return ret;
-
-   return 0;
-}
-
-static void i915_ttm_free_cached_io_rsgt(struct drm_i915_gem_object *obj)
+/**
+ * i915_ttm_free_cached_io_rsgt - Free object cached LMEM information
+ * @obj: The GEM object
+ * This function frees any LMEM-related information that is cached on
+ * the object. For example the radix tree for fast page lookup and the
+ * cached refcounted sg-table
+ */
+void i915_ttm_free_cached_io_rsgt(struct drm_i915_gem_object *obj)
 {
struct radix_tree_iter iter;
void __rcu **slot;
@@ -403,56 +368,16 @@ static void i915_ttm_free_cached_io_rsgt(struct 
drm_i915_gem_object *obj)
obj->ttm.cached_io_rsgt = NULL;
 }
 
-static void
-i915_ttm_adjust_domains_after_move(struct drm_i915_gem_object *obj)
-{
-   struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
-
-   if (cpu_maps_iomem(bo->resource) || bo->ttm->caching != ttm_cached) {
-   obj->write_domain = I915_GEM_DOMAIN_WC;
-   obj->read_domains = I915_GEM_DOMAIN_WC;
-   } else {
-   obj->write_domain = I915_GEM_DOMAIN_CPU;
-   obj->read_domains = I915_GEM_DOMAIN_CPU;
-   }
-}
-
-static void i915_ttm_adjust_gem_after_move(struct drm_i915_gem_object *obj)
-{
-   struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
-   unsigned int cache_level;
-   unsigned int i;
-
-   /*
-* If object was moved to an allowable region, update the object
-

[Intel-gfx] [PATCH v4 0/2] drm/i915: Failsafe migration blits

[Thomas Hellström]

This patch series introduces failsafe migration blits.
The reason for this seemingly strange concept is that if the initial
clearing or readback of LMEM fails for some reason[1], and we then set up
either GPU- or CPU ptes to the allocated LMEM, we can expose old
contents from other clients.

So after each migration blit to LMEM, attach a dma-fence callback that
checks the migration fence error value and if it's an error,
performs a memcpy blit, instead.

Patch 1 splits out the TTM move code into separate files
Patch 2 implements the failsafe blits and related self-tests

[1] There are at least two ways we could trigger exposure of uninitialized
LMEM assuming the migration blits themselves never trigger a gpu hang.

a) A gpu operation preceding a pipelined eviction blit resets and sets the
error fence to -EIO, and the error is propagated across the TTM manager to
the clear / swapin blit of a newly allocated TTM resource. It aborts and
leaves the memory uninitialized.

b) Something wedges the GT while a migration blit is submitted. It ends up
never executed and TTM can fault user-space cpu-ptes into uninitialized
memory.

v3:
- Style fixes in second patch (Matthew Auld)
v4:
- More style fixes in second patch (Matthew Auld)

Thomas Hellström (2):
  drm/i915/ttm: Reorganize the ttm move code
  drm/i915/ttm: Failsafe migration blits

 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/gem/i915_gem_ttm.c   | 328 ++-
 drivers/gpu/drm/i915/gem/i915_gem_ttm.h   |  35 ++
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c  | 520 ++
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h  |  43 ++
 .../drm/i915/gem/selftests/i915_gem_migrate.c |  24 +-
 6 files changed, 670 insertions(+), 281 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
 create mode 100644 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h

-- 
2.31.1

Re: [Intel-gfx] [PATCH 1/2] drm/i915/dp: Optimize the FRL configuration for HDMI2.1 PCON

[Shankar, Uma]

> -Original Message-
> From: Nautiyal, Ankit K 
> Sent: Friday, October 29, 2021 11:32 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Sharma, Swati2 ; Shankar, Uma
> 
> Subject: [PATCH 1/2] drm/i915/dp: Optimize the FRL configuration for HDMI2.1
> PCON
> 
> Currently the HDMI2.1 PCON's frl link config DPCD registers are reset and 
> configured
> even if they are already configured.
> Also the HDMI Link Mode does not settle to FRL MODE immediately after HDMI 
> Link
> Status is active.
> 
> This patch:
> -Checks if the PCON is already configured for FRL.
> -Include HDMI Link Mode in wait for loop along with HDMI Link status DPCD.

Looks good to me.
Reviewed-by: Uma Shankar 

> Signed-off-by: Ankit Nautiyal 
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 37 +++--
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 6d5988f0f067..f5fd106e555c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2198,6 +2198,18 @@ static int intel_dp_hdmi_sink_max_frl(struct intel_dp
> *intel_dp)
>   return max_frl_rate;
>  }
> 
> +static bool
> +intel_dp_pcon_is_frl_trained(struct intel_dp *intel_dp,
> +  u8 max_frl_bw_mask, u8 *frl_trained_mask) {
> + if (drm_dp_pcon_hdmi_link_active(_dp->aux) &&
> + drm_dp_pcon_hdmi_link_mode(_dp->aux, frl_trained_mask) ==
> DP_PCON_HDMI_MODE_FRL &&
> + *frl_trained_mask >= max_frl_bw_mask)
> + return true;
> +
> + return false;
> +}
> +
>  static int intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)  {  
> #define
> TIMEOUT_FRL_READY_MS 500 @@ -2208,10 +2220,6 @@ static int
> intel_dp_pcon_start_frl_training(struct intel_dp *intel_dp)
>   u8 max_frl_bw_mask = 0, frl_trained_mask;
>   bool is_active;
> 
> - ret = drm_dp_pcon_reset_frl_config(_dp->aux);
> - if (ret < 0)
> - return ret;
> -
>   max_pcon_frl_bw = intel_dp->dfp.pcon_max_frl_bw;
>   drm_dbg(>drm, "PCON max rate = %d Gbps\n", max_pcon_frl_bw);
> 
> @@ -2223,6 +2231,12 @@ static int intel_dp_pcon_start_frl_training(struct 
> intel_dp
> *intel_dp)
>   if (max_frl_bw <= 0)
>   return -EINVAL;
> 
> + max_frl_bw_mask = intel_dp_pcon_set_frl_mask(max_frl_bw);
> + drm_dbg(>drm, "MAX_FRL_BW_MASK = %u\n", max_frl_bw_mask);
> +
> + if (intel_dp_pcon_is_frl_trained(intel_dp, max_frl_bw_mask,
> _trained_mask))
> + goto frl_trained;
> +
>   ret = drm_dp_pcon_frl_prepare(_dp->aux, false);
>   if (ret < 0)
>   return ret;
> @@ -2232,7 +2246,6 @@ static int intel_dp_pcon_start_frl_training(struct 
> intel_dp
> *intel_dp)
>   if (!is_active)
>   return -ETIMEDOUT;
> 
> - max_frl_bw_mask = intel_dp_pcon_set_frl_mask(max_frl_bw);
>   ret = drm_dp_pcon_frl_configure_1(_dp->aux, max_frl_bw,
> DP_PCON_ENABLE_SEQUENTIAL_LINK);
>   if (ret < 0)
> @@ -2248,19 +2261,15 @@ static int intel_dp_pcon_start_frl_training(struct
> intel_dp *intel_dp)
>* Wait for FRL to be completed
>* Check if the HDMI Link is up and active.
>*/
> - wait_for(is_active = drm_dp_pcon_hdmi_link_active(_dp->aux) ==
> true, TIMEOUT_HDMI_LINK_ACTIVE_MS);
> + wait_for(is_active =
> +  intel_dp_pcon_is_frl_trained(intel_dp, max_frl_bw_mask,
> _trained_mask),
> +  TIMEOUT_HDMI_LINK_ACTIVE_MS);
> 
>   if (!is_active)
>   return -ETIMEDOUT;
> 
> - /* Verify HDMI Link configuration shows FRL Mode */
> - if (drm_dp_pcon_hdmi_link_mode(_dp->aux, _trained_mask) !=
> - DP_PCON_HDMI_MODE_FRL) {
> - drm_dbg(>drm, "HDMI couldn't be trained in FRL Mode\n");
> - return -EINVAL;
> - }
> - drm_dbg(>drm, "MAX_FRL_MASK = %u, FRL_TRAINED_MASK =
> %u\n", max_frl_bw_mask, frl_trained_mask);
> -
> +frl_trained:
> + drm_dbg(>drm, "FRL_TRAINED_MASK = %u\n", frl_trained_mask);
>   intel_dp->frl.trained_rate_gbps =
> intel_dp_pcon_get_frl_mask(frl_trained_mask);
>   intel_dp->frl.is_trained = true;
>   drm_dbg(>drm, "FRL trained with : %d Gbps\n", intel_dp-
> >frl.trained_rate_gbps);
> --
> 2.25.1

Re: [Intel-gfx] [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Ville Syrjälä]

On Tue, Nov 02, 2021 at 03:59:32PM +0100, Maxime Ripard wrote:
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct 
> drm_connector *connector,
>   u32 max_tmds_clock = hf_vsdb[5] * 5000;
>   struct drm_scdc *scdc = >scdc;
>  
> - if (max_tmds_clock > 34) {
> + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>   display->max_tmds_clock = max_tmds_clock;
>   DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>   display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder 
> *encoder,
>   if (scdc->scrambling.low_rates)
>   pipe_config->hdmi_scrambling = true;
>  
> - if (pipe_config->port_clock > 34) {
> + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>   pipe_config->hdmi_scrambling = true;
>   pipe_config->hdmi_high_tmds_clock_ratio = true;
>   }

All of that is HDMI 2.0 stuff. So this just makes it all super
confusing IMO. Nak.

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[Petri Latvala]

On Tue, Nov 02, 2021 at 06:45:38PM -0700, John Harrison wrote:
> On 11/2/2021 16:34, Matthew Brost wrote:
> > On Thu, Oct 21, 2021 at 04:40:40PM -0700, john.c.harri...@intel.com wrote:
> > > From: John Harrison 
> > > 
> > > Some of the capture tests were using explicit contexts, some not. Some
> > > were poking the per engine pre-emption timeout, some not. This would
> > > lead to sporadic failures due to random timeouts, contexts being
> > > banned depending upon how many subtests were run and/or how many
> > > engines a given platform has, and other such failures.
> > > 
> > > So, update all tests to be conistent.
> > > 
> > > Signed-off-by: John Harrison 
> > > ---
> > >   tests/i915/gem_exec_capture.c | 80 +--
> > >   1 file changed, 58 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> > > index c85c198f7..e373d24ed 100644
> > > --- a/tests/i915/gem_exec_capture.c
> > > +++ b/tests/i915/gem_exec_capture.c
> > > @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset 
> > > *obj_offsets, int obj_count,
> > >   return blobs;
> > >   }
> > > +static void configure_hangs(int fd, const struct intel_execution_engine2 
> > > *e, int ctxt_id)
> > > +{
> > > + /* Ensure fast hang detection */
> > > + gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 
> > > 250);
> > > + gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 
> > > 500);
> > #define for 250, 500?
> Is there any point? There is no special reason for the values other than
> small enough to be fast and long enough to not be too small to be usable. So
> there isn't really any particular name to give them beyond
> 'SHORT_PREEMPT_TIMEOUT' or some such. And the whole point of the helper
> function is that the values are programmed in one place only and not used
> anywhere else. So there is no worry about repetition of magic numbers.

In about one year everyone has forgotten this explanation and will
wonder if it's related to some in-kernel behaviour or if there's some
other reason these values have been chosen.

So at least a comment why the values are these, please.


-- 
Petri Latvala


> 
> 
> > 
> > > +
> > > + /* Allow engine based resets and disable banning */
> > > + igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
> > > +}
> > > +
> > >   static void __capture1(int fd, int dir, uint64_t ahnd, const 
> > > intel_ctx_t *ctx,
> > > -unsigned ring, uint32_t target, uint64_t target_size)
> > > +const struct intel_execution_engine2 *e,
> > > +uint32_t target, uint64_t target_size)
> > >   {
> > >   const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> > >   struct drm_i915_gem_exec_object2 obj[4];
> > > @@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t 
> > > ahnd, const intel_ctx_t *ctx,
> > >   struct offset offset;
> > >   int i;
> > > + configure_hangs(fd, e, ctx->id);
> > > +
> > >   memset(obj, 0, sizeof(obj));
> > >   obj[SCRATCH].handle = gem_create(fd, 4096);
> > >   obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
> > > @@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t 
> > > ahnd, const intel_ctx_t *ctx,
> > >   memset(, 0, sizeof(execbuf));
> > >   execbuf.buffers_ptr = (uintptr_t)obj;
> > >   execbuf.buffer_count = ARRAY_SIZE(obj);
> > > - execbuf.flags = ring;
> > > + execbuf.flags = e->flags;
> > >   if (gen > 3 && gen < 6)
> > >   execbuf.flags |= I915_EXEC_SECURE;
> > >   execbuf.rsvd1 = ctx->id;
> > > @@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t 
> > > ahnd, const intel_ctx_t *ctx,
> > >   gem_close(fd, obj[SCRATCH].handle);
> > >   }
> > > -static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned 
> > > ring)
> > > +static void capture(int fd, int dir, const intel_ctx_t *ctx,
> > > + const struct intel_execution_engine2 *e)
> > >   {
> > >   uint32_t handle;
> > >   uint64_t ahnd;
> > > @@ -335,7 +349,7 @@ static void capture(int fd, int dir, const 
> > > intel_ctx_t *ctx, unsigned ring)
> > >   handle = gem_create(fd, obj_size);
> > >   ahnd = get_reloc_ahnd(fd, ctx->id);
> > > - __capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
> > > + __capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
> > >   gem_close(fd, handle);
> > >   put_ahnd(ahnd);
> > > @@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
> > >   }
> > >   static struct offset *
> > > -__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> > > -   unsigned int size, int count,
> > > -   unsigned int flags)
> > > +__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > > +const struct intel_execution_engine2 *e,
> > > +unsigned int size, int count, unsigned 

Re: [Intel-gfx] [PATCH 01/13] drm/connector: Add define for HDMI 1.4 Maximum Pixel Rate

[Neil Armstrong]

On 02/11/2021 15:59, Maxime Ripard wrote:
> A lot of drivers open-code the HDMI 1.4 maximum pixel rate in their
> driver to test whether the resolutions are supported or if the
> scrambling needs to be enabled.
> 
> Let's create a common define for everyone to use it.
> 
> Cc: Alex Deucher 
> Cc: amd-...@lists.freedesktop.org
> Cc: Andrzej Hajda 
> Cc: Benjamin Gaignard 
> Cc: "Christian König" 
> Cc: Emma Anholt 
> Cc: intel-gfx@lists.freedesktop.org
> Cc: Jani Nikula 
> Cc: Jernej Skrabec 
> Cc: Jerome Brunet 
> Cc: Jonas Karlman 
> Cc: Jonathan Hunter 
> Cc: Joonas Lahtinen 
> Cc: Kevin Hilman 
> Cc: Laurent Pinchart 
> Cc: linux-amlo...@lists.infradead.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> Cc: Martin Blumenstingl 
> Cc: Neil Armstrong 
> Cc: "Pan, Xinhui" 
> Cc: Robert Foss 
> Cc: Rodrigo Vivi 
> Cc: Thierry Reding 
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  | 4 ++--
>  drivers/gpu/drm/drm_edid.c | 2 +-
>  drivers/gpu/drm/i915/display/intel_hdmi.c  | 2 +-
>  drivers/gpu/drm/meson/meson_dw_hdmi.c  | 4 ++--
>  drivers/gpu/drm/radeon/radeon_encoders.c   | 2 +-
>  drivers/gpu/drm/sti/sti_hdmi_tx3g4c28phy.c | 2 +-
>  drivers/gpu/drm/tegra/sor.c| 8 
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++--
>  include/drm/drm_connector.h| 2 ++
>  9 files changed, 16 insertions(+), 14 deletions(-)

For meson & bridge/synopsys/dw-hdmi:

Acked-by: Neil Armstrong 

> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 62ae63565d3a..3a58db357be0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -46,7 +46,7 @@
>  /* DW-HDMI Controller >= 0x200a are at least compliant with SCDC version 1 */
>  #define SCDC_MIN_SOURCE_VERSION  0x1
>  
> -#define HDMI14_MAX_TMDSCLK   34000
> +#define HDMI14_MAX_TMDSCLK   (DRM_HDMI_14_MAX_TMDS_CLK_KHZ * 1000)
>  
>  enum hdmi_datamap {
>   RGB444_8B = 0x01,
> @@ -1264,7 +1264,7 @@ static bool dw_hdmi_support_scdc(struct dw_hdmi *hdmi,
>* for low rates is not supported either
>*/
>   if (!display->hdmi.scdc.scrambling.low_rates &&
> - display->max_tmds_clock <= 34)
> + display->max_tmds_clock <= DRM_HDMI_14_MAX_TMDS_CLK_KHZ)
>   return false;
>  
>   return true;
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7aa2a56a71c8..ec8fb2d098ae 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4966,7 +4966,7 @@ static void drm_parse_hdmi_forum_vsdb(struct 
> drm_connector *connector,
>   u32 max_tmds_clock = hf_vsdb[5] * 5000;
>   struct drm_scdc *scdc = >scdc;
>  
> - if (max_tmds_clock > 34) {
> + if (max_tmds_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>   display->max_tmds_clock = max_tmds_clock;
>   DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>   display->max_tmds_clock);
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index d2e61f6c6e08..0666203d52b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2226,7 +2226,7 @@ int intel_hdmi_compute_config(struct intel_encoder 
> *encoder,
>   if (scdc->scrambling.low_rates)
>   pipe_config->hdmi_scrambling = true;
>  
> - if (pipe_config->port_clock > 34) {
> + if (pipe_config->port_clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ) {
>   pipe_config->hdmi_scrambling = true;
>   pipe_config->hdmi_high_tmds_clock_ratio = true;
>   }
> diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c 
> b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> index 0afbd1e70bfc..8078667aea0e 100644
> --- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
> @@ -434,7 +434,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void 
> *data,
>   readl_relaxed(priv->io_base + _REG(VPU_HDMI_SETTING));
>  
>   DRM_DEBUG_DRIVER("\"%s\" div%d\n", mode->name,
> -  mode->clock > 34 ? 40 : 10);
> +  mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ ? 40 : 10);
>  
>   /* Enable clocks */
>   regmap_update_bits(priv->hhi, HHI_HDMI_CLK_CNTL, 0x, 0x100);
> @@ -457,7 +457,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void 
> *data,
>   dw_hdmi->data->top_write(dw_hdmi, HDMITX_TOP_BIST_CNTL, BIT(12));
>  
>   /* TMDS pattern setup */
> - if (mode->clock > 34 &&
> + if (mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ &&
>   dw_hdmi->output_bus_fmt == MEDIA_BUS_FMT_YUV8_1X24) {
>   dw_hdmi->data->top_write(dw_hdmi, 

Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915: fixup dma_fence_wait usage

[Matthew Auld]

On 02/11/2021 18:48, Patchwork wrote:

*Patch Details*
*Series:*   drm/i915: fixup dma_fence_wait usage
*URL:*	https://patchwork.freedesktop.org/series/96504/ 


*State:*failure
*Details:* 
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21505/index.html 




  CI Bug Log - changes from CI_DRM_10828_full -> Patchwork_21505_full


Summary

*FAILURE*

Serious unknown changes coming with Patchwork_21505_full absolutely need 
to be

verified manually.

If you think the reported changes have nothing to do with the changes
introduced in Patchwork_21505_full, please notify your bug team to allow 
them

to document this new failure mode, which will reduce false positives in CI.


Participating hosts (10 -> 10)

No changes in participating hosts


Possible new issues

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



  IGT changes


Possible regressions

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
  o shard-iclb: PASS


-> FAIL






False positive. Failure looks unrelated.




Known issues

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



  CI changes


Issues hit

  * boot:
  o shard-glk: (PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

)
-> (PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

,
PASS

Re: [Intel-gfx] [PATCH 28/29] drm/i915/gvt: convert to use vfio_register_group_dev()

[Christoph Hellwig]

On Tue, Nov 02, 2021 at 01:41:36PM -0300, Jason Gunthorpe wrote:
> On Tue, Nov 02, 2021 at 08:06:00AM +0100, Christoph Hellwig wrote:
> > This is straightforward conversion, the intel_vgpu already has a pointer
> > to the vfio_dev, which can be replaced with the embedded structure and
> > we can replace all the mdev_get_drvdata() with a simple container_of().
> 
> This should be using vfio_register_emulated_iommu_dev(), right?

Yes, once rebased to 5.16-rc where this function appears.