[Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/guc: Remove more GuC error capture noise
== Series Details == Series: drm/i915/guc: Remove more GuC error capture noise URL : https://patchwork.freedesktop.org/series/106493/ State : failure == Summary == CI Bug Log - changes from CI_DRM_11925_full -> Patchwork_106493v1_full Summary --- **FAILURE** Serious unknown changes coming with Patchwork_106493v1_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_106493v1_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. Participating hosts (13 -> 13) -- No changes in participating hosts Possible new issues --- Here are the unknown changes that may have been introduced in Patchwork_106493v1_full: ### IGT changes ### Possible regressions * igt@device_reset@unbind-reset-rebind: - shard-kbl: [PASS][1] -> [FAIL][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-kbl4/igt@device_re...@unbind-reset-rebind.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-kbl4/igt@device_re...@unbind-reset-rebind.html * igt@kms_plane_cursor@pipe-d-viewport-size-64: - shard-tglb: [PASS][3] -> [INCOMPLETE][4] [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-tglb1/igt@kms_plane_cur...@pipe-d-viewport-size-64.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-tglb8/igt@kms_plane_cur...@pipe-d-viewport-size-64.html Known issues Here are the changes found in Patchwork_106493v1_full that come from known issues: ### IGT changes ### Issues hit * igt@gem_eio@in-flight-contexts-immediate: - shard-apl: [PASS][5] -> [TIMEOUT][6] ([i915#3063]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-apl3/igt@gem_...@in-flight-contexts-immediate.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-apl7/igt@gem_...@in-flight-contexts-immediate.html * igt@gem_exec_balancer@parallel-keep-submit-fence: - shard-iclb: [PASS][7] -> [SKIP][8] ([i915#4525]) +2 similar issues [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-iclb2/igt@gem_exec_balan...@parallel-keep-submit-fence.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-iclb3/igt@gem_exec_balan...@parallel-keep-submit-fence.html * igt@gem_exec_capture@pi@vecs0: - shard-iclb: [PASS][9] -> [INCOMPLETE][10] ([i915#3371]) [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-iclb7/igt@gem_exec_capture@p...@vecs0.html [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-iclb6/igt@gem_exec_capture@p...@vecs0.html * igt@gem_exec_fair@basic-deadline: - shard-glk: [PASS][11] -> [FAIL][12] ([i915#2846]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-glk2/igt@gem_exec_f...@basic-deadline.html [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-glk8/igt@gem_exec_f...@basic-deadline.html * igt@gem_exec_fair@basic-flow@rcs0: - shard-kbl: [PASS][13] -> [SKIP][14] ([fdo#109271]) [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-kbl6/igt@gem_exec_fair@basic-f...@rcs0.html [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-kbl1/igt@gem_exec_fair@basic-f...@rcs0.html * igt@gem_exec_fair@basic-pace-share@rcs0: - shard-glk: [PASS][15] -> [FAIL][16] ([i915#2842]) +1 similar issue [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-glk6/igt@gem_exec_fair@basic-pace-sh...@rcs0.html [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-glk5/igt@gem_exec_fair@basic-pace-sh...@rcs0.html * igt@gem_exec_fair@basic-pace@vcs1: - shard-iclb: NOTRUN -> [FAIL][17] ([i915#2842]) [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-iclb2/igt@gem_exec_fair@basic-p...@vcs1.html * igt@gem_workarounds@suspend-resume-context: - shard-apl: [PASS][18] -> [DMESG-WARN][19] ([i915#180]) +4 similar issues [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/shard-apl3/igt@gem_workarou...@suspend-resume-context.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-apl8/igt@gem_workarou...@suspend-resume-context.html * igt@gen3_render_tiledy_blits: - shard-skl: NOTRUN -> [SKIP][20] ([fdo#109271]) +34 similar issues [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-skl7/igt@gen3_render_tiledy_blits.html * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-dp: - shard-apl: NOTRUN -> [SKIP][21] ([fdo#109271] / [i915#1937]) [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/shard-apl
[Intel-gfx] ✓ Fi.CI.BAT: success for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev7)
== Series Details ==
Series: Fixes integer overflow or integer truncation issues in page lookups,
ttm place configuration and scatterlist creation (rev7)
URL : https://patchwork.freedesktop.org/series/104704/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_11928 -> Patchwork_104704v7
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/index.html
Participating hosts (37 -> 37)
--
Additional (1): bat-dg2-8
Missing(1): fi-icl-u2
Known issues
Here are the changes found in Patchwork_104704v7 that come from known issues:
### IGT changes ###
Issues hit
* igt@gem_exec_suspend@basic-s3@smem:
- fi-rkl-11600: NOTRUN -> [FAIL][1] ([fdo#103375])
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-rkl-11600/igt@gem_exec_suspend@basic...@smem.html
* igt@i915_selftest@live@hangcheck:
- fi-hsw-4770:[PASS][2] -> [INCOMPLETE][3] ([i915#4785])
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-hsw-4770/igt@i915_selftest@l...@hangcheck.html
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-hsw-4770/igt@i915_selftest@l...@hangcheck.html
* igt@i915_selftest@live@requests:
- fi-elk-e7500: [PASS][4] -> [DMESG-FAIL][5] ([i915#4528])
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-elk-e7500/igt@i915_selftest@l...@requests.html
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-elk-e7500/igt@i915_selftest@l...@requests.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-rkl-11600: NOTRUN -> [SKIP][6] ([fdo#111827])
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-rkl-11600/igt@kms_chamel...@common-hpd-after-suspend.html
* igt@runner@aborted:
- fi-hsw-4770:NOTRUN -> [FAIL][7] ([fdo#109271] / [i915#4312] /
[i915#5594] / [i915#6246])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-hsw-4770/igt@run...@aborted.html
- fi-elk-e7500: NOTRUN -> [FAIL][8] ([fdo#109271] / [i915#4312])
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-elk-e7500/igt@run...@aborted.html
Possible fixes
* igt@core_hotunplug@unbind-rebind:
- {bat-adln-1}: [DMESG-WARN][9] ([i915#6297]) -> [PASS][10]
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/bat-adln-1/igt@core_hotunp...@unbind-rebind.html
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/bat-adln-1/igt@core_hotunp...@unbind-rebind.html
* igt@i915_pm_rpm@module-reload:
- fi-cfl-8109u: [DMESG-FAIL][11] ([i915#62]) -> [PASS][12]
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-cfl-8109u/igt@i915_pm_...@module-reload.html
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-cfl-8109u/igt@i915_pm_...@module-reload.html
* igt@i915_selftest@live@guc:
- {bat-dg2-9}:[DMESG-WARN][13] ([i915#5763]) -> [PASS][14] +6
similar issues
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/bat-dg2-9/igt@i915_selftest@l...@guc.html
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/bat-dg2-9/igt@i915_selftest@l...@guc.html
* igt@i915_selftest@live@ring_submission:
- fi-cfl-8109u: [DMESG-WARN][15] ([i915#5904]) -> [PASS][16] +29
similar issues
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-cfl-8109u/igt@i915_selftest@live@ring_submission.html
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-cfl-8109u/igt@i915_selftest@live@ring_submission.html
* igt@i915_suspend@basic-s2idle-without-i915:
- fi-cfl-8109u: [DMESG-WARN][17] ([i915#5904] / [i915#62]) ->
[PASS][18]
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-cfl-8109u/igt@i915_susp...@basic-s2idle-without-i915.html
[18]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-cfl-8109u/igt@i915_susp...@basic-s2idle-without-i915.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-rkl-11600: [INCOMPLETE][19] ([i915#5982]) -> [PASS][20]
[19]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-rkl-11600/igt@i915_susp...@basic-s3-without-i915.html
[20]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-rkl-11600/igt@i915_susp...@basic-s3-without-i915.html
*
igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions-varying-size:
- fi-bsw-kefka: [FAIL][21] ([i915#6298]) -> [PASS][22]
[21]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11928/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cur...@atomic-transitions-varying-size.html
[22]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v7/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cur...@atomic-transitions-varyi
Re: [Intel-gfx] [Intel-gfx 5/6] drm/i915/guc: Rename GuC log relay debugfs descriptively
On Mon, 09 May 2022 14:01:50 -0700, Alan Previn wrote:
>
> @@ -190,11 +190,11 @@ static int guc_log_relay_release(struct inode *inode,
> struct file *file)
> return 0;
> }
>
> -static const struct file_operations guc_log_relay_fops = {
> +static const struct file_operations guc_log_relay_ctl_fops = {
> .owner = THIS_MODULE,
> - .open = guc_log_relay_open,
> - .write = guc_log_relay_write,
> - .release = guc_log_relay_release,
> + .open = guc_log_relay_ctl_open,
> + .write = guc_log_relay_ctl_write,
> + .release = guc_log_relay_ctl_release,
> };
>
> void intel_guc_log_debugfs_register(struct intel_guc_log *log,
> @@ -204,7 +204,7 @@ void intel_guc_log_debugfs_register(struct intel_guc_log
> *log,
> { "guc_log_dump", &guc_log_dump_fops, NULL },
> { "guc_load_err_log_dump", &guc_load_err_log_dump_fops, NULL },
> { "guc_log_level", &guc_log_level_fops, NULL },
> - { "guc_log_relay", &guc_log_relay_fops, NULL },
> + { "guc_log_relay_ctl", &guc_log_relay_ctl_fops, NULL },
Even though debugfs, any issue with changing the file name from the uapi
point of view? Any scripts etc. which will need to be updated?
> { "guc_log_relay_buf_size", &guc_log_relay_buf_size_fops, NULL
> },
> { "guc_log_relay_subbuf_count",
> &guc_log_relay_subbuf_count_fops, NULL },
> };
[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev7)
== Series Details == Series: Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev7) URL : https://patchwork.freedesktop.org/series/104704/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately.
[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev7)
== Series Details ==
Series: Fixes integer overflow or integer truncation issues in page lookups,
ttm place configuration and scatterlist creation (rev7)
URL : https://patchwork.freedesktop.org/series/104704/
State : warning
== Summary ==
Error: dim checkpatch failed
fd0eb0f6ea9e drm: Move and add a few utility macros into drm util header
-:87: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#87: FILE: include/drm/drm_util.h:92:
+#define overflows_type(x, T) \
+ (is_type_unsigned(x) ? \
+ is_type_unsigned(T) ? \
+ (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1
: 0 \
+ : (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) -
1)) ? 1 : 0 \
+ : is_type_unsigned(T) ? \
+ ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >>
BITS_PER_TYPE(T)) ? 1 : 0 \
+ : (sizeof(x) > sizeof(T)) ? \
+ ((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : 0)
-:87: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'T' - possible side-effects?
#87: FILE: include/drm/drm_util.h:92:
+#define overflows_type(x, T) \
+ (is_type_unsigned(x) ? \
+ is_type_unsigned(T) ? \
+ (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1
: 0 \
+ : (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) -
1)) ? 1 : 0 \
+ : is_type_unsigned(T) ? \
+ ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >>
BITS_PER_TYPE(T)) ? 1 : 0 \
+ : (sizeof(x) > sizeof(T)) ? \
+ ((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : 0)
total: 0 errors, 0 warnings, 2 checks, 100 lines checked
5ca6d2f20a7e drm/i915/gem: Typecheck page lookups
-:138: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#138: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:413:
+#define i915_gem_object_page_iter_get_sg(obj, it, n, offset) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_page_iter_get_sg(obj, it, n, offset); \
+})
-:187: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#187: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:458:
+#define i915_gem_object_get_sg(obj, n, offset) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_sg(obj, n, offset); \
+})
-:215: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#215: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:483:
+__i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj, pgoff_t n,
+ unsigned int *offset)
-:236: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#236: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:503:
+#define i915_gem_object_get_sg_dma(obj, n, offset) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_sg_dma(obj, n, offset); \
+})
-:274: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#274: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:539:
+#define i915_gem_object_get_page(obj, n) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_page(obj, n); \
+})
-:311: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#311: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:574:
+#define i915_gem_object_get_dirty_page(obj, n) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_dirty_page(obj, n); \
+})
-:352: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#352: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:612:
+#define i915_gem_object_get_dma_address_len(obj, n, len) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_dma_address_len(obj, n, len); \
+})
-:389: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#389: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:647:
+#define i915_gem_object_get_dma_address(obj, n) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_dma_address(obj, n); \
+})
total: 0 errors, 0 warnings, 8 checks, 616 lines checked
a5e4f12a288f drm/i915: Check for integer truncation on scatterlist creation
-:200: WARNING:NEW_TYPEDEFS: do not add new typedefs
#200: FILE: drivers/gpu/drm/i915/i915_scatterlist.h:224:
+typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
-:201: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in
parentheses
#201: FILE: drivers/gpu/drm/i915/i915_scatterlist.h:225:
+#define sg_alloc_table(sgt, nents, gfp) \
+ overflows_type(nents, __sg_size_t) ? -E2BIG : (sg_alloc_table)(sgt,
(__sg_size_t)(nents), gfp)
-:201: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'nents' - possible
side-effects?
#201: FILE: drivers/gpu/drm/i915/i915_scatterlist.h:225:
+#define sg_alloc_ta
Re: [Intel-gfx] [Intel-gfx 4/6] drm/i915/guc: Provide debugfs for log relay sub-buf info
On Mon, 09 May 2022 14:01:49 -0700, Alan Previn wrote:
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index f454d53a8bca..35709202b09c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -15,7 +15,7 @@
>
> static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log);
>
> -static u32 intel_guc_log_size(struct intel_guc_log *log)
> +u32 intel_guc_log_size(struct intel_guc_log *log)
> {
> /*
>* GuC Log buffer Layout:
> @@ -41,6 +41,12 @@ static u32 intel_guc_log_size(struct intel_guc_log *log)
> return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
> CAPTURE_BUFFER_SIZE;
> }
>
> +#define GUC_LOG_RELAY_SUBBUF_COUNT 8
> +u32 intel_guc_log_relay_subbuf_count(struct intel_guc_log *log)
> +{
> + return GUC_LOG_RELAY_SUBBUF_COUNT;
> +}
uapi wise, instead of exposing guc_log_size and subbuf_count, why not
expose subbuf_size and subbuf_count?
> +static int guc_log_relay_buf_size_get(void *data, u64 *val)
> +{
> + struct intel_guc_log *log = data;
> +
> + if (!log)
> + return -ENODEV;
> + if (!log->vma)
> + return -ENODEV;
Why are these checks needed? Hasn't log been initialized and attached at
intel_gt_debugfs_register_files()/debugfs_create_file() time itself?
Also if needed let's do:
if (!log || !log->vma)
return -ENODEV;
> +
> + *val = (u64)intel_guc_log_size(log);
> +
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_buf_size_fops,
> + guc_log_relay_buf_size_get, NULL,
> + "%lld\n");
> +
> +static int guc_log_relay_subbuf_count_get(void *data, u64 *val)
> +{
> + struct intel_guc_log *log = data;
> +
> + if (!log)
> + return -ENODEV;
> + if (!log->vma)
> + return -ENODEV;
Same issue as above.
> +
> + *val = intel_guc_log_relay_subbuf_count(log);
> +
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(guc_log_relay_subbuf_count_fops,
> + guc_log_relay_subbuf_count_get, NULL,
> + "%lld\n");
> +
> static int guc_log_relay_open(struct inode *inode, struct file *file)
> {
> struct intel_guc_log *log = inode->i_private;
>
> + if (!log)
> + return -ENODEV;
> +
Same issue as above.
> if (!intel_guc_is_ready(log_to_guc(log)))
> return -ENODEV;
>
> @@ -166,6 +205,8 @@ void intel_guc_log_debugfs_register(struct intel_guc_log
> *log,
> { "guc_load_err_log_dump", &guc_load_err_log_dump_fops, NULL },
> { "guc_log_level", &guc_log_level_fops, NULL },
> { "guc_log_relay", &guc_log_relay_fops, NULL },
> + { "guc_log_relay_buf_size", &guc_log_relay_buf_size_fops, NULL
> },
> + { "guc_log_relay_subbuf_count",
> &guc_log_relay_subbuf_count_fops, NULL },
> };
>
> if (!intel_guc_is_supported(log_to_guc(log)))
> --
> 2.25.1
>
Re: [Intel-gfx] [Intel-gfx 3/6] drm/i915/guc: Add a helper for log buffer size
On Mon, 09 May 2022 14:01:48 -0700, Alan Previn wrote:
>
> Add a helper to get GuC log buffer size.
>
> Signed-off-by: Alan Previn
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 49 --
> 1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index d902b40ded0e..f454d53a8bca 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -15,6 +15,32 @@
>
> static void guc_log_copy_debuglogs_for_relay(struct intel_guc_log *log);
>
> +static u32 intel_guc_log_size(struct intel_guc_log *log)
> +{
> + /*
> + * GuC Log buffer Layout:
> + *
> + * NB: Ordering must follow "enum guc_log_buffer_type".
> + *
> + * +===+ 00B
> + * | Debug state header |
> + * +---+ 32B
> + * |Crash dump state header|
> + * +---+ 64B
> + * | Capture state header |
> + * +---+ 96B
> + * | |
> + * +===+ PAGE_SIZE (4KB)
> + * | Debug logs |
> + * +===+ + DEBUG_SIZE
> + * |Crash Dump logs|
> + * +===+ + CRASH_SIZE
> + * | Capture logs |
> + * +===+ + CAPTURE_SIZE
> + */
> + return PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
> CAPTURE_BUFFER_SIZE;
> +}
> +
> /**
> * DOC: GuC firmware log
> *
> @@ -464,32 +490,11 @@ int intel_guc_log_create(struct intel_guc_log *log)
>
> GEM_BUG_ON(log->vma);
>
> - /*
> - * GuC Log buffer Layout
> - * (this ordering must follow "enum guc_log_buffer_type" definition)
> - *
> - * +===+ 00B
> - * | Debug state header |
> - * +---+ 32B
> - * |Crash dump state header|
> - * +---+ 64B
> - * | Capture state header |
> - * +---+ 96B
> - * | |
> - * +===+ PAGE_SIZE (4KB)
> - * | Debug logs |
> - * +===+ + DEBUG_SIZE
> - * |Crash Dump logs|
> - * +===+ + CRASH_SIZE
> - * | Capture logs |
> - * +===+ + CAPTURE_SIZE
> - */
> if (intel_guc_capture_output_min_size_est(guc) > CAPTURE_BUFFER_SIZE)
> DRM_WARN("GuC log buffer for state_capture maybe too small. %d
> < %d\n",
>CAPTURE_BUFFER_SIZE,
> intel_guc_capture_output_min_size_est(guc));
>
> - guc_log_size = PAGE_SIZE + CRASH_BUFFER_SIZE + DEBUG_BUFFER_SIZE +
> -CAPTURE_BUFFER_SIZE;
> + guc_log_size = intel_guc_log_size(log);
>
> vma = intel_guc_allocate_vma(guc, guc_log_size);
> if (IS_ERR(vma)) {
My nit-pick suggestions are:
* Call the static function guc_log_size() (don't append intel_ prefix for
static/internal functions)
* Eliminate the guc_log_size variable and just do
* vma = intel_guc_allocate_vma(guc, guc_log_size());
Otherwise this is:
Reviewed-by: Ashutosh Dixit
Re: [Intel-gfx] [PATCH] drm/i915/display: Cleanup intel_phy_is_combo()
> > -Original Message-
> > From: Murthy, Arun R
> > Sent: Monday, July 18, 2022 8:32 PM
> > To: Srivatsa, Anusha ; intel-
> > g...@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Cleanup
> > intel_phy_is_combo()
> >
> > > -Original Message-
> > > From: Intel-gfx On Behalf
> > > Of Anusha Srivatsa
> > > Sent: Tuesday, July 19, 2022 12:42 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH] drm/i915/display: Cleanup
> > > intel_phy_is_combo()
> > >
> > > No functional change. Cleanup the intel_phy_is_combo to accomodate
> > > for cases where combo phy is not available.
> > >
> > > Cc: Matt Roper
> > > Signed-off-by: Anusha Srivatsa
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 9 +
> > > 1 file changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index a0f84cbe974f..b69208cf9a5e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2082,20 +2082,13 @@ bool intel_phy_is_combo(struct
> > > drm_i915_private *dev_priv, enum phy phy) {
> > > if (phy == PHY_NONE)
> > > return false;
> > > - else if (IS_DG2(dev_priv))
> > > - /*
> > > - * DG2 outputs labelled as "combo PHY" in the bspec use
> > > - * SNPS PHYs with completely different programming,
> > > - * hence we always return false here.
> > > - */
> > > - return false;
> > I feel it would be good to retain this. This is very well commented.
> > In future upon adding something like DISPLAY_VER(dev_priv) >= 11, like
> > the one done below can create confusion.
>
> What if I retain the comments with the code change?
>
Retaining this comment will do, but I feel this cleanup can be taken upon
updating this with the future platforms. Other can comment over here.
Thanks and Regards,
Arun R Murthy
[Intel-gfx] [v4, 1/2] drm/i915/edid: convert DP, HDMI and LVDS to drm_edid
On Fri, Jul 01, 2022 at 12:57:38PM +0300, Ville Syrjälä wrote:
>On Fri, Jul 01, 2022 at 11:55:38AM +0300, Jani Nikula wrote:
>> Convert all the connectors that use cached connector edid and
>> detect_edid to drm_edid.
>>
>> Since drm_get_edid() calls drm_connector_update_edid_property() while
>> drm_edid_read*() do not, we need to call drm_edid_connector_update()
>> separately, in part due to the EDID caching behaviour in HDMI and
>> DP. Especially DP depends on the details parsed from EDID. (The big
>> behavioural change conflating EDID reading with parsing and property
>> update was done in commit 5186421cbfe2 ("drm: Introduce epoch counter to
>> drm_connector"))
>>
>> v4: Call drm_edid_connector_update() after reading HDMI/DP EDID
>>
>> v3: Don't leak vga switcheroo EDID in LVDS init (Ville)
>>
>> v2: Don't leak opregion fallback EDID (Ville)
>>
>> Signed-off-by: Jani Nikula
>> ---
>> .../gpu/drm/i915/display/intel_connector.c| 4 +-
>> .../drm/i915/display/intel_display_types.h| 4 +-
>> drivers/gpu/drm/i915/display/intel_dp.c | 80 +++
>> drivers/gpu/drm/i915/display/intel_hdmi.c | 28 ---
>> drivers/gpu/drm/i915/display/intel_lvds.c | 37 +
>> 5 files changed, 87 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_connector.c
>> b/drivers/gpu/drm/i915/display/intel_connector.c
>> index 1dcc268927a2..d83b2a64f618 100644
>> --- a/drivers/gpu/drm/i915/display/intel_connector.c
>> +++ b/drivers/gpu/drm/i915/display/intel_connector.c
>> @@ -95,12 +95,12 @@ void intel_connector_destroy(struct drm_connector
>> *connector)
>> {
>> struct intel_connector *intel_connector = to_intel_connector(connector);
>>
>> -kfree(intel_connector->detect_edid);
>> +drm_edid_free(intel_connector->detect_edid);
>>
>> intel_hdcp_cleanup(intel_connector);
>>
>> if (!IS_ERR_OR_NULL(intel_connector->edid))
>> -kfree(intel_connector->edid);
>> +drm_edid_free(intel_connector->edid);
>>
>> intel_panel_fini(intel_connector);
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
>> b/drivers/gpu/drm/i915/display/intel_display_types.h
>> index 0da9b208d56e..d476df0ac9df 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
>> @@ -592,8 +592,8 @@ struct intel_connector {
>> struct intel_panel panel;
>>
>> /* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
>> -struct edid *edid;
>> -struct edid *detect_edid;
>> +const struct drm_edid *edid;
>> +const struct drm_edid *detect_edid;
>>
>> /* Number of times hotplug detection was tried after an HPD interrupt */
>> int hotplug_retries;
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 32292c0be2bd..8a3b2dbebe04 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -3577,12 +3577,11 @@ static u8 intel_dp_autotest_edid(struct intel_dp
>> *intel_dp)
>> intel_dp->aux.i2c_defer_count);
>> intel_dp->compliance.test_data.edid =
>> INTEL_DP_RESOLUTION_FAILSAFE;
>> } else {
>> -struct edid *block = intel_connector->detect_edid;
>> +/* FIXME: Get rid of drm_edid_raw() */
>> +const struct edid *block =
>> drm_edid_raw(intel_connector->detect_edid);
>>
>> -/* We have to write the checksum
>> - * of the last block read
>> - */
>> -block += intel_connector->detect_edid->extensions;
>> +/* We have to write the checksum of the last block read */
>> +block += block->extensions;
>>
>> if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_EDID_CHECKSUM,
>> block->checksum) <= 0)
>> @@ -4461,7 +4460,7 @@ bool intel_digital_port_connected(struct intel_encoder
>> *encoder)
>> return is_connected;
>> }
>>
>> -static struct edid *
>> +static const struct drm_edid *
>> intel_dp_get_edid(struct intel_dp *intel_dp)
>> {
>> struct intel_connector *intel_connector = intel_dp->attached_connector;
>> @@ -4472,18 +4471,22 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
>> if (IS_ERR(intel_connector->edid))
>> return NULL;
>>
>> -return drm_edid_duplicate(intel_connector->edid);
>> +return drm_edid_dup(intel_connector->edid);
>> } else
>> -return drm_get_edid(&intel_connector->base,
>> -&intel_dp->aux.ddc);
>> +return drm_edid_read_ddc(&intel_connector->base,
>> + &intel_dp->aux.ddc);
>> }
>>
>> static void
>> intel_dp_update_dfp(struct intel_dp *intel_dp,
>> -const struct edid *edid)
>> +const st
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Pass drm_i915_private struct instead of gt for gen11_gu_misc_irq_handler() (rev2)
== Series Details ==
Series: drm/i915: Pass drm_i915_private struct instead of gt for
gen11_gu_misc_irq_handler() (rev2)
URL : https://patchwork.freedesktop.org/series/106449/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_11927 -> Patchwork_106449v2
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/index.html
Participating hosts (36 -> 30)
--
Additional (1): fi-rkl-11600
Missing(7): bat-dg1-5 bat-dg2-8 bat-dg2-9 bat-adlp-4 bat-adln-1 bat-jsl-3
bat-rpls-2
Known issues
Here are the changes found in Patchwork_106449v2 that come from known issues:
### IGT changes ###
Issues hit
* igt@gem_exec_suspend@basic-s3@smem:
- fi-rkl-11600: NOTRUN -> [FAIL][1] ([fdo#103375])
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@gem_exec_suspend@basic...@smem.html
* igt@gem_huc_copy@huc-copy:
- fi-rkl-11600: NOTRUN -> [SKIP][2] ([i915#2190])
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@gem_huc_c...@huc-copy.html
* igt@gem_lmem_swapping@parallel-random-engines:
- fi-rkl-11600: NOTRUN -> [SKIP][3] ([i915#4613]) +3 similar issues
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@gem_lmem_swapp...@parallel-random-engines.html
* igt@gem_tiled_pread_basic:
- fi-rkl-11600: NOTRUN -> [SKIP][4] ([i915#3282])
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@gem_tiled_pread_basic.html
* igt@i915_pm_backlight@basic-brightness:
- fi-rkl-11600: NOTRUN -> [SKIP][5] ([i915#3012])
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@i915_pm_backli...@basic-brightness.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-rkl-11600: NOTRUN -> [SKIP][6] ([fdo#111827]) +8 similar issues
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@kms_chamel...@hdmi-hpd-fast.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
- fi-rkl-11600: NOTRUN -> [SKIP][7] ([i915#4103])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@kms_cursor_leg...@basic-busy-flip-before-cursor.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-rkl-11600: NOTRUN -> [SKIP][8] ([fdo#109285] / [i915#4098])
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@kms_force_connector_ba...@force-load-detect.html
* igt@kms_psr@sprite_plane_onoff:
- fi-rkl-11600: NOTRUN -> [SKIP][9] ([i915#1072]) +3 similar issues
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@kms_psr@sprite_plane_onoff.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-rkl-11600: NOTRUN -> [SKIP][10] ([i915#3555] / [i915#4098])
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@kms_setm...@basic-clone-single-crtc.html
* igt@prime_vgem@basic-read:
- fi-rkl-11600: NOTRUN -> [SKIP][11] ([fdo#109295] / [i915#3291] /
[i915#3708]) +2 similar issues
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@prime_v...@basic-read.html
* igt@prime_vgem@basic-userptr:
- fi-rkl-11600: NOTRUN -> [SKIP][12] ([fdo#109295] / [i915#3301] /
[i915#3708])
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-rkl-11600/igt@prime_v...@basic-userptr.html
Possible fixes
* igt@i915_selftest@live@gt_heartbeat:
- fi-skl-6700k2: [DMESG-FAIL][13] ([i915#5334]) -> [PASS][14]
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11927/fi-skl-6700k2/igt@i915_selftest@live@gt_heartbeat.html
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-skl-6700k2/igt@i915_selftest@live@gt_heartbeat.html
- fi-kbl-7567u: [DMESG-FAIL][15] ([i915#5334]) -> [PASS][16]
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11927/fi-kbl-7567u/igt@i915_selftest@live@gt_heartbeat.html
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-kbl-7567u/igt@i915_selftest@live@gt_heartbeat.html
* igt@i915_selftest@live@hangcheck:
- {fi-jsl-1}: [INCOMPLETE][17] ([i915#5134]) -> [PASS][18]
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11927/fi-jsl-1/igt@i915_selftest@l...@hangcheck.html
[18]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106449v2/fi-jsl-1/igt@i915_selftest@l...@hangcheck.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=10
[Intel-gfx] ✗ Fi.CI.BUILD: failure for Remove the VFIO_IOMMU_NOTIFY_DMA_UNMAP notifier
== Series Details == Series: Remove the VFIO_IOMMU_NOTIFY_DMA_UNMAP notifier URL : https://patchwork.freedesktop.org/series/106501/ State : failure == Summary == Error: patch https://patchwork.freedesktop.org/api/1.0/series/106501/revisions/1/mbox/ not applied Applying: vfio: Replace the DMA unmapping notifier with a callback Using index info to reconstruct a base tree... M drivers/s390/cio/vfio_ccw_ops.c M drivers/s390/cio/vfio_ccw_private.h M drivers/vfio/vfio.c M include/linux/vfio.h Falling back to patching base and 3-way merge... Auto-merging include/linux/vfio.h CONFLICT (content): Merge conflict in include/linux/vfio.h Auto-merging drivers/vfio/vfio.c Auto-merging drivers/s390/cio/vfio_ccw_private.h CONFLICT (content): Merge conflict in drivers/s390/cio/vfio_ccw_private.h Auto-merging drivers/s390/cio/vfio_ccw_ops.c CONFLICT (content): Merge conflict in drivers/s390/cio/vfio_ccw_ops.c error: Failed to merge in the changes. hint: Use 'git am --show-current-patch=diff' to see the failed patch Patch failed at 0001 vfio: Replace the DMA unmapping notifier with a callback When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort".
Re: [Intel-gfx] [PATCH] drm/i915: Fix NPD in PMU during driver teardown
On Tue, Jul 19, 2022 at 10:00:01AM +0100, Tvrtko Ursulin wrote:
On 12/07/2022 22:03, Umesh Nerlige Ramappa wrote:
On Mon, Jul 04, 2022 at 09:31:55AM +0100, Tvrtko Ursulin wrote:
On 01/07/2022 15:54, Summers, Stuart wrote:
On Fri, 2022-07-01 at 09:37 +0100, Tvrtko Ursulin wrote:
On 01/07/2022 01:11, Umesh Nerlige Ramappa wrote:
On Thu, Jun 30, 2022 at 09:00:28PM +, Stuart Summers wrote:
In the driver teardown, we are unregistering the gt prior
to unregistering the PMU. This means there is a small window
of time in which the application can request metrics from the
PMU, some of which are calling into the uapi engines list,
while the engines are not available. In this case we can
see null pointer dereferences.
Fix this ordering in both the driver load and unload sequences.
Additionally add a check for engine presence to prevent this
NPD in the event this ordering is accidentally reversed. Print
a debug message indicating when they aren't available.
v1: Actually address the driver load/unload ordering issue
Signed-off-by: Stuart Summers
---
I thought this is likely happening because intel_gpu_top is running
in
the background when i915 is unloaded. I tried a quick repro, I
don't see
the unload succeed ("fatal module in use", not sure if this was a
partial unload), but when I try to kill intel_gpu_top, I get an
NPD.
This is in the event disable path - i915_pmu_event_stop ->
i915_pmu_disable.
So i915 failed to unload (as expected - with perf events open we
elevate
the module ref count via i915_pmu_event_init -> drm_dev_get), then
you
quit intel_gpu_top and get NPD? On the engine lookup? With the
re-ordered init/fini sequence as from this patch?
With elevated module count there shouldn't be any unloading happening
so
I am intrigued.
It's likely that you are seeing a different path (unload) leading
to the
same issue.
I think in i915_pmu_disable/disable should be aware of event-
hw.state
and or pmu->closed states before accessing the event. Maybe like,
if (event->hw.state != PERF_HES_STOPPED && is_engine_event(event))
{
@Tvrtko, wondering if this case is tested by igt@perf
_pmu@module-unload.
A bit yes. From what Stuart wrote it seems the test would need to be
extended to cover the case where PMU is getting opened while module
unload is in progress.
But the NPD you saw is for the moment confusing so I don't know what
is
happening.
I am not clear if we should use event->hw.state or pmu->closed here
and
if/how they are related. IMO, for this issue, the engine check is
good
enough too, so we don't really need the pmu state checks.
Thoughts?
Engine check at the moment feels like papering.
Indeed as you say I think the pmu->closed might be the solution.
Perhaps
the race is as mentioned above. PMU open happening in parallel to
unload..
If the sequence of events userspace triggers is:
i915_pmu_event_init
i915_pmu_event_start
i915_pmu_enable
i915_pmu_event_read
I guess pmu->closed can get set halfway in i915_pmu_event_init. What
would be the effect of that.. We'd try to get a module reference
while
in the process of unloading. Which is probably very bad.. So possibly
a
final check on pmu->close is needed there. Ho hum.. can it be made
safe
is the question.
It doesn't explain the NPD on Ctrl-C though.. intel_gpu_top keeps
the
evens open all the time. So I think more info needed, for me at
least.
So one thing here is this doesn't have to do with module unload, but
module unbind specifically (while perf is open). I don't know if the
NPD from Umesh is the same as what we're seeing here. I'd really like
to separate these unless you know for sure that's related. Also it
would be interesting to know if this patch fixes your issue as well.
I still think the re-ordering in i915_driver.c should be enough and we
shouldn't need to check pmu->closed. The unregister should be enough to
ensure the perf tools are notified that new events aren't allowed, and
at that time the engine structures are still intact. And even if for
some reason the perf code still calls in to our function pointers, we
have these engine checks as a failsafe.
I'm by the way uploading one more version here with a drm_WARN_ONCE
instead of the debug print.
Problem is I am not a fan of papering so lets get to the bottom of
the issue first. (In the meantime simple patch to re-order driver
fini is okay since that seems obvious enough, I tnink.)
We need to see call traces from any oopses and try to extend
perf_pmu to catch them. And we need to understand the problem, if
it is a real problem, which I laid out last week about race
between module unload and elevating the module use count from our
perf event init.
Without understanding the details of possible failure mode flows
we don't know how much the papering with engine checks solves and
how much it leaves broken.
If you guys are too busy to tackle that I'll put it onto myself,
but help would certainly be appreciated.
Looks like Stuart/Chris are
Re: [Intel-gfx] [PATCH] drm/i915: Pass drm_i915_private struct instead of gt for gen11_gu_misc_irq_handler()
On 18.07.2022 20:34, Anusha Srivatsa wrote: gen11_gu_misc_irq_handler() does not do anything tile specific. Cc: Matt Roper Signed-off-by: Anusha Srivatsa Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Remove more GuC-Err-Cap noise
On 7/19/2022 10:28, Alan Previn wrote:
Remove the CONFIG_DRM_I915_DEBUG_GUC version of the
__out macro. The original thought was we have additional
dmesg entries in the event that the last gpu_coredump
error capture state was never retrieved, we don't
lose the new capture. These additional messages only
when CONFIG_DRM_I915_DEBUG_GUC is on. However it should
have been a drm_dbg instead of drm_warn. Additionally,
upon further inspection, it became clear we don't really
need this additional messages to align with execlist
as well as remove some more unncessary noise.
Signed-off-by: Alan Previn
---
drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 75257bd20ff0..a9910962d2dc 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -1365,16 +1365,8 @@ guc_capture_reg_to_str(const struct intel_guc *guc, u32
owner, u32 type,
return NULL;
}
-#ifdef CONFIG_DRM_I915_DEBUG_GUC
-#define __out(a, ...) \
- do { \
- drm_warn((&(a)->i915->drm), __VA_ARGS__); \
- i915_error_printf((a), __VA_ARGS__); \
- } while (0)
-#else
#define __out(a, ...) \
i915_error_printf(a, __VA_ARGS__)
-#endif
Is there any point in keeping the _out wrapper? Why not just call
i915_error_printf directly? Seems like an unnecessary level of
obfuscation now.
John.
#define GCAP_PRINT_INTEL_ENG_INFO(ebuf, eng) \
do { \
Re: [Intel-gfx] [PATCH v2] drm/i915/guc: support v69 in parallel to v70
On 7/18/2022 16:07, Daniele Ceraolo Spurio wrote:
This patch re-introduces support for GuC v69 in parallel to v70. As this
is a quick fix, v69 has been re-introduced as the single "fallback" guc
version in case v70 is not available on disk and only for platforms that
are out of force_probe and require the GuC by default. All v69 specific
code has been labeled as such for easy identification, and the same was
done for all v70 functions for which there is a separate v69 version,
to avoid accidentally calling the wrong version via the unlabeled name.
When the fallback mode kicks in, a drm_notice message is printed in
dmesg to inform the user of the required update. The existing
logging of the fetch function has also been updated so that we no
longer complain immediately if we can't find a fw and we only throw an
error if the fetch of both the base and fallback blobs fails.
The plan is to follow this up with a more complex rework to allow for
multiple different GuC versions to be supported at the same time.
v2: reduce the fallback to platform that require it, switch to
firmware_request_nowarn(), improve logs.
Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")
Link: https://lists.freedesktop.org/archives/intel-gfx/2022-July/301640.html
Signed-off-by: Daniele Ceraolo Spurio
Cc: John Harrison
Cc: Matthew Brost
Cc: Matt Roper
Cc: Dave Airlie
Cc: Michal Wajdeczko
Acked-by: Rodrigo Vivi
Reviewed-by: John Harrison
---
drivers/gpu/drm/i915/gt/intel_context_types.h | 11 +-
.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 3 +
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 5 +
drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 45 +++
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 352 +++---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 56 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 7 +
7 files changed, 417 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index d2d75d9c0c8d..04eacae1aca5 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -275,10 +275,17 @@ struct intel_context {
u8 child_index;
/** @guc: GuC specific members for parallel submission */
struct {
- /** @wqi_head: head pointer in work queue */
+ /** @wqi_head: cached head pointer in work queue */
u16 wqi_head;
- /** @wqi_tail: tail pointer in work queue */
+ /** @wqi_tail: cached tail pointer in work queue */
u16 wqi_tail;
+ /** @wq_head: pointer to the actual head in work queue
*/
+ u32 *wq_head;
+ /** @wq_tail: pointer to the actual head in work queue
*/
+ u32 *wq_tail;
+ /** @wq_status: pointer to the status in work queue */
+ u32 *wq_status;
+
/**
* @parent_page: page in context state (ce->state) used
* by parent for work queue, process descriptor
diff --git a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
index 4ef9990ed7f8..29ef8afc8c2e 100644
--- a/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/i915/gt/uc/abi/guc_actions_abi.h
@@ -122,6 +122,9 @@ enum intel_guc_action {
INTEL_GUC_ACTION_SCHED_CONTEXT_MODE_DONE = 0x1002,
INTEL_GUC_ACTION_SCHED_ENGINE_MODE_SET = 0x1003,
INTEL_GUC_ACTION_SCHED_ENGINE_MODE_DONE = 0x1004,
+ INTEL_GUC_ACTION_V69_SET_CONTEXT_PRIORITY = 0x1005,
+ INTEL_GUC_ACTION_V69_SET_CONTEXT_EXECUTION_QUANTUM = 0x1006,
+ INTEL_GUC_ACTION_V69_SET_CONTEXT_PREEMPTION_TIMEOUT = 0x1007,
INTEL_GUC_ACTION_CONTEXT_RESET_NOTIFICATION = 0x1008,
INTEL_GUC_ACTION_ENGINE_FAILURE_NOTIFICATION = 0x1009,
INTEL_GUC_ACTION_HOST2GUC_UPDATE_CONTEXT_POLICIES = 0x100B,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index d0d99f178f2d..a7acffbf15d1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -170,6 +170,11 @@ struct intel_guc {
/** @ads_engine_usage_size: size of engine usage in the ADS */
u32 ads_engine_usage_size;
+ /** @lrc_desc_pool_v69: object allocated to hold the GuC LRC descriptor pool */
+ struct i915_vma *lrc_desc_pool_v69;
+ /** @lrc_desc_pool_vaddr_v69: contents of the GuC LRC descriptor pool */
+ void *lrc_desc_pool_vaddr_v69;
+
/**
* @context_lookup: used to resolve intel_context from guc_id, if a
* context is present in this structure it is registered with the GuC
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
b/drivers
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/guc: Remove more GuC error capture noise
== Series Details ==
Series: drm/i915/guc: Remove more GuC error capture noise
URL : https://patchwork.freedesktop.org/series/106493/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_11925 -> Patchwork_106493v1
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/index.html
Participating hosts (37 -> 32)
--
Additional (1): bat-jsl-3
Missing(6): bat-dg1-5 bat-dg2-8 bat-dg2-9 bat-adlp-4 bat-adln-1
bat-rpls-2
Known issues
Here are the changes found in Patchwork_106493v1 that come from known issues:
### IGT changes ###
Warnings
* igt@i915_selftest@live@hangcheck:
- fi-hsw-g3258: [INCOMPLETE][1] ([i915#4785]) -> [INCOMPLETE][2]
([i915#3303] / [i915#4785])
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/fi-hsw-g3258/igt@i915_selftest@l...@hangcheck.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/fi-hsw-g3258/igt@i915_selftest@l...@hangcheck.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-rkl-11600: [FAIL][3] ([fdo#103375]) -> [INCOMPLETE][4]
([i915#5982])
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11925/fi-rkl-11600/igt@i915_susp...@basic-s3-without-i915.html
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/fi-rkl-11600/igt@i915_susp...@basic-s3-without-i915.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
[fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
[fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
[fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#3003]: https://gitlab.freedesktop.org/drm/intel/issues/3003
[i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
[i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
[i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
[i915#5903]: https://gitlab.freedesktop.org/drm/intel/issues/5903
[i915#5982]: https://gitlab.freedesktop.org/drm/intel/issues/5982
Build changes
-
* Linux: CI_DRM_11925 -> Patchwork_106493v1
CI-20190529: 20190529
CI_DRM_11925: 5dfb5a4e47e73362e5557bc87d1b97bb96dc8903 @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_6592: d7c0659613199a5dcf535ed3add68ed1991ead7e @
https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_106493v1: 5dfb5a4e47e73362e5557bc87d1b97bb96dc8903 @
git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
38371677b955 drm/i915/guc: Remove more GuC-Err-Cap noise
== Logs ==
For more details see:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106493v1/index.html
Re: [Intel-gfx] [Intel-gfx 2/6] drm/i915/guc: Add unaligned wc memcpy for copying GuC Log
On Mon, 09 May 2022 14:01:47 -0700, Alan Previn wrote:
>
> Add usage of unaligned wc mempy in read_update_log_buffer
> as newer formats of GuC debug-log-events are no longer
> guaranteed to be exactly 4-dwords long per event.
>
> Signed-off-by: Alan Previn
> ---
> drivers/gpu/drm/i915/gt/uc/intel_guc_log.c | 9 ++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index 09f4d5fbca82..d902b40ded0e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -301,13 +301,16 @@ static void _guc_log_copy_debuglogs_for_relay(struct
> intel_guc_log *log)
>
> /* Just copy the newly written data */
> if (read_offset > write_offset) {
> - i915_memcpy_from_wc(dst_data, src_data, write_offset);
> + if (!i915_memcpy_from_wc(dst_data, src_data,
> write_offset))
> + i915_unaligned_memcpy_from_wc(dst_data,
> src_data, write_offset);
> bytes_to_copy = buffer_size - read_offset;
> } else {
> bytes_to_copy = write_offset - read_offset;
> }
> - i915_memcpy_from_wc(dst_data + read_offset,
> - src_data + read_offset, bytes_to_copy);
> + if (!i915_memcpy_from_wc(dst_data + read_offset,
> + src_data + read_offset, bytes_to_copy))
> + i915_unaligned_memcpy_from_wc(dst_data + read_offset,
> + src_data + read_offset,
> bytes_to_copy);
It should be possible to call i915_unaligned_memcpy_from_wc()
unconditionally? Looks like it would work but not sure of the performance
gain between first attempting an aligned copy and falling back to
unaligned. Assuming there is some, this is:
Reviewed-by: Ashutosh Dixit
Re: [Intel-gfx] [PATCH] drm/i915/display: Cleanup intel_phy_is_combo()
> -Original Message-
> From: Murthy, Arun R
> Sent: Monday, July 18, 2022 8:32 PM
> To: Srivatsa, Anusha ; intel-
> g...@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH] drm/i915/display: Cleanup
> intel_phy_is_combo()
>
> > -Original Message-
> > From: Intel-gfx On Behalf Of
> > Anusha Srivatsa
> > Sent: Tuesday, July 19, 2022 12:42 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH] drm/i915/display: Cleanup
> > intel_phy_is_combo()
> >
> > No functional change. Cleanup the intel_phy_is_combo to accomodate for
> > cases where combo phy is not available.
> >
> > Cc: Matt Roper
> > Signed-off-by: Anusha Srivatsa
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 9 +
> > 1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index a0f84cbe974f..b69208cf9a5e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2082,20 +2082,13 @@ bool intel_phy_is_combo(struct
> > drm_i915_private *dev_priv, enum phy phy) {
> > if (phy == PHY_NONE)
> > return false;
> > - else if (IS_DG2(dev_priv))
> > - /*
> > -* DG2 outputs labelled as "combo PHY" in the bspec use
> > -* SNPS PHYs with completely different programming,
> > -* hence we always return false here.
> > -*/
> > - return false;
> I feel it would be good to retain this. This is very well commented. In future
> upon adding something like DISPLAY_VER(dev_priv) >= 11, like the one done
> below can create confusion.
What if I retain the comments with the code change?
Anusha
>
> > else if (IS_ALDERLAKE_S(dev_priv))
> > return phy <= PHY_E;
> > else if (IS_DG1(dev_priv) || IS_ROCKETLAKE(dev_priv))
> > return phy <= PHY_D;
> > else if (IS_JSL_EHL(dev_priv))
> > return phy <= PHY_C;
> > - else if (DISPLAY_VER(dev_priv) >= 11)
> > + else if (IS_ALDERLAKE_P(dev_priv) || IS_DISPLAY_VER(dev_priv, 11,
> > 12))
> > return phy <= PHY_B;
> > else
> > return false;
> > --
> > 2.25.1
>
> Thanks and Regards,
> Arun R Murthy
>
[Intel-gfx] [Intel-gfx 1/1] drm/i915/guc: Remove more GuC-Err-Cap noise
Remove the CONFIG_DRM_I915_DEBUG_GUC version of the
__out macro. The original thought was we have additional
dmesg entries in the event that the last gpu_coredump
error capture state was never retrieved, we don't
lose the new capture. These additional messages only
when CONFIG_DRM_I915_DEBUG_GUC is on. However it should
have been a drm_dbg instead of drm_warn. Additionally,
upon further inspection, it became clear we don't really
need this additional messages to align with execlist
as well as remove some more unncessary noise.
Signed-off-by: Alan Previn
---
drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 75257bd20ff0..a9910962d2dc 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -1365,16 +1365,8 @@ guc_capture_reg_to_str(const struct intel_guc *guc, u32
owner, u32 type,
return NULL;
}
-#ifdef CONFIG_DRM_I915_DEBUG_GUC
-#define __out(a, ...) \
- do { \
- drm_warn((&(a)->i915->drm), __VA_ARGS__); \
- i915_error_printf((a), __VA_ARGS__); \
- } while (0)
-#else
#define __out(a, ...) \
i915_error_printf(a, __VA_ARGS__)
-#endif
#define GCAP_PRINT_INTEL_ENG_INFO(ebuf, eng) \
do { \
--
2.25.1
[Intel-gfx] [Intel-gfx 0/1] drm/i915/guc: Remove more GuC error capture noise
This series removes unnecessary drm_warns that gets built when CONFIG_DRM_I915_DEBUG_GUC is set. Alan Previn (1): drm/i915/guc: Remove more GuC-Err-Cap noise drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 8 1 file changed, 8 deletions(-) base-commit: 8342f0dc591389ddc341617f8208c18f462e0b24 -- 2.25.1
[Intel-gfx] ✓ Fi.CI.BAT: success for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev6)
== Series Details ==
Series: Fixes integer overflow or integer truncation issues in page lookups,
ttm place configuration and scatterlist creation (rev6)
URL : https://patchwork.freedesktop.org/series/104704/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_11923 -> Patchwork_104704v6
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v6/index.html
Participating hosts (36 -> 37)
--
Additional (1): bat-dg2-9
Known issues
Here are the changes found in Patchwork_104704v6 that come from known issues:
### IGT changes ###
Issues hit
* igt@i915_selftest@live@requests:
- fi-pnv-d510:[PASS][1] -> [DMESG-FAIL][2] ([i915#4528])
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11923/fi-pnv-d510/igt@i915_selftest@l...@requests.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v6/fi-pnv-d510/igt@i915_selftest@l...@requests.html
Possible fixes
* igt@i915_selftest@live@sanitycheck:
- {bat-adln-1}: [DMESG-FAIL][3] ([i915#6297]) -> [PASS][4]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11923/bat-adln-1/igt@i915_selftest@l...@sanitycheck.html
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v6/bat-adln-1/igt@i915_selftest@l...@sanitycheck.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#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#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
[i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
[i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
[i915#3595]: https://gitlab.freedesktop.org/drm/intel/issues/3595
[i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
[i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
[i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
[i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
[i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
[i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
[i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
[i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
[i915#4528]: https://gitlab.freedesktop.org/drm/intel/issues/4528
[i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
[i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
[i915#5174]: https://gitlab.freedesktop.org/drm/intel/issues/5174
[i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
[i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
[i915#5763]: https://gitlab.freedesktop.org/drm/intel/issues/5763
[i915#6297]: https://gitlab.freedesktop.org/drm/intel/issues/6297
Build changes
-
* Linux: CI_DRM_11923 -> Patchwork_104704v6
CI-20190529: 20190529
CI_DRM_11923: ff2849b5552c960205ac3e3b3f7a7be78a96702a @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_6592: d7c0659613199a5dcf535ed3add68ed1991ead7e @
https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_104704v6: ff2849b5552c960205ac3e3b3f7a7be78a96702a @
git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
ab66aade407a drm/i915: Remove truncation warning for large objects
821908e6ffe6 drm/i915: Use error code as -E2BIG when the size of gem ttm object
is too large
c1714af71852 drm/i915: Check if the size is too big while creating shmem file
ef9456c56f81 drm/i915: Check for integer truncation on the configuration of ttm
place
1b3a7139baf1 drm/i915: Check for integer truncation on scatterlist creation
c6df3d0c779f drm/i915/gem: Typecheck page lookups
d5fbb4ad562d drm: Move and add a few utility macros into drm util header
== Logs ==
For more details see:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_104704v6/index.html
[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation (rev6)
== Series Details ==
Series: Fixes integer overflow or integer truncation issues in page lookups,
ttm place configuration and scatterlist creation (rev6)
URL : https://patchwork.freedesktop.org/series/104704/
State : warning
== Summary ==
Error: dim checkpatch failed
854c8d829590 drm: Move and add a few utility macros into drm util header
-:87: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'x' - possible side-effects?
#87: FILE: include/drm/drm_util.h:92:
+#define overflows_type(x, T) \
+ (is_type_unsigned(x) ? \
+ is_type_unsigned(T) ? \
+ (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1
: 0 \
+ : (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) -
1)) ? 1 : 0 \
+ : is_type_unsigned(T) ? \
+ ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >>
BITS_PER_TYPE(T)) ? 1 : 0 \
+ : (sizeof(x) > sizeof(T)) ? \
+ ((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : 0)
-:87: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'T' - possible side-effects?
#87: FILE: include/drm/drm_util.h:92:
+#define overflows_type(x, T) \
+ (is_type_unsigned(x) ? \
+ is_type_unsigned(T) ? \
+ (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1
: 0 \
+ : (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) -
1)) ? 1 : 0 \
+ : is_type_unsigned(T) ? \
+ ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >>
BITS_PER_TYPE(T)) ? 1 : 0 \
+ : (sizeof(x) > sizeof(T)) ? \
+ ((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : 0)
total: 0 errors, 0 warnings, 2 checks, 100 lines checked
8c58b65ea183 drm/i915/gem: Typecheck page lookups
-:138: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#138: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:413:
+#define i915_gem_object_page_iter_get_sg(obj, it, n, offset) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_page_iter_get_sg(obj, it, n, offset); \
+})
-:187: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#187: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:458:
+#define i915_gem_object_get_sg(obj, n, offset) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_sg(obj, n, offset); \
+})
-:215: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#215: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:483:
+__i915_gem_object_get_sg_dma(struct drm_i915_gem_object *obj, pgoff_t n,
+ unsigned int *offset)
-:236: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#236: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:503:
+#define i915_gem_object_get_sg_dma(obj, n, offset) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_sg_dma(obj, n, offset); \
+})
-:274: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#274: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:539:
+#define i915_gem_object_get_page(obj, n) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_page(obj, n); \
+})
-:311: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#311: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:574:
+#define i915_gem_object_get_dirty_page(obj, n) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_dirty_page(obj, n); \
+})
-:352: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#352: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:612:
+#define i915_gem_object_get_dma_address_len(obj, n, len) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_dma_address_len(obj, n, len); \
+})
-:389: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'n' - possible side-effects?
#389: FILE: drivers/gpu/drm/i915/gem/i915_gem_object.h:647:
+#define i915_gem_object_get_dma_address(obj, n) ({ \
+ exactly_pgoff_t(n); \
+ __i915_gem_object_get_dma_address(obj, n); \
+})
total: 0 errors, 0 warnings, 8 checks, 616 lines checked
59f1bda3a657 drm/i915: Check for integer truncation on scatterlist creation
-:200: WARNING:NEW_TYPEDEFS: do not add new typedefs
#200: FILE: drivers/gpu/drm/i915/i915_scatterlist.h:224:
+typedef unsigned int __sg_size_t; /* see linux/scatterlist.h */
-:201: ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in
parentheses
#201: FILE: drivers/gpu/drm/i915/i915_scatterlist.h:225:
+#define sg_alloc_table(sgt, nents, gfp) \
+ overflows_type(nents, __sg_size_t) ? -E2BIG : (sg_alloc_table)(sgt,
(__sg_size_t)(nents), gfp)
-:201: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'nents' - possible
side-effects?
#201: FILE: drivers/gpu/drm/i915/i915_scatterlist.h:225:
+#define sg_alloc_ta
Re: [Intel-gfx] [PATCH] drm/i915/guc: support v69 in parallel to v70
On 7/19/2022 12:34 AM, Tvrtko Ursulin wrote:
On 18/07/2022 17:41, Ceraolo Spurio, Daniele wrote:
On 7/18/2022 3:02 AM, Tvrtko Ursulin wrote:
Hi,
On 15/07/2022 23:54, Daniele Ceraolo Spurio wrote:
This patch re-introduces support for GuC v69 in parallel to v70. As
this
is a quick fix, v69 has been re-introduced as the single "fallback"
guc
version in case v70 is not available on disk. All v69 specific code
has
been labeled as such for easy identification, and the same was done
for
all v70 functions for which there is a separate v69 version, to avoid
accidentally calling the wrong version via the unlabeled name.
When the fallback mode kicks in, a drm_warn message is printed in
dmesg
to warn the user of the required update.
The plan is to follow this up with a more complex rework to allow for
multiple different GuC versions to be supported at the same time.
Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")
Please check if I got this right:
* ADL-P was out of "force probe" starting from 5.17.
* GuC fw got bumped from v62 to v69 in 5.18.
Does this mean you would also need to handle v62 to avoid regressing
ADL-P from 5.17 to 5.18? I couldn't figure out when ADL-P switched
from execlists to GuC due a bit convoluted supported/wanted/needed
macros etc, so not entirely sure.
I haven't checked about previous GuC versions because the report from
Dave was on the 69->70 transition and about re-introducing v69
support, so I just focused on that. Let me dig on the versions and on
what would be needed to support all 3 revs (if it is required).
Secondly, my concern with the approach like in this patch is that it
would grow the i915 code base *if* there is no incentive to keep the
compatiblity breaking firware updates in check.
The grow of the i915 code is inevitable. Even without changes to
existing platforms, new features for new platforms will require new
GuC interfaces. Sometimes the GuC team also refactors an existing
interface so that it can include a new aspect of an existing feature.
We'll have to discuss with them how to minimize breakages in such
scenarios.
To think about in tandem with this is the question of whether many
more fallback versions need to be handled, even for platforms which
only use GuC to load HuC? Those would also regress in the media
encoding side of things, even if they don't use GuC submission, right?
The only HuC-only platform is ADL-S and that went out of force probe
when we were on GuC 62, so definitely nothing older than that will be
needed.
I was referring to platforms where HuC is used for some encoding
types. List on
https://github.com/intel/media-driver/blob/master/docs/media_features.md#media-features-summary.
It is not entirely clear to me from that list - you are saying the HuC
is actually used only on ADL-S? I was going by the existence of HuC
firmware files only so might be wrong just as well.
Like GuC, HuC can be enabled via modparam on anything gen11+, but it is
only enabled by default on a subset of platforms, which are all the
platforms for which we enable GuC submission, plus ADL-S. Of those, the
only ones out of force probe are the ADL variants and their derivatives,
so they're the only ones we need to guarantee backwards compatibility for.
See uc_expand_default_options() in intel_uc.c for further details.
Daniele
Regards,
Tvrtko
If this is so, the approach from this patch would feel rushed in my
view.
It totally is, no argument there. As mentioned in the commit message,
the plan is to replace the whole thing with a more flexible and
cleaner mechanism, but we do need something for the upcoming 5.19
release so there is no time to do this properly from the get-go.
There is also the question of being able to automatically load the
latest _compatible_ (same major version) GuC fw found on disk. Aka
allowing a bugfix firmware update which does not require a kernel
update. In theory should be possible but we don't have that
implemented either, right?
We do not. Something like this was actually shot down when GuC first
came around. We used to have simlinks for the GuC binary to be able
to drop in a replacement like you said, but there were concerns about
how to validate all the possible kernel:fw combinations this could
cause, hence why in the end we went with the exact match model. Note
that at the time we didn't have a patch number for bugfix tracking in
GuC, so the decision made more sense back then than it does now.
We've already restarted the discussion internally.
Daniele
Regards,
Tvrtko
Link:
https://lists.freedesktop.org/archives/intel-gfx/2022-July/301640.html
Signed-off-by: Daniele Ceraolo Spurio
Cc: John Harrison
Cc: Matthew Brost
Cc: Matt Roper
Cc: Dave Airlie
---
drivers/gpu/drm/i915/gt/intel_context_types.h | 11 +-
.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 3 +
drivers/gpu/drm/i915/gt/uc/intel_guc.h | 5 +
drive
Re: [Intel-gfx] ✗ Fi.CI.IGT: failure for drm/i915/tgl+: Fix HDMI transcoder clock vs. DDI BUF disabling order
Filed a new issue https://gitlab.freedesktop.org/drm/intel/-/issues/6460 and re-reported. But shards re-reporting is not working at the moment? FYI @Sarvela, Tomi P Lakshmi. -Original Message- From: Deak, Imre Sent: Monday, July 18, 2022 5:21 AM To: intel-gfx@lists.freedesktop.org; Nautiyal, Ankit K ; Vudum, Lakshminarayana Subject: Re: ✗ Fi.CI.IGT: failure for drm/i915/tgl+: Fix HDMI transcoder clock vs. DDI BUF disabling order On Sat, Jun 18, 2022 at 12:50:26AM +, Patchwork wrote: > == Series Details == > > Series: drm/i915/tgl+: Fix HDMI transcoder clock vs. DDI BUF disabling order > URL : https://patchwork.freedesktop.org/series/105290/ > State : failure Thanks for the review, patch pushed to drm-intel-next. The failure is unrelated, see below. > == Summary == > > CI Bug Log - changes from CI_DRM_11775_full -> Patchwork_105290v1_full > > > Summary > --- > > **FAILURE** > > Serious unknown changes coming with Patchwork_105290v1_full absolutely need > to be > verified manually. > > If you think the reported changes have nothing to do with the changes > introduced in Patchwork_105290v1_full, please notify your bug team to allow > them > to document this new failure mode, which will reduce false positives in CI. > > > > Participating hosts (13 -> 10) > -- > > Missing(3): shard-rkl shard-dg1 shard-tglu > > Possible new issues > --- > > Here are the unknown changes that may have been introduced in > Patchwork_105290v1_full: > > ### IGT changes ### > > Possible regressions > > * igt@kms_atomic_transition@modeset-transition-nonblocking@1x-outputs: > - shard-tglb: [PASS][1] -> [INCOMPLETE][2] >[1]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-tglb5/igt@kms_atomic_transition@modeset-transition-nonblock...@1x-outputs.html >[2]: > https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_105290v1/shard-tglb > 3/igt@kms_atomic_transition@modeset-transition-nonblocking@1x-outputs. > html There is no HDMI output modesetted on the above machine, so the issue is unrelated. > Known issues > > > Here are the changes found in Patchwork_105290v1_full that come from known > issues: > > ### CI changes ### > > Issues hit > > * boot: > - shard-apl: ([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], [PASS][47], [PASS][48], > [PASS][49], [PASS][50], [FAIL][51], [PASS][52]) ([i915#4386]) >[3]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl8/boot.html >[4]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl8/boot.html >[5]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl8/boot.html >[6]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl8/boot.html >[7]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl7/boot.html >[8]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl7/boot.html >[9]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl7/boot.html >[10]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl7/boot.html >[11]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl6/boot.html >[12]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl6/boot.html >[13]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl6/boot.html >[14]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl4/boot.html >[15]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl4/boot.html >[16]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl4/boot.html >[17]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl3/boot.html >[18]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl3/boot.html >[19]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl3/boot.html >[20]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl3/boot.html >[21]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl2/boot.html >[22]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl2/boot.html >[23]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/shard-apl2/boot.html >[24]: > https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11775/sh
[Intel-gfx] ✓ Fi.CI.BAT: success for HDR aux backlight range calculation
== Series Details ==
Series: HDR aux backlight range calculation
URL : https://patchwork.freedesktop.org/series/106475/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_11922 -> Patchwork_106475v1
Summary
---
**SUCCESS**
No regressions found.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/index.html
Participating hosts (37 -> 36)
--
Missing(1): bat-adln-1
Known issues
Here are the changes found in Patchwork_106475v1 that come from known issues:
### IGT changes ###
Issues hit
* igt@gem_huc_copy@huc-copy:
- fi-icl-u2: NOTRUN -> [SKIP][1] ([i915#2190])
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@gem_huc_c...@huc-copy.html
* igt@gem_lmem_swapping@random-engines:
- fi-icl-u2: NOTRUN -> [SKIP][2] ([i915#4613]) +3 similar issues
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@gem_lmem_swapp...@random-engines.html
* igt@i915_selftest@live@gem:
- fi-pnv-d510:NOTRUN -> [DMESG-FAIL][3] ([i915#4528])
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-pnv-d510/igt@i915_selftest@l...@gem.html
* igt@i915_selftest@live@reset:
- bat-adlp-4: [PASS][4] -> [DMESG-FAIL][5] ([i915#4983])
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11922/bat-adlp-4/igt@i915_selftest@l...@reset.html
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/bat-adlp-4/igt@i915_selftest@l...@reset.html
* igt@i915_suspend@basic-s3-without-i915:
- fi-icl-u2: NOTRUN -> [SKIP][6] ([i915#5903])
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@i915_susp...@basic-s3-without-i915.html
* igt@kms_chamelium@common-hpd-after-suspend:
- fi-blb-e6850: NOTRUN -> [SKIP][7] ([fdo#109271])
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-blb-e6850/igt@kms_chamel...@common-hpd-after-suspend.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-icl-u2: NOTRUN -> [SKIP][8] ([fdo#111827]) +8 similar issues
[8]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@kms_chamel...@hdmi-hpd-fast.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
- fi-icl-u2: NOTRUN -> [SKIP][9] ([i915#4103])
[9]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@kms_cursor_leg...@basic-busy-flip-before-cursor.html
* igt@kms_force_connector_basic@force-connector-state:
- fi-icl-u2: NOTRUN -> [WARN][10] ([i915#6008])
[10]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@kms_force_connector_ba...@force-connector-state.html
* igt@kms_force_connector_basic@force-load-detect:
- fi-icl-u2: NOTRUN -> [SKIP][11] ([fdo#109285])
[11]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@kms_force_connector_ba...@force-load-detect.html
* igt@kms_setmode@basic-clone-single-crtc:
- fi-icl-u2: NOTRUN -> [SKIP][12] ([i915#3555])
[12]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@kms_setm...@basic-clone-single-crtc.html
* igt@prime_vgem@basic-userptr:
- fi-icl-u2: NOTRUN -> [SKIP][13] ([fdo#109295] / [i915#3301])
[13]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@prime_v...@basic-userptr.html
* igt@runner@aborted:
- bat-adlp-4: NOTRUN -> [FAIL][14] ([i915#4312])
[14]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/bat-adlp-4/igt@run...@aborted.html
Possible fixes
* igt@fbdev@read:
- {bat-rpls-2}: [SKIP][15] ([i915#2582]) -> [PASS][16] +4 similar
issues
[15]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11922/bat-rpls-2/igt@fb...@read.html
[16]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/bat-rpls-2/igt@fb...@read.html
* igt@gem_ctx_create@basic-files:
- fi-icl-u2: [DMESG-FAIL][17] -> [PASS][18]
[17]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11922/fi-icl-u2/igt@gem_ctx_cre...@basic-files.html
[18]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/fi-icl-u2/igt@gem_ctx_cre...@basic-files.html
* igt@i915_selftest@live@hangcheck:
- bat-dg1-5: [DMESG-FAIL][19] ([i915#4494] / [i915#4957]) ->
[PASS][20]
[19]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11922/bat-dg1-5/igt@i915_selftest@l...@hangcheck.html
[20]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_106475v1/bat-dg1-5/igt@i915_selftest@l...@hangcheck.html
* igt@i915_selftest@live@hugepages:
- {bat-rpls-1}: [DMESG-WARN][21] ([i915#5278]) -> [PASS][22]
[21]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11922/bat-rpls-1/igt@i915_selftest@l...@hugepages.html
[22]:
https://int
[Intel-gfx] [PATCH v4 6/7] drm/i915: Use error code as -E2BIG when the size of gem ttm object is too large
The ttm_bo_init_reserved() functions returns -ENOSPC if the size is too big to add vma. The direct function that returns -ENOSPC is drm_mm_insert_node_in_range(). To handle the same error as other code returning -E2BIG when the size is too large, it converts return value to -E2BIG. Signed-off-by: Gwan-gyeong Mun Cc: Chris Wilson Cc: Matthew Auld Cc: Thomas Hellström Reviewed-by: Nirmoy Das Reviewed-by: Mauro Carvalho Chehab --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 88f2887627dc..4d478bf325be 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -1249,6 +1249,17 @@ int __i915_gem_ttm_object_init(struct intel_memory_region *mem, ret = ttm_bo_init_reserved(&i915->bdev, i915_gem_to_ttm(obj), bo_type, &i915_sys_placement, page_size >> PAGE_SHIFT, &ctx, NULL, NULL, i915_ttm_bo_destroy); + + /* +* XXX: The ttm_bo_init_reserved() functions returns -ENOSPC if the size +* is too big to add vma. The direct function that returns -ENOSPC is +* drm_mm_insert_node_in_range(). To handle the same error as other code +* that returns -E2BIG when the size is too large, it converts -ENOSPC to +* -E2BIG. +*/ + if (size >> PAGE_SHIFT > INT_MAX && ret == -ENOSPC) + ret = -E2BIG; + if (ret) return i915_ttm_err_to_gem(ret); -- 2.34.1
[Intel-gfx] [PATCH v4 1/7] drm: Move and add a few utility macros into drm util header
It moves overflows_type utility macro into drm util header from i915_utils
header. The overflows_type can be used to catch the truncation between data
types. And it adds safe_conversion() macro which performs a type conversion
(cast) of an source value into a new variable, checking that the
destination is large enough to hold the source value.
And it adds exact_type and exactly_pgoff_t macro to catch type mis-match
while compiling.
v3: Add is_type_unsigned() macro (Mauro)
Modify overflows_type() macro to consider signed data types (Mauro)
Fix the problem that safe_conversion() macro always returns true
v4: Fix kernel-doc markups
Signed-off-by: Gwan-gyeong Mun
Cc: Thomas Hellström
Cc: Matthew Auld
Cc: Nirmoy Das
Cc: Jani Nikula
Reviewed-by: Mauro Carvalho Chehab
---
drivers/gpu/drm/i915/i915_utils.h | 5 +-
include/drm/drm_util.h| 77 +++
2 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_utils.h
b/drivers/gpu/drm/i915/i915_utils.h
index c10d68cdc3ca..345e5b2dc1cd 100644
--- a/drivers/gpu/drm/i915/i915_utils.h
+++ b/drivers/gpu/drm/i915/i915_utils.h
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#ifdef CONFIG_X86
#include
@@ -111,10 +112,6 @@ bool i915_error_injected(void);
#define range_overflows_end_t(type, start, size, max) \
range_overflows_end((type)(start), (type)(size), (type)(max))
-/* Note we don't consider signbits :| */
-#define overflows_type(x, T) \
- (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T))
-
#define ptr_mask_bits(ptr, n) ({ \
unsigned long __v = (unsigned long)(ptr); \
(typeof(ptr))(__v & -BIT(n)); \
diff --git a/include/drm/drm_util.h b/include/drm/drm_util.h
index 79952d8c4bba..1de9ee5704fa 100644
--- a/include/drm/drm_util.h
+++ b/include/drm/drm_util.h
@@ -62,6 +62,83 @@
*/
#define for_each_if(condition) if (!(condition)) {} else
+/**
+ * is_type_unsigned - helper for checking data type which is an unsigned data
+ * type or not
+ * @x: The data type to check
+ *
+ * Returns:
+ * True if the data type is an unsigned data type, false otherwise.
+ */
+#define is_type_unsigned(x) ((typeof(x))-1 >= (typeof(x))0)
+
+/**
+ * overflows_type - helper for checking the truncation between data types
+ * @x: Source for overflow type comparison
+ * @T: Destination for overflow type comparison
+ *
+ * It compares the values and size of each data type between the first and
+ * second argument to check whether truncation can occur when assigning the
+ * first argument to the variable of the second argument.
+ * Source and Destination can be used with or without sign bit.
+ * Composite data structures such as union and structure are not considered.
+ * Enum data types are not considered.
+ * Floating point data types are not considered.
+ *
+ * Returns:
+ * True if truncation can occur, false otherwise.
+ */
+
+#define overflows_type(x, T) \
+ (is_type_unsigned(x) ? \
+ is_type_unsigned(T) ? \
+ (sizeof(x) > sizeof(T) && (x) >> BITS_PER_TYPE(T)) ? 1
: 0 \
+ : (sizeof(x) >= sizeof(T) && (x) >> (BITS_PER_TYPE(T) -
1)) ? 1 : 0 \
+ : is_type_unsigned(T) ? \
+ ((x) < 0) ? 1 : (sizeof(x) > sizeof(T) && (x) >>
BITS_PER_TYPE(T)) ? 1 : 0 \
+ : (sizeof(x) > sizeof(T)) ? \
+ ((x) < 0) ? (((x) * -1) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : ((x) >> BITS_PER_TYPE(T)) ? 1 : 0 \
+ : 0)
+
+/**
+ * exact_type - break compile if source type and destination value's type are
+ * not the same
+ * @T: Source type
+ * @n: Destination value
+ *
+ * It is a helper macro for a poor man's -Wconversion: only allow variables of
+ * an exact type. It determines whether the source type and destination value's
+ * type are the same while compiling, and it breaks compile if two types are
+ * not the same
+ */
+#define exact_type(T, n) \
+ BUILD_BUG_ON(!__builtin_constant_p(n) &&
!__builtin_types_compatible_p(T, typeof(n)))
+
+/**
+ * exactly_pgoff_t - helper to check if the type of a value is pgoff_t
+ * @n: value to compare pgoff_t type
+ *
+ * It breaks compile if the argument value's type is not pgoff_t type.
+ */
+#define exactly_pgoff_t(n) exact_type(pgoff_t, n)
+
+/**
+ * safe_conversion - perform a type conversion (cast) of an source value into
+ * a new variable, checking that the destination is large enough to hold the
+ * source value.
+ * @ptr: Destination pointer address
+ * @value: Source value
+ *
+ * Returns:
+ * If the value would overflow the destination, it returns false.
+ */
+#define safe_conversion(ptr, value) ({ \
+ typeof(value) __v = (value); \
+ typeof(ptr) __ptr = (ptr); \
+ overflows_type(__v, *__ptr) ? 0 : ((*__ptr = (typeof(*__ptr))__v), 1); \
+})
+
/**
* drm_ca
[Intel-gfx] [PATCH v4 5/7] drm/i915: Check if the size is too big while creating shmem file
The __shmem_file_setup() function returns -EINVAL if size is greater than MAX_LFS_FILESIZE. To handle the same error as other code that returns -E2BIG when the size is too large, it add a code that returns -E2BIG when the size is larger than the size that can be handled. v4: If BITS_PER_LONG is 32, size > MAX_LFS_FILESIZE is always false, so it checks only when BITS_PER_LONG is 64. Signed-off-by: Gwan-gyeong Mun Cc: Chris Wilson Cc: Matthew Auld Cc: Thomas Hellström Reviewed-by: Nirmoy Das Reviewed-by: Mauro Carvalho Chehab Reported-by: kernel test robot --- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c index 604e8829e8ea..d41569ca6999 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c @@ -541,6 +541,20 @@ static int __create_shmem(struct drm_i915_private *i915, drm_gem_private_object_init(&i915->drm, obj, size); + /* XXX: The __shmem_file_setup() function returns -EINVAL if size is +* greater than MAX_LFS_FILESIZE. +* To handle the same error as other code that returns -E2BIG when +* the size is too large, we add a code that returns -E2BIG when the +* size is larger than the size that can be handled. +* If BITS_PER_LONG is 32, size > MAX_LFS_FILESIZE is always false, +* so we only needs to check when BITS_PER_LONG is 64. +* If BITS_PER_LONG is 32, E2BIG checks are processed when +* i915_gem_object_size_2big() is called before init_object() callback +* is called. +*/ + if (BITS_PER_LONG == 64 && size > MAX_LFS_FILESIZE) + return -E2BIG; + if (i915->mm.gemfs) filp = shmem_file_setup_with_mnt(i915->mm.gemfs, "i915", size, flags); -- 2.34.1
[Intel-gfx] [PATCH v4 2/7] drm/i915/gem: Typecheck page lookups
From: Chris Wilson
We need to check that we avoid integer overflows when looking up a page,
and so fix all the instances where we have mistakenly used a plain
integer instead of a more suitable long. Be pedantic and add integer
typechecking to the lookup so that we can be sure that we are safe.
And it also uses pgoff_t as our page lookups must remain compatible with
the page cache, pgoff_t is currently exactly unsigned long.
v2: Move added i915_utils's macro into drm_util header (Jani N)
v3: Make not use the same macro name on a function. (Mauro)
For kernel-doc, macros and functions are handled in the same namespace,
the same macro name on a function prevents ever adding documentation
for it.
v4: Add kernel-doc markups to the kAPI functions and macros (Mauoro)
Signed-off-by: Chris Wilson
Signed-off-by: Gwan-gyeong Mun
Cc: Tvrtko Ursulin
Cc: Matthew Auld
Cc: Thomas Hellström
Reviewed-by: Nirmoy Das
Reviewed-by: Mauro Carvalho Chehab
---
drivers/gpu/drm/i915/gem/i915_gem_object.c| 7 +-
drivers/gpu/drm/i915/gem/i915_gem_object.h| 293 --
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 27 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +-
.../drm/i915/gem/selftests/i915_gem_context.c | 12 +-
.../drm/i915/gem/selftests/i915_gem_mman.c| 8 +-
.../drm/i915/gem/selftests/i915_gem_object.c | 8 +-
drivers/gpu/drm/i915/i915_gem.c | 18 +-
drivers/gpu/drm/i915/i915_vma.c | 8 +-
9 files changed, 322 insertions(+), 61 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index ccec4055fde3..90996fe8ad45 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -421,10 +421,11 @@ void __i915_gem_object_invalidate_frontbuffer(struct
drm_i915_gem_object *obj,
static void
i915_gem_object_read_from_page_kmap(struct drm_i915_gem_object *obj, u64
offset, void *dst, int size)
{
+ pgoff_t idx = offset >> PAGE_SHIFT;
void *src_map;
void *src_ptr;
- src_map = kmap_atomic(i915_gem_object_get_page(obj, offset >>
PAGE_SHIFT));
+ src_map = kmap_atomic(i915_gem_object_get_page(obj, idx));
src_ptr = src_map + offset_in_page(offset);
if (!(obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
@@ -437,9 +438,10 @@ i915_gem_object_read_from_page_kmap(struct
drm_i915_gem_object *obj, u64 offset,
static void
i915_gem_object_read_from_page_iomap(struct drm_i915_gem_object *obj, u64
offset, void *dst, int size)
{
+ pgoff_t idx = offset >> PAGE_SHIFT;
+ dma_addr_t dma = i915_gem_object_get_dma_address(obj, idx);
void __iomem *src_map;
void __iomem *src_ptr;
- dma_addr_t dma = i915_gem_object_get_dma_address(obj, offset >>
PAGE_SHIFT);
src_map = io_mapping_map_wc(&obj->mm.region->iomap,
dma - obj->mm.region->region.start,
@@ -468,6 +470,7 @@ i915_gem_object_read_from_page_iomap(struct
drm_i915_gem_object *obj, u64 offset
*/
int i915_gem_object_read_from_page(struct drm_i915_gem_object *obj, u64
offset, void *dst, int size)
{
+ GEM_BUG_ON(overflows_type(offset >> PAGE_SHIFT, pgoff_t));
GEM_BUG_ON(offset >= obj->base.size);
GEM_BUG_ON(offset_in_page(offset) > PAGE_SIZE - size);
GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 6f0a3ce35567..7913f5402f56 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -27,8 +27,10 @@ enum intel_region_id;
* spot such a local variable, please consider fixing!
*
* Aside from our own locals (for which we have no excuse!):
- * - sg_table embeds unsigned int for num_pages
- * - get_user_pages*() mixed ints with longs
+ * - sg_table embeds unsigned int for nents
+ *
+ * We can check for invalidly typed locals with typecheck(), see for example
+ * i915_gem_object_get_sg().
*/
#define GEM_CHECK_SIZE_OVERFLOW(sz) \
GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX)
@@ -363,44 +365,289 @@ i915_gem_object_get_tile_row_size(const struct
drm_i915_gem_object *obj)
int i915_gem_object_set_tiling(struct drm_i915_gem_object *obj,
unsigned int tiling, unsigned int stride);
+/**
+ * __i915_gem_object_page_iter_get_sg - helper to find the target scatterlist
+ * pointer and the target page position using pgoff_t n input argument and
+ * i915_gem_object_page_iter
+ * @obj: i915 GEM buffer object
+ * @iter: i915 GEM buffer object page iterator
+ * @n: page offset
+ * @offset: searched physical offset,
+ * it will be used for returning physical page offset value
+ *
+ * Context: Takes and releases the mutex lock of the i915_gem_object_page_iter.
+ * Takes and releases the RCU lock to search the radix_tree of
+ *
[Intel-gfx] [PATCH v4 3/7] drm/i915: Check for integer truncation on scatterlist creation
From: Chris Wilson
There is an impedance mismatch between the scatterlist API using unsigned
int and our memory/page accounting in unsigned long. That is we may try
to create a scatterlist for a large object that overflows returning a
small table into which we try to fit very many pages. As the object size
is under control of userspace, we have to be prudent and catch the
conversion errors.
To catch the implicit truncation as we switch from unsigned long into the
scatterlist's unsigned int, we use overflows_type check and report
E2BIG prior to the operation. This is already used in our create ioctls to
indicate if the uABI request is simply too large for the backing store.
Failing that type check, we have a second check at sg_alloc_table time
to make sure the values we are passing into the scatterlist API are not
truncated.
It uses pgoff_t for locals that are dealing with page indices, in this
case, the page count is the limit of the page index.
And it uses safe_conversion() macro which performs a type conversion (cast)
of an integer value into a new variable, checking that the destination is
large enough to hold the source value.
v2: Move added i915_utils's macro into drm_util header (Jani N)
Signed-off-by: Chris Wilson
Signed-off-by: Gwan-gyeong Mun
Cc: Tvrtko Ursulin
Cc: Brian Welty
Cc: Matthew Auld
Cc: Thomas Hellström
Reviewed-by: Nirmoy Das
Reviewed-by: Mauro Carvalho Chehab
---
drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 --
drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 ---
drivers/gpu/drm/i915/gem/i915_gem_phys.c | 4
drivers/gpu/drm/i915/gem/i915_gem_shmem.c| 5 -
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 4
drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 -
drivers/gpu/drm/i915/gvt/dmabuf.c| 9 +
drivers/gpu/drm/i915/i915_scatterlist.h | 8
8 files changed, 33 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
index c698f95af15f..ff2e6e780631 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
@@ -37,10 +37,13 @@ static int i915_gem_object_get_pages_internal(struct
drm_i915_gem_object *obj)
struct sg_table *st;
struct scatterlist *sg;
unsigned int sg_page_sizes;
- unsigned int npages;
+ pgoff_t npages; /* restricted by sg_alloc_table */
int max_order;
gfp_t gfp;
+ if (!safe_conversion(&npages, obj->base.size >> PAGE_SHIFT))
+ return -E2BIG;
+
max_order = MAX_ORDER;
#ifdef CONFIG_SWIOTLB
if (is_swiotlb_active(obj->base.dev->dev)) {
@@ -67,7 +70,6 @@ static int i915_gem_object_get_pages_internal(struct
drm_i915_gem_object *obj)
if (!st)
return -ENOMEM;
- npages = obj->base.size / PAGE_SIZE;
if (sg_alloc_table(st, npages, GFP_KERNEL)) {
kfree(st);
return -ENOMEM;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 7913f5402f56..d5f823cc1c2e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -26,9 +26,6 @@ enum intel_region_id;
* this and catch if we ever need to fix it. In the meantime, if you do
* spot such a local variable, please consider fixing!
*
- * Aside from our own locals (for which we have no excuse!):
- * - sg_table embeds unsigned int for nents
- *
* We can check for invalidly typed locals with typecheck(), see for example
* i915_gem_object_get_sg().
*/
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
index 0d0e46dae559..88ba7266a3a5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_phys.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_phys.c
@@ -28,6 +28,10 @@ static int i915_gem_object_get_pages_phys(struct
drm_i915_gem_object *obj)
void *dst;
int i;
+ /* Contiguous chunk, with a single scatterlist element */
+ if (overflows_type(obj->base.size, sg->length))
+ return -E2BIG;
+
if (GEM_WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
return -EINVAL;
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 4eed3dd90ba8..604e8829e8ea 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -193,13 +193,16 @@ static int shmem_get_pages(struct drm_i915_gem_object
*obj)
struct drm_i915_private *i915 = to_i915(obj->base.dev);
struct intel_memory_region *mem = obj->mm.region;
struct address_space *mapping = obj->base.filp->f_mapping;
- const unsigned long page_count = obj->base.size / PAGE_SIZE;
unsigned int max_segment = i915_sg_segment_size();
struct sg_table *st;
struct sgt_iter sgt_iter;
+ pgoff_t page_cou
[Intel-gfx] [PATCH v4 7/7] drm/i915: Remove truncation warning for large objects
From: Chris Wilson
Having addressed the issues surrounding incorrect types for local
variables and potential integer truncation in using the scatterlist API,
we have closed all the loop holes we had previously identified with
dangerously large object creation. As such, we can eliminate the warning
put in place to remind us to complete the review.
Signed-off-by: Chris Wilson
Signed-off-by: Gwan-gyeong Mun
Cc: Tvrtko Ursulin
Cc: Brian Welty
Cc: Matthew Auld
Cc: Thomas Hellström
Testcase: igt@gem_create@create-massive
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4991
Reviewed-by: Nirmoy Das
Reviewed-by: Mauro Carvalho Chehab
---
drivers/gpu/drm/i915/gem/i915_gem_object.h | 15 ---
1 file changed, 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index d5f823cc1c2e..ae97811e8ff9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -20,25 +20,10 @@
enum intel_region_id;
-/*
- * XXX: There is a prevalence of the assumption that we fit the
- * object's page count inside a 32bit _signed_ variable. Let's document
- * this and catch if we ever need to fix it. In the meantime, if you do
- * spot such a local variable, please consider fixing!
- *
- * We can check for invalidly typed locals with typecheck(), see for example
- * i915_gem_object_get_sg().
- */
-#define GEM_CHECK_SIZE_OVERFLOW(sz) \
- GEM_WARN_ON((sz) >> PAGE_SHIFT > INT_MAX)
-
static inline bool i915_gem_object_size_2big(u64 size)
{
struct drm_i915_gem_object *obj;
- if (GEM_CHECK_SIZE_OVERFLOW(size))
- return true;
-
if (overflows_type(size, obj->base.size))
return true;
--
2.34.1
[Intel-gfx] [PATCH v4 4/7] drm/i915: Check for integer truncation on the configuration of ttm place
There is an impedance mismatch between the first/last valid page
frame number of ttm place in unsigned and our memory/page accounting in
unsigned long.
As the object size is under the control of userspace, we have to be prudent
and catch the conversion errors.
To catch the implicit truncation as we switch from unsigned long to
unsigned, we use overflows_type check and report E2BIG or overflow_type
prior to the operation.
v3: Not to change execution inside a macro. (Mauro)
Add safe_conversion_gem_bug_on() macro and remove temporal
SAFE_CONVERSION() macro.
v4: Fix unhandled GEM_BUG_ON() macro call from safe_conversion_gem_bug_on()
Signed-off-by: Gwan-gyeong Mun
Cc: Chris Wilson
Cc: Matthew Auld
Cc: Thomas Hellström
Reviewed-by: Nirmoy Das
Reviewed-by: Mauro Carvalho Chehab
Reported-by: kernel test robot
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 6 +++---
drivers/gpu/drm/i915/i915_gem.h | 4
drivers/gpu/drm/i915/intel_region_ttm.c | 20 +---
3 files changed, 24 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 9f2be1892b6c..88f2887627dc 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -140,14 +140,14 @@ i915_ttm_place_from_region(const struct
intel_memory_region *mr,
if (flags & I915_BO_ALLOC_CONTIGUOUS)
place->flags |= TTM_PL_FLAG_CONTIGUOUS;
if (offset != I915_BO_INVALID_OFFSET) {
- place->fpfn = offset >> PAGE_SHIFT;
- place->lpfn = place->fpfn + (size >> PAGE_SHIFT);
+ safe_conversion_gem_bug_on(&place->fpfn, offset >> PAGE_SHIFT);
+ safe_conversion_gem_bug_on(&place->lpfn, place->fpfn + (size >>
PAGE_SHIFT));
} else if (mr->io_size && mr->io_size < mr->total) {
if (flags & I915_BO_ALLOC_GPU_ONLY) {
place->flags |= TTM_PL_FLAG_TOPDOWN;
} else {
place->fpfn = 0;
- place->lpfn = mr->io_size >> PAGE_SHIFT;
+ safe_conversion_gem_bug_on(&place->lpfn, mr->io_size >>
PAGE_SHIFT);
}
}
}
diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
index 68d8d52bd541..327dacedd5d1 100644
--- a/drivers/gpu/drm/i915/i915_gem.h
+++ b/drivers/gpu/drm/i915/i915_gem.h
@@ -83,5 +83,9 @@ struct drm_i915_private;
#endif
#define I915_GEM_IDLE_TIMEOUT (HZ / 5)
+#define safe_conversion_gem_bug_on(ptr, value) !({ \
+ safe_conversion(ptr, value) ? 0 \
+ : (({ GEM_BUG_ON(overflows_type(value, *ptr)); }), 1); \
+})
#endif /* __I915_GEM_H__ */
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c
b/drivers/gpu/drm/i915/intel_region_ttm.c
index 575d67bc6ffe..f0d143948725 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -209,14 +209,26 @@ intel_region_ttm_resource_alloc(struct
intel_memory_region *mem,
if (flags & I915_BO_ALLOC_CONTIGUOUS)
place.flags |= TTM_PL_FLAG_CONTIGUOUS;
if (offset != I915_BO_INVALID_OFFSET) {
- place.fpfn = offset >> PAGE_SHIFT;
- place.lpfn = place.fpfn + (size >> PAGE_SHIFT);
+ if (!safe_conversion_gem_bug_on(&place.fpfn,
+ offset >> PAGE_SHIFT)) {
+ ret = -E2BIG;
+ goto out;
+ }
+ if (!safe_conversion_gem_bug_on(&place.lpfn,
+ place.fpfn + (size >>
PAGE_SHIFT))) {
+ ret = -E2BIG;
+ goto out;
+ }
} else if (mem->io_size && mem->io_size < mem->total) {
if (flags & I915_BO_ALLOC_GPU_ONLY) {
place.flags |= TTM_PL_FLAG_TOPDOWN;
} else {
place.fpfn = 0;
- place.lpfn = mem->io_size >> PAGE_SHIFT;
+ if (!safe_conversion_gem_bug_on(&place.lpfn,
+ mem->io_size >>
PAGE_SHIFT)) {
+ ret = -E2BIG;
+ goto out;
+ }
}
}
@@ -224,6 +236,8 @@ intel_region_ttm_resource_alloc(struct intel_memory_region
*mem,
mock_bo.bdev = &mem->i915->bdev;
ret = man->func->alloc(man, &mock_bo, &place, &res);
+
+out:
if (ret == -ENOSPC)
ret = -ENXIO;
if (!ret)
--
2.34.1
[Intel-gfx] [PATCH v4 0/7] Fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation
This patch series fixes integer overflow or integer truncation issues in page lookups, ttm place configuration and scatterlist creation, etc. We need to check that we avoid integer overflows when looking up a page, and so fix all the instances where we have mistakenly used a plain integer instead of a more suitable long. And there is an impedance mismatch between the scatterlist API using unsigned int and our memory/page accounting in unsigned long. That is we may try to create a scatterlist for a large object that overflows returning a small table into which we try to fit very many pages. As the object size is under the control of userspace, we have to be prudent and catch the conversion errors. To catch the implicit truncation as we switch from unsigned long into the scatterlist's unsigned int, we use our overflows_type check and report E2BIG prior to the operation. This is already used in our create ioctls to indicate if the uABI request is simply too large for the backing store. And ttm place also has the same problem with scatterlist creation, and we fix the integer truncation problem with the way approached by scatterlist creation. And It corrects the error code to return -E2BIG when creating gem objects using ttm or shmem, if the size is too large in each case. In order to provide a common macro, it moves and adds a few utility macros into drm util header v3: Modify overflows_type() macro to consider signed data types and add is_type_unsigned() macro (Mauro) Make not use the same macro name on a function. (Mauro) For kernel-doc, macros and functions are handled in the same namespace, the same macro name on a function prevents ever adding documentation for it. Not to change execution inside a macro. (Mauro) Fix the problem that safe_conversion() macro always returns true (G.G) Add safe_conversion_gem_bug_on() macro and remove temporal SAFE_CONVERSION() macro. (G.G.) v4: Fix build warnins that reported by kernel test robot. (kernel test robot ) Add kernel-doc markups to the kAPI functions and macros (Mauoro) Testcase: igt@gem_create@create-massive Testcase: igt@gem_userptr_blits@input-checking Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/4991 Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5411 Cc: Mauro Carvalho Chehab Cc: Chris Wilson Cc: Matthew Auld Cc: Thomas Hellström Cc: Nirmoy Das Cc: Jani Nikula Cc: David Airlie Cc: Daniel Vetter Chris Wilson (3): drm/i915/gem: Typecheck page lookups drm/i915: Check for integer truncation on scatterlist creation drm/i915: Remove truncation warning for large objects Gwan-gyeong Mun (4): drm: Move and add a few utility macros into drm util header drm/i915: Check for integer truncation on the configuration of ttm place drm/i915: Check if the size is too big while creating shmem file drm/i915: Use error code as -E2BIG when the size of gem ttm object is too large drivers/gpu/drm/i915/gem/i915_gem_internal.c | 6 +- drivers/gpu/drm/i915/gem/i915_gem_object.c| 7 +- drivers/gpu/drm/i915/gem/i915_gem_object.h| 303 +++--- drivers/gpu/drm/i915/gem/i915_gem_pages.c | 27 +- drivers/gpu/drm/i915/gem/i915_gem_phys.c | 4 + drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 19 +- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 23 +- drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 5 +- .../drm/i915/gem/selftests/i915_gem_context.c | 12 +- .../drm/i915/gem/selftests/i915_gem_mman.c| 8 +- .../drm/i915/gem/selftests/i915_gem_object.c | 8 +- drivers/gpu/drm/i915/gvt/dmabuf.c | 9 +- drivers/gpu/drm/i915/i915_gem.c | 18 +- drivers/gpu/drm/i915/i915_gem.h | 4 + drivers/gpu/drm/i915/i915_scatterlist.h | 8 + drivers/gpu/drm/i915/i915_utils.h | 5 +- drivers/gpu/drm/i915/i915_vma.c | 8 +- drivers/gpu/drm/i915/intel_region_ttm.c | 20 +- include/drm/drm_util.h| 77 + 19 files changed, 478 insertions(+), 93 deletions(-) -- 2.34.1
Re: [Intel-gfx] [PATCH 12/12] drm/i915: Parse DP/eDP max lane count from VBT
On Fri, Jul 15, 2022 at 11:20:44PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Limit the DP lane count based on the new VBT DP/eDP max
> lane count field.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 16
> drivers/gpu/drm/i915/display/intel_bios.h | 1 +
> drivers/gpu/drm/i915/display/intel_dp.c | 13 -
> 3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index cd86b65055ef..d8063c329b3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -2489,6 +2489,14 @@ static int _intel_bios_dp_max_link_rate(const struct
> intel_bios_encoder_data *de
> return
> parse_bdb_216_dp_max_link_rate(devdata->child.dp_max_link_rate);
> }
>
> +static int _intel_bios_dp_max_lane_count(const struct
> intel_bios_encoder_data *devdata)
> +{
> + if (!devdata || devdata->i915->vbt.version < 244)
> + return 0;
> +
> + return devdata->child.dp_max_lane_count + 1;
> +}
> +
> static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
>enum port port)
> {
> @@ -3674,6 +3682,14 @@ int intel_bios_dp_max_link_rate(struct intel_encoder
> *encoder)
> return _intel_bios_dp_max_link_rate(devdata);
> }
>
> +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder)
> +{
> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> + const struct intel_bios_encoder_data *devdata =
> i915->vbt.ports[encoder->port];
> +
> + return _intel_bios_dp_max_lane_count(devdata);
> +}
do we really need 2 functions here since this one is small and we don't have any
bit switches and all?!
or do you plan to reuse this anywhere else later?
> +
> int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder)
> {
> struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.h
> b/drivers/gpu/drm/i915/display/intel_bios.h
> index e47582b0de0a..e375405a7828 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -258,6 +258,7 @@ bool intel_bios_get_dsc_params(struct intel_encoder
> *encoder,
> int intel_bios_max_tmds_clock(struct intel_encoder *encoder);
> int intel_bios_hdmi_level_shift(struct intel_encoder *encoder);
> int intel_bios_dp_max_link_rate(struct intel_encoder *encoder);
> +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder);
> int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder);
> bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, enum
> port port);
> bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum port
> port);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 32292c0be2bd..0370c4c105dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -286,11 +286,22 @@ static int intel_dp_max_common_rate(struct intel_dp
> *intel_dp)
> return intel_dp_common_rate(intel_dp, intel_dp->num_common_rates - 1);
> }
>
> +static int intel_dp_max_source_lane_count(struct intel_digital_port
> *dig_port)
> +{
> + int vbt_max_lanes = intel_bios_dp_max_lane_count(&dig_port->base);
> + int max_lanes = dig_port->max_lanes;
> +
> + if (vbt_max_lanes)
> + max_lanes = min(max_lanes, vbt_max_lanes);
> +
> + return max_lanes;
> +}
> +
> /* Theoretical max between source and sink */
> static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
> {
> struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> - int source_max = dig_port->max_lanes;
> + int source_max = intel_dp_max_source_lane_count(dig_port);
> int sink_max = intel_dp->max_sink_lane_count;
> int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
> int lttpr_max =
> drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps);
> --
> 2.35.1
>
Re: [Intel-gfx] [PATCH 11/12] drm/i915: WARN if a port should use VBT provided vswing tables
On Fri, Jul 15, 2022 at 11:20:43PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> We don't parse the VBT vswing/preemphassis tables at all currently.
> Let's WARN if a port wants to use them so we get a heads up that
> whether we really need to implement this stuff or not. My
> current stash contains no VBTs with this bit set.
let's unlock a new can of worms?! :)
I believe this deserves a /* XXX: comment with the code in case
someone else finds this warns first and doesn't use the git blame
anyways
Reviewed-by: Rodrigo Vivi
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_bios.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index 51dde5bfd956..cd86b65055ef 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -2661,6 +2661,10 @@ static void parse_ddi_port(struct
> intel_bios_encoder_data *devdata)
> return;
> }
>
> + drm_WARN(&i915->drm, child->use_vbt_vswing,
> + "Port %c asks to use VBT vswing/preemph tables\n",
> + port_name(port));
> +
> if (i915->vbt.ports[port]) {
> drm_dbg_kms(&i915->drm,
> "More than one child device for port %c in VBT,
> using the first.\n",
> --
> 2.35.1
>
Re: [Intel-gfx] [PATCH 01/12] drm/i915: Unify VBT version number comments
On Fri, Jul 15, 2022 at 11:20:33PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Use a more standard form for the VT version number comments.
>
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_vbt_defs.h | 206 ++
> 1 file changed, 110 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> index 509b0a419c20..ba328d130991 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -75,6 +75,20 @@ struct bdb_header {
> u16 bdb_size;
> } __packed;
>
> +/*
> + * BDB version number dependencies are documented as:
> + *
> + * +
> + *indicates the field was introduced in version
> + *and is still valid
> + *
> + * -
> + *indicates the field was introduced in version
> + *and obsoleted in version +1.
> + *
> + * ??? indicates the specific version number is unknown
> + */
> +
> /*
> * There are several types of BIOS data blocks (BDBs), each block has
> * an ID and size in the first 3 bytes (ID in first, size in next 2).
> @@ -144,12 +158,12 @@ struct bdb_general_features {
> /* bits 3 */
> u8 disable_smooth_vision:1;
> u8 single_dvi:1;
> - u8 rotate_180:1;/* 181 */
> + u8 rotate_180:1;/* 181+ */
> u8 fdi_rx_polarity_inverted:1;
> - u8 vbios_extended_mode:1; /* 160 */
> - u8 copy_ilfp_dtd_to_sdvo_lvds_dtd:1;/* 160 */
> - u8 panel_best_fit_timing:1; /* 160 */
> - u8 ignore_strap_state:1;/* 160 */
> + u8 vbios_extended_mode:1; /* 160+ */
> + u8 copy_ilfp_dtd_to_sdvo_lvds_dtd:1;/* 160+ */
> + u8 panel_best_fit_timing:1; /* 160+ */
> + u8 ignore_strap_state:1;/* 160+ */
>
> /* bits 4 */
> u8 legacy_monitor_detect;
> @@ -164,11 +178,11 @@ struct bdb_general_features {
> u8 rsvd11:2; /* finish byte */
>
> /* bits 6 */
> - u8 tc_hpd_retry_timeout:7; /* 242 */
> + u8 tc_hpd_retry_timeout:7; /* 242+ */
> u8 rsvd12:1;
>
> /* bits 7 */
> - u8 afc_startup_config:2;/* 249 */
> + u8 afc_startup_config:2;/* 249+ */
> u8 rsvd13:6;
> } __packed;
>
> @@ -275,27 +289,27 @@ struct bdb_general_features {
> #define DVO_PORT_DPC 8
> #define DVO_PORT_DPD 9
> #define DVO_PORT_DPA 10
> -#define DVO_PORT_DPE 11 /* 193 */
> -#define DVO_PORT_HDMIE 12 /* 193
> */
> +#define DVO_PORT_DPE 11 /* 193+ */
> +#define DVO_PORT_HDMIE 12 /* 193+
> */
> #define DVO_PORT_DPF 13 /* N/A */
> #define DVO_PORT_HDMIF 14 /* N/A
> */
> -#define DVO_PORT_DPG 15 /* 217 */
> -#define DVO_PORT_HDMIG 16 /* 217
> */
> -#define DVO_PORT_DPH 17 /* 217 */
> -#define DVO_PORT_HDMIH 18 /* 217
> */
> -#define DVO_PORT_DPI 19 /* 217 */
> -#define DVO_PORT_HDMII 20 /* 217
> */
> -#define DVO_PORT_MIPIA 21 /* 171
> */
> -#define DVO_PORT_MIPIB 22 /* 171
> */
> -#define DVO_PORT_MIPIC 23 /* 171
> */
> -#define DVO_PORT_MIPID 24 /* 171
> */
> +#define DVO_PORT_DPG 15 /* 217+ */
> +#define DVO_PORT_HDMIG 16 /* 217+
> */
> +#define DVO_PORT_DPH 17 /* 217+ */
> +#define DVO_PORT_HDMIH 18 /* 217+
> */
> +#define DVO_PORT_DPI 19 /* 217+ */
> +#define DVO_PORT_HDMII 20 /* 217+
> */
> +#define DVO_PORT_MIPIA 21 /* 171+
> */
> +#define DVO_PORT_MIPIB 22 /* 171+
> */
> +#define DVO_PORT_MIPIC 23 /* 171+
> */
> +#define DVO_PORT_MIPID 24 /* 171+
> */
>
> -#define HDMI_MAX_DATA_RATE_PLATFORM 0 /* 204 */
> -#define HDMI_MAX_DATA_RATE_297 1
Re: [Intel-gfx] [PATCH 0/2] drm/i915/gt: Expose per gt defaults in sysfs
On Mon, Jul 18, 2022 at 06:07:06PM -0700, Ashutosh Dixit wrote: > Create a gt/gtN/.defaults/ directory (similar to > engine//.defaults/) to expose default parameter values for each > gt in sysfs. This allows userspace to restore default parameter values > after they may have changed. > > Patch 1: Creates the gt/gtN/.defaults/ directory > Patch 2: Adds per-gt RPS defaults (rps_max_freq_mhz and rps_min_freq_mhz) >to gt/gtN/.defaults/ > > An approved Level-0/oneAPI UMD pull request which consumes the exposed > defaults can be seen here: > https://github.com/intel/compute-runtime/pull/552 > The UMD pull request will be merged if/after this series is merged to i915. Pushed to drm-intel-gt-next. Thanks for the patches. > > Previous discussion on these patches can be seen here: > https://patchwork.freedesktop.org/patch/484238/?series=102665&rev=4 > https://patchwork.freedesktop.org/patch/483988/?series=102665&rev=3 > > Cc: Matt Roper > Cc: Tvrtko Ursulin > Cc: Andi Shyti > Signed-off-by: Ashutosh Dixit > > Ashutosh Dixit (2): > drm/i915/gt: Create gt/gtN/.defaults/ for per gt sysfs defaults > drm/i915/gt: Expose per-gt RPS defaults in sysfs > > drivers/gpu/drm/i915/gt/intel_gt_sysfs.c| 10 +++--- > drivers/gpu/drm/i915/gt/intel_gt_sysfs.h| 6 > drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 34 + > drivers/gpu/drm/i915/gt/intel_gt_types.h| 9 ++ > drivers/gpu/drm/i915/gt/intel_rps.c | 2 ++ > drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 12 +--- > 6 files changed, 64 insertions(+), 9 deletions(-) > > -- > 2.34.1 >
Re: [Intel-gfx] [PATCH 02/12] drm/i915/guc: Don't call ring_is_idle in GuC submission
On 19/07/2022 10:49, Tvrtko Ursulin wrote:
On 19/07/2022 01:09, John Harrison wrote:
On 7/18/2022 05:26, Tvrtko Ursulin wrote:
On 13/07/2022 00:31, john.c.harri...@intel.com wrote:
From: Matthew Brost
The engine registers really shouldn't be touched during GuC submission
as the GuC owns the registers. Don't call ring_is_idle and tie
Touch being just read and it is somehow harmful?
The registers are meaningless when GuC is controlling the submission.
The i915 driver has no knowledge of what context is or is not
executing on any given engine at any given time. So reading reading
the ring registers is incorrect - it can lead to bad assumptions about
what state the hardware is in.
Same is actually true with the execlists backend. The code in
ring_is_idle is not concerning itself with which context is running or
not. Just that the head/tail/ctl appear idle.
Problem/motivation appears to be on a higher than simply ring registers.
I am not claiming it makes sense with Guc and that it has to remain but
just suggesting for as a minimum clearer commit message.
intel_engine_is_idle strictly to the engine pm.
Strictly seems wrong - it is just ring_is_idle check that is replaced
and not the whole implementation of intel_engine_is_idle.
Because intel_engine_is_idle tied to the engine pm, retire requests
before checking intel_engines_are_idle in gt_drop_caches, and lastly
Why is re-ordering important? I at least can't understand it. I hope
it's not working around IGT failures.
If requests are physically completed but not retired then they will be
holding unnecessary PM references. So we need to flush those out
before checking for idle.
And if they are not as someone passes in DROP_RESET_ACTIVE? They will
not retire and code still enters intel_engines_are_idle so that has to
work, no? Something does not align for me still.
With "not retire" I meant potentially not retire within
I915_IDLE_ENGINES_TIMEOUT. I guess hack happens to work for some or all
IGTs which use DROP_RESET_ACTIVE.
Does it also mean patch would fix that problem without touching
intel_engine_is_idle/ring_is_idle - with just the re-ordering in
gt_drop_caches?
Regards,
Tvrtko
increase the timeout in gt_drop_caches for the intel_engines_are_idle
check.
Same here - why?
@Matthew Brost - do you recall which particular tests were hitting an
issue? I'm guessing gem_ctx_create? I believe that's the one that
creates and destroys thousands of contexts. That is much slower with
GuC (GuC communication required) than with execlists (i915 internal
state change only).
And if that is a logically separate change please split the patch up.
Regards,
Tvrtko
John.
Regards,
Tvrtko
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 +
drivers/gpu/drm/i915/i915_debugfs.c | 6 +++---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 283870c659911..959a7c92e8f4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1602,6 +1602,9 @@ static bool ring_is_idle(struct
intel_engine_cs *engine)
{
bool idle = true;
+ /* GuC submission shouldn't access HEAD & TAIL via MMIO */
+ GEM_BUG_ON(intel_engine_uses_guc(engine));
+
if (I915_SELFTEST_ONLY(!engine->mmio_base))
return true;
@@ -1668,6 +1671,16 @@ bool intel_engine_is_idle(struct
intel_engine_cs *engine)
if (!i915_sched_engine_is_empty(engine->sched_engine))
return false;
+ /*
+ * We shouldn't touch engine registers with GuC submission as
the GuC
+ * owns the registers. Let's tie the idle to engine pm, at
worst this
+ * function sometimes will falsely report non-idle when idle
during the
+ * delay to retire requests or with virtual engines and a request
+ * running on another instance within the same class / submit
mask.
+ */
+ if (intel_engine_uses_guc(engine))
+ return false;
+
/* Ring stopped? */
return ring_is_idle(engine);
}
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 94e5c29d2ee3a..ee5334840e9cb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -654,13 +654,13 @@ gt_drop_caches(struct intel_gt *gt, u64 val)
{
int ret;
+ if (val & DROP_RETIRE || val & DROP_RESET_ACTIVE)
+ intel_gt_retire_requests(gt);
+
if (val & DROP_RESET_ACTIVE &&
wait_for(intel_engines_are_idle(gt),
I915_IDLE_ENGINES_TIMEOUT))
intel_gt_set_wedged(gt);
- if (val & DROP_RETIRE)
- intel_gt_retire_requests(gt);
-
if (val & (DROP_IDLE | DROP_ACTIVE)) {
ret = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
if (ret)
diff --git a/drivers/gpu/
[Intel-gfx] [PATCH v3 3/3] drm/i915: Use luminance range calculated during edid parsing
Instead of using fixed 0 - 512 range use luminance range calculated
as a part of edid parsing. As a backup fall back to static 0 - 512.
v3: Clean-ups suggested by Jani Nikula
v2: Use values calculated during edid parsing
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
---
.../gpu/drm/i915/display/intel_dp_aux_backlight.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index c92d5bb2326a..83af95bce98d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -278,6 +278,8 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector
*connector, enum pipe pi
{
struct drm_i915_private *i915 = to_i915(connector->base.dev);
struct intel_panel *panel = &connector->panel;
+ struct drm_luminance_range_info *luminance_range =
+ &connector->base.display_info.luminance_range;
int ret;
if (panel->backlight.edp.intel.sdr_uses_aux) {
@@ -293,8 +295,17 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector
*connector, enum pipe pi
}
}
- panel->backlight.max = 512;
- panel->backlight.min = 0;
+ if (luminance_range->max_luminance) {
+ panel->backlight.max = luminance_range->max_luminance;
+ panel->backlight.min = luminance_range->min_luminance;
+ } else {
+ panel->backlight.max = 512;
+ panel->backlight.min = 0;
+ }
+
+ drm_dbg_kms(&i915->drm, "Using backlight range %d..%d\n",
panel->backlight.min,
+ panel->backlight.max);
+
panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector,
pipe);
panel->backlight.enabled = panel->backlight.level != 0;
--
2.25.1
[Intel-gfx] [PATCH v3 2/3] drm/amdgpu_dm: Rely on split out luminance calculation function
Luminance range calculation was split out into drm_edid.c and is now
part of edid parsing. Rely on values calculated during edid parsing and
use these for caps->aux_max_input_signal and caps->aux_min_input_signal.
v2: Use values calculated during edid parsing
Cc: Roman Li
Cc: Rodrigo Siqueira
Cc: Harry Wentland
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 +++
1 file changed, 4 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 3e83fed540e8..eb7abdeb8653 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2903,15 +2903,12 @@ static struct drm_mode_config_helper_funcs
amdgpu_dm_mode_config_helperfuncs = {
static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
{
- u32 max_avg, min_cll, max, min, q, r;
struct amdgpu_dm_backlight_caps *caps;
struct amdgpu_display_manager *dm;
struct drm_connector *conn_base;
struct amdgpu_device *adev;
struct dc_link *link = NULL;
- static const u8 pre_computed_values[] = {
- 50, 51, 52, 53, 55, 56, 57, 58, 59, 61, 62, 63, 65, 66, 68, 69,
- 71, 72, 74, 75, 77, 79, 81, 82, 84, 86, 88, 90, 92, 94, 96, 98};
+ struct drm_luminance_range_info *luminance_range;
int i;
if (!aconnector || !aconnector->dc_link)
@@ -2933,8 +2930,6 @@ static void update_connector_ext_caps(struct
amdgpu_dm_connector *aconnector)
caps = &dm->backlight_caps[i];
caps->ext_caps = &aconnector->dc_link->dpcd_sink_ext_caps;
caps->aux_support = false;
- max_avg = conn_base->hdr_sink_metadata.hdmi_type1.max_fall;
- min_cll = conn_base->hdr_sink_metadata.hdmi_type1.min_cll;
if (caps->ext_caps->bits.oled == 1 /*||
caps->ext_caps->bits.sdr_aux_backlight_control == 1 ||
@@ -2946,31 +2941,9 @@ static void update_connector_ext_caps(struct
amdgpu_dm_connector *aconnector)
else if (amdgpu_backlight == 1)
caps->aux_support = true;
- /* From the specification (CTA-861-G), for calculating the maximum
-* luminance we need to use:
-* Luminance = 50*2**(CV/32)
-* Where CV is a one-byte value.
-* For calculating this expression we may need float point precision;
-* to avoid this complexity level, we take advantage that CV is divided
-* by a constant. From the Euclids division algorithm, we know that CV
-* can be written as: CV = 32*q + r. Next, we replace CV in the
-* Luminance expression and get 50*(2**q)*(2**(r/32)), hence we just
-* need to pre-compute the value of r/32. For pre-computing the values
-* We just used the following Ruby line:
-* (0...32).each {|cv| puts (50*2**(cv/32.0)).round}
-* The results of the above expressions can be verified at
-* pre_computed_values.
-*/
- q = max_avg >> 5;
- r = max_avg % 32;
- max = (1 << q) * pre_computed_values[r];
-
- // min luminance: maxLum * (CV/255)^2 / 100
- q = DIV_ROUND_CLOSEST(min_cll, 255);
- min = max * DIV_ROUND_CLOSEST((q * q), 100);
-
- caps->aux_max_input_signal = max;
- caps->aux_min_input_signal = min;
+ luminance_range = &conn_base->display_info.luminance_range;
+ caps->aux_min_input_signal = luminance_range->min_luminance;
+ caps->aux_max_input_signal = luminance_range->max_luminance;
}
void amdgpu_dm_update_connector_after_detect(
--
2.25.1
[Intel-gfx] [PATCH v3 1/3] drm: New function to get luminance range based on static hdr metadata
Split luminance min/max calculation using static hdr metadata from
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:update_connector_ext_caps
into drm/drm_edid.c and use it during edid parsing. Calculated range is
stored into connector->display_info->luminance_range.
Add new data structure (drm_luminance_range_inf) to store luminance range
calculated using data from EDID's static hdr metadata block. Add this new
struct as a part of drm_display_info struct.
v3: Squashed adding drm_luminance_range_info patch here
v2: Calculate range during edid parsing
Cc: Roman Li
Cc: Rodrigo Siqueira
Cc: Harry Wentland
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
Acked-by: Jani Nikula
---
drivers/gpu/drm/drm_edid.c | 52 -
include/drm/drm_connector.h | 21 +++
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index bbc25e3b7220..90a5e26eafa8 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5165,6 +5165,51 @@ static void fixup_detailed_cea_mode_clock(struct
drm_display_mode *mode)
mode->clock = clock;
}
+static void drm_calculate_luminance_range(struct drm_connector *connector)
+{
+ struct hdr_static_metadata *hdr_metadata =
&connector->hdr_sink_metadata.hdmi_type1;
+ struct drm_luminance_range_info *luminance_range =
+ &connector->display_info.luminance_range;
+ static const u8 pre_computed_values[] = {
+ 50, 51, 52, 53, 55, 56, 57, 58, 59, 61, 62, 63, 65, 66, 68, 69,
+ 71, 72, 74, 75, 77, 79, 81, 82, 84, 86, 88, 90, 92, 94, 96, 98
+ };
+ u32 max_avg, min_cll, max, min, q, r;
+
+ if (!(hdr_metadata->metadata_type & BIT(HDMI_STATIC_METADATA_TYPE1)))
+ return;
+
+ max_avg = hdr_metadata->max_fall;
+ min_cll = hdr_metadata->min_cll;
+
+ /*
+* From the specification (CTA-861-G), for calculating the maximum
+* luminance we need to use:
+* Luminance = 50*2**(CV/32)
+* Where CV is a one-byte value.
+* For calculating this expression we may need float point precision;
+* to avoid this complexity level, we take advantage that CV is divided
+* by a constant. From the Euclids division algorithm, we know that CV
+* can be written as: CV = 32*q + r. Next, we replace CV in the
+* Luminance expression and get 50*(2**q)*(2**(r/32)), hence we just
+* need to pre-compute the value of r/32. For pre-computing the values
+* We just used the following Ruby line:
+* (0...32).each {|cv| puts (50*2**(cv/32.0)).round}
+* The results of the above expressions can be verified at
+* pre_computed_values.
+*/
+ q = max_avg >> 5;
+ r = max_avg % 32;
+ max = (1 << q) * pre_computed_values[r];
+
+ /* min luminance: maxLum * (CV/255)^2 / 100 */
+ q = DIV_ROUND_CLOSEST(min_cll, 255);
+ min = max * DIV_ROUND_CLOSEST((q * q), 100);
+
+ luminance_range->min_luminance = min;
+ luminance_range->max_luminance = max;
+}
+
static uint8_t eotf_supported(const u8 *edid_ext)
{
return edid_ext[2] &
@@ -5196,8 +5241,12 @@ drm_parse_hdr_metadata_block(struct drm_connector
*connector, const u8 *db)
connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
if (len >= 5)
connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
- if (len >= 6)
+ if (len >= 6) {
connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
+
+ /* Calculate only when all values are available */
+ drm_calculate_luminance_range(connector);
+ }
}
static void
@@ -6101,6 +6150,7 @@ static void drm_reset_display_info(struct drm_connector
*connector)
info->non_desktop = 0;
memset(&info->monitor_range, 0, sizeof(info->monitor_range));
+ memset(&info->luminance_range, 0, sizeof(info->luminance_range));
info->mso_stream_count = 0;
info->mso_pixel_overlap = 0;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 2c6fa746efac..248206bbd975 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -323,6 +323,22 @@ struct drm_monitor_range_info {
u8 max_vfreq;
};
+/**
+ * struct drm_luminance_range_info - Panel's luminance range for
+ * &drm_display_info. Calculated using data in EDID
+ *
+ * This struct is used to store a luminance range supported by panel
+ * as calculated using data from EDID's static hdr metadata.
+ *
+ * @min_luminance: This is the min supported luminance value
+ *
+ * @max_luminance: This is the max supported luminance value
+ */
+struct drm_luminance_range_info {
+ u32 min_luminance;
+ u32 max_luminance;
+};
+
/**
* enum drm_privacy_screen_status - privacy screen
[Intel-gfx] [PATCH v3 0/3] HDR aux backlight range calculation
This patch set splits out static hdr metadata backlight range parsing from gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c into gpu/drm/drm-edid.c as a new function. This new function is then used during edid parsing when HDR static metadata block parsing. Calculated values are stored in a new struct drm_luminance_range introduced into display_info. Amdgpu_dm.c and intel_dp_aux_backlight.c are using this new data. v3: Some clean-ups suggested by Jani Nikula v2: Calculate the range during edid parsing and store into display_info Cc: Roman Li Cc: Maarten Lankhorst Cc: Rodrigo Siqueira Cc: Harry Wentland Cc: Lyude Paul Cc: Mika Kahola Cc: Jani Nikula Cc: Manasi Navare Jouni Högander (3): drm: New function to get luminance range based on static hdr metadata drm/amdgpu_dm: Rely on split out luminance calculation function drm/i915: Use luminance range calculated during edid parsing .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++--- drivers/gpu/drm/drm_edid.c| 52 ++- .../drm/i915/display/intel_dp_aux_backlight.c | 15 +- include/drm/drm_connector.h | 21 4 files changed, 89 insertions(+), 34 deletions(-) -- 2.25.1
Re: [Intel-gfx] [PATCH 10/12] drm/i915/guc: Support larger contexts on newer hardware
On 19/07/2022 01:13, John Harrison wrote:
On 7/18/2022 05:35, Tvrtko Ursulin wrote:
On 13/07/2022 00:31, john.c.harri...@intel.com wrote:
From: Matthew Brost
The GuC needs a copy of a golden context for implementing watchdog
resets (aka media resets). This context is larger on newer platforms.
So adjust the size being allocated/copied accordingly.
What were the consequences of this being too small? Media watchdog
reset broken impacting userspace? Platforms? Do we have an IGT
testcase? Do we need a Fixes: tag? Copy stable?
Yes. Not sure if we have an IGT for the media watchdog. I recall writing
something a long time back but I don't think it ever got merged due to
push back that I don't recall right now. And no because it only affects
DG2 onwards which is still forceprobed.
Right, hm, I don't know if the MBD SKU promise for DG2 relies on force
probe removal or not. My impression certainly was that a bunch of uapi
we recently merged made people happy in that respect - that we satisfied
the commit to deliver that support with 5.19. Maybe I am wrong, or
perhaps to err on the side of safety you could add the right Fixes: tag
regardless? Pick some patch which enables GuC for DG2 if there isn't
anything better I guess. Or you could check with James.
Regards,
Tvrtko
John.
Regards,
Tvrtko
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
index ba7541f3ca610..74cbe8eaf5318 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c
@@ -464,7 +464,11 @@ static void fill_engine_enable_masks(struct
intel_gt *gt,
}
#define LR_HW_CONTEXT_SIZE (80 * sizeof(u32))
-#define LRC_SKIP_SIZE (LRC_PPHWSP_SZ * PAGE_SIZE + LR_HW_CONTEXT_SIZE)
+#define XEHP_LR_HW_CONTEXT_SIZE (96 * sizeof(u32))
+#define LR_HW_CONTEXT_SZ(i915) (GRAPHICS_VER_FULL(i915) >=
IP_VER(12, 50) ? \
+ XEHP_LR_HW_CONTEXT_SIZE : \
+ LR_HW_CONTEXT_SIZE)
+#define LRC_SKIP_SIZE(i915) (LRC_PPHWSP_SZ * PAGE_SIZE +
LR_HW_CONTEXT_SZ(i915))
static int guc_prep_golden_context(struct intel_guc *guc)
{
struct intel_gt *gt = guc_to_gt(guc);
@@ -525,7 +529,7 @@ static int guc_prep_golden_context(struct
intel_guc *guc)
* on all engines).
*/
ads_blob_write(guc, ads.eng_state_size[guc_class],
- real_size - LRC_SKIP_SIZE);
+ real_size - LRC_SKIP_SIZE(gt->i915));
ads_blob_write(guc, ads.golden_context_lrca[guc_class],
addr_ggtt);
@@ -599,7 +603,7 @@ static void guc_init_golden_context(struct
intel_guc *guc)
}
GEM_BUG_ON(ads_blob_read(guc,
ads.eng_state_size[guc_class]) !=
- real_size - LRC_SKIP_SIZE);
+ real_size - LRC_SKIP_SIZE(gt->i915));
GEM_BUG_ON(ads_blob_read(guc,
ads.golden_context_lrca[guc_class]) != addr_ggtt);
addr_ggtt += alloc_size;
Re: [Intel-gfx] [PATCH 02/12] drm/i915/guc: Don't call ring_is_idle in GuC submission
On 19/07/2022 01:09, John Harrison wrote:
On 7/18/2022 05:26, Tvrtko Ursulin wrote:
On 13/07/2022 00:31, john.c.harri...@intel.com wrote:
From: Matthew Brost
The engine registers really shouldn't be touched during GuC submission
as the GuC owns the registers. Don't call ring_is_idle and tie
Touch being just read and it is somehow harmful?
The registers are meaningless when GuC is controlling the submission.
The i915 driver has no knowledge of what context is or is not executing
on any given engine at any given time. So reading reading the ring
registers is incorrect - it can lead to bad assumptions about what state
the hardware is in.
Same is actually true with the execlists backend. The code in
ring_is_idle is not concerning itself with which context is running or
not. Just that the head/tail/ctl appear idle.
Problem/motivation appears to be on a higher than simply ring registers.
I am not claiming it makes sense with Guc and that it has to remain but
just suggesting for as a minimum clearer commit message.
intel_engine_is_idle strictly to the engine pm.
Strictly seems wrong - it is just ring_is_idle check that is replaced
and not the whole implementation of intel_engine_is_idle.
Because intel_engine_is_idle tied to the engine pm, retire requests
before checking intel_engines_are_idle in gt_drop_caches, and lastly
Why is re-ordering important? I at least can't understand it. I hope
it's not working around IGT failures.
If requests are physically completed but not retired then they will be
holding unnecessary PM references. So we need to flush those out before
checking for idle.
And if they are not as someone passes in DROP_RESET_ACTIVE? They will
not retire and code still enters intel_engines_are_idle so that has to
work, no? Something does not align for me still.
increase the timeout in gt_drop_caches for the intel_engines_are_idle
check.
Same here - why?
@Matthew Brost - do you recall which particular tests were hitting an
issue? I'm guessing gem_ctx_create? I believe that's the one that
creates and destroys thousands of contexts. That is much slower with GuC
(GuC communication required) than with execlists (i915 internal state
change only).
And if that is a logically separate change please split the patch up.
Regards,
Tvrtko
John.
Regards,
Tvrtko
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 +
drivers/gpu/drm/i915/i915_debugfs.c | 6 +++---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
3 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 283870c659911..959a7c92e8f4d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1602,6 +1602,9 @@ static bool ring_is_idle(struct intel_engine_cs
*engine)
{
bool idle = true;
+ /* GuC submission shouldn't access HEAD & TAIL via MMIO */
+ GEM_BUG_ON(intel_engine_uses_guc(engine));
+
if (I915_SELFTEST_ONLY(!engine->mmio_base))
return true;
@@ -1668,6 +1671,16 @@ bool intel_engine_is_idle(struct
intel_engine_cs *engine)
if (!i915_sched_engine_is_empty(engine->sched_engine))
return false;
+ /*
+ * We shouldn't touch engine registers with GuC submission as
the GuC
+ * owns the registers. Let's tie the idle to engine pm, at worst
this
+ * function sometimes will falsely report non-idle when idle
during the
+ * delay to retire requests or with virtual engines and a request
+ * running on another instance within the same class / submit mask.
+ */
+ if (intel_engine_uses_guc(engine))
+ return false;
+
/* Ring stopped? */
return ring_is_idle(engine);
}
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 94e5c29d2ee3a..ee5334840e9cb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -654,13 +654,13 @@ gt_drop_caches(struct intel_gt *gt, u64 val)
{
int ret;
+ if (val & DROP_RETIRE || val & DROP_RESET_ACTIVE)
+ intel_gt_retire_requests(gt);
+
if (val & DROP_RESET_ACTIVE &&
wait_for(intel_engines_are_idle(gt),
I915_IDLE_ENGINES_TIMEOUT))
intel_gt_set_wedged(gt);
- if (val & DROP_RETIRE)
- intel_gt_retire_requests(gt);
-
if (val & (DROP_IDLE | DROP_ACTIVE)) {
ret = intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index c22f29c3faa0e..53c7474dde495 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -278,7 +278,7 @@ struct i915_gem_mm {
u32 shrink_count;
};
-#define I915_IDLE_ENGINES_TIMEOUT (200) /* in ms */
+#define I915_IDLE_ENGINES_TIMEOUT (500) /* in ms */
unsigned long i915_fence_c
Re: [Intel-gfx] [PATCH 01/12] drm/i915: Remove bogus GEM_BUG_ON in unpark
On 19/07/2022 01:05, John Harrison wrote: On 7/18/2022 05:15, Tvrtko Ursulin wrote: On 13/07/2022 00:31, john.c.harri...@intel.com wrote: From: Matthew Brost Remove bogus GEM_BUG_ON which compared kernel context timeline seqno to seqno in memory on engine PM unpark. If a GT reset occurred these values might not match as a kernel context could be skipped. This bug was hidden by always switching to a kernel context on park (execlists requirement). Reset of the kernel context? Under which circumstances does that happen? As per description, the issue is with full GT reset. It is unclear if the claim is this to be a general problem or the assert is only invalid with the GuC. Lack of a CI reported issue suggests it is not a generic problem? Currently it is not an issue because we always switch to the kernel context because that's how execlists works and the entire driver is fundamentally based on execlist operation. When we stop using the kernel context as a (non-functional) barrier when using GuC submission, then you would see an issue without this fix. Issue is with GuC, GuC and full reset, or with full reset regardless of the backend? If issue is only with GuC patch should have drm/i915/guc prefix as minimum. But if it actually only becomes a problem when GuC backend stops parking with the kernel context when I think the whole unpark code should be refactored in a cleaner way than just removing the one assert. Otherwise what is the point of leaving everything else in there? Or if the issue is backend agnostic, *if* full reset happens to hit during parking, then it is different. Wouldn't that be a race with parking and reset which probably shouldn't happen to start with. Regards, Tvrtko John. Regards, Tvrtko Signed-off-by: Matthew Brost --- drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c index b0a4a2dbe3ee9..fb3e1599d04ec 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c @@ -68,8 +68,6 @@ static int __engine_unpark(struct intel_wakeref *wf) ce->timeline->seqno, READ_ONCE(*ce->timeline->hwsp_seqno), ce->ring->emit); - GEM_BUG_ON(ce->timeline->seqno != - READ_ONCE(*ce->timeline->hwsp_seqno)); } if (engine->unpark)
Re: [Intel-gfx] [PATCH v1 0/6] Move all drivers to a common dma-buf locking convention
On Fri, Jul 15, 2022 at 9:53 AM Dmitry Osipenko wrote: > > Hello, > > This series moves all drivers to a dynamic dma-buf locking specification. > From now on all dma-buf importers are made responsible for holding > dma-buf's reservation lock around all operations performed over dma-bufs. > This common locking convention allows us to utilize reservation lock more > broadly around kernel without fearing of potential dead locks. > > This patchset passes all i915 selftests. It was also tested using VirtIO, > Panfrost, Lima and Tegra drivers. I tested cases of display+GPU, > display+V4L and GPU+V4L dma-buf sharing, which covers majority of kernel > drivers since rest of the drivers share same or similar code paths. > > This is a continuation of [1] where Christian König asked to factor out > the dma-buf locking changes into separate series. > > [1] > https://lore.kernel.org/dri-devel/20220526235040.678984-1-dmitry.osipe...@collabora.com/ > > Dmitry Osipenko (6): > dma-buf: Add _unlocked postfix to function names > drm/gem: Take reservation lock for vmap/vunmap operations > dma-buf: Move all dma-bufs to dynamic locking specification > dma-buf: Acquire wait-wound context on attachment > media: videobuf2: Stop using internal dma-buf lock > dma-buf: Remove internal lock > > drivers/dma-buf/dma-buf.c | 198 +++--- > drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 4 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 4 +- > drivers/gpu/drm/armada/armada_gem.c | 14 +- > drivers/gpu/drm/drm_client.c | 4 +- > drivers/gpu/drm/drm_gem.c | 28 +++ > drivers/gpu/drm/drm_gem_cma_helper.c | 6 +- > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 6 +- > drivers/gpu/drm/drm_gem_shmem_helper.c| 6 +- > drivers/gpu/drm/drm_prime.c | 12 +- > drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 6 +- > drivers/gpu/drm/exynos/exynos_drm_gem.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 20 +- > .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +- > drivers/gpu/drm/i915/gem/i915_gem_object.h| 6 +- > .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 20 +- > drivers/gpu/drm/i915/i915_gem_evict.c | 2 +- > drivers/gpu/drm/i915/i915_gem_ww.c| 26 ++- > drivers/gpu/drm/i915/i915_gem_ww.h| 15 +- > drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 8 +- > drivers/gpu/drm/qxl/qxl_object.c | 17 +- > drivers/gpu/drm/qxl/qxl_prime.c | 4 +- > drivers/gpu/drm/tegra/gem.c | 27 +-- > drivers/infiniband/core/umem_dmabuf.c | 11 +- > .../common/videobuf2/videobuf2-dma-contig.c | 26 +-- > .../media/common/videobuf2/videobuf2-dma-sg.c | 23 +- > .../common/videobuf2/videobuf2-vmalloc.c | 17 +- For the videobuf2 changes: Acked-by: Tomasz Figa Best regards, Tomasz
Re: [Intel-gfx] [PATCH] drm/i915: Fix NPD in PMU during driver teardown
On 12/07/2022 22:03, Umesh Nerlige Ramappa wrote:
On Mon, Jul 04, 2022 at 09:31:55AM +0100, Tvrtko Ursulin wrote:
On 01/07/2022 15:54, Summers, Stuart wrote:
On Fri, 2022-07-01 at 09:37 +0100, Tvrtko Ursulin wrote:
On 01/07/2022 01:11, Umesh Nerlige Ramappa wrote:
On Thu, Jun 30, 2022 at 09:00:28PM +, Stuart Summers wrote:
In the driver teardown, we are unregistering the gt prior
to unregistering the PMU. This means there is a small window
of time in which the application can request metrics from the
PMU, some of which are calling into the uapi engines list,
while the engines are not available. In this case we can
see null pointer dereferences.
Fix this ordering in both the driver load and unload sequences.
Additionally add a check for engine presence to prevent this
NPD in the event this ordering is accidentally reversed. Print
a debug message indicating when they aren't available.
v1: Actually address the driver load/unload ordering issue
Signed-off-by: Stuart Summers
---
I thought this is likely happening because intel_gpu_top is running
in
the background when i915 is unloaded. I tried a quick repro, I
don't see
the unload succeed ("fatal module in use", not sure if this was a
partial unload), but when I try to kill intel_gpu_top, I get an
NPD.
This is in the event disable path - i915_pmu_event_stop ->
i915_pmu_disable.
So i915 failed to unload (as expected - with perf events open we
elevate
the module ref count via i915_pmu_event_init -> drm_dev_get), then
you
quit intel_gpu_top and get NPD? On the engine lookup? With the
re-ordered init/fini sequence as from this patch?
With elevated module count there shouldn't be any unloading happening
so
I am intrigued.
It's likely that you are seeing a different path (unload) leading
to the
same issue.
I think in i915_pmu_disable/disable should be aware of event-
hw.state
and or pmu->closed states before accessing the event. Maybe like,
if (event->hw.state != PERF_HES_STOPPED && is_engine_event(event))
{
@Tvrtko, wondering if this case is tested by igt@perf
_pmu@module-unload.
A bit yes. From what Stuart wrote it seems the test would need to be
extended to cover the case where PMU is getting opened while module
unload is in progress.
But the NPD you saw is for the moment confusing so I don't know what
is
happening.
I am not clear if we should use event->hw.state or pmu->closed here
and
if/how they are related. IMO, for this issue, the engine check is
good
enough too, so we don't really need the pmu state checks.
Thoughts?
Engine check at the moment feels like papering.
Indeed as you say I think the pmu->closed might be the solution.
Perhaps
the race is as mentioned above. PMU open happening in parallel to
unload..
If the sequence of events userspace triggers is:
i915_pmu_event_init
i915_pmu_event_start
i915_pmu_enable
i915_pmu_event_read
I guess pmu->closed can get set halfway in i915_pmu_event_init. What
would be the effect of that.. We'd try to get a module reference
while
in the process of unloading. Which is probably very bad.. So possibly
a
final check on pmu->close is needed there. Ho hum.. can it be made
safe
is the question.
It doesn't explain the NPD on Ctrl-C though.. intel_gpu_top keeps
the
evens open all the time. So I think more info needed, for me at
least.
So one thing here is this doesn't have to do with module unload, but
module unbind specifically (while perf is open). I don't know if the
NPD from Umesh is the same as what we're seeing here. I'd really like
to separate these unless you know for sure that's related. Also it
would be interesting to know if this patch fixes your issue as well.
I still think the re-ordering in i915_driver.c should be enough and we
shouldn't need to check pmu->closed. The unregister should be enough to
ensure the perf tools are notified that new events aren't allowed, and
at that time the engine structures are still intact. And even if for
some reason the perf code still calls in to our function pointers, we
have these engine checks as a failsafe.
I'm by the way uploading one more version here with a drm_WARN_ONCE
instead of the debug print.
Problem is I am not a fan of papering so lets get to the bottom of the
issue first. (In the meantime simple patch to re-order driver fini is
okay since that seems obvious enough, I tnink.)
We need to see call traces from any oopses and try to extend perf_pmu
to catch them. And we need to understand the problem, if it is a real
problem, which I laid out last week about race between module unload
and elevating the module use count from our perf event init.
Without understanding the details of possible failure mode flows we
don't know how much the papering with engine checks solves and how
much it leaves broken.
If you guys are too busy to tackle that I'll put it onto myself, but
help would certainly be appreciated.
Looks like Stuart/Chris are pointing towards the unbind as an issue.
I ran this sequence an
Re: [Intel-gfx] [PATCH] drm/i915/dp: wait on timeout before retry
> -Original Message-
> From: Murthy, Arun R
> Sent: Monday, July 18, 2022 4:49 PM
> To: Ville Syrjälä
> Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
> ;
> tprev...@gmail.com; Shankar, Uma ; Nikula, Jani
>
> Subject: RE: [PATCH] drm/i915/dp: wait on timeout before retry
>
> > -Original Message-
> > From: Ville Syrjälä
> > Sent: Monday, July 4, 2022 7:41 PM
> > To: Murthy, Arun R
> > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R
> > ; tprev...@gmail.com; Shankar, Uma
> > ; Nikula, Jani
> > Subject: Re: [PATCH] drm/i915/dp: wait on timeout before retry
> >
> > On Mon, Jul 04, 2022 at 12:53:52PM +0530, Arun R Murthy wrote:
> > > On linktraining error/timeout before retry need to wait for 400usec
> > > as per the DP CTS spec1.2
> >
> > s/CTS//
> >
> > > Under section 2.7.2 AUX Transaction Response/Reply Time-outs AUX
> > > Replier (the uPacket RX) must start sending the reply back to the
> > > AUX requester (the uPacket TX) within the response period of 300μs.
> > > The timer for Response Time-out starts ticking after the uPacket RX
> > > has finished receiving the AUX STOP condition which ends the AUX
> > > Request
> > transaction.
> > > The timer is reset either when the Response Time-out period has
> > > elapsed or when the uPacket RX has started to send the AUX Sync
> > > pattern (which follows
> > > 10 to 16 active pre-charge pulses) for the Reply transaction. If the
> > > uPacket TX does not receive a reply from the uPacket RX it must wait
> > > for a Reply Time-out period of 400us before initiating the next AUX
> > > Request transaction. The timer for the Reply Time-out starts ticking
> > > after the uPacket TX has finished sending the AUX STOP condition.
> > >
> > > The patch with commit 74ebf294a1dd ("drm/i915: Add a delay in
> > > Displayport AUX transactions for compliance testing") removes this
> > > delay mentioning the hardware already meets this requirement, but as
> > > per the spec the delay mentioned in the spec specifies how long to
> > > wait for the receiver response before timeout. So the delay here to
> > > wait for timeout and not a delay after timeout. The DP spec
> > > specifies a delay after timeout and hence adding this delay.
> >
> > Not sure what you're saying here. The spec states the reply timeout
> > should start counting once the TX has sent the AUX STOP, and gets
> > reset when the reply AUX SYNC is detected.
> >
> > If that doesn't match what the hardware is doing then we really need
> > to get bspec updated to say what is actually happening.
> >
> > Oh, and the reply timeout has been increased to 3.2ms in later
> > revisions of the spec to deal with LTTPRs. We should adjust the code to
> > match.
> >
> Will take this separately!
Hi Arun,
I would suggest to create an issue to track the LTTPR work and validate. Also
get the bspec
updated to match the hardware behavior.
With above done, we can take this patch to unblock multiple CI issues.
@Ville Syrjälä Hope this is ok.
Regards,
Uma Shankar
> Thanks and Regards,
> Arun R Murthy
>
Re: [Intel-gfx] [PATCH] drm/i915/display: Add debug print for scaler filter
> > -Original Message-
> > From: Intel-gfx On Behalf Of
> > Swati Sharma
> > Sent: Wednesday, July 6, 2022 3:53 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH] drm/i915/display: Add debug print for
> > scaler filter
> >
> > Add debug print statement to print scaler filter property value. Since
> > property can be set as either default or integer scaler; its good if
> > we can get debug print for the same in dmesg log.
>
> Looks Good to me.
> Reviewed-by: Uma Shankar
Pushed to drm-intel-next. Thanks for the change.
Regards,
Uma Shankar
> > Cc: Juha-Pekka Heikkila
> > Signed-off-by: Swati Sharma
> > ---
> > drivers/gpu/drm/i915/display/intel_crtc_state_dump.c | 9 +
> > drivers/gpu/drm/i915/display/intel_display_debugfs.c | 5 +++--
> > 2 files changed, 8 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > index 4ca6e9493ff2..e9212f69c360 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > @@ -134,8 +134,8 @@ static void intel_dump_plane_state(const struct
> > intel_plane_state *plane_state)
> > plane->base.base.id, plane->base.name,
> > fb->base.id, fb->width, fb->height, &fb->format->format,
> > fb->modifier, str_yes_no(plane_state->uapi.visible));
> > - drm_dbg_kms(&i915->drm, "\trotation: 0x%x, scaler: %d\n",
> > - plane_state->hw.rotation, plane_state->scaler_id);
> > + drm_dbg_kms(&i915->drm, "\trotation: 0x%x, scaler: %d, scaling_filter:
> > %d\n",
> > + plane_state->hw.rotation, plane_state->scaler_id,
> > +plane_state->hw.scaling_filter);
> > if (plane_state->uapi.visible)
> > drm_dbg_kms(&i915->drm,
> > "\tsrc: " DRM_RECT_FP_FMT " dst: " DRM_RECT_FMT
> "\n", @@
> > -262,10 +262,11 @@ void intel_crtc_state_dump(const struct
> > intel_crtc_state *pipe_config,
> >
> > if (DISPLAY_VER(i915) >= 9)
> > drm_dbg_kms(&i915->drm,
> > - "num_scalers: %d, scaler_users: 0x%x, scaler_id:
> > %d\n",
> > + "num_scalers: %d, scaler_users: 0x%x, scaler_id: %d,
> > +scaling_filter: %d\n",
> > crtc->num_scalers,
> > pipe_config->scaler_state.scaler_users,
> > - pipe_config->scaler_state.scaler_id);
> > + pipe_config->scaler_state.scaler_id,
> > + pipe_config->hw.scaling_filter);
> >
> > if (HAS_GMCH(i915))
> > drm_dbg_kms(&i915->drm,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 6c3954479047..225b6bfc783c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -722,10 +722,11 @@ static void intel_scaler_info(struct seq_file
> > *m, struct intel_crtc *crtc)
> >
> > /* Not all platformas have a scaler */
> > if (num_scalers) {
> > - seq_printf(m, "\tnum_scalers=%d, scaler_users=%x scaler_id=%d",
> > + seq_printf(m, "\tnum_scalers=%d, scaler_users=%x scaler_id=%d
> > +scaling_filter=%d",
> >num_scalers,
> >crtc_state->scaler_state.scaler_users,
> > - crtc_state->scaler_state.scaler_id);
> > + crtc_state->scaler_state.scaler_id,
> > + crtc_state->hw.scaling_filter);
> >
> > for (i = 0; i < num_scalers; i++) {
> > const struct intel_scaler *sc =
> > --
> > 2.25.1
Re: [Intel-gfx] [PATCH v2 01/21] drm/i915/gt: Ignore TLB invalidations on idle engines
From: Tvrtko Ursulin > Sent: 19 July 2022 08:25 ... > > It's not only the TLB flushes that cause grief. > > > > There is a loop that forces a write-back of all the frame buffer pages. > > With a large display and some cpu (like my Ivy bridge one) that > > takes long enough with pre-emption disabled that wakeup of RT processes > > (and any pinned to the cpu) takes far longer than one might have > > wished for. > > > > Since some X servers request a flush every few seconds this makes > > the system unusable for some workloads. > > Ok TLB invalidations as discussed in this patch does not apply to > Ivybridge. But what is the write back loop you mention which is causing > you grief? What size frame buffers are we talking about here? If they > don't fit in the mappable area recently we merged a patch* which > improves things in that situation but not sure you are hitting exactly that. I found the old email: What I've found is that the Intel i915 graphics driver uses the 'events_unbound' kernel worker thread to periodically execute drm_cflush_sg(). (see https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/drm_cache.c) I'm guessing this is to ensure that any writes to graphics memory become visible is a semi-timely manner. This loop takes about 1us per iteration split fairly evenly between whatever is in for_each_sg_page() and drm_cflush_page(). With a 2560x1440 display the loop count is 3600 (4 bytes/pixel) and the whole function takes around 3.3ms. IIRC the first few page flushes are quick (I bet they go into a fifo) and then they all get slow. The flushes are actually requested from userspace. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [Intel-gfx] linux-next: manual merge of the drm tree with the drm-misc-fixes tree
Hi Stephen,
On Mon, Jul 18, 2022 at 1:49 AM Stephen Rothwell wrote:
> On Mon, 11 Jul 2022 10:05:45 +0200 Christian König
> wrote:
> > Am 11.07.22 um 04:47 schrieb Stephen Rothwell:
> > >
> > > Today's linux-next merge of the drm tree got a conflict in:
> > >
> > >drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > >
> > > between commit:
> > >
> > >925b6e59138c ("Revert "drm/amdgpu: add drm buddy support to amdgpu"")
> > >
> > > from the drm-misc-fixes tree and commit:
> > >
> > >5e3f1e7729ec ("drm/amdgpu: fix start calculation in
> > > amdgpu_vram_mgr_new")
> > >
> > > from the drm tree.
> > >
> > > This is a mess :-( I have just reverted the above revert before mergin
> > > the drm tree for today, please fix it up.
> >
> > Sorry for the noise, the patch "5e3f1e7729ec ("drm/amdgpu: fix start
> > calculation in amdgpu_vram_mgr_new")" and another one is going to be
> > reverted from the drm tree as well.
> >
> > It's just that -fixes patches where faster than -next patches.
>
> Here we are a week later, -rc7 has been released and as far as I can
> tell (though I may have missed it), this is still a problem :-(
>
> I am still reverting 925b6e59138c (which is now in Linus' tree).
Thanks for the hint! After reverting that commit, drm-next (sort of[1])
merges cleanly into upstream again.
[1] There's still a small conflict due to the removal of
force_dpms_off, cfr. the difference between commits
3283c83eb6fcfbda and cc79950bf0904f58 ("drm/amd/display:
Ensure valid event timestamp for cursor-only commits") in
v5.19-rc7 resp. drm-next.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [Intel-gfx] [PATCH] drm/i915/guc: support v69 in parallel to v70
On 18/07/2022 17:41, Ceraolo Spurio, Daniele wrote:
On 7/18/2022 3:02 AM, Tvrtko Ursulin wrote:
Hi,
On 15/07/2022 23:54, Daniele Ceraolo Spurio wrote:
This patch re-introduces support for GuC v69 in parallel to v70. As this
is a quick fix, v69 has been re-introduced as the single "fallback" guc
version in case v70 is not available on disk. All v69 specific code has
been labeled as such for easy identification, and the same was done for
all v70 functions for which there is a separate v69 version, to avoid
accidentally calling the wrong version via the unlabeled name.
When the fallback mode kicks in, a drm_warn message is printed in dmesg
to warn the user of the required update.
The plan is to follow this up with a more complex rework to allow for
multiple different GuC versions to be supported at the same time.
Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")
Please check if I got this right:
* ADL-P was out of "force probe" starting from 5.17.
* GuC fw got bumped from v62 to v69 in 5.18.
Does this mean you would also need to handle v62 to avoid regressing
ADL-P from 5.17 to 5.18? I couldn't figure out when ADL-P switched
from execlists to GuC due a bit convoluted supported/wanted/needed
macros etc, so not entirely sure.
I haven't checked about previous GuC versions because the report from
Dave was on the 69->70 transition and about re-introducing v69 support,
so I just focused on that. Let me dig on the versions and on what would
be needed to support all 3 revs (if it is required).
Secondly, my concern with the approach like in this patch is that it
would grow the i915 code base *if* there is no incentive to keep the
compatiblity breaking firware updates in check.
The grow of the i915 code is inevitable. Even without changes to
existing platforms, new features for new platforms will require new GuC
interfaces. Sometimes the GuC team also refactors an existing interface
so that it can include a new aspect of an existing feature. We'll have
to discuss with them how to minimize breakages in such scenarios.
To think about in tandem with this is the question of whether many
more fallback versions need to be handled, even for platforms which
only use GuC to load HuC? Those would also regress in the media
encoding side of things, even if they don't use GuC submission, right?
The only HuC-only platform is ADL-S and that went out of force probe
when we were on GuC 62, so definitely nothing older than that will be
needed.
I was referring to platforms where HuC is used for some encoding types.
List on
https://github.com/intel/media-driver/blob/master/docs/media_features.md#media-features-summary.
It is not entirely clear to me from that list - you are saying the HuC
is actually used only on ADL-S? I was going by the existence of HuC
firmware files only so might be wrong just as well.
Regards,
Tvrtko
If this is so, the approach from this patch would feel rushed in my view.
It totally is, no argument there. As mentioned in the commit message,
the plan is to replace the whole thing with a more flexible and cleaner
mechanism, but we do need something for the upcoming 5.19 release so
there is no time to do this properly from the get-go.
There is also the question of being able to automatically load the
latest _compatible_ (same major version) GuC fw found on disk. Aka
allowing a bugfix firmware update which does not require a kernel
update. In theory should be possible but we don't have that
implemented either, right?
We do not. Something like this was actually shot down when GuC first
came around. We used to have simlinks for the GuC binary to be able to
drop in a replacement like you said, but there were concerns about how
to validate all the possible kernel:fw combinations this could cause,
hence why in the end we went with the exact match model. Note that at
the time we didn't have a patch number for bugfix tracking in GuC, so
the decision made more sense back then than it does now. We've already
restarted the discussion internally.
Daniele
Regards,
Tvrtko
Link:
https://lists.freedesktop.org/archives/intel-gfx/2022-July/301640.html
Signed-off-by: Daniele Ceraolo Spurio
Cc: John Harrison
Cc: Matthew Brost
Cc: Matt Roper
Cc: Dave Airlie
---
drivers/gpu/drm/i915/gt/intel_context_types.h | 11 +-
.../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 3 +
drivers/gpu/drm/i915/gt/uc/intel_guc.h | 5 +
drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 45 +++
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 348 +++---
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 57 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 7 +
7 files changed, 419 insertions(+), 57 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index d2d75d9c0c8d..04eacae1aca5 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/dr
Re: [Intel-gfx] [PATCH v2 01/21] drm/i915/gt: Ignore TLB invalidations on idle engines
Hi David,
On 18/07/2022 16:50, David Laight wrote:
From: Mauro Carvalho Chehab
Sent: 18 July 2022 15:54
On Mon, 18 Jul 2022 14:16:10 +0100
Tvrtko Ursulin wrote:
On 14/07/2022 13:06, Mauro Carvalho Chehab wrote:
From: Chris Wilson
Check if the device is powered down prior to any engine activity,
as, on such cases, all the TLBs were already invalidated, so an
explicit TLB invalidation is not needed, thus reducing the
performance regression impact due to it.
This becomes more significant with GuC, as it can only do so when
the connection to the GuC is awake.
Cc: sta...@vger.kernel.org
Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
Patch itself looks fine but I don't think we closed on the issue of
stable/fixes on this patch?
No, because TLB cache invalidation takes time and causes time outs, which
in turn affects applications and produce Kernel warnings.
It's not only the TLB flushes that cause grief.
There is a loop that forces a write-back of all the frame buffer pages.
With a large display and some cpu (like my Ivy bridge one) that
takes long enough with pre-emption disabled that wakeup of RT processes
(and any pinned to the cpu) takes far longer than one might have
wished for.
Since some X servers request a flush every few seconds this makes
the system unusable for some workloads.
Ok TLB invalidations as discussed in this patch does not apply to
Ivybridge. But what is the write back loop you mention which is causing
you grief? What size frame buffers are we talking about here? If they
don't fit in the mappable area recently we merged a patch* which
improves things in that situation but not sure you are hitting exactly that.
Regards,
Tvrtko
*) 230523ba24bd ("drm/i915/gem: Don't evict unmappable VMAs when pinning
with PIN_MAPPABLE (v2)")
Re: [Intel-gfx] [PATCH v2 05/21] drm/i915/gt: Skip TLB invalidations once wedged
On 18/07/2022 17:06, Mauro Carvalho Chehab wrote:
On Mon, 18 Jul 2022 14:45:22 +0100
Tvrtko Ursulin wrote:
On 14/07/2022 13:06, Mauro Carvalho Chehab wrote:
From: Chris Wilson
Skip all further TLB invalidations once the device is wedged and
had been reset, as, on such cases, it can no longer process instructions
on the GPU and the user no longer has access to the TLB's in each engine.
That helps to reduce the performance regression introduced by TLB
invalidate logic.
Cc: sta...@vger.kernel.org
Fixes: 7938d61591d3 ("drm/i915: Flush TLBs before releasing backing store")
Is the claim of a performance regression this solved based on a wedged
GPU which does not work any more to the extend where mmio tlb
invalidation requests keep timing out? If so please clarify in the
commit text and then it looks good to me. Even if it is IMO a very
borderline situation to declare something a fix.
Indeed this helps on a borderline situation: if GT is wedged, TLB
invalidation will timeout, so it makes sense to keep the patch with a
comment like:
drm/i915/gt: Skip TLB invalidations once wedged
Skip all further TLB invalidations once the device is wedged and
had been reset, as, on such cases, it can no longer process instructions
on the GPU and the user no longer has access to the TLB's in each engine.
So, an attempt to do a TLB cache invalidation will produce a timeout.
That helps to reduce the performance regression introduced by TLB
invalidate logic.
Yeah that is better but whether bothering stable with it is the
question. Wedged GPU means constant endless -EIO to userspace so very
hard to imagine that after a TLB invalidation timeout or two there would
be further ones. But okay, it's tiny so fine I guess.
Regards,
Tvrtko
Re: [Intel-gfx] [PATCH] drm/i915/hdmi: Prune modes that require HDMI2.1 FRL
> -Original Message-
> From: Nautiyal, Ankit K
> Sent: Tuesday, July 19, 2022 11:40 AM
> To: Murthy, Arun R ; intel-
> g...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/hdmi: Prune modes that require
> HDMI2.1 FRL
>
>
> On 7/19/2022 8:45 AM, Murthy, Arun R wrote:
> >> -Original Message-
> >> From: Nautiyal, Ankit K
> >> Sent: Friday, July 8, 2022 3:36 PM
> >> To: Murthy, Arun R ; intel-
> >> g...@lists.freedesktop.org
> >> Subject: Re: [Intel-gfx] [PATCH] drm/i915/hdmi: Prune modes that
> >> require
> >> HDMI2.1 FRL
> >>
> >> Hi Arun,
> >>
> >> Thanks for the comments.
> >>
> >> Please find my response inline.
> >>
> >> On 7/8/2022 9:30 AM, Murthy, Arun R wrote:
> -Original Message-
> From: Intel-gfx On Behalf
> Of Ankit Nautiyal
> Sent: Thursday, July 7, 2022 10:57 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH] drm/i915/hdmi: Prune modes that
> require
> HDMI2.1 FRL
>
> HDMI2.1 requires some higher resolution video modes to be
> enumerated only if HDMI2.1 Fixed Rate Link (FRL) is supported.
> Current platforms do not support FRL transmission so prune modes
> that require HDMI2.1 FRL.
>
> >>> If the hardware doesn't support FRL then it basically blocks HDMI2.1
> >> feature.
> >>> Then it wont be possible to use any resolution above 4k60 is it?
> >>
> >> Yes right. As I understand, the HDMI2.1a supersedes HDMI2.0b, and so
> >> the
> >>
> >> platforms supporting HDMI2.0 must now pass the HDMI2.1 CTS.
> >> The HDMI2.1a spec introduces Marketing Feature names for 4K100,
> >> 4K120, 8k@50, 8k@60 with suffix A, and B.
> >> Suffix A meaning mode supported without compression, and B meaning,
> >> mode supported with compression.
> >>
> >> There are CTS tests that expect these modes not to be enumerated, if
> >> the source does support the given requirements.
> >>
> >>
> > Thanks for the clarification.
> >
> Signed-off-by: Ankit Nautiyal
> ---
> drivers/gpu/drm/i915/display/intel_hdmi.c | 21
> >> +
> 1 file changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index ebd91aa69dd2..93c00b61795f 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -1974,6 +1974,20 @@ intel_hdmi_mode_clock_valid(struct
> drm_connector *connector, int clock,
> return status;
> }
>
> +/*
> + * HDMI2.1 requires higher resolution modes like 8k60, 4K120 to be
> + * enumerated only if FRL is supported. Platforms not supporting
> +FRL
> + * must prune these modes.
> + */
> +static bool
> +hdmi21_frl_quirk(int dotclock, bool frl_supported) {
> +if (dotclock >= 60 && !frl_supported)
> +return true;
> +
> +return false;
> +}
> +
> static enum drm_mode_status
> intel_hdmi_mode_valid(struct drm_connector *connector,
> struct drm_display_mode *mode) @@ -2001,6 +2015,13
> >> @@
> intel_hdmi_mode_valid(struct drm_connector *connector,
> clock *= 2;
> }
>
> +/*
> + * Current Platforms do not support HDMI2.1 FRL mode of
> transmission,
> + * so prune the modes that require FRL.
> + */
> +if (hdmi21_frl_quirk(clock, false))
> +return MODE_BAD;
> +
> >>> Instead of setting this frl_supported as false, can we get this info
> >>> from hardware, so that when our hardware supports it later it would
> >>> be
> >> easy to enable this.
> >>
> >> We can have something like:
> >>
> >> src_supports_frl()
> >>
> >> {
> >>
> >> /* FRL not supported in
> >>
> >> return false;
> >>
> >> }
> >>
> > Yes something like this looks good. It would be a good design to judge
> > this based on the Display version.
>
> I do agree, we need to have this check when we have HDMI2.1 support for
> any platform.
>
> In future patches, when FRL transmission will be enabled, at that time it
> would make sense to check for display version, and parse from VBT about
> what rate it allows etc.
>
Awaiting patch with handling this properly!
Reviewed-by: Arun R Murthy
Thanks and Regards,
Arun R Murthy
---
