Re: [Intel-gfx] [PATCH v2] drm/i915: Print return value on error
On 14.10.2022 17:46, Nirmoy Das wrote:
Print returned error code for better debuggability.
References: https://gitlab.freedesktop.org/drm/intel/-/issues/7211
Signed-off-by: Nirmoy Das
Reviewed-by: Andrzej Hajda
Regards
Andrzej
---
drivers/gpu/drm/i915/display/intel_fbdev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
b/drivers/gpu/drm/i915/display/intel_fbdev.c
index 112aa0447a0d..ab385d18ddcc 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -175,7 +175,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
}
if (IS_ERR(obj)) {
- drm_err(&dev_priv->drm, "failed to allocate framebuffer\n");
+ drm_err(&dev_priv->drm, "failed to allocate framebuffer
(%pe)\n", obj);
return PTR_ERR(obj);
}
@@ -256,7 +256,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
info = drm_fb_helper_alloc_fbi(helper);
if (IS_ERR(info)) {
- drm_err(&dev_priv->drm, "Failed to allocate fb_info\n");
+ drm_err(&dev_priv->drm, "Failed to allocate fb_info (%pe)\n",
info);
ret = PTR_ERR(info);
goto out_unpin;
}
@@ -291,7 +291,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
vaddr = i915_vma_pin_iomap(vma);
if (IS_ERR(vaddr)) {
drm_err(&dev_priv->drm,
- "Failed to remap framebuffer into virtual memory\n");
+ "Failed to remap framebuffer into virtual memory
(%pe)\n", vaddr);
ret = PTR_ERR(vaddr);
goto out_unpin;
}
Re: [Intel-gfx] [PATCH v2] drm/i915/dgfx: Keep PCI autosuspend control 'on' by default on all dGPU
> -Original Message-
> From: Gupta, Anshuman
> Sent: Friday, October 14, 2022 5:03 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: joonas.lahti...@linux.intel.com; tvrtko.ursu...@linux.intel.com; Auld,
> Matthew ; Vivi, Rodrigo ;
> Gupta, Anshuman
> Subject: [PATCH v2] drm/i915/dgfx: Keep PCI autosuspend control 'on' by
> default on all dGPU
>
> DGFX platforms has lmem and cpu can access the lmem objects via mmap and
> i915 internal i915_gem_object_pin_map() for
> i915 own usages. Both of these methods has pre-requisite requirement to keep
> GFX PCI endpoint in D0 for a supported iomem transaction over PCI link. (Refer
> PCIe specs 5.3.1.4.1)
>
> Both DG1/DG2 have a known hardware bug that violates the PCIe specs and
> support the iomem read write transaction over PCIe bus despite endpoint is D3
> state.
> Due to above H/W bug, we had never observed any issue with i915 runtime PM
> versus lmem access.
> But this issue becomes visible when PCIe gfx endpoint's upstream bridge enters
> to D3, at this point any lmem read/write access will be returned as
> unsupported
> request. But again this issue is not observed on every platform because it has
> been observed on few host machines
> DG1/DG2 endpoint's upstream bridge does not bind with pcieport driver.
> which really disables the PCIe power savings and leaves the bridge at D0
> state.
>
> We need a unique interface to read/write from lmem with runtime PM wakeref
> protection something similar to intel_uncore_{read, write}, keep autosuspend
> control to 'on' on all discrete platforms, until we have a unique interface to
> read/write from lmem.
>
> This just change the default autosuspend setting of i915 on dGPU, user can
> still
> change it to 'auto'.
>
> v2:
> - Modified the commit message and subject with more information.
> - Changed the Fixes tag to LMEM support commit. [Joonas]
> - Changed !HAS_LMEM() Cond to !IS_DGFX(). [Rodrigo]
>
> Fixes: b908be543e44 ("drm/i915: support creating LMEM objects")
> Suggested-by: Rodrigo Vivi
> Signed-off-by: Anshuman Gupta
Hi Rodrigo ,
I missed to add your conditional RB tag here.
I addressed all comments and changed the Fixes tag to b908be543e44 ("drm/i915:
support creating LMEM objects")
CI is green, shall I use your RB and merge this tag if nobody else has any
further review comment?
Thanks,
Anshuman.
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6ed5786bcd29..744cca507946 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -591,8 +591,15 @@ void intel_runtime_pm_enable(struct
> intel_runtime_pm *rpm)
> pm_runtime_use_autosuspend(kdev);
> }
>
> - /* Enable by default */
> - pm_runtime_allow(kdev);
> + /*
> + * FIXME: Temp hammer to keep autosupend disable on lmem
> supported platforms.
> + * As per PCIe specs 5.3.1.4.1, all iomem read write request over a
> PCIe
> + * function will be unsupported in case PCIe endpoint function is in
> D3.
> + * Let's keep i915 autosuspend control 'on' till we fix all known issue
> + * with lmem access in D3.
> + */
> + if (!IS_DGFX(i915))
> + pm_runtime_allow(kdev);
>
> /*
>* The core calls the driver load handler with an RPM reference held.
> --
> 2.25.1
[Intel-gfx] ✗ Fi.CI.BAT: failure for Enable YCbCr420 for VDSC (rev2)
== Series Details ==
Series: Enable YCbCr420 for VDSC (rev2)
URL : https://patchwork.freedesktop.org/series/109723/
State : failure
== Summary ==
CI Bug Log - changes from CI_DRM_12250 -> Patchwork_109723v2
Summary
---
**FAILURE**
Serious unknown changes coming with Patchwork_109723v2 absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_109723v2, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
External URL:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109723v2/index.html
Participating hosts (45 -> 45)
--
Additional (1): fi-rkl-guc
Missing(1): fi-bdw-samus
Possible new issues
---
Here are the unknown changes that may have been introduced in
Patchwork_109723v2:
### IGT changes ###
Possible regressions
* igt@gem_exec_gttfill@basic:
- fi-pnv-d510:[PASS][1] -> [FAIL][2]
[1]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12250/fi-pnv-d510/igt@gem_exec_gttf...@basic.html
[2]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109723v2/fi-pnv-d510/igt@gem_exec_gttf...@basic.html
* igt@runner@aborted:
- fi-rkl-guc: NOTRUN -> [FAIL][3]
[3]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109723v2/fi-rkl-guc/igt@run...@aborted.html
Suppressed
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_pm_rpm@module-reload:
- {fi-tgl-dsi}: [INCOMPLETE][4] ([i915#7190]) -> [INCOMPLETE][5]
[4]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12250/fi-tgl-dsi/igt@i915_pm_...@module-reload.html
[5]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109723v2/fi-tgl-dsi/igt@i915_pm_...@module-reload.html
Known issues
Here are the changes found in Patchwork_109723v2 that come from known issues:
### IGT changes ###
Issues hit
* igt@gem_linear_blits@basic:
- fi-pnv-d510:[PASS][6] -> [SKIP][7] ([fdo#109271])
[6]:
https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12250/fi-pnv-d510/igt@gem_linear_bl...@basic.html
[7]:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109723v2/fi-pnv-d510/igt@gem_linear_bl...@basic.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#7190]: https://gitlab.freedesktop.org/drm/intel/issues/7190
Build changes
-
* Linux: CI_DRM_12250 -> Patchwork_109723v2
CI-20190529: 20190529
CI_DRM_12250: 9b84132c9b266a89e2865cbe8b586dd0b14cc2a4 @
git://anongit.freedesktop.org/gfx-ci/linux
IGT_7016: 642f4bf44e2b42791b4d1684936a1bfbe2d099ee @
https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
Patchwork_109723v2: 9b84132c9b266a89e2865cbe8b586dd0b14cc2a4 @
git://anongit.freedesktop.org/gfx-ci/linux
### Linux commits
880ea128c03f drm/i915: Fill in native_420 field
a754ca744bd5 drm/i915: Enable YCbCr420 for VDSC
0ac8c6a76fe7 drm/i915: Adding the new registers for DSC
f32c1682dec5 drm/i915/dp: Check if DSC supports the given output_format
== Logs ==
For more details see:
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_109723v2/index.html
[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Enable YCbCr420 for VDSC (rev2)
== Series Details ==
Series: Enable YCbCr420 for VDSC (rev2)
URL : https://patchwork.freedesktop.org/series/109723/
State : warning
== Summary ==
Error: dim checkpatch failed
d4a9778e02e4 drm/i915/dp: Check if DSC supports the given output_format
3e53847fd2c1 drm/i915: Adding the new registers for DSC
6f3d23b30a32 drm/i915: Enable YCbCr420 for VDSC
-:189: CHECK:MACRO_ARG_REUSE: Macro argument reuse '_row' - possible
side-effects?
#189: FILE: drivers/gpu/drm/i915/display/intel_qp_tables.c:447:
+#define PARAM_TABLE(_minmax, _bpc, _row, _col, _is_420) do { \
+ if (bpc == (_bpc)) {\
+ if (_is_420)\
+ return
rc_range_##_minmax##qp420_##_bpc##bpc[_row][_col]; \
+ else\
+ return
rc_range_##_minmax##qp444_##_bpc##bpc[_row][_col]; \
+ } \
} while (0)
-:189: CHECK:MACRO_ARG_REUSE: Macro argument reuse '_col' - possible
side-effects?
#189: FILE: drivers/gpu/drm/i915/display/intel_qp_tables.c:447:
+#define PARAM_TABLE(_minmax, _bpc, _row, _col, _is_420) do { \
+ if (bpc == (_bpc)) {\
+ if (_is_420)\
+ return
rc_range_##_minmax##qp420_##_bpc##bpc[_row][_col]; \
+ else\
+ return
rc_range_##_minmax##qp444_##_bpc##bpc[_row][_col]; \
+ } \
} while (0)
total: 0 errors, 0 warnings, 2 checks, 228 lines checked
a6c473a5a5c0 drm/i915: Fill in native_420 field
[Intel-gfx] ✗ Fi.CI.BUILD: failure for Patch "drm/i915: Fix display problems after resume" has been added to the 6.0-stable tree
== Series Details == Series: Patch "drm/i915: Fix display problems after resume" has been added to the 6.0-stable tree URL : https://patchwork.freedesktop.org/series/109741/ State : failure == Summary == Error: patch https://patchwork.freedesktop.org/api/1.0/series/109741/revisions/1/mbox/ not applied Patch is empty. When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To record the empty patch as an empty commit, run "git am --allow-empty". To restore the original branch and stop patching, run "git am --abort".
[Intel-gfx] Patch "drm/i915: Fix display problems after resume" has been added to the 6.0-stable tree
This is a note to let you know that I've just added the patch titled
drm/i915: Fix display problems after resume
to the 6.0-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
drm-i915-fix-display-problems-after-resume.patch
and it can be found in the queue-6.0 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let know about it.
>From 6c482c62a635aa4f534d2439fbf8afa37452b986 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20Hellstr=C3=B6m?=
Date: Wed, 5 Oct 2022 14:11:59 +0200
Subject: drm/i915: Fix display problems after resume
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
From: Thomas Hellström
commit 6c482c62a635aa4f534d2439fbf8afa37452b986 upstream.
Commit 39a2bd34c933 ("drm/i915: Use the vma resource as argument for gtt
binding / unbinding") introduced a regression that due to the vma resource
tracking of the binding state, dpt ptes were not correctly repopulated.
Fix this by clearing the vma resource state before repopulating.
The state will subsequently be restored by the bind_vma operation.
Fixes: 39a2bd34c933 ("drm/i915: Use the vma resource as argument for gtt
binding / unbinding")
Signed-off-by: Thomas Hellström
Link:
https://patchwork.freedesktop.org/patch/msgid/20220912121957.31310-1-thomas.hellst...@linux.intel.com
Cc: Matthew Auld
Cc: intel-gfx@lists.freedesktop.org
Cc: # v5.18+
Reported-and-tested-by: Kevin Boulain
Tested-by: David de Sousa
Reviewed-by: Matthew Auld
Reviewed-by: Andrzej Hajda
Signed-off-by: Matthew Auld
Link:
https://patchwork.freedesktop.org/patch/msgid/20221005121159.340245-1-thomas.hellst...@linux.intel.com
(cherry picked from commit bc2472538c0d1cce334ffc9e97df0614cd2b1469)
Signed-off-by: Tvrtko Ursulin
Signed-off-by: Greg Kroah-Hartman
---
drivers/gpu/drm/i915/gt/intel_ggtt.c |8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -1267,10 +1267,16 @@ bool i915_ggtt_resume_vm(struct i915_add
atomic_read(&vma->flags) & I915_VMA_BIND_MASK;
GEM_BUG_ON(!was_bound);
- if (!retained_ptes)
+ if (!retained_ptes) {
+ /*
+* Clear the bound flags of the vma resource to allow
+* ptes to be repopulated.
+*/
+ vma->resource->bound_flags = 0;
vma->ops->bind_vma(vm, NULL, vma->resource,
obj ? obj->cache_level : 0,
was_bound);
+ }
if (obj) { /* only used during resume => exclusive access */
write_domain_objs |= fetch_and_zero(&obj->write_domain);
obj->read_domains |= I915_GEM_DOMAIN_GTT;
Patches currently in stable-queue which might be from
thomas.hellst...@linux.intel.com are
queue-6.0/drm-i915-gt-use-i915_vm_put-on-ppgtt_create-error-paths.patch
queue-6.0/drm-virtio-unlock-reservations-on-dma_resv_reserve_fences-error.patch
queue-6.0/drm-i915-fix-display-problems-after-resume.patch
Re: [Intel-gfx] [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function
Den 13.10.2022 15.18, skrev Maxime Ripard:
> The current construction of the named mode parsing doesn't allow to extend
> it easily. Let's move it to a separate function so we can add more
> parameters and modes.
>
> In order for the tests to still pass, some extra checks are needed, so
> it's not a 1:1 move.
>
> Signed-off-by: Maxime Ripard
>
I was hoping that someone else would step up and review these parser
patches since the parser code is rather difficult to read, for me at
least. I have studied it now, so I'll give it a try.
> ---
> Changes in v4:
> - Fold down all the named mode patches that were split into a single
> patch again to maintain bisectability
> ---
> drivers/gpu/drm/drm_modes.c | 73
> ++---
> 1 file changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c0dceff51cac..2f020ef2ddf2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[]
> = {
> "PAL",
> };
>
> +static int drm_mode_parse_cmdline_named_mode(const char *name,
> + unsigned int name_end,
> + struct drm_cmdline_mode
> *cmdline_mode)
> +{
> + unsigned int i;
> +
> + if (!name_end)
> + return 0;
name_end can't be zero since the argument is checked before calling this
function.
> +
> + /* If the name starts with a digit, it's not a named mode */
> + if (isdigit(name[0]))
> + return 0;
> +
> + /*
> + * If there's an equal sign in the name, the command-line
> + * contains only an option and no mode.
> + */
> + if (strnchr(name, name_end, '='))
> + return 0;
I think this check actually belongs in
drm_mode_parse_command_line_for_connector() after options_off is set. If
theres an equal sign it should skip all mode parsing and skip down to
drm_mode_parse_cmdline_options(). Which probably means that the mode
parsing should have been moved out to separate function to avoid using a
goto.
But that's probably beyond the scope of this patchset :)
> +
> +#define STR_STRICT_EQ(str, len, cmp) \
> + (str_has_prefix(str, cmp) == len)
> +
> + /* The connection status extras can be set without a mode. */
> + if (STR_STRICT_EQ(name, name_end, "d") ||
> + STR_STRICT_EQ(name, name_end, "D") ||
> + STR_STRICT_EQ(name, name_end, "e"))
> + return 0;
It took me a while to understand what is going on here.
If str_has_prefix() finds a match it returns strlen(prefix). Since
prefix is always of length 1, name_end has to always be 1 for the
statement to be true.
I would have written it like this:
/* The connection status extras can be set without a mode. */
if (name_end == 1) {
if (name[0] == "d" || name[0] == "D" || name[0] == "e")
return 0;
}
> +
> + /*
> + * We're sure we're a named mode at that point, iterate over the
that -> this ?
> + * list of modes we're aware of.
> + */
> + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> + int ret;
> +
> + ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> + if (ret != name_end)
> + continue;
> +
> + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> + cmdline_mode->specified = true;
> +
> + return 1;
> + }
> +
> + return -EINVAL;
> +}
> +
> /**
> * drm_mode_parse_command_line_for_connector - parse command line modeline
> for connector
> * @mode_option: optional per connector mode option
> @@ -2265,7 +2314,7 @@ bool drm_mode_parse_command_line_for_connector(const
> char *mode_option,
> const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> const char *options_ptr = NULL;
> char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> - int i, len, ret;
> + int len, ret;
>
> memset(mode, 0, sizeof(*mode));
> mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> @@ -2306,17 +2355,19 @@ bool drm_mode_parse_command_line_for_connector(const
> char *mode_option,
> parse_extras = true;
> }
>
> - /* First check for a named mode */
> - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> - ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> - if (ret == mode_end) {
> - if (refresh_ptr)
> - return false; /* named + refresh is invalid */
>
> - strcpy(mode->name, drm_named_modes_whitelist[i]);
> - mode->specified = true;
> - break;
> - }
> + if (mode_end) {
Shouldn't this be:
if (!mode_end)
re
