Re: 572994bf18ff prevents system boot
(cc: ainux.w...@gmail.com) Hi Am 03.10.21 um 20:09 schrieb Chuck Lever III: Hi- After updating one of my test systems to v5.15-rc, I found that it becomes unresponsive during the later part of the boot process. A power-on reset is necessary to recover. I bisected to this commit: 572994bf18ff ("drm/ast: Zero is missing in detect function") You don't have a monitor connected, I guess? In that case, we now trigger the helpers that poll for connected monitors. However, the overhead seems rather extreme. I'll have to try to reproduce this, or otherwise we can revert the commit. Best regards Thomas Checking out v5.15-rc3 and reverting this commit enables the system to boot again. 0b:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 30) (prog-if 00 [VGA controller]) DeviceName: ASPEED Video AST2400 Subsystem: Super Micro Computer Inc X10SRL-F Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- SERR- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature
[PATCH v2 0/5] mxsfb/nwl/panels: media bus format fixes
commit b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") added bus format probing to mxsfb this exposed several issues in the display stack as used on the Librem 5: The nwl bridge and the panels didn't bother to set any media bus formats and in that case mxsfb would not pick a reasonable default. This series aims to fix this. This series includes the patch from https://lore.kernel.org/dri-devel/yvlyh%2fsgbritg%2...@qwark.sigxcpu.org/ with a `dev_warn` added. The patches are against 5.15-rc3. I've marked a single patch with a 'fixes' which is enough to unbreak the display stack in 5.15. All patches of this series can be applied independently. Changes from v1: - Review comment by Marek Vasut https://lore.kernel.org/dri-devel/67e6056a-6157-795d-908d-d65cc803c...@denx.de/ Improve warning message - Move mxsfb patches to the end of the queue and the actual nwl fix to the front. Guido Günther (5): drm/bridge: nwl-dsi: Add atomic_get_input_bus_fmts drm/panel: mantix: Add media bus format drm/panel: st7703: Add media bus format drm: mxsfb: Print failed bus format in hex drm: mxsfb: Set proper default bus format when using a bridge drivers/gpu/drm/bridge/nwl-dsi.c | 35 +++ drivers/gpu/drm/mxsfb/mxsfb_kms.c | 8 - .../gpu/drm/panel/panel-mantix-mlaf057we51.c | 9 + drivers/gpu/drm/panel/panel-sitronix-st7703.c | 8 + 4 files changed, 59 insertions(+), 1 deletion(-) -- 2.33.0
[PATCH v2 1/5] drm/bridge: nwl-dsi: Add atomic_get_input_bus_fmts
Components further up in the chain might ask us for supported formats. Without this MEDIA_BUS_FMT_FIXED is assumed which then breaks display output with mxsfb since it can't determine a proper bus format. We handle the bus formats that correspond to the DSI formats the bridge can potentially output (see chapter 13.6 of the i.MX 8MQ reference manual) - which matches what xsfb can input. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther --- drivers/gpu/drm/bridge/nwl-dsi.c | 35 1 file changed, 35 insertions(+) diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index a251cc1f088f..27c80d36846b 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -1234,6 +1234,40 @@ static void nwl_dsi_bridge_detach(struct drm_bridge *bridge) drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0); } +static u32 *nwl_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, +struct drm_bridge_state *bridge_state, +struct drm_crtc_state *crtc_state, +struct drm_connector_state *conn_state, +u32 output_fmt, +unsigned int *num_input_fmts) +{ + u32 *input_fmts, input_fmt; + + *num_input_fmts = 0; + + switch (output_fmt) { + /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ + case MEDIA_BUS_FMT_FIXED: + input_fmt = MEDIA_BUS_FMT_RGB888_1X24; + break; + case MEDIA_BUS_FMT_RGB888_1X24: + case MEDIA_BUS_FMT_RGB666_1X18: + case MEDIA_BUS_FMT_RGB565_1X16: + input_fmt = output_fmt; + break; + default: + return NULL; + } + + input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) + return NULL; + input_fmts[0] = input_fmt; + *num_input_fmts = 1; + + return input_fmts; +} + static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = { .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, @@ -1241,6 +1275,7 @@ static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = { .atomic_check = nwl_dsi_bridge_atomic_check, .atomic_enable = nwl_dsi_bridge_atomic_enable, .atomic_disable = nwl_dsi_bridge_atomic_disable, + .atomic_get_input_bus_fmts = nwl_bridge_atomic_get_input_bus_fmts, .mode_set = nwl_dsi_bridge_mode_set, .mode_valid = nwl_dsi_bridge_mode_valid, .attach = nwl_dsi_bridge_attach, -- 2.33.0
[PATCH v2 2/5] drm/panel: mantix: Add media bus format
This allows the DSI bridge to detect the correct bus format. We currently only support MEDIA_BUS_FMT_RGB888_1X24. Signed-off-by: Guido Günther --- drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c index f0e9bce23c41..d6bcf1045255 100644 --- a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c +++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -232,6 +233,10 @@ static const struct drm_display_mode default_mode_ys = { .height_mm = 130, }; +static const u32 mantix_bus_formats[] = { + MEDIA_BUS_FMT_RGB888_1X24, +}; + static int mantix_get_modes(struct drm_panel *panel, struct drm_connector *connector) { @@ -253,6 +258,10 @@ static int mantix_get_modes(struct drm_panel *panel, connector->display_info.height_mm = mode->height_mm; drm_mode_probed_add(connector, mode); + drm_display_info_set_bus_formats(&connector->display_info, +mantix_bus_formats, +ARRAY_SIZE(mantix_bus_formats)); + return 1; } -- 2.33.0
[PATCH v2 3/5] drm/panel: st7703: Add media bus format
This allows the DSI bridge to detect the correct bus format. We currently only support MEDIA_BUS_FMT_RGB888_1X24. Signed-off-by: Guido Günther --- drivers/gpu/drm/panel/panel-sitronix-st7703.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c index a2c303e5732c..73f69c929a75 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c @@ -453,6 +453,10 @@ static int st7703_prepare(struct drm_panel *panel) return ret; } +static const u32 mantix_bus_formats[] = { + MEDIA_BUS_FMT_RGB888_1X24, +}; + static int st7703_get_modes(struct drm_panel *panel, struct drm_connector *connector) { @@ -474,6 +478,10 @@ static int st7703_get_modes(struct drm_panel *panel, connector->display_info.height_mm = mode->height_mm; drm_mode_probed_add(connector, mode); + drm_display_info_set_bus_formats(&connector->display_info, +mantix_bus_formats, +ARRAY_SIZE(mantix_bus_formats)); + return 1; } -- 2.33.0
[PATCH v2 4/5] drm: mxsfb: Print failed bus format in hex
media-bus-formats.h has them in hexadecimal as well so matching with that file saves one conversion when debugging. Signed-off-by: Guido Günther --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..d6abd2077114 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -89,7 +89,7 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb, ctrl |= CTRL_BUS_WIDTH_24; break; default: - dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); + dev_err(drm->dev, "Unknown media bus format 0x%x\n", bus_format); break; } -- 2.33.0
[PATCH v2 5/5] drm: mxsfb: Set proper default bus format when using a bridge
If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Reported-by: Martin Kepplinger Signed-off-by: Guido Günther --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index d6abd2077114..e3fbb8b58d5d 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) { + dev_warn_once(drm->dev, + "Bridge does not provide bus format, assuming MEDIA_BUS_FMT_RGB888_1X24.\n" + "Please fix bridge driver by handling atomic_get_input_bus_fmts.\n"); + bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } } /* If there is no bridge, use bus format from connector */ -- 2.33.0
Re: [PATCH] drm/i915: fix blank screen booting crashes
On Fri, 24 Sep 2021, Ville Syrjälä wrote: > On Tue, Sep 21, 2021 at 06:50:39PM -0700, Matthew Brost wrote: >> From: Hugh Dickins >> >> 5.15-rc1 crashes with blank screen when booting up on two ThinkPads >> using i915. Bisections converge convincingly, but arrive at different >> and surprising "culprits", none of them the actual culprit. >> >> netconsole (with init_netconsole() hacked to call i915_init() when >> logging has started, instead of by module_init()) tells the story: >> >> kernel BUG at drivers/gpu/drm/i915/i915_sw_fence.c:245! >> with RSI: 814d408b pointing to sw_fence_dummy_notify(). >> I've been building with CONFIG_CC_OPTIMIZE_FOR_SIZE=y, and that >> function needs to be 4-byte aligned. >> >> v2: >> (Jani Nikula) >> - Change BUG_ON to WARN_ON >> v3: >> (Jani / Tvrtko) >> - Short circuit __i915_sw_fence_init on WARN_ON >> v4: >> (Lucas) >> - Break WARN_ON changes out in a different patch >> >> Fixes: 62eaf0ae217d ("drm/i915/guc: Support request cancellation") >> Signed-off-by: Hugh Dickins >> Signed-off-by: Matthew Brost >> Reviewed-by: Matthew Brost >> --- >> drivers/gpu/drm/i915/gt/intel_context.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c >> b/drivers/gpu/drm/i915/gt/intel_context.c >> index ff637147b1a9..e7f78bc7ebfc 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_context.c >> +++ b/drivers/gpu/drm/i915/gt/intel_context.c >> @@ -362,8 +362,8 @@ static int __intel_context_active(struct i915_active >> *active) >> return 0; >> } >> >> -static int sw_fence_dummy_notify(struct i915_sw_fence *sf, >> - enum i915_sw_fence_notify state) >> +static int __i915_sw_fence_call >> +sw_fence_dummy_notify(struct i915_sw_fence *sf, enum i915_sw_fence_notify >> state) >> { >> return NOTIFY_DONE; >> } > > This thing seems broken beyond just this alignment stuff. I'm getting > this spew from DEBUG_OBJECTS all the time on a glk here: Nobody followed through with this, so: https://lore.kernel.org/r/20211002020257.34a0e...@oasis.local.home and cdc1e6e225e3 ("drm/i915: fix blank screen booting crashes") BR, Jani. > > [ 48.122629] [ cut here ] > [ 48.122640] ODEBUG: init destroyed (active state 0) object type: > i915_sw_fence hint: sw_fence_dummy_notify+0x0/0x10 [i915] > [ 48.122963] WARNING: CPU: 0 PID: 815 at lib/debugobjects.c:505 > debug_print_object+0x6e/0x90 > [ 48.122976] Modules linked in: i915 i2c_algo_bit ttm drm_kms_helper > syscopyarea sysfillrect sysimgblt fb_sys_fops prime_numbers intel_gtt agpgart > fuse nls_iso8859_1 nls_cp437 vfat fat intel_rapl_msr wmi_bmof > intel_rapl_common x86_pkg_temp_thermal r8169 realtek mdio_devres coretemp > libphy efi_pstore evdev sdhci_pci cqhci sdhci mei_me mmc_core i2c_i801 > intel_pmc_core mei led_class wmi i2c_smbus sch_fq_codel drm ip_tables > x_tables ipv6 autofs4 > [ 48.123119] CPU: 0 PID: 815 Comm: kms_async_flips Not tainted > 5.15.0-rc2-hsw+ #131 > [ 48.123125] Hardware name: Intel Corporation NUC7CJYH/NUC7JYB, BIOS > JYGLKCPX.86A.0027.2018.0125.1347 01/25/2018 > [ 48.123129] RIP: 0010:debug_print_object+0x6e/0x90 > [ 48.123137] Code: 07 08 02 83 c0 01 8b 4b 14 4c 8b 45 00 48 c7 c7 a0 19 0a > 82 89 05 66 07 08 02 8b 43 10 48 8b 14 c5 c0 0d e4 81 e8 d7 2e 3c 00 <0f> 0b > 83 05 c5 c0 0c 01 01 48 83 c4 08 5b 5d c3 83 05 b7 c0 0c 01 > [ 48.123142] RSP: 0018:c9dabae0 EFLAGS: 00010282 > [ 48.123150] RAX: RBX: 88810004f848 RCX: > > [ 48.123154] RDX: 8001 RSI: 8112673f RDI: > 8112673f > [ 48.123159] RBP: a0577480 R08: 88827fbfcfe8 R09: > 0009fffb > [ 48.123163] R10: fffe R11: 3fff R12: > 88810a04d100 > [ 48.123167] R13: 88810a07d308 R14: 888109990800 R15: > 88810997b800 > [ 48.123171] FS: 7624b9c0() GS:888276e0() > knlGS: > [ 48.123176] CS: 0010 DS: ES: CR0: 80050033 > [ 48.123181] CR2: 77f93bf0 CR3: 000108e56000 CR4: > 00350ef0 > [ 48.123185] Call Trace: > [ 48.123190] i915_sw_fence_reinit+0x15/0x40 [i915] > [ 48.123341] intel_context_init+0x16b/0x1d0 [i915] > [ 48.123492] intel_context_create+0x33/0x100 [i915] > [ 48.123642] default_engines+0x9d/0x120 [i915] > [ 48.123806] i915_gem_create_context+0x465/0x630 [i915] > [ 48.125964] ? trace_kmalloc+0x29/0xd0 > [ 48.125976] ? kmem_cache_alloc_trace+0x121/0x620 > [ 48.125984] i915_gem_context_open+0x145/0x1d0 [i915] > [ 48.126172] i915_gem_open+0x75/0xb0 [i915] > [ 48.126364] drm_file_alloc+0x1b1/0x280 [drm] > [ 48.126427] drm_open+0xde/0x250 [drm] > [ 48.126482] drm_stub_open+0xa8/0x130 [drm] > [ 48.126538] chrdev_open+0xbf/0x240 > [ 48.126547] ? cdev_device_add+0x90/0x90 > [ 48.126553] do_dentry_open+0x151/0x3a0 > [ 48.126560] p
Re: [BUG 5.15-rc3] kernel BUG at drivers/gpu/drm/i915/i915_sw_fence.c:245!
On Sat, 02 Oct 2021, Hugh Dickins wrote: > On Sat, 2 Oct 2021, Linus Torvalds wrote: >> On Sat, Oct 2, 2021 at 5:17 AM Steven Rostedt wrote: >> > On Sat, 2 Oct 2021 03:17:29 -0700 (PDT) >> > Hugh Dickins wrote: >> > >> > > Yes (though bisection doesn't work right on this one): the fix >> > >> > Interesting, as it appeared to be very reliable. But I didn't do the >> > "try before / after" on the patch. >> >> Well, even the before/after might well have worked, since the problem >> depended on how that sw_fence_dummy_notify() function ended up >> aligned. So random unrelated changes could re-align it just by >> mistake. > > Yup. > >> >> Patch applied directly. > > Great, thanks a lot. Thanks & sorry, really looks like we managed to drop this between the cracks. :( > >> >> I'd also like to point out how that BUG_ON() actually made things >> worse, and made this harder to debug. If it had been a WARN_ON_ONCE(), >> this would presumably not even have needed bisecting, it would have >> been obvious. >> >> BUG_ON() really is pretty much *always* the wrong thing to do. It >> onl;y results in problems being harder to see because you end up with >> a dead machine and the message is often hidden. > > Jani made the same point. But I guess they then went off into the weeds > of how to recover when warning, that the fix itself did not progress. Yes. That, as well as removing the entire alignment thing to reuse a couple of bits for flags. Too fragile for its own good. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v2 1/5] drm/bridge: nwl-dsi: Add atomic_get_input_bus_fmts
Am Montag, dem 04.10.2021 um 09:27 +0200 schrieb Guido Günther: > Components further up in the chain might ask us for supported formats. > > Without this MEDIA_BUS_FMT_FIXED is assumed which then breaks display > output with mxsfb since it can't determine a proper bus format. > > We handle the bus formats that correspond to the DSI formats the bridge > can potentially output (see chapter 13.6 of the i.MX 8MQ reference > manual) - which matches what xsfb can input. > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if > present") > > Signed-off-by: Guido Günther Reviewed-by: Lucas Stach > --- > drivers/gpu/drm/bridge/nwl-dsi.c | 35 > 1 file changed, 35 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c > b/drivers/gpu/drm/bridge/nwl-dsi.c > index a251cc1f088f..27c80d36846b 100644 > --- a/drivers/gpu/drm/bridge/nwl-dsi.c > +++ b/drivers/gpu/drm/bridge/nwl-dsi.c > @@ -1234,6 +1234,40 @@ static void nwl_dsi_bridge_detach(struct drm_bridge > *bridge) > drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0); > } > > +static u32 *nwl_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, > + struct drm_bridge_state > *bridge_state, > + struct drm_crtc_state > *crtc_state, > + struct drm_connector_state > *conn_state, > + u32 output_fmt, > + unsigned int *num_input_fmts) > +{ > + u32 *input_fmts, input_fmt; > + > + *num_input_fmts = 0; > + > + switch (output_fmt) { > + /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ > + case MEDIA_BUS_FMT_FIXED: > + input_fmt = MEDIA_BUS_FMT_RGB888_1X24; > + break; > + case MEDIA_BUS_FMT_RGB888_1X24: > + case MEDIA_BUS_FMT_RGB666_1X18: > + case MEDIA_BUS_FMT_RGB565_1X16: > + input_fmt = output_fmt; > + break; > + default: > + return NULL; > + } > + > + input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL); > + if (!input_fmts) > + return NULL; > + input_fmts[0] = input_fmt; > + *num_input_fmts = 1; > + > + return input_fmts; > +} > + > static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = { > .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > @@ -1241,6 +1275,7 @@ static const struct drm_bridge_funcs > nwl_dsi_bridge_funcs = { > .atomic_check = nwl_dsi_bridge_atomic_check, > .atomic_enable = nwl_dsi_bridge_atomic_enable, > .atomic_disable = nwl_dsi_bridge_atomic_disable, > + .atomic_get_input_bus_fmts = nwl_bridge_atomic_get_input_bus_fmts, > .mode_set = nwl_dsi_bridge_mode_set, > .mode_valid = nwl_dsi_bridge_mode_valid, > .attach = nwl_dsi_bridge_attach,
Re: [PATCH v2 4/5] drm: mxsfb: Print failed bus format in hex
Am Montag, dem 04.10.2021 um 09:27 +0200 schrieb Guido Günther: > media-bus-formats.h has them in hexadecimal as well so matching with > that file saves one conversion when debugging. > > Signed-off-by: Guido Günther Reviewed-by: Lucas Stach > --- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index af6c620adf6e..d6abd2077114 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -89,7 +89,7 @@ static void mxsfb_set_formats(struct mxsfb_drm_private > *mxsfb, > ctrl |= CTRL_BUS_WIDTH_24; > break; > default: > - dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); > + dev_err(drm->dev, "Unknown media bus format 0x%x\n", > bus_format); > break; > } >
Re: [PATCH v2 5/5] drm: mxsfb: Set proper default bus format when using a bridge
Am Montag, dem 04.10.2021 um 09:27 +0200 schrieb Guido Günther: > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > that case. > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if > present") > I don't think this qualifies for stable, so I would drop this tag, as the stable maintainers are quite trigger happy to pull in patches with a fixes tag. Also the subject isn't quite correct, this isn't setting a "proper" bus format, but rather adds a fallback. Other than that: Reviewed-by: Lucas Stach Regards, Lucas > Reported-by: Martin Kepplinger > Signed-off-by: Guido Günther > --- > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > index d6abd2077114..e3fbb8b58d5d 100644 > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > @@ -369,6 +369,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc > *crtc, > drm_atomic_get_new_bridge_state(state, > mxsfb->bridge); > bus_format = bridge_state->input_bus_cfg.format; > + if (bus_format == MEDIA_BUS_FMT_FIXED) { > + dev_warn_once(drm->dev, > + "Bridge does not provide bus format, > assuming MEDIA_BUS_FMT_RGB888_1X24.\n" > + "Please fix bridge driver by handling > atomic_get_input_bus_fmts.\n"); > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > + } > } > > /* If there is no bridge, use bus format from connector */
Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sat, Oct 02, 2021 at 07:28:02PM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > On 21/10/02 05:30AM, Ville Syrjälä wrote: > > > On Sat, Oct 02, 2021 at 01:05:47AM +0300, Ville Syrjälä wrote: > > > > On Fri, Oct 01, 2021 at 04:48:15PM -0400, Sean Paul wrote: > > > > > On Fri, Oct 01, 2021 at 10:00:50PM +0300, Ville Syrjälä wrote: > > > > > > On Fri, Oct 01, 2021 at 02:36:55PM -0400, Sean Paul wrote: > > > > > > > > > > > > > > Thank you for revising, Fernando! I've pushed the set to > > > > > > > drm-misc-next (along > > > > > > > with the necessary drm-tip conflict resolutions). > > > > > > > > > > > > Ugh. Did anyone actually review the locking changes this does? > > > > > > I shot the previous i915 stuff down because the commit messages > > > > > > did not address any of it. > > > > > > > > > > I reviewed the set on 9/17, I didn't see your feedback on that thread. > > > > > > > > It was much earlir than that. > > > > https://lists.freedesktop.org/archives/dri-devel/2021-June/313193.html > > Sorry, I'm new to this and it did not occur to me to search for similar > patches > in the mailing list archives in case there were additional comments that > applied > to my change set. > > In case I had done that I would have found that, as you mentioned, you had > already raised two issues back in June: > > On Tue, Jun 29, 2021, Ville Syrjälä wrote: > > > > That looks wrong. You're using a private ctx here, but still > > passing dev->mode_config.acquire_ctx to the lower level stuff. > > > > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be > > equivalent to drm_modeset_{lock,unlock}_all() when it comes to > > mode_config.mutex. So would need a proper review whether we > > actually need that lock or not. > > The first one was pointing out the same error I would later repeat in my patch > series (ups). > > After further inspection of the code it looks to me that changing this: > > intel_modeset_setup_hw_state(dev, dev->mode_config.acquire_ctx); > > ...into this: > > intel_modeset_setup_hw_state(dev, &ctx); > > ...would be enough. Yes. > > Why? The only difference between the old drm_modeset_{lock,unlock}_all() > functions and the new DRM_MODESET_LOCK_ALL_{BEGIN,END}() macros is that the > former use a global context stored in dev->mode_config.acquire_ctx while the > latter depend on a user provided one (typically in the stack). > > In the old (working) code the global context structure is freed in > drm_modeset_unlock_all() thus we are sure no one is holding a reference to it > at > that point. This means that as long as no one accesses the global > dev->mode_config.acquire_ctx context in the block that runs between lock/BEGIN > and unlock/END, the code should be equivalent before and after my changes. > > In fact, now that my patch series removes the drm_modeset_{lock,unlock}_all() > functions, the acquire_ctx field of the drm_mode_config structure should be > deleted: > > /** > * @acquire_ctx: > * > * Global implicit acquire context used by atomic drivers for legacy > * IOCTLs. Deprecated, since implicit locking contexts make it > * impossible to use driver-private &struct drm_modeset_lock. Users of > * this must hold @mutex. > */ > struct drm_modeset_acquire_ctx *acquire_ctx; > > If I had done that (ie. removing this field) I would have detected the problem > when compiling. > > There is another place (in the amdgpu driver) where this field is still being > referenced, but before I investigate that I would like to know if you agree > that > this is a good path to follow. Yeah, removing the mode_config.acquire_ctx is a good idea if it's no longer needed. > > Regarding the second issue you raised... > > > Also DRM_MODESET_LOCK_ALL_{BEGIN,END}() do not seem to be > > equivalent to drm_modeset_{lock,unlock}_all() when it comes to > > mode_config.mutex. So would need a proper review whether we > > actually need that lock or not. > > ...the only difference regarding mode_config.mutex I see is that in the new > macros the mutex is locked only under this condition: > > if (!drm_drv_uses_atomic_modeset(dev)) > > ...which seems reasonable, right? Is this what you were referring to or is it > something else? In order to eliminate the lock one first has to determine what that lock might be protecting here, and then prove that such protection is not actually needed. -- Ville Syrjälä Intel
Re: [RFC 1/6] sched: Add nice value change notifier
On 01/10/2021 16:48, Peter Zijlstra wrote: On Fri, Oct 01, 2021 at 11:32:16AM +0100, Tvrtko Ursulin wrote: On 01/10/2021 10:04, Tvrtko Ursulin wrote: Hi Peter, On 30/09/2021 19:33, Peter Zijlstra wrote: On Thu, Sep 30, 2021 at 06:15:47PM +0100, Tvrtko Ursulin wrote: void set_user_nice(struct task_struct *p, long nice) { bool queued, running; - int old_prio; + int old_prio, ret; struct rq_flags rf; struct rq *rq; @@ -6913,6 +6945,9 @@ void set_user_nice(struct task_struct *p, long nice) */ p->sched_class->prio_changed(rq, p, old_prio); + ret = atomic_notifier_call_chain(&user_nice_notifier_list, nice, p); + WARN_ON_ONCE(ret != NOTIFY_DONE); + out_unlock: task_rq_unlock(rq, p, &rf); } No, we're not going to call out to exported, and potentially unbounded, functions under scheduler locks. Agreed, that's another good point why it is even more hairy, as I have generally alluded in the cover letter. Do you have any immediate thoughts on possible alternatives? Like for instance if I did a queue_work from set_user_nice and then ran a notifier chain async from a worker? I haven't looked at yet what repercussion would that have in terms of having to cancel the pending workers when tasks exit. I can try and prototype that and see how it would look. Hm or I simply move calling the notifier chain to after task_rq_unlock? That would leave it run under the tasklist lock so probably still quite bad. Hmm? That's for normalize_rt_tasks() only, right? Just don't have it call the notifier in that special case (that's a magic sysrq thing anyway). You mean my talk about tasklist_lock? No, it is also on the syscall part I am interested in as well. Call chain looks like this: sys_setpriority() { ... rcu_read_lock(); read_lock(&tasklist_lock); ... set_one_prio() set_user_nice() { ... task_rq_lock(); -> my notifier from this RFC [1] task_rq_unlock(); -> I can move the notifier here for _some_ improvement [2] } ... read_unlock(&tasklist_lock); rcu_read_unlock(); } So this RFC had the notifier call chain at [1], which I understood was the thing you initially pointed was horrible, being under a scheduler lock. I can trivially move it to [2] but that still leaves it under the tasklist lock. I don't have a good feel how much better that would be. If not good enough then I will look for a smarter solution with less opportunity for global impact. Regards, Tvrtko
Re: Lockdep spalt on killing a processes
The problem is a bit different. The callback is on the dependent fence, while we need to signal the scheduler fence. Daniel is right that this needs an irq_work struct to handle this properly. Christian. Am 01.10.21 um 17:10 schrieb Andrey Grodzovsky: From what I see here you supposed to have actual deadlock and not only warning, sched_fence->finished is first signaled from within hw fence done callback (drm_sched_job_done_cb) but then again from within it's own callback (drm_sched_entity_kill_jobs_cb) and so looks like same fence object is recursively signaled twice. This leads to attempt to lock fence->lock second time while it's already locked. I don't see a need to call drm_sched_fence_finished from within drm_sched_entity_kill_jobs_cb as this callback already registered on sched_fence->finished fence (entity->last_scheduled == s_fence->finished) and hence the signaling already took place. Andrey On 2021-10-01 6:50 a.m., Christian König wrote: Hey, Andrey. while investigating some memory management problems I've got the logdep splat below. Looks like something is wrong with drm_sched_entity_kill_jobs_cb(), can you investigate? Thanks, Christian. [11176.741052] [11176.741056] WARNING: possible recursive locking detected [11176.741060] 5.15.0-rc1-00031-g9d546d600800 #171 Not tainted [11176.741066] [11176.741070] swapper/12/0 is trying to acquire lock: [11176.741074] 9c337ed175a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741088] but task is already holding lock: [11176.741092] 9c337ed172a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741100] other info that might help us debug this: [11176.741104] Possible unsafe locking scenario: [11176.741108] CPU0 [11176.741110] [11176.741113] lock(&fence->lock); [11176.741118] lock(&fence->lock); [11176.741122] *** DEADLOCK *** [11176.741125] May be due to missing lock nesting notation [11176.741128] 2 locks held by swapper/12/0: [11176.741133] #0: 9c339c30f768 (&ring->fence_drv.lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741142] #1: 9c337ed172a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741151] stack backtrace: [11176.741155] CPU: 12 PID: 0 Comm: swapper/12 Not tainted 5.15.0-rc1-00031-g9d546d600800 #171 [11176.741160] Hardware name: System manufacturer System Product Name/PRIME X399-A, BIOS 0808 10/12/2018 [11176.741165] Call Trace: [11176.741169] [11176.741173] dump_stack_lvl+0x5b/0x74 [11176.741181] dump_stack+0x10/0x12 [11176.741186] __lock_acquire.cold+0x208/0x2df [11176.741197] lock_acquire+0xc6/0x2d0 [11176.741204] ? dma_fence_signal+0x28/0x80 [11176.741212] _raw_spin_lock_irqsave+0x4d/0x70 [11176.741219] ? dma_fence_signal+0x28/0x80 [11176.741225] dma_fence_signal+0x28/0x80 [11176.741230] drm_sched_fence_finished+0x12/0x20 [gpu_sched] [11176.741240] drm_sched_entity_kill_jobs_cb+0x1c/0x50 [gpu_sched] [11176.741248] dma_fence_signal_timestamp_locked+0xac/0x1a0 [11176.741254] dma_fence_signal+0x3b/0x80 [11176.741260] drm_sched_fence_finished+0x12/0x20 [gpu_sched] [11176.741268] drm_sched_job_done.isra.0+0x7f/0x1a0 [gpu_sched] [11176.741277] drm_sched_job_done_cb+0x12/0x20 [gpu_sched] [11176.741284] dma_fence_signal_timestamp_locked+0xac/0x1a0 [11176.741290] dma_fence_signal+0x3b/0x80 [11176.741296] amdgpu_fence_process+0xd1/0x140 [amdgpu] [11176.741504] sdma_v4_0_process_trap_irq+0x8c/0xb0 [amdgpu] [11176.741731] amdgpu_irq_dispatch+0xce/0x250 [amdgpu] [11176.741954] amdgpu_ih_process+0x81/0x100 [amdgpu] [11176.742174] amdgpu_irq_handler+0x26/0xa0 [amdgpu] [11176.742393] __handle_irq_event_percpu+0x4f/0x2c0 [11176.742402] handle_irq_event_percpu+0x33/0x80 [11176.742408] handle_irq_event+0x39/0x60 [11176.742414] handle_edge_irq+0x93/0x1d0 [11176.742419] __common_interrupt+0x50/0xe0 [11176.742426] common_interrupt+0x80/0x90 [11176.742431] [11176.742436] asm_common_interrupt+0x1e/0x40 [11176.742442] RIP: 0010:cpuidle_enter_state+0xff/0x470 [11176.742449] Code: 0f a3 05 04 54 24 01 0f 82 70 02 00 00 31 ff e8 37 5d 6f ff 80 7d d7 00 0f 85 e9 01 00 00 e8 58 a2 7f ff fb 66 0f 1f 44 00 00 <45> 85 ff 0f 88 01 01 00 00 49 63 c7 4c 2b 75 c8 48 8d 14 40 48 8d [11176.742455] RSP: 0018:b6970021fe48 EFLAGS: 0202 [11176.742461] RAX: 0059be25 RBX: 0002 RCX: [11176.742465] RDX: RSI: RDI: 9efeed78 [11176.742470] RBP: b6970021fe80 R08: 0001 R09: 0001 [11176.742473] R10: 0001 R11: 0001 R12: 9c3350b0e800 [11176.742477] R13: a00e9680 R14: 0a2a49ada060 R15: 0002 [11176.742483] ? cpuidle_enter_state+0xf8/0x470 [11176.742489] ? cpuidle_enter_state+0xf8/0x470 [11176.7424
[PATCH] drm/fbdev: Clamp fbdev surface size if too large
Clamp the fbdev surface size of the available maximum height to avoid failing to init console emulation. An example error is shown below. bad framebuffer height 2304, should be >= 768 && <= 768 [drm] Initialized simpledrm 1.0.0 20200625 for simple-framebuffer.0 on minor 0 simple-framebuffer simple-framebuffer.0: [drm] *ERROR* fbdev: Failed to setup generic emulation (ret=-22) This is especially a problem with drivers that have very small screen sizes and cannot over-allocate at all. Signed-off-by: Thomas Zimmermann Fixes: 11e8f5fd223b ("drm: Add simpledrm driver") Reported-by: Amanoel Dawod Reported-by: Zoltán Kővágó Reported-by: Michael Stapelberg Cc: Daniel Vetter Cc: Maxime Ripard Cc: dri-devel@lists.freedesktop.org Cc: # v5.14+ --- drivers/gpu/drm/drm_fb_helper.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 6860223f0068..364f11900b37 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1508,6 +1508,7 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, { struct drm_client_dev *client = &fb_helper->client; struct drm_device *dev = fb_helper->dev; + struct drm_mode_config *config = &dev->mode_config; int ret = 0; int crtc_count = 0; struct drm_connector_list_iter conn_iter; @@ -1665,6 +1666,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper, /* Handle our overallocation */ sizes.surface_height *= drm_fbdev_overalloc; sizes.surface_height /= 100; + if (sizes.surface_height > config->max_height) { + drm_warn(dev, "Fbdev over-allocation too large; clamping height to %d\n", +config->max_height); + sizes.surface_height = config->max_height; + } /* push down into drivers */ ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); -- 2.33.0
Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: > On 21/10/02 09:13AM, Fernando Ramos wrote: > > > > Sean, could you revert the whole patch series? I'll have a deeper look into > > the > > patch set and come up with a v3 where all these issues will be addressed. > > > > Hi Sean, > > I now understand the nature of the issue that caused the problem with i915 and > have proceed to remove the global context structure (which revealed a similar > issue in the amdgpu driver). > > I have prepared a V3 version of the patch set where these issues should > hopefully be fixed for both the i915 and amdgpu drivers. > > In order to prevent causing more disruption, could you tell me what the proper > way to proceed would be? In particular: > > 1. Is there any place where I can push my changes so that they are tested > on a i915 machine? (Some type of automated pool) cc:intel-gfx, which it looks like you did, _but_ your patches did did not even apply against drm-tip so our CI rejected it. There was a reply to the patches from CI indicating that. And that is one reason I probably just ignored the whole thing. If it doesn't even apply/build it's not worth my time to read. > > 2. I can test the amdgpu driver on my machine but, what about all the other > architectures? What is the standard procedure? Should I simply publish V3 > and wait for feedback from the different vendors? (I would hate to cause > a > simular situation again) > > 3. Should I post V3 on top of drm-next or drm-misc-next? The normal rule is: always work on drm-tip. That is what gets tested by our CI as well. Yes, it does mean a bit of extra hurdles during development since drm-tip is a rebasing tree, but there are tools like dim retip to help out here. As for where to merge them. I would generally recommed against merging i915 patches through drm-misc unless there is a very compelling reason to do so. i915 is a fast moving target and if there are significant changes coming in via drm-misc they usually will cause conflicts for people during drm-tip rebuild. Also I would expect to see an ack requested from i915 maintainers for merging anything significant via drm-misc, which I don't think happened in this case. -- Ville Syrjälä Intel
Re: [PATCH] drm/fbdev: Clamp fbdev surface size if too large
On Mon, Oct 04, 2021 at 10:15:06AM +0200, Thomas Zimmermann wrote: > Clamp the fbdev surface size of the available maximum height to avoid > failing to init console emulation. An example error is shown below. > > bad framebuffer height 2304, should be >= 768 && <= 768 > [drm] Initialized simpledrm 1.0.0 20200625 for simple-framebuffer.0 on > minor 0 > simple-framebuffer simple-framebuffer.0: [drm] *ERROR* fbdev: Failed to > setup generic emulation (ret=-22) > > This is especially a problem with drivers that have very small screen > sizes and cannot over-allocate at all. > > Signed-off-by: Thomas Zimmermann > Fixes: 11e8f5fd223b ("drm: Add simpledrm driver") > Reported-by: Amanoel Dawod > Reported-by: Zoltán Kővágó > Reported-by: Michael Stapelberg > Cc: Daniel Vetter > Cc: Maxime Ripard > Cc: dri-devel@lists.freedesktop.org > Cc: # v5.14+ > --- > drivers/gpu/drm/drm_fb_helper.c | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 6860223f0068..364f11900b37 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -1508,6 +1508,7 @@ static int drm_fb_helper_single_fb_probe(struct > drm_fb_helper *fb_helper, > { > struct drm_client_dev *client = &fb_helper->client; > struct drm_device *dev = fb_helper->dev; > + struct drm_mode_config *config = &dev->mode_config; > int ret = 0; > int crtc_count = 0; > struct drm_connector_list_iter conn_iter; > @@ -1665,6 +1666,11 @@ static int drm_fb_helper_single_fb_probe(struct > drm_fb_helper *fb_helper, > /* Handle our overallocation */ > sizes.surface_height *= drm_fbdev_overalloc; > sizes.surface_height /= 100; > + if (sizes.surface_height > config->max_height) { > + drm_warn(dev, "Fbdev over-allocation too large; clamping height > to %d\n", > + config->max_height); drm_warn() seems a bit excessive. drm_info()? Or could just have no printk and use a simple min() perhaps. > + sizes.surface_height = config->max_height; > + } > > /* push down into drivers */ > ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes); > -- > 2.33.0 -- Ville Syrjälä Intel
Re: [PATCH v2 5/5] drm: mxsfb: Set proper default bus format when using a bridge
Hi, On Mon, Oct 04, 2021 at 09:58:37AM +0200, Lucas Stach wrote: > Am Montag, dem 04.10.2021 um 09:27 +0200 schrieb Guido Günther: > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > > that case. > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if > > present") > > > I don't think this qualifies for stable, so I would drop this tag, as > the stable maintainers are quite trigger happy to pull in patches with > a fixes tag. Also the subject isn't quite correct, this isn't setting a > "proper" bus format, but rather adds a fallback. Other than that: Adjusted for v3 (which I'll hold off a bit in case there are more comments) and dropped the Fixes: tag which is on the nwl driver only now. thanks! -- Guido > > Reviewed-by: Lucas Stach > > Regards, > Lucas > > > Reported-by: Martin Kepplinger > > Signed-off-by: Guido Günther > > > --- > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > index d6abd2077114..e3fbb8b58d5d 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > @@ -369,6 +369,12 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc > > *crtc, > > drm_atomic_get_new_bridge_state(state, > > mxsfb->bridge); > > bus_format = bridge_state->input_bus_cfg.format; > > + if (bus_format == MEDIA_BUS_FMT_FIXED) { > > + dev_warn_once(drm->dev, > > + "Bridge does not provide bus format, > > assuming MEDIA_BUS_FMT_RGB888_1X24.\n" > > + "Please fix bridge driver by handling > > atomic_get_input_bus_fmts.\n"); > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > + } > > } > > > > /* If there is no bridge, use bus format from connector */ > >
Re: [RFC 1/6] sched: Add nice value change notifier
On Mon, Oct 04, 2021 at 09:12:37AM +0100, Tvrtko Ursulin wrote: > On 01/10/2021 16:48, Peter Zijlstra wrote: > > Hmm? That's for normalize_rt_tasks() only, right? Just don't have it > > call the notifier in that special case (that's a magic sysrq thing > > anyway). > > You mean my talk about tasklist_lock? No, it is also on the syscall part I > am interested in as well. Call chain looks like this: Urgh, I alwys miss that because it lives outside of sched.. :/ > sys_setpriority() > { > ... > rcu_read_lock(); > read_lock(&tasklist_lock); > ... > set_one_prio() > set_user_nice() > { > ... > task_rq_lock(); > -> my notifier from this RFC [1] > task_rq_unlock(); > -> I can move the notifier here for _some_ improvement [2] > } > ... > read_unlock(&tasklist_lock); > rcu_read_unlock(); > } > > So this RFC had the notifier call chain at [1], which I understood was the > thing you initially pointed was horrible, being under a scheduler lock. > > I can trivially move it to [2] but that still leaves it under the tasklist > lock. I don't have a good feel how much better that would be. If not good > enough then I will look for a smarter solution with less opportunity for > global impact. So task_list lock is pretty terrible and effectively unbound already (just create more tasks etc..) so adding a notifier call there shouldn't really make it much worse.
Re: [Intel-gfx] [PATCH v2 00/17] drm: cleanup: Use DRM_MODESET_LOCK_ALL_* helpers where possible
On Mon, 04 Oct 2021, Ville Syrjälä wrote: > On Sun, Oct 03, 2021 at 12:32:14AM +0200, Fernando Ramos wrote: >> On 21/10/02 09:13AM, Fernando Ramos wrote: >> > >> > Sean, could you revert the whole patch series? I'll have a deeper look >> > into the >> > patch set and come up with a v3 where all these issues will be addressed. >> > >> >> Hi Sean, >> >> I now understand the nature of the issue that caused the problem with i915 >> and >> have proceed to remove the global context structure (which revealed a similar >> issue in the amdgpu driver). >> >> I have prepared a V3 version of the patch set where these issues should >> hopefully be fixed for both the i915 and amdgpu drivers. >> >> In order to prevent causing more disruption, could you tell me what the >> proper >> way to proceed would be? In particular: >> >> 1. Is there any place where I can push my changes so that they are tested >> on a i915 machine? (Some type of automated pool) > > cc:intel-gfx, which it looks like you did, _but_ your patches did > did not even apply against drm-tip so our CI rejected it. There was > a reply to the patches from CI indicating that. And that is one > reason I probably just ignored the whole thing. If it doesn't > even apply/build it's not worth my time to read. > >> >> 2. I can test the amdgpu driver on my machine but, what about all the other >> architectures? What is the standard procedure? Should I simply publish >> V3 >> and wait for feedback from the different vendors? (I would hate to >> cause a >> simular situation again) >> >> 3. Should I post V3 on top of drm-next or drm-misc-next? > > The normal rule is: always work on drm-tip. That is what gets > tested by our CI as well. Yes, it does mean a bit of extra hurdles > during development since drm-tip is a rebasing tree, but there are > tools like dim retip to help out here. > > As for where to merge them. I would generally recommed against merging > i915 patches through drm-misc unless there is a very compelling reason > to do so. i915 is a fast moving target and if there are significant > changes coming in via drm-misc they usually will cause conflicts for > people during drm-tip rebuild. Also I would expect to see an ack > requested from i915 maintainers for merging anything significant via > drm-misc, which I don't think happened in this case. Indeed. All other things aside, it looks like it has enough conflict potential to warrant merging via drm-intel anyway. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v13 00/35] NVIDIA Tegra power management patches for 5.16
On 27-09-21, 01:40, Dmitry Osipenko wrote: > This series adds runtime PM support to Tegra drivers and enables core > voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. > > All patches in this series are interdependent and should go via Tegra tree. So you don't need any OPP changes anymore ? I just came back from vacation, don't know what you guys discussed in between :) -- viresh
Re: [PATCH v13 01/35] opp: Change type of dev_pm_opp_attach_genpd(names) argument
On 27-09-21, 01:40, Dmitry Osipenko wrote: > Elements of the 'names' array are not changed by the code, constify them > for consistency. > > Signed-off-by: Dmitry Osipenko > --- > drivers/opp/core.c | 6 +++--- > include/linux/pm_opp.h | 8 > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 04b4691a8aac..3057beabd370 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -2348,12 +2348,12 @@ static void _opp_detach_genpd(struct opp_table > *opp_table) > * "required-opps" are added in DT. > */ > struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > - const char **names, struct device ***virt_devs) > + const char * const *names, struct device ***virt_devs) > { > struct opp_table *opp_table; > struct device *virt_dev; > int index = 0, ret = -EINVAL; > - const char **name = names; > + const char * const *name = names; > > opp_table = _add_opp_table(dev, false); > if (IS_ERR(opp_table)) > @@ -2457,7 +2457,7 @@ static void devm_pm_opp_detach_genpd(void *data) > * > * Return: 0 on success and errorno otherwise. > */ > -int devm_pm_opp_attach_genpd(struct device *dev, const char **names, > +int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names, >struct device ***virt_devs) > { > struct opp_table *opp_table; > diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h > index a95d6fdd20b6..879c138c7b8e 100644 > --- a/include/linux/pm_opp.h > +++ b/include/linux/pm_opp.h > @@ -156,9 +156,9 @@ int devm_pm_opp_set_clkname(struct device *dev, const > char *name); > struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int > (*set_opp)(struct dev_pm_set_opp_data *data)); > void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table); > int devm_pm_opp_register_set_opp_helper(struct device *dev, int > (*set_opp)(struct dev_pm_set_opp_data *data)); > -struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char > **names, struct device ***virt_devs); > +struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, const char * > const *names, struct device ***virt_devs); > void dev_pm_opp_detach_genpd(struct opp_table *opp_table); > -int devm_pm_opp_attach_genpd(struct device *dev, const char **names, struct > device ***virt_devs); > +int devm_pm_opp_attach_genpd(struct device *dev, const char * const *names, > struct device ***virt_devs); > struct dev_pm_opp *dev_pm_opp_xlate_required_opp(struct opp_table > *src_table, struct opp_table *dst_table, struct dev_pm_opp *src_opp); > int dev_pm_opp_xlate_performance_state(struct opp_table *src_table, struct > opp_table *dst_table, unsigned int pstate); > int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq); > @@ -376,7 +376,7 @@ static inline int devm_pm_opp_set_clkname(struct device > *dev, const char *name) > return -EOPNOTSUPP; > } > > -static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > const char **names, struct device ***virt_devs) > +static inline struct opp_table *dev_pm_opp_attach_genpd(struct device *dev, > const char * const *names, struct device ***virt_devs) > { > return ERR_PTR(-EOPNOTSUPP); > } > @@ -384,7 +384,7 @@ static inline struct opp_table > *dev_pm_opp_attach_genpd(struct device *dev, cons > static inline void dev_pm_opp_detach_genpd(struct opp_table *opp_table) {} > > static inline int devm_pm_opp_attach_genpd(struct device *dev, > -const char **names, > +const char * const *names, > struct device ***virt_devs) > { > return -EOPNOTSUPP; Applied. Thanks. -- viresh
[PATCH] drm/connector: refer to CTA-861-G in the "content type" prop docs
The KMS documentation doesn't say much about the meaning of each content type. Add a reference to the specification defining them. Signed-off-by: Simon Ser Cc: Emmanuel Gil Peyrot Cc: Daniel Vetter Cc: Pekka Paalanen Cc: Ville Syrjala Cc: Jani Nikula --- drivers/gpu/drm/drm_connector.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 3bc782b630b9..79d8163686cd 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -1397,6 +1397,8 @@ EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); * Game: * Content type is game * + * The meaning of each content type is defined in CTA-861-G table 15. + * * Drivers can set up this property by calling * drm_connector_attach_content_type_property(). Decoding to * infoframe values is done through drm_hdmi_avi_infoframe_content_type(). -- 2.33.0
Re: [PATCH] drm/connector: refer to CTA-861-G in the "content type" prop docs
On Mon, Oct 04, 2021 at 09:12:50AM +, Simon Ser wrote: > The KMS documentation doesn't say much about the meaning of each > content type. Add a reference to the specification defining them. > > Signed-off-by: Simon Ser > Cc: Emmanuel Gil Peyrot > Cc: Daniel Vetter > Cc: Pekka Paalanen > Cc: Ville Syrjala > Cc: Jani Nikula > --- > drivers/gpu/drm/drm_connector.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index 3bc782b630b9..79d8163686cd 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -1397,6 +1397,8 @@ > EXPORT_SYMBOL(drm_connector_attach_dp_subconnector_property); > * Game: > * Content type is game > * > + * The meaning of each content type is defined in CTA-861-G table 15. > + * A bit annoying to have to refer to an external spec, but copy pasting the whole thing here seems a bit questionable. Reviewed-by: Ville Syrjälä > * Drivers can set up this property by calling > * drm_connector_attach_content_type_property(). Decoding to > * infoframe values is done through drm_hdmi_avi_infoframe_content_type(). > -- > 2.33.0 > -- Ville Syrjälä Intel
Re: [PATCH] drm/connector: refer to CTA-861-G in the "content type" prop docs
On Monday, October 4th, 2021 at 11:22, Ville Syrjälä wrote: > A bit annoying to have to refer to an external spec, but copy pasting > the whole thing here seems a bit questionable. Yeah, I'm mostly worried about copyright. We could also invent our own descriptions (Is that even possible without infringing copyright? The person inventing the new descriptions needs to read the original spec to know what to write…), but I think referring to the original spec is desirable regardless.
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->fence; int index = cursor->index; dma_fence_put(fence); fence = NULL; next: if (index == -1) { /* Try picking the exclusive fence. */ index++; fence = dma_resv_excl_fence(obj); if (!fence) goto next; } else if (cursor->fences && index < cursor->fences->shared_count) { /* Try picking next shared fence. */ struct dma_resv_list *fences = cursor->fences; fence = rcu_dereference(fences->shared[index++]); } if (fence) { if (dma_fence_is_signaled(fence)) goto next; /* Skip signaled. */ fence = dma_fence_get_rcu(fence); WARN_ON(!fence); } cursor->fence = fence; cursor->index = index; (I started with a loop here but ended with goto based flow since it ended up more succinct.) At least if I don't have a handling flaw in there it looks like easier to follow flow to me. Plus picking a not signaled fence works without a reference FWIW. How does it look to you? Regards, Tvrtko + if (!cursor->fence || !dma_fence_is_signaled(cursor->fence)) + break; + } while (true); +} + +/** + * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. + * @cursor: the curso
Re: [PATCH 15/16] Revert "drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()"
On Sat, Oct 02, 2021 at 11:45:41AM -0400, Sean Paul wrote: > From: Sean Paul > > This reverts commit 399190e70816886e2bca1f3f3bc3d9c544af88e7. > > This patchset breaks on intel platforms and was previously NACK'd by > Ville. > > Cc: Ville Syrjälä > Cc: Fernando Ramos > Signed-off-by: Sean Paul Yeah, best to try again from the start I think. For the series Acked-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/display/intel_display.c | 18 +- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 2bf01416d656..134a6acbd8fb 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -43,7 +43,6 @@ > #include > #include > #include > -#include > > #include "display/intel_audio.h" > #include "display/intel_crt.h" > @@ -13477,13 +13476,22 @@ void intel_display_resume(struct drm_device *dev) > if (state) > state->acquire_ctx = &ctx; > > - DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); > + drm_modeset_acquire_init(&ctx, 0); > > - ret = __intel_display_resume(dev, state, &ctx); > + while (1) { > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > + if (ret != -EDEADLK) > + break; > > - intel_enable_ipc(dev_priv); > + drm_modeset_backoff(&ctx); > + } > + > + if (!ret) > + ret = __intel_display_resume(dev, state, &ctx); > > - DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > + intel_enable_ipc(dev_priv); > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > > if (ret) > drm_err(&dev_priv->drm, > -- > Sean Paul, Software Engineer, Google / Chromium OS -- Ville Syrjälä Intel
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. That won't work. It's perfectly valid for dma_fence_get_rcu() to return NULL when we are racing here. Keep in mind that we don't hold any locks. What we could do is to return NULL and repeat with a new sequence immediately though. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->fence; int index = cursor->index; dma_fence_put(fence); fence = NULL; next: if (index == -1) { /* Try picking the exclusive fence. */ index++; fence = dma_resv_excl_fence(obj); if (!fence) goto next; } else if (cursor->fences && index < cursor->fences->shared_count) { /* Try picking next shared fence. */ struct dma_resv_list *fences = cursor->fences; fence = rcu_dereference(fences->shared[index++]); } if (fence) { if (dma_fence_is_signaled(fence)) goto next; /* Skip signaled. */ fence = dma_fence_get_rcu(fence); WARN_ON(!fence); } cursor->fence = fence; cursor->index = index; (I started with a loop here but ended with goto based flow since it ended up more succinct.) At least if I don't have a handling flaw in there it looks like easier to follow flow to me. Plus picking a not signaled fence works without a reference FWIW. I strongly don't think that this will work correctly. You need to grab a reference first when you want to call dma_fence_is_signaled(), that's why I used the testbit approach initially. How does it look to you? Mhm, let me try to reorder the loo
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 04/10/2021 10:53, Christian König wrote: Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. That won't work. It's perfectly valid for dma_fence_get_rcu() to return NULL when we are racing here. Keep in mind that we don't hold any locks. Ah yes.. No need to change anything then, sorry for the confusion. I did not find any holes, the rest was just about how to maybe make the flow more obvious. Let me know if you want r-b now or later. Regards, Tvrtko What we could do is to return NULL and repeat with a new sequence immediately though. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->fence; int index = cursor->index; dma_fence_put(fence); fence = NULL; next: if (index == -1) { /* Try picking the exclusive fence. */ index++; fence = dma_resv_excl_fence(obj); if (!fence) goto next; } else if (cursor->fences && index < cursor->fences->shared_count) { /* Try picking next shared fence. */ struct dma_resv_list *fences = cursor->fences; fence = rcu_dereference(fences->shared[index++]); } if (fence) { if (dma_fence_is_signaled(fence)) goto next; /* Skip signaled. */ fence = dma_fence_get_rcu(fence); WARN_ON(!fence); } cursor->fence = fence; cursor->index = index; (I started with a loop here but ended with goto based flow since it ended up more succinct.) At least if I don't have a handling flaw in there it looks like easier to follow flow to me. Plus picking a not signaled fence works without a r
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Rewrite CL/CTG L-shaped memory detection
On Tue, Apr 27, 2021 at 10:58:07AM +0200, Daniel Vetter wrote: > On Mon, Apr 26, 2021 at 08:18:39PM +0300, Ville Syrjälä wrote: > > On Mon, Apr 26, 2021 at 06:08:59PM +0200, Daniel Vetter wrote: > > > On Thu, Apr 22, 2021 at 04:11:22PM +0300, Ville Syrjälä wrote: > > > > On Thu, Apr 22, 2021 at 11:49:43AM +0200, Daniel Vetter wrote: > > > > > On Wed, Apr 21, 2021 at 06:34:01PM +0300, Ville Syrjala wrote: > > > > > > From: Ville Syrjälä > > > > > > > > > > > > Currently we try to detect a symmetric memory configurations > > > > > > using a magic DCC2_MODIFIED_ENHANCED_DISABLE bit. That bit is > > > > > > either only set on a very specific subset of machines or it > > > > > > just does not exist (it's not mentioned in any public chipset > > > > > > datasheets I've found). As it happens my CL/CTG machines never > > > > > > set said bit, even if I populate the channels with identical > > > > > > sticks. > > > > > > > > > > > > So let's do the L-shaped memory detection the same way as the > > > > > > desktop variants, ie. just look at the DRAM rank boundary > > > > > > registers to see if both channels have an identical size. > > > > > > > > > > > > With this my CL/CTG no longer claim L-shaped memory when I use > > > > > > identical sticks. Also tested with non-matching sticks just to > > > > > > make sure the L-shaped memory is still properly detected. > > > > > > > > > > > > And for completeness let's update the debugfs code to dump > > > > > > the correct set of registers on each platform. > > > > > > > > > > > > Cc: Chris Wilson > > > > > > Signed-off-by: Ville Syrjälä > > > > > > > > > > Did you check this with the swapping igt? I have some vague memories > > > > > of > > > > > bug reports where somehow the machine was acting like it's L-shaped > > > > > memory > > > > > despite that banks were populated equally. I've iirc tried all kinds > > > > > of > > > > > tricks to figure it out, all to absolutely no avail. > > > > > > > > Did you have a specific test in mind? I ran a bunch of things > > > > that seemed swizzle related. All passed just fine. > > > > > > gem_tiled_swapping should be the one. It tries to cycle your entire system > > > memory through tiled buffers into swap and out of it. > > > > Passes with symmetric config, fails with L-shaped config (if I hack > > out the L-shape detection of course). So seems pretty solid. > > > > A kernel based self test that looks at the physical address would > > still be nice I suppose. Though depending on the size of your RAM > > sticks figuring out where exactly the switchover from two channels > > to one channels happens probably requires a bit of work due to > > the PCI hole/etc. > > > > Both my cl and ctg report this btw: > > bit6 swizzle for X-tiling = bit9/bit10/bit11 > > bit6 swizzle for Y-tiling = bit9/bit11 > > so unfortunately can't be sure the other swizzle modes would be > > correctly detected. > > I think testing-wise this is as good as it gets. So what do we think about putting this in? Currently this only works by sheer luck more or less. On my machines we have a false positive which is safe now that the pin quirk got fixed, but if some other machines have a false negative then things are not going to go so well. -- Ville Syrjälä Intel
[PATCH] drm/i915/tc: Delete bogus NULL check in intel_ddi_encoder_destroy()
The "digi_port" pointer can't be NULL and we have already dereferenced it so checking for NULL is not necessary. Delete the check. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/i915/display/intel_ddi.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 51d07e9af9f3..b9c6eb13804f 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -4025,8 +4025,7 @@ static void intel_ddi_encoder_destroy(struct drm_encoder *encoder) intel_display_power_flush_work(i915); drm_encoder_cleanup(encoder); - if (dig_port) - kfree(dig_port->hdcp_port_data.streams); + kfree(dig_port->hdcp_port_data.streams); kfree(dig_port); } -- 2.20.1
[PATCH] drm/msm: potential error pointer dereference in init()
The msm_iommu_new() returns error pointers on failure so check for that to avoid an Oops. Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index ae48f41821cf..ad247c06e198 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -908,6 +908,10 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms) return 0; mmu = msm_iommu_new(dpu_kms->dev->dev, domain); + if (IS_ERR(mmu)) { + iommu_domain_free(domain); + return PTR_ERR(mmu); + } aspace = msm_gem_address_space_create(mmu, "dpu1", 0x1000, 0x1 - 0x1000); -- 2.20.1
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin: On 04/10/2021 10:53, Christian König wrote: Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. That won't work. It's perfectly valid for dma_fence_get_rcu() to return NULL when we are racing here. Keep in mind that we don't hold any locks. Ah yes.. No need to change anything then, sorry for the confusion. I did not find any holes, the rest was just about how to maybe make the flow more obvious. Let me know if you want r-b now or later. Now would be good. I've tried to make that more cleaner, but this only lead to repeating the code more often. Regards, Christian. Regards, Tvrtko What we could do is to return NULL and repeat with a new sequence immediately though. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->fence; int index = cursor->index; dma_fence_put(fence); fence = NULL; next: if (index == -1) { /* Try picking the exclusive fence. */ index++; fence = dma_resv_excl_fence(obj); if (!fence) goto next; } else if (cursor->fences && index < cursor->fences->shared_count) { /* Try picking next shared fence. */ struct dma_resv_list *fences = cursor->fences; fence = rcu_dereference(fences->shared[index++]); } if (fence) { if (dma_fence_is_signaled(fence)) goto next; /* Skip signaled. */ fence = dma_fence_get_rcu(fence); WARN_ON(!fence); } cursor->fence = fence; cursor->index = index; (I started with a loop here but ended with goto based flo
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 04/10/2021 11:44, Christian König wrote: Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin: On 04/10/2021 10:53, Christian König wrote: Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. That won't work. It's perfectly valid for dma_fence_get_rcu() to return NULL when we are racing here. Keep in mind that we don't hold any locks. Ah yes.. No need to change anything then, sorry for the confusion. I did not find any holes, the rest was just about how to maybe make the flow more obvious. Let me know if you want r-b now or later. Now would be good. I've tried to make that more cleaner, but this only lead to repeating the code more often. Reviewed-by: Tvrtko Ursulin Regards, Tvrtko Regards, Christian. Regards, Tvrtko What we could do is to return NULL and repeat with a new sequence immediately though. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->fence; int index = cursor->index; dma_fence_put(fence); fence = NULL; next: if (index == -1) { /* Try picking the exclusive fence. */ index++; fence = dma_resv_excl_fence(obj); if (!fence) goto next; } else if (cursor->fences && index < cursor->fences->shared_count) { /* Try picking next shared fence. */ struct dma_resv_list *fences = cursor->fences; fence = rcu_dereference(fences->shared[index++]); } if (fence) { if (dma_fence_is_signaled(fence)) goto next; /* Skip signaled. */ fence = dma_fence_get_rcu(fence); WARN_ON(!fence); } cursor->f
Re: [PATCH v13 13/35] drm/tegra: gr2d: Support generic power domain and runtime PM
On Fri, 1 Oct 2021 at 21:00, Dmitry Osipenko wrote: > > 01.10.2021 17:55, Ulf Hansson пишет: > > On Fri, 1 Oct 2021 at 16:29, Dmitry Osipenko wrote: > >> > >> 01.10.2021 16:39, Ulf Hansson пишет: > >>> On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: > > Add runtime power management and support generic power domains. > > Tested-by: Peter Geis # Ouya T30 > Tested-by: Paul Fertser # PAZ00 T20 > Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 > Tested-by: Matt Merhar # Ouya T30 > Signed-off-by: Dmitry Osipenko > --- > drivers/gpu/drm/tegra/gr2d.c | 155 +-- > >>> > >>> [...] > >>> > static int gr2d_remove(struct platform_device *pdev) > @@ -259,15 +312,101 @@ static int gr2d_remove(struct platform_device > *pdev) > return err; > } > > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > >>> > >>> There is no guarantee that the ->runtime_suspend() has been invoked > >>> here, which means that clock may be left prepared/enabled beyond this > >>> point. > >>> > >>> I suggest you call pm_runtime_force_suspend(), instead of > >>> pm_runtime_disable(), to make sure that gets done. > >> > >> The pm_runtime_disable() performs the final synchronization, please see > >> [1]. > >> > >> [1] > >> https://elixir.bootlin.com/linux/v5.15-rc3/source/drivers/base/power/runtime.c#L1412 > > > > pm_runtime_disable() end up calling _pm_runtime_barrier(), which calls > > cancel_work_sync() if dev->power.request_pending has been set. > > > > If the work that was punted to the pm_wq in rpm_idle() has not been > > started yet, we end up just canceling it. In other words, there are no > > guarantees it runs to completion. > > You're right. Although, in a case of this particular patch, the syncing > is actually implicitly done by pm_runtime_dont_use_autosuspend(). > > But for drivers which don't use auto-suspend, there is no sync. This > looks like a disaster, it's a very common pattern for drivers to > 'put+disable'. > > > Moreover, use space may have bumped the usage count via sysfs for the > > device (pm_runtime_forbid()) to keep the device runtime resumed. > > Right, this is also a disaster in a case of driver removal. > > >> Calling pm_runtime_force_suspend() isn't correct because each 'enable' > >> must have the corresponding 'disable'. Hence there is no problem here. > > > > pm_runtime_force_suspend() calls pm_runtime_disable(), so I think that > > should be fine. No? > > [adding Rafael] > > Rafael, could you please explain how drivers are supposed to properly > suspend and disable RPM to cut off power and reset state that was > altered by the driver's resume callback? What we're missing? Is Ulf's > suggestion acceptable? > > The RPM state of a device is getting reset on driver's removal, hence > all refcounts that were bumped by the rpm-resume callback of the device > driver will be screwed up if device is kept resumed after removal. I > just verified that it's true in practice. Note that, what makes the Tegra drivers a bit special is that they are always built with CONFIG_PM being set (selected from the "SoC" Kconfig). Therefore, pm_runtime_force_suspend() can work for some of these cases. Using this, would potentially avoid the driver from having to runtime resume the device in ->remove(), according to the below generic sequence, which is used in many drivers. pm_runtime_get_sync() clk_disable_unprepare() (+ additional things to turn off the device) pm_runtime_disable() pm_runtime_put_noidle() Kind regards Uffe
Re: [PATCH 1/4] drm: Introduce drm_modeset_lock_ctx_retry()
On Tue, Jul 20, 2021 at 03:44:49PM +0200, Daniel Vetter wrote: > On Thu, Jul 15, 2021 at 09:49:51PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä > > > > Quite a few places are hand rolling the modeset lock backoff dance. > > Let's suck that into a helper macro that is easier to use without > > forgetting some steps. > > > > The main downside is probably that the implementation of > > drm_with_modeset_lock_ctx() is a bit harder to read than a hand > > rolled version on account of being split across three functions, > > but the actual code using it ends up being much simpler. > > > > Cc: Sean Paul > > Cc: Daniel Vetter > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/drm_modeset_lock.c | 44 ++ > > include/drm/drm_modeset_lock.h | 20 ++ > > 2 files changed, 64 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_modeset_lock.c > > b/drivers/gpu/drm/drm_modeset_lock.c > > index fcfe1a03c4a1..083df96632e8 100644 > > --- a/drivers/gpu/drm/drm_modeset_lock.c > > +++ b/drivers/gpu/drm/drm_modeset_lock.c > > @@ -425,3 +425,47 @@ int drm_modeset_lock_all_ctx(struct drm_device *dev, > > return 0; > > } > > EXPORT_SYMBOL(drm_modeset_lock_all_ctx); > > + > > +void _drm_modeset_lock_begin(struct drm_modeset_acquire_ctx *ctx, > > +struct drm_atomic_state *state, > > +unsigned int flags, int *ret) > > +{ > > + drm_modeset_acquire_init(ctx, flags); > > + > > + if (state) > > + state->acquire_ctx = ctx; > > + > > + *ret = -EDEADLK; > > +} > > +EXPORT_SYMBOL(_drm_modeset_lock_begin); > > + > > +bool _drm_modeset_lock_loop(int *ret) > > +{ > > + if (*ret == -EDEADLK) { > > + *ret = 0; > > + return true; > > + } > > + > > + return false; > > +} > > +EXPORT_SYMBOL(_drm_modeset_lock_loop); > > + > > +void _drm_modeset_lock_end(struct drm_modeset_acquire_ctx *ctx, > > + struct drm_atomic_state *state, > > + int *ret) > > +{ > > + if (*ret == -EDEADLK) { > > + if (state) > > + drm_atomic_state_clear(state); > > + > > + *ret = drm_modeset_backoff(ctx); > > + if (*ret == 0) { > > + *ret = -EDEADLK; > > + return; > > + } > > + } > > + > > + drm_modeset_drop_locks(ctx); > > + drm_modeset_acquire_fini(ctx); > > +} > > +EXPORT_SYMBOL(_drm_modeset_lock_end); > > diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h > > index aafd07388eb7..5eaad2533de5 100644 > > --- a/include/drm/drm_modeset_lock.h > > +++ b/include/drm/drm_modeset_lock.h > > @@ -26,6 +26,7 @@ > > > > #include > > > > +struct drm_atomic_state; > > struct drm_modeset_lock; > > > > /** > > @@ -203,4 +204,23 @@ modeset_lock_fail: > > \ > > if (!drm_drv_uses_atomic_modeset(dev)) \ > > mutex_unlock(&dev->mode_config.mutex); > > > > +void _drm_modeset_lock_begin(struct drm_modeset_acquire_ctx *ctx, > > +struct drm_atomic_state *state, > > +unsigned int flags, > > +int *ret); > > +bool _drm_modeset_lock_loop(int *ret); > > +void _drm_modeset_lock_end(struct drm_modeset_acquire_ctx *ctx, > > + struct drm_atomic_state *state, > > + int *ret); > > + > > +/* > > + * Note that one must always use "continue" rather than > > + * "break" or "return" to handle errors within the > > + * drm_modeset_lock_ctx_retry() block. > > I'm not sold on loop macros with these kind of restrictions, C just isn't > a great language for these. That's why e.g. drm_connector_iter doesn't > give you a macro, but only the begin/next/end function calls explicitly. We already use this pattern extensively in i915. Gem ww ctx has one, power domains/pps/etc. use a similar things. It makes the code pretty nice, with the slight caveat that an accidental 'break' can ruin your day. But so can an accidental return with other constructs (and we even had that happen a few times with the connector iterators), so not a dealbreaker IMO. So if we don't want this drm wide I guess I can propose this just for i915 since it fits in perfectly there. > > Yes the macro we have is also not nice, but at least it's a screaming > macro since it's all uppercase, so options are all a bit sucky. Which > leads me to think we have a bit a https://xkcd.com/927/ situation going > on. > > I think minimally we should have one way to do this. Well, there is no one way atm. All you can do is hand roll all the boilerplate (and likely get it slightly wrong) if you don't want lock_all. The current macros only help with lock_all, and IMO the hidden gotos are even uglier than a hidden for loop. Fernando already hit a case where he couldn't use the macros twice due to conflicting goto labels.
[PATCH v2] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
In case of a modeset where a mode gets split across mutiple CRTCs in the driver specific implementation (bigjoiner in i915) we wrongly count the affected CRTCs based on the drm_crtc_mask and indicate the stolen CRTC as an affected CRTC in atomic_check_only(). This triggers a warning since affected CRTCs doent match requested CRTC. To fix this in such bigjoiner configurations, we should only increment affected crtcs if that CRTC is enabled in UAPI not if it is just used internally in the driver to split the mode. There is no way we can adjust requested_crtc calculation as suggested in review comments because the crtc gets stolen only after the atomic_check call. Cc: Ville Syrjälä Cc: Simon Ser Cc: Pekka Paalanen Cc: Daniel Stone Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org Signed-off-by: Manasi Navare --- drivers/gpu/drm/drm_atomic.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ff1416cd609a..44e7ebf43a2a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1360,8 +1360,10 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } } - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) - affected_crtc |= drm_crtc_mask(crtc); + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->enable) + affected_crtc |= drm_crtc_mask(crtc); + } /* * For commits that allow modesets drivers can add other CRTCs to the -- 2.19.1
Re: [PATCH v5 5/8] drm/panfrost: Add a new ioctl to submit batches
On 30/09/2021 20:09, Boris Brezillon wrote: > This should help limit the number of ioctls when submitting multiple > jobs. The new ioctl also supports syncobj timelines and BO access flags. > > v5: > * Fix typos > * Add BUILD_BUG_ON() checks to make sure SUBMIT_BATCH_VERSION and > descriptor sizes are synced > * Simplify error handling in panfrost_ioctl_batch_submit() > * Don't disable implicit fences on exclusive references > > v4: > * Implement panfrost_ioctl_submit() as a wrapper around > panfrost_submit_job() > * Replace stride fields by a version field which is mapped to > a tuple internally > > v3: > * Re-use panfrost_get_job_bos() and panfrost_get_job_in_syncs() in the > old submit path > > Signed-off-by: Boris Brezillon Reviewed-by: Steven Price > --- > drivers/gpu/drm/panfrost/panfrost_drv.c | 584 > drivers/gpu/drm/panfrost/panfrost_job.c | 4 +- > include/uapi/drm/panfrost_drm.h | 92 > 3 files changed, 492 insertions(+), 188 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index f8f430f68090..30dc158d56e6 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -147,193 +147,6 @@ panfrost_get_job_mappings(struct drm_file *file_priv, > struct panfrost_job *job) > return 0; > } > > -/** > - * panfrost_lookup_bos() - Sets up job->bo[] with the GEM objects > - * referenced by the job. > - * @dev: DRM device > - * @file_priv: DRM file for this fd > - * @args: IOCTL args > - * @job: job being set up > - * > - * Resolve handles from userspace to BOs and attach them to job. > - * > - * Note that this function doesn't need to unreference the BOs on > - * failure, because that will happen at panfrost_job_cleanup() time. > - */ > -static int > -panfrost_lookup_bos(struct drm_device *dev, > - struct drm_file *file_priv, > - struct drm_panfrost_submit *args, > - struct panfrost_job *job) > -{ > - unsigned int i; > - int ret; > - > - job->bo_count = args->bo_handle_count; > - > - if (!job->bo_count) > - return 0; > - > - job->bo_flags = kvmalloc_array(job->bo_count, > -sizeof(*job->bo_flags), > -GFP_KERNEL | __GFP_ZERO); > - if (!job->bo_flags) > - return -ENOMEM; > - > - for (i = 0; i < job->bo_count; i++) > - job->bo_flags[i] = PANFROST_BO_REF_EXCLUSIVE; > - > - ret = drm_gem_objects_lookup(file_priv, > - (void __user *)(uintptr_t)args->bo_handles, > - job->bo_count, &job->bos); > - if (ret) > - return ret; > - > - return panfrost_get_job_mappings(file_priv, job); > -} > - > -/** > - * panfrost_copy_in_sync() - Sets up job->deps with the sync objects > - * referenced by the job. > - * @dev: DRM device > - * @file_priv: DRM file for this fd > - * @args: IOCTL args > - * @job: job being set up > - * > - * Resolve syncobjs from userspace to fences and attach them to job. > - * > - * Note that this function doesn't need to unreference the fences on > - * failure, because that will happen at panfrost_job_cleanup() time. > - */ > -static int > -panfrost_copy_in_sync(struct drm_device *dev, > - struct drm_file *file_priv, > - struct drm_panfrost_submit *args, > - struct panfrost_job *job) > -{ > - u32 *handles; > - int ret = 0; > - int i, in_fence_count; > - > - in_fence_count = args->in_sync_count; > - > - if (!in_fence_count) > - return 0; > - > - handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL); > - if (!handles) { > - ret = -ENOMEM; > - DRM_DEBUG("Failed to allocate incoming syncobj handles\n"); > - goto fail; > - } > - > - if (copy_from_user(handles, > -(void __user *)(uintptr_t)args->in_syncs, > -in_fence_count * sizeof(u32))) { > - ret = -EFAULT; > - DRM_DEBUG("Failed to copy in syncobj handles\n"); > - goto fail; > - } > - > - for (i = 0; i < in_fence_count; i++) { > - struct dma_fence *fence; > - > - ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0, > - &fence); > - if (ret) > - goto fail; > - > - ret = drm_sched_job_add_dependency(&job->base, fence); > - > - if (ret) > - goto fail; > - } > - > -fail: > - kvfree(handles); > - return ret; > -} > - > -static int panfrost_ioctl_submit(struct drm_device *dev, void *data, > - struct drm_file *file) > -{ > - struct panfrost_device *pfdev = dev->dev_private; > - struct drm_panfrost_submit *args = data; >
Re: [PATCH v5 6/8] drm/panfrost: Support synchronization jobs
On 30/09/2021 20:09, Boris Brezillon wrote: > Sometimes, all the user wants to do is add a synchronization point. > Userspace can already do that by submitting a NULL job, but this implies > submitting something to the GPU when we could simply skip the job and > signal the done fence directly. > > v5: > * New patch > > Signed-off-by: Boris Brezillon I had thought we would be fine without kbase's "dependency only atom" because we don't have the fan-{in,out} problems that kbase's atoms produce. But if we're ending up with real hardware NULL jobs then this is clearly better. A couple of minor points below, but as far as I can tell this is functionally correct. Reviewed-by: Steven Price > --- > drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++-- > drivers/gpu/drm/panfrost/panfrost_job.c | 6 ++ > include/uapi/drm/panfrost_drm.h | 7 +++ > 3 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 30dc158d56e6..89a0c484310c 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -542,7 +542,9 @@ static const struct panfrost_submit_ioctl_version_info > submit_versions[] = { > [1] = { 48, 8, 16 }, > }; > > -#define PANFROST_JD_ALLOWED_REQS PANFROST_JD_REQ_FS > +#define PANFROST_JD_ALLOWED_REQS \ > + (PANFROST_JD_REQ_FS | \ > + PANFROST_JD_REQ_DEP_ONLY) > > static int > panfrost_submit_job(struct drm_device *dev, struct drm_file *file_priv, > @@ -559,7 +561,10 @@ panfrost_submit_job(struct drm_device *dev, struct > drm_file *file_priv, > if (args->requirements & ~PANFROST_JD_ALLOWED_REQS) > return -EINVAL; > > - if (!args->head) > + /* If this is a dependency-only job, the job chain head should be NULL, > + * otherwise it should be non-NULL. > + */ > + if ((args->head != 0) != !(args->requirements & > PANFROST_JD_REQ_DEP_ONLY)) NIT: There's confusion over NULL vs 0 here - the code is correct (args->head is a u64 and not a pointer for a kernel) but the comment makes it seem like it should be a pointer. We could side-step the mismatch by rewriting as below: !args->head == !(args->requirements & PANFROST_JD_REQ_DEP_ONLY) Although I'm not convinced whether that's more readable or not! > return -EINVAL; > > bo_stride = submit_versions[version].bo_ref_stride; > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > b/drivers/gpu/drm/panfrost/panfrost_job.c > index 0367cee8f6df..6d8706d4a096 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -192,6 +192,12 @@ static void panfrost_job_hw_submit(struct panfrost_job > *job, int js) > u64 jc_head = job->jc; > int ret; > > + if (job->requirements & PANFROST_JD_REQ_DEP_ONLY) { > + /* Nothing to execute, signal the fence directly. */ > + dma_fence_signal_locked(job->done_fence); > + return; > + } > + It took me a while to convince myself that the reference counting for the PM reference is correct. Before panfrost_job_hw_submit() always returned with an extra reference, but now we have a case which doesn't. AFAICT this is probably fine because we dereference on the path where the hardware has completed the job (which obviously won't happen here). But I'm still a bit uneasy whether the reference counts are always correct. Steve > panfrost_devfreq_record_busy(&pfdev->pfdevfreq); > > ret = pm_runtime_get_sync(pfdev->dev); > diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h > index 5e3f8a344f41..b9df066970f6 100644 > --- a/include/uapi/drm/panfrost_drm.h > +++ b/include/uapi/drm/panfrost_drm.h > @@ -46,6 +46,13 @@ extern "C" { > #define DRM_IOCTL_PANFROST_PERFCNT_DUMP > DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct > drm_panfrost_perfcnt_dump) > > #define PANFROST_JD_REQ_FS (1 << 0) > + > +/* > + * Dependency only job. The job chain head should be set to 0 when this flag > + * is set. > + */ > +#define PANFROST_JD_REQ_DEP_ONLY (1 << 1) > + > /** > * struct drm_panfrost_submit - ioctl argument for submitting commands to > the 3D > * engine. >
Re: [PATCH v2] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
On Mon, Oct 04, 2021 at 04:36:29AM -0700, Manasi Navare wrote: > In case of a modeset where a mode gets split across mutiple CRTCs > in the driver specific implementation (bigjoiner in i915) we wrongly count > the affected CRTCs based on the drm_crtc_mask and indicate the stolen CRTC as > an affected CRTC in atomic_check_only(). > This triggers a warning since affected CRTCs doent match requested CRTC. > > To fix this in such bigjoiner configurations, we should only > increment affected crtcs if that CRTC is enabled in UAPI not > if it is just used internally in the driver to split the mode. > > There is no way we can adjust requested_crtc calculation as suggested > in review comments because the crtc gets stolen only after the atomic_check > call. The uapi crtc_state->enable value does not change due to bigjoiner stealing the crtc. So I don't understand what you're trying to say here. The only internal thing that could alter the set of enabled crtcs on the uapi level is update_connector_routing(true), but that is always called before drm_atomic_check_only(). > > Cc: Ville Syrjälä > Cc: Simon Ser > Cc: Pekka Paalanen > Cc: Daniel Stone > Cc: Daniel Vetter > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Manasi Navare > --- > drivers/gpu/drm/drm_atomic.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index ff1416cd609a..44e7ebf43a2a 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1360,8 +1360,10 @@ int drm_atomic_check_only(struct drm_atomic_state > *state) > } > } > > - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) > - affected_crtc |= drm_crtc_mask(crtc); > + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { > + if (new_crtc_state->enable) > + affected_crtc |= drm_crtc_mask(crtc); > + } > > /* >* For commits that allow modesets drivers can add other CRTCs to the > -- > 2.19.1 -- Ville Syrjälä Intel
[PATCH v3] drm/atomic: Add the crtc to affected crtc only if uapi.enable = true
In case of a modeset where a mode gets split across mutiple CRTCs in the driver specific implementation (bigjoiner in i915) we wrongly count the affected CRTCs based on the drm_crtc_mask and indicate the stolen CRTC as an affected CRTC in atomic_check_only(). This triggers a warning since affected CRTCs doent match requested CRTC. To fix this in such bigjoiner configurations, we should only increment affected crtcs if that CRTC is enabled in UAPI not if it is just used internally in the driver to split the mode. v3: Add the same uapi crtc_state->enable check in requested crtc calc (Ville) Cc: Ville Syrjälä Cc: Simon Ser Cc: Pekka Paalanen Cc: Daniel Stone Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org Signed-off-by: Manasi Navare --- drivers/gpu/drm/drm_atomic.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ff1416cd609a..a1e4c7905ebb 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1310,8 +1310,10 @@ int drm_atomic_check_only(struct drm_atomic_state *state) DRM_DEBUG_ATOMIC("checking %p\n", state); - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) - requested_crtc |= drm_crtc_mask(crtc); + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->enable) + requested_crtc |= drm_crtc_mask(crtc); + } for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { ret = drm_atomic_plane_check(old_plane_state, new_plane_state); @@ -1360,8 +1362,10 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } } - for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) - affected_crtc |= drm_crtc_mask(crtc); + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { + if (new_crtc_state->enable) + affected_crtc |= drm_crtc_mask(crtc); + } /* * For commits that allow modesets drivers can add other CRTCs to the -- 2.19.1
Re: [PATCH v5 6/8] drm/panfrost: Support synchronization jobs
On Mon, 4 Oct 2021 12:30:42 +0100 Steven Price wrote: > On 30/09/2021 20:09, Boris Brezillon wrote: > > Sometimes, all the user wants to do is add a synchronization point. > > Userspace can already do that by submitting a NULL job, but this implies > > submitting something to the GPU when we could simply skip the job and > > signal the done fence directly. > > > > v5: > > * New patch > > > > Signed-off-by: Boris Brezillon > > I had thought we would be fine without kbase's "dependency only atom" > because we don't have the fan-{in,out} problems that kbase's atoms > produce. But if we're ending up with real hardware NULL jobs then this > is clearly better. > > A couple of minor points below, but as far as I can tell this is > functionally correct. > > Reviewed-by: Steven Price > > > --- > > drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++-- > > drivers/gpu/drm/panfrost/panfrost_job.c | 6 ++ > > include/uapi/drm/panfrost_drm.h | 7 +++ > > 3 files changed, 20 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c > > b/drivers/gpu/drm/panfrost/panfrost_drv.c > > index 30dc158d56e6..89a0c484310c 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > > @@ -542,7 +542,9 @@ static const struct panfrost_submit_ioctl_version_info > > submit_versions[] = { > > [1] = { 48, 8, 16 }, > > }; > > > > -#define PANFROST_JD_ALLOWED_REQS PANFROST_JD_REQ_FS > > +#define PANFROST_JD_ALLOWED_REQS \ > > + (PANFROST_JD_REQ_FS | \ > > +PANFROST_JD_REQ_DEP_ONLY) > > > > static int > > panfrost_submit_job(struct drm_device *dev, struct drm_file *file_priv, > > @@ -559,7 +561,10 @@ panfrost_submit_job(struct drm_device *dev, struct > > drm_file *file_priv, > > if (args->requirements & ~PANFROST_JD_ALLOWED_REQS) > > return -EINVAL; > > > > - if (!args->head) > > + /* If this is a dependency-only job, the job chain head should be NULL, > > +* otherwise it should be non-NULL. > > +*/ > > + if ((args->head != 0) != !(args->requirements & > > PANFROST_JD_REQ_DEP_ONLY)) > > NIT: There's confusion over NULL vs 0 here - the code is correct > (args->head is a u64 and not a pointer for a kernel) but the comment > makes it seem like it should be a pointer. > > We could side-step the mismatch by rewriting as below: > > !args->head == !(args->requirements & PANFROST_JD_REQ_DEP_ONLY) > > Although I'm not convinced whether that's more readable or not! I'll replace 'NULL' by 'zero' in the comment. > > > return -EINVAL; > > > > bo_stride = submit_versions[version].bo_ref_stride; > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c > > b/drivers/gpu/drm/panfrost/panfrost_job.c > > index 0367cee8f6df..6d8706d4a096 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > > @@ -192,6 +192,12 @@ static void panfrost_job_hw_submit(struct panfrost_job > > *job, int js) > > u64 jc_head = job->jc; > > int ret; > > > > + if (job->requirements & PANFROST_JD_REQ_DEP_ONLY) { > > + /* Nothing to execute, signal the fence directly. */ > > + dma_fence_signal_locked(job->done_fence); > > + return; > > + } > > + > > It took me a while to convince myself that the reference counting for > the PM reference is correct. Before panfrost_job_hw_submit() always > returned with an extra reference, but now we have a case which doesn't. > AFAICT this is probably fine because we dereference on the path where > the hardware has completed the job (which obviously won't happen here). > But I'm still a bit uneasy whether the reference counts are always correct. I think it is. We only decrement the PM count in the interrupt handler path, and as you pointed out, we won't reach that path here. But if that helps, I can move this if to `panfrost_job_run()`: /* Nothing to execute, signal the fence directly. */ if (job->requirements & PANFROST_JD_REQ_DEP_ONLY) dma_fence_signal_locked(job->done_fence); else panfrost_job_hw_submit(job, slot); > > Steve > > > panfrost_devfreq_record_busy(&pfdev->pfdevfreq); > > > > ret = pm_runtime_get_sync(pfdev->dev); > > diff --git a/include/uapi/drm/panfrost_drm.h > > b/include/uapi/drm/panfrost_drm.h > > index 5e3f8a344f41..b9df066970f6 100644 > > --- a/include/uapi/drm/panfrost_drm.h > > +++ b/include/uapi/drm/panfrost_drm.h > > @@ -46,6 +46,13 @@ extern "C" { > > #define DRM_IOCTL_PANFROST_PERFCNT_DUMP > > DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct > > drm_panfrost_perfcnt_dump) > > > > #define PANFROST_JD_REQ_FS (1 << 0) > > + > > +/* > > + * Dependency only job. The job chain head should be set to 0 when this > > flag > > + * is set. > > + */ > > +#define PANFROST_JD_REQ_DEP_ONLY (1 << 1) > > + > > /** > > * struct drm_panfrost_submit -
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
Am 04.10.21 um 12:50 schrieb Tvrtko Ursulin: On 04/10/2021 11:44, Christian König wrote: Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin: On 04/10/2021 10:53, Christian König wrote: Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. That won't work. It's perfectly valid for dma_fence_get_rcu() to return NULL when we are racing here. Keep in mind that we don't hold any locks. Ah yes.. No need to change anything then, sorry for the confusion. I did not find any holes, the rest was just about how to maybe make the flow more obvious. Let me know if you want r-b now or later. Now would be good. I've tried to make that more cleaner, but this only lead to repeating the code more often. Reviewed-by: Tvrtko Ursulin Thanks, but what about the rest? The selftests in this version still have some bugs which I already fixed, but I think we could push most of the set. Christian. Regards, Tvrtko Regards, Christian. Regards, Tvrtko What we could do is to return NULL and repeat with a new sequence immediately though. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->fence; int index = cursor->index; dma_fence_put(fence); fence = NULL; next: if (index == -1) { /* Try picking the exclusive fence. */ index++; fence = dma_resv_excl_fence(obj); if (!fence) goto next; } else if (cursor->fences && index < cursor->fences->shared_count) { /* Try picking next shared fence. */ struct dma_resv_list *fences = cursor->fences; fen
Re: [PATCH v5 6/8] drm/panfrost: Support synchronization jobs
On 04/10/2021 13:24, Boris Brezillon wrote: > On Mon, 4 Oct 2021 12:30:42 +0100 > Steven Price wrote: [...] >> >> It took me a while to convince myself that the reference counting for >> the PM reference is correct. Before panfrost_job_hw_submit() always >> returned with an extra reference, but now we have a case which doesn't. >> AFAICT this is probably fine because we dereference on the path where >> the hardware has completed the job (which obviously won't happen here). >> But I'm still a bit uneasy whether the reference counts are always correct. > > I think it is. We only decrement the PM count in the interrupt handler > path, and as you pointed out, we won't reach that path here. But if > that helps, I can move this if to `panfrost_job_run()`: > > /* Nothing to execute, signal the fence directly. */ > if (job->requirements & PANFROST_JD_REQ_DEP_ONLY) > dma_fence_signal_locked(job->done_fence); > else > panfrost_job_hw_submit(job, slot); > I think that would make it a bit more readable - really panfrost_job_hw_submit() needs a bit of TLC, I did post a patch ages ago[1] but it didn't get any feedback and then I forgot about it. Things have moved on so it would need a little bit of rework. Thanks, Steve [1] https://lore.kernel.org/dri-devel/20210512152419.30003-1-steven.pr...@arm.com/
Re: [PATCH 03/28] dma-buf: add dma_resv selftest
On 01/10/2021 11:05, Christian König wrote: Just exercising a very minor subset of the functionality, but already proven useful. Signed-off-by: Christian König --- drivers/dma-buf/Makefile | 3 +- drivers/dma-buf/selftests.h | 1 + drivers/dma-buf/st-dma-resv.c | 164 ++ 3 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 drivers/dma-buf/st-dma-resv.c diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 1ef021273a06..511805dbeb75 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -11,6 +11,7 @@ obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o dmabuf_selftests-y := \ selftest.o \ st-dma-fence.o \ - st-dma-fence-chain.o + st-dma-fence-chain.o \ + st-dma-resv.o obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h index bc8cea67bf1e..97d73aaa31da 100644 --- a/drivers/dma-buf/selftests.h +++ b/drivers/dma-buf/selftests.h @@ -12,3 +12,4 @@ selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */ selftest(dma_fence, dma_fence) selftest(dma_fence_chain, dma_fence_chain) +selftest(dma_resv, dma_resv) diff --git a/drivers/dma-buf/st-dma-resv.c b/drivers/dma-buf/st-dma-resv.c new file mode 100644 index ..ea44769d058d --- /dev/null +++ b/drivers/dma-buf/st-dma-resv.c @@ -0,0 +1,164 @@ +/* SPDX-License-Identifier: MIT */ + +/* +* Copyright © 2019 Intel Corporation +*/ May want to update the year. + +//#include +//#include +//#include +//#include +//#include And remove these? + +#include +#include +#include + +#include "selftest.h" + +static struct spinlock fence_lock; + +static const char *fence_name(struct dma_fence *f) +{ + return "selftest"; +} + +static const struct dma_fence_ops fence_ops = { + .get_driver_name = fence_name, + .get_timeline_name = fence_name, +}; + +static struct dma_fence *alloc_fence(void) +{ + struct dma_fence *f; + + f = kmalloc(sizeof(*f), GFP_KERNEL); + if (!f) + return NULL; + + dma_fence_init(f, &fence_ops, &fence_lock, 0, 0); + return f; +} + +static int sanitycheck(void *arg) +{ + struct dma_fence *f; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_fence_signal(f); + dma_fence_put(f); + return 0; +} + +static int test_excl_signaling(void *arg) +{ + struct dma_resv resv; + struct dma_fence *f; + int err = -EINVAL; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + dma_resv_add_excl_fence(&resv, f); + if (dma_resv_test_signaled(&resv, false)) { + pr_err("Resv unexpectedly signaled\n"); + goto err_free; + } + dma_fence_signal(f); + if (!dma_resv_test_signaled(&resv, false)) { + pr_err("Resv not reporting signaled\n"); + goto err_free; + } + err = 0; +err_free: + dma_resv_fini(&resv); + dma_fence_put(f); + return err; +} + +static int test_shared_signaling(void *arg) +{ + struct dma_resv resv; + struct dma_fence *f; + int err; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + err = dma_resv_reserve_shared(&resv, 1); + if (err) { + pr_err("Resv shared slot allocation failed\n"); + goto err_free; + } + + err = -EINVAL; + dma_resv_add_shared_fence(&resv, f); + if (dma_resv_test_signaled(&resv, true)) { + pr_err("Resv unexpectedly signaled\n"); + goto err_free; + } + dma_fence_signal(f); + if (!dma_resv_test_signaled(&resv, true)) { + pr_err("Resv not reporting signaled\n"); + goto err_free; + } + err = 0; +err_free: + dma_resv_fini(&resv); + dma_fence_put(f); + return err; +} Task for a rainy day - consolidate the above two into parameter driven dma_resv setup (more fences, mixed signaling status, mixed exclusive and shared, different signaling mode) and common verification stages. + +static int test_excl_for_each(void *arg) +{ + struct dma_resv_iter cursor; + struct dma_fence *f, *fence; + struct dma_resv resv; + int err; + + f = alloc_fence(); + if (!f) + return -ENOMEM; + + dma_resv_init(&resv); + dma_resv_add_excl_fence(&resv, f); + + err = -EINVAL; + dma_resv_for_each_fence(&cursor, &resv, false, fence) { What about the dma_resv_assert_held(cursor->obj) assert? I must be missing something.. + if (f != fence) { + pr_err("Unexpected fence\n"); Best set err to something, unit tests should be super robust, like if unexpected fence follows
Re: [PATCH 01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7
On 04/10/2021 13:59, Christian König wrote: Am 04.10.21 um 12:50 schrieb Tvrtko Ursulin: On 04/10/2021 11:44, Christian König wrote: Am 04.10.21 um 12:34 schrieb Tvrtko Ursulin: On 04/10/2021 10:53, Christian König wrote: Am 04.10.21 um 11:29 schrieb Tvrtko Ursulin: On 01/10/2021 11:05, Christian König wrote: Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. v5: fix typos in the documentation v6: fix coding error when excl fence is NULL v7: one more logic fix Signed-off-by: Christian König --- drivers/dma-buf/dma-resv.c | 100 + include/linux/dma-resv.h | 95 +++ 2 files changed, 195 insertions(+) diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..3cbcf66a137e 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,106 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iteration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index == -1) { + cursor->fence = dma_resv_excl_fence(obj); + cursor->index++; + if (!cursor->fence) + continue; + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + break; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index++; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + cursor->fence = dma_fence_get_rcu(cursor->fence); Worth having an assert dma_fence_get_rcu does not fail here? Not sure that I have seen debug build only asserts though on the DRM core side. That won't work. It's perfectly valid for dma_fence_get_rcu() to return NULL when we are racing here. Keep in mind that we don't hold any locks. Ah yes.. No need to change anything then, sorry for the confusion. I did not find any holes, the rest was just about how to maybe make the flow more obvious. Let me know if you want r-b now or later. Now would be good. I've tried to make that more cleaner, but this only lead to repeating the code more often. Reviewed-by: Tvrtko Ursulin Thanks, but what about the rest? I'll go through the core patches, it just taking time. i915 patches, again, I'd prefer you drop the busy ioctl but at least you have i915_request_await_object as a pilot. The rest of i915 I'd prefer someone who knows the display paths can answer whether locked or unlocked iterator is the right one. The selftests in this version still have some bugs which I already fixed, but I think we could push most of the set. Ah.. I just replied on that one. Regards, Tvrtko Christian. Regards, Tvrtko Regards, Christian. Regards, Tvrtko What we could do is to return NULL and repeat with a new sequence immediately though. On the bike shedding front, would it be clearer if the continue condition on signaled fences was standalone, using the continue statement? I'd also possibly re-arrange the three if-else blocks so that the end of iteration is not sandwiched between blocks handling exclusive and shared, and flow tweaked a bit, like: struct dma_fence *fence = cursor->
Re: 572994bf18ff prevents system boot
> On Oct 4, 2021, at 3:07 AM, Thomas Zimmermann wrote: > > (cc: ainux.w...@gmail.com) > > Hi > > Am 03.10.21 um 20:09 schrieb Chuck Lever III: >> Hi- >> After updating one of my test systems to v5.15-rc, I found that it >> becomes unresponsive during the later part of the boot process. A >> power-on reset is necessary to recover. >> I bisected to this commit: >> 572994bf18ff ("drm/ast: Zero is missing in detect function") > > You don't have a monitor connected, I guess? Correct, my lab systems use IPMI and a browser-attached console. > In that case, we now trigger the helpers that poll for connected monitors. > However, the overhead seems rather extreme. > > I'll have to try to reproduce this, or otherwise we can revert the commit. It's strange, only that system in my lab seems to have a problem. The others work fine. Thanks for having a look! > Best regards > Thomas > >> Checking out v5.15-rc3 and reverting this commit enables the system >> to boot again. >> 0b:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics >> Family (rev 30) (prog-if 00 [VGA controller]) >> DeviceName: ASPEED Video AST2400 >> Subsystem: Super Micro Computer Inc X10SRL-F >> Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- >> Stepping- SERR- FastB2B- DisINTx- >> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- >> SERR- > Interrupt: pin A routed to IRQ 18 >> Region 0: Memory at fa00 (32-bit, non-prefetchable) [size=16M] >> Region 1: Memory at fb00 (32-bit, non-prefetchable) [size=128K] >> Region 2: I/O ports at c000 [size=128] >> Expansion ROM at 000c [virtual] [disabled] [size=128K] >> Capabilities: [40] Power Management version 3 >> Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA >> PME(D0+,D1+,D2+,D3hot+,D3cold+) >> Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- >> Capabilities: [50] MSI: Enable- Count=1/4 Maskable- 64bit+ >> Address: Data: >> Kernel driver in use: ast >> Kernel modules: ast >> -- >> Chuck Lever > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer -- Chuck Lever
[PATCH] drm/msm/disp: fix endian bug in debugfs code
The "vbif->features" is type unsigned long but the debugfs file is treating it as a u32 type. This will work in little endian systems, but the correct thing is to change the debugfs to use an unsigned long. Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") Signed-off-by: Dan Carpenter --- You might wonder why this code has so many casts. It's required because this data is const. Which is fine because the file is read only. drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c index 21d20373eb8b..e645a886e3c6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c @@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms *dpu_kms, struct dentry *debugfs_root) debugfs_vbif = debugfs_create_dir(vbif_name, entry); - debugfs_create_u32("features", 0600, debugfs_vbif, - (u32 *)&vbif->features); + debugfs_create_ulong("features", 0600, debugfs_vbif, +(unsigned long *)&vbif->features); debugfs_create_u32("xin_halt_timeout", 0400, debugfs_vbif, (u32 *)&vbif->xin_halt_timeout); -- 2.20.1
[PATCH] drm/msm: Fix potential Oops in a6xx_gmu_rpmh_init()
There are two problems here: 1) The "seqptr" is used uninitalized when we free it at the end. 2) The a6xx_gmu_get_mmio() function returns error pointers. It never returns true. Fixes: 64245fc55172 ("drm/msm/a6xx: use AOP-initialized PDC for a650") Fixes: f8fc924e088e ("drm/msm/a6xx: Fix PDC register overlap") Signed-off-by: Dan Carpenter --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index a7c58018959f..3bd6e579ea89 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -512,11 +512,11 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu) struct adreno_gpu *adreno_gpu = &a6xx_gpu->base; struct platform_device *pdev = to_platform_device(gmu->dev); void __iomem *pdcptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc"); - void __iomem *seqptr; + void __iomem *seqptr = NULL; uint32_t pdc_address_offset; bool pdc_in_aop = false; - if (!pdcptr) + if (IS_ERR(pdcptr)) goto err; if (adreno_is_a650(adreno_gpu) || adreno_is_a660_family(adreno_gpu)) @@ -528,7 +528,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu) if (!pdc_in_aop) { seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq"); - if (!seqptr) + if (IS_ERR(seqptr)) goto err; } -- 2.20.1
Re: 572994bf18ff prevents system boot
Hi Am 04.10.21 um 15:34 schrieb Chuck Lever III: On Oct 4, 2021, at 3:07 AM, Thomas Zimmermann wrote: (cc: ainux.w...@gmail.com) Hi Am 03.10.21 um 20:09 schrieb Chuck Lever III: Hi- After updating one of my test systems to v5.15-rc, I found that it becomes unresponsive during the later part of the boot process. A power-on reset is necessary to recover. I bisected to this commit: 572994bf18ff ("drm/ast: Zero is missing in detect function") You don't have a monitor connected, I guess? Correct, my lab systems use IPMI and a browser-attached console. In that case, we now trigger the helpers that poll for connected monitors. However, the overhead seems rather extreme. I'll have to try to reproduce this, or otherwise we can revert the commit. It's strange, only that system in my lab seems to have a problem. The others work fine. Thanks for having a look! Is it a HW or FW problem? Maybe a different revision? I'm asking because the problematic commit does the correct thing. If there is no VGA cable connected, the driver should poll until it detects one. The overhead should be minimal. But I'll try to reproduce anyway. Best regards Thomas Best regards Thomas Checking out v5.15-rc3 and reverting this commit enables the system to boot again. 0b:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 30) (prog-if 00 [VGA controller]) DeviceName: ASPEED Video AST2400 Subsystem: Super Micro Computer Inc X10SRL-F Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- SERR- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer -- Chuck Lever -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer OpenPGP_signature Description: OpenPGP digital signature
Re: 572994bf18ff prevents system boot
> On Oct 4, 2021, at 10:07 AM, Thomas Zimmermann wrote: > > Hi > > Am 04.10.21 um 15:34 schrieb Chuck Lever III: >>> On Oct 4, 2021, at 3:07 AM, Thomas Zimmermann wrote: >>> >>> (cc: ainux.w...@gmail.com) >>> >>> Hi >>> >>> Am 03.10.21 um 20:09 schrieb Chuck Lever III: Hi- After updating one of my test systems to v5.15-rc, I found that it becomes unresponsive during the later part of the boot process. A power-on reset is necessary to recover. I bisected to this commit: 572994bf18ff ("drm/ast: Zero is missing in detect function") >>> >>> You don't have a monitor connected, I guess? >> Correct, my lab systems use IPMI and a browser-attached console. >>> In that case, we now trigger the helpers that poll for connected monitors. >>> However, the overhead seems rather extreme. >>> >>> I'll have to try to reproduce this, or otherwise we can revert the commit. >> It's strange, only that system in my lab seems to have a problem. >> The others work fine. >> Thanks for having a look! > > Is it a HW or FW problem? Maybe a different revision? It's possible. I don't know how to further diagnose the issue, though. Any guidance appreciated! > I'm asking because the problematic commit does the correct thing. If there is > no VGA cable connected, the driver should poll until it detects one. The > overhead should be minimal. > > But I'll try to reproduce anyway. > > Best regards > Thomas > >>> Best regards >>> Thomas >>> Checking out v5.15-rc3 and reverting this commit enables the system to boot again. 0b:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 30) (prog-if 00 [VGA controller]) DeviceName: ASPEED Video AST2400 Subsystem: Super Micro Computer Inc X10SRL-F Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- SERR- >>> Interrupt: pin A routed to IRQ 18 Region 0: Memory at fa00 (32-bit, non-prefetchable) [size=16M] Region 1: Memory at fb00 (32-bit, non-prefetchable) [size=128K] Region 2: I/O ports at c000 [size=128] Expansion ROM at 000c [virtual] [disabled] [size=128K] Capabilities: [40] Power Management version 3 Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA PME(D0+,D1+,D2+,D3hot+,D3cold+) Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME- Capabilities: [50] MSI: Enable- Count=1/4 Maskable- 64bit+ Address: Data: Kernel driver in use: ast Kernel modules: ast -- Chuck Lever >>> >>> -- >>> Thomas Zimmermann >>> Graphics Driver Developer >>> SUSE Software Solutions Germany GmbH >>> Maxfeldstr. 5, 90409 Nürnberg, Germany >>> (HRB 36809, AG Nürnberg) >>> Geschäftsführer: Felix Imendörffer >> -- >> Chuck Lever > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer -- Chuck Lever
Re: [PATCH 2/2] amd/amdgpu_dm: Verify Gamma and Degamma LUT sizes using DRM Core check
On 2021-10-01 15:56, Sean Paul wrote: > On Wed, Sep 29, 2021 at 03:39:26PM -0400, Mark Yacoub wrote: >> From: Mark Yacoub >> >> [Why] >> drm_atomic_helper_check_crtc now verifies both legacy and non-legacy LUT >> sizes. There is no need to check it within amdgpu_dm_atomic_check. >> >> [How] >> Remove the local call to verify LUT sizes and use DRM Core function >> instead. >> >> Tested on ChromeOS Zork. >> >> Signed-off-by: Mark Yacoub >> --- >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> index 07adac1a8c42b..96a1d006b777e 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -10683,6 +10683,10 @@ static int amdgpu_dm_atomic_check(struct drm_device >> *dev, >> } >> } >> #endif >> +ret = drm_atomic_helper_check_crtc(state); >> +if (ret) >> +return ret; >> + >> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, >> new_crtc_state, i) { >> dm_old_crtc_state = to_dm_crtc_state(old_crtc_state); >> >> @@ -10692,10 +10696,6 @@ static int amdgpu_dm_atomic_check(struct drm_device >> *dev, >> dm_old_crtc_state->dsc_force_changed == false) >> continue; >> >> -ret = amdgpu_dm_verify_lut_sizes(new_crtc_state); > > From a quick glance, I think you can now delete this function. It's called > from > amdgpu_dm_update_crtc_color_mgmt() which is part of the commit, so the lut > sizes > should have already been checked. > I agree. Would be nice if we could drop the extra call in the CM function (after confirming that it isn't needed). Harry > If the call from amdgpu_dm_update_crtc_color_mgmt() is not possible to remove, > you could replace it with a call to the new helper function. And if _that_ is > not possible, please make amdgpu_dm_verify_lut_sizes() static :-) > > Sean > >> -if (ret) >> -goto fail; >> - >> if (!new_crtc_state->enable) >> continue; >> >> -- >> 2.33.0.685.g46640cef36-goog >> >
[RFC 8/8] drm/i915: Connect with the process nice change notifier
From: Tvrtko Ursulin Connect i915 with the process nice change notifier so that our scheduling can react to runtime adjustments, on top of previously added nice value inheritance at context create time. To achieve this we use the previously added map of clients per owning tasks in combination with the list of GEM contexts per client. To avoid possibly unnecessary complications the updated context nice value will only apply to future submissions against the context. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_drm_client.c | 31 ++ drivers/gpu/drm/i915/i915_drm_client.h | 3 +++ 2 files changed, 34 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c index 82b9636482ef..e34c1228f65b 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.c +++ b/drivers/gpu/drm/i915/i915_drm_client.c @@ -7,10 +7,35 @@ #include #include +#include "gem/i915_gem_context.h" #include "i915_drm_client.h" #include "i915_gem.h" #include "i915_utils.h" +static int +clients_notify(struct notifier_block *nb, unsigned long val, void *ptr) +{ + struct i915_drm_clients *clients = + container_of(nb, typeof(*clients), prio_notifier); + struct i915_drm_client *client; + + rcu_read_lock(); + read_lock(&clients->lock); + hash_for_each_possible(clients->tasks, client, node, (uintptr_t)ptr) { + struct i915_gem_context *ctx; + + if (client->owner != ptr) + continue; + + list_for_each_entry_rcu(ctx, &client->ctx_list, client_link) + ctx->sched.nice = (int)val; + } + read_unlock(&clients->lock); + rcu_read_unlock(); + + return NOTIFY_DONE; +} + void i915_drm_clients_init(struct i915_drm_clients *clients, struct drm_i915_private *i915) { @@ -21,6 +46,10 @@ void i915_drm_clients_init(struct i915_drm_clients *clients, rwlock_init(&clients->lock); hash_init(clients->tasks); + + memset(&clients->prio_notifier, 0, sizeof(clients->prio_notifier)); + clients->prio_notifier.notifier_call = clients_notify; + register_user_nice_notifier(&clients->prio_notifier); } struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients) @@ -75,6 +104,8 @@ void __i915_drm_client_free(struct kref *kref) void i915_drm_clients_fini(struct i915_drm_clients *clients) { + unregister_user_nice_notifier(&clients->prio_notifier); + GEM_BUG_ON(!xa_empty(&clients->xarray)); xa_destroy(&clients->xarray); } diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h index 42fd79f0558a..dda26aa42ac9 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.h +++ b/drivers/gpu/drm/i915/i915_drm_client.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -24,6 +25,8 @@ struct i915_drm_clients { rwlock_t lock; DECLARE_HASHTABLE(tasks, 6); + + struct notifier_block prio_notifier; }; struct i915_drm_client { -- 2.30.2
[RFC 3/8] drm/i915: Make GEM contexts track DRM clients
From: Tvrtko Ursulin Make GEM contexts keep a reference to i915_drm_client for the whole of of their lifetime which will come handy in following patches. v2: Don't bother supporting selftests contexts from debugfs. (Chris) v3 (Lucas): Finish constructing ctx before adding it to the list v4 (Ram): Rebase. v5: Trivial rebase for proto ctx changes. v6: Rebase after clients no longer track name and pid. Signed-off-by: Tvrtko Ursulin Reviewed-by: Chris Wilson # v5 Reviewed-by: Aravind Iddamsetty # v5 Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 + drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 3 +++ 2 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 8208fd5b72c3..9296d69681d7 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -956,6 +956,9 @@ static void i915_gem_context_release_work(struct work_struct *work) if (vm) i915_vm_put(vm); + if (ctx->client) + i915_drm_client_put(ctx->client); + mutex_destroy(&ctx->engines_mutex); mutex_destroy(&ctx->lut_mutex); @@ -1373,6 +1376,8 @@ static void gem_context_register(struct i915_gem_context *ctx, ctx->file_priv = fpriv; ctx->pid = get_task_pid(current, PIDTYPE_PID); + ctx->client = i915_drm_client_get(fpriv->client); + snprintf(ctx->name, sizeof(ctx->name), "%s[%d]", current->comm, pid_nr(ctx->pid)); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index c4617e4d9fa9..598c57ac5cdf 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -277,6 +277,9 @@ struct i915_gem_context { /** @link: place with &drm_i915_private.context_list */ struct list_head link; + /** @client: struct i915_drm_client */ + struct i915_drm_client *client; + /** * @ref: reference count * -- 2.30.2
[RFC 2/8] drm/i915: Explicitly track DRM clients
From: Tvrtko Ursulin Tracking DRM clients more explicitly will allow later patches to accumulate past and current GPU usage in a centralised place and also consolidate access to owning task pid/name. Unique client id is also assigned for the purpose of distinguishing/ consolidating between multiple file descriptors owned by the same process. v2: Chris Wilson: * Enclose new members into dedicated structs. * Protect against failed sysfs registration. v3: * sysfs_attr_init. v4: * Fix for internal clients. v5: * Use cyclic ida for client id. (Chris) * Do not leak pid reference. (Chris) * Tidy code with some locals. v6: * Use xa_alloc_cyclic to simplify locking. (Chris) * No need to unregister individial sysfs files. (Chris) * Rebase on top of fpriv kref. * Track client closed status and reflect in sysfs. v7: * Make drm_client more standalone concept. v8: * Simplify sysfs show. (Chris) * Always track name and pid. v9: * Fix cyclic id assignment. v10: * No need for a mutex around xa_alloc_cyclic. * Refactor sysfs into own function. * Unregister sysfs before freeing pid and name. * Move clients setup into own function. v11: * Call clients init directly from driver init. (Chris) v12: * Do not fail client add on id wrap. (Maciej) v13 (Lucas): Rebase. v14: * Dropped sysfs bits. v15: * Dropped tracking of pid/ and name. * Dropped RCU freeing of the client object. Signed-off-by: Tvrtko Ursulin Reviewed-by: Chris Wilson # v11 Reviewed-by: Aravind Iddamsetty # v11 Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/Makefile | 5 +- drivers/gpu/drm/i915/i915_drm_client.c | 68 ++ drivers/gpu/drm/i915/i915_drm_client.h | 50 +++ drivers/gpu/drm/i915/i915_drv.c| 6 +++ drivers/gpu/drm/i915/i915_drv.h| 5 ++ drivers/gpu/drm/i915/i915_gem.c| 21 ++-- 6 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_drm_client.c create mode 100644 drivers/gpu/drm/i915/i915_drm_client.h diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 5c8e022a7383..005b5df425a1 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -32,8 +32,9 @@ subdir-ccflags-y += -I$(srctree)/$(src) # Please keep these build lists sorted! # core driver code -i915-y += i915_drv.o \ - i915_config.o \ +i915-y += i915_config.o \ + i915_drm_client.o \ + i915_drv.o \ i915_irq.o \ i915_getparam.o \ i915_mitigations.o \ diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c new file mode 100644 index ..e61e9ba15256 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_drm_client.c @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2020 Intel Corporation + */ + +#include +#include +#include + +#include "i915_drm_client.h" +#include "i915_gem.h" +#include "i915_utils.h" + +void i915_drm_clients_init(struct i915_drm_clients *clients, + struct drm_i915_private *i915) +{ + clients->i915 = i915; + clients->next_id = 0; + + xa_init_flags(&clients->xarray, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ); +} + +struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients) +{ + struct i915_drm_client *client; + struct xarray *xa = &clients->xarray; + int ret; + + client = kzalloc(sizeof(*client), GFP_KERNEL); + if (!client) + return ERR_PTR(-ENOMEM); + + xa_lock_irq(xa); + ret = __xa_alloc_cyclic(xa, &client->id, client, xa_limit_32b, + &clients->next_id, GFP_KERNEL); + xa_unlock_irq(xa); + if (ret < 0) + goto err; + + kref_init(&client->kref); + client->clients = clients; + + return client; + +err: + kfree(client); + + return ERR_PTR(ret); +} + +void __i915_drm_client_free(struct kref *kref) +{ + struct i915_drm_client *client = + container_of(kref, typeof(*client), kref); + struct xarray *xa = &client->clients->xarray; + unsigned long flags; + + xa_lock_irqsave(xa, flags); + __xa_erase(xa, client->id); + xa_unlock_irqrestore(xa, flags); + kfree(client); +} + +void i915_drm_clients_fini(struct i915_drm_clients *clients) +{ + GEM_BUG_ON(!xa_empty(&clients->xarray)); + xa_destroy(&clients->xarray); +} diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h new file mode 100644 index ..e8986ad51176 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_drm_client.h @@ -0,0 +1,50 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2020 Intel Corporation + */ + +#ifndef __I915_DRM_CLIENT_H__ +#define __I915_DRM_CLIENT_H__ + +#include +#include + +struct drm_i915_private; + +struct i915_drm_clients { + struct drm_i915_private *i915; +
[RFC 5/8] drm/i915: Keep track of registered clients indexed by task struct
From: Tvrtko Ursulin A simple hash table of registered clients indexed by the task struct pointer is kept to be used in a following patch. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 ++ drivers/gpu/drm/i915/i915_drm_client.c | 31 - drivers/gpu/drm/i915/i915_drm_client.h | 13 + 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index d1992ba59ed8..8d4d687ab1d0 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1932,6 +1932,8 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, return -EIO; } + i915_drm_client_update_owner(ext_data.fpriv->client, current); + ext_data.pc = proto_context_create(i915, args->flags); if (IS_ERR(ext_data.pc)) return PTR_ERR(ext_data.pc); diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c index 91a8559bebf7..82b9636482ef 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.c +++ b/drivers/gpu/drm/i915/i915_drm_client.c @@ -18,6 +18,9 @@ void i915_drm_clients_init(struct i915_drm_clients *clients, clients->next_id = 0; xa_init_flags(&clients->xarray, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ); + + rwlock_init(&clients->lock); + hash_init(clients->tasks); } struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients) @@ -42,6 +45,8 @@ struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients) INIT_LIST_HEAD(&client->ctx_list); client->clients = clients; + i915_drm_client_update_owner(client, current); + return client; err: @@ -54,9 +59,14 @@ void __i915_drm_client_free(struct kref *kref) { struct i915_drm_client *client = container_of(kref, typeof(*client), kref); - struct xarray *xa = &client->clients->xarray; + struct i915_drm_clients *clients = client->clients; + struct xarray *xa = &clients->xarray; unsigned long flags; + write_lock(&clients->lock); + hash_del(&client->node); + write_unlock(&clients->lock); + xa_lock_irqsave(xa, flags); __xa_erase(xa, client->id); xa_unlock_irqrestore(xa, flags); @@ -68,3 +78,22 @@ void i915_drm_clients_fini(struct i915_drm_clients *clients) GEM_BUG_ON(!xa_empty(&clients->xarray)); xa_destroy(&clients->xarray); } + +void i915_drm_client_update_owner(struct i915_drm_client *client, + struct task_struct *owner) +{ + struct i915_drm_clients *clients; + + if (READ_ONCE(client->owner) == owner) + return; + + clients = client->clients; + write_lock(&clients->lock); + if (READ_ONCE(client->owner) != owner) { + if (client->owner) + hash_del(&client->node); + client->owner = owner; + hash_add(clients->tasks, &client->node, (uintptr_t)owner); + } + write_unlock(&clients->lock); +} diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h index 0207dfad4568..42fd79f0558a 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.h +++ b/drivers/gpu/drm/i915/i915_drm_client.h @@ -6,8 +6,11 @@ #ifndef __I915_DRM_CLIENT_H__ #define __I915_DRM_CLIENT_H__ +#include #include #include +#include +#include #include #include @@ -18,6 +21,9 @@ struct i915_drm_clients { struct xarray xarray; u32 next_id; + + rwlock_t lock; + DECLARE_HASHTABLE(tasks, 6); }; struct i915_drm_client { @@ -28,6 +34,9 @@ struct i915_drm_client { spinlock_t ctx_lock; /* For add/remove from ctx_list. */ struct list_head ctx_list; /* List of contexts belonging to client. */ + struct task_struct *owner; /* No reference kept, never dereferenced. */ + struct hlist_node node; + struct i915_drm_clients *clients; }; @@ -52,4 +61,8 @@ struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients); void i915_drm_clients_fini(struct i915_drm_clients *clients); +void i915_drm_client_update_owner(struct i915_drm_client *client, + struct task_struct *owner); + + #endif /* !__I915_DRM_CLIENT_H__ */ -- 2.30.2
[RFC 6/8] drm/i915: Make some recently added vfuncs use full scheduling attribute
From: Tvrtko Ursulin Code added in 71ed60112d5d ("drm/i915: Add kick_backend function to i915_sched_engine") and ee242ca704d3 ("drm/i915/guc: Implement GuC priority management") introduced some scheduling related vfuncs which take integer request priority as argument. Make them instead take struct i915_sched_attr, which is the type encapsulating this information, so it probably aligns with the design better. It definitely enables extending the set of scheduling attributes. Signed-off-by: Tvrtko Ursulin Cc: Matthew Brost Cc: Daniele Ceraolo Spurio --- drivers/gpu/drm/i915/gt/intel_execlists_submission.c | 4 +++- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c| 3 ++- drivers/gpu/drm/i915/i915_scheduler.c| 4 ++-- drivers/gpu/drm/i915/i915_scheduler_types.h | 4 ++-- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index 7147fe80919e..e91d803a6453 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -3216,11 +3216,13 @@ static bool can_preempt(struct intel_engine_cs *engine) return engine->class != RENDER_CLASS; } -static void kick_execlists(const struct i915_request *rq, int prio) +static void kick_execlists(const struct i915_request *rq, + const struct i915_sched_attr *attr) { struct intel_engine_cs *engine = rq->engine; struct i915_sched_engine *sched_engine = engine->sched_engine; const struct i915_request *inflight; + const int prio = attr->priority; /* * We only need to kick the tasklet once for the high priority diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index ba0de35f6323..b5883a4365ca 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2414,9 +2414,10 @@ static void guc_init_breadcrumbs(struct intel_engine_cs *engine) } static void guc_bump_inflight_request_prio(struct i915_request *rq, - int prio) + const struct i915_sched_attr *attr) { struct intel_context *ce = rq->context; + const int prio = attr->priority; u8 new_guc_prio = map_i915_prio_to_guc_prio(prio); /* Short circuit function */ diff --git a/drivers/gpu/drm/i915/i915_scheduler.c b/drivers/gpu/drm/i915/i915_scheduler.c index 762127dd56c5..534bab99fcdc 100644 --- a/drivers/gpu/drm/i915/i915_scheduler.c +++ b/drivers/gpu/drm/i915/i915_scheduler.c @@ -255,7 +255,7 @@ static void __i915_schedule(struct i915_sched_node *node, /* Must be called before changing the nodes priority */ if (sched_engine->bump_inflight_request_prio) - sched_engine->bump_inflight_request_prio(from, prio); + sched_engine->bump_inflight_request_prio(from, attr); WRITE_ONCE(node->attr.priority, prio); @@ -280,7 +280,7 @@ static void __i915_schedule(struct i915_sched_node *node, /* Defer (tasklet) submission until after all of our updates. */ if (sched_engine->kick_backend) - sched_engine->kick_backend(node_to_request(node), prio); + sched_engine->kick_backend(node_to_request(node), attr); } spin_unlock(&sched_engine->lock); diff --git a/drivers/gpu/drm/i915/i915_scheduler_types.h b/drivers/gpu/drm/i915/i915_scheduler_types.h index b0a1b58c7893..24b9ac1c2ce2 100644 --- a/drivers/gpu/drm/i915/i915_scheduler_types.h +++ b/drivers/gpu/drm/i915/i915_scheduler_types.h @@ -177,13 +177,13 @@ struct i915_sched_engine { * @kick_backend: kick backend after a request's priority has changed */ void(*kick_backend)(const struct i915_request *rq, - int prio); + const struct i915_sched_attr *attr); /** * @bump_inflight_request_prio: update priority of an inflight request */ void(*bump_inflight_request_prio)(struct i915_request *rq, - int prio); + const struct i915_sched_attr *attr); /** * @retire_inflight_request_prio: indicate request is retired to -- 2.30.2
[RFC 4/8] drm/i915: Track all user contexts per client
From: Tvrtko Ursulin We soon want to start answering questions like how much GPU time is the context belonging to a client which exited still using. To enable this we start tracking all context belonging to a client on a separate list. Signed-off-by: Tvrtko Ursulin Reviewed-by: Aravind Iddamsetty Reviewed-by: Chris Wilson Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 12 drivers/gpu/drm/i915/gem/i915_gem_context_types.h | 3 +++ drivers/gpu/drm/i915/i915_drm_client.c| 2 ++ drivers/gpu/drm/i915/i915_drm_client.h| 5 + 4 files changed, 22 insertions(+) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 9296d69681d7..d1992ba59ed8 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1169,6 +1169,7 @@ static void set_closed_name(struct i915_gem_context *ctx) static void context_close(struct i915_gem_context *ctx) { + struct i915_drm_client *client; struct i915_address_space *vm; /* Flush any concurrent set_engines() */ @@ -1205,6 +1206,13 @@ static void context_close(struct i915_gem_context *ctx) list_del(&ctx->link); spin_unlock(&ctx->i915->gem.contexts.lock); + client = ctx->client; + if (client) { + spin_lock(&client->ctx_lock); + list_del_rcu(&ctx->client_link); + spin_unlock(&client->ctx_lock); + } + mutex_unlock(&ctx->mutex); /* @@ -1385,6 +1393,10 @@ static void gem_context_register(struct i915_gem_context *ctx, old = xa_store(&fpriv->context_xa, id, ctx, GFP_KERNEL); WARN_ON(old); + spin_lock(&ctx->client->ctx_lock); + list_add_tail_rcu(&ctx->client_link, &ctx->client->ctx_list); + spin_unlock(&ctx->client->ctx_lock); + spin_lock(&i915->gem.contexts.lock); list_add_tail(&ctx->link, &i915->gem.contexts.list); spin_unlock(&i915->gem.contexts.lock); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h index 598c57ac5cdf..b878e1b13b38 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h @@ -280,6 +280,9 @@ struct i915_gem_context { /** @client: struct i915_drm_client */ struct i915_drm_client *client; + /** link: &drm_client.context_list */ + struct list_head client_link; + /** * @ref: reference count * diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c index e61e9ba15256..91a8559bebf7 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.c +++ b/drivers/gpu/drm/i915/i915_drm_client.c @@ -38,6 +38,8 @@ struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients) goto err; kref_init(&client->kref); + spin_lock_init(&client->ctx_lock); + INIT_LIST_HEAD(&client->ctx_list); client->clients = clients; return client; diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h index e8986ad51176..0207dfad4568 100644 --- a/drivers/gpu/drm/i915/i915_drm_client.h +++ b/drivers/gpu/drm/i915/i915_drm_client.h @@ -7,6 +7,8 @@ #define __I915_DRM_CLIENT_H__ #include +#include +#include #include struct drm_i915_private; @@ -23,6 +25,9 @@ struct i915_drm_client { unsigned int id; + spinlock_t ctx_lock; /* For add/remove from ctx_list. */ + struct list_head ctx_list; /* List of contexts belonging to client. */ + struct i915_drm_clients *clients; }; -- 2.30.2
Re: [PATCH v13 00/35] NVIDIA Tegra power management patches for 5.16
04.10.2021 12:11, Viresh Kumar пишет: > On 27-09-21, 01:40, Dmitry Osipenko wrote: >> This series adds runtime PM support to Tegra drivers and enables core >> voltage scaling for Tegra20/30 SoCs, resolving overheating troubles. >> >> All patches in this series are interdependent and should go via Tegra tree. > > So you don't need any OPP changes anymore ? I just came back from > vacation, don't know what you guys discussed in between :) > We discussed it and decided that we don't need more OPP/domain core changes. It's already good enough for the starter and making it all absolutely ideal doesn't worth the effort for now.
Re: [PATCH] drm/msm/dsi: use bulk clk API
On Fri 01 Oct 18:27 PDT 2021, Dmitry Baryshkov wrote: > Use clk_bulk_* API instead of hand-coding them. Note, this drops support > for legacy clk naming (e.g. "iface_clk" instead of just "iface"), > however all in-kernel device trees were converted long long ago. The > warning is present there since 2017. > Nice! Reviewed-by: Bjorn Andersson Regards, Bjorn > Signed-off-by: Dmitry Baryshkov > --- > drivers/gpu/drm/msm/dsi/dsi_host.c | 59 ++ > 1 file changed, 12 insertions(+), 47 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c > b/drivers/gpu/drm/msm/dsi/dsi_host.c > index e269df285136..3b81f40bba2e 100644 > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > @@ -106,7 +106,8 @@ struct msm_dsi_host { > phys_addr_t ctrl_size; > struct regulator_bulk_data supplies[DSI_DEV_REGULATOR_MAX]; > > - struct clk *bus_clks[DSI_BUS_CLK_MAX]; > + int num_bus_clks; > + struct clk_bulk_data bus_clks[DSI_BUS_CLK_MAX]; > > struct clk *byte_clk; > struct clk *esc_clk; > @@ -374,15 +375,14 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host) > int i, ret = 0; > > /* get bus clocks */ > - for (i = 0; i < cfg->num_bus_clks; i++) { > - msm_host->bus_clks[i] = msm_clk_get(pdev, > - cfg->bus_clk_names[i]); > - if (IS_ERR(msm_host->bus_clks[i])) { > - ret = PTR_ERR(msm_host->bus_clks[i]); > - pr_err("%s: Unable to get %s clock, ret = %d\n", > - __func__, cfg->bus_clk_names[i], ret); > - goto exit; > - } > + for (i = 0; i < cfg->num_bus_clks; i++) > + msm_host->bus_clks[i].id = cfg->bus_clk_names[i]; > + msm_host->num_bus_clks = cfg->num_bus_clks; > + > + ret = devm_clk_bulk_get(&pdev->dev, msm_host->num_bus_clks, > msm_host->bus_clks); > + if (ret < 0) { > + dev_err(&pdev->dev, "Unable to get clocks, ret = %d\n", ret); > + goto exit; > } > > /* get link and source clocks */ > @@ -433,41 +433,6 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host) > return ret; > } > > -static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host) > -{ > - const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg; > - int i, ret; > - > - DBG("id=%d", msm_host->id); > - > - for (i = 0; i < cfg->num_bus_clks; i++) { > - ret = clk_prepare_enable(msm_host->bus_clks[i]); > - if (ret) { > - pr_err("%s: failed to enable bus clock %d ret %d\n", > - __func__, i, ret); > - goto err; > - } > - } > - > - return 0; > -err: > - for (; i > 0; i--) > - clk_disable_unprepare(msm_host->bus_clks[i]); > - > - return ret; > -} > - > -static void dsi_bus_clk_disable(struct msm_dsi_host *msm_host) > -{ > - const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg; > - int i; > - > - DBG(""); > - > - for (i = cfg->num_bus_clks - 1; i >= 0; i--) > - clk_disable_unprepare(msm_host->bus_clks[i]); > -} > - > int msm_dsi_runtime_suspend(struct device *dev) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -478,7 +443,7 @@ int msm_dsi_runtime_suspend(struct device *dev) > if (!msm_host->cfg_hnd) > return 0; > > - dsi_bus_clk_disable(msm_host); > + clk_bulk_disable_unprepare(msm_host->num_bus_clks, msm_host->bus_clks); > > return 0; > } > @@ -493,7 +458,7 @@ int msm_dsi_runtime_resume(struct device *dev) > if (!msm_host->cfg_hnd) > return 0; > > - return dsi_bus_clk_enable(msm_host); > + return clk_bulk_prepare_enable(msm_host->num_bus_clks, > msm_host->bus_clks); > } > > int dsi_link_clk_set_rate_6g(struct msm_dsi_host *msm_host) > -- > 2.33.0 >
[RFC 1/8] sched: Add nice value change notifier
From: Tvrtko Ursulin Implement a simple notifier chain via which interested parties can track when process nice value changes. Simple because it is global so each user would have to track which tasks it is interested in. First intended use case are GPU drivers using task nice as priority hint when scheduling GPU contexts belonging to respective clients. To use register_user_nice_notifier and unregister_user_nice_notifier functions are provided and new nice value and pointer to task_struct being modified passed to the callbacks. v2: * Move the notifier chain outside task_rq_lock. (Peter) Opens: * Security. Would some sort of a per process mechanism be better and feasible? x Peter Zijlstra thinks it may be passable now that it is outside core scheduler locks. * Put it all behind kconfig to be selected by interested drivers? Signed-off-by: Tvrtko Ursulin Cc: Ingo Molnar Cc: Peter Zijlstra Cc: Juri Lelli Cc: Vincent Guittot --- include/linux/sched.h | 5 + kernel/sched/core.c | 37 - 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/include/linux/sched.h b/include/linux/sched.h index c1a927ddec64..1fcec88e5dbc 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -2309,4 +2309,9 @@ static inline void sched_core_free(struct task_struct *tsk) { } static inline void sched_core_fork(struct task_struct *p) { } #endif +struct notifier_block; + +extern int register_user_nice_notifier(struct notifier_block *); +extern int unregister_user_nice_notifier(struct notifier_block *); + #endif diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 1bba4128a3e6..fc90b603bb6f 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -6864,10 +6864,42 @@ static inline int rt_effective_prio(struct task_struct *p, int prio) } #endif +ATOMIC_NOTIFIER_HEAD(user_nice_notifier_list); + +/** + * register_user_nice_notifier - Register function to be called when task nice changes + * @nb: Info about notifier function to be called + * + * Registers a function with the list of functions to be called when task nice + * value changes. + * + * Currently always returns zero, as atomic_notifier_chain_register() + * always returns zero. + */ +int register_user_nice_notifier(struct notifier_block *nb) +{ + return atomic_notifier_chain_register(&user_nice_notifier_list, nb); +} +EXPORT_SYMBOL(register_user_nice_notifier); + +/** + * unregister_user_nice_notifier - Unregister previously registered user nice notifier + * @nb: Hook to be unregistered + * + * Unregisters a previously registered user nice notifier function. + * + * Returns zero on success, or %-ENOENT on failure. + */ +int unregister_user_nice_notifier(struct notifier_block *nb) +{ + return atomic_notifier_chain_unregister(&user_nice_notifier_list, nb); +} +EXPORT_SYMBOL(unregister_user_nice_notifier); + void set_user_nice(struct task_struct *p, long nice) { bool queued, running; - int old_prio; + int old_prio, ret; struct rq_flags rf; struct rq *rq; @@ -6915,6 +6947,9 @@ void set_user_nice(struct task_struct *p, long nice) out_unlock: task_rq_unlock(rq, p, &rf); + + ret = atomic_notifier_call_chain(&user_nice_notifier_list, nice, p); + WARN_ON_ONCE(ret != NOTIFY_DONE); } EXPORT_SYMBOL(set_user_nice); -- 2.30.2
[RFC v2 0/8] CPU + GPU synchronised priority scheduling
From: Tvrtko Ursulin This is a somewhat early sketch of one of my ideas intended for early feedback from the core scheduler experts. First and last two patches in the series are the most interesting ones for people outside of i915. (Note I did not copy everyone on all patches but just the cover letter for context and the rest should be available from the mailing list.) General idea is that current processing landscape seems to be more and more composed of pipelines where computations are done on multiple hardware devices. Furthermore some of the non-CPU devices, like in this case many GPUs supported by the i915 driver, actually support priority based scheduling which is currently rather inaccesible to the user (in terms of being able to control it from the outside). >From these two statements a question arises on how to allow for a simple, effective and consolidated user experience. In other words why user would not be able to do something like: $ nice ffmmpeg ...transcode my videos... $ my-favourite-game And have the nice hint apply to GPU parts of the transcode pipeline as well? Another reason why I started thinking about this is that I noticed Chrome browser for instance uses nice to de-prioritise background tabs. So again, having that decision propagate to the GPU rendering pipeline sounds like a big plus to the overall user experience. This RFC implements this idea with the hairy part being the notifier chain I added to enable dynamic adjustments. It is a global notifier which raises a few questions so I am very curious what experts will think here. Please see the opens in the first patch for more on this. Last patch ("drm/i915: Connect with the process nice change notifier") demonstrates how i915 can use the notifier. With a bit of notable tracking being required and addedd in "drm/i915: Keep track of registered clients indexed by task struct". On a more positive note the thing seems to even work as is. For instance I roughly simulated the above scenario by running a GPU hog at three nice levels and a GfxBench TRex in parallel (as a game proxy). This is what I got: GPU hog nice | TRex fps --+--- not running | 48.9 0 | 42.7 10 | 47.9 -10 | 29.0 When re-niced the background GPU hog has a much smaller effect on the performance of the game user is running in the foreground. So it appears the feature can indeed improve the user experience. Question is just if people are happy with this method of implementing it. v2: * Moved notifier outside task_rq_lock. * Some improvements and restructuring on the i915 side of the series. Cc: Ingo Molnar Cc: Peter Zijlstra Cc: Juri Lelli Cc: Vincent Guittot Tvrtko Ursulin (8): sched: Add nice value change notifier drm/i915: Explicitly track DRM clients drm/i915: Make GEM contexts track DRM clients drm/i915: Track all user contexts per client drm/i915: Keep track of registered clients indexed by task struct drm/i915: Make some recently added vfuncs use full scheduling attribute drm/i915: Inherit process nice for context scheduling priority drm/i915: Connect with the process nice change notifier drivers/gpu/drm/i915/Makefile | 5 +- drivers/gpu/drm/i915/gem/i915_gem_context.c | 20 +++ .../gpu/drm/i915/gem/i915_gem_context_types.h | 6 + .../drm/i915/gt/intel_execlists_submission.c | 6 +- .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 3 +- drivers/gpu/drm/i915/i915_drm_client.c| 130 ++ drivers/gpu/drm/i915/i915_drm_client.h| 71 ++ drivers/gpu/drm/i915/i915_drv.c | 6 + drivers/gpu/drm/i915/i915_drv.h | 5 + drivers/gpu/drm/i915/i915_gem.c | 21 ++- drivers/gpu/drm/i915/i915_request.c | 2 +- drivers/gpu/drm/i915/i915_request.h | 5 + drivers/gpu/drm/i915/i915_scheduler.c | 16 ++- drivers/gpu/drm/i915/i915_scheduler.h | 14 ++ drivers/gpu/drm/i915/i915_scheduler_types.h | 12 +- include/linux/sched.h | 5 + kernel/sched/core.c | 37 - 17 files changed, 346 insertions(+), 18 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_drm_client.c create mode 100644 drivers/gpu/drm/i915/i915_drm_client.h -- 2.30.2
[RFC 7/8] drm/i915: Inherit process nice for context scheduling priority
From: Tvrtko Ursulin Introduce the concept of context nice value which matches the process nice. We do this by extending the struct i915_sched_attr and add a helper (i915_sched_attr_priority) to be used to convert to effective priority when used by backend code and for priority sorting. Context nice is then inherited from the process which creates the GEM context and utilised secondary to context priority, only when the latter has been left at the default setting, in order to avoid disturbing any application made choices of low and high (batch processing and maybe latency sensitive compositing). In those cases nice value adjusts the effective priority in the narrow band of -19 to +20 around I915_CONTEXT_DEFAULT_PRIORITY. This means that in theory userspace using the context priority uapi directly has a wider range of possible adjustments (thought to be beneficial), but in practice that only applies to execlists platforms. With GuC there are only three priority buckets (less than zero is low priority, zero is normal and greater than zero is high) which therefore interact as expected with the nice adjustment. It makes the question of should the nice be a sub-range of GEM priorities, or stretched across the whole, a moot one. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/gem/i915_gem_context.c| 1 + .../gpu/drm/i915/gt/intel_execlists_submission.c | 4 ++-- drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 +- drivers/gpu/drm/i915/i915_request.c| 2 +- drivers/gpu/drm/i915/i915_request.h| 5 + drivers/gpu/drm/i915/i915_scheduler.c | 12 drivers/gpu/drm/i915/i915_scheduler.h | 14 ++ drivers/gpu/drm/i915/i915_scheduler_types.h| 8 8 files changed, 40 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 8d4d687ab1d0..fed0733cb652 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -257,6 +257,7 @@ proto_context_create(struct drm_i915_private *i915, unsigned int flags) if (i915->params.enable_hangcheck) pc->user_flags |= BIT(UCONTEXT_PERSISTENCE); pc->sched.priority = I915_PRIORITY_NORMAL; + pc->sched.nice = task_nice(current); if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) { if (!HAS_EXECLISTS(i915)) { diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c index e91d803a6453..1a02c65823a7 100644 --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c @@ -250,7 +250,7 @@ static struct i915_priolist *to_priolist(struct rb_node *rb) static int rq_prio(const struct i915_request *rq) { - return READ_ONCE(rq->sched.attr.priority); + return i915_request_priority(rq); } static int effective_prio(const struct i915_request *rq) @@ -3221,8 +3221,8 @@ static void kick_execlists(const struct i915_request *rq, { struct intel_engine_cs *engine = rq->engine; struct i915_sched_engine *sched_engine = engine->sched_engine; + const int prio = i915_sched_attr_priority(attr); const struct i915_request *inflight; - const int prio = attr->priority; /* * We only need to kick the tasklet once for the high priority diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c index b5883a4365ca..f258607685a2 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c @@ -2417,7 +2417,7 @@ static void guc_bump_inflight_request_prio(struct i915_request *rq, const struct i915_sched_attr *attr) { struct intel_context *ce = rq->context; - const int prio = attr->priority; + const int prio = i915_sched_attr_priority(attr); u8 new_guc_prio = map_i915_prio_to_guc_prio(prio); /* Short circuit function */ diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 79da5eca60af..a8c6f3a64895 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -1930,7 +1930,7 @@ static int print_sched_attr(const struct i915_sched_attr *attr, return x; x += snprintf(buf + x, len - x, - " prio=%d", attr->priority); + " prio=%d nice=%d", attr->priority, attr->nice); return x; } diff --git a/drivers/gpu/drm/i915/i915_request.h b/drivers/gpu/drm/i915/i915_request.h index 7bd9ed20623e..c2c4c344837e 100644 --- a/drivers/gpu/drm/i915/i915_request.h +++ b/drivers/gpu/drm/i915/i915_request.h @@ -399,6 +399,11 @@ long i915_request_wait(struct i915_request *rq, #define I915_WAIT
Re: Lockdep spalt on killing a processes
I see my confusion, we hang all unsubmitted jobs on the last submitted to HW job. Yea, in this case indeed rescheduling to a different thread context will avoid the splat but the schedule work cannot be done for each dependency signalling but rather they way we do for ttm_bo_delayed_delete with a list of dependencies to signal. Otherwise some of the schedule work will be drop because previous invocation is still pending execution. Andrey On 2021-10-04 4:14 a.m., Christian König wrote: The problem is a bit different. The callback is on the dependent fence, while we need to signal the scheduler fence. Daniel is right that this needs an irq_work struct to handle this properly. Christian. Am 01.10.21 um 17:10 schrieb Andrey Grodzovsky: From what I see here you supposed to have actual deadlock and not only warning, sched_fence->finished is first signaled from within hw fence done callback (drm_sched_job_done_cb) but then again from within it's own callback (drm_sched_entity_kill_jobs_cb) and so looks like same fence object is recursively signaled twice. This leads to attempt to lock fence->lock second time while it's already locked. I don't see a need to call drm_sched_fence_finished from within drm_sched_entity_kill_jobs_cb as this callback already registered on sched_fence->finished fence (entity->last_scheduled == s_fence->finished) and hence the signaling already took place. Andrey On 2021-10-01 6:50 a.m., Christian König wrote: Hey, Andrey. while investigating some memory management problems I've got the logdep splat below. Looks like something is wrong with drm_sched_entity_kill_jobs_cb(), can you investigate? Thanks, Christian. [11176.741052] [11176.741056] WARNING: possible recursive locking detected [11176.741060] 5.15.0-rc1-00031-g9d546d600800 #171 Not tainted [11176.741066] [11176.741070] swapper/12/0 is trying to acquire lock: [11176.741074] 9c337ed175a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741088] but task is already holding lock: [11176.741092] 9c337ed172a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741100] other info that might help us debug this: [11176.741104] Possible unsafe locking scenario: [11176.741108] CPU0 [11176.741110] [11176.741113] lock(&fence->lock); [11176.741118] lock(&fence->lock); [11176.741122] *** DEADLOCK *** [11176.741125] May be due to missing lock nesting notation [11176.741128] 2 locks held by swapper/12/0: [11176.741133] #0: 9c339c30f768 (&ring->fence_drv.lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741142] #1: 9c337ed172a8 (&fence->lock){-.-.}-{3:3}, at: dma_fence_signal+0x28/0x80 [11176.741151] stack backtrace: [11176.741155] CPU: 12 PID: 0 Comm: swapper/12 Not tainted 5.15.0-rc1-00031-g9d546d600800 #171 [11176.741160] Hardware name: System manufacturer System Product Name/PRIME X399-A, BIOS 0808 10/12/2018 [11176.741165] Call Trace: [11176.741169] [11176.741173] dump_stack_lvl+0x5b/0x74 [11176.741181] dump_stack+0x10/0x12 [11176.741186] __lock_acquire.cold+0x208/0x2df [11176.741197] lock_acquire+0xc6/0x2d0 [11176.741204] ? dma_fence_signal+0x28/0x80 [11176.741212] _raw_spin_lock_irqsave+0x4d/0x70 [11176.741219] ? dma_fence_signal+0x28/0x80 [11176.741225] dma_fence_signal+0x28/0x80 [11176.741230] drm_sched_fence_finished+0x12/0x20 [gpu_sched] [11176.741240] drm_sched_entity_kill_jobs_cb+0x1c/0x50 [gpu_sched] [11176.741248] dma_fence_signal_timestamp_locked+0xac/0x1a0 [11176.741254] dma_fence_signal+0x3b/0x80 [11176.741260] drm_sched_fence_finished+0x12/0x20 [gpu_sched] [11176.741268] drm_sched_job_done.isra.0+0x7f/0x1a0 [gpu_sched] [11176.741277] drm_sched_job_done_cb+0x12/0x20 [gpu_sched] [11176.741284] dma_fence_signal_timestamp_locked+0xac/0x1a0 [11176.741290] dma_fence_signal+0x3b/0x80 [11176.741296] amdgpu_fence_process+0xd1/0x140 [amdgpu] [11176.741504] sdma_v4_0_process_trap_irq+0x8c/0xb0 [amdgpu] [11176.741731] amdgpu_irq_dispatch+0xce/0x250 [amdgpu] [11176.741954] amdgpu_ih_process+0x81/0x100 [amdgpu] [11176.742174] amdgpu_irq_handler+0x26/0xa0 [amdgpu] [11176.742393] __handle_irq_event_percpu+0x4f/0x2c0 [11176.742402] handle_irq_event_percpu+0x33/0x80 [11176.742408] handle_irq_event+0x39/0x60 [11176.742414] handle_edge_irq+0x93/0x1d0 [11176.742419] __common_interrupt+0x50/0xe0 [11176.742426] common_interrupt+0x80/0x90 [11176.742431] [11176.742436] asm_common_interrupt+0x1e/0x40 [11176.742442] RIP: 0010:cpuidle_enter_state+0xff/0x470 [11176.742449] Code: 0f a3 05 04 54 24 01 0f 82 70 02 00 00 31 ff e8 37 5d 6f ff 80 7d d7 00 0f 85 e9 01 00 00 e8 58 a2 7f ff fb 66 0f 1f 44 00 00 <45> 85 ff 0f 88 01 01 00 00 49 63 c7 4c 2b 75 c8 48 8d 14 40 48 8d [11176.742455] RSP: 0018:b6970021fe48 EFLAGS: 0202 [11176.742461] RAX: 0
Re: [PATCH] drm/i915/pmu: Connect engine busyness stats from GuC to pmu
On 24/09/2021 23:34, Umesh Nerlige Ramappa wrote: With GuC handling scheduling, i915 is not aware of the time that a context is scheduled in and out of the engine. Since i915 pmu relies on this info to provide engine busyness to the user, GuC shares this info with i915 for all engines using shared memory. For each engine, this info contains: - total busyness: total time that the context was running (total) - id: id of the running context (id) - start timestamp: timestamp when the context started running (start) At the time (now) of sampling the engine busyness, if the id is valid (!= ~0), and start is non-zero, then the context is considered to be active and the engine busyness is calculated using the below equation engine busyness = total + (now - start) All times are obtained from the gt clock base. For inactive contexts, engine busyness is just equal to the total. The start and total values provided by GuC are 32 bits and wrap around in a few minutes. Since perf pmu provides busyness as 64 bit monotonically increasing values, there is a need for this implementation to account for overflows and extend the time to 64 bits before returning busyness to the user. In order to do that, a worker runs periodically at frequqncy = 1/8th the time it takes for the timestamp to wrap. As an frequency example, that would be once in 27 seconds for a gt clock frequency of 19.2 MHz. Opens and wip that are targeted for later patches: 1) On global gt reset the total busyness of engines resets and i915 needs to fix that so that user sees monotonically increasing busyness. 2) In runtime suspend mode, the worker may not need to be run. We could stop the worker on suspend and rerun it on resume provided that the guc pm timestamp does not tick during suspend. 2) sounds easy since there are park/unpark hooks for pmu already. Will see if I can figure out why you did not just immediately do it. I would also document in the commit message the known problem of possible over-accounting, just for historical reference. v2: (Tvrtko) - Include details in commit message - Move intel engine busyness function into execlist code - Use union inside engine->stats - Use natural type for ping delay jiffies - Drop active_work condition checks - Use for_each_engine if iterating all engines - Drop seq locking, use spinlock at guc level to update engine stats - Document worker specific details Signed-off-by: John Harrison Signed-off-by: Umesh Nerlige Ramappa --- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 26 +-- drivers/gpu/drm/i915/gt/intel_engine_types.h | 82 --- .../drm/i915/gt/intel_execlists_submission.c | 32 +++ .../gpu/drm/i915/gt/uc/abi/guc_actions_abi.h | 1 + drivers/gpu/drm/i915/gt/uc/intel_guc.h| 26 +++ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.c| 21 ++ drivers/gpu/drm/i915/gt/uc/intel_guc_ads.h| 5 + drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 13 ++ .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 204 ++ drivers/gpu/drm/i915/i915_reg.h | 2 + 10 files changed, 363 insertions(+), 49 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c index 2ae57e4656a3..6fcc70a313d9 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c @@ -1873,22 +1873,6 @@ void intel_engine_dump(struct intel_engine_cs *engine, intel_engine_print_breadcrumbs(engine, m); } -static ktime_t __intel_engine_get_busy_time(struct intel_engine_cs *engine, - ktime_t *now) -{ - ktime_t total = engine->stats.total; - - /* -* If the engine is executing something at the moment -* add it to the total. -*/ - *now = ktime_get(); - if (READ_ONCE(engine->stats.active)) - total = ktime_add(total, ktime_sub(*now, engine->stats.start)); - - return total; -} - /** * intel_engine_get_busy_time() - Return current accumulated engine busyness * @engine: engine to report on @@ -1898,15 +1882,7 @@ static ktime_t __intel_engine_get_busy_time(struct intel_engine_cs *engine, */ ktime_t intel_engine_get_busy_time(struct intel_engine_cs *engine, ktime_t *now) { - unsigned int seq; - ktime_t total; - - do { - seq = read_seqcount_begin(&engine->stats.lock); - total = __intel_engine_get_busy_time(engine, now); - } while (read_seqcount_retry(&engine->stats.lock, seq)); - - return total; + return engine->busyness(engine, now); } struct intel_context * diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h index 5ae1207c363b..490166b54ed6 100644 --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h @@ -432,6 +432,12 @@ struct intel_engine_cs { voi
Re: [PATCH 05/10] drm/connector: Add a drm_connector privacy-screen helper functions (v2)
Hi, On 10/4/21 5:22 PM, Ville Syrjälä wrote: > On Sat, Oct 02, 2021 at 06:36:13PM +0200, Hans de Goede wrote: >> Add 2 drm_connector privacy-screen helper functions: >> >> 1. drm_connector_attach_privacy_screen_provider(), this function creates >> and attaches the standard privacy-screen properties and registers a >> generic notifier for generating sysfs-connector-status-events on external >> changes to the privacy-screen status. >> >> 2. drm_connector_update_privacy_screen(), update the privacy-screen's >> sw_state if the connector has a privacy-screen. >> >> Changes in v2: >> - Do not update connector->state->privacy_screen_sw_state on >> atomic-commits. >> - Change drm_connector_update_privacy_screen() to take drm_connector_state >> as argument instead of a full drm_atomic_state. This allows the helper >> to be called by drivers when they are enabling crtcs/encoders/connectors. >> >> Reviewed-by: Emil Velikov >> Reviewed-by: Lyude Paul >> Signed-off-by: Hans de Goede >> --- >> drivers/gpu/drm/drm_connector.c | 102 >> include/drm/drm_connector.h | 11 >> 2 files changed, 113 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_connector.c >> b/drivers/gpu/drm/drm_connector.c >> index b2f1f1b1bfb4..00cf3b6135f6 100644 >> --- a/drivers/gpu/drm/drm_connector.c >> +++ b/drivers/gpu/drm/drm_connector.c >> @@ -28,6 +28,7 @@ >> #include >> #include >> #include >> +#include >> #include >> >> #include >> @@ -462,6 +463,11 @@ void drm_connector_cleanup(struct drm_connector >> *connector) >> DRM_CONNECTOR_REGISTERED)) >> drm_connector_unregister(connector); >> >> +if (connector->privacy_screen) { >> +drm_privacy_screen_put(connector->privacy_screen); >> +connector->privacy_screen = NULL; >> +} >> + >> if (connector->tile_group) { >> drm_mode_put_tile_group(dev, connector->tile_group); >> connector->tile_group = NULL; >> @@ -543,6 +549,10 @@ int drm_connector_register(struct drm_connector >> *connector) >> /* Let userspace know we have a new connector */ >> drm_sysfs_hotplug_event(connector->dev); >> >> +if (connector->privacy_screen) >> +drm_privacy_screen_register_notifier(connector->privacy_screen, >> + &connector->privacy_screen_notifier); >> + >> mutex_lock(&connector_list_lock); >> list_add_tail(&connector->global_connector_list_entry, &connector_list); >> mutex_unlock(&connector_list_lock); >> @@ -578,6 +588,11 @@ void drm_connector_unregister(struct drm_connector >> *connector) >> list_del_init(&connector->global_connector_list_entry); >> mutex_unlock(&connector_list_lock); >> >> +if (connector->privacy_screen) >> +drm_privacy_screen_unregister_notifier( >> +connector->privacy_screen, >> +&connector->privacy_screen_notifier); >> + >> if (connector->funcs->early_unregister) >> connector->funcs->early_unregister(connector); >> >> @@ -2442,6 +2457,93 @@ drm_connector_attach_privacy_screen_properties(struct >> drm_connector *connector) >> } >> EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties); >> >> +static void drm_connector_update_privacy_screen_properties( >> +struct drm_connector *connector, bool set_sw_state) >> +{ >> +enum drm_privacy_screen_status sw_state, hw_state; >> + >> +drm_privacy_screen_get_state(connector->privacy_screen, >> + &sw_state, &hw_state); >> + >> +if (set_sw_state) >> +connector->state->privacy_screen_sw_state = sw_state; >> +drm_object_property_set_value(&connector->base, >> +connector->privacy_screen_hw_state_property, hw_state); >> +} >> + >> +static int drm_connector_privacy_screen_notifier( >> +struct notifier_block *nb, unsigned long action, void *data) >> +{ >> +struct drm_connector *connector = >> +container_of(nb, struct drm_connector, privacy_screen_notifier); >> +struct drm_device *dev = connector->dev; >> + >> +drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> +drm_connector_update_privacy_screen_properties(connector, true); > > This thing still seems pretty unatomic in essence. The standard rule > is that non-immutable properties do not change under external > influence. So if userspace is unaware of the change then it could > just flip the state back to where it was previously. Ie. seems racy > at least which could in theory lead to some funny ping pong in the > state. > > To go proper atomic route here the state of the prop should be > fully cotrolled by userspace. Is that not possible for some reason? No, the privacy-screen can be toggled on/off with a Fn + somekey hotkey combo on the laptop's keyboard, this is fully handled by the laptop's embedded-controller.
Re: [PATCH v5 02/15] drm/edid: Break out reading block 0 of the EDID
Hi Douglas, On Tue, Sep 14, 2021 at 10:23 PM Douglas Anderson wrote: > A future change wants to be able to read just block 0 of the EDID, so > break it out of drm_do_get_edid() into a sub-function. > > This is intended to be a no-op change--just code movement. > > Signed-off-by: Douglas Anderson Thanks for your patch, which is now commit bac9c29482248b00 ("drm/edid: Break out reading block 0 of the EDID") in drm-next. > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1905,6 +1905,44 @@ int drm_add_override_edid_modes(struct drm_connector > *connector) > } > EXPORT_SYMBOL(drm_add_override_edid_modes); > > +static struct edid *drm_do_get_edid_base_block( > + int (*get_edid_block)(void *data, u8 *buf, unsigned int block, > + size_t len), > + void *data, bool *edid_corrupt, int *null_edid_counter) > +{ > + int i; > + void *edid; > + > + edid = kmalloc(EDID_LENGTH, GFP_KERNEL); > + if (edid == NULL) > + return NULL; > + > + /* base block fetch */ > + for (i = 0; i < 4; i++) { > + if (get_edid_block(data, edid, 0, EDID_LENGTH)) > + goto out; > + if (drm_edid_block_valid(edid, 0, false, edid_corrupt)) > + break; > + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > + if (null_edid_counter) > + (*null_edid_counter)++; > + goto carp; > + } > + } > + if (i == 4) > + goto carp; > + > + return edid; > + > +carp: > + kfree(edid); > + return ERR_PTR(-EINVAL); > + > +out: > + kfree(edid); > + return NULL; > +} > + > /** > * drm_do_get_edid - get EDID data using a custom EDID block read function > * @connector: connector we're probing > @@ -1938,25 +1976,16 @@ struct edid *drm_do_get_edid(struct drm_connector > *connector, > if (override) > return override; > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > + edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, > + &connector->edid_corrupt, > + > &connector->null_edid_counter); > + if (IS_ERR_OR_NULL(edid)) { > + if (IS_ERR(edid)) So edid is an error code, not a valid pointer... > + connector_bad_edid(connector, edid, 1); ... while connector_bad_edid() expects edid to be a valid pointer, causing a crash: Unable to handle kernel NULL pointer dereference at virtual address 0068 Mem abort info: ESR = 0x9604 EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 FSC = 0x04: level 0 translation fault Data abort info: ISV = 0, ISS = 0x0004 CM = 0, WnR = 0 [0068] user address but active_mm is swapper Internal error: Oops: 9604 [#1] PREEMPT SMP CPU: 0 PID: 7 Comm: kworker/u4:0 Not tainted 5.15.0-rc3-arm64-renesas-00629-geb2d42841024-dirty #1313 Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT) Workqueue: events_unbound deferred_probe_work_func pstate: 2005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : connector_bad_edid+0x28/0x1a8 lr : drm_do_get_edid+0x260/0x268 sp : 8000121336a0 x29: 8000121336a0 x28: 0a373200 x27: 1ffe PM: always-on/ee16.mmc: stop x26: 0005 x25: 0041 x24: x23: 08a25080 x22: 8000106bd990 x21: 081496c0 x20: 0001 x19: ffea x18: 0010 x17: x16: x15: x14: x13: x12: x11: x10: x9 : x8 : x7 : 0080 x6 : 09c71000 x5 : x4 : 0069 x3 : 0a3c2900 x2 : 0001 x1 : ffea x0 : 09c71000 Call trace: connector_bad_edid+0x28/0x1a8 drm_do_get_edid+0x260/0x268 adv7511_get_edid+0xb4/0xd0 adv7511_bridge_get_edid+0x10/0x18 > return NULL; > - > - /* base block fetch */ > - for (i = 0; i < 4; i++) { > - if (get_edid_block(data, edid, 0, EDID_LENGTH)) > - goto out; > - if (drm_edid_block_valid(edid, 0, false, > -&connector->edid_corrupt)) > - break; > - if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > - connector->null_edid_counter++; > - goto carp; > - } > } > - if (i == 4) > - goto carp; > > - /* if there's no extensions, we're done */ > + /* if there's no extensions or no connector, we're done */ >
Re: [PATCH 09/10] drm/i915: Add intel_modeset_probe_defer() helper
On Sat, Oct 02, 2021 at 06:36:17PM +0200, Hans de Goede wrote: > The upcoming privacy-screen support adds another check for > deferring probe till some other drivers have bound first. > > Factor out the current vga_switcheroo_client_probe_defer() check > into an intel_modeset_probe_defer() helper, so that further > probe-deferral checks can be added there. > > Signed-off-by: Hans de Goede > --- > drivers/gpu/drm/i915/display/intel_display.c | 13 + > drivers/gpu/drm/i915/display/intel_display.h | 1 + > drivers/gpu/drm/i915/i915_pci.c | 9 ++--- > 3 files changed, 16 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index 60b2bc3ad011..e67f3207ba54 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -32,6 +32,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -12690,6 +12691,18 @@ void intel_modeset_driver_remove_nogem(struct > drm_i915_private *i915) > intel_bios_driver_remove(i915); > } > > +bool intel_modeset_probe_defer(struct pci_dev *pdev) > +{ > + /* > + * apple-gmux is needed on dual GPU MacBook Pro > + * to probe the panel if we're the inactive GPU. > + */ > + if (vga_switcheroo_client_probe_defer(pdev)) > + return true; > + > + return false; > +} Seems fine. Presumably Jani isn't too grumpy about it :P Reviewed-by: Ville Syrjälä > + > void intel_display_driver_register(struct drm_i915_private *i915) > { > if (!HAS_DISPLAY(i915)) > diff --git a/drivers/gpu/drm/i915/display/intel_display.h > b/drivers/gpu/drm/i915/display/intel_display.h > index 3028072c2cf3..d3d34acb6c08 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.h > +++ b/drivers/gpu/drm/i915/display/intel_display.h > @@ -633,6 +633,7 @@ void intel_display_driver_register(struct > drm_i915_private *i915); > void intel_display_driver_unregister(struct drm_i915_private *i915); > > /* modesetting */ > +bool intel_modeset_probe_defer(struct pci_dev *pdev); > void intel_modeset_init_hw(struct drm_i915_private *i915); > int intel_modeset_init_noirq(struct drm_i915_private *i915); > int intel_modeset_init_nogem(struct drm_i915_private *i915); > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c > index d4a6a9dcf182..cf4ad648b742 100644 > --- a/drivers/gpu/drm/i915/i915_pci.c > +++ b/drivers/gpu/drm/i915/i915_pci.c > @@ -22,8 +22,6 @@ > * > */ > > -#include > - > #include > #include > > @@ -1187,11 +1185,8 @@ static int i915_pci_probe(struct pci_dev *pdev, const > struct pci_device_id *ent) > if (PCI_FUNC(pdev->devfn)) > return -ENODEV; > > - /* > - * apple-gmux is needed on dual GPU MacBook Pro > - * to probe the panel if we're the inactive GPU. > - */ > - if (vga_switcheroo_client_probe_defer(pdev)) > + /* Detect if we need to wait for other drivers early on */ > + if (intel_modeset_probe_defer(pdev)) > return -EPROBE_DEFER; > > err = i915_driver_probe(pdev, ent); > -- > 2.31.1 -- Ville Syrjälä Intel
Re: [PATCH 15/16] Revert "drm/i915: cleanup: drm_modeset_lock_all_ctx() --> DRM_MODESET_LOCK_ALL_BEGIN()"
On Mon, Oct 04, 2021 at 12:41:00PM +0300, Ville Syrjälä wrote: > On Sat, Oct 02, 2021 at 11:45:41AM -0400, Sean Paul wrote: > > From: Sean Paul > > > > This reverts commit 399190e70816886e2bca1f3f3bc3d9c544af88e7. > > > > This patchset breaks on intel platforms and was previously NACK'd by > > Ville. > > > > Cc: Ville Syrjälä > > Cc: Fernando Ramos > > Signed-off-by: Sean Paul > > Yeah, best to try again from the start I think. Pushed the revert set (and left the TODO item out for now). Thanks for raising the issue. @Fernando, hopefully you can revise and post again. Thank you for your patches and your effort! Sean > > For the series > Acked-by: Ville Syrjälä > > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 18 +- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index 2bf01416d656..134a6acbd8fb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -43,7 +43,6 @@ > > #include > > #include > > #include > > -#include > > > > #include "display/intel_audio.h" > > #include "display/intel_crt.h" > > @@ -13477,13 +13476,22 @@ void intel_display_resume(struct drm_device *dev) > > if (state) > > state->acquire_ctx = &ctx; > > > > - DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); > > + drm_modeset_acquire_init(&ctx, 0); > > > > - ret = __intel_display_resume(dev, state, &ctx); > > + while (1) { > > + ret = drm_modeset_lock_all_ctx(dev, &ctx); > > + if (ret != -EDEADLK) > > + break; > > > > - intel_enable_ipc(dev_priv); > > + drm_modeset_backoff(&ctx); > > + } > > + > > + if (!ret) > > + ret = __intel_display_resume(dev, state, &ctx); > > > > - DRM_MODESET_LOCK_ALL_END(dev, ctx, ret); > > + intel_enable_ipc(dev_priv); > > + drm_modeset_drop_locks(&ctx); > > + drm_modeset_acquire_fini(&ctx); > > > > if (ret) > > drm_err(&dev_priv->drm, > > -- > > Sean Paul, Software Engineer, Google / Chromium OS > > -- > Ville Syrjälä > Intel -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v13 13/35] drm/tegra: gr2d: Support generic power domain and runtime PM
04.10.2021 14:01, Ulf Hansson пишет: > On Fri, 1 Oct 2021 at 21:00, Dmitry Osipenko wrote: >> >> 01.10.2021 17:55, Ulf Hansson пишет: >>> On Fri, 1 Oct 2021 at 16:29, Dmitry Osipenko wrote: 01.10.2021 16:39, Ulf Hansson пишет: > On Mon, 27 Sept 2021 at 00:42, Dmitry Osipenko wrote: >> >> Add runtime power management and support generic power domains. >> >> Tested-by: Peter Geis # Ouya T30 >> Tested-by: Paul Fertser # PAZ00 T20 >> Tested-by: Nicolas Chauvet # PAZ00 T20 and TK1 T124 >> Tested-by: Matt Merhar # Ouya T30 >> Signed-off-by: Dmitry Osipenko >> --- >> drivers/gpu/drm/tegra/gr2d.c | 155 +-- > > [...] > >> static int gr2d_remove(struct platform_device *pdev) >> @@ -259,15 +312,101 @@ static int gr2d_remove(struct platform_device >> *pdev) >> return err; >> } >> >> + pm_runtime_dont_use_autosuspend(&pdev->dev); >> + pm_runtime_disable(&pdev->dev); > > There is no guarantee that the ->runtime_suspend() has been invoked > here, which means that clock may be left prepared/enabled beyond this > point. > > I suggest you call pm_runtime_force_suspend(), instead of > pm_runtime_disable(), to make sure that gets done. The pm_runtime_disable() performs the final synchronization, please see [1]. [1] https://elixir.bootlin.com/linux/v5.15-rc3/source/drivers/base/power/runtime.c#L1412 >>> >>> pm_runtime_disable() end up calling _pm_runtime_barrier(), which calls >>> cancel_work_sync() if dev->power.request_pending has been set. >>> >>> If the work that was punted to the pm_wq in rpm_idle() has not been >>> started yet, we end up just canceling it. In other words, there are no >>> guarantees it runs to completion. >> >> You're right. Although, in a case of this particular patch, the syncing >> is actually implicitly done by pm_runtime_dont_use_autosuspend(). >> >> But for drivers which don't use auto-suspend, there is no sync. This >> looks like a disaster, it's a very common pattern for drivers to >> 'put+disable'. >> >>> Moreover, use space may have bumped the usage count via sysfs for the >>> device (pm_runtime_forbid()) to keep the device runtime resumed. >> >> Right, this is also a disaster in a case of driver removal. >> Calling pm_runtime_force_suspend() isn't correct because each 'enable' must have the corresponding 'disable'. Hence there is no problem here. >>> >>> pm_runtime_force_suspend() calls pm_runtime_disable(), so I think that >>> should be fine. No? >> >> [adding Rafael] >> >> Rafael, could you please explain how drivers are supposed to properly >> suspend and disable RPM to cut off power and reset state that was >> altered by the driver's resume callback? What we're missing? Is Ulf's >> suggestion acceptable? >> >> The RPM state of a device is getting reset on driver's removal, hence >> all refcounts that were bumped by the rpm-resume callback of the device >> driver will be screwed up if device is kept resumed after removal. I >> just verified that it's true in practice. > > Note that, what makes the Tegra drivers a bit special is that they are > always built with CONFIG_PM being set (selected from the "SoC" > Kconfig). > > Therefore, pm_runtime_force_suspend() can work for some of these > cases. Using this, would potentially avoid the driver from having to > runtime resume the device in ->remove(), according to the below > generic sequence, which is used in many drivers. > > pm_runtime_get_sync() > clk_disable_unprepare() (+ additional things to turn off the device) > pm_runtime_disable() > pm_runtime_put_noidle() It's not a problem to change this patchset. The problem is that if you'll grep mainline for 'pm_runtime_disable', you will find that there are a lot of drivers in a potential trouble. I'm proposing that we should change pm_runtime_disable() to perform the syncing with this oneliner: diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index ec94049442b9..5c9f28165824 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -1380,6 +1380,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_barrier); */ void __pm_runtime_disable(struct device *dev, bool check_resume) { + flush_work(&dev->power.work); + spin_lock_irq(&dev->power.lock); if (dev->power.disable_depth > 0) { Objections? The sysfs rpm-forbid is a separate problem and it's less troublesome since it requires root privileges. It's also not something that userspace touches casually. For now I don't know what could be done about it.
Re: [PATCH 10/10] drm/i915: Add privacy-screen support (v2)
Hi, On 10/4/21 5:37 PM, Ville Syrjälä wrote: > On Sat, Oct 02, 2021 at 06:36:18PM +0200, Hans de Goede wrote: >> Add support for eDP panels with a built-in privacy screen using the >> new drm_privacy_screen class. >> >> Changes in v2: >> - Call drm_connector_update_privacy_screen() from >> intel_enable_ddi_dp() / intel_ddi_update_pipe_dp() instead of adding a >> for_each_new_connector_in_state() loop to intel_atomic_commit_tail() >> - Move the probe-deferral check to the intel_modeset_probe_defer() helper >> >> Signed-off-by: Hans de Goede >> --- >> drivers/gpu/drm/i915/display/intel_atomic.c | 1 + >> drivers/gpu/drm/i915/display/intel_ddi.c | 3 +++ >> drivers/gpu/drm/i915/display/intel_display.c | 10 ++ >> drivers/gpu/drm/i915/display/intel_dp.c | 10 ++ >> 4 files changed, 24 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c >> b/drivers/gpu/drm/i915/display/intel_atomic.c >> index b4e7ac51aa31..a62550711e98 100644 >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c >> @@ -139,6 +139,7 @@ int intel_digital_connector_atomic_check(struct >> drm_connector *conn, >> new_conn_state->base.picture_aspect_ratio != >> old_conn_state->base.picture_aspect_ratio || >> new_conn_state->base.content_type != >> old_conn_state->base.content_type || >> new_conn_state->base.scaling_mode != >> old_conn_state->base.scaling_mode || >> +new_conn_state->base.privacy_screen_sw_state != >> old_conn_state->base.privacy_screen_sw_state || >> !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) >> crtc_state->mode_changed = true; >> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c >> b/drivers/gpu/drm/i915/display/intel_ddi.c >> index 51cd0420e00e..e4496c830a35 100644 >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c >> @@ -25,6 +25,7 @@ >> * >> */ >> >> +#include >> #include >> >> #include "i915_drv.h" >> @@ -3022,6 +3023,7 @@ static void intel_enable_ddi_dp(struct >> intel_atomic_state *state, >> if (port == PORT_A && DISPLAY_VER(dev_priv) < 9) >> intel_dp_stop_link_train(intel_dp, crtc_state); >> >> +drm_connector_update_privacy_screen(conn_state); >> intel_edp_backlight_on(crtc_state, conn_state); >> >> if (!dig_port->lspcon.active || dig_port->dp.has_hdmi_sink) >> @@ -3247,6 +3249,7 @@ static void intel_ddi_update_pipe_dp(struct >> intel_atomic_state *state, >> intel_drrs_update(intel_dp, crtc_state); >> >> intel_backlight_update(state, encoder, crtc_state, conn_state); >> +drm_connector_update_privacy_screen(conn_state); >> } >> >> void intel_ddi_update_pipe(struct intel_atomic_state *state, >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c >> b/drivers/gpu/drm/i915/display/intel_display.c >> index e67f3207ba54..9a5dbe51458d 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -42,6 +42,7 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -12693,6 +12694,8 @@ void intel_modeset_driver_remove_nogem(struct >> drm_i915_private *i915) >> >> bool intel_modeset_probe_defer(struct pci_dev *pdev) >> { >> +struct drm_privacy_screen *privacy_screen; >> + >> /* >> * apple-gmux is needed on dual GPU MacBook Pro >> * to probe the panel if we're the inactive GPU. >> @@ -12700,6 +12703,13 @@ bool intel_modeset_probe_defer(struct pci_dev *pdev) >> if (vga_switcheroo_client_probe_defer(pdev)) >> return true; >> >> +/* If the LCD panel has a privacy-screen, wait for it */ >> +privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL); >> +if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER) >> +return true; >> + >> +drm_privacy_screen_put(privacy_screen); >> + >> return false; >> } >> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c >> b/drivers/gpu/drm/i915/display/intel_dp.c >> index 74a657ae131a..91207310dc0d 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c >> @@ -37,6 +37,7 @@ >> #include >> #include >> #include >> +#include >> #include >> >> #include "g4x_dp.h" >> @@ -4808,6 +4809,7 @@ static bool intel_edp_init_connector(struct intel_dp >> *intel_dp, >> struct drm_connector *connector = &intel_connector->base; >> struct drm_display_mode *fixed_mode = NULL; >> struct drm_display_mode *downclock_mode = NULL; >> +struct drm_privacy_screen *privacy_screen; >> bool has_dpcd; >> enum pipe pipe = INVALID_PIPE; >> struct edid *edid; >> @@ -4902,6 +4904,14 @@ static bool intel_edp_init_connector(struct intel_dp >> *intel_dp, >> fixed_mode->hdisplay, fixed_mode
Re: [PATCH 05/10] drm/connector: Add a drm_connector privacy-screen helper functions (v2)
On Sat, Oct 02, 2021 at 06:36:13PM +0200, Hans de Goede wrote: > Add 2 drm_connector privacy-screen helper functions: > > 1. drm_connector_attach_privacy_screen_provider(), this function creates > and attaches the standard privacy-screen properties and registers a > generic notifier for generating sysfs-connector-status-events on external > changes to the privacy-screen status. > > 2. drm_connector_update_privacy_screen(), update the privacy-screen's > sw_state if the connector has a privacy-screen. > > Changes in v2: > - Do not update connector->state->privacy_screen_sw_state on > atomic-commits. > - Change drm_connector_update_privacy_screen() to take drm_connector_state > as argument instead of a full drm_atomic_state. This allows the helper > to be called by drivers when they are enabling crtcs/encoders/connectors. > > Reviewed-by: Emil Velikov > Reviewed-by: Lyude Paul > Signed-off-by: Hans de Goede > --- > drivers/gpu/drm/drm_connector.c | 102 > include/drm/drm_connector.h | 11 > 2 files changed, 113 insertions(+) > > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c > index b2f1f1b1bfb4..00cf3b6135f6 100644 > --- a/drivers/gpu/drm/drm_connector.c > +++ b/drivers/gpu/drm/drm_connector.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -462,6 +463,11 @@ void drm_connector_cleanup(struct drm_connector > *connector) > DRM_CONNECTOR_REGISTERED)) > drm_connector_unregister(connector); > > + if (connector->privacy_screen) { > + drm_privacy_screen_put(connector->privacy_screen); > + connector->privacy_screen = NULL; > + } > + > if (connector->tile_group) { > drm_mode_put_tile_group(dev, connector->tile_group); > connector->tile_group = NULL; > @@ -543,6 +549,10 @@ int drm_connector_register(struct drm_connector > *connector) > /* Let userspace know we have a new connector */ > drm_sysfs_hotplug_event(connector->dev); > > + if (connector->privacy_screen) > + drm_privacy_screen_register_notifier(connector->privacy_screen, > +&connector->privacy_screen_notifier); > + > mutex_lock(&connector_list_lock); > list_add_tail(&connector->global_connector_list_entry, &connector_list); > mutex_unlock(&connector_list_lock); > @@ -578,6 +588,11 @@ void drm_connector_unregister(struct drm_connector > *connector) > list_del_init(&connector->global_connector_list_entry); > mutex_unlock(&connector_list_lock); > > + if (connector->privacy_screen) > + drm_privacy_screen_unregister_notifier( > + connector->privacy_screen, > + &connector->privacy_screen_notifier); > + > if (connector->funcs->early_unregister) > connector->funcs->early_unregister(connector); > > @@ -2442,6 +2457,93 @@ drm_connector_attach_privacy_screen_properties(struct > drm_connector *connector) > } > EXPORT_SYMBOL(drm_connector_attach_privacy_screen_properties); > > +static void drm_connector_update_privacy_screen_properties( > + struct drm_connector *connector, bool set_sw_state) > +{ > + enum drm_privacy_screen_status sw_state, hw_state; > + > + drm_privacy_screen_get_state(connector->privacy_screen, > + &sw_state, &hw_state); > + > + if (set_sw_state) > + connector->state->privacy_screen_sw_state = sw_state; > + drm_object_property_set_value(&connector->base, > + connector->privacy_screen_hw_state_property, hw_state); > +} > + > +static int drm_connector_privacy_screen_notifier( > + struct notifier_block *nb, unsigned long action, void *data) > +{ > + struct drm_connector *connector = > + container_of(nb, struct drm_connector, privacy_screen_notifier); > + struct drm_device *dev = connector->dev; > + > + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > + drm_connector_update_privacy_screen_properties(connector, true); This thing still seems pretty unatomic in essence. The standard rule is that non-immutable properties do not change under external influence. So if userspace is unaware of the change then it could just flip the state back to where it was previously. Ie. seems racy at least which could in theory lead to some funny ping pong in the state. To go proper atomic route here the state of the prop should be fully cotrolled by userspace. Is that not possible for some reason? > + drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > + drm_sysfs_connector_status_event(connector, > + connector->privacy_screen_sw_state_property); > + drm_sysfs_connector_status_event(connector, > + connector->pr
Re: [PATCH 10/10] drm/i915: Add privacy-screen support (v2)
On Sat, Oct 02, 2021 at 06:36:18PM +0200, Hans de Goede wrote: > Add support for eDP panels with a built-in privacy screen using the > new drm_privacy_screen class. > > Changes in v2: > - Call drm_connector_update_privacy_screen() from > intel_enable_ddi_dp() / intel_ddi_update_pipe_dp() instead of adding a > for_each_new_connector_in_state() loop to intel_atomic_commit_tail() > - Move the probe-deferral check to the intel_modeset_probe_defer() helper > > Signed-off-by: Hans de Goede > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 1 + > drivers/gpu/drm/i915/display/intel_ddi.c | 3 +++ > drivers/gpu/drm/i915/display/intel_display.c | 10 ++ > drivers/gpu/drm/i915/display/intel_dp.c | 10 ++ > 4 files changed, 24 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > b/drivers/gpu/drm/i915/display/intel_atomic.c > index b4e7ac51aa31..a62550711e98 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -139,6 +139,7 @@ int intel_digital_connector_atomic_check(struct > drm_connector *conn, > new_conn_state->base.picture_aspect_ratio != > old_conn_state->base.picture_aspect_ratio || > new_conn_state->base.content_type != > old_conn_state->base.content_type || > new_conn_state->base.scaling_mode != > old_conn_state->base.scaling_mode || > + new_conn_state->base.privacy_screen_sw_state != > old_conn_state->base.privacy_screen_sw_state || > !drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) > crtc_state->mode_changed = true; > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > b/drivers/gpu/drm/i915/display/intel_ddi.c > index 51cd0420e00e..e4496c830a35 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -25,6 +25,7 @@ > * > */ > > +#include > #include > > #include "i915_drv.h" > @@ -3022,6 +3023,7 @@ static void intel_enable_ddi_dp(struct > intel_atomic_state *state, > if (port == PORT_A && DISPLAY_VER(dev_priv) < 9) > intel_dp_stop_link_train(intel_dp, crtc_state); > > + drm_connector_update_privacy_screen(conn_state); > intel_edp_backlight_on(crtc_state, conn_state); > > if (!dig_port->lspcon.active || dig_port->dp.has_hdmi_sink) > @@ -3247,6 +3249,7 @@ static void intel_ddi_update_pipe_dp(struct > intel_atomic_state *state, > intel_drrs_update(intel_dp, crtc_state); > > intel_backlight_update(state, encoder, crtc_state, conn_state); > + drm_connector_update_privacy_screen(conn_state); > } > > void intel_ddi_update_pipe(struct intel_atomic_state *state, > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index e67f3207ba54..9a5dbe51458d 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -42,6 +42,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -12693,6 +12694,8 @@ void intel_modeset_driver_remove_nogem(struct > drm_i915_private *i915) > > bool intel_modeset_probe_defer(struct pci_dev *pdev) > { > + struct drm_privacy_screen *privacy_screen; > + > /* >* apple-gmux is needed on dual GPU MacBook Pro >* to probe the panel if we're the inactive GPU. > @@ -12700,6 +12703,13 @@ bool intel_modeset_probe_defer(struct pci_dev *pdev) > if (vga_switcheroo_client_probe_defer(pdev)) > return true; > > + /* If the LCD panel has a privacy-screen, wait for it */ > + privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL); > + if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER) > + return true; > + > + drm_privacy_screen_put(privacy_screen); > + > return false; > } > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 74a657ae131a..91207310dc0d 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -37,6 +37,7 @@ > #include > #include > #include > +#include > #include > > #include "g4x_dp.h" > @@ -4808,6 +4809,7 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > struct drm_connector *connector = &intel_connector->base; > struct drm_display_mode *fixed_mode = NULL; > struct drm_display_mode *downclock_mode = NULL; > + struct drm_privacy_screen *privacy_screen; > bool has_dpcd; > enum pipe pipe = INVALID_PIPE; > struct edid *edid; > @@ -4902,6 +4904,14 @@ static bool intel_edp_init_connector(struct intel_dp > *intel_dp, > fixed_mode->hdisplay, fixed_mode->vdisplay); > } > > + privacy_screen = drm_privacy_screen_get(dev->dev, NULL); > + if (!IS_ERR(privacy_screen)) { > +
Re: [PATCH] drm/msm/a6xx: Serialize GMU communication
On 10/2/2021 1:02 AM, Rob Clark wrote: From: Rob Clark I've seen some crashes in our crash reporting that *look* like multiple threads stomping on each other while communicating with GMU. So wrap all those paths in a lock. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 3 ++ drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 40 +++ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index a7c58018959f..8b73f70766a4 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -296,6 +296,8 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state) u32 val; int request, ack; + WARN_ON_ONCE(!mutex_is_locked(&gmu->lock)); + if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits)) return -EINVAL; @@ -337,6 +339,8 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state) { int bit; + WARN_ON_ONCE(!mutex_is_locked(&gmu->lock)); + if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits)) return; @@ -1482,6 +1486,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node) if (!pdev) return -ENODEV; + mutex_init(&gmu->lock); + gmu->dev = &pdev->dev; of_dma_configure(gmu->dev, node, true); diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h index 3c74f64e3126..84bd516f01e8 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h @@ -44,6 +44,9 @@ struct a6xx_gmu_bo { struct a6xx_gmu { struct device *dev; + /* For serializing communication with the GMU: */ + struct mutex lock; + struct msm_gem_address_space *aspace; void * __iomem mmio; diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index f6a4dbef796b..bd7bdeff5d6f 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -881,7 +881,7 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu) A6XX_RBBM_INT_0_MASK_UCHE_OOB_ACCESS | \ A6XX_RBBM_INT_0_MASK_UCHE_TRAP_INTR) -static int a6xx_hw_init(struct msm_gpu *gpu) +static int hw_init(struct msm_gpu *gpu) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); @@ -1135,6 +1135,19 @@ static int a6xx_hw_init(struct msm_gpu *gpu) return ret; } +static int a6xx_hw_init(struct msm_gpu *gpu) +{ + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); + int ret; + + mutex_lock(&a6xx_gpu->gmu.lock); + ret = hw_init(gpu); + mutex_unlock(&a6xx_gpu->gmu.lock); + + return ret; +} + static void a6xx_dump(struct msm_gpu *gpu) { DRM_DEV_INFO(&gpu->pdev->dev, "status: %08x\n", @@ -1509,7 +1522,9 @@ static int a6xx_pm_resume(struct msm_gpu *gpu) trace_msm_gpu_resume(0); + mutex_lock(&a6xx_gpu->gmu.lock); ret = a6xx_gmu_resume(a6xx_gpu); + mutex_unlock(&a6xx_gpu->gmu.lock); if (ret) return ret; @@ -1532,7 +1547,9 @@ static int a6xx_pm_suspend(struct msm_gpu *gpu) msm_devfreq_suspend(gpu); + mutex_lock(&a6xx_gpu->gmu.lock); ret = a6xx_gmu_stop(a6xx_gpu); + mutex_unlock(&a6xx_gpu->gmu.lock); if (ret) return ret; @@ -1547,18 +1564,19 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value) { struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); - static DEFINE_MUTEX(perfcounter_oob); - mutex_lock(&perfcounter_oob); + mutex_lock(&a6xx_gpu->gmu.lock); /* Force the GPU power on so we can read this register */ a6xx_gmu_set_oob(&a6xx_gpu->gmu, GMU_OOB_PERFCOUNTER_SET); *value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO, - REG_A6XX_CP_ALWAYS_ON_COUNTER_HI); + REG_A6XX_CP_ALWAYS_ON_COUNTER_HI); a6xx_gmu_clear_oob(&a6xx_gpu->gmu, GMU_OOB_PERFCOUNTER_SET); - mutex_unlock(&perfcounter_oob); + + mutex_unlock(&a6xx_gpu->gmu.lock); + return 0; } @@ -1622,6 +1640,16 @@ static unsigned long a6xx_gpu_busy(struct msm_gpu *gpu) return (unsigned long)busy_time; } +void a6xx_gpu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp) +{ + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu); + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu); + + mutex_lock(&a6xx_gpu->gmu.lock); + a6xx_gmu_set_freq(gpu, opp); + mutex_unlock(&a6xx_gpu->gmu.lock); +} + static struct msm_gem_address_space * a6xx_create_address_space(struct msm_gpu *g
[PATCH] drm/edid: Fix crash with zero/invalid EDID
In the commit bac9c2948224 ("drm/edid: Break out reading block 0 of the EDID") I broke out reading the base block of the EDID to its own function. Unfortunately, when I did that I messed up the handling when drm_edid_is_zero() indicated that we had an EDID that was all 0x00 or when we went through 4 loops and didn't get a valid EDID. Specifically I needed to pass the broken EDID to connector_bad_edid() but now I was passing an error-pointer. Let's re-jigger things so we can pass the bad EDID in properly. Fixes: bac9c2948224 ("drm/edid: Break out reading block 0 of the EDID") Reported-by: kernel test robot Reported-by: Geert Uytterhoeven Signed-off-by: Douglas Anderson --- drivers/gpu/drm/drm_edid.c | 27 +++ 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 9b19eee0e1b4..9c9463ec5465 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1911,13 +1911,15 @@ int drm_add_override_edid_modes(struct drm_connector *connector) } EXPORT_SYMBOL(drm_add_override_edid_modes); -static struct edid *drm_do_get_edid_base_block( +static struct edid *drm_do_get_edid_base_block(struct drm_connector *connector, int (*get_edid_block)(void *data, u8 *buf, unsigned int block, size_t len), - void *data, bool *edid_corrupt, int *null_edid_counter) + void *data) { - int i; + int *null_edid_counter = connector ? &connector->null_edid_counter : NULL; + bool *edid_corrupt = connector ? &connector->edid_corrupt : NULL; void *edid; + int i; edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (edid == NULL) @@ -1941,9 +1943,8 @@ static struct edid *drm_do_get_edid_base_block( return edid; carp: - kfree(edid); - return ERR_PTR(-EINVAL); - + if (connector) + connector_bad_edid(connector, edid, 1); out: kfree(edid); return NULL; @@ -1982,14 +1983,9 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, if (override) return override; - edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, - &connector->edid_corrupt, - &connector->null_edid_counter); - if (IS_ERR_OR_NULL(edid)) { - if (IS_ERR(edid)) - connector_bad_edid(connector, edid, 1); + edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block, data); + if (!edid) return NULL; - } /* if there's no extensions or no connector, we're done */ valid_extensions = edid[0x7e]; @@ -2142,14 +2138,13 @@ u32 drm_edid_get_panel_id(struct i2c_adapter *adapter) struct edid *edid; u32 panel_id; - edid = drm_do_get_edid_base_block(drm_do_probe_ddc_edid, adapter, - NULL, NULL); + edid = drm_do_get_edid_base_block(NULL, drm_do_probe_ddc_edid, adapter); /* * There are no manufacturer IDs of 0, so if there is a problem reading * the EDID then we'll just return 0. */ - if (IS_ERR_OR_NULL(edid)) + if (!edid) return 0; panel_id = edid_extract_panel_id(edid); -- 2.33.0.800.g4c38ced690-goog
Re: [PATCH v5 02/15] drm/edid: Break out reading block 0 of the EDID
Hi, On Mon, Oct 4, 2021 at 8:42 AM Geert Uytterhoeven wrote: > > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > > + edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, > > + &connector->edid_corrupt, > > + > > &connector->null_edid_counter); > > + if (IS_ERR_OR_NULL(edid)) { > > + if (IS_ERR(edid)) > > So edid is an error code, not a valid pointer... > > > + connector_bad_edid(connector, edid, 1); > > ... while connector_bad_edid() expects edid to be a valid pointer, > causing a crash: > > Unable to handle kernel NULL pointer dereference at virtual address Sigh. Thanks for the report and analysis. I guess I don't have any displays reporting invalid EDIDs to test with. Hopefully this will help: https://lore.kernel.org/r/20211004092100.1.Ic90a5ebd44c75db963112be167a03cc96f9fb249@changeid/ -Doug
Re: [PATCH] dt-bindings: drm/bridge: ti-sn65dsi86: Fix reg value
On Fri, 24 Sep 2021 14:35:12 +0200, Geert Uytterhoeven wrote: > make dtbs_check: > > arch/arm64/boot/dts/qcom/sdm850-lenovo-yoga-c630.dt.yaml: bridge@2c: > reg:0:0: 45 was expected > > According to the datasheet, the I2C address can be either 0x2c or 0x2d, > depending on the ADDR control input. > > Fixes: e3896e6dddf0b821 ("dt-bindings: drm/bridge: Document sn65dsi86 bridge > bindings") > Signed-off-by: Geert Uytterhoeven > --- > Also seen with the in-flight Falcon DSI display output patch: > > arch/arm64/boot/dts/renesas/r8a779a0-falcon.dt.yaml: sn65dsi86@2c: > reg:0:0: 45 was expected > --- > .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml| 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Applied, thanks!
Re: [PATCH v5 02/15] drm/edid: Break out reading block 0 of the EDID
Hi Doug, On Mon, Oct 4, 2021 at 6:26 PM Doug Anderson wrote: > On Mon, Oct 4, 2021 at 8:42 AM Geert Uytterhoeven > wrote: > > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > > > + edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, > > > + &connector->edid_corrupt, > > > + > > > &connector->null_edid_counter); > > > + if (IS_ERR_OR_NULL(edid)) { > > > + if (IS_ERR(edid)) > > > > So edid is an error code, not a valid pointer... > > > > > + connector_bad_edid(connector, edid, 1); > > > > ... while connector_bad_edid() expects edid to be a valid pointer, > > causing a crash: > > > > Unable to handle kernel NULL pointer dereference at virtual address > > Sigh. Thanks for the report and analysis. I guess I don't have any > displays reporting invalid EDIDs to test with. Hopefully this will > help: It doesn't happen all the time. Looks like my EDID is only invalid after a reset needed to resolve an s2ram crash in the adv7511 driver... > https://lore.kernel.org/r/20211004092100.1.Ic90a5ebd44c75db963112be167a03cc96f9fb249@changeid/ Thanks for the quick fix! 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: [PATCH] drm/edid: Fix crash with zero/invalid EDID
Hi Douglas, On Mon, Oct 4, 2021 at 6:22 PM Douglas Anderson wrote: > In the commit bac9c2948224 ("drm/edid: Break out reading block 0 of > the EDID") I broke out reading the base block of the EDID to its own > function. Unfortunately, when I did that I messed up the handling when > drm_edid_is_zero() indicated that we had an EDID that was all 0x00 or > when we went through 4 loops and didn't get a valid EDID. Specifically > I needed to pass the broken EDID to connector_bad_edid() but now I was > passing an error-pointer. > > Let's re-jigger things so we can pass the bad EDID in properly. > > Fixes: bac9c2948224 ("drm/edid: Break out reading block 0 of the EDID") > Reported-by: kernel test robot > Reported-by: Geert Uytterhoeven > Signed-off-by: Douglas Anderson The crash is was seeing is gone, so Tested-by: Geert Uytterhoeven 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: [PATCH net-next 2/5] dt-bindings: net: brcm,unimac-mdio: Add asp-v2.0
On Fri, 24 Sep 2021 14:44:48 -0700, Justin Chen wrote: > The ASP 2.0 Ethernet controller uses a brcm unimac. > > Signed-off-by: Justin Chen > Signed-off-by: Florian Fainelli > --- > Documentation/devicetree/bindings/net/brcm,unimac-mdio.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH 2/2] dt-bindings: panel-simple-dsi: add JDI R63452 panel bindings
On Sat, 25 Sep 2021 12:31:33 +0200, Raffaele Tranquillini wrote: > This add the bindings for the JDI FHD_R63452 1080x1920 5.2" LCD DSI > panel used on the Xiaomi Mi 5 smartphone. > > Signed-off-by: Raffaele Tranquillini > --- > .../devicetree/bindings/display/panel/panel-simple-dsi.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring
Re: [PATCH v3 1/2] dt-bindings: add bindings for the Sharp LS060T1SX01 panel
On Sun, 26 Sep 2021 03:10:04 +0300, Dmitry Baryshkov wrote: > Add devicetree bindings for the Sharp LS060T1SX01 6.0" FullHD panel > using NT35695 driver. This panel can be found i.e. in the Dragonboard > Display Adapter bundle. > > Signed-off-by: Dmitry Baryshkov > --- > .../display/panel/sharp,ls060t1sx01.yaml | 56 +++ > 1 file changed, 56 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/sharp,ls060t1sx01.yaml > Reviewed-by: Rob Herring
Re: [PATCH] dt-bindings: display: simple: hardware can use ddc-i2c-bus
On Mon, 27 Sep 2021 23:45:03 +0200, David Heidelberg wrote: > Both hardware and driver can communicate DDC over i2c bus. > > Fixes warnings as: > arch/arm/boot/dts/tegra20-paz00.dt.yaml: panel: 'ddc-i2c-bus' does not match > any of the regexes: 'pinctrl-[0-9]+' > From schema: > /home/runner/work/linux/linux/Documentation/devicetree/bindings/display/panel/panel-simple.yaml > > Signed-off-by: David Heidelberg > --- > .../devicetree/bindings/display/panel/panel-simple.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH 6/6] dt-bindings: gpu: Add ASPEED GFX bindings document
On Tue, 28 Sep 2021 10:57:03 +0800, tommy-huang wrote: > Add ast2600-gfx description for gfx driver. > > Signed-off-by: tommy-huang > --- > Documentation/devicetree/bindings/gpu/aspeed-gfx.txt | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH v3 1/3] dt-bindings: msm: dsi: Add MSM8953 dsi phy
On Tue, 28 Sep 2021 18:49:27 +0530, Sireesh Kodali wrote: > SoCs based on the MSM8953 platform use the 14nm DSI PHY driver > > Signed-off-by: Sireesh Kodali > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH v2] drm/i915: Clean up disabled warnings
On Tue, 14 Sep 2021, Nathan Chancellor wrote: > i915 enables a wider set of warnings with '-Wall -Wextra' then disables > several with cc-disable-warning. If an unknown flag gets added to > KBUILD_CFLAGS when building with clang, all subsequent calls to > cc-{disable-warning,option} will fail, meaning that all of these > warnings do not get disabled [1]. > > A separate series will address the root cause of the issue by not adding > these flags when building with clang [2]; however, the symptom of these > extra warnings appearing can be addressed separately by just removing > the calls to cc-disable-warning, which makes the build ever so slightly > faster because the compiler does not need to be called as much before > building. > > The following warnings are supported by GCC 4.9 and clang 10.0.1, which > are the minimum supported versions of these compilers so the call to > cc-disable-warning is not necessary. Masahiro cleaned this up for the > reset of the kernel in commit 4c8dd95a723d ("kbuild: add some extra > warning flags unconditionally"). > > * -Wmissing-field-initializers > * -Wsign-compare > * -Wtype-limits > * -Wunused-parameter > > -Wunused-but-set-variable was implemented in clang 13.0.0 and > -Wframe-address was implemented in clang 12.0.0 so the > cc-disable-warning calls are kept for these two warnings. > > Lastly, -Winitializer-overrides is clang's version of -Woverride-init, > which is disabled for the specific files that are problematic. clang > added a compatibility alias in clang 8.0.0 so -Winitializer-overrides > can be removed. > > [1]: https://lore.kernel.org/r/202108210311.cbtcgoul-...@intel.com/ > [2]: https://lore.kernel.org/r/20210824022640.2170859-1-nat...@kernel.org/ > > Reviewed-by: Nick Desaulniers > Signed-off-by: Nathan Chancellor Thanks for the patch, and sorry for the delay. Exceptionally pushed to drm-intel-gt-next instead of drm-intel-next because some of the dependencies such as 43192617f781 ("drm/i915: Enable -Wsometimes-uninitialized") were queued there too. BR, Jani. > --- > > v1 -> v2: > https://lore.kernel.org/r/20210824232237.2085342-1-nat...@kernel.org/ > > * Rebase on drm-intel-gt-next now that the prerequisite patch series has > been merged: https://lore.kernel.org/r/87wnnj13t5@intel.com/ > > * Add Nick's reviewed-by tag. > > drivers/gpu/drm/i915/Makefile | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index c584188aa15a..fd99374583d5 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -13,13 +13,11 @@ > # will most likely get a sudden build breakage... Hopefully we will fix > # new warnings before CI updates! > subdir-ccflags-y := -Wall -Wextra > -subdir-ccflags-y += $(call cc-disable-warning, unused-parameter) > -subdir-ccflags-y += $(call cc-disable-warning, type-limits) > -subdir-ccflags-y += $(call cc-disable-warning, missing-field-initializers) > +subdir-ccflags-y += -Wno-unused-parameter > +subdir-ccflags-y += -Wno-type-limits > +subdir-ccflags-y += -Wno-missing-field-initializers > +subdir-ccflags-y += -Wno-sign-compare > subdir-ccflags-y += $(call cc-disable-warning, unused-but-set-variable) > -# clang warnings > -subdir-ccflags-y += $(call cc-disable-warning, sign-compare) > -subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides) > subdir-ccflags-y += $(call cc-disable-warning, frame-address) > subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror > > > base-commit: 43192617f7816bb74584c1df06f57363afd15337 -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 10/10] drm/i915: Add privacy-screen support (v2)
On Mon, Oct 04, 2021 at 06:02:21PM +0200, Hans de Goede wrote: > Hi, > > On 10/4/21 5:37 PM, Ville Syrjälä wrote: > > On Sat, Oct 02, 2021 at 06:36:18PM +0200, Hans de Goede wrote: > >> Add support for eDP panels with a built-in privacy screen using the > >> new drm_privacy_screen class. > >> > >> Changes in v2: > >> - Call drm_connector_update_privacy_screen() from > >> intel_enable_ddi_dp() / intel_ddi_update_pipe_dp() instead of adding a > >> for_each_new_connector_in_state() loop to intel_atomic_commit_tail() > >> - Move the probe-deferral check to the intel_modeset_probe_defer() helper > >> > >> Signed-off-by: Hans de Goede > >> --- > >> drivers/gpu/drm/i915/display/intel_atomic.c | 1 + > >> drivers/gpu/drm/i915/display/intel_ddi.c | 3 +++ > >> drivers/gpu/drm/i915/display/intel_display.c | 10 ++ > >> drivers/gpu/drm/i915/display/intel_dp.c | 10 ++ > >> 4 files changed, 24 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > >> b/drivers/gpu/drm/i915/display/intel_atomic.c > >> index b4e7ac51aa31..a62550711e98 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_atomic.c > >> +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > >> @@ -139,6 +139,7 @@ int intel_digital_connector_atomic_check(struct > >> drm_connector *conn, > >>new_conn_state->base.picture_aspect_ratio != > >> old_conn_state->base.picture_aspect_ratio || > >>new_conn_state->base.content_type != > >> old_conn_state->base.content_type || > >>new_conn_state->base.scaling_mode != > >> old_conn_state->base.scaling_mode || > >> + new_conn_state->base.privacy_screen_sw_state != > >> old_conn_state->base.privacy_screen_sw_state || > >>!drm_connector_atomic_hdr_metadata_equal(old_state, new_state)) > >>crtc_state->mode_changed = true; > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > >> b/drivers/gpu/drm/i915/display/intel_ddi.c > >> index 51cd0420e00e..e4496c830a35 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_ddi.c > >> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > >> @@ -25,6 +25,7 @@ > >> * > >> */ > >> > >> +#include > >> #include > >> > >> #include "i915_drv.h" > >> @@ -3022,6 +3023,7 @@ static void intel_enable_ddi_dp(struct > >> intel_atomic_state *state, > >>if (port == PORT_A && DISPLAY_VER(dev_priv) < 9) > >>intel_dp_stop_link_train(intel_dp, crtc_state); > >> > >> + drm_connector_update_privacy_screen(conn_state); > >>intel_edp_backlight_on(crtc_state, conn_state); > >> > >>if (!dig_port->lspcon.active || dig_port->dp.has_hdmi_sink) > >> @@ -3247,6 +3249,7 @@ static void intel_ddi_update_pipe_dp(struct > >> intel_atomic_state *state, > >>intel_drrs_update(intel_dp, crtc_state); > >> > >>intel_backlight_update(state, encoder, crtc_state, conn_state); > >> + drm_connector_update_privacy_screen(conn_state); > >> } > >> > >> void intel_ddi_update_pipe(struct intel_atomic_state *state, > >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c > >> b/drivers/gpu/drm/i915/display/intel_display.c > >> index e67f3207ba54..9a5dbe51458d 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_display.c > >> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> @@ -42,6 +42,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> #include > >> #include > >> @@ -12693,6 +12694,8 @@ void intel_modeset_driver_remove_nogem(struct > >> drm_i915_private *i915) > >> > >> bool intel_modeset_probe_defer(struct pci_dev *pdev) > >> { > >> + struct drm_privacy_screen *privacy_screen; > >> + > >>/* > >> * apple-gmux is needed on dual GPU MacBook Pro > >> * to probe the panel if we're the inactive GPU. > >> @@ -12700,6 +12703,13 @@ bool intel_modeset_probe_defer(struct pci_dev > >> *pdev) > >>if (vga_switcheroo_client_probe_defer(pdev)) > >>return true; > >> > >> + /* If the LCD panel has a privacy-screen, wait for it */ > >> + privacy_screen = drm_privacy_screen_get(&pdev->dev, NULL); > >> + if (IS_ERR(privacy_screen) && PTR_ERR(privacy_screen) == -EPROBE_DEFER) > >> + return true; > >> + > >> + drm_privacy_screen_put(privacy_screen); > >> + > >>return false; > >> } > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > >> b/drivers/gpu/drm/i915/display/intel_dp.c > >> index 74a657ae131a..91207310dc0d 100644 > >> --- a/drivers/gpu/drm/i915/display/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c > >> @@ -37,6 +37,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> #include > >> > >> #include "g4x_dp.h" > >> @@ -4808,6 +4809,7 @@ static bool intel_edp_init_connector(struct intel_dp > >> *intel_dp, > >>struct drm_connector *connector = &intel_connector->base; > >>struct drm_display_mode *fixed_mode = NULL; > >>struct drm_display_mode *downclock_mode = NULL; > >> + struct drm_privacy_screen *pr
[PATCH 00/10] backlight: qcom-wled: fix and solidify handling of enabled-strings
This patchset fixes WLED's handling of enabled-strings: besides some cleanup it is now actually possible to specify a non-contiguous array of enabled strings (not necessarily starting at zero) and the values from DT are now validated to prevent possible unexpected out-of-bounds register and array element accesses. Off-by-one mistakes in the maximum number of strings, also causing out-of-bounds access, have been addressed as well. Marijn Suijten (10): backlight: qcom-wled: Pass number of elements to read to read_u32_array backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion backlight: qcom-wled: Override num-strings when enabled-strings is set backlight: qcom-wled: Validate enabled string indices in DT backlight: qcom-wled: Fix off-by-one maximum with default num_strings backlight: qcom-wled: Remove unnecessary 4th default string in wled3 backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5 backlight: qcom-wled: Remove unnecessary double whitespace backlight: qcom-wled: Consistently use enabled-strings in set_brightness backlight: qcom-wled: Consider enabled_strings in autodetection drivers/video/backlight/qcom-wled.c | 88 ++--- 1 file changed, 55 insertions(+), 33 deletions(-) -- 2.33.0
[PATCH 02/10] backlight: qcom-wled: Use cpu_to_le16 macro to perform conversion
The kernel already provides appropriate primitives to perform endianness conversion which should be used in favour of manual bit-wrangling. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 25 +++-- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 6af808af2328..9927ed98944a 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -231,14 +231,14 @@ struct wled { static int wled3_set_brightness(struct wled *wled, u16 brightness) { int rc, i; - u8 v[2]; + u16 v; - v[0] = brightness & 0xff; - v[1] = (brightness >> 8) & 0xf; + v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + - WLED3_SINK_REG_BRIGHT(i), v, 2); + WLED3_SINK_REG_BRIGHT(i), + &v, sizeof(v)); if (rc < 0) return rc; } @@ -249,19 +249,18 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness) static int wled4_set_brightness(struct wled *wled, u16 brightness) { int rc, i; - u16 low_limit = wled->max_brightness * 4 / 1000; - u8 v[2]; + u16 v, low_limit = wled->max_brightness * 4 / 1000; /* WLED4's lower limit of operation is 0.4% */ if (brightness > 0 && brightness < low_limit) brightness = low_limit; - v[0] = brightness & 0xff; - v[1] = (brightness >> 8) & 0xf; + v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->sink_addr + - WLED4_SINK_REG_BRIGHT(i), v, 2); + WLED4_SINK_REG_BRIGHT(i), + &v, sizeof(v)); if (rc < 0) return rc; } @@ -272,22 +271,20 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) static int wled5_set_brightness(struct wled *wled, u16 brightness) { int rc, offset; - u16 low_limit = wled->max_brightness * 1 / 1000; - u8 v[2]; + u16 v, low_limit = wled->max_brightness * 1 / 1000; /* WLED5's lower limit is 0.1% */ if (brightness < low_limit) brightness = low_limit; - v[0] = brightness & 0xff; - v[1] = (brightness >> 8) & 0x7f; + v = cpu_to_le16(brightness & WLED5_SINK_REG_BRIGHT_MAX_15B); offset = (wled->cfg.mod_sel == MOD_A) ? WLED5_SINK_REG_MOD_A_BRIGHTNESS_LSB : WLED5_SINK_REG_MOD_B_BRIGHTNESS_LSB; rc = regmap_bulk_write(wled->regmap, wled->sink_addr + offset, - v, 2); + &v, sizeof(v)); return rc; } -- 2.33.0
[PATCH 05/10] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
When not specifying num-strings in the DT the default is used, but +1 is added to it which turns wled3 into 4 and wled4/5 into 5 strings instead of 3 and 4 respectively, causing out of bounds reads and register read/writes. This +1 exists for a deficiency in the DT parsing code, and is simply omitted entirely - solving this oob issue - by allowing one extra iteration of the wled_var_cfg function parsing this particular property. Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 27e8949c7922..66ce77ee3099 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1255,17 +1255,17 @@ static const struct wled_var_cfg wled5_ovp_cfg = { static u32 wled3_num_strings_values_fn(u32 idx) { - return idx + 1; + return idx; } static const struct wled_var_cfg wled3_num_strings_cfg = { .fn = wled3_num_strings_values_fn, - .size = 3, + .size = 4, /* [0, 3] */ }; static const struct wled_var_cfg wled4_num_strings_cfg = { .fn = wled3_num_strings_values_fn, - .size = 4, + .size = 5, /* [0, 4] */ }; static u32 wled3_switch_freq_values_fn(u32 idx) @@ -1520,8 +1520,6 @@ static int wled_configure(struct wled *wled) *bool_opts[i].val_ptr = true; } - cfg->num_strings = cfg->num_strings + 1; - string_len = of_property_count_elems_of_size(dev->of_node, "qcom,enabled-strings", sizeof(u32)); -- 2.33.0
[PATCH 06/10] backlight: qcom-wled: Remove unnecessary 4th default string in wled3
The previous commit improves num_strings parsing to not go over the maximum of 3 strings for wled3 anymore. Likewise this default index for a hypothetical 4th string is invalid and could access registers that are not mapped to the desired purpose. Removing this value gets rid of undesired confusion and avoids the possibility of accessing registers at this offset even if the 4th array element is used by accident. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 66ce77ee3099..9ec1bdd374d2 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -946,7 +946,7 @@ static const struct wled_config wled3_config_defaults = { .cs_out_en = false, .ext_gen = false, .cabc = false, - .enabled_strings = {0, 1, 2, 3}, + .enabled_strings = {0, 1, 2}, }; static int wled4_setup(struct wled *wled) -- 2.33.0
[PATCH 01/10] backlight: qcom-wled: Pass number of elements to read to read_u32_array
of_property_read_u32_array takes the number of elements to read as last argument. This does not always need to be 4 (sizeof(u32)) but should instead be the size of the array in DT as read just above with of_property_count_elems_of_size. To not make such an error go unnoticed again the driver now bails accordingly when of_property_read_u32_array returns an error. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index d094299c2a48..6af808af2328 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1528,11 +1528,18 @@ static int wled_configure(struct wled *wled) string_len = of_property_count_elems_of_size(dev->of_node, "qcom,enabled-strings", sizeof(u32)); - if (string_len > 0) - of_property_read_u32_array(dev->of_node, + if (string_len > 0) { + rc = of_property_read_u32_array(dev->of_node, "qcom,enabled-strings", wled->cfg.enabled_strings, - sizeof(u32)); + string_len); + if (rc) { + dev_err(dev, "Failed to read %d elements from " + "qcom,enabled-strings: %d\n", + string_len, rc); + return -EINVAL; + } + } return 0; } -- 2.33.0
[PATCH 03/10] backlight: qcom-wled: Override num-strings when enabled-strings is set
DT-bindings do not specify num-strings as mandatory property, yet it is required to be specified even if enabled-strings is used. The length of that property-array should already be enough to determine exactly which and how many strings to enable. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 9927ed98944a..29910e603c42 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1536,6 +1536,8 @@ static int wled_configure(struct wled *wled) string_len, rc); return -EINVAL; } + + cfg->num_strings = string_len; } return 0; -- 2.33.0
[PATCH 04/10] backlight: qcom-wled: Validate enabled string indices in DT
The strings passed in DT may possibly cause out-of-bounds register accesses and should be validated before use. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 14 ++ 1 file changed, 14 insertions(+) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 29910e603c42..27e8949c7922 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1526,6 +1526,12 @@ static int wled_configure(struct wled *wled) "qcom,enabled-strings", sizeof(u32)); if (string_len > 0) { + if (string_len > wled->max_string_count) { + dev_err(dev, "Cannot have more than %d strings\n", + wled->max_string_count); + return -EINVAL; + } + rc = of_property_read_u32_array(dev->of_node, "qcom,enabled-strings", wled->cfg.enabled_strings, @@ -1537,6 +1543,14 @@ static int wled_configure(struct wled *wled) return -EINVAL; } + for (i = 0; i < string_len; ++i) { + if (wled->cfg.enabled_strings[i] >= wled->max_string_count) { + dev_err(dev, "qcom,enabled-strings index %d at %d is out of bounds\n", + wled->cfg.enabled_strings[i], i); + return -EINVAL; + } + } + cfg->num_strings = string_len; } -- 2.33.0
[PATCH 09/10] backlight: qcom-wled: Consistently use enabled-strings in set_brightness
The hardware is capable of controlling any non-contiguous sequence of LEDs specified in the DT using qcom,enabled-strings as u32 array, and this also follows from the DT-bindings documentation. The numbers specified in this array represent indices of the LED strings that are to be enabled and disabled. Its value is appropriately used to setup and enable string modules, but completely disregarded in the set_brightness paths which only iterate over the number of strings linearly. Take an example where only string 2 is enabled with qcom,enabled_strings=<2>: this string is appropriately enabled but subsequent brightness changes would have only touched the zero'th brightness register because num_strings is 1 here. This is simply addressed by looking up the string for this index in the enabled_strings array just like the other codepaths that iterate over num_strings. Fixes: 775d2ffb4af6 ("backlight: qcom-wled: Restructure the driver for WLED3") Fixes: 03b2b5e86986 ("backlight: qcom-wled: Add support for WLED4 peripheral") Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index dbe503007ada..c0b8d10c20a2 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -237,7 +237,7 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness) for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + - WLED3_SINK_REG_BRIGHT(i), + WLED3_SINK_REG_BRIGHT(wled->cfg.enabled_strings[i]), &v, sizeof(v)); if (rc < 0) return rc; @@ -259,7 +259,7 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->sink_addr + - WLED4_SINK_REG_BRIGHT(i), + WLED4_SINK_REG_BRIGHT(wled->cfg.enabled_strings[i]), &v, sizeof(v)); if (rc < 0) return rc; -- 2.33.0
[PATCH 08/10] backlight: qcom-wled: Remove unnecessary double whitespace
Remove redundant spaces inside for loop conditions. No other double spaces were found that are not part of indentation with `[^\s] `. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index f143b2563bfe..dbe503007ada 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -235,7 +235,7 @@ static int wled3_set_brightness(struct wled *wled, u16 brightness) v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); - for (i = 0; i < wled->cfg.num_strings; ++i) { + for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->ctrl_addr + WLED3_SINK_REG_BRIGHT(i), &v, sizeof(v)); @@ -257,7 +257,7 @@ static int wled4_set_brightness(struct wled *wled, u16 brightness) v = cpu_to_le16(brightness & WLED3_SINK_REG_BRIGHT_MAX); - for (i = 0; i < wled->cfg.num_strings; ++i) { + for (i = 0; i < wled->cfg.num_strings; ++i) { rc = regmap_bulk_write(wled->regmap, wled->sink_addr + WLED4_SINK_REG_BRIGHT(i), &v, sizeof(v)); -- 2.33.0
[PATCH 07/10] backlight: qcom-wled: Provide enabled_strings default for wled 4 and 5
Only wled 3 sets a sensible default that allows operating this driver with just qcom,num-strings in the DT; wled 4 and 5 require qcom,enabled-strings to be provided otherwise enabled_strings remains zero-initialized, resuling in every string-specific register write (currently only the setup and config functions, brightness follows in a future patch) to only configure the zero'th string multiple times. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index 9ec1bdd374d2..f143b2563bfe 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -1077,6 +1077,7 @@ static const struct wled_config wled4_config_defaults = { .cabc = false, .external_pfet = false, .auto_detection_enabled = false, + .enabled_strings = {0, 1, 2, 3}, }; static int wled5_setup(struct wled *wled) @@ -1190,6 +1191,7 @@ static const struct wled_config wled5_config_defaults = { .cabc = false, .external_pfet = false, .auto_detection_enabled = false, + .enabled_strings = {0, 1, 2, 3}, }; static const u32 wled3_boost_i_limit_values[] = { -- 2.33.0
[PATCH 10/10] backlight: qcom-wled: Consider enabled_strings in autodetection
Following the previous commit using enabled_strings in set_brightness, enabled_strings is now also used in the autodetection path for consistent behaviour: when a list of strings is specified in DT only those strings will be probed for autodetection, analogous to how the number of strings that need to be probed is already bound by qcom,num-strings. Signed-off-by: Marijn Suijten Reviewed-by: AngeloGioacchino Del Regno --- drivers/video/backlight/qcom-wled.c | 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/drivers/video/backlight/qcom-wled.c b/drivers/video/backlight/qcom-wled.c index c0b8d10c20a2..2c49f5d8dc26 100644 --- a/drivers/video/backlight/qcom-wled.c +++ b/drivers/video/backlight/qcom-wled.c @@ -569,7 +569,7 @@ static irqreturn_t wled_short_irq_handler(int irq, void *_wled) static void wled_auto_string_detection(struct wled *wled) { - int rc = 0, i, delay_time_us; + int rc = 0, i, j, delay_time_us; u32 sink_config = 0; u8 sink_test = 0, sink_valid = 0, val; bool fault_set; @@ -616,14 +616,15 @@ static void wled_auto_string_detection(struct wled *wled) /* Iterate through the strings one by one */ for (i = 0; i < wled->cfg.num_strings; i++) { - sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + i)); + j = wled->cfg.enabled_strings[i]; + sink_test = BIT((WLED4_SINK_REG_CURR_SINK_SHFT + j)); /* Enable feedback control */ rc = regmap_write(wled->regmap, wled->ctrl_addr + - WLED3_CTRL_REG_FEEDBACK_CONTROL, i + 1); + WLED3_CTRL_REG_FEEDBACK_CONTROL, j + 1); if (rc < 0) { dev_err(wled->dev, "Failed to enable feedback for SINK %d rc = %d\n", - i + 1, rc); + j + 1, rc); goto failed_detect; } @@ -632,7 +633,7 @@ static void wled_auto_string_detection(struct wled *wled) WLED4_SINK_REG_CURR_SINK, sink_test); if (rc < 0) { dev_err(wled->dev, "Failed to configure SINK %d rc=%d\n", - i + 1, rc); + j + 1, rc); goto failed_detect; } @@ -659,7 +660,7 @@ static void wled_auto_string_detection(struct wled *wled) if (fault_set) dev_dbg(wled->dev, "WLED OVP fault detected with SINK %d\n", - i + 1); + j + 1); else sink_valid |= sink_test; @@ -699,15 +700,16 @@ static void wled_auto_string_detection(struct wled *wled) /* Enable valid sinks */ if (wled->version == 4) { for (i = 0; i < wled->cfg.num_strings; i++) { + j = wled->cfg.enabled_strings[i]; if (sink_config & - BIT(WLED4_SINK_REG_CURR_SINK_SHFT + i)) + BIT(WLED4_SINK_REG_CURR_SINK_SHFT + j)) val = WLED4_SINK_REG_STR_MOD_MASK; else /* Disable modulator_en for unused sink */ val = 0; rc = regmap_write(wled->regmap, wled->sink_addr + - WLED4_SINK_REG_STR_MOD_EN(i), val); + WLED4_SINK_REG_STR_MOD_EN(j), val); if (rc < 0) { dev_err(wled->dev, "Failed to configure MODULATOR_EN rc=%d\n", rc); -- 2.33.0
[PULL] drm-intel-next
Hi Dave and Daniel, Here goes an accumulated pull request. A special highlight to the ADL-P (XE_LPD) and DG2 display support preparation and on a big clean-up in the display portion of the driver. Here goes drm-intel-next-2021-10-04: Cross-subsystem Changes: - fbdev/efifb: Release PCI device's runtime PM ref during FB destr\ oy (Imre) i915 Core Driver Changes: - Only access SFC_DONE in media when not fused off for graphics 12 and newer. - Double Memory latency values from pcode for DG2 (Matt Roper) - ADL-S PCI ID update (Tejas) - New DG1 PCI ID (Jose) - Fix regression with uncore refactoring (Dave) i915 Display Changes: - ADL-P display (XE_LPD) fixes and updates (Ankit, Jani, Matt Roper, Anusham, Jose, Imre, Vandita) - DG2 display fixes (Ankit, Jani) - Expand PCH_CNP tweaked display workaround to all newer displays (Anshuman) - General display simplifications and clean-ups (Jani, Swati, Jose, Ville) - PSR Clean-ups, dropping support for BDW/HSD and enable PSR2 selective fetch by default (Jose, Gwan-gyeong) - Nuke ORIGIN_GTT (Jose) - Return proper DPRX link training result (Lee) - FBC related refactor and fixes (Ville) - Yet another attempt to solve the fast+narrow vs slow+wide eDP link training (Kai-Heng) - DP 2.0 preparation work (Jani) - Silence __iomem sparse warn (Ville) - Clean up DPLL stuff (Ville) - Fix various dp/edp max rates (Matt Atwood, Animesh, Jani) - Remove VBT ddi_port_info caching (Jani) - DSI driver improvements (Lee) - HDCP fixes (Juston) - Associate ACPI connector nodes with connector entries (Heikki) - Add support for out-of-bound hotplug events (Hans) - VESA vendor block and drm/i915 MSO use of it (Jani) - Fixes for bigjoiner (Ville) - Update memory bandwidth parameters (RK) - DMC related fixes (Chris, Jose) - HDR related fixes and improvements (Tejas) - g4x/vlv/chv CxSR/wm fixes/cleanups (Ville) - Use BIOS provided value for RKL Audio's HDA link (Kai-Heng) - Fix the dsc check while selecting min_cdclk (Vandita) - Split and constify vtable (Dave) - Add ww context to intel_dpt_pin (Maarten) - Fix bdb version check (Lukasz) - DP per-lane drive settings prep work and other DP fixes (Ville) Thanks, Rodrigo. The following changes since commit 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f: Linux 5.15-rc1 (2021-09-12 16:28:37 -0700) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-next-2021-10-04 for you to fetch changes up to 104c1b3d6fb6a794babd5e2ffd6a5183b5a3d6c7: drm/i915: Allow per-lane drive settings with LTTPRs (2021-10-04 13:04:36 +0300) Cross-subsystem Changes: - fbdev/efifb: Release PCI device's runtime PM ref during FB destr\ oy (Imre) i915 Core Driver Changes: - Only access SFC_DONE in media when not fused off for graphics 12 and newer. - Double Memory latency values from pcode for DG2 (Matt Roper) - ADL-S PCI ID update (Tejas) - New DG1 PCI ID (Jose) - Fix regression with uncore refactoring (Dave) i915 Display Changes: - ADL-P display (XE_LPD) fixes and updates (Ankit, Jani, Matt Roper, Anusham, Jose, Imre, Vandita) - DG2 display fixes (Ankit, Jani) - Expand PCH_CNP tweaked display workaround to all newer displays (Anshuman) - General display simplifications and clean-ups (Jani, Swati, Jose, Ville) - PSR Clean-ups, dropping support for BDW/HSD and enable PSR2 selective fetch by default (Jose, Gwan-gyeong) - Nuke ORIGIN_GTT (Jose) - Return proper DPRX link training result (Lee) - FBC related refactor and fixes (Ville) - Yet another attempt to solve the fast+narrow vs slow+wide eDP link training (Kai-Heng) - DP 2.0 preparation work (Jani) - Silence __iomem sparse warn (Ville) - Clean up DPLL stuff (Ville) - Fix various dp/edp max rates (Matt Atwood, Animesh, Jani) - Remove VBT ddi_port_info caching (Jani) - DSI driver improvements (Lee) - HDCP fixes (Juston) - Associate ACPI connector nodes with connector entries (Heikki) - Add support for out-of-bound hotplug events (Hans) - VESA vendor block and drm/i915 MSO use of it (Jani) - Fixes for bigjoiner (Ville) - Update memory bandwidth parameters (RK) - DMC related fixes (Chris, Jose) - HDR related fixes and improvements (Tejas) - g4x/vlv/chv CxSR/wm fixes/cleanups (Ville) - Use BIOS provided value for RKL Audio's HDA link (Kai-Heng) - Fix the dsc check while selecting min_cdclk (Vandita) - Split and constify vtable (Dave) - Add ww context to intel_dpt_pin (Maarten) - Fix bdb version check (Lukasz) - DP per-lane drive settings prep work and other DP fixes (Ville) Animesh Manna (3): drm/i915/dg2: UHBR tables added for pll programming drm/i915/dp: fix EHL/JSL max source rates calculation drm/i915/dp: fix for ADL_P/S dp/edp max source rates Ankit Nautiyal (2): drm/i915/display: Fix the 12 BPC bits for PIPE_MISC reg drm/i915/dg2: Configure PCON in DP pre-enable path Anshuman Gupta (1): drm/i915: Twea