Patch "drm/plane-helper: Add the missing declaration of drm_atomic_state" has been added to the 6.1-stable tree
This is a note to let you know that I've just added the patch titled
drm/plane-helper: Add the missing declaration of drm_atomic_state
to the 6.1-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-plane-helper-add-the-missing-declaration-of-drm_atomic_state.patch
and it can be found in the queue-6.1 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let know about it.
>From 4e699e34f923188175986ad8a74ab99f7034075e Mon Sep 17 00:00:00 2001
From: Ma Jun
Date: Fri, 16 Dec 2022 11:05:26 +0800
Subject: drm/plane-helper: Add the missing declaration of drm_atomic_state
From: Ma Jun
commit 4e699e34f923188175986ad8a74ab99f7034075e upstream.
Add the missing declaration of struct drm_atomic_state to fix the
compile error below:
error: 'struct drm_atomic_state' declared inside parameter
list will not be visible outside of this definition or declaration [-Werror]
Signed-off-by: Ma Jun
Reviewed-by: Thomas Zimmermann
Signed-off-by: Thomas Zimmermann
Fixes: 8401bd361f59 ("drm/plane-helper: Add a drm_plane_helper_atomic_check()
helper")
Cc: Javier Martinez Canillas
Cc: Thomas Zimmermann
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-devel@lists.freedesktop.org
Cc: # v6.1+
Link:
https://patchwork.freedesktop.org/patch/msgid/20221216030526.1335609-1-ma...@amd.com
Signed-off-by: Greg Kroah-Hartman
---
include/drm/drm_plane_helper.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index ff83d2621687..3a574e8cd22f 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -26,6 +26,7 @@
#include
+struct drm_atomic_state;
struct drm_crtc;
struct drm_framebuffer;
struct drm_modeset_acquire_ctx;
--
2.39.0
Patches currently in stable-queue which might be from ma...@amd.com are
queue-6.1/drm-plane-helper-add-the-missing-declaration-of-drm_atomic_state.patch
Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation
On 09.01.2023 13:24, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Revert to the original explicit approach and document the reasoning
behind it.
v2:
* DG2 needs to be covered too. (Matt)
v3:
* Full version check for Gen12 to avoid catching all future platforms.
(Matt)
Signed-off-by: Tvrtko Ursulin
Cc: Matt Roper
Cc: Balasubramani Vivekanandan
Cc: Andrzej Hajda
Reviewed-by: Andrzej Hajda # v1
---
drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 75a7cb33..5521fa057aab 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
unsigned int num = 0;
unsigned long flags;
- if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+ /*
+* New platforms should not be added with catch-all-newer (>=)
+* condition so that any later platform added triggers the below warning
+* and in turn mandates a human cross-check of whether the invalidation
+* flows have compatible semantics.
+*
+* For instance with the 11.00 -> 12.00 transition three out of five
+* respective engine registers were moved to masked type. Then after the
+* 12.00 -> 12.50 transition multi cast handling is required too.
+*/
+
+ if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+ GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
regs = NULL;
num = ARRAY_SIZE(xehp_regs);
- } else if (GRAPHICS_VER(i915) == 12) {
+ } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
+ GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
MTL support is lost? IP_VER(12, 70)
And again it looks for me inconsistent, some unknown platforms are
covered, for example 12.54, some not, for example 12.11.
Regards
Andrzej
regs = gen12_regs;
num = ARRAY_SIZE(gen12_regs);
} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
Re: [PATCH] drm/meson: dw-hdmi: Fix devm_regulator_*get_enable*() conversion
On 09/01/2023 23:00, Marek Szyprowski wrote:
devm_regulator_get_enable_optional() function returns 0 on success, so
use it for the check if function succeded instead of the -ENODEV value.
Fixes: 429e87063661 ("drm/meson: dw-hdmi: Use devm_regulator_*get_enable*()")
Signed-off-by: Marek Szyprowski
---
drivers/gpu/drm/meson/meson_dw_hdmi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/meson/meson_dw_hdmi.c
b/drivers/gpu/drm/meson/meson_dw_hdmi.c
index 7642f740272b..534621a13a34 100644
--- a/drivers/gpu/drm/meson/meson_dw_hdmi.c
+++ b/drivers/gpu/drm/meson/meson_dw_hdmi.c
@@ -718,7 +718,7 @@ static int meson_dw_hdmi_bind(struct device *dev, struct
device *master,
dw_plat_data = &meson_dw_hdmi->dw_plat_data;
ret = devm_regulator_get_enable_optional(dev, "hdmi");
- if (ret != -ENODEV)
+ if (ret < 0)
return ret;
meson_dw_hdmi->hdmitx_apb = devm_reset_control_get_exclusive(dev,
Acked-by: Neil Armstrong
Re: [PATCH] drm/msm/dpu: disable DSC blocks for SM8350
On Mon, 09 Jan 2023 23:43:09 +0200, Dmitry Baryshkov wrote: > SM8350 has newer version of DSC blocks, which are not supported by the > driver yet. Remove them for now until these blocks are supported by the > driver. > > Applied, thanks! [1/1] drm/msm/dpu: disable DSC blocks for SM8350 https://gitlab.freedesktop.org/lumag/msm/-/commit/3b2551eaeac3 Best regards, -- Dmitry Baryshkov
Re: [PATCH] drm/meson: dw-hdmi: Fix devm_regulator_*get_enable*() conversion
Hi, On Mon, 09 Jan 2023 23:00:33 +0100, Marek Szyprowski wrote: > devm_regulator_get_enable_optional() function returns 0 on success, so > use it for the check if function succeded instead of the -ENODEV value. > > Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next) [1/1] drm/meson: dw-hdmi: Fix devm_regulator_*get_enable*() conversion https://cgit.freedesktop.org/drm/drm-misc/commit/?id=67d0a30128c9f644595dfe67ac0fb941a716a6c9 -- Neil
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
Hi Daniel, On Mon, 9 Jan 2023 21:40:21 +0100 Daniel Vetter wrote: > On Mon, Jan 09, 2023 at 06:17:48PM +0100, Boris Brezillon wrote: > > Hi Jason, > > > > On Mon, 9 Jan 2023 09:45:09 -0600 > > Jason Ekstrand wrote: > > > > > On Thu, Jan 5, 2023 at 1:40 PM Matthew Brost > > > wrote: > > > > > > > On Mon, Jan 02, 2023 at 08:30:19AM +0100, Boris Brezillon wrote: > > > > > On Fri, 30 Dec 2022 12:55:08 +0100 > > > > > Boris Brezillon wrote: > > > > > > > > > > > On Fri, 30 Dec 2022 11:20:42 +0100 > > > > > > Boris Brezillon wrote: > > > > > > > > > > > > > Hello Matthew, > > > > > > > > > > > > > > On Thu, 22 Dec 2022 14:21:11 -0800 > > > > > > > Matthew Brost wrote: > > > > > > > > > > > > > > > In XE, the new Intel GPU driver, a choice has made to have a 1 > > > > > > > > to 1 > > > > > > > > mapping between a drm_gpu_scheduler and drm_sched_entity. At > > > > > > > > first > > > > this > > > > > > > > seems a bit odd but let us explain the reasoning below. > > > > > > > > > > > > > > > > 1. In XE the submission order from multiple drm_sched_entity is > > > > > > > > not > > > > > > > > guaranteed to be the same completion even if targeting the same > > > > > > > > > > > > hardware > > > > > > > > engine. This is because in XE we have a firmware scheduler, the > > > > > > > > > > > > GuC, > > > > > > > > which allowed to reorder, timeslice, and preempt submissions. > > > > > > > > If a > > > > using > > > > > > > > shared drm_gpu_scheduler across multiple drm_sched_entity, the > > > > > > > > TDR > > > > falls > > > > > > > > apart as the TDR expects submission order == completion order. > > > > > > > > > > > > Using a > > > > > > > > dedicated drm_gpu_scheduler per drm_sched_entity solve this > > > > problem. > > > > > > > > > > > > > > Oh, that's interesting. I've been trying to solve the same sort of > > > > > > > issues to support Arm's new Mali GPU which is relying on a > > > > FW-assisted > > > > > > > scheduling scheme (you give the FW N streams to execute, and it > > > > > > > does > > > > > > > the scheduling between those N command streams, the kernel driver > > > > > > > does timeslice scheduling to update the command streams passed to > > > > > > > the > > > > > > > FW). I must admit I gave up on using drm_sched at some point, > > > > > > > mostly > > > > > > > because the integration with drm_sched was painful, but also > > > > > > > because > > > > I > > > > > > > felt trying to bend drm_sched to make it interact with a > > > > > > > timeslice-oriented scheduling model wasn't really future proof. > > > > > > > > > > > Giving > > > > > > > drm_sched_entity exlusive access to a drm_gpu_scheduler probably > > > > > > > > > > > might > > > > > > > help for a few things (didn't think it through yet), but I feel > > > > > > > it's > > > > > > > coming short on other aspects we have to deal with on Arm GPUs. > > > > > > > > > > > > > > > > > > > Ok, so I just had a quick look at the Xe driver and how it > > > > > > instantiates the drm_sched_entity and drm_gpu_scheduler, and I > > > > > > think I > > > > > > have a better understanding of how you get away with using drm_sched > > > > > > while still controlling how scheduling is really done. Here > > > > > > drm_gpu_scheduler is just a dummy abstract that let's you use the > > > > > > drm_sched job queuing/dep/tracking mechanism. The whole run-queue > > > > > > > > > > > > > > You nailed it here, we use the DRM scheduler for queuing jobs, > > > > dependency tracking and releasing jobs to be scheduled when dependencies > > > > are met, and lastly a tracking mechanism of inflights jobs that need to > > > > be cleaned up if an error occurs. It doesn't actually do any scheduling > > > > aside from the most basic level of not overflowing the submission ring > > > > buffer. In this sense, a 1 to 1 relationship between entity and > > > > scheduler fits quite well. > > > > > > > > > > Yeah, I think there's an annoying difference between what AMD/NVIDIA/Intel > > > want here and what you need for Arm thanks to the number of FW queues > > > available. I don't remember the exact number of GuC queues but it's at > > > least 1k. This puts it in an entirely different class from what you have > > > on > > > Mali. Roughly, there's about three categories here: > > > > > > 1. Hardware where the kernel is placing jobs on actual HW rings. This is > > > old Mali, Intel Haswell and earlier, and probably a bunch of others. > > > (Intel BDW+ with execlists is a weird case that doesn't fit in this > > > categorization.) > > > > > > 2. Hardware (or firmware) with a very limited number of queues where > > > you're going to have to juggle in the kernel in order to run desktop > > > Linux. > > > > > > 3. Firmware scheduling with a high queue count. In this case, you don't > > > want the kernel scheduling anything. Just throw
Re: [PATCH 1/4] memcg: Track exported dma-buffers
On Mon 09-01-23 21:38:04, T.J. Mercier wrote: > When a buffer is exported to userspace, use memcg to attribute the > buffer to the allocating cgroup until all buffer references are > released. > > Unlike the dmabuf sysfs stats implementation, this memcg accounting > avoids contention over the kernfs_rwsem incurred when creating or > removing nodes. I am not familiar with dmabuf infrastructure so please bear with me. AFAIU this patch adds a dmabuf specific counter to find out the amount of dmabuf memory used. But I do not see any actual charging implemented for that memory. I have looked at two random users of dma_buf_export cma_heap_allocate and it allocates pages to back the dmabuf (AFAIU) by cma_alloc which doesn't account to memcg, system_heap_allocate uses alloc_largest_available which relies on order_flags which doesn't seem to ever use __GFP_ACCOUNT. This would mean that the counter doesn't represent any actual memory reflected in the overall memory consumption of a memcg. I believe this is rather unexpected and confusing behavior. While some counters overlap and their sum would exceed the charged memory we do not have any that doesn't correspond to any memory (at least not for non-root memcgs). -- Michal Hocko SUSE Labs
Re: [RESEND PATCH] drm/mediatek: Add support for AR30 and BA30
Il 09/01/23 19:26, Justin Green ha scritto: Add support for AR30 and BA30 pixel formats to the Mediatek DRM driver. Tested using "modetest -P" on an MT8195. Signed-off-by: Justin Green Hello Justin, this commit does not apply against next-20230110 because of your own AFBC support addition :-) Can you please rebase and send a v2? Cheers, Angelo
Re: [PATCH 038/606] drm/i2c/ch7006: Convert to i2c's .probe_new()
Hello, I fatfingered my git tooling and got the author of this patch wrong. My intention is that the author is Uwe Kleine-König and not my other self with my private email address. Tell me if I should resend to simplify patch application. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 038/606] drm/i2c/ch7006: Convert to i2c's .probe_new()
Hello Uwe, On 1/10/23 10:06, Uwe Kleine-König wrote: > Hello, > > I fatfingered my git tooling and got the author of this patch wrong. My > intention is that the author is > > Uwe Kleine-König > That's what I thought but good to have a confirmation from you. > and not my other self with my private email address. Tell me if I should > resend to simplify patch application. > No need, I can amend that locally before pushing. Thanks! > Best regards > Uwe > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation
On 10/01/2023 08:23, Andrzej Hajda wrote:
On 09.01.2023 13:24, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Revert to the original explicit approach and document the reasoning
behind it.
v2:
* DG2 needs to be covered too. (Matt)
v3:
* Full version check for Gen12 to avoid catching all future platforms.
(Matt)
Signed-off-by: Tvrtko Ursulin
Cc: Matt Roper
Cc: Balasubramani Vivekanandan
Cc: Andrzej Hajda
Reviewed-by: Andrzej Hajda # v1
---
drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 75a7cb33..5521fa057aab 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct
intel_gt *gt)
unsigned int num = 0;
unsigned long flags;
- if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+ /*
+ * New platforms should not be added with catch-all-newer (>=)
+ * condition so that any later platform added triggers the below
warning
+ * and in turn mandates a human cross-check of whether the
invalidation
+ * flows have compatible semantics.
+ *
+ * For instance with the 11.00 -> 12.00 transition three out of five
+ * respective engine registers were moved to masked type. Then
after the
+ * 12.00 -> 12.50 transition multi cast handling is required too.
+ */
+
+ if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+ GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
regs = NULL;
num = ARRAY_SIZE(xehp_regs);
- } else if (GRAPHICS_VER(i915) == 12) {
+ } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
+ GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
MTL support is lost? IP_VER(12, 70)
AFAIU Matt says MTL is still incomplete anyway, so that would be added
in an explicit patch here.
And again it looks for me inconsistent, some unknown platforms are
covered, for example 12.54, some not, for example 12.11.
.11 and .54 as hypotheticals? You suggest this instead:
if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
regs = NULL;
num = ARRAY_SIZE(xehp_regs);
} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
regs = gen12_regs;
num = ARRAY_SIZE(gen12_regs);
?
It's fine by me if that covers all currently known platforms.
Regards,
Tvrtko
[PATCH] drm/mediatek: include missing headers
Fix the follow sparse warnings by adding missing headers: drivers/gpu/drm/mediatek/mtk_cec.c:251:24: sparse: warning: symbol 'mtk_cec_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:221:24: sparse: warning: symbol 'mtk_disp_ccorr_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_rdma.c:390:24: sparse: warning: symbol 'mtk_disp_rdma_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_gamma.c:209:24: sparse: warning: symbol 'mtk_disp_gamma_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_ovl.c:565:24: sparse: warning: symbol 'mtk_disp_ovl_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_color.c:164:24: sparse: warning: symbol 'mtk_disp_color_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_aal.c:161:24: sparse: warning: symbol 'mtk_disp_aal_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_dpi.c:1109:24: sparse: warning: symbol 'mtk_dpi_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_hdmi_ddc.c:340:24: sparse: warning: symbol 'mtk_hdmi_ddc_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_dsi.c:1223:24: sparse: warning: symbol 'mtk_dsi_driver' was not declared. Should it be static? Signed-off-by: Miles Chen --- drivers/gpu/drm/mediatek/mtk_cec.c| 2 ++ drivers/gpu/drm/mediatek/mtk_disp_aal.c | 1 + drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 1 + drivers/gpu/drm/mediatek/mtk_disp_color.c | 1 + drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 1 + drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 1 + drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 1 + drivers/gpu/drm/mediatek/mtk_dpi.c| 1 + drivers/gpu/drm/mediatek/mtk_dsi.c| 1 + drivers/gpu/drm/mediatek/mtk_hdmi_ddc.c | 3 +++ 10 files changed, 13 insertions(+) diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c b/drivers/gpu/drm/mediatek/mtk_cec.c index cdfa648910b2..b640bc0559e7 100644 --- a/drivers/gpu/drm/mediatek/mtk_cec.c +++ b/drivers/gpu/drm/mediatek/mtk_cec.c @@ -12,6 +12,8 @@ #include #include "mtk_cec.h" +#include "mtk_hdmi.h" +#include "mtk_drm_drv.h" #define TR_CONFIG 0x00 #define CLEAR_CEC_IRQ BIT(15) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c b/drivers/gpu/drm/mediatek/mtk_disp_aal.c index 0f9d7efb61d7..434e8a9ce8ab 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c @@ -14,6 +14,7 @@ #include "mtk_disp_drv.h" #include "mtk_drm_crtc.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_drv.h" #define DISP_AAL_EN0x #define AAL_EN BIT(0) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c index 3a53ebc4e172..1773379b2439 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c @@ -14,6 +14,7 @@ #include "mtk_disp_drv.h" #include "mtk_drm_crtc.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_drv.h" #define DISP_CCORR_EN 0x #define CCORR_EN BIT(0) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c b/drivers/gpu/drm/mediatek/mtk_disp_color.c index 473f5bb5cbad..cac9206079e7 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_color.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c @@ -14,6 +14,7 @@ #include "mtk_disp_drv.h" #include "mtk_drm_crtc.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_drv.h" #define DISP_COLOR_CFG_MAIN0x0400 #define DISP_COLOR_START_MT27010x0f00 diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c index bbd558a036ec..c844942603f7 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c @@ -14,6 +14,7 @@ #include "mtk_disp_drv.h" #include "mtk_drm_crtc.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_drv.h" #define DISP_GAMMA_EN 0x #define GAMMA_EN BIT(0) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c index 84daeaffab6a..9d8c986700ee 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c @@ -19,6 +19,7 @@ #include "mtk_disp_drv.h" #include "mtk_drm_crtc.h" #include "mtk_drm_ddp_comp.h" +#include "mtk_drm_drv.h" #define DISP_REG_OVL_INTEN 0x0004 #define OVL_FME_CPL_INTBIT(1) diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c index 0ec2e4049e07..a5a0c3bac35d 100644 --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c @@ -17
Re: [PATCH v2 2/3] drm/panel: boe-tv101wum-nl6: Reduce lcm_reset to send initial code time
Il 10/01/23 06:54, xinlei@mediatek.com ha scritto: From: Xinlei Lee Since the panel spec stipulates that the time from lcm_reset to DSI to send the initial code should be greater than 6ms and less than 40ms, so reduce the delay before sending the initial code and avoid panel exceptions. Please change the commit title to describe what you're doing. drm/panel: boe-tv101wum-nl6: Remove extra delay in init commands and the commit description should also contain something like Reduce the delay after LCM reset by removing an extra delay in the initialization commands array. The required delay of at least 6ms after reset is guaranteed by boe_panel_prepare(). Regards, Angelo
Re: [PATCH v2 3/3] drm/panel: boe-tv101wum-nl6: Fine tune the panel power sequence
Il 10/01/23 06:54, xinlei@mediatek.com ha scritto:
From: Xinlei Lee
For "boe,tv105wum-nw0" this special panel, it is stipulated in the
panel spec that MIPI needs to keep the LP11 state before the
lcm_reset pin is pulled high.
Signed-off-by: Xinlei Lee
---
drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index f0093035f1ff..67df61de64ae 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -36,6 +36,7 @@ struct panel_desc {
const struct panel_init_cmd *init_cmds;
unsigned int lanes;
bool discharge_on_disable;
+ bool lp11_before_reset;
};
struct boe_panel {
@@ -1261,6 +1262,10 @@ static int boe_panel_prepare(struct drm_panel *panel)
usleep_range(1, 11000);
+ if (boe->desc->lp11_before_reset) {
+ mipi_dsi_dcs_nop(boe->dsi);
NOP will never reach the driveric if it is in reset, which should apparently be
the state of it at that point in code.
I guess that you wanted to do that after LCM reset and before sending init cmds.
Regards,
Angelo
Re: [PATCH] drm/mxsfb: improve clk handling for axi clk
Hello Uwe, On 7/20/20 17:32, Uwe Kleine-König wrote: > Ignoring errors from devm_clk_get() is wrong. To handle not all platforms > having an axi clk use devm_clk_get_optional() instead and do proper error > handling. > > Also the clk API handles NULL as a dummy clk (which is also returned by > devm_clk_get_optional() if there is no clk) so there is no need to check > for NULL before calling clk_prepare_enable() or its counter part. > > Signed-off-by: Uwe Kleine-König Patch looks good to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v1 2/2] drm/virtio: Add the hotplug_mode_update property for rescanning of modes
On Fri, Jan 06, 2023 at 10:35:15AM +0100, Daniel Vetter wrote: > On Fri, Jan 06, 2023 at 09:56:40AM +0100, Gerd Hoffmann wrote: > > On Thu, Nov 17, 2022 at 05:30:54PM -0800, Vivek Kasireddy wrote: > > > Setting this property will allow the userspace to look for new modes or > > > position info when a hotplug event occurs. > > > > This works just fine for modes today. > > > > I assume this is this need to have userspace also check for position > > info updates added by patch #1)? > > What does this thing even do? Quick grep says qxl and vmwgfx also use > this, but it's not documented anywhere, and it's also not done with any > piece of common code. Which all looks really fishy. It's again a virtualization-specific thing. On physical hardware you typically have no idea which of your two monitors stands left and which stands right. On virtual hardware the host knows how the two windows for the two heads are arranged and can pass on that information to the guest. suggested_x/y properties added by patch #1 do pass that information to userspace so the display server can arrange things correctly without manual invention. I have no clue though why this hotplug_mode_update property exists in the first place and why mutter checks it. IMHO mutter could just check for suggested_x/y directly. take care, Gerd
Re: [PATCH v1 1/2] drm/virtio: Attach and set suggested_x/y properties for the connector
Hi,
> +static void virtio_gpu_update_output_position(struct virtio_gpu_output
> *output)
> +{
> + struct drm_connector *connector = &output->conn;
> + struct drm_device *dev = connector->dev;
> +
> + drm_object_property_set_value(&connector->base,
> + dev->mode_config.suggested_x_property, output->info.r.x);
> + drm_object_property_set_value(&connector->base,
> + dev->mode_config.suggested_y_property, output->info.r.y);
> +}
This fails sparse checking
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/virtio/virtgpu_vq.c:654:70: sparse: sparse: incorrect type
>> in argument 3 (different base types) @@ expected unsigned long long
>> [usertype] val @@ got restricted __le32 [usertype] x @@
drivers/gpu/drm/virtio/virtgpu_vq.c:654:70: sparse: expected unsigned
long long [usertype] val
drivers/gpu/drm/virtio/virtgpu_vq.c:654:70: sparse: got restricted
__le32 [usertype] x
>> drivers/gpu/drm/virtio/virtgpu_vq.c:656:70: sparse: sparse: incorrect type
>> in argument 3 (different base types) @@ expected unsigned long long
>> [usertype] val @@ got restricted __le32 [usertype] y @@
drivers/gpu/drm/virtio/virtgpu_vq.c:656:70: sparse: expected unsigned
long long [usertype] val
drivers/gpu/drm/virtio/virtgpu_vq.c:656:70: sparse: got restricted
__le32 [usertype] y
take care,
Gerd
Re: [PATCH] drm/mediatek: include missing headers
Il 10/01/23 10:16, Miles Chen ha scritto: Fix the follow sparse warnings by adding missing headers: drivers/gpu/drm/mediatek/mtk_cec.c:251:24: sparse: warning: symbol 'mtk_cec_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_ccorr.c:221:24: sparse: warning: symbol 'mtk_disp_ccorr_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_rdma.c:390:24: sparse: warning: symbol 'mtk_disp_rdma_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_gamma.c:209:24: sparse: warning: symbol 'mtk_disp_gamma_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_ovl.c:565:24: sparse: warning: symbol 'mtk_disp_ovl_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_color.c:164:24: sparse: warning: symbol 'mtk_disp_color_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_disp_aal.c:161:24: sparse: warning: symbol 'mtk_disp_aal_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_dpi.c:1109:24: sparse: warning: symbol 'mtk_dpi_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_hdmi_ddc.c:340:24: sparse: warning: symbol 'mtk_hdmi_ddc_driver' was not declared. Should it be static? drivers/gpu/drm/mediatek/mtk_dsi.c:1223:24: sparse: warning: symbol 'mtk_dsi_driver' was not declared. Should it be static? Signed-off-by: Miles Chen Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH] drm/mediatek: stop using 0 as NULL pointer
Il 10/01/23 04:12, Miles Chen ha scritto:
Use NULL for NULL pointer to fix the following sparse warning:
drivers/gpu/drm/mediatek/mtk_drm_gem.c:265:27: sparse: warning: Using plain
integer as NULL pointer
Signed-off-by: Miles Chen
Please add the appropriate tag...
Fixes: 3df64d7b0a4f ("drm/mediatek: Implement gem prime vmap/vunmap function")
after which:
Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH] drm/virtio: Fix GEM handle creation UAF
On 1/10/23 04:47, Rob Clark wrote:
> On Mon, Jan 9, 2023 at 3:28 PM Dmitry Osipenko
> wrote:
>>
>> On 12/17/22 02:33, Rob Clark wrote:
>>> From: Rob Clark
>>>
>>> Userspace can guess the handle value and try to race GEM object creation
>>> with handle close, resulting in a use-after-free if we dereference the
>>> object after dropping the handle's reference. For that reason, dropping
>>> the handle's reference must be done *after* we are done dereferencing
>>> the object.
>>>
>>> Signed-off-by: Rob Clark
>>> ---
>>> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 19 +--
>>> 1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> Added fixes/stable tags and applied this virtio-gpu patch to misc-fixes.
>> The Panfrost patch is untouched.
>
> Thanks.. the panfrost patch was not intended to be part of the same
> series (but apparently that is what happens when I send them at the
> same time), and was superceded by a patch from Steven Price (commit
> 4217c6ac8174 ("drm/panfrost: Fix GEM handle creation ref-counting")
> already applied to misc-fixes
Okay, I wanted to make clear what has been applied.
--
Best regards,
Dmitry
Re: [PATCH v3] drm/i915: Do not cover all future platforms in TLB invalidation
On 10.01.2023 10:16, Tvrtko Ursulin wrote:
On 10/01/2023 08:23, Andrzej Hajda wrote:
On 09.01.2023 13:24, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin
Revert to the original explicit approach and document the reasoning
behind it.
v2:
* DG2 needs to be covered too. (Matt)
v3:
* Full version check for Gen12 to avoid catching all future
platforms.
(Matt)
Signed-off-by: Tvrtko Ursulin
Cc: Matt Roper
Cc: Balasubramani Vivekanandan
Cc: Andrzej Hajda
Reviewed-by: Andrzej Hajda # v1
---
drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 75a7cb33..5521fa057aab 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct
intel_gt *gt)
unsigned int num = 0;
unsigned long flags;
- if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+ /*
+ * New platforms should not be added with catch-all-newer (>=)
+ * condition so that any later platform added triggers the
below warning
+ * and in turn mandates a human cross-check of whether the
invalidation
+ * flows have compatible semantics.
+ *
+ * For instance with the 11.00 -> 12.00 transition three out of
five
+ * respective engine registers were moved to masked type. Then
after the
+ * 12.00 -> 12.50 transition multi cast handling is required too.
+ */
+
+ if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50) &&
+ GRAPHICS_VER_FULL(i915) <= IP_VER(12, 55)) {
regs = NULL;
num = ARRAY_SIZE(xehp_regs);
- } else if (GRAPHICS_VER(i915) == 12) {
+ } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
+ GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
MTL support is lost? IP_VER(12, 70)
AFAIU Matt says MTL is still incomplete anyway, so that would be added
in an explicit patch here.
I've missed this part, sorry for the noise then :)
And as I see PVC is similar story.
And again it looks for me inconsistent, some unknown platforms are
covered, for example 12.54, some not, for example 12.11.
.11 and .54 as hypotheticals? You suggest this instead:
if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
regs = NULL;
num = ARRAY_SIZE(xehp_regs);
} else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
regs = gen12_regs;
num = ARRAY_SIZE(gen12_regs);
?
For me this perfectly follows the 'strict' approach :)
It's fine by me if that covers all currently known platforms.
My grep in i915_pci.c agrees.
Regards
Andrzej
Regards,
Tvrtko
Re: [PATCH v2 RESEND] adreno: Shutdown the GPU properly
On Mon, 9 Jan 2023 at 23:25, Joel Fernandes (Google)
wrote:
>
> During kexec on ARM device, we notice that device_shutdown() only calls
> pm_runtime_force_suspend() while shutting down the GPU. This means the GPU
> kthread is still running and further, there maybe active submits.
>
> This causes all kinds of issues during a kexec reboot:
>
> Warning from shutdown path:
>
> [ 292.509662] WARNING: CPU: 0 PID: 6304 at [...]
> adreno_runtime_suspend+0x3c/0x44
> [ 292.509863] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
> [ 292.509872] pstate: 8049 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 292.509881] pc : adreno_runtime_suspend+0x3c/0x44
> [ 292.509891] lr : pm_generic_runtime_suspend+0x30/0x44
> [ 292.509905] sp : ffc014473bf0
> [...]
> [ 292.510043] Call trace:
> [ 292.510051] adreno_runtime_suspend+0x3c/0x44
> [ 292.510061] pm_generic_runtime_suspend+0x30/0x44
> [ 292.510071] pm_runtime_force_suspend+0x54/0xc8
> [ 292.510081] adreno_shutdown+0x1c/0x28
> [ 292.510090] platform_shutdown+0x2c/0x38
> [ 292.510104] device_shutdown+0x158/0x210
> [ 292.510119] kernel_restart_prepare+0x40/0x4c
>
> And here from GPU kthread, an SError OOPs:
>
> [ 192.648789] el1h_64_error+0x7c/0x80
> [ 192.648812] el1_interrupt+0x20/0x58
> [ 192.648833] el1h_64_irq_handler+0x18/0x24
> [ 192.648854] el1h_64_irq+0x7c/0x80
> [ 192.648873] local_daif_inherit+0x10/0x18
> [ 192.648900] el1h_64_sync_handler+0x48/0xb4
> [ 192.648921] el1h_64_sync+0x7c/0x80
> [ 192.648941] a6xx_gmu_set_oob+0xbc/0x1fc
> [ 192.648968] a6xx_hw_init+0x44/0xe38
> [ 192.648991] msm_gpu_hw_init+0x48/0x80
> [ 192.649013] msm_gpu_submit+0x5c/0x1a8
> [ 192.649034] msm_job_run+0xb0/0x11c
> [ 192.649058] drm_sched_main+0x170/0x434
> [ 192.649086] kthread+0x134/0x300
> [ 192.649114] ret_from_fork+0x10/0x20
>
> Fix by calling adreno_system_suspend() in the device_shutdown() path.
>
> [ Applied Rob Clark feedback on fixing adreno_unbind() similarly, also
> tested as above. ]
>
> Cc: Rob Clark
> Cc: Steven Rostedt
> Cc: Ricardo Ribalda
> Cc: Ross Zwisler
Reviewed-by: Ricardo Ribalda
> Signed-off-by: Joel Fernandes (Google)
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 628806423f7d..36f062c7582f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -551,13 +551,14 @@ static int adreno_bind(struct device *dev, struct
> device *master, void *data)
> return 0;
> }
>
> +static int adreno_system_suspend(struct device *dev);
> static void adreno_unbind(struct device *dev, struct device *master,
> void *data)
> {
> struct msm_drm_private *priv = dev_get_drvdata(master);
> struct msm_gpu *gpu = dev_to_gpu(dev);
>
> - pm_runtime_force_suspend(dev);
> + WARN_ON_ONCE(adreno_system_suspend(dev));
> gpu->funcs->destroy(gpu);
>
> priv->gpu_pdev = NULL;
> @@ -609,7 +610,7 @@ static int adreno_remove(struct platform_device *pdev)
>
> static void adreno_shutdown(struct platform_device *pdev)
> {
> - pm_runtime_force_suspend(&pdev->dev);
> + WARN_ON_ONCE(adreno_system_suspend(&pdev->dev));
> }
>
> static const struct of_device_id dt_match[] = {
> --
> 2.39.0.314.g84b9a713c41-goog
>
--
Ricardo Ribalda
Re: [bug report] habanalabs: Timestamps buffers registration
On Mon, Jan 09, 2023 at 03:07:33PM +, Farah Kassabri wrote:
> > 2171 {
> > 2172 struct hl_ts_buff *ts_buff = NULL;
> > 2173 u32 size, num_elements;
> > 2174 void *p;
> > 2175
> > 2176 num_elements = *(u32 *)args;
> >
> > This business of passing void pointers and pretending that
> > hl_cb_mmap_mem_alloc() and hl_ts_alloc_buf() are the same function is a
> > nightmare.
> >
> > Create two ->alloc functions. Split hl_mmap_mem_buf_alloc() into one
> > function that allocates idr stuff. Create a function to free/remove the idr
> > stuff. Create two new helper function that call the idr function and then
> > the
> > appropriate alloc() function.
> >
> > It will be much cleaner than using a void pointer.
>
> I'm not sure I understood your intention.
> What void pointers are you referring to ? the args in this line rc =
> buf->behavior->alloc(buf, gfp, args); ?
> If yes what's so bad about it, it much simpler to have one common function
> and call specific implementation through pointers.
> BTW same goes to the map function also, not just the alloc (each behavior has
> alloc and map method)
>
Yeah, you're right. I didn't look at this carefully. I'm sorry.
> >
> > 2177
> > --> 2178 ts_buff = kzalloc(sizeof(*ts_buff), GFP_KERNEL);
> > ^^ Smatch is
> > correct that it should be
> > used here.
>
> Sure will be fixed.
>
> >
> > 2179 if (!ts_buff)
> > 2180 return -ENOMEM;
> > 2181
> > 2182 /* Allocate the user buffer */
> > 2183 size = num_elements * sizeof(u64);
> >
> > Can this have an integer overflow on 32bit systems?
>
> I'll define "size" as size_t instead of u32.
>
This can't actually overflow because it's checked in the caller.
Perhaps the careful way to write this is to change size to size_t as you
suggest which fixes the issue for 64bit systems and use size_mul() so it
doesn't overflow on 32bit systems either.
size = size_mul(num_elements, sizeof(u64));
But it doesn't really matter either way because num_elements is capped
in the caller.
regards,
dan carpenter
Re: [PATCH] drm/mxsfb: improve clk handling for axi clk
On 1/10/23 10:26, Javier Martinez Canillas wrote: > Hello Uwe, > > On 7/20/20 17:32, Uwe Kleine-König wrote: >> Ignoring errors from devm_clk_get() is wrong. To handle not all platforms >> having an axi clk use devm_clk_get_optional() instead and do proper error >> handling. >> >> Also the clk API handles NULL as a dummy clk (which is also returned by >> devm_clk_get_optional() if there is no clk) so there is no need to check >> for NULL before calling clk_prepare_enable() or its counter part. >> >> Signed-off-by: Uwe Kleine-König > > Patch looks good to me. > > Reviewed-by: Javier Martinez Canillas > I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 038/606] drm/i2c/ch7006: Convert to i2c's .probe_new()
On 11/18/22 23:36, Uwe Kleine-König wrote: > The probe function doesn't make use of the i2c_device_id * parameter so it > can be trivially converted. > > Signed-off-by: Uwe Kleine-König > --- I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 15/15] backlight: backlight: Drop the deprecated fb_blank property
On Mon, 09 Jan 2023, Sam Ravnborg wrote: > Hi Daniel. > > On Mon, Jan 09, 2023 at 11:06:35AM +, Daniel Thompson wrote: > > On Sat, Jan 07, 2023 at 07:26:29PM +0100, Sam Ravnborg via B4 Submission > > Endpoint wrote: > > > From: Sam Ravnborg > > > > > > With all users gone remove the deprecated fb_blank member in > > > backlight_properties. > > > > > > Signed-off-by: Sam Ravnborg > > > Cc: Lee Jones > > > Cc: Daniel Thompson > > > Cc: Jingoo Han > > > > > > Reviewed-by: Daniel Thompson > > Thanks for the follow-up on all the backlight related patches. > > > > > > > PS Please don't treat this like a maintainer Acked-by: and merge it > >(Lee's not on holiday so work with Lee to figure out the merge > >strategy ;-) ). > Nope, I am aware that the usual pattern here and wait for Lee to show > up. It's on the list. Only 50 more reviews in the backlog now! > For this patch there is a bug as I need to update a comment. > I will fix this when I resend after all the patches in flight has > landed. So likely after the next merge window, -- Lee Jones [李琼斯]
Re: [PATCH 039/606] drm/i2c/sil164: Convert to i2c's .probe_new()
On 11/18/22 23:36, Uwe Kleine-König wrote: > From: Uwe Kleine-König > > The probe function doesn't make use of the i2c_device_id * parameter so it > can be trivially converted. > > Signed-off-by: Uwe Kleine-König > --- I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 040/606] drm/i2c/tda9950: Convert to i2c's .probe_new()
On 11/18/22 23:36, Uwe Kleine-König wrote: > From: Uwe Kleine-König > > The probe function doesn't make use of the i2c_device_id * parameter so it > can be trivially converted. > > Signed-off-by: Uwe Kleine-König > -- I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 041/606] drm/i2c/tda998x: Convert to i2c's .probe_new()
On 11/18/22 23:36, Uwe Kleine-König wrote: > From: Uwe Kleine-König > > The probe function doesn't make use of the i2c_device_id * parameter so it > can be trivially converted. > > Signed-off-by: Uwe Kleine-König > --- I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 042/606] drm/panel: olimex-lcd-olinuxino: Convert to i2c's .probe_new()
On 11/18/22 23:36, Uwe Kleine-König wrote: > From: Uwe Kleine-König > > The probe function doesn't make use of the i2c_device_id * parameter so it > can be trivially converted. > > Signed-off-by: Uwe Kleine-König > --- I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 043/606] drm/panel: raspberrypi-touchscreen: Convert to i2c's .probe_new()
On 11/18/22 23:36, Uwe Kleine-König wrote: > From: Uwe Kleine-König > > The probe function doesn't make use of the i2c_device_id * parameter so it > can be trivially converted. > > Signed-off-by: Uwe Kleine-König > --- I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH v3 0/7] drm/bridge_connector: perform HPD enablement automatically
On Tue, Jan 10, 2023 at 8:07 AM Laurentiu Palcu
wrote:
> On Mon, Jan 09, 2023 at 10:26:28PM +0200, Dmitry Baryshkov wrote:
> > On 09/01/2023 18:21, Laurentiu Palcu wrote:
> > > It looks like there are some issues with this patchset... :/ I just
> > > fetched the drm-tip and, with these patches included, the "Hot plug
> > > detection already enabled" warning is back for i.MX DCSS.
> >
> > Could you please provide a backtrace?
>
> Sure, see below:
>
> [ cut here ]
> Hot plug detection already enabled
> WARNING: CPU: 2 PID: 9 at drivers/gpu/drm/drm_bridge.c:1257
> drm_bridge_hpd_enable+0x94/0x9c [drm]
> Modules linked in: videobuf2_memops snd_soc_simple_card
> snd_soc_simple_card_utils fsl_imx8_ddr_perf videobuf2_common
> snd_soc_imx_spdif adv7511 etnaviv imx8m_ddrc imx_dcss mc cec nwl_dsi gov
> CPU: 2 PID: 9 Comm: kworker/u8:0 Not tainted 6.2.0-rc2-15208-g25b283acd578 #6
> Hardware name: NXP i.MX8MQ EVK (DT)
> Workqueue: events_unbound deferred_probe_work_func
> pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : drm_bridge_hpd_enable+0x94/0x9c [drm]
> lr : drm_bridge_hpd_enable+0x94/0x9c [drm]
> sp : 89ef3740
> x29: 89ef3740 x28: 09331f00 x27: 1000
> x26: 0020 x25: 81148ed8 x24: 0a8fe000
> x23: fffd x22: 05086348 x21: 81133ee0
> x20: 0550d800 x19: 05086288 x18: 0006
> x17: x16: 896ef008 x15: 972891004260
> x14: 2a1403e19400 x13: 972891004260 x12: 2a1403e19400
> x11: 7100385f29400801 x10: 0aa0 x9 : 88112744
> x8 : 00250b00 x7 : 0003 x6 : 0011
> x5 : x4 : bd986a48 x3 : 0001
> x2 : x1 : x0 : 0025
> Call trace:
> drm_bridge_hpd_enable+0x94/0x9c [drm]
> drm_bridge_connector_enable_hpd+0x2c/0x3c [drm_kms_helper]
> drm_kms_helper_poll_enable+0x94/0x10c [drm_kms_helper]
> drm_helper_probe_single_connector_modes+0x1a8/0x510 [drm_kms_helper]
> drm_client_modeset_probe+0x204/0x1190 [drm]
> __drm_fb_helper_initial_config_and_unlock+0x5c/0x4a4 [drm_kms_helper]
> drm_fb_helper_initial_config+0x54/0x6c [drm_kms_helper]
> drm_fbdev_client_hotplug+0xd0/0x140 [drm_kms_helper]
> drm_fbdev_generic_setup+0x90/0x154 [drm_kms_helper]
> dcss_kms_attach+0x1c8/0x254 [imx_dcss]
> dcss_drv_platform_probe+0x90/0xfc [imx_dcss]
> platform_probe+0x70/0xcc
> really_probe+0xc4/0x2e0
> __driver_probe_device+0x80/0xf0
> driver_probe_device+0xe0/0x164
> __device_attach_driver+0xc0/0x13c
> bus_for_each_drv+0x84/0xe0
> __device_attach+0xa4/0x1a0
> device_initial_probe+0x1c/0x30
> bus_probe_device+0xa4/0xb0
> deferred_probe_work_func+0x90/0xd0
> process_one_work+0x200/0x474
> worker_thread+0x74/0x43c
> kthread+0xfc/0x110
> ret_from_fork+0x10/0x20
> ---[ end trace ]---
I get a similar trace on R-Car Gen2 (Koelsch with R-Car M2-W) and
Gen3 (Salvator-XS with R-Car H3 ES2.0), and bisected it to commit
92d755d8f13b6791 ("drm/bridge_connector: rely on drm_kms_helper_poll_*
for HPD enablement") in drm-misc/for-linux-next.
As I do not have any displays connected, I do not know what is the
full impact.
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 v3 1/1] drm/i915/gt: Start adding module oriented dmesg output
Hi John,
[...]
> +#define gt_WARN_ON(_gt, _condition) \
> + gt_WARN(_gt, _condition, "%s", "gt_WARN_ON(" __stringify(_condition)
> ")")
> +
> +#define gt_WARN_ON_ONCE(_gt, _condition) \
> + gt_WARN_ONCE(_gt, _condition, "%s", "gt_WARN_ONCE("
> __stringify(_condition) ")")
> +
> +#define gt_WARN(_gt, _condition, _fmt, ...) \
> + drm_WARN(&(_gt)->i915->drm, _condition, "GT%u: " _fmt, (_gt)->info.id,
> ##__VA_ARGS__)
> +
> +#define gt_WARN_ONCE(_gt, _condition, _fmt, ...) \
> + drm_WARN_ONCE(&(_gt)->i915->drm, _condition, "GT%u: " _fmt,
> (_gt)->info.id, ##__VA_ARGS__)
do we need some order here? gt_WARN and gt_WARN_ONCE should go
before respectively gt_WARN_ON and gt_WARN_ON_ONCE.
The rest looks good.
Andi
Re: [PATCH] drm/mxsfb: improve clk handling for axi clk
On 1/10/23 11:06, Javier Martinez Canillas wrote: On 1/10/23 10:26, Javier Martinez Canillas wrote: Hello Uwe, On 7/20/20 17:32, Uwe Kleine-König wrote: Ignoring errors from devm_clk_get() is wrong. To handle not all platforms having an axi clk use devm_clk_get_optional() instead and do proper error handling. Also the clk API handles NULL as a dummy clk (which is also returned by devm_clk_get_optional() if there is no clk) so there is no need to check for NULL before calling clk_prepare_enable() or its counter part. Signed-off-by: Uwe Kleine-König Patch looks good to me. Reviewed-by: Javier Martinez Canillas I've pushed this to drm-misc (dri-misc-next) now. Thanks! Thanks, I admit, I missed the patch, sorry. It does indeed look correct. Reviewed-by: Marek Vasut
Re: [PATCH][next] habanalabs: Replace zero-length arrays with flexible-array members
On Mon, Jan 09, 2023 at 07:39:47PM -0600, Gustavo A. R. Silva wrote: > Zero-length arrays are deprecated[1] and we are moving towards > adopting C99 flexible-array members instead. So, replace zero-length > arrays in a couple of structures with flex-array members. > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > routines on memcpy() and help us make progress towards globally > enabling -fstrict-flex-arrays=3 [2]. > > Link: > https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays > [1] > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [2] > Link: https://github.com/KSPP/linux/issues/78 > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Stanislaw Gruszka
Re: [PATCH v3] drm: Only select I2C_ALGOBIT for drivers that actually need it
On 12/19/22 09:49, Javier Martinez Canillas wrote: > Hello Uwe, > > On 12/19/22 09:36, Uwe Kleine-König wrote: >> While working on a drm driver that doesn't need the i2c algobit stuff I >> noticed that DRM selects this code even though only 8 drivers actually use >> it. While also only some drivers use i2c, keep the select for I2C for the >> next cleanup patch. Still prepare this already by also selecting I2C for >> the individual drivers. >> >> Signed-off-by: Uwe Kleine-König >> --- > > Thanks for sending a v3 of this. > > Reviewed-by: Javier Martinez Canillas > I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] MAINTAINERS: drm/hisilicon: Drop Chen Feng
On 12/19/22 10:04, Javier Martinez Canillas wrote: > On 12/19/22 09:53, Uwe Kleine-König wrote: >> The listed address doesn't work any more: >> >> puck.c...@hisilicon.com >> host mx5.hisilicon.com [124.71.93.234] >> SMTP error from remote mail server after RCPT >> TO:: >> 551 5.1.1 : Recipient address rejected: >> Failed recipient validation check.: host 127.0.0.1[127.0.0.1] said: >> 554 5.7.1 recipient verify from ldap failed (in reply to RCPT TO command) >> >> Signed-off-by: Uwe Kleine-König >> --- > > Reviewed-by: Javier Martinez Canillas > I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
[RFC DO NOT MERGE] treewide: use __xchg in most obvious places
This patch tries to show usability of __xchg helper.
It is not intended to be merged, but I can convert
it to proper patchset if necessary.
There are many more places where __xchg can be used.
This demo shows the most spectacular cases IMHO:
- previous value is returned from function,
- temporary variables are in use.
As a result readability is much better and diffstat is quite
nice, less local vars to look at.
In many cases whole body of functions is replaced
with __xchg(ptr, val), so as further refactoring the whole
function can be removed and __xchg can be called directly.
Signed-off-by: Andrzej Hajda
---
arch/arm/probes/uprobes/core.c| 8 ++--
arch/csky/kernel/probes/uprobes.c | 9 ++---
arch/mips/kernel/irq_txx9.c | 7 ++-
arch/mips/kernel/process.c| 8 +++-
arch/mips/kernel/uprobes.c| 10 ++
arch/powerpc/include/asm/kvm_ppc.h| 7 ++-
arch/powerpc/kernel/uprobes.c | 10 ++
arch/powerpc/mm/init_64.c | 7 ++-
arch/riscv/kernel/probes/uprobes.c| 9 ++---
arch/s390/kernel/uprobes.c| 7 ++-
arch/s390/kvm/interrupt.c | 6 ++
arch/sh/kernel/traps_32.c | 6 ++
.../accessibility/speakup/speakup_dectlk.c| 7 ++-
drivers/accessibility/speakup/speakup_soft.c | 7 ++-
drivers/block/drbd/drbd_receiver.c| 5 ++---
drivers/cdrom/cdrom.c | 7 ++-
drivers/gpu/drm/drm_atomic_uapi.c | 14 +++---
drivers/iommu/iova.c | 7 ++-
drivers/misc/ti-st/st_core.c | 10 +++---
drivers/mtd/nand/raw/qcom_nandc.c | 11 ---
drivers/net/ethernet/ibm/ehea/ehea_main.c | 11 +++
.../microchip/sparx5/sparx5_calendar.c| 10 --
drivers/net/usb/rtl8150.c | 9 +++--
drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 8 +++-
.../net/wireless/marvell/mwifiex/sta_ioctl.c | 7 +++
drivers/scsi/device_handler/scsi_dh_alua.c| 8 +++-
drivers/scsi/lpfc/lpfc_sli.c | 7 ++-
drivers/staging/rtl8192e/rtllib_tx.c | 7 ++-
drivers/tty/hvc/hvc_iucv.c| 8 +++-
drivers/video/fbdev/sis/sis_main.c| 6 ++
drivers/xen/grant-table.c | 6 ++
fs/namespace.c| 6 ++
include/linux/ptr_ring.h | 7 ++-
include/linux/qed/qed_chain.h | 19 +++
io_uring/io_uring.c | 7 ++-
mm/kmsan/init.c | 7 ++-
mm/memcontrol.c | 8 ++--
net/mac80211/rc80211_minstrel_ht.c| 6 ++
sound/pci/asihpi/hpidebug.c | 8 +++-
.../selftests/bpf/progs/dummy_st_ops.c| 7 ++-
40 files changed, 99 insertions(+), 225 deletions(-)
diff --git a/arch/arm/probes/uprobes/core.c b/arch/arm/probes/uprobes/core.c
index f5f790c6e5f896..77ce8ae431376d 100644
--- a/arch/arm/probes/uprobes/core.c
+++ b/arch/arm/probes/uprobes/core.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include
#include
@@ -61,12 +62,7 @@ unsigned long
arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
struct pt_regs *regs)
{
- unsigned long orig_ret_vaddr;
-
- orig_ret_vaddr = regs->ARM_lr;
- /* Replace the return addr with trampoline addr */
- regs->ARM_lr = trampoline_vaddr;
- return orig_ret_vaddr;
+ return __xchg(®s->ARM_lr, trampoline_vaddr);
}
int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
diff --git a/arch/csky/kernel/probes/uprobes.c
b/arch/csky/kernel/probes/uprobes.c
index 2d31a12e46cfee..775fe88b5f0016 100644
--- a/arch/csky/kernel/probes/uprobes.c
+++ b/arch/csky/kernel/probes/uprobes.c
@@ -3,6 +3,7 @@
* Copyright (C) 2014-2016 Pratyush Anand
*/
#include
+#include
#include
#include
#include
@@ -123,13 +124,7 @@ unsigned long
arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
struct pt_regs *regs)
{
- unsigned long ra;
-
- ra = regs->lr;
-
- regs->lr = trampoline_vaddr;
-
- return ra;
+ return __xchg(®s->lr, trampoline_vaddr);
}
int arch_uprobe_exception_notify(struct notifier_block *self,
diff --git a/arch/mips/kernel/irq_txx9.c b/arch/mips/kernel/irq_txx9.c
index af3ef4c9f7de1e..b5abe24ea7cfb9 100644
--- a/arch/mips/kernel/irq_txx9.c
+++ b/arch/mips/kernel/irq_txx9.c
@@ -15,6 +15,7 @@
*/
#include
#include
+#include
#include
#include
#include
@@ -159,13 +160,9 @@ void __init txx9_irq_init(unsigned long baseaddr)
int __init txx9_irq_set_pri(in
Re: [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:
> This patch tries to show usability of __xchg helper.
> It is not intended to be merged, but I can convert
> it to proper patchset if necessary.
>
> There are many more places where __xchg can be used.
> This demo shows the most spectacular cases IMHO:
> - previous value is returned from function,
> - temporary variables are in use.
>
> As a result readability is much better and diffstat is quite
> nice, less local vars to look at.
> In many cases whole body of functions is replaced
> with __xchg(ptr, val), so as further refactoring the whole
> function can be removed and __xchg can be called directly.
...
> arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
> struct pt_regs *regs)
> {
> - unsigned long orig_ret_vaddr;
> -
> - orig_ret_vaddr = regs->ARM_lr;
> - /* Replace the return addr with trampoline addr */
> - regs->ARM_lr = trampoline_vaddr;
> - return orig_ret_vaddr;
> + return __xchg(®s->ARM_lr, trampoline_vaddr);
> }
If it's not a callback, the entire function can be killed.
And this is a good example of the function usage.
OTOH, these places might have a side effect (if it's in deep CPU
handlers), means we need to do this carefully.
...
> static inline void *qed_chain_produce(struct qed_chain *p_chain)
> {
> - void *p_ret = NULL, *p_prod_idx, *p_prod_page_idx;
> + void *p_prod_idx, *p_prod_page_idx;
>
> if (is_chain_u16(p_chain)) {
> if ((p_chain->u.chain16.prod_idx &
> @@ -390,11 +391,8 @@ static inline void *qed_chain_produce(struct qed_chain
> *p_chain)
> p_chain->u.chain32.prod_idx++;
> }
>
> - p_ret = p_chain->p_prod_elem;
> - p_chain->p_prod_elem = (void *)(((u8 *)p_chain->p_prod_elem) +
> - p_chain->elem_size);
> -
> - return p_ret;
> + return __xchg(&p_chain->p_prod_elem,
> + (void *)(((u8 *)p_chain->p_prod_elem) +
> p_chain->elem_size));
Wondering if you still need a (void *) casting after the change. Ditto for the
rest of similar cases.
> }
...
Btw, is it done by coccinelle? If no, why not providing the script?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] drm/vc4: dsi: Drop unused i2c include
On 12/19/22 09:49, Javier Martinez Canillas wrote: > On 12/19/22 09:40, Uwe Kleine-König wrote: >> The driver doesn't make use of any symbol provided by . So >> drop the include. >> >> Signed-off-by: Uwe Kleine-König >> --- > > Reviewed-by: Javier Martinez Canillas > I've pushed this to drm-misc (dri-misc-next) now. Thanks! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
On 09/01/2023 17:27, Jason Ekstrand wrote:
[snip]
>>> AFAICT it proposes to have 1:1 between *userspace* created
contexts (per
>>> context _and_ engine) and drm_sched. I am not sure avoiding
invasive changes
>>> to the shared code is in the spirit of the overall idea and instead
>>> opportunity should be used to look at way to refactor/improve
drm_sched.
Maybe? I'm not convinced that what Xe is doing is an abuse at all or
really needs to drive a re-factor. (More on that later.) There's only
one real issue which is that it fires off potentially a lot of kthreads.
Even that's not that bad given that kthreads are pretty light and you're
not likely to have more kthreads than userspace threads which are much
heavier. Not ideal, but not the end of the world either. Definitely
something we can/should optimize but if we went through with Xe without
this patch, it would probably be mostly ok.
>> Yes, it is 1:1 *userspace* engines and drm_sched.
>>
>> I'm not really prepared to make large changes to DRM scheduler
at the
>> moment for Xe as they are not really required nor does Boris
seem they
>> will be required for his work either. I am interested to see
what Boris
>> comes up with.
>>
>>> Even on the low level, the idea to replace drm_sched threads
with workers
>>> has a few problems.
>>>
>>> To start with, the pattern of:
>>>
>>> while (not_stopped) {
>>> keep picking jobs
>>> }
>>>
>>> Feels fundamentally in disagreement with workers (while
obviously fits
>>> perfectly with the current kthread design).
>>
>> The while loop breaks and worker exists if no jobs are ready.
I'm not very familiar with workqueues. What are you saying would fit
better? One scheduling job per work item rather than one big work item
which handles all available jobs?
Yes and no, it indeed IMO does not fit to have a work item which is
potentially unbound in runtime. But it is a bit moot conceptual mismatch
because it is a worst case / theoretical, and I think due more
fundamental concerns.
If we have to go back to the low level side of things, I've picked this
random spot to consolidate what I have already mentioned and perhaps expand.
To start with, let me pull out some thoughts from workqueue.rst:
"""
Generally, work items are not expected to hog a CPU and consume many
cycles. That means maintaining just enough concurrency to prevent work
processing from stalling should be optimal.
"""
For unbound queues:
"""
The responsibility of regulating concurrency level is on the users.
"""
Given the unbound queues will be spawned on demand to service all queued
work items (more interesting when mixing up with the system_unbound_wq),
in the proposed design the number of instantiated worker threads does
not correspond to the number of user threads (as you have elsewhere
stated), but pessimistically to the number of active user contexts. That
is the number which drives the maximum number of not-runnable jobs that
can become runnable at once, and hence spawn that many work items, and
in turn unbound worker threads.
Several problems there.
It is fundamentally pointless to have potentially that many more threads
than the number of CPU cores - it simply creates a scheduling storm.
Unbound workers have no CPU / cache locality either and no connection
with the CPU scheduler to optimize scheduling patterns. This may matter
either on large systems or on small ones. Whereas the current design
allows for scheduler to notice userspace CPU thread keeps waking up the
same drm scheduler kernel thread, and so it can keep them on the same
CPU, the unbound workers lose that ability and so 2nd CPU might be
getting woken up from low sleep for every submission.
Hence, apart from being a bit of a impedance mismatch, the proposal has
the potential to change performance and power patterns and both large
and small machines.
>>> Secondly, it probably demands separate workers (not optional),
otherwise
>>> behaviour of shared workqueues has either the potential to
explode number
>>> kernel threads anyway, or add latency.
>>>
>>
>> Right now the system_unbound_wq is used which does have a limit
on the
>> number of threads, right? I do have a FIXME to allow a worker to be
>> passed in similar to TDR.
>>
>> WRT to latency, the 1:1 ratio could actually have lower latency
as 2 GPU
>> schedulers can be pushing jobs into the backend / cleaning up
jobs in
>> parallel.
>>
>
> Thought of one more point here where why in Xe we absolutely want
a 1 to
> 1 ratio between entity and scheduler - the way we implement
timeslicing
> for preempt fences.
>
> Let me try to explain.
>
> Preempt fences are implemented via the generic messaging
[PATCH v4] drm/i915: Do not cover all future platforms in TLB invalidation
From: Tvrtko Ursulin
Revert to the original explicit approach and document the reasoning
behind it.
v2:
* DG2 needs to be covered too. (Matt)
v3:
* Full version check for Gen12 to avoid catching all future platforms.
(Matt)
v4:
* Be totally explicit on the Gen12 branch. (Andrzej)
Signed-off-by: Tvrtko Ursulin
Cc: Matt Roper
Cc: Balasubramani Vivekanandan
Cc: Andrzej Hajda
Reviewed-by: Andrzej Hajda # v1
Reviewed-by: Matt Roper # v3
---
drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 75a7cb33..5721bf85d119 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
unsigned int num = 0;
unsigned long flags;
- if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
+ /*
+* New platforms should not be added with catch-all-newer (>=)
+* condition so that any later platform added triggers the below warning
+* and in turn mandates a human cross-check of whether the invalidation
+* flows have compatible semantics.
+*
+* For instance with the 11.00 -> 12.00 transition three out of five
+* respective engine registers were moved to masked type. Then after the
+* 12.00 -> 12.50 transition multi cast handling is required too.
+*/
+
+ if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
+ GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
regs = NULL;
num = ARRAY_SIZE(xehp_regs);
- } else if (GRAPHICS_VER(i915) == 12) {
+ } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
+ GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
regs = gen12_regs;
num = ARRAY_SIZE(gen12_regs);
} else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
--
2.34.1
Re: [Intel-gfx] [PATCH v3 1/1] drm/i915/gt: Start adding module oriented dmesg output
On 09/01/2023 23:48, john.c.harri...@intel.com wrote:
From: John Harrison
When trying to analyse bug reports from CI, customers, etc. it can be
difficult to work out exactly what is happening on which GT in a
multi-GT system. So add GT oriented debug/error message wrappers. If
used instead of the drm_ equivalents, you get the same output but with
a GT# prefix on it.
v2: Go back to using lower case names (combined review feedback).
Convert intel_gt.c as a first step.
v3: Add gt_err_ratelimited() as well, undo one conversation that might
not have a GT pointer in some scenarios (review feedback from Michal W).
Split definitions into separate header (review feedback from Jani).
Convert all intel_gt*.c files.
Signed-off-by: John Harrison
---
drivers/gpu/drm/i915/gt/intel_gt.c| 96 +--
.../gpu/drm/i915/gt/intel_gt_clock_utils.c| 8 +-
drivers/gpu/drm/i915/gt/intel_gt_irq.c| 9 +-
drivers/gpu/drm/i915/gt/intel_gt_mcr.c| 9 +-
drivers/gpu/drm/i915/gt/intel_gt_pm.c | 9 +-
drivers/gpu/drm/i915/gt/intel_gt_print.h | 51 ++
drivers/gpu/drm/i915/gt/intel_gt_sysfs.c | 4 +-
drivers/gpu/drm/i915/gt/intel_gt_sysfs_pm.c | 34 ++-
drivers/gpu/drm/i915/gt/intel_gtt.c | 7 +-
9 files changed, 129 insertions(+), 98 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gt/intel_gt_print.h
diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
b/drivers/gpu/drm/i915/gt/intel_gt.c
index 75a7cb33c..bb6152da89706 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt.c
@@ -22,6 +22,7 @@
#include "intel_gt_debugfs.h"
#include "intel_gt_mcr.h"
#include "intel_gt_pm.h"
+#include "intel_gt_print.h"
#include "intel_gt_regs.h"
#include "intel_gt_requests.h"
#include "intel_migrate.h"
@@ -89,9 +90,8 @@ static int intel_gt_probe_lmem(struct intel_gt *gt)
if (err == -ENODEV)
return 0;
- drm_err(&i915->drm,
- "Failed to setup region(%d) type=%d\n",
- err, INTEL_MEMORY_LOCAL);
+ gt_err(gt, "Failed to setup region(%d) type=%d\n",
+ err, INTEL_MEMORY_LOCAL);
return err;
}
@@ -200,14 +200,14 @@ int intel_gt_init_hw(struct intel_gt *gt)
ret = i915_ppgtt_init_hw(gt);
if (ret) {
- drm_err(&i915->drm, "Enabling PPGTT failed (%d)\n", ret);
+ gt_err(gt, "Enabling PPGTT failed (%d)\n", ret);
goto out;
}
/* We can't enable contexts until all firmware is loaded */
ret = intel_uc_init_hw(>->uc);
if (ret) {
- i915_probe_error(i915, "Enabling uc failed (%d)\n", ret);
+ gt_probe_error(gt, "Enabling uc failed (%d)\n", ret);
goto out;
}
@@ -257,7 +257,7 @@ intel_gt_clear_error_registers(struct intel_gt *gt,
* some errors might have become stuck,
* mask them.
*/
- drm_dbg(>->i915->drm, "EIR stuck: 0x%08x, masking\n", eir);
+ gt_dbg(gt, "EIR stuck: 0x%08x, masking\n", eir);
intel_uncore_rmw(uncore, EMR, 0, eir);
intel_uncore_write(uncore, GEN2_IIR,
I915_MASTER_ERROR_INTERRUPT);
@@ -291,16 +291,16 @@ static void gen6_check_faults(struct intel_gt *gt)
for_each_engine(engine, gt, id) {
fault = GEN6_RING_FAULT_REG_READ(engine);
if (fault & RING_FAULT_VALID) {
- drm_dbg(&engine->i915->drm, "Unexpected fault\n"
- "\tAddr: 0x%08lx\n"
- "\tAddress space: %s\n"
- "\tSource ID: %d\n"
- "\tType: %d\n",
- fault & PAGE_MASK,
- fault & RING_FAULT_GTTSEL_MASK ?
- "GGTT" : "PPGTT",
- RING_FAULT_SRCID(fault),
- RING_FAULT_FAULT_TYPE(fault));
+ gt_dbg(gt, "Unexpected fault\n"
+ "\tAddr: 0x%08lx\n"
+ "\tAddress space: %s\n"
+ "\tSource ID: %d\n"
+ "\tType: %d\n",
+ fault & PAGE_MASK,
+ fault & RING_FAULT_GTTSEL_MASK ?
+ "GGTT" : "PPGTT",
+ RING_FAULT_SRCID(fault),
+ RING_FAULT_FAULT_TYPE(fault));
}
}
}
@@ -327,17 +327,17 @@ static void xehp_check_faults(struct intel_gt *gt)
fault_addr = ((u64)(fault_data1 & FAULT_VA_HIGH_BITS) << 44) |
((u64)fault_data0 << 12);
- drm_dbg(>->i915->drm, "Unexpected fault\n"
-
Re: [PATCH] doc: add dma-buf IOCTL code to table
On Tuesday, November 29th, 2022 at 10:56, Christian König wrote: > Should I also push this? I can push to drm-misc-next, but is that the suitable repo?
Re: [PATCH 1/7] drm/atomic: log drm_atomic_replace_property_blob_from_id() errors
Ping
Re: [PATCH] doc: add dma-buf IOCTL code to table
Am 10.01.23 um 12:49 schrieb Simon Ser: On Tuesday, November 29th, 2022 at 10:56, Christian König wrote: Should I also push this? I can push to drm-misc-next, but is that the suitable repo? I think so, unless you think that this is a necessary bug fix which should be backported. Christian.
Re: [PATCH] doc: add dma-buf IOCTL code to table
On Tuesday, January 10th, 2023 at 12:53, Christian König wrote: > Am 10.01.23 um 12:49 schrieb Simon Ser: > > > On Tuesday, November 29th, 2022 at 10:56, Christian König > > christian.koe...@amd.com wrote: > > > > > Should I also push this? > > > I can push to drm-misc-next, but is that the suitable repo? > > I think so, unless you think that this is a necessary bug fix which > should be backported. Thanks! I pushed it there.
Re: [PATCH] drm: Alloc high address for drm buddy topdown flag
On 07/01/2023 15:15, Arunpravin Paneer Selvam wrote:
As we are observing low numbers in viewperf graphics benchmark, we
are strictly not allowing the top down flag enabled allocations
to steal the memory space from cpu visible region.
The approach is, we are sorting each order list entries in
ascending order and compare the last entry of each order
list in the freelist and return the max block.
Did you also run the selftests? Does everything still pass and complete
in a reasonable amount of time?
This patch improves the viewperf 3D benchmark scores.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/drm_buddy.c | 81 -
1 file changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 11bb59399471..50916b2f2fc5 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -38,6 +38,25 @@ static void drm_block_free(struct drm_buddy *mm,
kmem_cache_free(slab_blocks, block);
}
+static void list_insert_sorted(struct drm_buddy *mm,
+ struct drm_buddy_block *block)
+{
+ struct drm_buddy_block *node;
+ struct list_head *head;
+
+ head = &mm->free_list[drm_buddy_block_order(block)];
+ if (list_empty(head)) {
+ list_add(&block->link, head);
+ return;
+ }
+
+ list_for_each_entry(node, head, link)
+ if (drm_buddy_block_offset(block) <
drm_buddy_block_offset(node))
+ break;
+
+ __list_add(&block->link, node->link.prev, &node->link);
+}
+
static void mark_allocated(struct drm_buddy_block *block)
{
block->header &= ~DRM_BUDDY_HEADER_STATE;
@@ -52,8 +71,7 @@ static void mark_free(struct drm_buddy *mm,
block->header &= ~DRM_BUDDY_HEADER_STATE;
block->header |= DRM_BUDDY_FREE;
- list_add(&block->link,
-&mm->free_list[drm_buddy_block_order(block)]);
+ list_insert_sorted(mm, block);
One advantage of not sorting is when splitting down a large block.
Previously the most-recently-split would be at the start of the list for
the next order down, where potentially the next allocation could use it.
So perhaps less fragmentation if it's all part of one BO. Otherwise I
don't see any other downsides, other than the extra overhead of sorting.
}
static void mark_split(struct drm_buddy_block *block)
@@ -387,20 +405,26 @@ alloc_range_bias(struct drm_buddy *mm,
}
static struct drm_buddy_block *
-get_maxblock(struct list_head *head)
+get_maxblock(struct drm_buddy *mm, unsigned int order)
{
struct drm_buddy_block *max_block = NULL, *node;
+ unsigned int i;
- max_block = list_first_entry_or_null(head,
-struct drm_buddy_block,
-link);
- if (!max_block)
- return NULL;
+ for (i = order; i <= mm->max_order; ++i) {
+ if (!list_empty(&mm->free_list[i])) {
+ node = list_last_entry(&mm->free_list[i],
+ struct drm_buddy_block,
+ link);
+ if (!max_block) {
+ max_block = node;
+ continue;
+ }
- list_for_each_entry(node, head, link) {
- if (drm_buddy_block_offset(node) >
- drm_buddy_block_offset(max_block))
- max_block = node;
+ if (drm_buddy_block_offset(node) >
+ drm_buddy_block_offset(max_block)) {
Formatting doesn't look right here.
Going to test this today with some workloads with small-bar and i915
just to see if this improves/impacts anything for us.
+ max_block = node;
+ }
+ }
}
return max_block;
@@ -412,20 +436,23 @@ alloc_from_freelist(struct drm_buddy *mm,
unsigned long flags)
{
struct drm_buddy_block *block = NULL;
- unsigned int i;
+ unsigned int tmp;
int err;
- for (i = order; i <= mm->max_order; ++i) {
- if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
- block = get_maxblock(&mm->free_list[i]);
- if (block)
- break;
- } else {
- block = list_first_entry_or_null(&mm->free_list[i],
-struct drm_buddy_block,
-link);
- if (block)
- break;
+ if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
+ block = get_maxblock(mm, order);
+ if (block)
+ /* Store the obt
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
On 10/01/2023 11:28, Tvrtko Ursulin wrote:
On 09/01/2023 17:27, Jason Ekstrand wrote:
[snip]
>>> AFAICT it proposes to have 1:1 between *userspace* created
contexts (per
>>> context _and_ engine) and drm_sched. I am not sure avoiding
invasive changes
>>> to the shared code is in the spirit of the overall idea and
instead
>>> opportunity should be used to look at way to refactor/improve
drm_sched.
Maybe? I'm not convinced that what Xe is doing is an abuse at all or
really needs to drive a re-factor. (More on that later.) There's
only one real issue which is that it fires off potentially a lot of
kthreads. Even that's not that bad given that kthreads are pretty
light and you're not likely to have more kthreads than userspace
threads which are much heavier. Not ideal, but not the end of the
world either. Definitely something we can/should optimize but if we
went through with Xe without this patch, it would probably be mostly ok.
>> Yes, it is 1:1 *userspace* engines and drm_sched.
>>
>> I'm not really prepared to make large changes to DRM scheduler
at the
>> moment for Xe as they are not really required nor does Boris
seem they
>> will be required for his work either. I am interested to see
what Boris
>> comes up with.
>>
>>> Even on the low level, the idea to replace drm_sched threads
with workers
>>> has a few problems.
>>>
>>> To start with, the pattern of:
>>>
>>> while (not_stopped) {
>>> keep picking jobs
>>> }
>>>
>>> Feels fundamentally in disagreement with workers (while
obviously fits
>>> perfectly with the current kthread design).
>>
>> The while loop breaks and worker exists if no jobs are ready.
I'm not very familiar with workqueues. What are you saying would fit
better? One scheduling job per work item rather than one big work item
which handles all available jobs?
Yes and no, it indeed IMO does not fit to have a work item which is
potentially unbound in runtime. But it is a bit moot conceptual mismatch
because it is a worst case / theoretical, and I think due more
fundamental concerns.
If we have to go back to the low level side of things, I've picked this
random spot to consolidate what I have already mentioned and perhaps
expand.
To start with, let me pull out some thoughts from workqueue.rst:
"""
Generally, work items are not expected to hog a CPU and consume many
cycles. That means maintaining just enough concurrency to prevent work
processing from stalling should be optimal.
"""
For unbound queues:
"""
The responsibility of regulating concurrency level is on the users.
"""
Given the unbound queues will be spawned on demand to service all queued
work items (more interesting when mixing up with the system_unbound_wq),
in the proposed design the number of instantiated worker threads does
not correspond to the number of user threads (as you have elsewhere
stated), but pessimistically to the number of active user contexts. That
is the number which drives the maximum number of not-runnable jobs that
can become runnable at once, and hence spawn that many work items, and
in turn unbound worker threads.
Several problems there.
It is fundamentally pointless to have potentially that many more threads
than the number of CPU cores - it simply creates a scheduling storm.
To make matters worse, if I follow the code correctly, all these per
user context worker thread / work items end up contending on the same
lock or circular buffer, both are one instance per GPU:
guc_engine_run_job
-> submit_engine
a) wq_item_append
-> wq_wait_for_space
-> msleep
b) xe_guc_ct_send
-> guc_ct_send
-> mutex_lock(&ct->lock);
-> later a potential msleep in h2g_has_room
Regards,
Tvrtko
Re: [PATCH v5 0/3] Add PinePhone Pro display support
On Tue, Jan 3, 2023 at 12:07 AM Javier Martinez Canillas wrote: > This series adds support for the display present in the PinePhone Pro. > > Patch #1 adds a devicetree binding schema for panels based on the Himax > HX8394 controller, such as the HSD060BHW4 720x1440 TFT LCD panel present > in the PinePhone Pro. Patch #2 adds the panel driver for this controller > and finally patch #3 adds an entry for the driver in MAINTAINERS file. > > This version doesn't include the DTS changes, since Ondrej mentioned that > there are still things to sort out before enabling it. The DTS bits will > be proposed as a follow-up patch series. > > This allows for example the Fedora distro to support the PinePhone Pro with > a DTB provided by the firmware. > > This is a v5 of the patch-set that addresses issues pointed out in v4: I looked over the patches a last time. This driver looks great. Acks by Krzysztof and Sam are in place. Patches applied to drm-misc-next! Yours, Linus Walleij
Re: [RFC PATCH 00/20] Initial Xe driver submission
+Frank, who's also working on the pvr uAPI. Hi, On Thu, 22 Dec 2022 14:21:07 -0800 Matthew Brost wrote: > The code has been organized such that we have all patches that touch areas > outside of drm/xe first for review, and then the actual new driver in a > separate > commit. The code which is outside of drm/xe is included in this RFC while > drm/xe is not due to the size of the commit. The drm/xe is code is available > in > a public repo listed below. > > Xe driver commit: > https://cgit.freedesktop.org/drm/drm-xe/commit/?h=drm-xe-next&id=9cb016ebbb6a275f57b1cb512b95d5a842391ad7 > > Xe kernel repo: > https://cgit.freedesktop.org/drm/drm-xe/ Sorry to hijack this thread, again, but I'm currently working on the pancsf uAPI, and I was wondering how DRM maintainers/developers felt about the new direction taken by the Xe driver on some aspects of their uAPI (to decide if I should copy these patterns or go the old way): - plan for ioctl extensions through '__u64 extensions;' fields (the vulkan way, basically) - turning the GETPARAM in DEV_QUERY which can return more than a 64-bit integer at a time - having ioctls taking sub-operations instead of one ioctl per operation (I'm referring to VM_BIND here, which handles map, unmap, restart, ... through a single entry point) Regards, Boris
[PATCH v2] drm/nouveau: Remove file nouveau_fbcon.c
Commit 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
converted nouveau to generic fbdev emulation. The driver's internal
implementation later got accidentally restored during a merge commit.
Remove the file from the driver. No functional changes.
v2:
* point Fixes tag to merge commit (Alex)
Signed-off-by: Thomas Zimmermann
Reviewed-by: Alex Deucher
Fixes: 4e291f2f5853 ("Merge tag 'drm-misc-next-2022-11-10-1' of
git://anongit.freedesktop.org/drm/drm-misc into drm-next")
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Sam Ravnborg
Cc: Jani Nikula
Cc: Dave Airlie
Cc: dri-devel@lists.freedesktop.org
Cc: nouv...@lists.freedesktop.org
---
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 613
1 file changed, 613 deletions(-)
delete mode 100644 drivers/gpu/drm/nouveau/nouveau_fbcon.c
diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
b/drivers/gpu/drm/nouveau/nouveau_fbcon.c
deleted file mode 100644
index e87de7906f78..
--- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c
+++ /dev/null
@@ -1,613 +0,0 @@
-/*
- * Copyright © 2007 David Airlie
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice (including the next
- * paragraph) shall be included in all copies or substantial portions of the
- * Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
- * Authors:
- * David Airlie
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include "nouveau_drv.h"
-#include "nouveau_gem.h"
-#include "nouveau_bo.h"
-#include "nouveau_fbcon.h"
-#include "nouveau_chan.h"
-#include "nouveau_vmm.h"
-
-#include "nouveau_crtc.h"
-
-MODULE_PARM_DESC(nofbaccel, "Disable fbcon acceleration");
-int nouveau_nofbaccel = 0;
-module_param_named(nofbaccel, nouveau_nofbaccel, int, 0400);
-
-MODULE_PARM_DESC(fbcon_bpp, "fbcon bits-per-pixel (default: auto)");
-static int nouveau_fbcon_bpp;
-module_param_named(fbcon_bpp, nouveau_fbcon_bpp, int, 0400);
-
-static void
-nouveau_fbcon_fillrect(struct fb_info *info, const struct fb_fillrect *rect)
-{
- struct nouveau_fbdev *fbcon = info->par;
- struct nouveau_drm *drm = nouveau_drm(fbcon->helper.dev);
- struct nvif_device *device = &drm->client.device;
- int ret;
-
- if (info->state != FBINFO_STATE_RUNNING)
- return;
-
- ret = -ENODEV;
- if (!in_interrupt() && !(info->flags & FBINFO_HWACCEL_DISABLED) &&
- mutex_trylock(&drm->client.mutex)) {
- if (device->info.family < NV_DEVICE_INFO_V0_TESLA)
- ret = nv04_fbcon_fillrect(info, rect);
- else
- if (device->info.family < NV_DEVICE_INFO_V0_FERMI)
- ret = nv50_fbcon_fillrect(info, rect);
- else
- ret = nvc0_fbcon_fillrect(info, rect);
- mutex_unlock(&drm->client.mutex);
- }
-
- if (ret == 0)
- return;
-
- if (ret != -ENODEV)
- nouveau_fbcon_gpu_lockup(info);
- drm_fb_helper_cfb_fillrect(info, rect);
-}
-
-static void
-nouveau_fbcon_copyarea(struct fb_info *info, const struct fb_copyarea *image)
-{
- struct nouveau_fbdev *fbcon = info->par;
- struct nouveau_drm *drm = nouveau_drm(fbcon->helper.dev);
- struct nvif_device *device = &drm->client.device;
- int ret;
-
- if (info->state != FBINFO_STATE_RUNNING)
- return;
-
- ret = -ENODEV;
- if (!in_interrupt() && !(info->flags & FBINFO_HWACCEL_DISABLED) &&
- mutex_trylock(&drm->client.mutex)) {
- if (device->info.family < NV_DEVICE_INFO_V0_TESLA)
- ret = nv04_fbcon_copyarea(info, image);
- else
- if (device->info.family < NV_DEVICE_INFO_V0_FERMI)
- ret = nv50_fbcon_copyarea(info, image);
- else
-
Re: [PATCH v5 0/3] Add PinePhone Pro display support
Hello Linus, On 1/10/23 13:30, Linus Walleij wrote: > On Tue, Jan 3, 2023 at 12:07 AM Javier Martinez Canillas > wrote: > >> This series adds support for the display present in the PinePhone Pro. >> >> Patch #1 adds a devicetree binding schema for panels based on the Himax >> HX8394 controller, such as the HSD060BHW4 720x1440 TFT LCD panel present >> in the PinePhone Pro. Patch #2 adds the panel driver for this controller >> and finally patch #3 adds an entry for the driver in MAINTAINERS file. >> >> This version doesn't include the DTS changes, since Ondrej mentioned that >> there are still things to sort out before enabling it. The DTS bits will >> be proposed as a follow-up patch series. >> >> This allows for example the Fedora distro to support the PinePhone Pro with >> a DTB provided by the firmware. >> >> This is a v5 of the patch-set that addresses issues pointed out in v4: > > I looked over the patches a last time. This driver looks great. > Acks by Krzysztof and Sam are in place. > Patches applied to drm-misc-next! > Awesome. Thanks a lot! -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [Intel-gfx] [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On 10.01.2023 12:07, Andy Shevchenko wrote:
On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:
This patch tries to show usability of __xchg helper.
It is not intended to be merged, but I can convert
it to proper patchset if necessary.
There are many more places where __xchg can be used.
This demo shows the most spectacular cases IMHO:
- previous value is returned from function,
- temporary variables are in use.
As a result readability is much better and diffstat is quite
nice, less local vars to look at.
In many cases whole body of functions is replaced
with __xchg(ptr, val), so as further refactoring the whole
function can be removed and __xchg can be called directly.
...
arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
struct pt_regs *regs)
{
- unsigned long orig_ret_vaddr;
-
- orig_ret_vaddr = regs->ARM_lr;
- /* Replace the return addr with trampoline addr */
- regs->ARM_lr = trampoline_vaddr;
- return orig_ret_vaddr;
+ return __xchg(®s->ARM_lr, trampoline_vaddr);
}
If it's not a callback, the entire function can be killed.
And this is a good example of the function usage.
OTOH, these places might have a side effect (if it's in deep CPU
handlers), means we need to do this carefully.
...
static inline void *qed_chain_produce(struct qed_chain *p_chain)
{
- void *p_ret = NULL, *p_prod_idx, *p_prod_page_idx;
+ void *p_prod_idx, *p_prod_page_idx;
if (is_chain_u16(p_chain)) {
if ((p_chain->u.chain16.prod_idx &
@@ -390,11 +391,8 @@ static inline void *qed_chain_produce(struct qed_chain
*p_chain)
p_chain->u.chain32.prod_idx++;
}
- p_ret = p_chain->p_prod_elem;
- p_chain->p_prod_elem = (void *)(((u8 *)p_chain->p_prod_elem) +
- p_chain->elem_size);
-
- return p_ret;
+ return __xchg(&p_chain->p_prod_elem,
+ (void *)(((u8 *)p_chain->p_prod_elem) +
p_chain->elem_size));
Wondering if you still need a (void *) casting after the change. Ditto for the
rest of similar cases.
IMHO it is not needed also before the change and IIRC gcc has an
extension which allows to drop (u8 *) cast as well [1].
[1]: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
}
...
Btw, is it done by coccinelle? If no, why not providing the script?
Yes I have used cocci. My cocci skills are far from perfect, so I did
not want to share my dirty code, but this is nothing secret:
@r1@
expression x, v;
local idexpression p;
@@
- p = x;
- x = v;
- return p;
+ return __xchg(&x, v);
@depends on r1@
expression e;
@@
__xchg(
- &*e,
+ e,
...)
@depends on r1@
expression t;
@@
- if (t) {
+ if (t)
return __xchg(...);
- }
@depends on r1@
type t;
identifier p;
expression e;
@@
(
- t p;
|
- t p = e;
)
... when != p
Regards
Andrzej
Re: [PATCH v2] drm/nouveau: Remove file nouveau_fbcon.c
Hello Thomas,
On 1/10/23 13:35, Thomas Zimmermann wrote:
> Commit 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
> converted nouveau to generic fbdev emulation. The driver's internal
> implementation later got accidentally restored during a merge commit.
> Remove the file from the driver. No functional changes.
>
> v2:
> * point Fixes tag to merge commit (Alex)
>
> Signed-off-by: Thomas Zimmermann
> Reviewed-by: Alex Deucher
> Fixes: 4e291f2f5853 ("Merge tag 'drm-misc-next-2022-11-10-1' of
> git://anongit.freedesktop.org/drm/drm-misc into drm-next")
I believe the fixes tag should be before the S-o-B ? At least that is
the case in most commits and Documentation/process/maintainer-tip.rst
example. But you could fix it just before applying.
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH v2] drm/nouveau: Remove file nouveau_fbcon.c
Am 10.01.23 um 13:49 schrieb Javier Martinez Canillas:
Hello Thomas,
On 1/10/23 13:35, Thomas Zimmermann wrote:
Commit 4a16dd9d18a0 ("drm/nouveau/kms: switch to drm fbdev helpers")
converted nouveau to generic fbdev emulation. The driver's internal
implementation later got accidentally restored during a merge commit.
Remove the file from the driver. No functional changes.
v2:
* point Fixes tag to merge commit (Alex)
Signed-off-by: Thomas Zimmermann
Reviewed-by: Alex Deucher
Fixes: 4e291f2f5853 ("Merge tag 'drm-misc-next-2022-11-10-1' of
git://anongit.freedesktop.org/drm/drm-misc into drm-next")
I believe the fixes tag should be before the S-o-B ? At least that is
the case in most commits and Documentation/process/maintainer-tip.rst
example. But you could fix it just before applying.
I'll do.
The patch looks good to me.
Reviewed-by: Javier Martinez Canillas
Thanks.
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [RFC 3/3] drm: Update file owner during use
On 06/01/2023 18:00, Daniel Vetter wrote:
On Fri, Jan 06, 2023 at 03:53:13PM +0100, Christian König wrote:
Am 06.01.23 um 11:53 schrieb Daniel Vetter:
On Fri, Jan 06, 2023 at 11:32:17AM +0100, Christian König wrote:
Am 05.01.23 um 13:32 schrieb Daniel Vetter:
[SNIP]
For the case of an master fd I actually don't see the reason why we should
limit that? And fd can become master if it either was master before or has
CAP_SYS_ADMIN. Why would we want an extra check for the pid/tgid here?
This is just info/debug printing, I don't see the connection to
drm_auth/master stuff? Aside from the patch mixes up the master opener and
the current user due to fd passing or something like that.
That's exactly why my comment meant as well.
The connect is that the drm_auth/master code currently the pid/tgid as
indicator if the "owner" of the fd has changed and so if an access should be
allowed or not. I find that approach a bit questionable.
Note that we cannot do that (I think at least, after pondering this some
more) because it would break the logind master fd passing scheme - there
the receiving compositor is explicitly _not_ allowed to acquire master
rights on its own. So the master priviledges must not move with the fd or
things can go wrong.
That could be the rational behind that, but why doesn't logind then just
pass on a normal render node to the compositor?
Because the compositor wants the kms node. We have 3 access levels in drm
- render stuff
- modeset stuff (needs a card* node and master rights for changing things)
- set/drop master (needs root)
logind wants to give the compositor modeset access, but not master
drop/set access (because vt switching is controlled by logind).
The pid check in drm_auth is for the use-case where you start your
compositor on a root vt (or setuid-root), and then want to make sure
that after cred dropping, set/drop master keeps working. Because in that
case the vt switch dance is done by the compositor.
Maybe we should document this stuff a bit better :-)
Maybe add a friendly warning? E.g. like "Don't touch it, it works!" :)
I think Tvrtko just volunteered for that :-) Maybe addition in the
drm-uapi.rst section would be good that fills out the gaps we have.
I can attempt to copy, paste and tidy what you wrote here, albeit with
less than full degree of authority. Assuming into the existing comment
above drm_master_check_perm?
But in terms of where my series is going next I would need some
clarification in the other sub-thread.
Regards,
Tvrtko
So basically this is a valid use case where logind set/get the master status
of a fd while the compositor uses the master functionality?
Yup, and the compositor is _not_ allowed to call these. Despite that it's
the exact sime struct file - it has to be the same struct file in both
loging and compositor, otherwise logind cannot orchestratet the vt switch
dance for the compositors. Which unlike non-logind vt switching has the
nice property that if a compositor dies/goes rogue, logind can still force
the switch. With vt-only switching you need the sysrq to reset the console
to text and kill the foreground process for the same effect.
-Daniel
Christian.
-Daniel
Christian.
-Daniel
Regards,
Christian.
Regards,
Tvrtko
-Daniel
return 0;
if (!capable(CAP_SYS_ADMIN))
diff --git a/drivers/gpu/drm/drm_debugfs.c
b/drivers/gpu/drm/drm_debugfs.c
index 42f657772025..9d4e3146a2b8 100644
--- a/drivers/gpu/drm/drm_debugfs.c
+++ b/drivers/gpu/drm/drm_debugfs.c
@@ -90,15 +90,17 @@ static int drm_clients_info(struct seq_file
*m, void *data)
*/
mutex_lock(&dev->filelist_mutex);
list_for_each_entry_reverse(priv, &dev->filelist, lhead) {
- struct task_struct *task;
bool is_current_master = drm_is_current_master(priv);
+ struct task_struct *task;
+ struct pid *pid;
- rcu_read_lock(); /* locks pid_task()->comm */
- task = pid_task(priv->pid, PIDTYPE_TGID);
+ rcu_read_lock(); /* Locks priv->pid and pid_task()->comm! */
+ pid = rcu_dereference(priv->pid);
+ task = pid_task(pid, PIDTYPE_TGID);
uid = task ? __task_cred(task)->euid : GLOBAL_ROOT_UID;
seq_printf(m, "%20s %5d %3d %c %c %5d %10u\n",
task ? task->comm : "",
- pid_vnr(priv->pid),
+ pid_vnr(pid),
priv->minor->index,
is_current_master ? 'y' : 'n',
priv->authenticated ? 'y' : 'n',
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 20a9aef2b398..3433f9610dba 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -156,7 +156,7 @@ struct drm_file *drm_file_alloc(struct
drm_minor *minor)
if (!file)
return ERR_PTR(-ENOMEM);
- file->pid = get_pid(task_tgid(current));
+ rcu_assign_pointer(file->pid, get_pid(task_tgid(current)));
Re: [PATCH][next] habanalabs: Replace zero-length arrays with flexible-array members
On Tue, Jan 10, 2023 at 12:46 PM Stanislaw Gruszka wrote: > > On Mon, Jan 09, 2023 at 07:39:47PM -0600, Gustavo A. R. Silva wrote: > > Zero-length arrays are deprecated[1] and we are moving towards > > adopting C99 flexible-array members instead. So, replace zero-length > > arrays in a couple of structures with flex-array members. > > > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE > > routines on memcpy() and help us make progress towards globally > > enabling -fstrict-flex-arrays=3 [2]. > > > > Link: > > https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays > > [1] > > Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [2] > > Link: https://github.com/KSPP/linux/issues/78 > > Signed-off-by: Gustavo A. R. Silva > > Reviewed-by: Stanislaw Gruszka Thanks, applied to -next. Oded
Re: [PATCH] ACPI: Fix selecting the wrong ACPI fwnode for the iGPU on some Dell laptops
On Monday, January 9, 2023 9:57:21 PM CET Hans de Goede wrote:
> The Dell Latitude E6430 both with and without the optional NVidia dGPU
> has a bug in its ACPI tables which is causing Linux to assign the wrong
> ACPI fwnode / companion to the pci_device for the i915 iGPU.
>
> Specifically under the PCI root bridge there are these 2 ACPI Device()s :
>
> Scope (_SB.PCI0)
> {
> Device (GFX0)
> {
> Name (_ADR, 0x0002) // _ADR: Address
> }
>
> ...
>
> Device (VID)
> {
> Name (_ADR, 0x0002) // _ADR: Address
> ...
>
> Method (_DOS, 1, NotSerialized) // _DOS: Disable Output Switching
> {
> VDP8 = Arg0
> VDP1 (One, VDP8)
> }
>
> Method (_DOD, 0, NotSerialized) // _DOD: Display Output Devices
> {
> ...
> }
> ...
> }
> }
>
> The non-functional GFX0 ACPI device is a problem, because this gets
> returned as ACPI companion-device by acpi_find_child_device() for the iGPU.
>
> This is a long standing problem and the i915 driver does use the ACPI
> companion for some things, but works fine without it.
>
> However since commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
> acpi_get_pci_dev() relies on the physical-node pointer in the acpi_device
> and that is set on the wrong acpi_device because of the wrong
> acpi_find_child_device() return. This breaks the ACPI video code, leading
> to non working backlight control in some cases.
Interesting. Sorry for the trouble.
> Make find_child_checks() return a higher score for children which have
> pnp-ids set by various scan helpers like acpi_is_video_device(), so
> that it picks the right companion-device.
This has a potential of changing the behavior in some cases that are not
relevant here which is generally risky.
> An alternative approach would be to directly call acpi_is_video_device()
> from find_child_checks() but that would be somewhat computationally
> expensive given that acpi_find_child_device() iterates over all the
> PCI0 children every time it is called.
I agree with the above, but my fix would be something like the patch below (not
really tested, but it builds).
---
drivers/acpi/glue.c | 14 --
drivers/acpi/scan.c |7 +--
include/acpi/acpi_bus.h |3 ++-
3 files changed, 19 insertions(+), 5 deletions(-)
Index: linux-pm/include/acpi/acpi_bus.h
===
--- linux-pm.orig/include/acpi/acpi_bus.h
+++ linux-pm/include/acpi/acpi_bus.h
@@ -230,7 +230,8 @@ struct acpi_pnp_type {
u32 hardware_id:1;
u32 bus_address:1;
u32 platform_id:1;
- u32 reserved:29;
+ u32 backlight:1;
+ u32 reserved:28;
};
struct acpi_device_pnp {
Index: linux-pm/drivers/acpi/scan.c
===
--- linux-pm.orig/drivers/acpi/scan.c
+++ linux-pm/drivers/acpi/scan.c
@@ -1370,9 +1370,12 @@ static void acpi_set_pnp_ids(acpi_handle
* Some devices don't reliably have _HIDs & _CIDs, so add
* synthetic HIDs to make sure drivers can find them.
*/
- if (acpi_is_video_device(handle))
+ if (acpi_is_video_device(handle)) {
acpi_add_id(pnp, ACPI_VIDEO_HID);
- else if (acpi_bay_match(handle))
+ pnp->type.backlight = 1;
+ break;
+ }
+ if (acpi_bay_match(handle))
acpi_add_id(pnp, ACPI_BAY_HID);
else if (acpi_dock_match(handle))
acpi_add_id(pnp, ACPI_DOCK_HID);
Index: linux-pm/drivers/acpi/glue.c
===
--- linux-pm.orig/drivers/acpi/glue.c
+++ linux-pm/drivers/acpi/glue.c
@@ -75,7 +75,8 @@ static struct acpi_bus_type *acpi_get_bu
}
#define FIND_CHILD_MIN_SCORE 1
-#define FIND_CHILD_MAX_SCORE 2
+#define FIND_CHILD_MID_SCORE 2
+#define FIND_CHILD_MAX_SCORE 3
static int match_any(struct acpi_device *adev, void *not_used)
{
@@ -96,8 +97,17 @@ static int find_child_checks(struct acpi
return -ENODEV;
status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);
- if (status == AE_NOT_FOUND)
+ if (status == AE_NOT_FOUND) {
+ /*
+* Special case: backlight device objects without _STA are
+* preferred to other objects with the same _ADR value, because
+* it is more likely that they are actually useful.
+*/
+ if (adev->pnp.type.backlight)
+ return FIND_CHILD_MID_SCORE;
+
return FIND_CHILD_MIN_SCORE;
+ }
if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_ENABLED))
return -ENODEV;
Re: [Intel-gfx] [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On Tue, Jan 10, 2023 at 01:46:37PM +0100, Andrzej Hajda wrote: > On 10.01.2023 12:07, Andy Shevchenko wrote: > > On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote: ... > > > + return __xchg(&p_chain->p_prod_elem, > > > + (void *)(((u8 *)p_chain->p_prod_elem) + > > > p_chain->elem_size)); > > > > Wondering if you still need a (void *) casting after the change. Ditto for > > the > > rest of similar cases. > > IMHO it is not needed also before the change and IIRC gcc has an extension > which allows to drop (u8 *) cast as well [1]. I guess you can drop at least the former one. > [1]: https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html ... > > Btw, is it done by coccinelle? If no, why not providing the script? > > Yes I have used cocci. My cocci skills are far from perfect, so I did not > want to share my dirty code, but this is nothing secret: Thank you! It's not about secrecy, it's about automation / error proofness. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 3/7] dt-bindings: display/msm: document MDSS on SM8550
On Mon, Jan 9, 2023 at 8:30 AM Rob Herring wrote: > > > On Mon, 09 Jan 2023 11:15:19 +0100, Neil Armstrong wrote: > > Document the MDSS hardware found on the Qualcomm SM8550 platform. > > > > Reviewed-by: Krzysztof Kozlowski > > Signed-off-by: Neil Armstrong > > --- > > .../bindings/display/msm/qcom,sm8550-mdss.yaml | 331 > > + > > 1 file changed, 331 insertions(+) > > > > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' > on your patch (DT_CHECKER_FLAGS is new in v5.13): > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > Documentation/devicetree/bindings/display/msm/qcom,sm8550-mdss.example.dts:21:18: > fatal error: dt-bindings/clock/qcom,sm8550-dispcc.h: No such file or > directory >21 | #include > | ^~~~ > compilation terminated. > make[1]: *** [scripts/Makefile.lib:434: > Documentation/devicetree/bindings/display/msm/qcom,sm8550-mdss.example.dtb] > Error 1 > make[1]: *** Waiting for unfinished jobs > make: *** [Makefile:1508: dt_binding_check] Error 2 Now failing in linux-next... Why was this applied before the dependency? Rob
Re: [PATCH 2/2] drm/debugfs: add descriptions to struct parameters
On 1/5/23 16:30, Maíra Canal wrote:
The structs drm_debugfs_info and drm_debugfs_entry don't have
descriptions for their parameters, which is causing the following warnings:
include/drm/drm_debugfs.h:93: warning: Function parameter or member
'name' not described in 'drm_debugfs_info'
include/drm/drm_debugfs.h:93: warning: Function parameter or member
'show' not described in 'drm_debugfs_info'
include/drm/drm_debugfs.h:93: warning: Function parameter or member
'driver_features' not described in 'drm_debugfs_info'
include/drm/drm_debugfs.h:93: warning: Function parameter or member
'data' not described in 'drm_debugfs_info'
include/drm/drm_debugfs.h:105: warning: Function parameter or member
'dev' not described in 'drm_debugfs_entry'
include/drm/drm_debugfs.h:105: warning: Function parameter or member
'file' not described in 'drm_debugfs_entry'
include/drm/drm_debugfs.h:105: warning: Function parameter or member
'list' not described in 'drm_debugfs_entry'
Therefore, fix the warnings by adding descriptions to all struct
parameters.
Reported-by: Stephen Rothwell
Signed-off-by: Maíra Canal
Applied series to drm-misc-next.
Best Regards,
- Maíra Canal
---
include/drm/drm_debugfs.h | 18 ++
1 file changed, 18 insertions(+)
diff --git a/include/drm/drm_debugfs.h b/include/drm/drm_debugfs.h
index 53b7297260a5..7616f457ce70 100644
--- a/include/drm/drm_debugfs.h
+++ b/include/drm/drm_debugfs.h
@@ -86,9 +86,22 @@ struct drm_info_node {
* core.
*/
struct drm_debugfs_info {
+ /** @name: File name */
const char *name;
+
+ /**
+* @show:
+*
+* Show callback. &seq_file->private will be set to the &struct
+* drm_debugfs_entry corresponding to the instance of this info
+* on a given &struct drm_device.
+*/
int (*show)(struct seq_file*, void*);
+
+ /** @driver_features: Required driver features for this entry. */
u32 driver_features;
+
+ /** @data: Driver-private data, should not be device-specific. */
void *data;
};
@@ -99,8 +112,13 @@ struct drm_debugfs_info {
* drm_debugfs_info on a &struct drm_device.
*/
struct drm_debugfs_entry {
+ /** @dev: &struct drm_device for this node. */
struct drm_device *dev;
+
+ /** @file: Template for this node. */
struct drm_debugfs_info file;
+
+ /** @list: Linked list of all device nodes. */
struct list_head list;
};
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
On Tue, Jan 10, 2023 at 5:28 AM Tvrtko Ursulin <
tvrtko.ursu...@linux.intel.com> wrote:
>
>
> On 09/01/2023 17:27, Jason Ekstrand wrote:
>
> [snip]
>
> > >>> AFAICT it proposes to have 1:1 between *userspace* created
> > contexts (per
> > >>> context _and_ engine) and drm_sched. I am not sure avoiding
> > invasive changes
> > >>> to the shared code is in the spirit of the overall idea and
> instead
> > >>> opportunity should be used to look at way to refactor/improve
> > drm_sched.
> >
> >
> > Maybe? I'm not convinced that what Xe is doing is an abuse at all or
> > really needs to drive a re-factor. (More on that later.) There's only
> > one real issue which is that it fires off potentially a lot of kthreads.
> > Even that's not that bad given that kthreads are pretty light and you're
> > not likely to have more kthreads than userspace threads which are much
> > heavier. Not ideal, but not the end of the world either. Definitely
> > something we can/should optimize but if we went through with Xe without
> > this patch, it would probably be mostly ok.
> >
> > >> Yes, it is 1:1 *userspace* engines and drm_sched.
> > >>
> > >> I'm not really prepared to make large changes to DRM scheduler
> > at the
> > >> moment for Xe as they are not really required nor does Boris
> > seem they
> > >> will be required for his work either. I am interested to see
> > what Boris
> > >> comes up with.
> > >>
> > >>> Even on the low level, the idea to replace drm_sched threads
> > with workers
> > >>> has a few problems.
> > >>>
> > >>> To start with, the pattern of:
> > >>>
> > >>>while (not_stopped) {
> > >>> keep picking jobs
> > >>>}
> > >>>
> > >>> Feels fundamentally in disagreement with workers (while
> > obviously fits
> > >>> perfectly with the current kthread design).
> > >>
> > >> The while loop breaks and worker exists if no jobs are ready.
> >
> >
> > I'm not very familiar with workqueues. What are you saying would fit
> > better? One scheduling job per work item rather than one big work item
> > which handles all available jobs?
>
> Yes and no, it indeed IMO does not fit to have a work item which is
> potentially unbound in runtime. But it is a bit moot conceptual mismatch
> because it is a worst case / theoretical, and I think due more
> fundamental concerns.
>
> If we have to go back to the low level side of things, I've picked this
> random spot to consolidate what I have already mentioned and perhaps
> expand.
>
> To start with, let me pull out some thoughts from workqueue.rst:
>
> """
> Generally, work items are not expected to hog a CPU and consume many
> cycles. That means maintaining just enough concurrency to prevent work
> processing from stalling should be optimal.
> """
>
> For unbound queues:
> """
> The responsibility of regulating concurrency level is on the users.
> """
>
> Given the unbound queues will be spawned on demand to service all queued
> work items (more interesting when mixing up with the system_unbound_wq),
> in the proposed design the number of instantiated worker threads does
> not correspond to the number of user threads (as you have elsewhere
> stated), but pessimistically to the number of active user contexts.
Those are pretty much the same in practice. Rather, user threads is
typically an upper bound on the number of contexts. Yes, a single user
thread could have a bunch of contexts but basically nothing does that
except IGT. In real-world usage, it's at most one context per user thread.
> That
> is the number which drives the maximum number of not-runnable jobs that
> can become runnable at once, and hence spawn that many work items, and
> in turn unbound worker threads.
>
> Several problems there.
>
> It is fundamentally pointless to have potentially that many more threads
> than the number of CPU cores - it simply creates a scheduling storm.
>
> Unbound workers have no CPU / cache locality either and no connection
> with the CPU scheduler to optimize scheduling patterns. This may matter
> either on large systems or on small ones. Whereas the current design
> allows for scheduler to notice userspace CPU thread keeps waking up the
> same drm scheduler kernel thread, and so it can keep them on the same
> CPU, the unbound workers lose that ability and so 2nd CPU might be
> getting woken up from low sleep for every submission.
>
> Hence, apart from being a bit of a impedance mismatch, the proposal has
> the potential to change performance and power patterns and both large
> and small machines.
>
Ok, thanks for explaining the issue you're seeing in more detail. Yes,
deferred kwork does appear to mismatch somewhat with what the scheduler
needs or at least how it's worked in the past. How much impact will that
mismatch have? Unclear.
> > >>> Secondly, it probably demands separate workers (not optional)
Re: [PATCH v3 0/3] Add generic framebuffer support to EFI earlycon driver
On Wed, 28 Dec 2022 at 15:04, Andy Shevchenko wrote: > > On Fri, Dec 23, 2022 at 03:42:33PM +0100, Ard Biesheuvel wrote: > > (cc Andy) > > I believe there are two reasons I'm Cc'ed now: > - the Cc was forgotten. because I remember reviewing some parts > of this contribution > - this conflicts (to some extent) with my patch that speeds up > the scrolling > > For the first it's obvious what to do, I think Markuss can include me > in his v4. > > For the second I don't see the functional clash. The scrolling in this > series is not anyhow optimized. I think my patch should go first as > - it is less intrusive > - it has been tested, or can be tested easily > > Tell me if I'm missing something here. > Thanks for your input.
Re: [PATCH v3 0/3] Add generic framebuffer support to EFI earlycon driver
On Fri, 23 Dec 2022 at 15:58, Markuss Broks wrote: > > Hi Ard, > > On 12/23/22 16:42, Ard Biesheuvel wrote: > > (cc Andy) > > > > > > On Wed, 21 Dec 2022 at 11:54, Markuss Broks wrote: > >> Make the EFI earlycon driver be suitable for any linear framebuffers. > >> This should be helpful for early porting of boards with no other means of > >> output, like smartphones/tablets. There seems to be an issue with > >> early_ioremap > >> function on ARM32, but I am unable to find the exact cause. It appears the > >> mappings > >> returned by it are somehow incorrect, thus the driver is disabled on ARM. > > The reason that this driver is disabled on ARM is because the struct > > screen_info is not populated early enough, as it is retrieved from a > > UEFI configuration table. > > I believe I must be hitting some other bug then, since my driver should > not use `struct screen_info` when the arguments are specified manually > (e.g. in device-tree or in kernel command line options), and it still is > broken on ARM when they are. Define 'broken' > I got it to work on ARM when I moved the > early console initialization later into the kernel booting process, but > that mostly defeats the purpose of early console driver, I believe. I've > been thinking that it could be some stuff not getting initialized early > enough indeed, but I've got no clue what could it be. > This is likely due to the fact that the ARM init code sets up the PTE bits for various memory types, and using them beforehand is likely to result in problems.
Re: [PATCH v5 3/7] accel/ivpu: Add GEM buffer object management
On Mon, Jan 9, 2023 at 2:24 PM Jacek Lawrynowicz
wrote:
>
> Adds four types of GEM-based BOs for the VPU:
> - shmem
> - userptr
> - internal
> - prime
>
> All types are implemented as struct ivpu_bo, based on
> struct drm_gem_object. VPU address is allocated when buffer is created
> except for imported prime buffers that allocate it in BO_INFO IOCTL due
> to missing file_priv arg in gem_prime_import callback.
> Internal buffers are pinned on creation, the rest of buffers types
> can be pinned on demand (in SUBMIT IOCTL).
> Buffer VPU address, allocated pages and mappings are released when the
> buffer is destroyed.
> Eviction mechism is planned for future versions.
>
> Add three new IOCTLs: BO_CREATE, BO_INFO, BO_USERPTR
>
> Signed-off-by: Jacek Lawrynowicz
> ---
> drivers/accel/ivpu/Makefile | 1 +
> drivers/accel/ivpu/ivpu_drv.c | 31 +-
> drivers/accel/ivpu/ivpu_drv.h | 1 +
> drivers/accel/ivpu/ivpu_gem.c | 820 ++
> drivers/accel/ivpu/ivpu_gem.h | 128 ++
> include/uapi/drm/ivpu_accel.h | 127 ++
> 6 files changed, 1106 insertions(+), 2 deletions(-)
> create mode 100644 drivers/accel/ivpu/ivpu_gem.c
> create mode 100644 drivers/accel/ivpu/ivpu_gem.h
>
> diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
> index 59cd7843b218..5d7c5862399c 100644
> --- a/drivers/accel/ivpu/Makefile
> +++ b/drivers/accel/ivpu/Makefile
> @@ -3,6 +3,7 @@
>
> intel_vpu-y := \
> ivpu_drv.o \
> + ivpu_gem.o \
> ivpu_hw_mtl.o \
> ivpu_mmu.o \
> ivpu_mmu_context.o
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index d7982f451781..0b9034499c4c 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -12,8 +12,10 @@
> #include
> #include
> #include
> +#include
>
> #include "ivpu_drv.h"
> +#include "ivpu_gem.h"
> #include "ivpu_hw.h"
> #include "ivpu_mmu.h"
> #include "ivpu_mmu_context.h"
> @@ -49,6 +51,24 @@ struct ivpu_file_priv *ivpu_file_priv_get(struct
> ivpu_file_priv *file_priv)
> return file_priv;
> }
>
> +struct ivpu_file_priv *ivpu_file_priv_get_by_ctx_id(struct ivpu_device
> *vdev, unsigned long id)
> +{
> + struct ivpu_file_priv *file_priv;
> +
> + xa_lock_irq(&vdev->context_xa);
> + file_priv = xa_load(&vdev->context_xa, id);
> + /* file_priv may still be in context_xa during file_priv_release() */
> + if (file_priv && !kref_get_unless_zero(&file_priv->ref))
> + file_priv = NULL;
> + xa_unlock_irq(&vdev->context_xa);
> +
> + if (file_priv)
> + ivpu_dbg(vdev, KREF, "file_priv get by id: ctx %u refcount
> %u\n",
> +file_priv->ctx.id, kref_read(&file_priv->ref));
> +
> + return file_priv;
> +}
> +
> static void file_priv_release(struct kref *ref)
> {
> struct ivpu_file_priv *file_priv = container_of(ref, struct
> ivpu_file_priv, ref);
> @@ -57,7 +77,7 @@ static void file_priv_release(struct kref *ref)
> ivpu_dbg(vdev, FILE, "file_priv release: ctx %u\n",
> file_priv->ctx.id);
>
> ivpu_mmu_user_context_fini(vdev, &file_priv->ctx);
> - WARN_ON(xa_erase_irq(&vdev->context_xa, file_priv->ctx.id) !=
> file_priv);
> + drm_WARN_ON(&vdev->drm, xa_erase_irq(&vdev->context_xa,
> file_priv->ctx.id) != file_priv);
> kfree(file_priv);
> }
>
> @@ -66,7 +86,7 @@ void ivpu_file_priv_put(struct ivpu_file_priv **link)
> struct ivpu_file_priv *file_priv = *link;
> struct ivpu_device *vdev = file_priv->vdev;
>
> - WARN_ON(!file_priv);
> + drm_WARN_ON(&vdev->drm, !file_priv);
>
> ivpu_dbg(vdev, KREF, "file_priv put: ctx %u refcount %u\n",
> file_priv->ctx.id, kref_read(&file_priv->ref));
> @@ -200,6 +220,9 @@ static void ivpu_postclose(struct drm_device *dev, struct
> drm_file *file)
> static const struct drm_ioctl_desc ivpu_drm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(IVPU_GET_PARAM, ivpu_get_param_ioctl, 0),
> DRM_IOCTL_DEF_DRV(IVPU_SET_PARAM, ivpu_set_param_ioctl, 0),
> + DRM_IOCTL_DEF_DRV(IVPU_BO_CREATE, ivpu_bo_create_ioctl, 0),
> + DRM_IOCTL_DEF_DRV(IVPU_BO_INFO, ivpu_bo_info_ioctl, 0),
> + DRM_IOCTL_DEF_DRV(IVPU_BO_USERPTR, ivpu_bo_userptr_ioctl, 0),
> };
>
> int ivpu_shutdown(struct ivpu_device *vdev)
> @@ -233,6 +256,10 @@ static const struct drm_driver driver = {
>
> .open = ivpu_open,
> .postclose = ivpu_postclose,
> + .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> + .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> + .gem_prime_import = ivpu_gem_prime_import,
> + .gem_prime_mmap = drm_gem_prime_mmap,
>
> .ioctls = ivpu_drm_ioctls,
> .num_ioctls = ARRAY_SIZE(ivpu_drm_ioctls),
> diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> index a749a0b97703..e8a43dbe5a3a 100644
> --- a/drivers/accel/ivpu/ivpu_drv.h
> +++ b/
Re: [PATCH 1/2] drm/imx/dcss: Drop if blocks with always false condition
Hi,
On Fri, Dec 30, 2022 at 02:00:24PM +0100, Uwe Kleine-König wrote:
> dcss_drv_platform_remove() is only called for a device after
> dcss_drv_platform_probe() returned 0. In that case dev_set_drvdata() was
> called with a non-NULL value and so dev_get_drvdata() won't return NULL.
>
> Signed-off-by: Uwe Kleine-König
Reviewed-by: Laurentiu Palcu
Pushed to drm-misc-next.
Thanks,
laurentiu
> ---
> drivers/gpu/drm/imx/dcss/dcss-drv.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-drv.c
> b/drivers/gpu/drm/imx/dcss/dcss-drv.c
> index 1c70f70247f6..5c88eecf2ce0 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-drv.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-drv.c
> @@ -85,9 +85,6 @@ static int dcss_drv_platform_remove(struct platform_device
> *pdev)
> {
> struct dcss_drv *mdrv = dev_get_drvdata(&pdev->dev);
>
> - if (!mdrv)
> - return 0;
> -
> dcss_kms_detach(mdrv->kms);
> dcss_dev_destroy(mdrv->dcss);
>
> --
> 2.38.1
>
Re: [PATCH 2/2] drm/imx/dcss: Don't call dev_set_drvdata(..., NULL);
Hi, On Fri, Dec 30, 2022 at 02:00:25PM +0100, Uwe Kleine-König wrote: > The driver core takes care about removing driver data, so this can be > dropped from the driver. > > Signed-off-by: Uwe Kleine-König Reviewed-by: Laurentiu Palcu Pushed to drm-misc-next. Thanks, laurentiu > --- > drivers/gpu/drm/imx/dcss/dcss-drv.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/imx/dcss/dcss-drv.c > b/drivers/gpu/drm/imx/dcss/dcss-drv.c > index 5c88eecf2ce0..3d5402193a11 100644 > --- a/drivers/gpu/drm/imx/dcss/dcss-drv.c > +++ b/drivers/gpu/drm/imx/dcss/dcss-drv.c > @@ -74,8 +74,6 @@ static int dcss_drv_platform_probe(struct platform_device > *pdev) > dcss_shutoff: > dcss_dev_destroy(mdrv->dcss); > > - dev_set_drvdata(dev, NULL); > - > err: > kfree(mdrv); > return err; > @@ -88,8 +86,6 @@ static int dcss_drv_platform_remove(struct platform_device > *pdev) > dcss_kms_detach(mdrv->kms); > dcss_dev_destroy(mdrv->dcss); > > - dev_set_drvdata(&pdev->dev, NULL); > - > kfree(mdrv); > > return 0; > -- > 2.38.1 >
Re: [PATCH v6 00/10] drm: Remove usage of deprecated DRM_* macros
I pushed the last 3 patches to drm-misc-next.
[PATCH 1/2] drm: Add DRM-managed alloc_workqueue() and alloc_ordered_workqueue()
Add drmm_alloc_workqueue() and drmm_alloc_ordered_workqueue(), the helpers
that provide managed workqueue cleanup. The workqueue will be destroyed
with the final reference of the DRM device.
Signed-off-by: Jiasheng Jiang
---
drivers/gpu/drm/drm_managed.c | 66 +++
include/drm/drm_managed.h | 8 +
2 files changed, 74 insertions(+)
diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
index 4cf214de50c4..d3bd6247eec9 100644
--- a/drivers/gpu/drm/drm_managed.c
+++ b/drivers/gpu/drm/drm_managed.c
@@ -271,6 +271,13 @@ static void drmm_mutex_release(struct drm_device *dev,
void *res)
mutex_destroy(lock);
}
+static void drmm_destroy_workqueue(struct drm_device *dev, void *res)
+{
+ struct workqueue_struct *wq = res;
+
+ destroy_workqueue(wq);
+}
+
/**
* drmm_mutex_init - &drm_device-managed mutex_init()
* @dev: DRM device
@@ -289,3 +296,62 @@ int drmm_mutex_init(struct drm_device *dev, struct mutex
*lock)
return drmm_add_action_or_reset(dev, drmm_mutex_release, lock);
}
EXPORT_SYMBOL(drmm_mutex_init);
+
+/**
+ * drmm_alloc_workqueue - &drm_device-managed alloc_workqueue()
+ * @dev: DRM device
+ * @wq: workqueue to be allocated
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ *
+ * This is a &drm_device-managed version of alloc_workqueue().
+ * The initialized lock is automatically destroyed on the final
+ * drm_dev_put().
+ */
+int drmm_alloc_workqueue(struct drm_device *dev,
+ struct workqueue_struct *wq, const char *fmt,
+ unsigned int flags, int max_active, ...)
+{
+ va_list args;
+
+ va_start(args, max_active);
+ wq = alloc_workqueue(fmt, flags, max_active, args);
+ va_end(args);
+
+ if (!wq)
+ return -ENOMEM;
+
+ return drmm_add_action_or_reset(dev, drmm_destroy_workqueue, wq);
+}
+EXPORT_SYMBOL(drmm_alloc_workqueue);
+
+/**
+ * drmm_alloc_ordered_workqueue - &drm_device-managed
+ * alloc_ordered_workqueue()
+ * @dev: DRM device
+ * @wq: workqueue to be allocated
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ *
+ * This is a &drm_device-managed version of alloc_ordered_workqueue().
+ * The initialized lock is automatically destroyed on the final
+ * drm_dev_put().
+ */
+int drmm_alloc_ordered_workqueue(struct drm_device *dev,
+ struct workqueue_struct *wq,
+ const char *fmt, unsigned int flags, ...)
+{
+ va_list args;
+
+ va_start(args, flags);
+ wq = alloc_ordered_workqueue(fmt, flags, args);
+ va_end(args);
+
+ if (!wq)
+ return -ENOMEM;
+
+ return drmm_add_action_or_reset(dev, drmm_destroy_workqueue, wq);
+}
+EXPORT_SYMBOL(drmm_alloc_ordered_workqueue);
diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
index 359883942612..68cecc14e1af 100644
--- a/include/drm/drm_managed.h
+++ b/include/drm/drm_managed.h
@@ -107,4 +107,12 @@ void drmm_kfree(struct drm_device *dev, void *data);
int drmm_mutex_init(struct drm_device *dev, struct mutex *lock);
+int drmm_alloc_workqueue(struct drm_device *dev,
+ struct workqueue_struct *wq, const char *fmt,
+ unsigned int flags, int max_active, ...);
+
+int drmm_alloc_ordered_workqueue(struct drm_device *dev,
+ struct workqueue_struct *wq,
+ const char *fmt, unsigned int flags, ...);
+
#endif
--
2.25.1
Re: [PATCH] drm: Alloc high address for drm buddy topdown flag
Hi Matthew,
On 1/10/2023 5:32 PM, Matthew Auld wrote:
On 07/01/2023 15:15, Arunpravin Paneer Selvam wrote:
As we are observing low numbers in viewperf graphics benchmark, we
are strictly not allowing the top down flag enabled allocations
to steal the memory space from cpu visible region.
The approach is, we are sorting each order list entries in
ascending order and compare the last entry of each order
list in the freelist and return the max block.
Did you also run the selftests? Does everything still pass and
complete in a reasonable amount of time?
I will try giving a run
This patch improves the viewperf 3D benchmark scores.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/drm_buddy.c | 81 -
1 file changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 11bb59399471..50916b2f2fc5 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -38,6 +38,25 @@ static void drm_block_free(struct drm_buddy *mm,
kmem_cache_free(slab_blocks, block);
}
+static void list_insert_sorted(struct drm_buddy *mm,
+ struct drm_buddy_block *block)
+{
+ struct drm_buddy_block *node;
+ struct list_head *head;
+
+ head = &mm->free_list[drm_buddy_block_order(block)];
+ if (list_empty(head)) {
+ list_add(&block->link, head);
+ return;
+ }
+
+ list_for_each_entry(node, head, link)
+ if (drm_buddy_block_offset(block) <
drm_buddy_block_offset(node))
+ break;
+
+ __list_add(&block->link, node->link.prev, &node->link);
+}
+
static void mark_allocated(struct drm_buddy_block *block)
{
block->header &= ~DRM_BUDDY_HEADER_STATE;
@@ -52,8 +71,7 @@ static void mark_free(struct drm_buddy *mm,
block->header &= ~DRM_BUDDY_HEADER_STATE;
block->header |= DRM_BUDDY_FREE;
- list_add(&block->link,
- &mm->free_list[drm_buddy_block_order(block)]);
+ list_insert_sorted(mm, block);
One advantage of not sorting is when splitting down a large block.
Previously the most-recently-split would be at the start of the list
for the next order down, where potentially the next allocation could
use it. So perhaps less fragmentation if it's all part of one BO.
Otherwise I don't see any other downsides, other than the extra
overhead of sorting.
Allocating from freelist is traversing through right side (i.e top most
address range) and for TOPDOWN flag allocations we just split the top
most large block once and the subsequent TOPDOWN low order allocations
would get block from same already split large block
For the normal allocations, I modified to retrieve the blocks in each
order list from the last entry which has the high probability of getting
top most blocks as we have sorted the blocks in each order list.
Thus the bottom most large blocks are not frequently used, hence we have
more space for the visible region on dGPU.
For APU which has small sized VRAM space, the allocations are now
ordered and we don't allocate randomly from freelist solving
fragmentation issues.
}
static void mark_split(struct drm_buddy_block *block)
@@ -387,20 +405,26 @@ alloc_range_bias(struct drm_buddy *mm,
}
static struct drm_buddy_block *
-get_maxblock(struct list_head *head)
+get_maxblock(struct drm_buddy *mm, unsigned int order)
{
struct drm_buddy_block *max_block = NULL, *node;
+ unsigned int i;
- max_block = list_first_entry_or_null(head,
- struct drm_buddy_block,
- link);
- if (!max_block)
- return NULL;
+ for (i = order; i <= mm->max_order; ++i) {
+ if (!list_empty(&mm->free_list[i])) {
+ node = list_last_entry(&mm->free_list[i],
+ struct drm_buddy_block,
+ link);
+ if (!max_block) {
+ max_block = node;
+ continue;
+ }
- list_for_each_entry(node, head, link) {
- if (drm_buddy_block_offset(node) >
- drm_buddy_block_offset(max_block))
- max_block = node;
+ if (drm_buddy_block_offset(node) >
+ drm_buddy_block_offset(max_block)) {
Formatting doesn't look right here.
I will check.
Going to test this today with some workloads with small-bar and i915
just to see if this improves/impacts anything for us.
+ max_block = node;
+ }
+ }
}
return max_block;
@@ -412,20 +436,23 @@ alloc_from_freelist(struct drm_buddy *mm,
unsigned long flags)
{
struct drm_buddy_block *block = NULL;
- unsigned int i;
+ unsigned int tmp;
int err;
- for (i = order; i <= mm->max_order; ++i) {
- if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
- block = get_maxblock(&mm->free_list[i]);
- if (block)
- break;
- } else {
- blo
Re: [PATCH v3 3/7] dt-bindings: display/msm: document MDSS on SM8550
On 10/01/2023 15:54, Rob Herring wrote: On Mon, Jan 9, 2023 at 8:30 AM Rob Herring wrote: On Mon, 09 Jan 2023 11:15:19 +0100, Neil Armstrong wrote: Document the MDSS hardware found on the Qualcomm SM8550 platform. Reviewed-by: Krzysztof Kozlowski Signed-off-by: Neil Armstrong --- .../bindings/display/msm/qcom,sm8550-mdss.yaml | 331 + 1 file changed, 331 insertions(+) My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/display/msm/qcom,sm8550-mdss.example.dts:21:18: fatal error: dt-bindings/clock/qcom,sm8550-dispcc.h: No such file or directory 21 | #include | ^~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:434: Documentation/devicetree/bindings/display/msm/qcom,sm8550-mdss.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs make: *** [Makefile:1508: dt_binding_check] Error 2 Now failing in linux-next... Why was this applied before the dependency? I failed to notice the dependency while applying. It was taken out, but probably too late to propagate into today's linux-next. It will be fixed tomorrow. Please excuse me for the troubles. -- With best wishes Dmitry
Re: [PATCH] ACPI: Fix selecting the wrong ACPI fwnode for the iGPU on some Dell laptops
Hi Rafael,
On 1/10/23 14:33, Rafael J. Wysocki wrote:
> On Monday, January 9, 2023 9:57:21 PM CET Hans de Goede wrote:
>> The Dell Latitude E6430 both with and without the optional NVidia dGPU
>> has a bug in its ACPI tables which is causing Linux to assign the wrong
>> ACPI fwnode / companion to the pci_device for the i915 iGPU.
>>
>> Specifically under the PCI root bridge there are these 2 ACPI Device()s :
>>
>> Scope (_SB.PCI0)
>> {
>> Device (GFX0)
>> {
>> Name (_ADR, 0x0002) // _ADR: Address
>> }
>>
>> ...
>>
>> Device (VID)
>> {
>> Name (_ADR, 0x0002) // _ADR: Address
>> ...
>>
>> Method (_DOS, 1, NotSerialized) // _DOS: Disable Output Switching
>> {
>> VDP8 = Arg0
>> VDP1 (One, VDP8)
>> }
>>
>> Method (_DOD, 0, NotSerialized) // _DOD: Display Output Devices
>> {
>> ...
>> }
>> ...
>> }
>> }
>>
>> The non-functional GFX0 ACPI device is a problem, because this gets
>> returned as ACPI companion-device by acpi_find_child_device() for the iGPU.
>>
>> This is a long standing problem and the i915 driver does use the ACPI
>> companion for some things, but works fine without it.
>>
>> However since commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
>> acpi_get_pci_dev() relies on the physical-node pointer in the acpi_device
>> and that is set on the wrong acpi_device because of the wrong
>> acpi_find_child_device() return. This breaks the ACPI video code, leading
>> to non working backlight control in some cases.
>
> Interesting. Sorry for the trouble.
No problem, as mentioned this is actually a long standing issue / bug
in the ACPI tables, it just never surfaced before.
>> Make find_child_checks() return a higher score for children which have
>> pnp-ids set by various scan helpers like acpi_is_video_device(), so
>> that it picks the right companion-device.
>
> This has a potential of changing the behavior in some cases that are not
> relevant here which is generally risky.
>
>> An alternative approach would be to directly call acpi_is_video_device()
>> from find_child_checks() but that would be somewhat computationally
>> expensive given that acpi_find_child_device() iterates over all the
>> PCI0 children every time it is called.
>
> I agree with the above, but my fix would be something like the patch below
> (not
> really tested, but it builds).
Thanks, I have just given this a spin on my E6430 and I can confirm
it still fixes things.
I'll send out this version (re-using most of the v1 commitmsg) as a v2
right away.
Regards,
Hans
>
> ---
> drivers/acpi/glue.c | 14 --
> drivers/acpi/scan.c |7 +--
> include/acpi/acpi_bus.h |3 ++-
> 3 files changed, 19 insertions(+), 5 deletions(-)
>
> Index: linux-pm/include/acpi/acpi_bus.h
> ===
> --- linux-pm.orig/include/acpi/acpi_bus.h
> +++ linux-pm/include/acpi/acpi_bus.h
> @@ -230,7 +230,8 @@ struct acpi_pnp_type {
> u32 hardware_id:1;
> u32 bus_address:1;
> u32 platform_id:1;
> - u32 reserved:29;
> + u32 backlight:1;
> + u32 reserved:28;
> };
>
> struct acpi_device_pnp {
> Index: linux-pm/drivers/acpi/scan.c
> ===
> --- linux-pm.orig/drivers/acpi/scan.c
> +++ linux-pm/drivers/acpi/scan.c
> @@ -1370,9 +1370,12 @@ static void acpi_set_pnp_ids(acpi_handle
>* Some devices don't reliably have _HIDs & _CIDs, so add
>* synthetic HIDs to make sure drivers can find them.
>*/
> - if (acpi_is_video_device(handle))
> + if (acpi_is_video_device(handle)) {
> acpi_add_id(pnp, ACPI_VIDEO_HID);
> - else if (acpi_bay_match(handle))
> + pnp->type.backlight = 1;
> + break;
> + }
> + if (acpi_bay_match(handle))
> acpi_add_id(pnp, ACPI_BAY_HID);
> else if (acpi_dock_match(handle))
> acpi_add_id(pnp, ACPI_DOCK_HID);
> Index: linux-pm/drivers/acpi/glue.c
> ===
> --- linux-pm.orig/drivers/acpi/glue.c
> +++ linux-pm/drivers/acpi/glue.c
> @@ -75,7 +75,8 @@ static struct acpi_bus_type *acpi_get_bu
> }
>
> #define FIND_CHILD_MIN_SCORE 1
> -#define FIND_CHILD_MAX_SCORE 2
> +#define FIND_CHILD_MID_SCORE 2
> +#define FIND_CHILD_MAX_SCORE 3
>
> static int match_any(struct acpi_device *adev, void *not_used)
> {
> @@ -96,8 +97,17 @@ static int find_child_checks(struct acpi
> return -ENODEV;
>
> status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);
> - if (status == AE_NOT_FOUND)
> + if (status == AE_NOT_FOUND) {
> + /*
> + * Special case:
[PATCH 2/2] drm/i915: Replace alloc*workqueue with DRM helpers
Replace alloc*workqueue with DRM helpers in order to avoid memory leak.
Moreover, check the return value since the workqueue may be NULL and
cause NULL pointer dereference.
Fixes: c26a058680dc ("drm/i915: Use a high priority wq for nonblocking plane
updates")
Fixes: 757fffcfdffb ("drm/i915: Put all non-blocking modesets onto an ordered
wq")
Signed-off-by: Jiasheng Jiang
---
drivers/gpu/drm/i915/display/intel_display.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 6c2686ecb62a..8acef38ca985 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -41,6 +41,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -8654,9 +8655,16 @@ int intel_modeset_init_noirq(struct drm_i915_private
*i915)
intel_dmc_ucode_init(i915);
- i915->display.wq.modeset = alloc_ordered_workqueue("i915_modeset", 0);
- i915->display.wq.flip = alloc_workqueue("i915_flip", WQ_HIGHPRI |
- WQ_UNBOUND,
WQ_UNBOUND_MAX_ACTIVE);
+ ret = drmm_alloc_ordered_workqueue(&i915->drm,
+ i915->display.wq.modeset,
"i915_modeset", 0);
+ if (ret)
+ goto cleanup_vga_client_pw_domain_dmc;
+
+ ret = drmm_alloc_workqueue(&i915->drm, i915->display.wq.flip,
+ "i915_flip", WQ_HIGHPRI | WQ_UNBOUND,
+ WQ_UNBOUND_MAX_ACTIVE);
+ if (ret)
+ goto cleanup_vga_client_pw_domain_dmc;
intel_mode_config_init(i915);
--
2.25.1
[PATCH v2] ACPI: Fix selecting the wrong ACPI fwnode for the iGPU on some Dell laptops
The Dell Latitude E6430 both with and without the optional NVidia dGPU
has a bug in its ACPI tables which is causing Linux to assign the wrong
ACPI fwnode / companion to the pci_device for the i915 iGPU.
Specifically under the PCI root bridge there are these 2 ACPI Device()s :
Scope (_SB.PCI0)
{
Device (GFX0)
{
Name (_ADR, 0x0002) // _ADR: Address
}
...
Device (VID)
{
Name (_ADR, 0x0002) // _ADR: Address
...
Method (_DOS, 1, NotSerialized) // _DOS: Disable Output Switching
{
VDP8 = Arg0
VDP1 (One, VDP8)
}
Method (_DOD, 0, NotSerialized) // _DOD: Display Output Devices
{
...
}
...
}
}
The non-functional GFX0 ACPI device is a problem, because this gets
returned as ACPI companion-device by acpi_find_child_device() for the iGPU.
This is a long standing problem and the i915 driver does use the ACPI
companion for some things, but works fine without it.
However since commit 63f534b8bad9 ("ACPI: PCI: Rework acpi_get_pci_dev()")
acpi_get_pci_dev() relies on the physical-node pointer in the acpi_device
and that is set on the wrong acpi_device because of the wrong
acpi_find_child_device() return. This breaks the ACPI video code,
leading to non working backlight control in some cases.
Add a type.backlight flag, mark ACPI video bus devices with this and make
find_child_checks() return a higher score for children with this flag set,
so that it picks the right companion-device.
Co-developed-by: Rafael J. Wysocki
Signed-off-by: Hans de Goede
---
Changes in v2:
- Switch to Rafael's suggested implementation using a type.backlight flag
and only make find_child_checks() return a higher score when this is set
---
drivers/acpi/glue.c | 14 --
drivers/acpi/scan.c | 7 +--
include/acpi/acpi_bus.h | 3 ++-
3 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/acpi/glue.c b/drivers/acpi/glue.c
index 204fe94c7e45..a194f30876c5 100644
--- a/drivers/acpi/glue.c
+++ b/drivers/acpi/glue.c
@@ -75,7 +75,8 @@ static struct acpi_bus_type *acpi_get_bus_type(struct device
*dev)
}
#define FIND_CHILD_MIN_SCORE 1
-#define FIND_CHILD_MAX_SCORE 2
+#define FIND_CHILD_MID_SCORE 2
+#define FIND_CHILD_MAX_SCORE 3
static int match_any(struct acpi_device *adev, void *not_used)
{
@@ -96,8 +97,17 @@ static int find_child_checks(struct acpi_device *adev, bool
check_children)
return -ENODEV;
status = acpi_evaluate_integer(adev->handle, "_STA", NULL, &sta);
- if (status == AE_NOT_FOUND)
+ if (status == AE_NOT_FOUND) {
+ /*
+* Special case: backlight device objects without _STA are
+* preferred to other objects with the same _ADR value, because
+* it is more likely that they are actually useful.
+*/
+ if (adev->pnp.type.backlight)
+ return FIND_CHILD_MID_SCORE;
+
return FIND_CHILD_MIN_SCORE;
+ }
if (ACPI_FAILURE(status) || !(sta & ACPI_STA_DEVICE_ENABLED))
return -ENODEV;
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 274344434282..0c6f06abe3f4 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1370,9 +1370,12 @@ static void acpi_set_pnp_ids(acpi_handle handle, struct
acpi_device_pnp *pnp,
* Some devices don't reliably have _HIDs & _CIDs, so add
* synthetic HIDs to make sure drivers can find them.
*/
- if (acpi_is_video_device(handle))
+ if (acpi_is_video_device(handle)) {
acpi_add_id(pnp, ACPI_VIDEO_HID);
- else if (acpi_bay_match(handle))
+ pnp->type.backlight = 1;
+ break;
+ }
+ if (acpi_bay_match(handle))
acpi_add_id(pnp, ACPI_BAY_HID);
else if (acpi_dock_match(handle))
acpi_add_id(pnp, ACPI_DOCK_HID);
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index cd3b75e08ec3..e44be31115a6 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -230,7 +230,8 @@ struct acpi_pnp_type {
u32 hardware_id:1;
u32 bus_address:1;
u32 platform_id:1;
- u32 reserved:29;
+ u32 backlight:1;
+ u32 reserved:28;
};
struct acpi_device_pnp {
--
2.39.0
Re: [PATCH v2 RESEND] adreno: Shutdown the GPU properly
On Mon, Jan 9, 2023 at 2:25 PM Joel Fernandes (Google)
wrote:
>
> During kexec on ARM device, we notice that device_shutdown() only calls
> pm_runtime_force_suspend() while shutting down the GPU. This means the GPU
> kthread is still running and further, there maybe active submits.
>
> This causes all kinds of issues during a kexec reboot:
>
> Warning from shutdown path:
>
> [ 292.509662] WARNING: CPU: 0 PID: 6304 at [...]
> adreno_runtime_suspend+0x3c/0x44
> [ 292.509863] Hardware name: Google Lazor (rev3 - 8) with LTE (DT)
> [ 292.509872] pstate: 8049 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 292.509881] pc : adreno_runtime_suspend+0x3c/0x44
> [ 292.509891] lr : pm_generic_runtime_suspend+0x30/0x44
> [ 292.509905] sp : ffc014473bf0
> [...]
> [ 292.510043] Call trace:
> [ 292.510051] adreno_runtime_suspend+0x3c/0x44
> [ 292.510061] pm_generic_runtime_suspend+0x30/0x44
> [ 292.510071] pm_runtime_force_suspend+0x54/0xc8
> [ 292.510081] adreno_shutdown+0x1c/0x28
> [ 292.510090] platform_shutdown+0x2c/0x38
> [ 292.510104] device_shutdown+0x158/0x210
> [ 292.510119] kernel_restart_prepare+0x40/0x4c
>
> And here from GPU kthread, an SError OOPs:
>
> [ 192.648789] el1h_64_error+0x7c/0x80
> [ 192.648812] el1_interrupt+0x20/0x58
> [ 192.648833] el1h_64_irq_handler+0x18/0x24
> [ 192.648854] el1h_64_irq+0x7c/0x80
> [ 192.648873] local_daif_inherit+0x10/0x18
> [ 192.648900] el1h_64_sync_handler+0x48/0xb4
> [ 192.648921] el1h_64_sync+0x7c/0x80
> [ 192.648941] a6xx_gmu_set_oob+0xbc/0x1fc
> [ 192.648968] a6xx_hw_init+0x44/0xe38
> [ 192.648991] msm_gpu_hw_init+0x48/0x80
> [ 192.649013] msm_gpu_submit+0x5c/0x1a8
> [ 192.649034] msm_job_run+0xb0/0x11c
> [ 192.649058] drm_sched_main+0x170/0x434
> [ 192.649086] kthread+0x134/0x300
> [ 192.649114] ret_from_fork+0x10/0x20
>
> Fix by calling adreno_system_suspend() in the device_shutdown() path.
>
> [ Applied Rob Clark feedback on fixing adreno_unbind() similarly, also
> tested as above. ]
>
> Cc: Rob Clark
> Cc: Steven Rostedt
> Cc: Ricardo Ribalda
> Cc: Ross Zwisler
> Signed-off-by: Joel Fernandes (Google)
Reviewed-by: Rob Clark
> ---
> drivers/gpu/drm/msm/adreno/adreno_device.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
> b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 628806423f7d..36f062c7582f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -551,13 +551,14 @@ static int adreno_bind(struct device *dev, struct
> device *master, void *data)
> return 0;
> }
>
> +static int adreno_system_suspend(struct device *dev);
> static void adreno_unbind(struct device *dev, struct device *master,
> void *data)
> {
> struct msm_drm_private *priv = dev_get_drvdata(master);
> struct msm_gpu *gpu = dev_to_gpu(dev);
>
> - pm_runtime_force_suspend(dev);
> + WARN_ON_ONCE(adreno_system_suspend(dev));
> gpu->funcs->destroy(gpu);
>
> priv->gpu_pdev = NULL;
> @@ -609,7 +610,7 @@ static int adreno_remove(struct platform_device *pdev)
>
> static void adreno_shutdown(struct platform_device *pdev)
> {
> - pm_runtime_force_suspend(&pdev->dev);
> + WARN_ON_ONCE(adreno_system_suspend(&pdev->dev));
> }
>
> static const struct of_device_id dt_match[] = {
> --
> 2.39.0.314.g84b9a713c41-goog
>
Re: [PATCH v5 5/7] accel/ivpu: Implement firmware parsing and booting
On Mon, Jan 9, 2023 at 2:24 PM Jacek Lawrynowicz
wrote:
>
> Read, parse and boot VPU firmware image.
>
> Co-developed-by: Andrzej Kacprowski
> Signed-off-by: Andrzej Kacprowski
> Co-developed-by: Krystian Pradzynski
> Signed-off-by: Krystian Pradzynski
> Signed-off-by: Jacek Lawrynowicz
> ---
> drivers/accel/ivpu/Makefile | 1 +
> drivers/accel/ivpu/ivpu_drv.c | 131 +-
> drivers/accel/ivpu/ivpu_drv.h | 11 +
> drivers/accel/ivpu/ivpu_fw.c | 419 ++
> drivers/accel/ivpu/ivpu_fw.h | 38 +++
> drivers/accel/ivpu/ivpu_hw_mtl.c | 10 +
> drivers/accel/ivpu/vpu_boot_api.h | 349 +
> include/uapi/drm/ivpu_accel.h | 21 ++
> 8 files changed, 979 insertions(+), 1 deletion(-)
> create mode 100644 drivers/accel/ivpu/ivpu_fw.c
> create mode 100644 drivers/accel/ivpu/ivpu_fw.h
> create mode 100644 drivers/accel/ivpu/vpu_boot_api.h
>
> diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
> index 46595f0112e3..9fa6a76e9d79 100644
> --- a/drivers/accel/ivpu/Makefile
> +++ b/drivers/accel/ivpu/Makefile
> @@ -3,6 +3,7 @@
>
> intel_vpu-y := \
> ivpu_drv.o \
> + ivpu_fw.o \
> ivpu_gem.o \
> ivpu_hw_mtl.o \
> ivpu_ipc.o \
> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> index 6643ae6b5a52..53e103f64832 100644
> --- a/drivers/accel/ivpu/ivpu_drv.c
> +++ b/drivers/accel/ivpu/ivpu_drv.c
> @@ -14,10 +14,13 @@
> #include
> #include
>
> +#include "vpu_boot_api.h"
> #include "ivpu_drv.h"
> +#include "ivpu_fw.h"
> #include "ivpu_gem.h"
> #include "ivpu_hw.h"
> #include "ivpu_ipc.h"
> +#include "ivpu_jsm_msg.h"
> #include "ivpu_mmu.h"
> #include "ivpu_mmu_context.h"
>
> @@ -32,6 +35,10 @@ int ivpu_dbg_mask;
> module_param_named(dbg_mask, ivpu_dbg_mask, int, 0644);
> MODULE_PARM_DESC(dbg_mask, "Driver debug mask. See IVPU_DBG_* macros.");
>
> +int ivpu_test_mode;
> +module_param_named_unsafe(test_mode, ivpu_test_mode, int, 0644);
> +MODULE_PARM_DESC(test_mode, "Test mode: 0 - normal operation, 1 - fw unit
> test, 2 - null hw");
> +
> u8 ivpu_pll_min_ratio;
> module_param_named(pll_min_ratio, ivpu_pll_min_ratio, byte, 0644);
> MODULE_PARM_DESC(pll_min_ratio, "Minimum PLL ratio used to set VPU
> frequency");
> @@ -129,6 +136,28 @@ static int ivpu_get_param_ioctl(struct drm_device *dev,
> void *data, struct drm_f
> case DRM_IVPU_PARAM_CONTEXT_ID:
> args->value = file_priv->ctx.id;
> break;
> + case DRM_IVPU_PARAM_FW_API_VERSION:
> + if (args->index < VPU_FW_API_VER_NUM) {
> + struct vpu_firmware_header *fw_hdr;
> +
> + fw_hdr = (struct vpu_firmware_header
> *)vdev->fw->file->data;
> + args->value = fw_hdr->api_version[args->index];
> + } else {
> + ret = -EINVAL;
> + }
> + break;
> + case DRM_IVPU_PARAM_ENGINE_HEARTBEAT:
> + ret = ivpu_jsm_get_heartbeat(vdev, args->index, &args->value);
> + break;
> + case DRM_IVPU_PARAM_UNIQUE_INFERENCE_ID:
> + args->value =
> (u64)atomic64_inc_return(&vdev->unique_id_counter);
> + break;
> + case DRM_IVPU_PARAM_TILE_CONFIG:
> + args->value = vdev->hw->tile_fuse;
> + break;
> + case DRM_IVPU_PARAM_SKU:
> + args->value = vdev->hw->sku;
> + break;
> default:
> ret = -EINVAL;
> break;
> @@ -226,11 +255,85 @@ static const struct drm_ioctl_desc ivpu_drm_ioctls[] = {
> DRM_IOCTL_DEF_DRV(IVPU_BO_USERPTR, ivpu_bo_userptr_ioctl, 0),
> };
>
> +static int ivpu_wait_for_ready(struct ivpu_device *vdev)
> +{
> + struct ivpu_ipc_consumer cons;
> + struct ivpu_ipc_hdr ipc_hdr;
> + unsigned long timeout;
> + int ret;
> +
> + if (ivpu_test_mode == IVPU_TEST_MODE_FW_TEST)
> + return 0;
> +
> + ivpu_ipc_consumer_add(vdev, &cons, IVPU_IPC_CHAN_BOOT_MSG);
> +
> + timeout = jiffies + msecs_to_jiffies(vdev->timeout.boot);
> + while (1) {
> + ret = ivpu_ipc_irq_handler(vdev);
> + if (ret)
> + break;
> + ret = ivpu_ipc_receive(vdev, &cons, &ipc_hdr, NULL, 0);
> + if (ret != -ETIMEDOUT || time_after_eq(jiffies, timeout))
> + break;
> +
> + cond_resched();
> + }
> +
> + ivpu_ipc_consumer_del(vdev, &cons);
> +
> + if (!ret && ipc_hdr.data_addr != IVPU_IPC_BOOT_MSG_DATA_ADDR) {
> + ivpu_err(vdev, "Invalid VPU ready message: 0x%x\n",
> +ipc_hdr.data_addr);
> + return -EIO;
> + }
> +
> + if (!ret)
> + ivpu_info(vdev, "VPU ready message received successfully\n");
> + else
> +
Re: [PATCH] drm/i915: Add missing check and destroy for alloc_workqueue
On Sat, Jan 07, 2023 at 02:29:22AM +0800, Daniel Vetter wrote:
>On Fri, Jan 06, 2023 at 05:09:34PM +0800, Jiasheng Jiang wrote:
>> Add checks for the return value of alloc_workqueue and
>> alloc_ordered_workqueue as they may return NULL pointer and cause NULL
>> pointer dereference.
>> Moreover, destroy them when fails later in order to avoid memory leak.
>> Because in `drivers/gpu/drm/i915/i915_driver.c`, if
>> intel_modeset_init_noirq fails, its workqueues will not be destroyed.
>>
>> Fixes: c26a058680dc ("drm/i915: Use a high priority wq for nonblocking plane
>> updates")
>> Fixes: 757fffcfdffb ("drm/i915: Put all non-blocking modesets onto an
>> ordered wq")
>> Signed-off-by: Jiasheng Jiang
>
> So you have an entire pile of these, and I think that's a really good
> excuse to
> - create a drmm_alloc_workqueue helper for these (and
> drmm_alloc_ordered_workqueue) using the drmm_add_action_or_reset()
> function for automatic drm_device cleanup
> - use that instead in all drivers, which means you do not have to make any
> error handling mor complex
>
> Up for that? In that case please also convert any existing alloc*workqueue
> callsites in drm, and make this all a patch series (since then there would
> be dependencies).
Fine, I have submitted two patches. The first one create the
drmm_alloc_workqueue and drmm_alloc_ordered_workqueue helpers. And the
second one replace the alloc*workqueue with DRM helpers in
`drivers/gpu/drm/i915/display/intel_display.c`.
If there is no problem in these two, I will submitted the other patches
that replace the alloc*workqueue in other DRM files.
Thanks,
Jiang
Re: [PATCH] drm: Alloc high address for drm buddy topdown flag
On 10/01/2023 12:02, Matthew Auld wrote:
On 07/01/2023 15:15, Arunpravin Paneer Selvam wrote:
As we are observing low numbers in viewperf graphics benchmark, we
are strictly not allowing the top down flag enabled allocations
to steal the memory space from cpu visible region.
The approach is, we are sorting each order list entries in
ascending order and compare the last entry of each order
list in the freelist and return the max block.
Did you also run the selftests? Does everything still pass and complete
in a reasonable amount of time?
This patch improves the viewperf 3D benchmark scores.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/drm_buddy.c | 81 -
1 file changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 11bb59399471..50916b2f2fc5 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -38,6 +38,25 @@ static void drm_block_free(struct drm_buddy *mm,
kmem_cache_free(slab_blocks, block);
}
+static void list_insert_sorted(struct drm_buddy *mm,
+ struct drm_buddy_block *block)
+{
+ struct drm_buddy_block *node;
+ struct list_head *head;
+
+ head = &mm->free_list[drm_buddy_block_order(block)];
+ if (list_empty(head)) {
+ list_add(&block->link, head);
+ return;
+ }
+
+ list_for_each_entry(node, head, link)
+ if (drm_buddy_block_offset(block) <
drm_buddy_block_offset(node))
+ break;
+
+ __list_add(&block->link, node->link.prev, &node->link);
+}
+
static void mark_allocated(struct drm_buddy_block *block)
{
block->header &= ~DRM_BUDDY_HEADER_STATE;
@@ -52,8 +71,7 @@ static void mark_free(struct drm_buddy *mm,
block->header &= ~DRM_BUDDY_HEADER_STATE;
block->header |= DRM_BUDDY_FREE;
- list_add(&block->link,
- &mm->free_list[drm_buddy_block_order(block)]);
+ list_insert_sorted(mm, block);
One advantage of not sorting is when splitting down a large block.
Previously the most-recently-split would be at the start of the list for
the next order down, where potentially the next allocation could use it.
So perhaps less fragmentation if it's all part of one BO. Otherwise I
don't see any other downsides, other than the extra overhead of sorting.
}
static void mark_split(struct drm_buddy_block *block)
@@ -387,20 +405,26 @@ alloc_range_bias(struct drm_buddy *mm,
}
static struct drm_buddy_block *
-get_maxblock(struct list_head *head)
+get_maxblock(struct drm_buddy *mm, unsigned int order)
{
struct drm_buddy_block *max_block = NULL, *node;
+ unsigned int i;
- max_block = list_first_entry_or_null(head,
- struct drm_buddy_block,
- link);
- if (!max_block)
- return NULL;
+ for (i = order; i <= mm->max_order; ++i) {
+ if (!list_empty(&mm->free_list[i])) {
+ node = list_last_entry(&mm->free_list[i],
+ struct drm_buddy_block,
+ link);
+ if (!max_block) {
+ max_block = node;
+ continue;
+ }
- list_for_each_entry(node, head, link) {
- if (drm_buddy_block_offset(node) >
- drm_buddy_block_offset(max_block))
- max_block = node;
+ if (drm_buddy_block_offset(node) >
+ drm_buddy_block_offset(max_block)) {
Formatting doesn't look right here.
Going to test this today with some workloads with small-bar and i915
just to see if this improves/impacts anything for us.
No surprises, and as advertised seems to lead to reduced utilisation of
the mappable part for buffers that don't explicitly need it (TOPDOWN).
Assuming the selftests are still happy,
Reviewed-by: Matthew Auld
+ max_block = node;
+ }
+ }
}
return max_block;
@@ -412,20 +436,23 @@ alloc_from_freelist(struct drm_buddy *mm,
unsigned long flags)
{
struct drm_buddy_block *block = NULL;
- unsigned int i;
+ unsigned int tmp;
int err;
- for (i = order; i <= mm->max_order; ++i) {
- if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
- block = get_maxblock(&mm->free_list[i]);
- if (block)
- break;
- } else {
- block = list_first_entry_or_null(&mm->free_list[i],
- struct drm_buddy_block,
- link);
- if (block)
- break;
+ if (flags & DRM_BUDDY_TOPDOWN_ALLOCATION) {
+ block = get_maxblock(mm, order);
+ if (block)
+ /* Store the obtained block order */
+ tmp = drm_buddy_block_order(block);
+ } else {
+ for (tmp = order; tmp <= mm->max_order; ++tmp) {
+ if (!list_empty(&mm->free_list[tmp])) {
+ block = list_last_entry(&mm->free_list[tmp],
+
Re: [PATCH] drm/amd/display: No need for Null pointer check before kfree
On 12/27/22 13:39, Deepak R Varma wrote:
kfree() & vfree() internally performs NULL check on the pointer handed
to it and take no action if it indeed is NULL. Hence there is no need
for a pre-check of the memory pointer before handing it to
kfree()/vfree().
Issue reported by ifnullfree.cocci Coccinelle semantic patch script.
Signed-off-by: Deepak R Varma
---
drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c | 3 +--
drivers/gpu/drm/amd/display/dc/clk_mgr/dcn32/dcn32_clk_mgr.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c
b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c
index 3ce0ee0d012f..694a9d3d92ae 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn30/dcn30_clk_mgr.c
@@ -577,8 +577,7 @@ void dcn3_clk_mgr_construct(
void dcn3_clk_mgr_destroy(struct clk_mgr_internal *clk_mgr)
{
- if (clk_mgr->base.bw_params)
- kfree(clk_mgr->base.bw_params);
+ kfree(clk_mgr->base.bw_params);
if (clk_mgr->wm_range_table)
dm_helpers_free_gpu_mem(clk_mgr->base.ctx,
DC_MEM_ALLOC_TYPE_GART,
diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn32/dcn32_clk_mgr.c
b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn32/dcn32_clk_mgr.c
index 200fcec19186..ba9814f88f48 100644
--- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn32/dcn32_clk_mgr.c
+++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn32/dcn32_clk_mgr.c
@@ -783,8 +783,7 @@ void dcn32_clk_mgr_construct(
void dcn32_clk_mgr_destroy(struct clk_mgr_internal *clk_mgr)
{
- if (clk_mgr->base.bw_params)
- kfree(clk_mgr->base.bw_params);
+ kfree(clk_mgr->base.bw_params);
if (clk_mgr->wm_range_table)
dm_helpers_free_gpu_mem(clk_mgr->base.ctx,
DC_MEM_ALLOC_TYPE_GART,
--
2.34.1
Hi,
Reviewed-by: Rodrigo Siqueira
And applied to amd-staging-drm-next.
Thanks
Siqueira
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
On Tue, Jan 10, 2023 at 12:19:35PM +, Tvrtko Ursulin wrote:
>
> On 10/01/2023 11:28, Tvrtko Ursulin wrote:
> >
> >
> > On 09/01/2023 17:27, Jason Ekstrand wrote:
> >
> > [snip]
> >
> > > >>> AFAICT it proposes to have 1:1 between *userspace* created
> > > contexts (per
> > > >>> context _and_ engine) and drm_sched. I am not sure avoiding
> > > invasive changes
> > > >>> to the shared code is in the spirit of the overall idea and
> > > instead
> > > >>> opportunity should be used to look at way to refactor/improve
> > > drm_sched.
> > >
> > >
> > > Maybe? I'm not convinced that what Xe is doing is an abuse at all
> > > or really needs to drive a re-factor. (More on that later.)
> > > There's only one real issue which is that it fires off potentially a
> > > lot of kthreads. Even that's not that bad given that kthreads are
> > > pretty light and you're not likely to have more kthreads than
> > > userspace threads which are much heavier. Not ideal, but not the
> > > end of the world either. Definitely something we can/should
> > > optimize but if we went through with Xe without this patch, it would
> > > probably be mostly ok.
> > >
> > > >> Yes, it is 1:1 *userspace* engines and drm_sched.
> > > >>
> > > >> I'm not really prepared to make large changes to DRM scheduler
> > > at the
> > > >> moment for Xe as they are not really required nor does Boris
> > > seem they
> > > >> will be required for his work either. I am interested to see
> > > what Boris
> > > >> comes up with.
> > > >>
> > > >>> Even on the low level, the idea to replace drm_sched threads
> > > with workers
> > > >>> has a few problems.
> > > >>>
> > > >>> To start with, the pattern of:
> > > >>>
> > > >>> while (not_stopped) {
> > > >>> keep picking jobs
> > > >>> }
> > > >>>
> > > >>> Feels fundamentally in disagreement with workers (while
> > > obviously fits
> > > >>> perfectly with the current kthread design).
> > > >>
> > > >> The while loop breaks and worker exists if no jobs are ready.
> > >
> > >
> > > I'm not very familiar with workqueues. What are you saying would fit
> > > better? One scheduling job per work item rather than one big work
> > > item which handles all available jobs?
> >
> > Yes and no, it indeed IMO does not fit to have a work item which is
> > potentially unbound in runtime. But it is a bit moot conceptual mismatch
> > because it is a worst case / theoretical, and I think due more
> > fundamental concerns.
> >
> > If we have to go back to the low level side of things, I've picked this
> > random spot to consolidate what I have already mentioned and perhaps
> > expand.
> >
> > To start with, let me pull out some thoughts from workqueue.rst:
> >
> > """
> > Generally, work items are not expected to hog a CPU and consume many
> > cycles. That means maintaining just enough concurrency to prevent work
> > processing from stalling should be optimal.
> > """
> >
> > For unbound queues:
> > """
> > The responsibility of regulating concurrency level is on the users.
> > """
> >
> > Given the unbound queues will be spawned on demand to service all queued
> > work items (more interesting when mixing up with the system_unbound_wq),
> > in the proposed design the number of instantiated worker threads does
> > not correspond to the number of user threads (as you have elsewhere
> > stated), but pessimistically to the number of active user contexts. That
> > is the number which drives the maximum number of not-runnable jobs that
> > can become runnable at once, and hence spawn that many work items, and
> > in turn unbound worker threads.
> >
> > Several problems there.
> >
> > It is fundamentally pointless to have potentially that many more threads
> > than the number of CPU cores - it simply creates a scheduling storm.
>
> To make matters worse, if I follow the code correctly, all these per user
> context worker thread / work items end up contending on the same lock or
> circular buffer, both are one instance per GPU:
>
> guc_engine_run_job
> -> submit_engine
> a) wq_item_append
> -> wq_wait_for_space
> -> msleep
a) is dedicated per xe_engine
Also you missed the step of programming the ring which is dedicated per
xe_engine
> b) xe_guc_ct_send
> -> guc_ct_send
> -> mutex_lock(&ct->lock);
> -> later a potential msleep in h2g_has_room
Techincally there is 1 instance per GT not GPU, yes this is shared but
in practice there will always be space in the CT channel so contention
on the lock should be rare.
I haven't read your rather long reply yet, but also FWIW using a
workqueue has suggested by AMD (original authors of the DRM scheduler)
when we ran this design by them.
Matt
>
> Regards,
>
> Tvrtko
Re: [PATCH] drm/amd/display: Fix set scaling doesn's work
On 11/22/22 06:20, hongao wrote: [Why] Setting scaling does not correctly update CRTC state. As a result dc stream state's src (composition area) && dest (addressable area) was not calculated as expected. This causes set scaling doesn's work. [How] Correctly update CRTC state when setting scaling property. Signed-off-by: hongao 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 3e1ecca72430..a88a6f758748 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -9386,8 +9386,8 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, goto fail; } - if (dm_old_con_state->abm_level != - dm_new_con_state->abm_level) + if (dm_old_con_state->abm_level != dm_new_con_state->abm_level || + dm_old_con_state->scaling != dm_new_con_state->scaling) new_crtc_state->connectors_changed = true; } Hi, This change lgtm, and I also run it in our CI, and from IGT perspective, we are good. Harry, do you have any comment about this change? Thanks Siqueira
Re: [PATCH v4] drm/i915: Do not cover all future platforms in TLB invalidation
On Tue, Jan 10, 2023 at 11:35:33AM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Revert to the original explicit approach and document the reasoning
> behind it.
>
> v2:
> * DG2 needs to be covered too. (Matt)
>
> v3:
> * Full version check for Gen12 to avoid catching all future platforms.
>(Matt)
>
> v4:
> * Be totally explicit on the Gen12 branch. (Andrzej)
>
> Signed-off-by: Tvrtko Ursulin
> Cc: Matt Roper
> Cc: Balasubramani Vivekanandan
> Cc: Andrzej Hajda
> Reviewed-by: Andrzej Hajda # v1
> Reviewed-by: Matt Roper # v3
Reviewed-by: Matt Roper
for v4 as well.
Matt
> ---
> drivers/gpu/drm/i915/gt/intel_gt.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 75a7cb33..5721bf85d119 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -1070,10 +1070,23 @@ static void mmio_invalidate_full(struct intel_gt *gt)
> unsigned int num = 0;
> unsigned long flags;
>
> - if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 50)) {
> + /*
> + * New platforms should not be added with catch-all-newer (>=)
> + * condition so that any later platform added triggers the below warning
> + * and in turn mandates a human cross-check of whether the invalidation
> + * flows have compatible semantics.
> + *
> + * For instance with the 11.00 -> 12.00 transition three out of five
> + * respective engine registers were moved to masked type. Then after the
> + * 12.00 -> 12.50 transition multi cast handling is required too.
> + */
> +
> + if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 50) ||
> + GRAPHICS_VER_FULL(i915) == IP_VER(12, 55)) {
> regs = NULL;
> num = ARRAY_SIZE(xehp_regs);
> - } else if (GRAPHICS_VER(i915) == 12) {
> + } else if (GRAPHICS_VER_FULL(i915) == IP_VER(12, 0) ||
> +GRAPHICS_VER_FULL(i915) == IP_VER(12, 10)) {
> regs = gen12_regs;
> num = ARRAY_SIZE(gen12_regs);
> } else if (GRAPHICS_VER(i915) >= 8 && GRAPHICS_VER(i915) <= 11) {
> --
> 2.34.1
>
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
next-20230110: arm64: defconfig+kselftest config boot failed - Unable to handle kernel paging request at virtual address fffffffffffffff8
[ please ignore this email if this regression already reported ] Today's Linux next tag next-20230110 boot passes with defconfig but boot fails with defconfig + kselftest merge config on arm64 devices and qemu-arm64. Reported-by: Linux Kernel Functional Testing We are bisecting this problem and get back to you shortly. GOOD: next-20230109 (defconfig + kselftests configs) BAD: next-20230110 (defconfig + kselftests configs) kernel crash log [1]: [ 15.302140] Unable to handle kernel paging request at virtual address fff8 [ 15.309906] Mem abort info: [ 15.312659] ESR = 0x9604 [ 15.316365] EC = 0x25: DABT (current EL), IL = 32 bits [ 15.321626] SET = 0, FnV = 0 [ 15.324644] EA = 0, S1PTW = 0 [ 15.327744] FSC = 0x04: level 0 translation fault [ 15.332619] Data abort info: [ 15.335422] ISV = 0, ISS = 0x0004 [ 15.339226] CM = 0, WnR = 0 [ 15.342154] swapper pgtable: 4k pages, 48-bit VAs, pgdp=1496c000 [ 15.348795] [fff8] pgd=, p4d= [ 15.355524] Internal error: Oops: 9604 [#1] PREEMPT SMP [ 15.361729] Modules linked in: meson_gxl dwmac_generic snd_soc_meson_gx_sound_card snd_soc_meson_card_utils lima gpu_sched drm_shmem_helper meson_drm drm_dma_helper crct10dif_ce meson_ir rc_core meson_dw_hdmi dw_hdmi meson_canvas dwmac_meson8b stmmac_platform meson_rng stmmac rng_core cec meson_gxbb_wdt drm_display_helper snd_soc_meson_aiu snd_soc_meson_codec_glue pcs_xpcs snd_soc_meson_t9015 amlogic_gxl_crypto crypto_engine display_connector snd_soc_simple_amplifier drm_kms_helper drm nvmem_meson_efuse [ 15.405976] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted 6.2.0-rc3-next-20230110 #1 [ 15.413563] Hardware name: Libre Computer AML-S905X-CC (DT) [ 15.419086] Workqueue: events_unbound deferred_probe_work_func [ 15.424863] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 15.431762] pc : of_drm_find_bridge+0x38/0x70 [drm] [ 15.436594] lr : of_drm_find_bridge+0x20/0x70 [drm] [ 15.441423] sp : 8a04b9b0 [ 15.444700] x29: 8a04b9b0 x28: 08de5810 x27: 08de5808 [ 15.451772] x26: 08de5800 x25: 084cb8b0 x24: 01223c00 [ 15.458844] x23: x22: 0001 x21: 7fa61a28 [ 15.465917] x20: 084ca080 x19: 7fa61a28 x18: 019bd700 [ 15.472989] x17: 6d64685f77645f6e x16: x15: 0004 [ 15.480062] x14: 89bab410 x13: x12: 0003 [ 15.487135] x11: x10: x9 : [ 15.494207] x8 : 810a70a0 x7 : 64410079616b6f01 x6 : 80416403 [ 15.501279] x5 : 03644100 x4 : 0080 x3 : 00416400 [ 15.508352] x2 : 01128000 x1 : x0 : [ 15.515426] Call trace: [ 15.517863] Insufficient stack space to handle exception! [ 15.517867] ESR: 0x9647 -- DABT (current EL) [ 15.517871] FAR: 0x8a047ff0 [ 15.517873] Task stack: [0x8a048000..0x8a04c000] [ 15.517877] IRQ stack: [0x88008000..0x8800c000] [ 15.517880] Overflow stack: [0x7d9c1320..0x7d9c2320] [ 15.517884] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted 6.2.0-rc3-next-20230110 #1 [ 15.517890] Hardware name: Libre Computer AML-S905X-CC (DT) [ 15.517895] Workqueue: events_unbound deferred_probe_work_func [ 15.517915] pstate: 83c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 15.517923] pc : el1_abort+0x4/0x5c [ 15.517932] lr : el1h_64_sync_handler+0x60/0xac [ 15.517939] sp : 8a048020 [ 15.517941] x29: 8a048020 x28: 01128000 x27: 08de5808 [ 15.517950] x26: 08de5800 x25: 8a04b608 x24: 01128000 [ 15.517957] x23: a0c5 x22: 880321dc x21: 8a048180 [ 15.517965] x20: 898e1000 x19: 8a048290 x18: 019bd700 [ 15.517972] x17: 0011 x16: x15: 0004 [ 15.517979] x14: 89bab410 x13: x12: [ 15.517986] x11: 0030 x10: 89013a1c x9 : 890401a0 [ 15.517994] x8 : 0025 x7 : 205d363234353135 x6 : 352e35312020205b [ 15.518001] x5 : 89f766b7 x4 : 88fe695c x3 : 000c [ 15.518008] x2 : 9604 x1 : 9604 x0 : 8a048030 [ 15.518017] Kernel panic - not syncing: kernel stack overflow [ 15.518020] SMP: stopping secondary CPUs [ 15.518027] Kernel Offset: disabled [ 15.518029] CPU features: 0x0,01000100,421b [ 15.518034] Memory Limit: none [ 15.679388] ---[ end Kernel panic - not syncing: kernel stack overflow ]--- [1] https://storage.kernelci.org/next/master/next-20230110/arm64/defconfig/clang-16/lab-broonie/kselftest-arm64-meson-gxl-s905x-libretech-cc
Re: [PATCH 1/2] backlight: pwm_bl: configure pwm only once per backlight toggle
On Mon, Jan 09, 2023 at 09:47:57PM +0100, Uwe Kleine-König wrote: > When the function pwm_backlight_update_status() was called with > brightness > 0, pwm_get_state() was called twice (once directly and once > in compute_duty_cycle). Also pwm_apply_state() was called twice (once in > pwm_backlight_power_on() and once directly). > > Optimize this to do both calls only once. > > Signed-off-by: Uwe Kleine-König This will reverse the order in which the regulator is toggled versus the PWM starting/stopping. It would be nice to that in the description. However I can't see why it would be a problem (since both remain in the same place relative to the sleeps) so: Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight
On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote: > Most but not all PWMs drive the PWM pin to its inactive state when > disabled. Rely on the lowlevel PWM implementation to implement > duty_cycle = 0 in an energy efficient way and don't disable the PWM. I'm a little worried about this one. I thought the PWM APIs allow the duty cycle to be rounded up or down slightly during the apply. So when you say "rely on the lowlevel to implement duty_cycle = 0 to..." is it confirmed that this is true (and that all PWMs *can* implement a duty_cycle of 0 without rounding up)? Daniel. > This fixes backlight disabling e.g. on i.MX6 when an inverted PWM is > used. > > Signed-off-by: Uwe Kleine-König > --- > drivers/video/backlight/pwm_bl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/video/backlight/pwm_bl.c > b/drivers/video/backlight/pwm_bl.c > index 0509fecd5715..7bdc5d570a12 100644 > --- a/drivers/video/backlight/pwm_bl.c > +++ b/drivers/video/backlight/pwm_bl.c > @@ -109,7 +109,7 @@ static int pwm_backlight_update_status(struct > backlight_device *bl) > pwm_backlight_power_off(pb); > > pwm_get_state(pb->pwm, &state); > - state.enabled = false; > + state.enabled = true; > state.duty_cycle = 0; > pwm_apply_state(pb->pwm, &state); > } > -- > 2.39.0 >
Re: [PATCH v3 0/7] drm/bridge_connector: perform HPD enablement automatically
On 10/01/2023 08:57, Laurentiu Palcu wrote: Hi, On Mon, Jan 09, 2023 at 10:26:28PM +0200, Dmitry Baryshkov wrote: Hi, On 09/01/2023 18:21, Laurentiu Palcu wrote: Hi Dmitry, It looks like there are some issues with this patchset... :/ I just fetched the drm-tip and, with these patches included, the "Hot plug detection already enabled" warning is back for i.MX DCSS. Could you please provide a backtrace? Sure, see below: I wondered, why didn't I see this on msm, my main target nowadays. The msm driver is calling msm_kms_helper_poll_init() after initializing fbdev, so all previous kms_helper_poll_enable() calls return early. I think I have the fix ready. Let me test it locally before posting. [ cut here ] Hot plug detection already enabled WARNING: CPU: 2 PID: 9 at drivers/gpu/drm/drm_bridge.c:1257 drm_bridge_hpd_enable+0x94/0x9c [drm] Modules linked in: videobuf2_memops snd_soc_simple_card snd_soc_simple_card_utils fsl_imx8_ddr_perf videobuf2_common snd_soc_imx_spdif adv7511 etnaviv imx8m_ddrc imx_dcss mc cec nwl_dsi gov CPU: 2 PID: 9 Comm: kworker/u8:0 Not tainted 6.2.0-rc2-15208-g25b283acd578 #6 Hardware name: NXP i.MX8MQ EVK (DT) Workqueue: events_unbound deferred_probe_work_func pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : drm_bridge_hpd_enable+0x94/0x9c [drm] lr : drm_bridge_hpd_enable+0x94/0x9c [drm] sp : 89ef3740 x29: 89ef3740 x28: 09331f00 x27: 1000 x26: 0020 x25: 81148ed8 x24: 0a8fe000 x23: fffd x22: 05086348 x21: 81133ee0 x20: 0550d800 x19: 05086288 x18: 0006 x17: x16: 896ef008 x15: 972891004260 x14: 2a1403e19400 x13: 972891004260 x12: 2a1403e19400 x11: 7100385f29400801 x10: 0aa0 x9 : 88112744 x8 : 00250b00 x7 : 0003 x6 : 0011 x5 : x4 : bd986a48 x3 : 0001 x2 : x1 : x0 : 0025 Call trace: drm_bridge_hpd_enable+0x94/0x9c [drm] drm_bridge_connector_enable_hpd+0x2c/0x3c [drm_kms_helper] drm_kms_helper_poll_enable+0x94/0x10c [drm_kms_helper] drm_helper_probe_single_connector_modes+0x1a8/0x510 [drm_kms_helper] drm_client_modeset_probe+0x204/0x1190 [drm] __drm_fb_helper_initial_config_and_unlock+0x5c/0x4a4 [drm_kms_helper] drm_fb_helper_initial_config+0x54/0x6c [drm_kms_helper] drm_fbdev_client_hotplug+0xd0/0x140 [drm_kms_helper] drm_fbdev_generic_setup+0x90/0x154 [drm_kms_helper] dcss_kms_attach+0x1c8/0x254 [imx_dcss] dcss_drv_platform_probe+0x90/0xfc [imx_dcss] platform_probe+0x70/0xcc really_probe+0xc4/0x2e0 __driver_probe_device+0x80/0xf0 driver_probe_device+0xe0/0x164 __device_attach_driver+0xc0/0x13c bus_for_each_drv+0x84/0xe0 __device_attach+0xa4/0x1a0 device_initial_probe+0x1c/0x30 bus_probe_device+0xa4/0xb0 deferred_probe_work_func+0x90/0xd0 process_one_work+0x200/0x474 worker_thread+0x74/0x43c kthread+0xfc/0x110 ret_from_fork+0x10/0x20 ---[ end trace ]--- Cheers, Laurentiu After a short investigation, it seems that we end up calling drm_bridge_hpd_enable() from both drm_kms_helper_poll_init() and drm_fbdev_generic_setup(), hence the warning. There are drivers using the drm_bridge_connector API that also call drm_kms_helper_poll_init() followed by drm_fbdev_generic_setup(). So, they might experience the same behavior, unless I'm missing something... :/ Also, even if drm_fbdev_generic_setup() is not called in the driver initialization, the warning will still appear the first time the GETCONNECTOR ioctl is called, because that'll call drm_helper_probe_single_connector_modes() helper which will eventually call drm_bridge_hpd_enable(). Any idea? Cheers, Laurentiu On Wed, Nov 02, 2022 at 09:06:58PM +0300, Dmitry Baryshkov wrote: From all the drivers using drm_bridge_connector only iMX/dcss and OMAP DRM driver do a proper work of calling drm_bridge_connector_en/disable_hpd() in right places. Rather than teaching each and every driver how to properly handle drm_bridge_connector's HPD, make that automatic. Add two additional drm_connector helper funcs: enable_hpd() and disable_hpd(). Make drm_kms_helper_poll_* functions call them (as this is the time where the drm_bridge_connector's functions are called by the drivers too). Changes since v2: - Fixed a typo in the commit message of the second patch. Changes since v1: - Rebased on top of v6.1-rc1 - Removed the drm_bridge_connector_enable_hpd() from drm_bridge_connector_init() - Removed extra underscore prefix from drm_bridge_connector_en/disable_hpd() helpers Dmitry Baryshkov (7): drm/poll-helper: merge drm_kms_helper_poll_disable() and _fini() drm/probe-helper: enable and disable HPD on connectors drm/bridge_connector: rely on drm_kms_helper_poll_* for HPD enablement drm/imx/d
Re: next-20230110: arm64: defconfig+kselftest config boot failed - Unable to handle kernel paging request at virtual address fffffffffffffff8
[+ James and Nathan]
On Tue, Jan 10, 2023 at 09:44:40PM +0530, Naresh Kamboju wrote:
> [ please ignore this email if this regression already reported ]
>
> Today's Linux next tag next-20230110 boot passes with defconfig but
> boot fails with
> defconfig + kselftest merge config on arm64 devices and qemu-arm64.
>
> Reported-by: Linux Kernel Functional Testing
>
> We are bisecting this problem and get back to you shortly.
>
> GOOD: next-20230109 (defconfig + kselftests configs)
> BAD: next-20230110 (defconfig + kselftests configs)
I couldn't find a kselftests .config in the tree (assumedly I'm just ont
looking hard enough), but does that happen to enable CONFIG_STACK_TRACER=y?
If so, since you're using clang, I wonder if this is an issue with
68a63a412d18 ("arm64: Fix build with CC=clang, CONFIG_FTRACE=y and
CONFIG_STACK_TRACER=y")?
Please let us know how the bisection goes...
Will
> kernel crash log [1]:
>
> [ 15.302140] Unable to handle kernel paging request at virtual
> address fff8
> [ 15.309906] Mem abort info:
> [ 15.312659] ESR = 0x9604
> [ 15.316365] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 15.321626] SET = 0, FnV = 0
> [ 15.324644] EA = 0, S1PTW = 0
> [ 15.327744] FSC = 0x04: level 0 translation fault
> [ 15.332619] Data abort info:
> [ 15.335422] ISV = 0, ISS = 0x0004
> [ 15.339226] CM = 0, WnR = 0
> [ 15.342154] swapper pgtable: 4k pages, 48-bit VAs, pgdp=1496c000
> [ 15.348795] [fff8] pgd=, p4d=
> [ 15.355524] Internal error: Oops: 9604 [#1] PREEMPT SMP
> [ 15.361729] Modules linked in: meson_gxl dwmac_generic
> snd_soc_meson_gx_sound_card snd_soc_meson_card_utils lima gpu_sched
> drm_shmem_helper meson_drm drm_dma_helper crct10dif_ce meson_ir
> rc_core meson_dw_hdmi dw_hdmi meson_canvas dwmac_meson8b
> stmmac_platform meson_rng stmmac rng_core cec meson_gxbb_wdt
> drm_display_helper snd_soc_meson_aiu snd_soc_meson_codec_glue pcs_xpcs
> snd_soc_meson_t9015 amlogic_gxl_crypto crypto_engine display_connector
> snd_soc_simple_amplifier drm_kms_helper drm nvmem_meson_efuse
> [ 15.405976] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted
> 6.2.0-rc3-next-20230110 #1
> [ 15.413563] Hardware name: Libre Computer AML-S905X-CC (DT)
> [ 15.419086] Workqueue: events_unbound deferred_probe_work_func
> [ 15.424863] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 15.431762] pc : of_drm_find_bridge+0x38/0x70 [drm]
> [ 15.436594] lr : of_drm_find_bridge+0x20/0x70 [drm]
> [ 15.441423] sp : 8a04b9b0
> [ 15.444700] x29: 8a04b9b0 x28: 08de5810 x27:
> 08de5808
> [ 15.451772] x26: 08de5800 x25: 084cb8b0 x24:
> 01223c00
> [ 15.458844] x23: x22: 0001 x21:
> 7fa61a28
> [ 15.465917] x20: 084ca080 x19: 7fa61a28 x18:
> 019bd700
> [ 15.472989] x17: 6d64685f77645f6e x16: x15:
> 0004
> [ 15.480062] x14: 89bab410 x13: x12:
> 0003
> [ 15.487135] x11: x10: x9 :
>
> [ 15.494207] x8 : 810a70a0 x7 : 64410079616b6f01 x6 :
> 80416403
> [ 15.501279] x5 : 03644100 x4 : 0080 x3 :
> 00416400
> [ 15.508352] x2 : 01128000 x1 : x0 :
>
> [ 15.515426] Call trace:
> [ 15.517863] Insufficient stack space to handle exception!
> [ 15.517867] ESR: 0x9647 -- DABT (current EL)
> [ 15.517871] FAR: 0x8a047ff0
> [ 15.517873] Task stack: [0x8a048000..0x80000a04c000]
> [ 15.517877] IRQ stack: [0x88008000..0x8800c000]
> [ 15.517880] Overflow stack: [0x7d9c1320..0x7d9c2320]
> [ 15.517884] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted
> 6.2.0-rc3-next-20230110 #1
> [ 15.517890] Hardware name: Libre Computer AML-S905X-CC (DT)
> [ 15.517895] Workqueue: events_unbound deferred_probe_work_func
> [ 15.517915] pstate: 83c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 15.517923] pc : el1_abort+0x4/0x5c
> [ 15.517932] lr : el1h_64_sync_handler+0x60/0xac
> [ 15.517939] sp : 8a048020
> [ 15.517941] x29: 8a048020 x28: 01128000 x27:
> 08de5808
> [ 15.517950] x26: 08de5800 x25: 8a04b608 x24:
> 01128000
> [ 15.517957] x23: a0c5 x22: 880321dc x21:
> 8a048180
> [ 15.517965] x20: 898e1000 x19: 8a048290 x18:
> 019bd700
> [
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
On Tue, Jan 10, 2023 at 11:28:08AM +, Tvrtko Ursulin wrote:
>
>
> On 09/01/2023 17:27, Jason Ekstrand wrote:
>
> [snip]
>
> > >>> AFAICT it proposes to have 1:1 between *userspace* created
> > contexts (per
> > >>> context _and_ engine) and drm_sched. I am not sure avoiding
> > invasive changes
> > >>> to the shared code is in the spirit of the overall idea and instead
> > >>> opportunity should be used to look at way to refactor/improve
> > drm_sched.
> >
> >
> > Maybe? I'm not convinced that what Xe is doing is an abuse at all or
> > really needs to drive a re-factor. (More on that later.) There's only
> > one real issue which is that it fires off potentially a lot of kthreads.
> > Even that's not that bad given that kthreads are pretty light and you're
> > not likely to have more kthreads than userspace threads which are much
> > heavier. Not ideal, but not the end of the world either. Definitely
> > something we can/should optimize but if we went through with Xe without
> > this patch, it would probably be mostly ok.
> >
> > >> Yes, it is 1:1 *userspace* engines and drm_sched.
> > >>
> > >> I'm not really prepared to make large changes to DRM scheduler
> > at the
> > >> moment for Xe as they are not really required nor does Boris
> > seem they
> > >> will be required for his work either. I am interested to see
> > what Boris
> > >> comes up with.
> > >>
> > >>> Even on the low level, the idea to replace drm_sched threads
> > with workers
> > >>> has a few problems.
> > >>>
> > >>> To start with, the pattern of:
> > >>>
> > >>> while (not_stopped) {
> > >>> keep picking jobs
> > >>> }
> > >>>
> > >>> Feels fundamentally in disagreement with workers (while
> > obviously fits
> > >>> perfectly with the current kthread design).
> > >>
> > >> The while loop breaks and worker exists if no jobs are ready.
> >
> >
> > I'm not very familiar with workqueues. What are you saying would fit
> > better? One scheduling job per work item rather than one big work item
> > which handles all available jobs?
>
> Yes and no, it indeed IMO does not fit to have a work item which is
> potentially unbound in runtime. But it is a bit moot conceptual mismatch
> because it is a worst case / theoretical, and I think due more fundamental
> concerns.
>
> If we have to go back to the low level side of things, I've picked this
> random spot to consolidate what I have already mentioned and perhaps expand.
>
> To start with, let me pull out some thoughts from workqueue.rst:
>
> """
> Generally, work items are not expected to hog a CPU and consume many cycles.
> That means maintaining just enough concurrency to prevent work processing
> from stalling should be optimal.
> """
>
> For unbound queues:
> """
> The responsibility of regulating concurrency level is on the users.
> """
>
> Given the unbound queues will be spawned on demand to service all queued
> work items (more interesting when mixing up with the system_unbound_wq), in
> the proposed design the number of instantiated worker threads does not
> correspond to the number of user threads (as you have elsewhere stated), but
> pessimistically to the number of active user contexts. That is the number
> which drives the maximum number of not-runnable jobs that can become
> runnable at once, and hence spawn that many work items, and in turn unbound
> worker threads.
>
> Several problems there.
>
> It is fundamentally pointless to have potentially that many more threads
> than the number of CPU cores - it simply creates a scheduling storm.
>
We can use a different work queue if this is an issue, have a FIXME
which indicates we should allow the user to pass in the work queue.
> Unbound workers have no CPU / cache locality either and no connection with
> the CPU scheduler to optimize scheduling patterns. This may matter either on
> large systems or on small ones. Whereas the current design allows for
> scheduler to notice userspace CPU thread keeps waking up the same drm
> scheduler kernel thread, and so it can keep them on the same CPU, the
> unbound workers lose that ability and so 2nd CPU might be getting woken up
> from low sleep for every submission.
>
I guess I don't understand kthread vs. workqueue scheduling internals.
> Hence, apart from being a bit of a impedance mismatch, the proposal has the
> potential to change performance and power patterns and both large and small
> machines.
>
We are going to have to test this out I suppose and play around to see
if this design has any real world impacts. As Jason said, yea probably
will need a bit of help here from others. Will CC relavent parties on
next rev.
> > >>> Secondly, it probably demands separate workers (not optional),
> > otherwise
> > >>> behaviour of shared workqueues has either the potential to
> > explode numb
[linux-next:master] BUILD REGRESSION 435bf71af3a0aa8067f3b87ff9febf68b564dbb6
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master branch HEAD: 435bf71af3a0aa8067f3b87ff9febf68b564dbb6 Add linux-next specific files for 20230110 Error/Warning reports: https://lore.kernel.org/oe-kbuild-all/202301102024.acwvrffq-...@intel.com Error/Warning: (recently discovered and may have been fixed) Warning: Documentation/arm/samsung/gpio.rst references a file that doesn't exist: Documentation/arm/samsung-s3c24xx/gpio.rst aarch64-linux-ld: ID map text too big or misaligned drivers/gpu/drm/ttm/ttm_bo_util.c:364:32: error: implicit declaration of function 'vmap'; did you mean 'kmap'? [-Werror=implicit-function-declaration] drivers/gpu/drm/ttm/ttm_bo_util.c:429:17: error: implicit declaration of function 'vunmap'; did you mean 'kunmap'? [-Werror=implicit-function-declaration] Unverified Error/Warning (likely false positive, please contact us if interested): net/devlink/leftover.c:7608 devlink_fmsg_prepare_skb() error: uninitialized symbol 'err'. Error/Warning ids grouped by kconfigs: gcc_recent_errors |-- arm64-allyesconfig | `-- aarch64-linux-ld:ID-map-text-too-big-or-misaligned |-- mips-allyesconfig | |-- drivers-gpu-drm-ttm-ttm_bo_util.c:error:implicit-declaration-of-function-vmap | `-- drivers-gpu-drm-ttm-ttm_bo_util.c:error:implicit-declaration-of-function-vunmap |-- x86_64-allnoconfig | `-- Warning:Documentation-arm-samsung-gpio.rst-references-a-file-that-doesn-t-exist:Documentation-arm-samsung-s3c24xx-gpio.rst `-- x86_64-randconfig-m001-20230109 `-- net-devlink-leftover.c-devlink_fmsg_prepare_skb()-error:uninitialized-symbol-err-. elapsed time: 723m configs tested: 76 configs skipped: 6 gcc tested configs: x86_64allnoconfig arc defconfig s390 allmodconfig alpha defconfig x86_64 randconfig-a011-20230109 i386 randconfig-a014-20230109 x86_64 randconfig-a013-20230109 i386 randconfig-a011-20230109 s390defconfig x86_64 randconfig-a012-20230109 i386 randconfig-a016-20230109 i386defconfig i386 randconfig-a015-20230109 x86_64 randconfig-a014-20230109 i386 randconfig-a013-20230109 x86_64 randconfig-a016-20230109 x86_64 randconfig-a015-20230109 x86_64 defconfig s390 allyesconfig i386 randconfig-a012-20230109 x86_64 rhel-8.3 x86_64 allyesconfig ia64 allmodconfig i386 allyesconfig arm defconfig riscvrandconfig-r042-20230109 s390 randconfig-r044-20230109 arm64allyesconfig arm randconfig-r046-20230108 arc randconfig-r043-20230108 arm allyesconfig arc randconfig-r043-20230109 sh sdk7780_defconfig xtensa audio_kc705_defconfig um i386_defconfig um x86_64_defconfig powerpc allnoconfig x86_64 rhel-8.3-func x86_64rhel-8.3-kselftests x86_64 rhel-8.3-bpf x86_64 rhel-8.3-syz x86_64 rhel-8.3-kunit x86_64 rhel-8.3-kvm m68k allyesconfig m68k allmodconfig arc allyesconfig arm64 defconfig alphaallyesconfig sh allmodconfig mips allyesconfig powerpc allmodconfig m68kdefconfig mips gcw0_defconfig powerpc ppc40x_defconfig clang tested configs: i386 randconfig-a004-20230109 i386 randconfig-a002-20230109 i386 randconfig-a003-20230109 i386 randconfig-a006-20230109 i386 randconfig-a001-20230109 i386 randconfig-a005-20230109 hexagon randconfig-r045-20230109 arm randconfig-r046-20230109 riscvrandconfig-r042-20230108 hexagon randconfig-r041-20230108 x86_64 randconfig-a003-20230109 hexagon randconfig-r041-20230109 x86_64 randconfig-a002-20230109 hexagon randconfig-r045-20230108 x86_64 randconfig-a004-20230109 x86_64 randco
Re: next-20230110: arm64: defconfig+kselftest config boot failed - Unable to handle kernel paging request at virtual address fffffffffffffff8
On Tue, Jan 10, 2023, at 17:14, Naresh Kamboju wrote:
> [ please ignore this email if this regression already reported ]
>
> Today's Linux next tag next-20230110 boot passes with defconfig but
> boot fails with
> defconfig + kselftest merge config on arm64 devices and qemu-arm64.
>
> Reported-by: Linux Kernel Functional Testing
>
> We are bisecting this problem and get back to you shortly.
>
> GOOD: next-20230109 (defconfig + kselftests configs)
> BAD: next-20230110 (defconfig + kselftests configs)
>
> kernel crash log [1]:
>
> [ 15.302140] Unable to handle kernel paging request at virtual
> address fff8
> [ 15.309906] Mem abort info:
> [ 15.312659] ESR = 0x9604
> [ 15.316365] EC = 0x25: DABT (current EL), IL = 32 bits
> [ 15.321626] SET = 0, FnV = 0
> [ 15.324644] EA = 0, S1PTW = 0
> [ 15.327744] FSC = 0x04: level 0 translation fault
> [ 15.332619] Data abort info:
> [ 15.335422] ISV = 0, ISS = 0x0004
> [ 15.339226] CM = 0, WnR = 0
> [ 15.342154] swapper pgtable: 4k pages, 48-bit VAs, pgdp=1496c000
> [ 15.348795] [fff8] pgd=, p4d=
> [ 15.355524] Internal error: Oops: 9604 [#1] PREEMPT SMP
> [ 15.361729] Modules linked in: meson_gxl dwmac_generic
> snd_soc_meson_gx_sound_card snd_soc_meson_card_utils lima gpu_sched
> drm_shmem_helper meson_drm drm_dma_helper crct10dif_ce meson_ir
> rc_core meson_dw_hdmi dw_hdmi meson_canvas dwmac_meson8b
> stmmac_platform meson_rng stmmac rng_core cec meson_gxbb_wdt
> drm_display_helper snd_soc_meson_aiu snd_soc_meson_codec_glue pcs_xpcs
> snd_soc_meson_t9015 amlogic_gxl_crypto crypto_engine display_connector
> snd_soc_simple_amplifier drm_kms_helper drm nvmem_meson_efuse
> [ 15.405976] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted
> 6.2.0-rc3-next-20230110 #1
> [ 15.413563] Hardware name: Libre Computer AML-S905X-CC (DT)
> [ 15.419086] Workqueue: events_unbound deferred_probe_work_func
> [ 15.424863] pstate: 0005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 15.431762] pc : of_drm_find_bridge+0x38/0x70 [drm]
> [ 15.436594] lr : of_drm_find_bridge+0x20/0x70 [drm]
The line is
drivers/gpu/drm/drm_bridge.c:1310: if (bridge->of_node == np) {
The list_head here is a NULL pointer, so ->of_node points
to address negative 8, i.e. fff8
This is linked list corruption, which typically happens as
part of a use-after-free, and could be the result of a
failed registration causing an object to be freed after
it is added to the list.
Unfortunately, there are no patches to this file between
next-20230109 and next-20230110, so the bug probably is
not actually in this file.
> [ 15.515426] Call trace:
> [ 15.517863] Insufficient stack space to handle exception!
> [ 15.517867] ESR: 0x9647 -- DABT (current EL)
> [ 15.517871] FAR: 0x8a047ff0
> [ 15.517873] Task stack: [0x8a048000..0x8a04c000]
> [ 15.517877] IRQ stack: [0x88008000..0x8800c000]
> [ 15.517880] Overflow stack: [0x7d9c1320..0x7d9c2320]
> [ 15.517884] CPU: 1 PID: 9 Comm: kworker/u8:0 Not tainted
> 6.2.0-rc3-next-20230110 #1
> [ 15.517890] Hardware name: Libre Computer AML-S905X-CC (DT)
> [ 15.517895] Workqueue: events_unbound deferred_probe_work_func
> [ 15.517915] pstate: 83c5 (Nzcv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [ 15.517923] pc : el1_abort+0x4/0x5c
> [ 15.517932] lr : el1h_64_sync_handler+0x60/0xac
> [ 15.517939] sp : 8a048020
Not sure about the missing stack trace: I can see that the stack
pointer is on a task stack, which is reported as having overflown,
but I don't see why it's unable to print the stack while running
from the overflow stack.
A stack overflow is often caused by unbounded recursion, which
can happen when a device driver binds itself to a device that it
has just created. The log does look a bit suspicious here,
with multiple registrations for c883a000.hdmi-tx:
986 08:02:56.487871 [ 15.141218] meson-drm d010.vpu: Queued 2 outputs
on vpu
987 08:02:56.493572 [ 15.141615] meson8b-dwmac c941.ethernet: Ring
mode enabled
988 08:02:56.504769 [ 15.150744] meson-drm d010.vpu: bound
c883a000.hdmi-tx (ops meson_dw_hdmi_ops [meson_dw_hdmi])
989 08:02:56.515743 [ 15.154970] meson8b-dwmac c941.ethernet: Enable
RX Mitigation via HW Watchdog Timer
990 08:02:56.521531 [ 15.159175] lima d00c.gpu: pp0 - mali450 version
major 0 minor 0
991 08:02:56.526718 [ 15.161436] meson-drm d010.vpu: Failed to find
HDMI transceiver bridge
992 08:02:56.532417 [ 15.168933] lima d00c.gpu: pp1 - mali450 version
major 0 minor 0
993 08:02:56.537747 [ 15.206102] meson-drm d010
[PATCH][next] drm/i915/guc: Replace zero-length arrays with flexible-array members
Zero-length arrays are deprecated[1] and we are moving towards
adopting C99 flexible-array members, instead. So, replace zero-length
arrays in a couple of structures (three, actually) with flex-array
members.
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
routines on memcpy() and help us make progress towards globally
enabling -fstrict-flex-arrays=3 [2].
Link:
https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays
[1]
Link: https://gcc.gnu.org/pipermail/gcc-patches/2022-October/602902.html [2]
Link: https://github.com/KSPP/linux/issues/78
Signed-off-by: Gustavo A. R. Silva
---
drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
b/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
index 3624abfd22d1..9d589c28f40f 100644
--- a/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/guc_capture_fwif.h
@@ -73,7 +73,7 @@ struct guc_debug_capture_list_header {
struct guc_debug_capture_list {
struct guc_debug_capture_list_header header;
- struct guc_mmio_reg regs[0];
+ struct guc_mmio_reg regs[];
} __packed;
/**
@@ -125,7 +125,7 @@ struct guc_state_capture_header_t {
struct guc_state_capture_t {
struct guc_state_capture_header_t header;
- struct guc_mmio_reg mmio_entries[0];
+ struct guc_mmio_reg mmio_entries[];
} __packed;
enum guc_capture_group_types {
@@ -145,7 +145,7 @@ struct guc_state_capture_group_header_t {
/* this is the top level structure where an error-capture dump starts */
struct guc_state_capture_group_t {
struct guc_state_capture_group_header_t grp_header;
- struct guc_state_capture_t capture_entries[0];
+ struct guc_state_capture_t capture_entries[];
} __packed;
/**
--
2.34.1
Re: next-20230110: arm64: defconfig+kselftest config boot failed - Unable to handle kernel paging request at virtual address fffffffffffffff8
On Tue, Jan 10, 2023 at 04:32:59PM +, Will Deacon wrote:
> On Tue, Jan 10, 2023 at 09:44:40PM +0530, Naresh Kamboju wrote:
> > GOOD: next-20230109 (defconfig + kselftests configs)
> > BAD: next-20230110 (defconfig + kselftests configs)
> I couldn't find a kselftests .config in the tree (assumedly I'm just ont
> looking hard enough), but does that happen to enable CONFIG_STACK_TRACER=y?
It's adding on all the config fragments in
tools/testing/selftests/*/config
which includes ftrace, which does set STACK_TRACER>
> If so, since you're using clang, I wonder if this is an issue with
> 68a63a412d18 ("arm64: Fix build with CC=clang, CONFIG_FTRACE=y and
> CONFIG_STACK_TRACER=y")?
ftrace also enables FTRACE.
> Please let us know how the bisection goes...
Not sure that Naresh has a bisection going, I don't think he's got
direct access to such a board.
signature.asc
Description: PGP signature
Re: [Intel-gfx] [RFC PATCH 04/20] drm/sched: Convert drm scheduler to use a work queue rather than kthread
On 10/01/2023 15:55, Matthew Brost wrote:
On Tue, Jan 10, 2023 at 12:19:35PM +, Tvrtko Ursulin wrote:
On 10/01/2023 11:28, Tvrtko Ursulin wrote:
On 09/01/2023 17:27, Jason Ekstrand wrote:
[snip]
>>> AFAICT it proposes to have 1:1 between *userspace* created
contexts (per
>>> context _and_ engine) and drm_sched. I am not sure avoiding
invasive changes
>>> to the shared code is in the spirit of the overall idea and
instead
>>> opportunity should be used to look at way to refactor/improve
drm_sched.
Maybe? I'm not convinced that what Xe is doing is an abuse at all
or really needs to drive a re-factor. (More on that later.)
There's only one real issue which is that it fires off potentially a
lot of kthreads. Even that's not that bad given that kthreads are
pretty light and you're not likely to have more kthreads than
userspace threads which are much heavier. Not ideal, but not the
end of the world either. Definitely something we can/should
optimize but if we went through with Xe without this patch, it would
probably be mostly ok.
>> Yes, it is 1:1 *userspace* engines and drm_sched.
>>
>> I'm not really prepared to make large changes to DRM scheduler
at the
>> moment for Xe as they are not really required nor does Boris
seem they
>> will be required for his work either. I am interested to see
what Boris
>> comes up with.
>>
>>> Even on the low level, the idea to replace drm_sched threads
with workers
>>> has a few problems.
>>>
>>> To start with, the pattern of:
>>>
>>> while (not_stopped) {
>>> keep picking jobs
>>> }
>>>
>>> Feels fundamentally in disagreement with workers (while
obviously fits
>>> perfectly with the current kthread design).
>>
>> The while loop breaks and worker exists if no jobs are ready.
I'm not very familiar with workqueues. What are you saying would fit
better? One scheduling job per work item rather than one big work
item which handles all available jobs?
Yes and no, it indeed IMO does not fit to have a work item which is
potentially unbound in runtime. But it is a bit moot conceptual mismatch
because it is a worst case / theoretical, and I think due more
fundamental concerns.
If we have to go back to the low level side of things, I've picked this
random spot to consolidate what I have already mentioned and perhaps
expand.
To start with, let me pull out some thoughts from workqueue.rst:
"""
Generally, work items are not expected to hog a CPU and consume many
cycles. That means maintaining just enough concurrency to prevent work
processing from stalling should be optimal.
"""
For unbound queues:
"""
The responsibility of regulating concurrency level is on the users.
"""
Given the unbound queues will be spawned on demand to service all queued
work items (more interesting when mixing up with the system_unbound_wq),
in the proposed design the number of instantiated worker threads does
not correspond to the number of user threads (as you have elsewhere
stated), but pessimistically to the number of active user contexts. That
is the number which drives the maximum number of not-runnable jobs that
can become runnable at once, and hence spawn that many work items, and
in turn unbound worker threads.
Several problems there.
It is fundamentally pointless to have potentially that many more threads
than the number of CPU cores - it simply creates a scheduling storm.
To make matters worse, if I follow the code correctly, all these per user
context worker thread / work items end up contending on the same lock or
circular buffer, both are one instance per GPU:
guc_engine_run_job
-> submit_engine
a) wq_item_append
-> wq_wait_for_space
-> msleep
a) is dedicated per xe_engine
Hah true, what its for then? I thought throttling the LRCA ring is done via:
drm_sched_init(&ge->sched, &drm_sched_ops,
e->lrc[0].ring.size / MAX_JOB_SIZE_BYTES,
Is there something more to throttle other than the ring? It is
throttling something using msleeps..
Also you missed the step of programming the ring which is dedicated per
xe_engine
I was trying to quickly find places which serialize on something in the
backend, ringbuffer emission did not seem to do that but maybe I missed
something.
b) xe_guc_ct_send
-> guc_ct_send
-> mutex_lock(&ct->lock);
-> later a potential msleep in h2g_has_room
Techincally there is 1 instance per GT not GPU, yes this is shared but
in practice there will always be space in the CT channel so contention
on the lock should be rare.
Yeah I used the term GPU to be more understandable to outside audience.
I am somewhat disappointed that the Xe opportunity hasn't been used to
improve upon the CT communication bottlenecks. I mean those backoff
sleeps and lock contention. I w
Re: [PATCH v5 0/5] Improve GPU reset sequence for Adreno GPU
On Mon, Jan 02, 2023 at 04:18:26PM +0530, Akhil P Oommen wrote: > > This is a rework of [1] using genpd instead of 'reset' framework. > > As per the recommended reset sequence of Adreno gpu, we should ensure that > gpucc-cx-gdsc has collapsed at hardware to reset gpu's internal hardware > states. > Because this gdsc is implemented as 'votable', gdsc driver doesn't poll and > wait until its hw status says OFF. > > So use the newly introduced genpd api (dev_pm_genpd_synced_poweroff()) to > provide a hint to the gdsc driver to poll for the hw status and use genpd > notifier to wait from adreno gpu driver until gdsc is turned OFF. > > This series is rebased on top of linux-next (20221215) since the changes span > multiple drivers. > > [1] https://patchwork.freedesktop.org/series/107507/ > @Rob, please find the PM and gdsc implementation changes picked up here: https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git tags/1672656511-1931-1-git-send-email-quic_akhi...@quicinc.com Regards, Bjorn > Changes in v5: > - Capture all Reviewed-by tags > > Changes in v4: > - Update genpd function documentation (Ulf) > > Changes in v3: > - Rename the var 'force_sync' to 'wait (Stephen) > > Changes in v2: > - Minor formatting fix > - Select PM_GENERIC_DOMAINS from Kconfig > > Akhil P Oommen (4): > clk: qcom: gdsc: Support 'synced_poweroff' genpd flag > drm/msm/a6xx: Vote for cx gdsc from gpu driver > drm/msm/a6xx: Remove cx gdsc polling using 'reset' > drm/msm/a6xx: Use genpd notifier to ensure cx-gdsc collapse > > Ulf Hansson (1): > PM: domains: Allow a genpd consumer to require a synced power off > > drivers/base/power/domain.c | 26 > drivers/clk/qcom/gdsc.c | 11 + > drivers/gpu/drm/msm/Kconfig | 1 + > drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 46 > --- > drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 7 ++ > drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 13 +++--- > drivers/gpu/drm/msm/msm_gpu.c | 4 --- > drivers/gpu/drm/msm/msm_gpu.h | 4 --- > include/linux/pm_domain.h | 5 > 9 files changed, 97 insertions(+), 20 deletions(-) > > -- > 2.7.4 >
Re: (subset) [PATCH v5 0/5] Improve GPU reset sequence for Adreno GPU
On Mon, 2 Jan 2023 16:18:26 +0530, Akhil P Oommen wrote: > This is a rework of [1] using genpd instead of 'reset' framework. > > As per the recommended reset sequence of Adreno gpu, we should ensure that > gpucc-cx-gdsc has collapsed at hardware to reset gpu's internal hardware > states. > Because this gdsc is implemented as 'votable', gdsc driver doesn't poll and > wait until its hw status says OFF. > > [...] Applied, thanks! [1/5] PM: domains: Allow a genpd consumer to require a synced power off commit: a9236a0aa7d7f52a974cc7eaa971fae92aa477c5 [2/5] clk: qcom: gdsc: Support 'synced_poweroff' genpd flag commit: 8b6af3b58cafc2cbdf6269f655b2d3731eb93c2f Best regards, -- Bjorn Andersson
Re: [PATCH 1/2] backlight: pwm_bl: configure pwm only once per backlight toggle
Hello Daniel, On Tue, Jan 10, 2023 at 04:17:16PM +, Daniel Thompson wrote: > On Mon, Jan 09, 2023 at 09:47:57PM +0100, Uwe Kleine-König wrote: > > When the function pwm_backlight_update_status() was called with > > brightness > 0, pwm_get_state() was called twice (once directly and once > > in compute_duty_cycle). Also pwm_apply_state() was called twice (once in > > pwm_backlight_power_on() and once directly). > > > > Optimize this to do both calls only once. > > > > Signed-off-by: Uwe Kleine-König > > This will reverse the order in which the regulator is toggled versus the > PWM starting/stopping. It would be nice to that in the description. Oh, that wasn't a concious choice. I agree this should be noted. The current state is also a bit confused because the duty cycle is setup before the regulator but the PWM only gets enabled afterwards. Expect a v2 with an updated commit log. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight
Hello Daniel, On Tue, Jan 10, 2023 at 04:26:14PM +, Daniel Thompson wrote: > On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote: > > Most but not all PWMs drive the PWM pin to its inactive state when > > disabled. Rely on the lowlevel PWM implementation to implement > > duty_cycle = 0 in an energy efficient way and don't disable the PWM. > > I'm a little worried about this one. > > I thought the PWM APIs allow the duty cycle to be rounded up or down > slightly during the apply. In my book only rounding down is correct, but in practise there is some deviation. Nearly all PWMs can implement a zero duty cycle. Those that cannot but emit a constant inactive signal when disabled are expected to disable when .duty_cycle = 0 is requested. (And for those that can neither implement a zero duty_cycle nor emit the inactive level (not sure there is any) all bets are lost with and without my patch.) So if this case will be hit (and noticed) this is fixable. However there are hardware PWMs that just freeze in their current state when disabled (e.g. mxs). That's why .duty_cycle=0 + .enabled=true is the safer bet. Only disable a PWM if you don't rely on the output state. See also commit 80a22fde803af6f390be49ee5ced6ee75595ba05. > So when you say "rely on the lowlevel to implement duty_cycle = 0 to..." > is it confirmed that this is true (and that all PWMs *can* implement > a duty_cycle of 0 without rounding up)? The scenario I had in mind that can realistically go wrong here is that a lowlevel driver that has the property that the inactive level is emitted for a disabled HW doesn't actually disable when .duty_cycle=0 is requested and so might consume slightly more energy. But I'm confident my patch is an improvement and I don't expect regressions. (Famous last words :-) I suggest to amend the commit log and add something like: If this change results in a regression, the bug is in the lowlevel pwm driver. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH] drm: Drop ARCH_MULTIPLATFORM from dependencies
On 12/10/22 10:21, Uwe Kleine-König wrote:
> Hello Arnd,
>
> On Fri, Dec 09, 2022 at 11:53:49PM +0100, Arnd Bergmann wrote:
>> On Fri, Dec 9, 2022, at 23:05, Uwe Kleine-König wrote:
>>> Some of these dependencies used to be sensible when only a small part of
>>> the platforms supported by ARCH=arm could be compiled together in a
>>> single kernel image. Nowadays ARCH_MULTIPLATFORM is only used as a guard
>>> for kernel options incompatible with a multiplatform image. See commit
>>> 84fc86360623 ("ARM: make ARCH_MULTIPLATFORM user-visible") for some more
>>> details.
>>>
>>> Signed-off-by: Uwe Kleine-König
>>
>> Makes sense,
>>
>> Acked-by: Arnd Bergmann
>
> Thanks. (But honestly I'm not surprised you agree to this patch after
> our conversation on irc :-)
>
This makes sense to me as well, but it would be great if someone else
from DRM can review/ack before pushing it.
Reviewed-by: Javier Martinez Canillas
>>> diff --git a/drivers/gpu/drm/omapdrm/Kconfig
>>> b/drivers/gpu/drm/omapdrm/Kconfig
>>> index 455e1a91f0e5..76ded1568bd0 100644
>>> --- a/drivers/gpu/drm/omapdrm/Kconfig
>>> +++ b/drivers/gpu/drm/omapdrm/Kconfig
>>> @@ -2,7 +2,7 @@
>>> config DRM_OMAP
>>> tristate "OMAP DRM"
>>> depends on DRM && OF
>>> - depends on ARCH_OMAP2PLUS || ARCH_MULTIPLATFORM
>>> + depends on ARCH_OMAP2PLUS
>>> select DRM_KMS_HELPER
>>> select VIDEOMODE_HELPERS
>>> select HDMI
>>
>> Since the original purpose of the ||ARCH_MULTIPLATFORM was to allow
>> building the driver on more targets, I wonder if we should instead
>> make that ||COMPILE_TEST, which would also allow building it on
>> x86 and others.
>
> I wondered about that, too, but thought that would be a new patch.
>
Agreed that making it || COMPILE_TEST should be in a separate patch.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [PATCH 2/2] backlight: pwm_bl: Don't disable the PWM to disable the backlight
On Tue, Jan 10, 2023 at 06:35:00PM +0100, Uwe Kleine-König wrote: > Hello Daniel, > > On Tue, Jan 10, 2023 at 04:26:14PM +, Daniel Thompson wrote: > > On Mon, Jan 09, 2023 at 09:47:58PM +0100, Uwe Kleine-König wrote: > > > Most but not all PWMs drive the PWM pin to its inactive state when > > > disabled. Rely on the lowlevel PWM implementation to implement > > > duty_cycle = 0 in an energy efficient way and don't disable the PWM. > > > > I'm a little worried about this one. > > > > I thought the PWM APIs allow the duty cycle to be rounded up or down > > slightly during the apply. > > In my book only rounding down is correct, but in practise there is some > deviation. > > Nearly all PWMs can implement a zero duty cycle. Those that cannot but > emit a constant inactive signal when disabled are expected to disable > when .duty_cycle = 0 is requested. (And for those that can neither > implement a zero duty_cycle nor emit the inactive level (not sure there > is any) all bets are lost with and without my patch.) > So if this case will be hit (and noticed) this is fixable. > > However there are hardware PWMs that just freeze in their current state > when disabled (e.g. mxs). That's why .duty_cycle=0 + .enabled=true is > the safer bet. Only disable a PWM if you don't rely on the output state. > See also commit 80a22fde803af6f390be49ee5ced6ee75595ba05. Reading this, it does strike me that if pwm_bl has a regulator or an enable GPIO then it does not rely on the output state. We could use the presence of either of these to choose to disable the PWM (which could potentially undrive the pin to save power). > > So when you say "rely on the lowlevel to implement duty_cycle = 0 to..." > > is it confirmed that this is true (and that all PWMs *can* implement > > a duty_cycle of 0 without rounding up)? > > The scenario I had in mind that can realistically go wrong here is that > a lowlevel driver that has the property that the inactive level is > emitted for a disabled HW doesn't actually disable when .duty_cycle=0 is > requested and so might consume slightly more energy. But I'm confident > my patch is an improvement and I don't expect regressions. (Famous last > words :-) > > I suggest to amend the commit log and add something like: > >If this change results in a regression, the bug is in the lowlevel >pwm driver. I guess I can live with that :-) . If the reasoning about regulator or enable GPIO makes sense then let's implement that. If not, a terse comment in the code reminding some future version of me that disabled PWM has undefined state (making clear that the absense of enable = false is deliberate) would be useful! Daniel.
