Re: Re: drm/debugfs: Drop conditionals around of_node pointers
Hi,
> -原始邮件-
> 发件人: "Maxime Ripard"
> 发送时间: 2024-04-29 19:30:24 (星期一)
> 收件人: "Sui Jingfeng"
> 抄送: "Sui Jingfeng" , "Maarten Lankhorst"
> , "Thomas Zimmermann"
> , "David Airlie" , "Daniel Vetter"
> , "Douglas Anderson" , "Laurent
> Pinchart" , "Biju Das"
> , dri-devel@lists.freedesktop.org,
> linux-ker...@vger.kernel.org
> 主题: Re: drm/debugfs: Drop conditionals around of_node pointers
>
> On Sun, Apr 28, 2024 at 04:52:13PM +0800, Sui Jingfeng wrote:
> > ping
> >
> > 在 2024/3/22 06:22, Sui Jingfeng 写道:
> > > Having conditional around the of_node pointer of the drm_bridge structure
> > > turns out to make driver code use ugly #ifdef blocks.
>
> The code being ugly is an opinion, what problem is it causing exactly?
>
> > Drop the conditionals to simplify debugfs.
>
> What does it simplifies?
>
> > >
> > > Fixes: d8dfccde2709 ("drm/bridge: Drop conditionals around of_node
> > > pointers")
> > > Signed-off-by: Sui Jingfeng
>
> Why do we want to backport that patch to stable?
My commit message is written based on commit of d8dfccde2709
$ git show c9e358dfc4a8
This patch is based on commit c9e358dfc4a8 ("driver-core: remove
conditionals around devicetree pointers").
Having conditional around the of_node pointer of the drm_bridge
structure turns out to make driver code use ugly #ifdef blocks. Drop the
conditionals to simplify drivers. While this slightly increases the size
of struct drm_bridge on non-OF system, the number of bridges used today
and foreseen tomorrow on those systems is very low, so this shouldn't be
an issue.
So drop #if conditionals by adding struct device_node forward declaration.
> Maxime
I'm just start to contribute by mimic other people's tone, there seems no need
to over read.
Re: [PATCH v2 1/2] drm/nouveau/firmware: Fix SG_DEBUG error with nvkm_firmware_ctor()
> V2: > * Fixup explanation as the prior one was bogus For v2 of both patches, Reviewed-by: Dave Airlie Please apply to drm-misc-fixes Dave.
Re: [PATCH] drm/panel: ili9341: Remove a superfluous else after return
On 4/29/2024 10:12 AM, Sui Jingfeng wrote: The else clause after the ruturn clause is not useful. Hi Sui, Spelling nit: ruturn --> return Besides that, Reviewed-by: Jessica Zhang Thanks, Jessica Zhang Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c index 3574681891e8..433572c4caf9 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c @@ -722,7 +722,8 @@ static int ili9341_probe(struct spi_device *spi) if (!strcmp(id->name, "sf-tc240t-9370-t")) return ili9341_dpi_probe(spi, dc, reset); - else if (!strcmp(id->name, "yx240qv29")) + + if (!strcmp(id->name, "yx240qv29")) return ili9341_dbi_probe(spi, dc, reset); return -1; -- 2.34.1
Re: [PATCH v2 1/8] drm/mipi-dsi: Fix theoretical int overflow in mipi_dsi_dcs_write_seq()
Hi,
On Fri, Apr 26, 2024 at 11:22 PM Sam Ravnborg wrote:
>
> On Sat, Apr 27, 2024 at 04:44:33AM +0300, Dmitry Baryshkov wrote:
> > On Sat, 27 Apr 2024 at 02:59, Douglas Anderson
> > wrote:
> > >
> > > The mipi_dsi_dcs_write_seq() macro makes a call to
> > > mipi_dsi_dcs_write_buffer() which returns a type ssize_t. The macro
> > > then stores it in an int and checks to see if it's negative. This
> > > could theoretically be a problem if "ssize_t" is larger than "int".
> > >
> > > To see the issue, imagine that "ssize_t" is 32-bits and "int" is
> > > 16-bits, you could see a problem if there was some code out there that
> > > looked like:
> > >
> > > mipi_dsi_dcs_write_seq(dsi, cmd, <32767 bytes as arguments>);
> > >
> > > ...since we'd get back that 32768 bytes were transferred and 32768
> > > stored in a 16-bit int would look negative.
> > >
> > > Though there are no callsites where we'd actually hit this (even if
> > > "int" was only 16-bit), it's cleaner to make the types match so let's
> > > fix it.
> > >
> > > Fixes: 2a9e9daf7523 ("drm/mipi-dsi: Introduce mipi_dsi_dcs_write_seq
> > > macro")
> > > Signed-off-by: Douglas Anderson
> > > ---
> > >
> > > Changes in v2:
> > > - New
> > >
> > > include/drm/drm_mipi_dsi.h | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> > > index 82b1cc434ea3..b3576be22bfa 100644
> > > --- a/include/drm/drm_mipi_dsi.h
> > > +++ b/include/drm/drm_mipi_dsi.h
> > > @@ -337,12 +337,12 @@ int
> > > mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> > > do {
> > > \
> > > static const u8 d[] = { cmd, seq };
> > > \
> > > struct device *dev = >dev;
> > > \
> > > - int ret;
> > > \
> > > + ssize_t ret;
> > > \
> > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));
> > > \
> > > if (ret < 0) {
> > > \
> > > dev_err_ratelimited(
> > > \
> > > dev, "sending command %#02x failed:
> > > %d\n", \
> > > - cmd, ret);
> > > \
> > > + cmd, (int)ret);
> > > \
> >
> > Please consider using %zd instead
>
> Hi Douglas,
> please consider the above for all the pathces, there are more places
> where a cast can be dropped.
Sure, I'll change in the next version. I personally prefer the %d with
an "int" type because technically we're printing an error code and
errors are int-sized. ...but I don't feel strongly and I guess there's
a tiny chance some bug in the code could lead to a negative value
that's more useful as 64-bits than 32-bits. ;-)
I will note that I will still need a cast in some of the later patches
for "%*ph" since, I believe, the size passed for the "*" in a printf
format string is defined to be an int, not a size_t or ssize_t.
-Doug
[linux-next:master] BUILD REGRESSION b0a2c79c6f3590b74742cbbc76687014d47972d8
tree/branch:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
branch HEAD: b0a2c79c6f3590b74742cbbc76687014d47972d8 Add linux-next specific
files for 20240429
Error/Warning reports:
https://lore.kernel.org/oe-kbuild-all/20240429.kkvw8mvg-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202404292317.uk7gvrsc-...@intel.com
https://lore.kernel.org/oe-kbuild-all/202404300248.dblz5vz7-...@intel.com
Error/Warning: (recently discovered and may have been fixed)
arch/arm64/boot/dts/ti/k3-am62a7-sk.dtb: syscon@4300: 'syscon@4008',
'syscon@4018' do not match any of the regexes: '^chipid@[0-9a-f]+$',
'^clock-controller@[0-9a-f]+$', '^mux-controller@[0-9a-f]+$', 'phy@[0-9a-f]+$',
'pinctrl-[0-9]+'
make[4]: *** No rule to make target
'arch/arm64/boot/dts/ti/k3-j784s4-evm-usxgmii-exp1-exp2.dtb', needed by
'arch/arm64/boot/dts/ti/'.
make[4]: *** No rule to make target
'arch/arm64/boot/dts/ti/k3-j784s4-evm-usxgmii-exp1-exp2.dtb', needed by
'arch/arm64/boot/dts/ti/dtbs-list'.
Unverified Error/Warning (likely false positive, please contact us if
interested):
{standard input}:2000: Error: unknown pseudo-op: `.lfe4886'
Error/Warning ids grouped by kconfigs:
gcc_recent_errors
|-- alpha-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arc-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arc-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arc-randconfig-001-20240429
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arm-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arm-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arm64-defconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- arm64-randconfig-r131-20230824
| `--
make:No-rule-to-make-target-arch-arm64-boot-dts-ti-k3-j784s4-evm-usxgmii-exp1-exp2.dtb-needed-by-arch-arm64-boot-dts-ti-dtbs-list-.
|-- csky-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- csky-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- i386-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- i386-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- i386-buildonly-randconfig-005-20240429
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- i386-randconfig-141-20240429
| |--
drivers-gpu-drm-bridge-cadence-cdns-mhdp8546-core.c-cdns_mhdp_atomic_enable()-warn:inconsistent-returns-mhdp-link_mutex-.
| |--
drivers-pwm-core.c-pwm_put()-warn:variable-dereferenced-before-check-pwm-(see-line-)
| `--
drivers-usb-typec-ucsi-ucsi.c-ucsi_get_pd_caps()-warn:passing-zero-to-ERR_PTR
|-- loongarch-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- loongarch-randconfig-002-20240429
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- m68k-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- m68k-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- microblaze-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- microblaze-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- mips-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- nios2-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- nios2-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- nios2-randconfig-001-20240429
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- nios2-randconfig-002-20240429
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- openrisc-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- parisc-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- parisc-allyesconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- parisc-randconfig-002-20240429
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- powerpc-allmodconfig
| `-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
|-- powerpc-randconfig-002-20240429
| |-- drivers-usb-dwc3-core.c:warning:variable-hw_mode-set-but-not-used
| |--
powerpc-linux-ld:drivers-gpu-drm-amd-amdgpu-..-display-dc-dml-calcs-dcn_calc_auto.o-uses-hard-float-drivers-gpu-drm-amd-amdgpu-..-display-amdgpu_dm-amdgpu_dm_helpers.o-uses-soft-float
| |--
powerpc-linux-ld:drivers-gpu-drm-amd-amdgpu-..-display-dc-dml-calcs-dcn_calc_math.o-uses-hard-float-drivers-gpu-drm-amd-amdgpu-..-display
AMD Dell m18 r1/7600XT Laptop Issue dcn31_panel_cntl_construct+0x49/0x60 [amdgpu]
is this a bug or a support issue for this laptop? [4.739919] [ cut here ] [4.739920] WARNING: CPU: 17 PID: 551 at drivers/gpu/drm/amd/amdgpu/../display/dc/dcn31/dcn31_panel_cntl.c:172 dcn31_panel_cntl_construct+0x49/0x60 [amdgpu] [4.740183] Modules linked in: amdgpu(+) video amdxcp i2c_algo_bit drm_ttm_helper ttm drm_exec gpu_sched drm_suballoc_helper rtsx_pci_sdmmc drm_buddy crct10dif_pclmul crc32_pclmul crc32c_intel mmc_core drm_display_helper polyval_clmulni r8169 nvme ucsi_acpi polyval_generic hid_multitouch nvme_core ghash_clmulni_intel sha512_ssse3 sha256_ssse3 sha1_ssse3 typec_ucsi ccp rtsx_pci sp5100_tco cec realtek nvme_auth typec i2c_hid_acpi wmi i2c_hid serio_raw scsi_dh_rdac scsi_dh_emc scsi_dh_alua fuse i2c_dev [4.740203] CPU: 17 PID: 551 Comm: (udev-worker) Not tainted 6.8.7-300.fc40.x86_64 #1 [4.740205] Hardware name: Alienware Alienware m18 R1 AMD/0XY1HF, BIOS 1.12.0 03/08/2024 [4.740206] RIP: 0010:dcn31_panel_cntl_construct+0x49/0x60 [amdgpu] [4.740363] Code: 57 10 8b 56 0c 85 d2 74 28 83 fa 01 74 23 48 8b 40 10 48 8b 38 48 85 ff 74 04 48 8b 7f 08 48 c7 c6 98 5d 1e c1 e8 17 2f ef fa <0f> 0b ba 0f 00 00 00 89 53 14 5b c3 cc cc cc cc 0f 1f 80 00 00 00 [4.740365] RSP: 0018:96b840ffb3a8 EFLAGS: 00010246 [4.740367] RAX: RBX: 899850d07a40 RCX: 0027 [4.740368] RDX: RSI: 0001 RDI: 89a73da618c0 [4.740369] RBP: 96b840ffb3e8 R08: R09: 96b840ffb160 [4.740370] R10: bd516808 R11: 0003 R12: 96b840ffb43c [4.740371] R13: c10902a0 R14: 96b840ffb7a0 R15: 89984f99f800 [4.740372] FS: 7f9f756d0980() GS:89a73da4() knlGS: [4.740373] CS: 0010 DS: ES: CR0: 80050033 [4.740374] CR2: 7f9f764e5b00 CR3: 000110086000 CR4: 00f50ef0 [4.740375] PKRU: 5554 [4.740376] Call Trace: [4.740378] [4.740379] ? dcn31_panel_cntl_construct+0x49/0x60 [amdgpu] [4.740525] ? __warn+0x81/0x130 [4.740529] ? dcn31_panel_cntl_construct+0x49/0x60 [amdgpu] [4.740663] ? report_bug+0x16f/0x1a0 [4.740666] ? handle_bug+0x3c/0x80 [4.740667] ? exc_invalid_op+0x17/0x70 [4.740669] ? asm_exc_invalid_op+0x1a/0x20 [4.740671] ? dcn31_panel_cntl_construct+0x49/0x60 [amdgpu] [4.740802] ? dcn31_panel_cntl_construct+0x49/0x60 [amdgpu] [4.740930] dcn32_panel_cntl_create+0x37/0x50 [amdgpu] [4.741076] construct_phy+0xac6/0xd30 [amdgpu] [4.741225] link_create+0x1da/0x210 [amdgpu] [4.741359] create_links+0x134/0x420 [amdgpu] [4.741504] dc_create+0x316/0x650 [amdgpu] [4.741645] amdgpu_dm_init.isra.0+0x2d4/0x1fb0 [amdgpu] [4.741803] ? prb_read_valid+0x1b/0x30 [4.741806] ? console_unlock+0x84/0x130 [4.741807] ? __wake_up_klogd.part.0+0x3c/0x60 [4.741809] ? vprintk_emit+0x175/0x2c0 [4.741810] ? dev_printk_emit+0xa3/0xd0 [4.741812] ? __pfx_smu_v13_0_update_pcie_parameters+0x10/0x10 [amdgpu] [4.741952] dm_hw_init+0x12/0x30 [amdgpu] [4.742094] amdgpu_device_init+0x1e9d/0x26d0 [amdgpu] [4.742208] amdgpu_driver_load_kms+0x19/0x190 [amdgpu] [4.742318] amdgpu_pci_probe+0x18b/0x510 [amdgpu] [4.742427] local_pci_probe+0x42/0xa0 [4.742430] pci_device_probe+0xc1/0x280 [4.742432] really_probe+0x19b/0x3e0 [4.742434] ? __pfx___driver_attach+0x10/0x10 [4.742435] __driver_probe_device+0x78/0x160 [4.742437] driver_probe_device+0x1f/0xa0 [4.742438] __driver_attach+0xba/0x1c0 [4.742440] bus_for_each_dev+0x8c/0xe0 [4.742441] bus_add_driver+0x116/0x220 [4.742443] driver_register+0x5c/0x100 [4.742445] ? __pfx_amdgpu_init+0x10/0x10 [amdgpu] [4.742559] do_one_initcall+0x58/0x320 [4.742562] do_init_module+0x60/0x240 [4.742564] __do_sys_init_module+0x17a/0x1b0 [4.742566] do_syscall_64+0x83/0x170 [4.742569] ? __count_memcg_events+0x4d/0xc0 [4.742570] ? count_memcg_events.constprop.0+0x1a/0x30 [4.742572] ? handle_mm_fault+0x1f2/0x350 [4.742574] ? do_user_addr_fault+0x304/0x670 [4.742576] ? exc_page_fault+0x7f/0x180 [4.742577] entry_SYSCALL_64_after_hwframe+0x78/0x80 [4.742579] RIP: 0033:0x7f9f760a357e [4.742588] Code: 48 8b 0d 9d 98 0c 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 49 89 ca b8 af 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 6a 98 0c 00 f7 d8 64 89 01 48 [4.742590] RSP: 002b:7ffdbdc2ba08 EFLAGS: 0246 ORIG_RAX: 00af [4.742592] RAX: ffda RBX: 55a1d88a69e0 RCX: 7f9f760a357e [4.742593] RDX: 7f9f761bb07d RSI: 019e11a6 RDI: 7f9f73c00010 [4.742594] RBP: 7ffdbdc2bac0 R08: 55a1d8874010 R09: 0007 [4.742594] R10: 0001 R11: 0246 R12: 7f9f761bb07d [4.742598] R13: 0002 R14:
Re: [PATCH] drm: ci: fix the xfails for apq8016
On 25/04/2024 11:01, Helen Koike wrote: On 08/04/2024 14:04, Abhinav Kumar wrote: Hi Helen Gentle reminder on this. If you are okay, we can land it via msm-next tree... Thanks Abhinav On 4/1/2024 1:48 PM, Abhinav Kumar wrote: After IGT migrating to dynamic sub-tests, the pipe prefixes in the expected fails list are incorrect. Lets drop those to accurately match the expected fails. In addition, update the xfails list to match the current passing list. This should have ideally failed in the CI run because some tests were marked as fail even though they passed but due to the mismatch in test names, the matching didn't correctly work and was resulting in those failures not being seen. Here is the passing pipeline for apq8016 with this change: https://gitlab.freedesktop.org/drm/msm/-/jobs/57050562 Signed-off-by: Abhinav Kumar I'm sorry about my delay. Acked-by: Helen Koike I'm also merging it to msm-misc-next. Applied, thanks. Helen Regards, Helen --- drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt | 13 + 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt index 44a5c62dedad..b14d4e884971 100644 --- a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt +++ b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt @@ -1,17 +1,6 @@ kms_3d,Fail kms_addfb_basic@addfb25-bad-modifier,Fail -kms_cursor_legacy@all-pipes-forked-bo,Fail -kms_cursor_legacy@all-pipes-forked-move,Fail -kms_cursor_legacy@all-pipes-single-bo,Fail -kms_cursor_legacy@all-pipes-single-move,Fail -kms_cursor_legacy@all-pipes-torture-bo,Fail -kms_cursor_legacy@all-pipes-torture-move,Fail -kms_cursor_legacy@pipe-A-forked-bo,Fail -kms_cursor_legacy@pipe-A-forked-move,Fail -kms_cursor_legacy@pipe-A-single-bo,Fail -kms_cursor_legacy@pipe-A-single-move,Fail -kms_cursor_legacy@pipe-A-torture-bo,Fail -kms_cursor_legacy@pipe-A-torture-move,Fail +kms_cursor_legacy@torture-bo,Fail kms_force_connector_basic@force-edid,Fail kms_hdmi_inject@inject-4k,Fail kms_selftest@drm_format,Timeout
Re: [PATCH 2/4] WIP: drm: Introduce rvkms
On Thu, 2024-04-25 at 15:46 +, Benno Lossin wrote:
> On 22.04.24 03:54, Lyude Paul wrote:
> > On Wed, 2024-03-27 at 21:06 +, Benno Lossin wrote:
> > > On 22.03.24 23:03, Lyude Paul wrote:
> > > > +
> > > > +pub(crate) type Connector =
> > > > connector::Connector;
> > > > +
> > > > +impl connector::DriverConnector for DriverConnector {
> > > > + type Initializer = impl PinInit;
> > > > +
> > > > + type State = ConnectorState;
> > > > +
> > > > + type Driver = RvkmsDriver;
> > > > +
> > > > + type Args = ();
> > > > +
> > > > + fn new(dev: , args: Self::Args) ->
> > > > Self::Initializer {
> > >
> > > And then here just return `Self`.
> > >
> > > This works, since there is a blanket impl `PinInit for T`.
> > >
> > > Looking at how you use this API, I am not sure if you actually
> > > need
> > > pin-init for the type that implements `DriverConnector`.
> > > Do you need to store eg `Mutex` or something else that needs
> > > pin-init in here in a more complex driver?
> >
> > Most likely yes - a lot of drivers have various private locks
> > contained
> > within their subclassed mode objects. I'm not sure we will in
> > rvkms's
> > connector since vkms doesn't really do much with connectors - but
> > we at
> > a minimum be using pinned types (spinlocks and hrtimers) in our
> > DriverCrtc implementation once I've started implementing support
> > for
> > vblanks[1]
> >
> > [1]
> > https://www.kernel.org/doc/html/v6.9-rc5/gpu/drm-kms.html?highlight=vblank#vertical-blanking
> >
> > In nova (the main reason I'm working on rvkms in the first place),
> > we'll definitely have locks in our connectors and possibly other
> > types.
>
> I see, in that case it would be a good idea to either have an RFC of
> the nova driver (or something else that needs pinned types) as
> motivation for why it needs to be pin-initialized.
I mean - I'll happily include this with the RFC of nova if nova is
ready at that point, but the purpose of rvkms is to exercise enough of
this API to justify merging it :P - and I think it's a lot likely rvkm
is probably going to be ready well before nova gets to the point of
modesetting. And we will definitely have some uses of pinned types in
rvkms once the driver's ready for submission.
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
[PATCH v2 2/2] drm/amd/display: Move PRIMARY plane zpos higher
From: Leo Li
[Why]
Compositors have different ways of assigning surfaces to DRM planes for
render offloading. It may decide between various strategies: overlay,
underlay, or a mix of both (see here for more info:
https://gitlab.freedesktop.org/emersion/libliftoff/-/issues/76)
One way for compositors to implement the underlay strategy is to assign
a higher zpos to the DRM_PRIMARY plane than the DRM_OVERLAY planes,
effectively turning the DRM_OVERLAY plane into an underlay plane.
Today, amdgpu attaches an immutable zpos of 0 to the DRM_PRIMARY plane.
This however, is an arbitrary restriction. DCN pipes are general
purpose, and can be arranged in any z-order. To support compositors
using this allocation scheme, we can set a non-zero immutable zpos for
the PRIMARY, allowing the placement of OVERLAYS (mutable zpos range
0-254) beneath the PRIMARY.
[How]
Assign a zpos = #no of OVERLAY planes to the PRIMARY plane. Then, clean
up any assumptions in the driver of PRIMARY plane having the lowest
zpos.
Signed-off-by: Leo Li
Reviewed-by: Harry Wentland
Acked-by: Pekka Paalanen
v2: Fix typo s/decending/descending/
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 34 +--
.../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 18 +++---
2 files changed, 44 insertions(+), 8 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 b4b5b73707c1..6782ca1137d4 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -80,6 +80,7 @@
#include
#include
#include
+#include
#include
#include
@@ -375,6 +376,20 @@ static inline void reverse_planes_order(struct
dc_surface_update *array_of_surfa
swap(array_of_surface_update[i], array_of_surface_update[j]);
}
+/*
+ * DC will program planes with their z-order determined by their ordering
+ * in the dc_surface_updates array. This comparator is used to sort them
+ * by descending zpos.
+ */
+static int dm_plane_layer_index_cmp(const void *a, const void *b)
+{
+ const struct dc_surface_update *sa = (struct dc_surface_update *)a;
+ const struct dc_surface_update *sb = (struct dc_surface_update *)b;
+
+ /* Sort by descending dc_plane layer_index (i.e. normalized_zpos) */
+ return sb->surface->layer_index - sa->surface->layer_index;
+}
+
/**
* update_planes_and_stream_adapter() - Send planes to be updated in DC
*
@@ -399,7 +414,8 @@ static inline bool update_planes_and_stream_adapter(struct
dc *dc,
struct dc_stream_update
*stream_update,
struct dc_surface_update
*array_of_surface_update)
{
- reverse_planes_order(array_of_surface_update, planes_count);
+ sort(array_of_surface_update, planes_count,
+sizeof(*array_of_surface_update), dm_plane_layer_index_cmp, NULL);
/*
* Previous frame finished and HW is ready for optimization.
@@ -9503,6 +9519,8 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
for (j = 0; j < status->plane_count; j++)
dummy_updates[j].surface = status->plane_states[0];
+ sort(dummy_updates, status->plane_count,
+sizeof(*dummy_updates), dm_plane_layer_index_cmp, NULL);
mutex_lock(>dc_lock);
dc_update_planes_and_stream(dm->dc,
@@ -10237,6 +10255,16 @@ static bool should_reset_plane(struct drm_atomic_state
*state,
if (new_crtc_state->color_mgmt_changed)
return true;
+ /*
+* On zpos change, planes need to be reordered by removing and re-adding
+* them one by one to the dc state, in order of descending zpos.
+*
+* TODO: We can likely skip bandwidth validation if the only thing that
+* changed about the plane was it'z z-ordering.
+*/
+ if (new_crtc_state->zpos_changed)
+ return true;
+
if (drm_atomic_crtc_needs_modeset(new_crtc_state))
return true;
@@ -11076,7 +11104,7 @@ static int amdgpu_dm_atomic_check(struct drm_device
*dev,
}
/* Remove exiting planes if they are modified */
- for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state,
new_plane_state, i) {
+ for_each_oldnew_plane_in_descending_zpos(state, plane, old_plane_state,
new_plane_state) {
if (old_plane_state->fb && new_plane_state->fb &&
get_mem_type(old_plane_state->fb) !=
get_mem_type(new_plane_state->fb))
@@ -11121,7 +11149,7 @@ static int amdgpu_dm_atomic_check(struct drm_device
*dev,
}
/* Add new/modified planes */
- for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state,
new_plane_state, i) {
+
[PATCH v2 1/2] drm/amd/display: Introduce overlay cursor mode
From: Leo Li
[Why]
DCN is the display hardware for amdgpu. DRM planes are backed by DCN
hardware pipes, which carry pixel data from one end (memory), to the
other (output encoder).
Each DCN pipe has the ability to blend in a cursor early on in the
pipeline. In other words, there are no dedicated cursor planes in DCN,
which makes cursor behavior somewhat unintuitive for compositors.
For example, if the cursor is in RGB format, but the top-most DRM plane
is in YUV format, DCN will not be able to blend them. Because of this,
amdgpu_dm rejects all configurations where a cursor needs to be enabled
on top of a YUV formatted plane.
>From a compositor's perspective, when computing an allocation for
hardware plane offloading, this cursor-on-yuv configuration result in an
atomic test failure. Since the failure reason is not obvious at all,
compositors will likely fall back to full rendering, which is not ideal.
Instead, amdgpu_dm can try to accommodate the cursor-on-yuv
configuration by opportunistically reserving a separate DCN pipe just
for the cursor. We can refer to this as "overlay cursor mode". It is
contrasted with "native cursor mode", where the native DCN per-pipe
cursor is used.
[How]
On each crtc, compute whether the cursor plane should be enabled in
overlay mode. If it is, mark the CRTC as requesting overlay cursor mode.
Overlay cursor should be enabled whenever there exists a underlying
plane that has YUV format, or is scaled differently than the cursor. It
should also be enabled if there is no underlying plane, or if underlying
planes do not cover the entire CRTC.
During DC validation, attempt to enable a separate DCN pipe for the
cursor if it's in overlay mode. If that fails, or if no overlay mode is
requested, then fallback to native mode.
v2:
* Update commit message for when overlay cursor should be enabled
* Also consider scale and no-underlying-plane case (cursor on crtc bg)
* Consider all underlying planes when determinig overlay/native, not
just the plane immediately beneath the cursor, as it may not cover the
entire CRTC.
* Fix typo s/decending/descending/
* Force native cursor on pre-DCN hardware
Signed-off-by: Leo Li
Acked-by: Harry Wentland
Acked-by: Pekka Paalanen
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 490 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 7 +
.../amd/display/amdgpu_dm/amdgpu_dm_crtc.c| 1 +
.../amd/display/amdgpu_dm/amdgpu_dm_plane.c | 13 +-
4 files changed, 386 insertions(+), 125 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 8245cc63712f..b4b5b73707c1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8490,8 +8490,22 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
* Disable the cursor first if we're disabling all the planes.
* It'll remain on the screen after the planes are re-enabled
* if we don't.
+*
+* If the cursor is transitioning from native to overlay mode, the
+* native cursor needs to be disabled first.
*/
- if (acrtc_state->active_planes == 0)
+ if (acrtc_state->cursor_mode == DM_CURSOR_OVERLAY_MODE &&
+ dm_old_crtc_state->cursor_mode == DM_CURSOR_NATIVE_MODE) {
+ struct dc_cursor_position cursor_position = {0};
+
+ dc_stream_set_cursor_position(acrtc_state->stream,
+ _position);
+ bundle->stream_update.cursor_position =
+ _state->stream->cursor_position;
+ }
+
+ if (acrtc_state->active_planes == 0 &&
+ dm_old_crtc_state->cursor_mode == DM_CURSOR_NATIVE_MODE)
amdgpu_dm_commit_cursors(state);
/* update planes when needed */
@@ -8505,7 +8519,8 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
struct dm_plane_state *dm_new_plane_state =
to_dm_plane_state(new_plane_state);
/* Cursor plane is handled after stream updates */
- if (plane->type == DRM_PLANE_TYPE_CURSOR) {
+ if (plane->type == DRM_PLANE_TYPE_CURSOR &&
+ acrtc_state->cursor_mode == DM_CURSOR_NATIVE_MODE) {
if ((fb && crtc == pcrtc) ||
(old_plane_state->fb && old_plane_state->crtc ==
pcrtc)) {
cursor_update = true;
@@ -8863,7 +8878,8 @@ static void amdgpu_dm_commit_planes(struct
drm_atomic_state *state,
* to be disabling a single plane - those pipes are being disabled.
*/
if (acrtc_state->active_planes &&
- (!updated_planes_and_streams || amdgpu_ip_version(dm->adev,
DCE_HWIP, 0) == 0))
+ (!updated_planes_and_streams || amdgpu_ip_version(dm->adev,
DCE_HWIP, 0) == 0) &&
+
[PATCH v8 35/35] drm-print: workaround compiler meh
For some reason I cannot grok, I get an unused variable 'category'
warning/error, though the usage follows immediately. This drops the
local var and directly derefs in the macro-call, which somehow avoids
the warning.
commit 9fd6f61a297e ("drm/print: add drm_dbg_printer() for drm device specific
printer")
CC: Jani Nikula
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/drm_print.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index efdf82f8cbbb..c400441cd77e 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -183,11 +183,10 @@ void __drm_printfn_dbg(struct drm_printer *p, struct
va_format *vaf)
{
const struct drm_device *drm = p->arg;
const struct device *dev = drm ? drm->dev : NULL;
- enum drm_debug_category category = p->category;
const char *prefix = p->prefix ?: "";
const char *prefix_pad = p->prefix ? " " : "";
- if (!__drm_debug_enabled(category))
+ if (!__drm_debug_enabled(p->category))
return;
/* Note: __builtin_return_address(0) is useless here. */
--
2.44.0
[PATCH v8 25/35] docs/dyndbg: explain new delimiters: comma, percent
Add mention of comma and percent delimiters into the respective paragraphs describing their equivalents: space and newline. Signed-off-by: Jim Cromie --- .../admin-guide/dynamic-debug-howto.rst| 18 ++ 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst index 742eb4230c6e..7b570f29ae98 100644 --- a/Documentation/admin-guide/dynamic-debug-howto.rst +++ b/Documentation/admin-guide/dynamic-debug-howto.rst @@ -73,16 +73,18 @@ Command Language Reference == At the basic lexical level, a command is a sequence of words separated -by spaces or tabs. So these are all equivalent:: +by spaces, tabs, or commas. So these are all equivalent:: :#> ddcmd file svcsock.c line 1603 +p :#> ddcmd "file svcsock.c line 1603 +p" :#> ddcmd ' file svcsock.c line 1603 +p ' + :#> ddcmd file,svcsock.c,line,1603,+p -Command submissions are bounded by a write() system call. -Multiple commands can be written together, separated by ``;`` or ``\n``:: +Command submissions are bounded by a write() system call. Multiple +commands can be written together, separated by ``%``, ``;`` or ``\n``:: - :#> ddcmd "func pnpacpi_get_resources +p; func pnp_assign_mem +p" + :#> ddcmd func foo +p % func bar +p + :#> ddcmd func foo +p \; func bar +p :#> ddcmd <<"EOC" func pnpacpi_get_resources +p func pnp_assign_mem +p @@ -104,7 +106,6 @@ The match-spec's select *prdbgs* from the catalog, upon which to apply the flags-spec, all constraints are ANDed together. An absent keyword is the same as keyword "*". - A match specification is a keyword, which selects the attribute of the callsite to be compared, and a value to compare against. Possible keywords are::: @@ -128,7 +129,6 @@ keywords are::: ``line-range`` cannot contain space, e.g. "1-30" is valid range but "1 - 30" is not. - The meanings of each keyword are: func @@ -153,9 +153,11 @@ module The given string is compared against the module name of each callsite. The module name is the string as seen in ``lsmod``, i.e. without the directory or the ``.ko`` -suffix and with ``-`` changed to ``_``. Examples:: +suffix and with ``-`` changed to ``_``. + +Examples:: - module sunrpc + module,sunrpc # with ',' as token separator module nfsd module drm* # both drm, drm_kms_helper -- 2.44.0
[PATCH v8 34/35] drm: restore CONFIG_DRM_USE_DYNAMIC_DEBUG un-BROKEN
Time for some quality CI Signed-off-by: Jim Cromie --- drivers/gpu/drm/Kconfig | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 5a0c476361c3..b2ea73ae48f0 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -54,8 +54,7 @@ config DRM_DEBUG_MM config DRM_USE_DYNAMIC_DEBUG bool "use dynamic debug to implement drm.debug" - default n - depends on BROKEN + default y depends on DRM depends on DYNAMIC_DEBUG || DYNAMIC_DEBUG_CORE depends on JUMP_LABEL -- 2.44.0
[PATCH v8 32/35] drm: use correct ccflags-y spelling
Incorrectly spelled CFLAGS- failed to add -DDYNAMIC_DEBUG_MODULE,
which broke builds with:
CONFIG_DRM_USE_DYNAMIC_DEBUG=y
CONFIG_DYNAMIC_DEBUG_CORE=y
CONFIG_DYNAMIC_DEBUG=n
Also add subdir-ccflags so that all drivers pick up the addition.
Fixes: 84ec67288c10 ("drm_print: wrap drm_*_dbg in dyndbg descriptor factory
macro")
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/Makefile | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 104b42df2e95..313516fc2ad5 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -3,7 +3,8 @@
# Makefile for the drm device driver. This driver provides support for the
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
-CFLAGS-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE
+ccflags-$(CONFIG_DRM_USE_DYNAMIC_DEBUG)+=
-DDYNAMIC_DEBUG_MODULE
+subdir-ccflags-$(CONFIG_DRM_USE_DYNAMIC_DEBUG) += -DDYNAMIC_DEBUG_MODULE
drm-y := \
drm_aperture.o \
--
2.44.0
[PATCH v8 31/35] drm-dyndbg: adapt to use DYNDBG_CLASSMAP_PARAM
use new export
---
drivers/gpu/drm/drm_print.c | 8 ++--
include/drm/drm_print.h | 6 --
2 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 4a5f2317229b..efdf82f8cbbb 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -69,12 +69,8 @@ DRM_CLASSMAP_DEFINE(drm_debug_classes,
DD_CLASS_TYPE_DISJOINT_BITS,
"DRM_UT_DP",
"DRM_UT_DRMRES");
-static struct ddebug_class_param drm_debug_bitmap = {
- .bits = &__drm_debug,
- .flags = "p",
- .map = _debug_classes,
-};
-module_param_cb(debug, _ops_dyndbg_classes, _debug_bitmap, 0600);
+DRM_CLASSMAP_PARAM_REF(debug, __drm_debug, drm_debug_classes, p);
+
#endif
void __drm_puts_coredump(struct drm_printer *p, const char *str)
diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
index 905fc25bf65a..95c667934bbb 100644
--- a/include/drm/drm_print.h
+++ b/include/drm/drm_print.h
@@ -141,11 +141,13 @@ enum drm_debug_category {
};
#ifdef CONFIG_DRM_USE_DYNAMIC_DEBUG
-#define DRM_CLASSMAP_DEFINE(...) DYNDBG_CLASSMAP_DEFINE(__VA_ARGS__)
-#define DRM_CLASSMAP_USE(name) DYNDBG_CLASSMAP_USE(name)
+#define DRM_CLASSMAP_DEFINE(...)DYNDBG_CLASSMAP_DEFINE(__VA_ARGS__)
+#define DRM_CLASSMAP_USE(name) DYNDBG_CLASSMAP_USE(name)
+#define DRM_CLASSMAP_PARAM_REF(...) DYNDBG_CLASSMAP_PARAM_REF(__VA_ARGS__)
#else
#define DRM_CLASSMAP_DEFINE(...)
#define DRM_CLASSMAP_USE(name)
+#define DRM_CLASSMAP_PARAM_REF(...)
#endif
static inline bool drm_debug_enabled_raw(enum drm_debug_category category)
--
2.44.0
[PATCH v8 27/35] selftests-dyndbg: test dyndbg-to-tracefs
Add a series of trace-tests: test_actual_trace() etc, to validate that
the dyndbg-to-tracefs feature (using +T flag) works as intended. The
1st test uses the global tracebuf, the rest use/excercise private
tracebufs.
These tests are currently optional, via "TRACE" arg1, because the
feature code is in-the-lab. But its an objective test, and pretty
user-interface oriented.
IOW this passes:
:#> ./tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
but this fails:
:#> ./tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh TRACE
So its won't break selftests success.
This allows the patch to be committed now w/o inducing selftest
failures, and the tests enabled later, with the promised code.
Signed-off-by: Jim Cromie
Co-developed-by: Łukasz Bartosik
Signed-off-by: Łukasz Bartosik
---
.../dynamic_debug/dyndbg_selftest.sh | 435 ++
1 file changed, 435 insertions(+)
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index 54acee58cb4e..65f31418870f 100755
--- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -308,6 +308,405 @@ function test_mod_submod {
check_match_ct =p 14 -v
}
+# tests below here are all actually using dyndbg->trace,
+# and verifying the writes
+
+function test_actual_trace {
+echo -e "${GREEN}# TEST_ACTUAL_TRACE ${NC}"
+ddcmd =_
+echo > /sys/kernel/tracing/trace
+echo 1 >/sys/kernel/tracing/tracing_on
+echo 1 >/sys/kernel/tracing/events/dyndbg/enable
+modprobe test_dynamic_debug dyndbg=class,D2_CORE,+T:0
+search_trace "D2_CORE msg"
+search_trace_name 0 1 "D2_CORE msg"
+check_match_ct =T 1
+tmark "trace-mark"
+search_trace "trace-mark"
+doprints
+search_trace "D2_CORE msg"
+ifrmmod test_dynamic_debug
+}
+
+function self_start {
+echo \# open, modprobe +T:selftest
+ddcmd open selftest
+check_trace_instance_dir selftest 1
+is_trace_instance_opened selftest
+modprobe test_dynamic_debug dyndbg=+T:selftest.mf
+check_match_ct =T:selftest.mf 5
+}
+
+function self_end_normal {
+echo \# disable -T:selftest, rmmod, close
+ddcmd module test_dynamic_debug -T:selftest # leave mf
+check_match_ct =:selftest.mf 5 -v
+ddcmd module test_dynamic_debug +:0
+ddcmd close selftest
+is_trace_instance_closed selftest
+ifrmmod test_dynamic_debug
+}
+
+function self_end_disable_anon {
+echo \# disable, close, rmmod
+ddcmd module test_dynamic_debug -T
+check_match_ct =:selftest.mf 5
+ddcmd module test_dynamic_debug +:0
+ddcmd close selftest
+is_trace_instance_closed selftest
+ifrmmod test_dynamic_debug
+}
+
+function self_end_disable_anon_mf {
+echo \# disable, close, rmmod
+ddcmd module test_dynamic_debug -Tf
+check_match_ct =:selftest.m 5
+ddcmd module test_dynamic_debug +:0
+ddcmd close selftest
+is_trace_instance_closed selftest
+ifrmmod test_dynamic_debug
+}
+
+function self_end_nodisable {
+echo \# SKIPPING: ddcmd module test_dynamic_debug -T:selftest
+ddcmd close selftest fail # close fails because selftest is still being
used
+check_err_msg "Device or resource busy"
+check_match_ct =T:selftest.mf 5
+rmmod test_dynamic_debug
+ddcmd close selftest # now selftest can be closed because rmmod removed
+ # all callsites which were using it
+is_trace_instance_closed selftest
+}
+
+function self_end_delete_directory {
+del_trace_instance_dir selftest 0
+check_err_msg "Device or resource busy"
+ddcmd module test_dynamic_debug -mT:selftest
+check_match_ct =:selftest.f 5
+del_trace_instance_dir selftest 0
+check_err_msg "Device or resource busy"
+ddcmd module test_dynamic_debug +:0
+ddcmd close selftest
+check_trace_instance_dir selftest 1
+is_trace_instance_closed selftest
+del_trace_instance_dir selftest 1
+check_trace_instance_dir selftest 0
+}
+
+function test_early_close () {
+ddcmd open kparm_stream
+ddcmd module usbcore +T:kparm_stream.mf
+check_match_ct =T:usb_stream.mf 161
+echo ":not-running # ddcmd module usbcore -T:kparm_stream.mf"
+ddcmd close kparm_stream
+}
+
+function self_test_ {
+echo "# SELFTEST $1"
+self_start
+self_end_$1
+}
+
+function cycle_tests_normal {
+echo -e "${GREEN}# CYCLE_TESTS_NORMAL ${NC}"
+self_test_ normal # ok
+self_test_ disable_anon # ok
+self_test_ normal # ok
+self_test_ disable_anon_mf # ok
+}
+
+function cycle_not_best_practices {
+echo -e "${GREEN}# CYCLE_TESTS_PROBLEMS ${NC}"
+self_test_ nodisable
+self_test_ normal
+self_test_ delete_directory
+}
+
+# proper life cycle - open, enable:named, disable:named, close
+function test_private_trace_simple_proper {
+echo -e "${GREEN}# TEST_PRIVATE_TRACE_1 ${NC}"
+# ddcmd
[PATCH v8 33/35] drm-drivers: DRM_CLASSMAP_USE in 2nd batch of drivers, helpers
Add a DRM_CLASSMAP_USE declaration to 2nd batch of helpers and *_drv.c
files. For drivers, add the decl just above the module's PARAMs,
since it identifies the "inherited" drm.debug param.
Note: with CONFIG_DRM_USE_DYNAMIC_DEBUG=y, a module not also declaring
DRM_CLASSMAP_USE will have its class'd prdbgs stuck in the initial
(disabled, but for DEBUG) state.
The stuck sites are evident in /proc/dynamic_debug/control as:
class:_UNKNOWN_ _id:N# control's last column
rather than a proper "enumeration":
class:DRM_UT_CORE
TLDR: This set of updates was found by choosing M for all DRM-config
items I found (not allmodconfig), building & modprobing them, and
grepping "class unknown," control. There may yet be others.
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/drm_gem_shmem_helper.c | 2 ++
drivers/gpu/drm/gud/gud_drv.c | 2 ++
drivers/gpu/drm/mgag200/mgag200_drv.c | 2 ++
drivers/gpu/drm/qxl/qxl_drv.c | 2 ++
drivers/gpu/drm/radeon/radeon_drv.c| 2 ++
drivers/gpu/drm/udl/udl_main.c | 2 ++
drivers/gpu/drm/vkms/vkms_drv.c| 2 ++
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 2 ++
8 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
index e435f986cd13..066d906e3199 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -23,6 +23,8 @@
#include
#include
+DRM_CLASSMAP_USE(drm_debug_classes);
+
MODULE_IMPORT_NS(DMA_BUF);
/**
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index 9d7bf8ee45f1..5b555045fce4 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -31,6 +31,8 @@
#include "gud_internal.h"
+DRM_CLASSMAP_USE(drm_debug_classes);
+
/* Only used internally */
static const struct drm_format_info gud_drm_format_r1 = {
.format = GUD_DRM_FORMAT_R1,
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 573dbe256aa8..88c5e24cc894 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -25,6 +25,8 @@ static int mgag200_modeset = -1;
MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
module_param_named(modeset, mgag200_modeset, int, 0400);
+DRM_CLASSMAP_USE(drm_debug_classes);
+
int mgag200_init_pci_options(struct pci_dev *pdev, u32 option, u32 option2)
{
struct device *dev = >dev;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index beee5563031a..1971bfa8a8a6 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -65,6 +65,8 @@ module_param_named(modeset, qxl_modeset, int, 0400);
MODULE_PARM_DESC(num_heads, "Number of virtual crtcs to expose (default 4)");
module_param_named(num_heads, qxl_num_crtc, int, 0400);
+DRM_CLASSMAP_USE(drm_debug_classes);
+
static struct drm_driver qxl_driver;
static struct pci_driver qxl_pci_driver;
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
b/drivers/gpu/drm/radeon/radeon_drv.c
index 7bf08164140e..d22308328c76 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -247,6 +247,8 @@ int radeon_cik_support = 1;
MODULE_PARM_DESC(cik_support, "CIK support (1 = enabled (default), 0 =
disabled)");
module_param_named(cik_support, radeon_cik_support, int, 0444);
+DRM_CLASSMAP_USE(drm_debug_classes);
+
static struct pci_device_id pciidlist[] = {
radeon_PCI_IDS
};
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 3ebe2ce55dfd..ba57c14454e5 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -19,6 +19,8 @@
#define NR_USB_REQUEST_CHANNEL 0x12
+DRM_CLASSMAP_USE(drm_debug_classes);
+
#define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE)
#define WRITES_IN_FLIGHT (20)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index dd0af086e7fa..086797c4b82b 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -39,6 +39,8 @@
static struct vkms_config *default_config;
+DRM_CLASSMAP_USE(drm_debug_classes);
+
static bool enable_cursor = true;
module_param_named(enable_cursor, enable_cursor, bool, 0444);
MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 58fb40c93100..c159f4d186a3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -275,6 +275,8 @@ static int vmw_probe(struct pci_dev *, const struct
pci_device_id *);
static int vmwgfx_pm_notifier(struct notifier_block *nb, unsigned long val,
void *ptr);
+DRM_CLASSMAP_USE(drm_debug_classes);
+
MODULE_PARM_DESC(restrict_iommu, "Try to limit IOMMU usage for TTM pages");
module_param_named(restrict_iommu, vmw_restrict_iommu, int,
[PATCH v8 28/35] dyndbg-doc: explain flags parse 1st
When writing queries to >control, flags are parsed 1st, since they are the only required field. So if the flags draw an error, then keyword errors aren't reported. This can be mildly confusing/annoying, so explain it instead. This note could be moved up to just after the grammar id's the flags, and before the match-spec is detailed. Opinions ? Signed-off-by: Jim Cromie --- Documentation/admin-guide/dynamic-debug-howto.rst | 10 ++ 1 file changed, 10 insertions(+) diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst index 7b570f29ae98..ccf3704f2143 100644 --- a/Documentation/admin-guide/dynamic-debug-howto.rst +++ b/Documentation/admin-guide/dynamic-debug-howto.rst @@ -106,6 +106,16 @@ The match-spec's select *prdbgs* from the catalog, upon which to apply the flags-spec, all constraints are ANDed together. An absent keyword is the same as keyword "*". +Note: because the match-spec can be empty, the flags are checked 1st, +then the pairs of keyword values. Flag errs will hide keyword errs: + + bash-5.2# ddcmd mod bar +foo + dyndbg: read 13 bytes from userspace + dyndbg: query 0: "mod bar +foo" mod:* + dyndbg: unknown flag 'o' + dyndbg: flags parse failed + dyndbg: processed 1 queries, with 0 matches, 1 errs + A match specification is a keyword, which selects the attribute of the callsite to be compared, and a value to compare against. Possible keywords are::: -- 2.44.0
[PATCH v8 30/35] drm+drivers: adapt to use DYNDBG_CLASSMAP_{DEFINE, USE}
Follow dynamic_debug API change from DECLARE_DYNDBG_CLASSMAP to
DYNDBG_CLASSMAP_{DEFINE,USE}.
Prior to this, we used DECLARE_DYNDBG_CLASSMAP, which was preserved to
decouple DRM conversion. I'm unsure of the full functionality
in-between, a round of lkp-testing will help.
Fixes: f158936b60a7 ("drm: POC drm on dyndbg - use in core, 2 helpers, 3
drivers.")
Signed-off-by: Jim Cromie
---
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 +---
drivers/gpu/drm/display/drm_dp_helper.c | 12 +---
drivers/gpu/drm/drm_crtc_helper.c | 12 +---
drivers/gpu/drm/drm_print.c | 25 +
drivers/gpu/drm/i915/i915_params.c | 12 +---
drivers/gpu/drm/nouveau/nouveau_drm.c | 12 +---
include/drm/drm_print.h | 8
7 files changed, 26 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index e4277298cf1a..b287f0cfd8fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -217,17 +217,7 @@ int amdgpu_damage_clips = -1; /* auto */
static void amdgpu_drv_delayed_reset_work_handler(struct work_struct *work);
-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "DRM_UT_CORE",
- "DRM_UT_DRIVER",
- "DRM_UT_KMS",
- "DRM_UT_PRIME",
- "DRM_UT_ATOMIC",
- "DRM_UT_VBL",
- "DRM_UT_STATE",
- "DRM_UT_LEASE",
- "DRM_UT_DP",
- "DRM_UT_DRMRES");
+DRM_CLASSMAP_USE(drm_debug_classes);
struct amdgpu_mgpu_info mgpu_info = {
.mutex = __MUTEX_INITIALIZER(mgpu_info.mutex),
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
b/drivers/gpu/drm/display/drm_dp_helper.c
index f5d4be897866..d3a7df09846f 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -41,17 +41,7 @@
#include "drm_dp_helper_internal.h"
-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "DRM_UT_CORE",
- "DRM_UT_DRIVER",
- "DRM_UT_KMS",
- "DRM_UT_PRIME",
- "DRM_UT_ATOMIC",
- "DRM_UT_VBL",
- "DRM_UT_STATE",
- "DRM_UT_LEASE",
- "DRM_UT_DP",
- "DRM_UT_DRMRES");
+DRM_CLASSMAP_USE(drm_debug_classes);
struct dp_aux_backlight {
struct backlight_device *base;
diff --git a/drivers/gpu/drm/drm_crtc_helper.c
b/drivers/gpu/drm/drm_crtc_helper.c
index 2dafc39a27cb..e9d229a393f4 100644
--- a/drivers/gpu/drm/drm_crtc_helper.c
+++ b/drivers/gpu/drm/drm_crtc_helper.c
@@ -50,17 +50,7 @@
#include "drm_crtc_helper_internal.h"
-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "DRM_UT_CORE",
- "DRM_UT_DRIVER",
- "DRM_UT_KMS",
- "DRM_UT_PRIME",
- "DRM_UT_ATOMIC",
- "DRM_UT_VBL",
- "DRM_UT_STATE",
- "DRM_UT_LEASE",
- "DRM_UT_DP",
- "DRM_UT_DRMRES");
+DRM_CLASSMAP_USE(drm_debug_classes);
/**
* DOC: overview
diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
index 699b7dbffd7b..4a5f2317229b 100644
--- a/drivers/gpu/drm/drm_print.c
+++ b/drivers/gpu/drm/drm_print.c
@@ -55,18 +55,19 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each
bit enables a debug cat
#if !defined(CONFIG_DRM_USE_DYNAMIC_DEBUG)
module_param_named(debug, __drm_debug, ulong, 0600);
#else
-/* classnames must match vals of enum drm_debug_category */
-DECLARE_DYNDBG_CLASSMAP(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS, 0,
- "DRM_UT_CORE",
- "DRM_UT_DRIVER",
- "DRM_UT_KMS",
- "DRM_UT_PRIME",
- "DRM_UT_ATOMIC",
- "DRM_UT_VBL",
- "DRM_UT_STATE",
- "DRM_UT_LEASE",
- "DRM_UT_DP",
- "DRM_UT_DRMRES");
+/* classnames must match value-symbols of enum drm_debug_category */
+DRM_CLASSMAP_DEFINE(drm_debug_classes, DD_CLASS_TYPE_DISJOINT_BITS,
+ DRM_UT_CORE,
+ "DRM_UT_CORE",
+ "DRM_UT_DRIVER",
+ "DRM_UT_KMS",
+ "DRM_UT_PRIME",
+ "DRM_UT_ATOMIC",
+ "DRM_UT_VBL",
+ "DRM_UT_STATE",
+ "DRM_UT_LEASE",
+ "DRM_UT_DP",
+ "DRM_UT_DRMRES");
static struct ddebug_class_param
[PATCH v8 29/35] dyndbg: add __counted_by annotations
Tell the compiler about our vectors (array,length), in 2 places:
h: struct _ddebug_info, which keeps refs to the __dyndbg_* ELF/DATA
sections, these are all vectors with a length.
c: struct ddebug_table, which has sub-refs into _ddebug_info.*
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 6 +++---
lib/dynamic_debug.c | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 090fe9554db7..c54d2a4e1d48 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -146,9 +146,9 @@ struct ddebug_class_user {
/* encapsulate linker provided built-in (or module) dyndbg data */
struct _ddebug_info {
- struct _ddebug *descs;
- struct ddebug_class_map *classes;
- struct ddebug_class_user *class_users;
+ struct _ddebug *descs __counted_by(num_descs);
+ struct ddebug_class_map *classes __counted_by(num_classes);
+ struct ddebug_class_user *class_users __counted_by(num_class_users);
unsigned int num_descs;
unsigned int num_classes;
unsigned int num_class_users;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 625838bd74aa..390a35508fb6 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -49,9 +49,9 @@ extern struct ddebug_class_user __stop___dyndbg_class_users[];
struct ddebug_table {
struct list_head link;
const char *mod_name;
- struct _ddebug *ddebugs;
- struct ddebug_class_map *classes;
- struct ddebug_class_user *class_users;
+ struct _ddebug *ddebugs __counted_by(num_ddebugs);
+ struct ddebug_class_map *classes __counted_by(num_classes);
+ struct ddebug_class_user *class_users __counted_by(num_class_users);
unsigned int num_ddebugs, num_classes, num_class_users;
};
--
2.44.0
[PATCH v8 26/35] selftests-dyndbg: add test_mod_submod
This new test-fn runs 3 module/submodule modprobe scenarios, variously
using both the generic dyndbg= modprobe arg, and the
test-module's classmap-params to manipulate the test-mod*'s pr_debugs.
In all cases, the current flag-settings are counted and tested vs
expectations.
The 3rd scenario recapitulates the DRM_USE_DYNAMIC_DEBUG=y failure.
1. 2 modprobes (super then sub), with separate dyndbg=class-settings
check module specific flag settings
2. modprobe submod, supermod is auto-loaded
set supermod class-params
check expected enablements in super & submod
3. modprobe super, with param=setting (like drm.debug=0x1ef)
modprobe submod
validate submod's class'd pr_debugs get properly enabled
The test uses multi-queries, with both commas and percents (to avoid
spaces and quoting). This is the main reason the test wasn't earlier
in the patchset, closer to the classmap patches its validating.
With some tedium, the tests could be refactored to split out early
tests which avoid multi-cmds, and test only the class-params.
Signed-off-by: Jim Cromie
---
.../dynamic_debug/dyndbg_selftest.sh | 60 +++
1 file changed, 60 insertions(+)
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index ddb04c0a7fd2..54acee58cb4e 100755
--- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -250,10 +250,70 @@ function test_percent_splitting {
ifrmmod test_dynamic_debug
}
+function test_mod_submod {
+echo -e "${GREEN}# TEST_MOD_SUBMOD ${NC}"
+ifrmmod test_dynamic_debug_submod
+ifrmmod test_dynamic_debug
+ddcmd =_
+
+# modprobe with class enablements
+modprobe test_dynamic_debug
dyndbg=class,D2_CORE,+pf%class,D2_KMS,+pt%class,D2_ATOMIC,+pm
+check_match_ct '\[test_dynamic_debug\]' 23 -r
+check_match_ct =pf 1
+check_match_ct =pt 1
+check_match_ct =pm 1
+
+modprobe test_dynamic_debug_submod
+check_match_ct test_dynamic_debug_submod 23 -r
+check_match_ct '\[test_dynamic_debug\]' 23 -r
+check_match_ct test_dynamic_debug 46 -r
+
+# change classes again, this time submod too
+ddcmd class,D2_CORE,+mf%class,D2_KMS,+lt%class,D2_ATOMIC,+ml "# add some
prefixes"
+check_match_ct =pmf 1 -v
+check_match_ct =plt 1 -v
+check_match_ct =pml 1 -v
+# submod changed too
+check_match_ct =mf 1 -v
+check_match_ct =lt 1 -v
+check_match_ct =ml 1 -v
+
+# now work the classmap-params
+# fresh start, to clear all above flags (test-fn limits)
+ifrmmod test_dynamic_debug_submod
+ifrmmod test_dynamic_debug
+modprobe test_dynamic_debug_submod # get supermod too
+
+echo 1 > /sys/module/test_dynamic_debug/parameters/p_disjoint_bits
+echo 4 > /sys/module/test_dynamic_debug/parameters/p_level_num
+# 2 mods * ( V1-3 + D2_CORE )
+check_match_ct =p 8 -v
+echo 3 > /sys/module/test_dynamic_debug/parameters/p_disjoint_bits
+echo 0 > /sys/module/test_dynamic_debug/parameters/p_level_num
+# 2 mods * ( D2_CORE, D2_DRIVER )
+check_match_ct =p 4 -v
+echo 0x16 > /sys/module/test_dynamic_debug/parameters/p_disjoint_bits
+echo 0 > /sys/module/test_dynamic_debug/parameters/p_level_num
+# 2 mods * ( D2_DRIVER, D2_KMS, D2_ATOMIC )
+check_match_ct =p 6 -v
+
+# recap DRM_USE_DYNAMIC_DEBUG regression
+ifrmmod test_dynamic_debug_submod
+ifrmmod test_dynamic_debug
+# set super-mod params
+modprobe test_dynamic_debug p_disjoint_bits=0x16 p_level_num=5
+check_match_ct =p 7 -v
+modprobe test_dynamic_debug_submod
+# see them picked up by submod
+check_match_ct =p 14 -v
+}
+
tests_list=(
basic_tests
+# these require test_dynamic_debug*.ko
comma_terminator_tests
test_percent_splitting
+test_mod_submod
)
# Run tests
--
2.44.0
[PATCH v8 21/35] dyndbg: treat comma as a token separator
Treat comma as a token terminator, just like a space. This allows a
user to avoid quoting hassles when spaces are otherwise needed:
:#> modprobe drm dyndbg=class,DRM_UT_CORE,+p\;class,DRM_UT_KMS,+p
or as a boot arg:
drm.dyndbg=class,DRM_UT_CORE,+p # todo: support multi-query here
Given the many ways a boot-line +args can be assembled and then passed
in/down/around shell based tools, this may allow side-stepping all
sorts of quoting hassles thru those layers.
existing query format:
modprobe test_dynamic_debug dyndbg="class D2_CORE +p"
new format:
modprobe test_dynamic_debug dyndbg=class,D2_CORE,+p
Signed-off-by: Jim Cromie
Co-developed-by: Łukasz Bartosik
Signed-off-by: Łukasz Bartosik
---
lib/dynamic_debug.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 31fd67597928..c1bc728cb050 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -290,6 +290,14 @@ static int ddebug_change(const struct ddebug_query *query,
return nfound;
}
+static char *skip_spaces_and_commas(const char *str)
+{
+ str = skip_spaces(str);
+ while (*str == ',')
+ str = skip_spaces(++str);
+ return (char *)str;
+}
+
/*
* Split the buffer `buf' into space-separated words.
* Handles simple " and ' quoting, i.e. without nested,
@@ -303,8 +311,8 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
while (*buf) {
char *end;
- /* Skip leading whitespace */
- buf = skip_spaces(buf);
+ /* Skip leading whitespace and comma */
+ buf = skip_spaces_and_commas(buf);
if (!*buf)
break; /* oh, it was trailing whitespace */
if (*buf == '#')
@@ -320,7 +328,7 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
return -EINVAL; /* unclosed quote */
}
} else {
- for (end = buf; *end && !isspace(*end); end++)
+ for (end = buf; *end && !isspace(*end) && *end != ',';
end++)
;
if (end == buf) {
pr_err("parse err after word:%d=%s\n", nwords,
@@ -592,7 +600,8 @@ static int ddebug_exec_queries(char *query, const char
*modname)
if (split)
*split++ = '\0';
- query = skip_spaces(query);
+ query = skip_spaces_and_commas(query);
+
if (!query || !*query || *query == '#')
continue;
--
2.44.0
[PATCH v8 24/35] selftests-dyndbg: test_percent_splitting multi-cmds on module classes
New fn tests multi-queries composed with % splitters. It uses both
test_dynamic_debug and test_dynamic_debug_submod, and manipulates
several classes at once. So despite the syntactic-oriented name, it
also tests classmaps.
Signed-off-by: Jim Cromie
---
.../dynamic_debug/dyndbg_selftest.sh | 20 +++
1 file changed, 20 insertions(+)
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index 7a7d437e948b..ddb04c0a7fd2 100755
--- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -231,9 +231,29 @@ function comma_terminator_tests {
ddcmd =_
}
+function test_percent_splitting {
+echo -e "${GREEN}# TEST_PERCENT_SPLITTING - multi-command splitting on %
${NC}"
+ifrmmod test_dynamic_debug_submod
+ifrmmod test_dynamic_debug
+ddcmd =_
+modprobe test_dynamic_debug
dyndbg=class,D2_CORE,+pf%class,D2_KMS,+pt%class,D2_ATOMIC,+pm
+check_match_ct =pf 1
+check_match_ct =pt 1
+check_match_ct =pm 1
+check_match_ct test_dynamic_debug 23 -r
+# add flags to those callsites
+ddcmd class,D2_CORE,+mf%class,D2_KMS,+lt%class,D2_ATOMIC,+ml
+check_match_ct =pmf 1
+check_match_ct =plt 1
+check_match_ct =pml 1
+check_match_ct test_dynamic_debug 23 -r
+ifrmmod test_dynamic_debug
+}
+
tests_list=(
basic_tests
comma_terminator_tests
+test_percent_splitting
)
# Run tests
--
2.44.0
[PATCH v8 23/35] dyndbg: split multi-query strings with %
Multi-query strings have long allowed:
modprobe drm dyndbg="class DRM_UT_CORE +p; class DRM_UT_KMS +p"
modprobe drm dyndbg=<
[ 203.902703] dyndbg: query parse failed
[ 203.902871] dyndbg: processed 2 queries, with 0 matches, 2 errs
bash: echo: write error: Invalid argument
The '%' splits the input into 2 queries, and both fail. Given the
limited utility of matching against the working parts of a format
string "foo: %d bar %s", nothing is actually lost here.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c1bc728cb050..625838bd74aa 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -596,7 +596,7 @@ static int ddebug_exec_queries(char *query, const char
*modname)
int i, errs = 0, exitcode = 0, rc, nfound = 0;
for (i = 0; query; query = split) {
- split = strpbrk(query, ";\n");
+ split = strpbrk(query, "%;\n");
if (split)
*split++ = '\0';
--
2.44.0
[PATCH v8 22/35] selftests-dyndbg: add comma_terminator_tests
New fn validates parsing and effect of queries using combinations of
commas and spaces to delimit the tokens.
It manipulates pr-debugs in builtin module/params, so might have deps
I havent foreseen on odd configurations.
Signed-off-by: Jim Cromie
---
.../selftests/dynamic_debug/dyndbg_selftest.sh | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index cb77ae142520..7a7d437e948b 100755
--- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -217,9 +217,23 @@ EOF
ddcmd =_
}
+function comma_terminator_tests {
+echo -e "${GREEN}# COMMA_TERMINATOR_TESTS ${NC}"
+# try combos of spaces & commas
+check_match_ct '\[params\]' 4 -r
+ddcmd module,params,=_ # commas as spaces
+ddcmd module,params,+mpf # turn on module's pr-debugs
+check_match_ct =pmf 4
+ddcmd ,module ,, , params, -p
+check_match_ct =mf 4
+ddcmd " , module ,,, , params, -m"#
+check_match_ct =f 4
+ddcmd =_
+}
tests_list=(
basic_tests
+comma_terminator_tests
)
# Run tests
--
2.44.0
[PATCH v8 20/35] dyndbg-doc: add classmap info to howto
Describe the 3 API macros providing dynamic_debug's classmaps DYNDBG_CLASSMAP_DEFINE - create, exports a module's classmap DYNDBG_CLASSMAP_USE- refer to exported map DYNDBG_CLASSMAP_PARAM - bind control param to the classmap DYNDBG_CLASSMAP_PARAM_REF + use module's storage - __drm_debug cc: linux-...@vger.kernel.org Signed-off-by: Jim Cromie --- v5 adjustments per Randy Dunlap v7 checkpatch fixes v8 more --- .../admin-guide/dynamic-debug-howto.rst | 63 ++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst index 6a8ce5a34382..742eb4230c6e 100644 --- a/Documentation/admin-guide/dynamic-debug-howto.rst +++ b/Documentation/admin-guide/dynamic-debug-howto.rst @@ -225,7 +225,6 @@ the ``p`` flag has meaning, other flags are ignored. Note the regexp ``^[-+=][fslmpt_]+$`` matches a flags specification. To clear all flags at once, use ``=_`` or ``-fslmpt``. - Debug messages during Boot Process == @@ -375,3 +374,65 @@ just a shortcut for ``print_hex_dump(KERN_DEBUG)``. For ``print_hex_dump_debug()``/``print_hex_dump_bytes()``, format string is its ``prefix_str`` argument, if it is constant string; or ``hexdump`` in case ``prefix_str`` is built dynamically. + +Dynamic Debug classmaps +=== + +Dyndbg allows selection/grouping of *prdbg* callsites using structural +info: module, file, function, line. Classmaps allow authors to add +their own domain-oriented groupings using class-names. Classmaps are +exported, so they referencable from other modules. + + # enable classes individually + :#> ddcmd class DRM_UT_CORE +p + :#> ddcmd class DRM_UT_KMS +p + # or more selectively + :#> ddcmd class DRM_UT_CORE module drm +p + +The "class FOO" syntax protects class'd prdbgs from generic overwrite:: + + # IOW this doesn't wipe any DRM.debug settings + :#> ddcmd -p + +To support the DRM.debug parameter, DYNDBG_CLASSMAP_PARAM* updates all +classes in a classmap, mapping param-bits 0..N onto the classes: +DRM_UT_<*> for the DRM use-case. + +Dynamic Debug Classmap API +== + +DYNDBG_CLASSMAP_DEFINE - modules use this to create classmaps, naming +each of the classes (stringified enum-symbols: "DRM_UT_<*>"), and +type, and mapping the class-names to consecutive _class_ids. + +By doing so, modules tell dyndbg that they have prdbgs with those +class_ids, and they authorize dyndbg to accept "class FOO" for the +module defining the classmap, and its contained classnames. + +DYNDBG_CLASSMAP_USE - drm drivers invoke this to ref the CLASSMAP that +drm DEFINEs. This shares the classmap definition, and authorizes +dyndbg to apply changes to the user module's class'd pr_debugs. It +also tells dyndbg how to initialize the user's prdbgs at modprobe, +based upon the current setting of the parent's controlling param. + +There are 2 types of classmaps: + + DD_CLASS_TYPE_DISJOINT_BITS: classes are independent, like DRM.debug + DD_CLASS_TYPE_LEVEL_NUM: classes are relative, ordered (V3 > V2) + +DYNDBG_CLASSMAP_PARAM - modelled after module_param_cb, it refers to a +DEFINEd classmap, and associates it to the param's data-store. This +state is then applied to DEFINEr and USEr modules when they're modprobed. + +This interface also enforces the DD_CLASS_TYPE_LEVEL_NUM relation +amongst the contained classnames; all classes are independent in the +control parser itself. + +Modules or module-groups (drm & drivers) can define multiple +classmaps, as long as they share the limited 0..62 per-module-group +_class_id range, without overlap. + +``#define DEBUG`` will enable all pr_debugs in scope, including any +class'd ones. This won't be reflected in the PARAM readback value, +but the class'd pr_debug callsites can be forced off by toggling the +classmap-kparam all-on then all-off. -- 2.44.0
[PATCH v8 21/35] dyndbg: treat comma as a token separator
Treat comma as a token terminator, just like a space. This allows a
user to avoid quoting hassles when spaces are otherwise needed:
:#> modprobe drm dyndbg=class,DRM_UT_CORE,+p\;class,DRM_UT_KMS,+p
or as a boot arg:
drm.dyndbg=class,DRM_UT_CORE,+p # todo: support multi-query here
Given the many ways a boot-line +args can be assembled and then passed
in/down/around shell based tools, this may allow side-stepping all
sorts of quoting hassles thru those layers.
existing query format:
modprobe test_dynamic_debug dyndbg="class D2_CORE +p"
new format:
modprobe test_dynamic_debug dyndbg=class,D2_CORE,+p
Signed-off-by: Jim Cromie
Co-developed-by: Łukasz Bartosik
Signed-off-by: Łukasz Bartosik
---
lib/dynamic_debug.c | 17 +
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 31fd67597928..c1bc728cb050 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -290,6 +290,14 @@ static int ddebug_change(const struct ddebug_query *query,
return nfound;
}
+static char *skip_spaces_and_commas(const char *str)
+{
+ str = skip_spaces(str);
+ while (*str == ',')
+ str = skip_spaces(++str);
+ return (char *)str;
+}
+
/*
* Split the buffer `buf' into space-separated words.
* Handles simple " and ' quoting, i.e. without nested,
@@ -303,8 +311,8 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
while (*buf) {
char *end;
- /* Skip leading whitespace */
- buf = skip_spaces(buf);
+ /* Skip leading whitespace and comma */
+ buf = skip_spaces_and_commas(buf);
if (!*buf)
break; /* oh, it was trailing whitespace */
if (*buf == '#')
@@ -320,7 +328,7 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
return -EINVAL; /* unclosed quote */
}
} else {
- for (end = buf; *end && !isspace(*end); end++)
+ for (end = buf; *end && !isspace(*end) && *end != ',';
end++)
;
if (end == buf) {
pr_err("parse err after word:%d=%s\n", nwords,
@@ -592,7 +600,8 @@ static int ddebug_exec_queries(char *query, const char
*modname)
if (split)
*split++ = '\0';
- query = skip_spaces(query);
+ query = skip_spaces_and_commas(query);
+
if (!query || !*query || *query == '#')
continue;
--
2.44.0
[PATCH v8 22/35] selftests-dyndbg: add comma_terminator_tests
New fn validates parsing and effect of queries using combinations of
commas and spaces to delimit the tokens.
It manipulates pr-debugs in builtin module/params, so might have deps
I havent foreseen on odd configurations.
Signed-off-by: Jim Cromie
---
.../selftests/dynamic_debug/dyndbg_selftest.sh | 14 ++
1 file changed, 14 insertions(+)
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index cb77ae142520..7a7d437e948b 100755
--- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -217,9 +217,23 @@ EOF
ddcmd =_
}
+function comma_terminator_tests {
+echo -e "${GREEN}# COMMA_TERMINATOR_TESTS ${NC}"
+# try combos of spaces & commas
+check_match_ct '\[params\]' 4 -r
+ddcmd module,params,=_ # commas as spaces
+ddcmd module,params,+mpf # turn on module's pr-debugs
+check_match_ct =pmf 4
+ddcmd ,module ,, , params, -p
+check_match_ct =mf 4
+ddcmd " , module ,,, , params, -m"#
+check_match_ct =f 4
+ddcmd =_
+}
tests_list=(
basic_tests
+comma_terminator_tests
)
# Run tests
--
2.44.0
[PATCH v8 11/35] dyndbg: silence debugs with no-change updates
In ddebug_apply_class_bitmap(), check for actual changes to the bits
before announcing them, to declutter logs.
no functional change.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 12 +++-
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 368381dbd266..8320cadeb251 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -595,7 +595,7 @@ static int ddebug_exec_queries(char *query, const char
*modname)
return nfound;
}
-/* apply a new bitmap to the sys-knob's current bit-state */
+/* apply a new class-param setting */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
unsigned long *new_bits, unsigned long
*old_bits,
const char *query_modname)
@@ -606,8 +606,9 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
int matches = 0;
int bi, ct;
- v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
*old_bits,
- query_modname ?: "");
+ if (*new_bits != *old_bits)
+ v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
+ *old_bits, query_modname ?: "'*'");
for (bi = 0; bi < map->length; bi++) {
if (test_bit(bi, new_bits) == test_bit(bi, old_bits))
@@ -622,8 +623,9 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
ct, map->class_names[bi], *new_bits);
}
- v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
*old_bits,
- query_modname ?: "");
+ if (*new_bits != *old_bits)
+ v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
+ *old_bits, query_modname ?: "'*'");
return matches;
}
--
2.44.0
[PATCH v8 13/35] dyndbg: tighten fn-sig of ddebug_apply_class_bitmap
old_bits arg is currently a pointer to the input bits, but this could
allow inadvertent changes to the input by the fn. Disallow this.
And constify new_bits while here.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 882354e1e78f..d4a0ae31d059 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -597,7 +597,8 @@ static int ddebug_exec_queries(char *query, const char
*modname)
/* apply a new class-param setting */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
-unsigned long *new_bits, unsigned long
*old_bits,
+const unsigned long *new_bits,
+const unsigned long old_bits,
const char *query_modname)
{
#define QUERY_SIZE 128
@@ -606,12 +607,12 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
int matches = 0;
int bi, ct;
- if (*new_bits != *old_bits)
+ if (*new_bits != old_bits)
v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
- *old_bits, query_modname ?: "'*'");
+ old_bits, query_modname ?: "'*'");
for (bi = 0; bi < map->length; bi++) {
- if (test_bit(bi, new_bits) == test_bit(bi, old_bits))
+ if (test_bit(bi, new_bits) == test_bit(bi, _bits))
continue;
snprintf(query, QUERY_SIZE, "class %s %c%s",
map->class_names[bi],
@@ -623,9 +624,9 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
ct, map->class_names[bi], *new_bits);
}
- if (*new_bits != *old_bits)
+ if (*new_bits != old_bits)
v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
- *old_bits, query_modname ?: "'*'");
+ old_bits, query_modname ?: "'*'");
return matches;
}
@@ -681,7 +682,7 @@ static int param_set_dyndbg_classnames(const char *instr,
const struct kernel_pa
continue;
}
curr_bits ^= BIT(cls_id);
- totct += ddebug_apply_class_bitmap(dcp, _bits,
dcp->bits, NULL);
+ totct += ddebug_apply_class_bitmap(dcp, _bits,
*dcp->bits, NULL);
*dcp->bits = curr_bits;
v2pr_info("%s: changed bit %d:%s\n", KP_NAME(kp),
cls_id,
map->class_names[cls_id]);
@@ -691,7 +692,7 @@ static int param_set_dyndbg_classnames(const char *instr,
const struct kernel_pa
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
curr_bits = CLASSMAP_BITMASK(cls_id + (wanted ? 1 : 0
));
- totct += ddebug_apply_class_bitmap(dcp, _bits,
_bits, NULL);
+ totct += ddebug_apply_class_bitmap(dcp, _bits,
old_bits, NULL);
*dcp->lvl = (cls_id + (wanted ? 1 : 0));
v2pr_info("%s: changed bit-%d: \"%s\" %lx->%lx\n",
KP_NAME(kp), cls_id,
map->class_names[cls_id], old_bits,
curr_bits);
@@ -745,7 +746,7 @@ static int param_set_dyndbg_module_classes(const char
*instr,
inrep &= CLASSMAP_BITMASK(map->length);
}
v2pr_info("bits:0x%lx > %s.%s\n", inrep, modnm ?: "*",
KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, , dcp->bits,
modnm);
+ totct += ddebug_apply_class_bitmap(dcp, , *dcp->bits,
modnm);
*dcp->bits = inrep;
break;
case DD_CLASS_TYPE_LEVEL_NUM:
@@ -758,7 +759,7 @@ static int param_set_dyndbg_module_classes(const char
*instr,
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
new_bits = CLASSMAP_BITMASK(inrep);
v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits,
KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, _bits, _bits,
modnm);
+ totct += ddebug_apply_class_bitmap(dcp, _bits, old_bits,
modnm);
*dcp->lvl = inrep;
break;
default:
--
2.44.0
[PATCH v8 20/35] dyndbg-doc: add classmap info to howto
Describe the 3 API macros providing dynamic_debug's classmaps DYNDBG_CLASSMAP_DEFINE - create, exports a module's classmap DYNDBG_CLASSMAP_USE- refer to exported map DYNDBG_CLASSMAP_PARAM - bind control param to the classmap DYNDBG_CLASSMAP_PARAM_REF + use module's storage - __drm_debug cc: linux-...@vger.kernel.org Signed-off-by: Jim Cromie --- v5 adjustments per Randy Dunlap v7 checkpatch fixes v8 more --- .../admin-guide/dynamic-debug-howto.rst | 63 ++- 1 file changed, 62 insertions(+), 1 deletion(-) diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst b/Documentation/admin-guide/dynamic-debug-howto.rst index 6a8ce5a34382..742eb4230c6e 100644 --- a/Documentation/admin-guide/dynamic-debug-howto.rst +++ b/Documentation/admin-guide/dynamic-debug-howto.rst @@ -225,7 +225,6 @@ the ``p`` flag has meaning, other flags are ignored. Note the regexp ``^[-+=][fslmpt_]+$`` matches a flags specification. To clear all flags at once, use ``=_`` or ``-fslmpt``. - Debug messages during Boot Process == @@ -375,3 +374,65 @@ just a shortcut for ``print_hex_dump(KERN_DEBUG)``. For ``print_hex_dump_debug()``/``print_hex_dump_bytes()``, format string is its ``prefix_str`` argument, if it is constant string; or ``hexdump`` in case ``prefix_str`` is built dynamically. + +Dynamic Debug classmaps +=== + +Dyndbg allows selection/grouping of *prdbg* callsites using structural +info: module, file, function, line. Classmaps allow authors to add +their own domain-oriented groupings using class-names. Classmaps are +exported, so they referencable from other modules. + + # enable classes individually + :#> ddcmd class DRM_UT_CORE +p + :#> ddcmd class DRM_UT_KMS +p + # or more selectively + :#> ddcmd class DRM_UT_CORE module drm +p + +The "class FOO" syntax protects class'd prdbgs from generic overwrite:: + + # IOW this doesn't wipe any DRM.debug settings + :#> ddcmd -p + +To support the DRM.debug parameter, DYNDBG_CLASSMAP_PARAM* updates all +classes in a classmap, mapping param-bits 0..N onto the classes: +DRM_UT_<*> for the DRM use-case. + +Dynamic Debug Classmap API +== + +DYNDBG_CLASSMAP_DEFINE - modules use this to create classmaps, naming +each of the classes (stringified enum-symbols: "DRM_UT_<*>"), and +type, and mapping the class-names to consecutive _class_ids. + +By doing so, modules tell dyndbg that they have prdbgs with those +class_ids, and they authorize dyndbg to accept "class FOO" for the +module defining the classmap, and its contained classnames. + +DYNDBG_CLASSMAP_USE - drm drivers invoke this to ref the CLASSMAP that +drm DEFINEs. This shares the classmap definition, and authorizes +dyndbg to apply changes to the user module's class'd pr_debugs. It +also tells dyndbg how to initialize the user's prdbgs at modprobe, +based upon the current setting of the parent's controlling param. + +There are 2 types of classmaps: + + DD_CLASS_TYPE_DISJOINT_BITS: classes are independent, like DRM.debug + DD_CLASS_TYPE_LEVEL_NUM: classes are relative, ordered (V3 > V2) + +DYNDBG_CLASSMAP_PARAM - modelled after module_param_cb, it refers to a +DEFINEd classmap, and associates it to the param's data-store. This +state is then applied to DEFINEr and USEr modules when they're modprobed. + +This interface also enforces the DD_CLASS_TYPE_LEVEL_NUM relation +amongst the contained classnames; all classes are independent in the +control parser itself. + +Modules or module-groups (drm & drivers) can define multiple +classmaps, as long as they share the limited 0..62 per-module-group +_class_id range, without overlap. + +``#define DEBUG`` will enable all pr_debugs in scope, including any +class'd ones. This won't be reflected in the PARAM readback value, +but the class'd pr_debug callsites can be forced off by toggling the +classmap-kparam all-on then all-off. -- 2.44.0
[PATCH v8 19/35] dyndbg-API: promote DYNDBG_CLASSMAP_PARAM to API
move the DYNDBG_CLASSMAP_PARAM macro from test-dynamic-debug.c into
the header, and refine it, by distinguishing the 2 use cases:
1.DYNDBG_CLASSMAP_PARAM_REF
for DRM, to pass in extern __drm_debug by name.
dyndbg keeps bits in it, so drm can still use it as before
2.DYNDBG_CLASSMAP_PARAM
new user (test_dynamic_debug) doesn't need to share state,
decls a static long unsigned int to store the bitvec.
__DYNDBG_CLASSMAP_PARAM
bottom layer - allocate,init a ddebug-class-param, module-param-cb.
Also clean up and improve comments in test-code, and add
MODULE_DESCRIPTIONs.
Signed-off-by: Jim Cromie
---
fixup drm-print.h add PARAM_REF forwarding macros
with DYNDBG_CLASSMAP_PARAM_REF in the API, add DRM_ variant
---
include/linux/dynamic_debug.h | 37 -
lib/dynamic_debug.c | 70 ++---
lib/test_dynamic_debug.c| 50 +--
lib/test_dynamic_debug_submod.c | 9 -
4 files changed, 110 insertions(+), 56 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index d1fc3035e19c..090fe9554db7 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -91,7 +91,7 @@ struct ddebug_class_map {
* used to validate a "class FOO .." >control command on the module
*/
#define __DYNDBG_CLASSMAP_DEFINE(_var, _maptype, _base, ...) \
- const char *_var##_classnames[] = { __VA_ARGS__ }; \
+ static const char *_var##_classnames[] = { __VA_ARGS__ }; \
struct ddebug_class_map __aligned(8) __used \
__section("__dyndbg_classes") _var = { \
.mod = THIS_MODULE, \
@@ -163,6 +163,41 @@ struct ddebug_class_param {
const struct ddebug_class_map *map;
};
+/**
+ * DYNDBG_CLASSMAP_PARAM - wrap a dyndbg-classmap with a controlling sys-param
+ * @_name sysfs node name
+ * @_var name of the struct classmap var defining the controlled classes
+ * @_flags flags to be toggled, typically just 'p'
+ *
+ * Creates a sysfs-param to control the classes defined by the
+ * classmap. Keeps bits in a private/static
+ */
+#define DYNDBG_CLASSMAP_PARAM(_name, _var, _flags) \
+ static unsigned long _name##_bvec; \
+ __DYNDBG_CLASSMAP_PARAM(_name, _name##_bvec, _var, _flags)
+
+/**
+ * DYNDBG_CLASSMAP_PARAM_REF - wrap a dyndbg-classmap with a controlling
sys-param
+ * @_name sysfs node name
+ * @_bits name of the module's unsigned long bit-vector, ex: __drm_debug
+ * @_var name of the struct classmap var defining the controlled classes
+ * @_flags flags to be toggled, typically just 'p'
+ *
+ * Creates a sysfs-param to control the classmap, keeping bitvec in user
@_bits.
+ * This lets drm use __drm_debug elsewhere too.
+ */
+#define DYNDBG_CLASSMAP_PARAM_REF(_name, _bits, _var, _flags) \
+ __DYNDBG_CLASSMAP_PARAM(_name, _bits, _var, _flags)
+
+#define __DYNDBG_CLASSMAP_PARAM(_name, _bits, _var, _flags)\
+ static struct ddebug_class_param _name##_##_flags = { \
+ .bits = &(_bits), \
+ .flags = #_flags, \
+ .map = &(_var), \
+ }; \
+ module_param_cb(_name, _ops_dyndbg_classes, \
+ &_name##_##_flags, 0600)
+
/*
* pr_debug() and friends are globally enabled or modules have selectively
* enabled them.
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index f0a274a3cc9e..31fd67597928 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -657,6 +657,30 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
#define CLASSMAP_BITMASK(width) ((1UL << (width)) - 1)
+static void ddebug_class_param_clamp_input(unsigned long *inrep, const struct
kernel_param *kp)
+{
+ const struct ddebug_class_param *dcp = kp->arg;
+ const struct ddebug_class_map *map = dcp->map;
+
+ switch (map->map_type) {
+ case DD_CLASS_TYPE_DISJOINT_BITS:
+ /* expect bits. mask and warn if too many */
+ if (*inrep & ~CLASSMAP_BITMASK(map->length)) {
+ pr_warn("%s: input: 0x%lx exceeds mask: 0x%lx,
masking\n",
+ KP_NAME(kp), *inrep,
CLASSMAP_BITMASK(map->length));
+ *inrep &= CLASSMAP_BITMASK(map->length);
+ }
+ break;
+ case DD_CLASS_TYPE_LEVEL_NUM:
+ /* input is bitpos, of highest verbosity to be enabled */
+ if (*inrep > map->length) {
+ pr_warn("%s: level:%ld exceeds max:%d, clamping\n",
+ KP_NAME(kp), *inrep,
[PATCH v8 18/35] selftests-dyndbg: exit 127 if no facility
Test if /proc/dynamic_debug/control exists, exit 127 otherwise.
This distinguishes an untestable config from both pass & fail.
The 127 choice is pretty arbitrary, but imitating bisect.
That control file's presense guarantees that dynamic-debugging is
configured (unless /proc is off, unusually), without dealing with the
mount.
Signed-off-by: Jim Cromie
---
tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh | 5 +
1 file changed, 5 insertions(+)
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
index 1be70af26a38..cb77ae142520 100755
--- a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -11,6 +11,11 @@ CYAN="\033[0;36m"
NC="\033[0;0m"
error_msg=""
+[ -e /proc/dynamic_debug/control ] || {
+echo -e "${RED}: kernel not configured for this test ${NC}"
+exit 127
+}
+
function vx () {
echo $1 > /sys/module/dynamic_debug/parameters/verbose
}
--
2.44.0
[PATCH v8 14/35] dyndbg: reduce verbose=3 messages in ddebug_add_module
When modprobing a module, dyndbg currently logs/says "add-module", and
then "skipping" if the module has no prdbgs. Instead just check 1st
and return quietly.
no functional change
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index d4a0ae31d059..43a8e04b8599 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1245,11 +1245,10 @@ static int ddebug_add_module(struct _ddebug_info *di,
const char *modname)
{
struct ddebug_table *dt;
- v3pr_info("add-module: %s.%d sites\n", modname, di->num_descs);
- if (!di->num_descs) {
- v3pr_info(" skip %s\n", modname);
+ if (!di->num_descs)
return 0;
- }
+
+ v3pr_info("add-module: %s %d sites\n", modname, di->num_descs);
dt = kzalloc(sizeof(*dt), GFP_KERNEL);
if (dt == NULL) {
--
2.44.0
[PATCH v8 09/35] dyndbg: drop NUM_TYPE_ARRAY
ARRAY_SIZE works here, since array decl is complete.
no functional change
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index b53217e4b711..8116d0a0d33a 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -106,11 +106,9 @@ struct ddebug_class_map {
.mod_name = KBUILD_MODNAME, \
.base = _base, \
.map_type = _maptype, \
- .length = NUM_TYPE_ARGS(char*, __VA_ARGS__),\
+ .length = ARRAY_SIZE(_var##_classnames),\
.class_names = _var##_classnames, \
}
-#define NUM_TYPE_ARGS(eltype, ...) \
-(sizeof((eltype[]){__VA_ARGS__}) / sizeof(eltype))
/* encapsulate linker provided built-in (or module) dyndbg data */
struct _ddebug_info {
--
2.44.0
[PATCH v8 10/35] dyndbg: reduce verbose/debug clutter
currently, for verbose=3, these are logged (blank lines for clarity):
dyndbg: query 0: "class DRM_UT_CORE +p" mod:*
dyndbg: split into words: "class" "DRM_UT_CORE" "+p"
dyndbg: op='+'
dyndbg: flags=0x1
dyndbg: *flagsp=0x1 *maskp=0x
dyndbg: parsed: func="" file="" module="" format="" lineno=0-0 class=...
dyndbg: no matches for query
dyndbg: no-match: func="" file="" module="" format="" lineno=0-0 class=...
dyndbg: processed 1 queries, with 0 matches, 0 errs
That is excessive, so this patch:
- shrinks 3 lines of 2nd stanza to single line
- drops 1st 2 lines of 3rd stanza
3rd line is like 1st, with result, not procedure.
2nd line is just status, retold in 4th, with more info.
New output:
dyndbg: query 0: "class DRM_UT_CORE +p" mod:*
dyndbg: split into words: "class" "DRM_UT_CORE" "+p"
dyndbg: op='+' flags=0x1 *flagsp=0x1 *maskp=0x
dyndbg: no-match: func="" file="" module="" format="" lineno=0-0 class=...
dyndbg: processed 1 queries, with 0 matches, 0 errs
no functional change.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 14 +++---
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 4a48f830507f..368381dbd266 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -266,9 +266,6 @@ static int ddebug_change(const struct ddebug_query *query,
}
mutex_unlock(_lock);
- if (!nfound && verbose)
- pr_info("no matches for query\n");
-
return nfound;
}
@@ -501,7 +498,6 @@ static int ddebug_parse_flags(const char *str, struct
flag_settings *modifiers)
pr_err("bad flag-op %c, at start of %s\n", *str, str);
return -EINVAL;
}
- v3pr_info("op='%c'\n", op);
for (; *str ; ++str) {
for (i = ARRAY_SIZE(opt_array) - 1; i >= 0; i--) {
@@ -515,7 +511,6 @@ static int ddebug_parse_flags(const char *str, struct
flag_settings *modifiers)
return -EINVAL;
}
}
- v3pr_info("flags=0x%x\n", modifiers->flags);
/* calculate final flags, mask based upon op */
switch (op) {
@@ -531,7 +526,7 @@ static int ddebug_parse_flags(const char *str, struct
flag_settings *modifiers)
modifiers->flags = 0;
break;
}
- v3pr_info("*flagsp=0x%x *maskp=0x%x\n", modifiers->flags,
modifiers->mask);
+ v3pr_info("op='%c' flags=0x%x maskp=0x%x\n", op, modifiers->flags,
modifiers->mask);
return 0;
}
@@ -541,7 +536,7 @@ static int ddebug_exec_query(char *query_string, const char
*modname)
struct flag_settings modifiers = {};
struct ddebug_query query = {};
#define MAXWORDS 9
- int nwords, nfound;
+ int nwords;
char *words[MAXWORDS];
nwords = ddebug_tokenize(query_string, words, MAXWORDS);
@@ -559,10 +554,7 @@ static int ddebug_exec_query(char *query_string, const
char *modname)
return -EINVAL;
}
/* actually go and implement the change */
- nfound = ddebug_change(, );
- vpr_info_dq(, nfound ? "applied" : "no-match");
-
- return nfound;
+ return ddebug_change(, );
}
/* handle multiple queries in query string, continue on error, return
--
2.44.0
[PATCH v8 17/35] selftests-dyndbg: add tools/testing/selftests/dynamic_debug/*
Add a selftest script for dynamic-debug. The config requires
CONFIG_TEST_DYNAMIC_DEBUG=m (and CONFIG_TEST_DYNAMIC_DEBUG_SUBMOD=m),
which tacitly requires either CONFIG_DYNAMIC_DEBUG=y or
CONFIG_DYNAMIC_DEBUG_CORE=y
ATM this has just basic_tests(), it modifies pr_debug flags in a few
builtins (init/main, params), counts the callsite flags changed, and
verifies against expected values.
This is backported from another feature branch; the support-fns (thx
Lukas) have unused features at the moment, they'll get used shortly.
The script enables simple virtme-ng testing:
$> vng --verbose --name v6.8-32-g30d431000676 --user root \
--cwd ../.. -a dynamic_debug.verbose=2 -p 4 \
./tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
virtme: waiting for virtiofsd to start
virtme: use 'microvm' QEMU architecture
...
[4.136168] virtme-init: Setting hostname to v6.8-32-g30d431000676...
[4.240874] virtme-init: starting script
test_dynamic_debug_submod not there
test_dynamic_debug not there
...
[4.474435] virtme-init: script returned {0}
Powering off.
[4.529318] ACPI: PM: Preparing to enter system sleep state S5
[4.529991] kvm: exiting hardware virtualization
[4.530428] reboot: Power down
And add dynamic_debug to TARGETS, so `make run_tests` sees it properly
for the impatient, set TARGETS explicitly:
bash-5.2# make TARGETS=dynamic_debug run_tests
make[1]: ...
TAP version 13
1..1
[ 35.552922] dyndbg: read 3 bytes from userspace
[ 35.553099] dyndbg: query 0: "=_" mod:*
[ 35.553544] dyndbg: processed 1 queries, with 1778 matches, 0 errs
...
TLDR:
This selftest is slightly naive wrt the init state of call-site flags.
In particular, it fails if class'd pr_debugs have been set
$ cat /etc/modprobe.d/drm-test.conf
options drm dyndbg=class,DRM_UT_CORE,+mfslt%class,DRM_UT_KMS,+mf
By Contract, class'd pr_debugs are protected from alteration by
default (only by direct "class FOO" queries), so the "=_" logged above
(TAP version 13) cannot affect the DRM_UT_CORE,KMS pr_debugs.
These class'd flag-settings, added by modprobe, alter the counts of
flag-matching patterns, breaking the tests' expectations.
Signed-off-by: Jim Cromie
Co-developed-by: Łukasz Bartosik
Signed-off-by: Łukasz Bartosik
---
MAINTAINERS | 1 +
tools/testing/selftests/Makefile | 1 +
.../testing/selftests/dynamic_debug/Makefile | 9 +
tools/testing/selftests/dynamic_debug/config | 2 +
.../dynamic_debug/dyndbg_selftest.sh | 231 ++
5 files changed, 244 insertions(+)
create mode 100644 tools/testing/selftests/dynamic_debug/Makefile
create mode 100644 tools/testing/selftests/dynamic_debug/config
create mode 100755 tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
diff --git a/MAINTAINERS b/MAINTAINERS
index cf9fccbc6bde..bdffd192d3df 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7526,6 +7526,7 @@ S:Maintained
F: include/linux/dynamic_debug.h
F: lib/dynamic_debug.c
F: lib/test_dynamic_debug*.c
+F: tools/testing/selftest/dynamic_debug/*
DYNAMIC INTERRUPT MODERATION
M: Tal Gilboa
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index e1504833654d..84edf0bd8e80 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -20,6 +20,7 @@ TARGETS += drivers/s390x/uvdevice
TARGETS += drivers/net/bonding
TARGETS += drivers/net/team
TARGETS += dt
+TARGETS += dynamic_debug
TARGETS += efivarfs
TARGETS += exec
TARGETS += fchmodat2
diff --git a/tools/testing/selftests/dynamic_debug/Makefile
b/tools/testing/selftests/dynamic_debug/Makefile
new file mode 100644
index ..6d06fa7f1040
--- /dev/null
+++ b/tools/testing/selftests/dynamic_debug/Makefile
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+# borrowed from Makefile for user memory selftests
+
+# No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
+all:
+
+TEST_PROGS := dyndbg_selftest.sh
+
+include ../lib.mk
diff --git a/tools/testing/selftests/dynamic_debug/config
b/tools/testing/selftests/dynamic_debug/config
new file mode 100644
index ..d080da571ac0
--- /dev/null
+++ b/tools/testing/selftests/dynamic_debug/config
@@ -0,0 +1,2 @@
+CONFIG_TEST_DYNAMIC_DEBUG=m
+CONFIG_TEST_DYNAMIC_DEBUG_SUBMOD=m
diff --git a/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
new file mode 100755
index ..1be70af26a38
--- /dev/null
+++ b/tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
@@ -0,0 +1,231 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+
+V=${V:=0} # invoke as V=1 $0 for global verbose
+RED="\033[0;31m"
+GREEN="\033[0;32m"
+YELLOW="\033[0;33m"
+BLUE="\033[0;34m"
+MAGENTA="\033[0;35m"
+CYAN="\033[0;36m"
+NC="\033[0;0m"
+error_msg=""
+
+function vx () {
+echo $1 > /sys/module/dynamic_debug/parameters/verbose
+}
+
+function ddgrep
[PATCH v8 08/35] dyndbg: split param_set_dyndbg_classes to _module & wrapper fns
Split api-fn: param_set_dyndbg_classes(), adding modname param and
passing NULL in from api-fn.
The new arg allows caller to specify that only one module is affected
by a prdbgs update. This selectivity will be used later to narrow the
scope of changes made.
no functional change.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 37 ++---
1 file changed, 22 insertions(+), 15 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index a1fd2e9dbafb..4a48f830507f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -711,18 +711,9 @@ static int param_set_dyndbg_classnames(const char *instr,
const struct kernel_pa
return 0;
}
-/**
- * param_set_dyndbg_classes - class FOO >control
- * @instr: string echo>d to sysfs, input depends on map_type
- * @kp:kp->arg has state: bits/lvl, map, map_type
- *
- * Enable/disable prdbgs by their class, as given in the arguments to
- * DECLARE_DYNDBG_CLASSMAP. For LEVEL map-types, enforce relative
- * levels by bitpos.
- *
- * Returns: 0 or <0 if error.
- */
-int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
+static int param_set_dyndbg_module_classes(const char *instr,
+ const struct kernel_param *kp,
+ const char *modnm)
{
const struct ddebug_class_param *dcp = kp->arg;
const struct ddebug_class_map *map = dcp->map;
@@ -759,8 +750,8 @@ int param_set_dyndbg_classes(const char *instr, const
struct kernel_param *kp)
KP_NAME(kp), inrep,
CLASSMAP_BITMASK(map->length));
inrep &= CLASSMAP_BITMASK(map->length);
}
- v2pr_info("bits:%lx > %s\n", inrep, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, , dcp->bits,
NULL);
+ v2pr_info("bits:0x%lx > %s.%s\n", inrep, modnm ?: "*",
KP_NAME(kp));
+ totct += ddebug_apply_class_bitmap(dcp, , dcp->bits,
modnm);
*dcp->bits = inrep;
break;
case DD_CLASS_TYPE_LEVEL_NUM:
@@ -773,7 +764,7 @@ int param_set_dyndbg_classes(const char *instr, const
struct kernel_param *kp)
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
new_bits = CLASSMAP_BITMASK(inrep);
v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits,
KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, _bits, _bits,
NULL);
+ totct += ddebug_apply_class_bitmap(dcp, _bits, _bits,
modnm);
*dcp->lvl = inrep;
break;
default:
@@ -782,6 +773,22 @@ int param_set_dyndbg_classes(const char *instr, const
struct kernel_param *kp)
vpr_info("%s: total matches: %d\n", KP_NAME(kp), totct);
return 0;
}
+
+/**
+ * param_set_dyndbg_classes - class FOO >control
+ * @instr: string echo>d to sysfs, input depends on map_type
+ * @kp:kp->arg has state: bits/lvl, map, map_type
+ *
+ * Enable/disable prdbgs by their class, as given in the arguments to
+ * DECLARE_DYNDBG_CLASSMAP. For LEVEL map-types, enforce relative
+ * levels by bitpos.
+ *
+ * Returns: 0 or <0 if error.
+ */
+int param_set_dyndbg_classes(const char *instr, const struct kernel_param *kp)
+{
+ return param_set_dyndbg_module_classes(instr, kp, NULL);
+}
EXPORT_SYMBOL(param_set_dyndbg_classes);
/**
--
2.44.0
[PATCH v8 15/35] dyndbg-API: remove DD_CLASS_TYPE_(DISJOINT|LEVEL)_NAMES and code
Remove the NAMED class types; these 2 classmap types accept class
names at the PARAM interface, for example:
echo +DRM_UT_CORE,-DRM_UT_KMS > /sys/module/drm/parameters/debug_names
The code works, but its only used by test-dynamic-debug, and wasn't
asked for by anyone else, so reduce test-surface, simplify things.
also rename enum class_map_type to enum ddebug_class_map_type.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 23 ++--
lib/dynamic_debug.c | 102 +++---
lib/test_dynamic_debug.c | 26 -
3 files changed, 14 insertions(+), 137 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 8116d0a0d33a..dd304e231f08 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -58,27 +58,16 @@ struct _ddebug {
#endif
} __attribute__((aligned(8)));
-enum class_map_type {
+enum ddebug_class_map_type {
DD_CLASS_TYPE_DISJOINT_BITS,
/**
-* DD_CLASS_TYPE_DISJOINT_BITS: classes are independent, one per bit.
-* expecting hex input. Built for drm.debug, basis for other types.
+* DD_CLASS_TYPE_DISJOINT_BITS: classes are independent, mapped to
bits[0..N].
+* Expects hex input. Built for drm.debug, basis for other types.
*/
DD_CLASS_TYPE_LEVEL_NUM,
/**
-* DD_CLASS_TYPE_LEVEL_NUM: input is numeric level, 0-N.
-* N turns on just bits N-1 .. 0, so N=0 turns all bits off.
-*/
- DD_CLASS_TYPE_DISJOINT_NAMES,
- /**
-* DD_CLASS_TYPE_DISJOINT_NAMES: input is a CSV of [+-]CLASS_NAMES,
-* classes are independent, like _DISJOINT_BITS.
-*/
- DD_CLASS_TYPE_LEVEL_NAMES,
- /**
-* DD_CLASS_TYPE_LEVEL_NAMES: input is a CSV of [+-]CLASS_NAMES,
-* intended for names like: INFO,DEBUG,TRACE, with a module prefix
-* avoid EMERG,ALERT,CRIT,ERR,WARNING: they're not debug
+* DD_CLASS_TYPE_LEVEL_NUM: input is numeric level, 0..N.
+* Input N turns on bits 0..N-1
*/
};
@@ -88,7 +77,7 @@ struct ddebug_class_map {
const char **class_names;
const int length;
const int base; /* index of 1st .class_id, allows split/shared
space */
- enum class_map_type map_type;
+ enum ddebug_class_map_type map_type;
};
/**
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 43a8e04b8599..d5701207febc 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -636,76 +636,6 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
#define CLASSMAP_BITMASK(width) ((1UL << (width)) - 1)
-/* accept comma-separated-list of [+-] classnames */
-static int param_set_dyndbg_classnames(const char *instr, const struct
kernel_param *kp)
-{
- const struct ddebug_class_param *dcp = kp->arg;
- const struct ddebug_class_map *map = dcp->map;
- unsigned long curr_bits, old_bits;
- char *cl_str, *p, *tmp;
- int cls_id, totct = 0;
- bool wanted;
-
- cl_str = tmp = kstrdup_and_replace(instr, '\n', '\0', GFP_KERNEL);
- if (!tmp)
- return -ENOMEM;
-
- /* start with previously set state-bits, then modify */
- curr_bits = old_bits = *dcp->bits;
- vpr_info("\"%s\" > %s:0x%lx\n", cl_str, KP_NAME(kp), curr_bits);
-
- for (; cl_str; cl_str = p) {
- p = strchr(cl_str, ',');
- if (p)
- *p++ = '\0';
-
- if (*cl_str == '-') {
- wanted = false;
- cl_str++;
- } else {
- wanted = true;
- if (*cl_str == '+')
- cl_str++;
- }
- cls_id = match_string(map->class_names, map->length, cl_str);
- if (cls_id < 0) {
- pr_err("%s unknown to %s\n", cl_str, KP_NAME(kp));
- continue;
- }
-
- /* have one or more valid class_ids of one *_NAMES type */
- switch (map->map_type) {
- case DD_CLASS_TYPE_DISJOINT_NAMES:
- /* the +/- pertains to a single bit */
- if (test_bit(cls_id, _bits) == wanted) {
- v3pr_info("no change on %s\n", cl_str);
- continue;
- }
- curr_bits ^= BIT(cls_id);
- totct += ddebug_apply_class_bitmap(dcp, _bits,
*dcp->bits, NULL);
- *dcp->bits = curr_bits;
- v2pr_info("%s: changed bit %d:%s\n", KP_NAME(kp),
cls_id,
- map->class_names[cls_id]);
- break;
- case DD_CLASS_TYPE_LEVEL_NAMES:
- /* cls_id = N in 0..max. wanted +/- determines N or N-1
*/
-
[PATCH v8 12/35] dyndbg: tighten ddebug_class_name() 1st arg type
Change function's 1st arg-type, and deref in the caller.
The fn doesn't need any other fields in the struct.
no functional change.
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 8320cadeb251..882354e1e78f 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -1120,12 +1120,12 @@ static void *ddebug_proc_next(struct seq_file *m, void
*p, loff_t *pos)
#define class_in_range(class_id, map) \
(class_id >= map->base && class_id < map->base + map->length)
-static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug
*dp)
+static const char *ddebug_class_name(struct ddebug_table *dt, struct _ddebug
*dp)
{
- struct ddebug_class_map *map = iter->table->classes;
- int i, nc = iter->table->num_classes;
+ struct ddebug_class_map *map = dt->classes;
+ int i;
- for (i = 0; i < nc; i++, map++)
+ for (i = 0; i < dt->num_classes; i++, map++)
if (class_in_range(dp->class_id, map))
return map->class_names[dp->class_id - map->base];
@@ -1159,7 +1159,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p)
seq_puts(m, "\"");
if (dp->class_id != _DPRINTK_CLASS_DFLT) {
- class = ddebug_class_name(iter, dp);
+ class = ddebug_class_name(iter->table, dp);
if (class)
seq_printf(m, " class:%s", class);
else
--
2.44.0
[PATCH v8 16/35] dyndbg-API: fix DECLARE_DYNDBG_CLASSMAP
DECLARE_DYNDBG_CLASSMAP() has a design error; its usage fails a basic
K rule: "define once, refer many times".
It is used across DRM core & drivers, each use re-defines the classmap
understood by that module; and all must match for the modules to
respond together when DRM.debug categories are enabled. This is
brittle, rubbish.
Worse, it causes the CONFIG_DRM_USE_DYNAMIC_DEBUG=Y regression; 1st
drm.ko loads, and dyndbg initializes its DRM.debug callsites, then a
drm-driver loads, but too late - it missed the DRM.debug enablement.
So replace it with 2 macros:
DYNDBG_CLASSMAP_DEFINE - invoked once from core - drm.ko
DYNDBG_CLASSMAP_USE- from all drm drivers and helpers.
DYNDBG_CLASSMAP_DEFINE: it reuses a renamed DECLARE_DYNDBG_CLASSMAP to
construct the struct classmap variable, but it drops the static
qualifier, and exports it instead.
DYNDBG_CLASSMAP_USE: then refers to the exported var by name:
* used from drivers, helper-mods
* lets us drop the repetitive "classname" args
* fixes 2nd-defn problem
* creates a ddebug_class_user record in new __dyndbg_class_users section
this allows ddebug_add_module(etal) to handle them per-module.
DECLARE_DYNDBG_CLASSMAP is preserved temporarily, to decouple DRM
adaptation work and avoid compile-errs before its done. IOW, DRM
gets fixed when they commit the adopt-new-api patches.
The DEFINE,USE distinction, and the separate usage record, allows
dyndbg to initialize the drivers & helpers DRM.debug callsites
separately after each is modprobed.
Basically, the classmap init-scan repeated for classmap-users.
To review, dyndbg's existing __dyndbg_classes[] section does:
. catalogs the classmaps defined by a module (or builtin modules)
. authorizes dyndbg to >control those class'd prdbgs for the module.
. DYNDBG_CLASSMAP_DEFINE creates classmaps in this section.
This patch adds __dyndbg_class_users[] section:
. catalogs uses/references to the classmap definitions.
. authorizes dyndbg to >control those class'd prdbgs in ref'g module.
. DYNDBG_CLASSMAP_USE() creates classmap-user records in this new section.
Now ddebug_add_module(etal) can handle classmap-uses similar to (and
after) classmaps; when a dependent module is loaded, its parent's
kernel params are scanned to find if a param is wired to dyndbg's
param-ops, whose classmap ref is the one being looked for.
To support this, theres a few data/header changes:
new struct ddebug_class_user
contains: user-module-name,
it records drm-driver's use of a classmap in the section, allowing lookup
struct ddebug_info gets 2 more fields to keep the new section with the others:
class_users, num_class_users.
set by dynamic_debug_init() for builtins.
or by kernel/module/main:load_info() for loadable modules.
vmlinux.lds.h: new BOUNDED_SECTION for __dyndbg_class_users
dynamic_debug.c has 2 changes in ddebug_add_module(), ddebug_change():
ddebug_add_module() already calls ddebug_attach_module_classes()
to handle classmaps DEFINEd by a module, now it also calls
ddebug_attach_user_module_classes() to handle USEd classmaps. To
avoid this work when possible, 1st scan the module's descriptors and
count the number of class'd pr_debugs.
ddebug_attach_user_module_classes() scans the module's class_users
section, follows the refs to the parent's classmap, and calls
ddebug_apply_params() on each. It also avoids work by checking the
module's class-ct.
ddebug_apply_params(new fn):
It scans module's/builtin kernel-params, calls ddebug_match_apply_kparam
for each to find the params/sysfs-nodes which may be wired to a classmap.
ddebug_match_apply_kparam(new fn):
1st, it tests the kernel-param.ops is dyndbg's; this guarantees that
the attached arg is a struct ddebug_class_param, which has a ref to
the param's state, and to the classmap defining the param's handling.
2nd, it requires that the classmap ref'd by the kparam is the one
we're called for; modules can use many separate classmaps (as
test_dynamic_debug does).
Then apply the "parent" kparam's setting to the dependent module,
using ddebug_apply_class_bitmap().
ddebug_change(and callees) also gets adjustments:
ddebug_find_valid_class(): This does a search over the module's
classmaps, looking for the class FOO echo'd to >control. So now it
searches over __dyndbg_class_users[] after __dyndbg_classes[].
ddebug_class_name(): return class-names for defined AND used classes.
test_dynamic_debug.c, test_dynamic_debug_submod.c:
This demonstrates the 2 types of classmaps & sysfs-params, following
the 4-part recipe:
1. define an enum for the classmap: DRM.debug has DRM_UT_{CORE,KMS,...}
multiple classes must share 0-62 classid space.
2. DYNDBG_CLASSMAP_DEFINE(.. DRM_UT_{CORE,KMS,...})
3. DYNDBG_CLASSMAP_PARAM* (classmap)
4. DYNDBG_CLASSMAP_USE()
by _submod only, skipping 2,3
Move all the enum declarations together, to better explain how they
share the 0..62 class-id space available to a module (non-overlapping
subranges).
reorg macros 2,3 by name.
[PATCH v8 06/35] dyndbg: replace classmap list with a vector
Classmaps are stored in an elf section/array, but are individually
list-linked onto dyndbg's per-module ddebug_table for operation.
This is unnecessary; even when ddebug_attach_classmap() is handling
the builtin section (with classmaps for multiple builtin modules), its
contents are ordered, so a module's possibly multiple classmaps will
be consecutive in the section, and could be treated as a vector/block,
since both start-addy and subrange length are in the ddebug_info arg.
IOW, this treats classmaps similarly to _ddebugs, which are already
kept as vector-refs (address+len).
So this changes:
struct ddebug_class_map drops list-head link.
struct ddebug_table drops the list-head maps, and gets: classes &
num_classes for the start-addy and num_classes, placed to improve
struct packing.
The loading: in ddebug_attach_module_classes(), replace the
for-the-modname list-add loop, with a forloop that finds the module's
subrange (start,length) of matching classmaps within the possibly
builtin classmaps vector, and saves those to the ddebug_table.
The reading/using: change list-foreach loops in ddebug_class_name() &
ddebug_find_valid_class() to walk the array from start to length.
Also:
Move #define __outvar up, above an added use in a fn-prototype.
Simplify ddebug_attach_module_classes args, ref has both addy,len.
no functional changes
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 1 -
lib/dynamic_debug.c | 61 ++-
2 files changed, 32 insertions(+), 30 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 5231aaf361c4..b53217e4b711 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -83,7 +83,6 @@ enum class_map_type {
};
struct ddebug_class_map {
- struct list_head link;
struct module *mod;
const char *mod_name; /* needed for builtins */
const char **class_names;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 152b04c05981..46e4cdd8e6be 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -45,10 +45,11 @@ extern struct ddebug_class_map __start___dyndbg_classes[];
extern struct ddebug_class_map __stop___dyndbg_classes[];
struct ddebug_table {
- struct list_head link, maps;
+ struct list_head link;
const char *mod_name;
- unsigned int num_ddebugs;
struct _ddebug *ddebugs;
+ struct ddebug_class_map *classes;
+ unsigned int num_ddebugs, num_classes;
};
struct ddebug_query {
@@ -147,13 +148,15 @@ static void vpr_info_dq(const struct ddebug_query *query,
const char *msg)
query->first_lineno, query->last_lineno, query->class_string);
}
+#define __outvar /* filled by callee */
static struct ddebug_class_map *ddebug_find_valid_class(struct ddebug_table
const *dt,
- const char
*class_string, int *class_id)
+ const char
*class_string,
+ __outvar int *class_id)
{
struct ddebug_class_map *map;
- int idx;
+ int i, idx;
- list_for_each_entry(map, >maps, link) {
+ for (map = dt->classes, i = 0; i < dt->num_classes; i++, map++) {
idx = match_string(map->class_names, map->length, class_string);
if (idx >= 0) {
*class_id = idx + map->base;
@@ -164,7 +167,6 @@ static struct ddebug_class_map
*ddebug_find_valid_class(struct ddebug_table cons
return NULL;
}
-#define __outvar /* filled by callee */
/*
* Search the tables for _ddebug's which match the given `query' and
* apply the `flags' and `mask' to them. Returns number of matching
@@ -1114,9 +1116,10 @@ static void *ddebug_proc_next(struct seq_file *m, void
*p, loff_t *pos)
static const char *ddebug_class_name(struct ddebug_iter *iter, struct _ddebug
*dp)
{
- struct ddebug_class_map *map;
+ struct ddebug_class_map *map = iter->table->classes;
+ int i, nc = iter->table->num_classes;
- list_for_each_entry(map, >table->maps, link)
+ for (i = 0; i < nc; i++, map++)
if (class_in_range(dp->class_id, map))
return map->class_names[dp->class_id - map->base];
@@ -1200,30 +1203,31 @@ static const struct proc_ops proc_fops = {
.proc_write = ddebug_proc_write
};
-static void ddebug_attach_module_classes(struct ddebug_table *dt,
-struct ddebug_class_map *classes,
-int num_classes)
+static void ddebug_attach_module_classes(struct ddebug_table *dt, struct
_ddebug_info *di)
{
struct ddebug_class_map *cm;
- int i, j, ct = 0;
+ int i, nc = 0;
- for (cm = classes, i = 0; i < num_classes; i++, cm++) {
+ /*
+* Find this module's classmaps in a
[PATCH v8 04/35] dyndbg: reword "class unknown, " to "class:_UNKNOWN_"
When a dyndbg classname is unknown to a kernel module (as before previous patch), the callsite is un-addressable via >control queries. The control-file displays this condition as "class unknown," currently. That spelling is sub-optimal, so change it to "class:_UNKNOWN_" to loudly announce the erroneous situation, and to make it exceedingly greppable. Signed-off-by: Jim Cromie --- lib/dynamic_debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c index f2c5e7910bb1..73ccf947d4aa 100644 --- a/lib/dynamic_debug.c +++ b/lib/dynamic_debug.c @@ -1154,7 +1154,7 @@ static int ddebug_proc_show(struct seq_file *m, void *p) if (class) seq_printf(m, " class:%s", class); else - seq_printf(m, " class unknown, _id:%d", dp->class_id); + seq_printf(m, " class:_UNKNOWN_ _id:%d", dp->class_id); } seq_puts(m, "\n"); -- 2.44.0
[PATCH v8 05/35] dyndbg: make ddebug_class_param union members same size
struct ddebug_class_param keeps a ref to the state-storage of the
param; make both class-types use the same unsigned long storage type.
ISTM this is simpler and safer.
Signed-off-by: Jim Cromie
---
include/linux/dynamic_debug.h | 2 +-
lib/dynamic_debug.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 4fcbf4d4fd0a..5231aaf361c4 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -124,7 +124,7 @@ struct _ddebug_info {
struct ddebug_class_param {
union {
unsigned long *bits;
- unsigned int *lvl;
+ unsigned long *lvl;
};
char flags[8];
const struct ddebug_class_map *map;
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 73ccf947d4aa..152b04c05981 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -799,7 +799,7 @@ int param_get_dyndbg_classes(char *buffer, const struct
kernel_param *kp)
case DD_CLASS_TYPE_LEVEL_NAMES:
case DD_CLASS_TYPE_LEVEL_NUM:
- return scnprintf(buffer, PAGE_SIZE, "%d\n", *dcp->lvl);
+ return scnprintf(buffer, PAGE_SIZE, "%ld\n", *dcp->lvl);
default:
return -1;
}
--
2.44.0
[PATCH v8 07/35] dyndbg: ddebug_apply_class_bitmap - add module arg, select on it
Add param: query_module to ddebug_apply_class_bitmap(), and pass it
thru to _ddebug_queries(), replacing NULL with query_module. This
allows its caller to update just one module, or all (as currently).
We'll use this later to propagate drm.debug to each USEr as they're
modprobed.
No functional change.
Signed-off-by: Jim Cromie
---
after `modprobe i915`, heres the module dependencies,
though not all on drm.debug.
bash-5.2# lsmod
Module Size Used by
i915 3133440 0
drm_buddy 20480 1 i915
ttm90112 1 i915
i2c_algo_bit 16384 1 i915
video 61440 1 i915
wmi32768 1 video
drm_display_helper200704 1 i915
drm_kms_helper208896 2 drm_display_helper,i915
drm 606208 5
drm_kms_helper,drm_display_helper,drm_buddy,i915,ttm
cec57344 2 drm_display_helper,i915
---
lib/dynamic_debug.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index 46e4cdd8e6be..a1fd2e9dbafb 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -605,7 +605,8 @@ static int ddebug_exec_queries(char *query, const char
*modname)
/* apply a new bitmap to the sys-knob's current bit-state */
static int ddebug_apply_class_bitmap(const struct ddebug_class_param *dcp,
-unsigned long *new_bits, unsigned long
*old_bits)
+unsigned long *new_bits, unsigned long
*old_bits,
+const char *query_modname)
{
#define QUERY_SIZE 128
char query[QUERY_SIZE];
@@ -613,7 +614,8 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
int matches = 0;
int bi, ct;
- v2pr_info("apply: 0x%lx to: 0x%lx\n", *new_bits, *old_bits);
+ v2pr_info("apply bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
*old_bits,
+ query_modname ?: "");
for (bi = 0; bi < map->length; bi++) {
if (test_bit(bi, new_bits) == test_bit(bi, old_bits))
@@ -622,12 +624,15 @@ static int ddebug_apply_class_bitmap(const struct
ddebug_class_param *dcp,
snprintf(query, QUERY_SIZE, "class %s %c%s",
map->class_names[bi],
test_bit(bi, new_bits) ? '+' : '-', dcp->flags);
- ct = ddebug_exec_queries(query, NULL);
+ ct = ddebug_exec_queries(query, query_modname);
matches += ct;
v2pr_info("bit_%d: %d matches on class: %s -> 0x%lx\n", bi,
ct, map->class_names[bi], *new_bits);
}
+ v2pr_info("applied bitmap: 0x%lx to: 0x%lx for %s\n", *new_bits,
*old_bits,
+ query_modname ?: "");
+
return matches;
}
@@ -682,7 +687,7 @@ static int param_set_dyndbg_classnames(const char *instr,
const struct kernel_pa
continue;
}
curr_bits ^= BIT(cls_id);
- totct += ddebug_apply_class_bitmap(dcp, _bits,
dcp->bits);
+ totct += ddebug_apply_class_bitmap(dcp, _bits,
dcp->bits, NULL);
*dcp->bits = curr_bits;
v2pr_info("%s: changed bit %d:%s\n", KP_NAME(kp),
cls_id,
map->class_names[cls_id]);
@@ -692,7 +697,7 @@ static int param_set_dyndbg_classnames(const char *instr,
const struct kernel_pa
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
curr_bits = CLASSMAP_BITMASK(cls_id + (wanted ? 1 : 0
));
- totct += ddebug_apply_class_bitmap(dcp, _bits,
_bits);
+ totct += ddebug_apply_class_bitmap(dcp, _bits,
_bits, NULL);
*dcp->lvl = (cls_id + (wanted ? 1 : 0));
v2pr_info("%s: changed bit-%d: \"%s\" %lx->%lx\n",
KP_NAME(kp), cls_id,
map->class_names[cls_id], old_bits,
curr_bits);
@@ -755,7 +760,7 @@ int param_set_dyndbg_classes(const char *instr, const
struct kernel_param *kp)
inrep &= CLASSMAP_BITMASK(map->length);
}
v2pr_info("bits:%lx > %s\n", inrep, KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, , dcp->bits);
+ totct += ddebug_apply_class_bitmap(dcp, , dcp->bits,
NULL);
*dcp->bits = inrep;
break;
case DD_CLASS_TYPE_LEVEL_NUM:
@@ -768,7 +773,7 @@ int param_set_dyndbg_classes(const char *instr, const
struct kernel_param *kp)
old_bits = CLASSMAP_BITMASK(*dcp->lvl);
new_bits = CLASSMAP_BITMASK(inrep);
v2pr_info("lvl:%ld bits:0x%lx > %s\n", inrep, new_bits,
KP_NAME(kp));
- totct += ddebug_apply_class_bitmap(dcp, _bits, _bits);
+
[PATCH v8 03/35] test-dyndbg: fixup CLASSMAP usage error
A more careful reading of logging output from test_dynamic_debug.ko
reveals:
lib/test_dynamic_debug.c:103 [test_dynamic_debug]do_cats =pmf "doing
categories\n"
lib/test_dynamic_debug.c:105 [test_dynamic_debug]do_cats =p "LOW msg\n"
class:MID
lib/test_dynamic_debug.c:106 [test_dynamic_debug]do_cats =p "MID msg\n" class:HI
lib/test_dynamic_debug.c:107 [test_dynamic_debug]do_cats =_ "HI msg\n" class
unknown, _id:13
107 says: HI is unknown, 105,106 have LOW/MID and MID/HI skew.
The enum's 1st val (explicitly initialized) was wrong; it must be
_base, not _base+1 (a DECLARE_DYNDBG_CLASSMAP param). So the last
enumeration exceeded the range of mapped class-id's, which triggered
the "class unknown" report. I coded in an error, intending to verify
err detection, then forgot, and missed that it was there.
So this patch fixes a bad usage of DECLARE_DYNDBG_CLASSMAP(), showing
that it is too error-prone. As noted in test-mod comments:
* Using the CLASSMAP api:
* - classmaps must have corresponding enum
* - enum symbols must match/correlate with class-name strings in the map.
* - base must equal enum's 1st value
* - multiple maps must set their base to share the 0-62 class_id space !!
Signed-off-by: Jim Cromie
---
lib/test_dynamic_debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/test_dynamic_debug.c b/lib/test_dynamic_debug.c
index 8dd250ad022b..a01f0193a419 100644
--- a/lib/test_dynamic_debug.c
+++ b/lib/test_dynamic_debug.c
@@ -75,7 +75,7 @@ DD_SYS_WRAP(disjoint_bits, p);
DD_SYS_WRAP(disjoint_bits, T);
/* symbolic input, independent bits */
-enum cat_disjoint_names { LOW = 11, MID, HI };
+enum cat_disjoint_names { LOW = 10, MID, HI };
DECLARE_DYNDBG_CLASSMAP(map_disjoint_names, DD_CLASS_TYPE_DISJOINT_NAMES, 10,
"LOW", "MID", "HI");
DD_SYS_WRAP(disjoint_names, p);
--
2.44.0
[PATCH v8 02/35] docs/dyndbg: update examples \012 to \n
commit 47ea6f99d06e ("dyndbg: use ESCAPE_SPACE for cat control")
changed the control-file to display format strings with "\n" rather
than "\012". Update the docs to match the new reality.
Signed-off-by: Jim Cromie
---
Documentation/admin-guide/dynamic-debug-howto.rst | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/dynamic-debug-howto.rst
b/Documentation/admin-guide/dynamic-debug-howto.rst
index 0e9b48daf690..6a8ce5a34382 100644
--- a/Documentation/admin-guide/dynamic-debug-howto.rst
+++ b/Documentation/admin-guide/dynamic-debug-howto.rst
@@ -52,12 +52,12 @@ query/commands to the control file. Example::
# grease the interface
:#> alias ddcmd='echo $* > /proc/dynamic_debug/control'
- :#> ddcmd '-p; module main func run* +p'
+ :#> ddcmd '-p; module main func run* +p' # disable all, then enable main
:#> grep =p /proc/dynamic_debug/control
- init/main.c:1424 [main]run_init_process =p " with arguments:\012"
- init/main.c:1426 [main]run_init_process =p "%s\012"
- init/main.c:1427 [main]run_init_process =p " with environment:\012"
- init/main.c:1429 [main]run_init_process =p "%s\012"
+ init/main.c:1424 [main]run_init_process =p " with arguments:\n"
+ init/main.c:1426 [main]run_init_process =p "%s\n"
+ init/main.c:1427 [main]run_init_process =p " with environment:\n"
+ init/main.c:1429 [main]run_init_process =p "%s\n"
Error messages go to console/syslog::
--
2.44.0
[PATCH v8 01/35] dyndbg: fix old BUG_ON in >control parser
Fix a BUG_ON from 2009. Even if it looks "unreachable" (I didn't
really look), lets make sure by removing it, doing pr_err and return
-EINVAL instead.
cc: sta...@vger.kernel.org
Signed-off-by: Jim Cromie
---
lib/dynamic_debug.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/dynamic_debug.c b/lib/dynamic_debug.c
index c78f335fa981..f2c5e7910bb1 100644
--- a/lib/dynamic_debug.c
+++ b/lib/dynamic_debug.c
@@ -302,7 +302,11 @@ static int ddebug_tokenize(char *buf, char *words[], int
maxwords)
} else {
for (end = buf; *end && !isspace(*end); end++)
;
- BUG_ON(end == buf);
+ if (end == buf) {
+ pr_err("parse err after word:%d=%s\n", nwords,
+ nwords ? words[nwords - 1] : "");
+ return -EINVAL;
+ }
}
/* `buf' is start of word, `end' is one past its end */
--
2.44.0
[PATCH v8 00/35] fix CONFIG_DRM_USE_DYNAMIC_DEBUG=y regression
hi Greg, Jason, DRM-folk,
This patchset fixes the CONFIG_DRM_USE_DYNAMIC_DEBUG=y regression,
Fixes: bb2ff6c27bc9 ("drm: Disable dynamic debug as broken")
this is v8.
Its also here:
https://github.com/jimc/linux/tree/dd-classmap-fix-8a
v7 had at least 2 problems:
https://lore.kernel.org/lkml/20231101002609.3533731-1-jim.cro...@gmail.com/
https://patchwork.freedesktop.org/series/125066/
1. missing __align(8) in METATDATA macro, giving too much placement
freedom to linker, caused weird segvs following non-ptr vals, but for
builtin modules only. found by lkp-test.
2. the main patch changed both the dyndbg API, and the drm/drivers.
This was a flag-day annoyance, and not practical. Fix by preserving
old API macro until "later", and splitting the patch and set into 2
sequential subsets. removal can wait.
What was broken ?
Booting a modular kernel with drm.debug=0x1ff, this enabled pr_debugs
only in drm itself, not the yet-to-be loaded driver + helpers. I had
tested with scripts doing lots of modprobes with dyndbg=<> options
permuting. I didn't notice that I didn't really test without them.
The deeper cause was my design error, a violation of the K rule:
"define once, refer many times".
DECLARE_DYNDBG_CLASSMAP defined the classmap, and was used everywhere,
re-declaring the same static classmap repeatedly. Jani Nikula actually
picked up on this at the time, but didn't scream loudly enough for
anyone to notice, I know I didn't get it then. One patchset across 2
trees didn't help either.
The revised classmap API "splits" it to def & ref.
DYNDBG_CLASSMAP_DEFINE fixes & updates the busted macro, EXPORTing the
classmap instead. It gets invoked once per subsystem, by the
parent/builtin, drm.ko for DRM.
DYNDBG_CLASSMAP_USE in drivers and helpers refer to the classmap by
name, which links the 2 modules (like __drm_debug already does).
These 2 tell dyndbg to map "class FOO" to the defined FOO_ID, which
allows it to make those changes via >control.
DYNDBG_CLASSMAP_PARAM*, defines the controlling kparam, and binds it
to both the _var, and the _DEFINEd classmap. So drm uses this to bind
the classmap to __drm_debug.
It provides the common control-point for the sub-system; it is applied
to the classmaps during modprobe of both _DEFINEr and USErs. It also
enforces the relative nature of LEVEL classmaps, ie V3>V2.
DECLARE_DYNDBG_CLASSMAP is preserved to decouple the DRM patches.
A new struct and elf section contain the _USEs; on modprobe, these are
scanned similarly to the _DEFINEs, but follow the references to their
defining modules, find the kparam wired to the classmap, and apply its
classmap settings to the USEr. This action is what V1 missed, which
is why drivers failed to enable debug during modprobe.
In order to recapitulate the regression scenario without involving
DRM, the patchset (v6 I think) adds test_dynamic_debug_submod, which
is a duplicate of its parent; _submod.c #defines _SUBMOD, and then
includes parent.
This puts _DEFINE and _USE close together in the same file, for
obviousness. It also guarantees that the submod always has the same
complement of debug()s, giving consistent output from both when
classmaps are working properly, as tested when changing callsites via
both param and >control.
To provide a turn-key selftest, the patchset also adds
./tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh, pilfered
from a debug-to-trace patchset I and Lukasz Bartozik have been working
out. It starts with basic_tests, then to test 2 new parsing
delimiters, which simplify testing of the classmap functionality.
It works nicely from virtme-ng:
[jimc@frodo vx]$ vrun_ --
./tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
doing: vng --verbose --name v6.9-rc5-34-g2f1ace6e1c68 \
--user root --cwd ../.. \
-a dynamic_debug.verbose=2 -p 4 \
-- ./tools/testing/selftests/dynamic_debug/dyndbg_selftest.sh
virtme: waiting for virtiofsd to start
...
[3.546739] ip (260) used greatest stack depth: 12368 bytes left
[3.609288] virtme-init: starting script
test_dynamic_debug_submod not there
test_dynamic_debug not there
# BASIC_TESTS
...
# Done on: Fri Apr 26 20:45:08 MDT 2024
[4.765751] virtme-init: script returned {0}
Powering off.
[4.805790] ACPI: PM: Preparing to enter system sleep state S5
[4.806223] kvm: exiting hardware virtualization
[4.806564] reboot: Power down
[jimc@frodo vx]$
I've been running the kernel on my x86 desktop & laptop, booting with
drm.debug=0x1f, then turning it all-off after sleep 15.
a few highlights from a bare-metal boot:
here modprobe amdgpu; dyndbg applies last bit/class/category, and finishes init,
then drm and amdgpu start logging as they execute
...
[9.019696] gandalf kernel: dyndbg: query 0: "class DRM_UT_ATOMIC +p"
mod:amdgpu
[9.019704] gandalf kernel: dyndbg: class-ref: amdgpu.DRM_UT_ATOMIC
module:amdgpu nd:4754 nc:0 nu:1
[9.020012] gandalf kernel: dyndbg: processed 1 queries, with 21
Re: drm-misc migration to Gitlab server
On 29/04/2024 14:56, Jani Nikula wrote: On Mon, 29 Apr 2024, Helen Koike wrote: On 01/04/2024 13:49, Tvrtko Ursulin wrote: No issues accessing the repo just a slight confusion and how to handle the workflow. More specifically, if I have a patch which wants to be merged to drm-misc-next, is the mailing list based worklflow still the way to go, or I should create a merge request, or I should apply for commit access via some new method other than adding permissions to my legacy fdo ssh account? I have this same question, I thought we would keep the workflow with dim tool, but after I pointed drm-misc remote to gitlab, dim is not happy. If I don't point drm-misc to gitlab, dim say it is outdated (but I'm using the last top of the tree of maintainer-tools). So I was wondering if dim requires changes or if the workflow changed. The workflow has not changed, only the location of the repo. Thanks for confirming. I'm afraid there's insufficient info here to say what exactly is going on though. I re-executed dim setup and it seems to be working now. Thanks. Helen BR, Jani.
[PATCH v2 2/2] drm/nouveau/gsp: Use the sg allocator for level 2 of radix3
Currently we allocate all 3 levels of radix3 page tables using
nvkm_gsp_mem_ctor(), which uses dma_alloc_coherent() for allocating all of
the relevant memory. This can end up failing in scenarios where the system
has very high memory fragmentation, and we can't find enough contiguous
memory to allocate level 2 of the page table.
Currently, this can result in runtime PM issues on systems where memory
fragmentation is high - as we'll fail to allocate the page table for our
suspend/resume buffer:
kworker/10:2: page allocation failure: order:7, mode:0xcc0(GFP_KERNEL),
nodemask=(null),cpuset=/,mems_allowed=0
CPU: 10 PID: 479809 Comm: kworker/10:2 Not tainted
6.8.6-201.ChopperV6.fc39.x86_64 #1
Hardware name: SLIMBOOK Executive/Executive, BIOS N.1.10GRU06 02/02/2024
Workqueue: pm pm_runtime_work
Call Trace:
dump_stack_lvl+0x64/0x80
warn_alloc+0x165/0x1e0
? __alloc_pages_direct_compact+0xb3/0x2b0
__alloc_pages_slowpath.constprop.0+0xd7d/0xde0
__alloc_pages+0x32d/0x350
__dma_direct_alloc_pages.isra.0+0x16a/0x2b0
dma_direct_alloc+0x70/0x270
nvkm_gsp_radix3_sg+0x5e/0x130 [nouveau]
r535_gsp_fini+0x1d4/0x350 [nouveau]
nvkm_subdev_fini+0x67/0x150 [nouveau]
nvkm_device_fini+0x95/0x1e0 [nouveau]
nvkm_udevice_fini+0x53/0x70 [nouveau]
nvkm_object_fini+0xb9/0x240 [nouveau]
nvkm_object_fini+0x75/0x240 [nouveau]
nouveau_do_suspend+0xf5/0x280 [nouveau]
nouveau_pmops_runtime_suspend+0x3e/0xb0 [nouveau]
pci_pm_runtime_suspend+0x67/0x1e0
? __pfx_pci_pm_runtime_suspend+0x10/0x10
__rpm_callback+0x41/0x170
? __pfx_pci_pm_runtime_suspend+0x10/0x10
rpm_callback+0x5d/0x70
? __pfx_pci_pm_runtime_suspend+0x10/0x10
rpm_suspend+0x120/0x6a0
pm_runtime_work+0x98/0xb0
process_one_work+0x171/0x340
worker_thread+0x27b/0x3a0
? __pfx_worker_thread+0x10/0x10
kthread+0xe5/0x120
? __pfx_kthread+0x10/0x10
ret_from_fork+0x31/0x50
? __pfx_kthread+0x10/0x10
ret_from_fork_asm+0x1b/0x30
Luckily, we don't actually need to allocate coherent memory for the page
table thanks to being able to pass the GPU a radix3 page table for
suspend/resume data. So, let's rewrite nvkm_gsp_radix3_sg() to use the sg
allocator for level 2. We continue using coherent allocations for lvl0 and
1, since they only take a single page.
V2:
* Don't forget to actually jump to the next scatterlist when we reach the
end of the scatterlist we're currently on when writing out the page table
for level 2
Signed-off-by: Lyude Paul
Cc: sta...@vger.kernel.org
---
.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 4 +-
.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c| 77 ---
2 files changed, 54 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 6f5d376d8fcc1..a11d16a16c3b2 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -15,7 +15,9 @@ struct nvkm_gsp_mem {
};
struct nvkm_gsp_radix3 {
- struct nvkm_gsp_mem mem[3];
+ struct nvkm_gsp_mem lvl0;
+ struct nvkm_gsp_mem lvl1;
+ struct sg_table lvl2;
};
int nvkm_gsp_sg(struct nvkm_device *, u64 size, struct sg_table *);
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index 9858c1438aa7f..fd4e80ba6adfc 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -1624,7 +1624,7 @@ r535_gsp_wpr_meta_init(struct nvkm_gsp *gsp)
meta->magic = GSP_FW_WPR_META_MAGIC;
meta->revision = GSP_FW_WPR_META_REVISION;
- meta->sysmemAddrOfRadix3Elf = gsp->radix3.mem[0].addr;
+ meta->sysmemAddrOfRadix3Elf = gsp->radix3.lvl0.addr;
meta->sizeOfRadix3Elf = gsp->fb.wpr2.elf.size;
meta->sysmemAddrOfBootloader = gsp->boot.fw.addr;
@@ -1919,8 +1919,9 @@ nvkm_gsp_sg(struct nvkm_device *device, u64 size, struct
sg_table *sgt)
static void
nvkm_gsp_radix3_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_radix3 *rx3)
{
- for (int i = ARRAY_SIZE(rx3->mem) - 1; i >= 0; i--)
- nvkm_gsp_mem_dtor(gsp, >mem[i]);
+ nvkm_gsp_sg_free(gsp->subdev.device, >lvl2);
+ nvkm_gsp_mem_dtor(gsp, >lvl1);
+ nvkm_gsp_mem_dtor(gsp, >lvl0);
}
/**
@@ -1960,36 +1961,60 @@ static int
nvkm_gsp_radix3_sg(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size,
struct nvkm_gsp_radix3 *rx3)
{
- u64 addr;
+ struct sg_dma_page_iter sg_dma_iter;
+ struct scatterlist *sg;
+ size_t bufsize;
+ u64 *pte;
+ int ret, i, page_idx = 0;
- for (int i = ARRAY_SIZE(rx3->mem) - 1; i >= 0; i--) {
- u64 *ptes;
- size_t bufsize;
- int ret, idx;
+ ret = nvkm_gsp_mem_ctor(gsp, GSP_PAGE_SIZE, >lvl0);
+ if (ret)
+ return ret;
- bufsize = ALIGN((size
[PATCH v2 1/2] drm/nouveau/firmware: Fix SG_DEBUG error with nvkm_firmware_ctor()
Currently, enabling SG_DEBUG in the kernel will cause nouveau to hit a BUG() on startup: kernel BUG at include/linux/scatterlist.h:187! invalid opcode: [#1] PREEMPT SMP NOPTI CPU: 7 PID: 930 Comm: (udev-worker) Not tainted 6.9.0-rc3Lyude-Test+ #30 Hardware name: MSI MS-7A39/A320M GAMING PRO (MS-7A39), BIOS 1.I0 01/22/2019 RIP: 0010:sg_init_one+0x85/0xa0 Code: 69 88 32 01 83 e1 03 f6 c3 03 75 20 a8 01 75 1e 48 09 cb 41 89 54 24 08 49 89 1c 24 41 89 6c 24 0c 5b 5d 41 5c e9 7b b9 88 00 <0f> 0b 0f 0b 0f 0b 48 8b 05 5e 46 9a 01 eb b2 66 66 2e 0f 1f 84 00 RSP: 0018:a776017bf6a0 EFLAGS: 00010246 RAX: RBX: a77600d87000 RCX: 002b RDX: 0001 RSI: RDI: a77680d87000 RBP: e000 R08: R09: R10: 98f4c46aa508 R11: R12: 98f4c46aa508 R13: 98f4c46aa008 R14: a77600d4a000 R15: a77600d4a018 FS: 7feeb5aae980() GS:98f5c4dc() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: 7f22cb9a4520 CR3: 0001043ba000 CR4: 003506f0 Call Trace: ? die+0x36/0x90 ? do_trap+0xdd/0x100 ? sg_init_one+0x85/0xa0 ? do_error_trap+0x65/0x80 ? sg_init_one+0x85/0xa0 ? exc_invalid_op+0x50/0x70 ? sg_init_one+0x85/0xa0 ? asm_exc_invalid_op+0x1a/0x20 ? sg_init_one+0x85/0xa0 nvkm_firmware_ctor+0x14a/0x250 [nouveau] nvkm_falcon_fw_ctor+0x42/0x70 [nouveau] ga102_gsp_booter_ctor+0xb4/0x1a0 [nouveau] r535_gsp_oneinit+0xb3/0x15f0 [nouveau] ? srso_return_thunk+0x5/0x5f ? srso_return_thunk+0x5/0x5f ? nvkm_udevice_new+0x95/0x140 [nouveau] ? srso_return_thunk+0x5/0x5f ? srso_return_thunk+0x5/0x5f ? ktime_get+0x47/0xb0 ? srso_return_thunk+0x5/0x5f nvkm_subdev_oneinit_+0x4f/0x120 [nouveau] nvkm_subdev_init_+0x39/0x140 [nouveau] ? srso_return_thunk+0x5/0x5f nvkm_subdev_init+0x44/0x90 [nouveau] nvkm_device_init+0x166/0x2e0 [nouveau] nvkm_udevice_init+0x47/0x70 [nouveau] nvkm_object_init+0x41/0x1c0 [nouveau] nvkm_ioctl_new+0x16a/0x290 [nouveau] ? __pfx_nvkm_client_child_new+0x10/0x10 [nouveau] ? __pfx_nvkm_udevice_new+0x10/0x10 [nouveau] nvkm_ioctl+0x126/0x290 [nouveau] nvif_object_ctor+0x112/0x190 [nouveau] nvif_device_ctor+0x23/0x60 [nouveau] nouveau_cli_init+0x164/0x640 [nouveau] nouveau_drm_device_init+0x97/0x9e0 [nouveau] ? srso_return_thunk+0x5/0x5f ? pci_update_current_state+0x72/0xb0 ? srso_return_thunk+0x5/0x5f nouveau_drm_probe+0x12c/0x280 [nouveau] ? srso_return_thunk+0x5/0x5f local_pci_probe+0x45/0xa0 pci_device_probe+0xc7/0x270 really_probe+0xe6/0x3a0 __driver_probe_device+0x87/0x160 driver_probe_device+0x1f/0xc0 __driver_attach+0xec/0x1f0 ? __pfx___driver_attach+0x10/0x10 bus_for_each_dev+0x88/0xd0 bus_add_driver+0x116/0x220 driver_register+0x59/0x100 ? __pfx_nouveau_drm_init+0x10/0x10 [nouveau] do_one_initcall+0x5b/0x320 do_init_module+0x60/0x250 init_module_from_file+0x86/0xc0 idempotent_init_module+0x120/0x2b0 __x64_sys_finit_module+0x5e/0xb0 do_syscall_64+0x83/0x160 ? srso_return_thunk+0x5/0x5f entry_SYSCALL_64_after_hwframe+0x71/0x79 RIP: 0033:0x7feeb5cc20cd Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 1b cd 0c 00 f7 d8 64 89 01 48 RSP: 002b:7ffcf220b2c8 EFLAGS: 0246 ORIG_RAX: 0139 RAX: ffda RBX: 55fdd2916aa0 RCX: 7feeb5cc20cd RDX: RSI: 55fdd29161e0 RDI: 0035 RBP: 7ffcf220b380 R08: 7feeb5d8fb20 R09: 7ffcf220b310 R10: 55fdd2909dc0 R11: 0246 R12: 55fdd29161e0 R13: 0002 R14: 55fdd29203e0 R15: 55fdd2909d80 We hit this when trying to initialize firmware of type NVKM_FIRMWARE_IMG_DMA because we allocate our memory with dma_alloc_coherent, and DMA allocations can't be turned back into memory pages - which a scatterlist needs in order to map them. So, fix this by allocating the memory with vmalloc instead(). V2: * Fixup explanation as the prior one was bogus Signed-off-by: Lyude Paul --- drivers/gpu/drm/nouveau/nvkm/core/firmware.c | 19 --- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c index adc60b25f8e6c..141b0a513bf52 100644 --- a/drivers/gpu/drm/nouveau/nvkm/core/firmware.c +++ b/drivers/gpu/drm/nouveau/nvkm/core/firmware.c @@ -205,7 +205,9 @@ nvkm_firmware_dtor(struct nvkm_firmware *fw) break; case NVKM_FIRMWARE_IMG_DMA: nvkm_memory_unref(); - dma_free_coherent(fw->device->dev, sg_dma_len(>mem.sgl), fw->img, fw->phys); + dma_unmap_single(fw->device->dev, fw->phys,
Re: drm-misc migration to Gitlab server
On Mon, 29 Apr 2024, Helen Koike wrote: > On 01/04/2024 13:49, Tvrtko Ursulin wrote: >> No issues accessing the repo just a slight confusion and how to handle >> the workflow. More specifically, if I have a patch which wants to be >> merged to drm-misc-next, is the mailing list based worklflow still the >> way to go, or I should create a merge request, or I should apply for >> commit access via some new method other than adding permissions to my >> legacy fdo ssh account? > > I have this same question, I thought we would keep the workflow with dim > tool, but after I pointed drm-misc remote to gitlab, dim is not happy. > > If I don't point drm-misc to gitlab, dim say it is outdated (but I'm > using the last top of the tree of maintainer-tools). > > So I was wondering if dim requires changes or if the workflow changed. The workflow has not changed, only the location of the repo. I'm afraid there's insufficient info here to say what exactly is going on though. BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 2/2] drm/nouveau/gsp: Use the sg allocator for level 2 of radix3
On Mon, 2024-04-29 at 16:03 +1000, Dave Airlie wrote:
> > Currently, this can result in runtime PM issues on systems where
> > memory
> > Luckily, we don't actually need to allocate coherent memory for the
> > page
> > table thanks to being able to pass the GPU a radix3 page table for
> > suspend/resume data. So, let's rewrite nvkm_gsp_radix3_sg() to use
> > the sg
> > allocator for level 2. We continue using coherent allocations for
> > lvl0 and
> > 1, since they only take a single page.
> >
> > Signed-off-by: Lyude Paul
> > Cc: sta...@vger.kernel.org
> > ---
> > .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 4 +-
> > .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 71 ---
> >
> > 2 files changed, 47 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > index 6f5d376d8fcc1..a11d16a16c3b2 100644
> > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > @@ -15,7 +15,9 @@ struct nvkm_gsp_mem {
> > };
> >
> > struct nvkm_gsp_radix3 {
> > - struct nvkm_gsp_mem mem[3];
> > + struct nvkm_gsp_mem lvl0;
> > + struct nvkm_gsp_mem lvl1;
> > + struct sg_table lvl2;
>
> This looks great, could we go a step further and combine lvl0 and
> lvl1
> into a 2 page allocation, I thought we could combine lvl0/lvl1 into a
> 2 page alloc, but that actually might be a bad idea under memory
> pressure.
I'm not sure I understand :P, do we want to go for that or not? TBH -
I'm not sure there's any hardware reason we wouldn't be able to do the
whole radix3 table as an sg allocation with two additional memory pages
added on for level 0 and 1 - since both of those can only be the size
of a single page anyway it probably doesn't make much of a difference.
The main reason I didn't end up doing that though is because it would
make the codepath in nvkm_radix3_sg() a lot uglier. We need the virtual
addresses of level 0-2's first/only pages to populate them, and we also
need the DMA addresses of level 1-2. There isn't an iterator that lets
you go through both DMA/virtual addresses as far as I can tell - and
even if there was we'd start having to keep track of when we reach the
end of a page in the loop and make sure that we always set pte to the
address of the third sg page on the first iteration of the loop. IMO,
scatterlist could definitely benefit from having an iterator that does
both and can be stepped through both in and out of for loop macros
(like Iterator in rust).
So - it's definitely possible, but considering:
* nvkm_gsp_mem isn't a very big struct
* We're only allocating a single page for level 0 and 1, so at least
according to the advice I got from Sima this should be a safe amount
to allocate coherently under memory pressure.
* It's just a lot easier code-wise having direct address to the
DMA/virt addresses for the first two levels
I decided to stay with nvkm_gsp_mem_ctor() for the first two pages and
just use nvkm_gsp_sg() for the rest. I can definitely convert the whole
thing to using nvkm_gsp_sg() if we really want though - but I don't
think it'll give us much benefit.
I'll send out the new version of the patch without these changes and a
fix for one of the issues with this patch I already mentioned to Timur,
just let me know what you end up deciding and I can revise the patch if
you want.
>
> Dave.
>
--
Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [PATCH] drm: deprecate driver date
On Mon, 29 Apr 2024, Hamza Mahfooz wrote: > On 4/29/24 12:43, Jani Nikula wrote: >> The driver date serves no useful purpose, because it's hardly ever >> updated. The information is misleading at best. >> >> As described in Documentation/gpu/drm-internals.rst: >> >>The driver date, formatted as MMDD, is meant to identify the date >>of the latest modification to the driver. However, as most drivers >>fail to update it, its value is mostly useless. The DRM core prints it >>to the kernel log at initialization time and passes it to userspace >>through the DRM_IOCTL_VERSION ioctl. >> >> Stop printing the driver date at init, and start returning the empty >> string "" as driver date through the DRM_IOCTL_VERSION ioctl. >> >> The driver date initialization in drivers and the struct drm_driver date >> member can be removed in follow-up. >> >> Signed-off-by: Jani Nikula > > I would prefer if it was dropped entirely in this patch, but if you feel > that would require too much back and forth, I'm okay with what is > currently proposed. I can if that's what people prefer, but decided to start with this for the inevitable discussion before putting in the effort. ;) > Reviewed-by: Hamza Mahfooz Thanks, Jani. -- Jani Nikula, Intel
Re: [PATCH] drm: deprecate driver date
On 4/29/24 12:43, Jani Nikula wrote: The driver date serves no useful purpose, because it's hardly ever updated. The information is misleading at best. As described in Documentation/gpu/drm-internals.rst: The driver date, formatted as MMDD, is meant to identify the date of the latest modification to the driver. However, as most drivers fail to update it, its value is mostly useless. The DRM core prints it to the kernel log at initialization time and passes it to userspace through the DRM_IOCTL_VERSION ioctl. Stop printing the driver date at init, and start returning the empty string "" as driver date through the DRM_IOCTL_VERSION ioctl. The driver date initialization in drivers and the struct drm_driver date member can be removed in follow-up. Signed-off-by: Jani Nikula I would prefer if it was dropped entirely in this patch, but if you feel that would require too much back and forth, I'm okay with what is currently proposed. Reviewed-by: Hamza Mahfooz --- The below approximates when each driver's date was last updated. $ git grepblame "\(\.date = \".*\"\|#define.*DRIVER_DATE\)" -- drivers/gpu drivers/accel fe77368c0f3e0 drivers/accel/habanalabs/common/habanalabs_drv.c 94 (Tomer Tayar 2023-02-19 11:58:46 +0200 104) .date = "20190505", 35b137630f08d drivers/accel/ivpu/ivpu_drv.h 20 (Jacek Lawrynowicz 2023-01-17 10:27:17 +0100 24) #define DRIVER_DATE "20230117" d38ceaf99ed01 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h 43 (Alex Deucher 2015-04-20 16:55:21 -0400 43) #define DRIVER_DATE"20150101" 61f1c4a8ab757 drivers/gpu/drm/arm/display/komeda/komeda_kms.c 51 (james qian wang (Arm Technology China) 2019-01-03 11:41:30 + 64) .date = "20181101", 8e22d79240d95 drivers/gpu/drm/arm/hdlcd_drv.c 343 (Liviu Dudau 2015-04-02 19:48:39 +0100 234) .date = "20151021", ad49f8602fe88 drivers/gpu/drm/arm/malidp_drv.c 232 (Liviu Dudau 2016-03-07 10:00:53 + 571) .date = "20160106", 4f2a8f5898ecd drivers/gpu/drm/aspeed/aspeed_gfx_drv.c 208 (Joel Stanley 2019-04-03 10:49:08 +1030 253) .date = "20180319", 312fec1405dd5 drivers/gpu/drm/ast/ast_drv.h 46 (Dave Airlie 2012-02-29 13:40:04 + 46) #define DRIVER_DATE "20120228" 1a396789f65a2 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 504 (Boris Brezillon 2015-01-06 11:13:28 +0100 741) .date = "20141504", 9913f74fe1570 drivers/gpu/drm/exynos/exynos_drm_drv.c 37 (Marek Szyprowski 2018-05-10 08:46:36 +0900 37) #define DRIVER_DATE"20180330" f90cd811ae7a3 drivers/gpu/drm/gma500/psb_drv.h 43 (Arthur Borsboom 2014-03-15 22:12:17 +0100 29) #define DRIVER_DATE "20140314" 1053d01864937 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c 1070 (Xu YiPing 2019-08-20 23:06:19 + 930).date = "20150718", 76c56a5affeba drivers/gpu/drm/hyperv/hyperv_drm_drv.c 22 (Deepak Rawat 2021-05-27 04:22:28 -0700 22) #define DRIVER_DATE "2020" 3570bd989acc6 drivers/gpu/drm/i915/i915_driver.h 18 (Jani Nikula 2023-09-29 12:43:23 +0300 18) #define DRIVER_DATE "20230929" 4babef0708656 drivers/gpu/drm/imagination/pvr_drv.h 12 (Sarah Walker 2023-11-22 16:34:26 + 12) #define PVR_DRIVER_DATE "20230904" c87e859cdeb5d drivers/gpu/drm/imx/lcdc/imx-lcdc.c 361 (Marian Cichy 2023-03-06 12:52:49 +0100 353) .date = "20200716", eb92830cdbc23 drivers/gpu/drm/kmb/kmb_drv.h 19 (Edmund Dea 2020-08-26 13:17:29 -0700 19) #define DRIVER_DATE"20210223" f39db26c54281 drivers/gpu/drm/loongson/lsdc_drv.c 28 (Sui Jingfeng 2023-06-15 22:36:12 +0800 28) #define DRIVER_DATE "20220701" 5fc537bfd0003 drivers/gpu/drm/mcde/mcde_drv.c 247 (Linus Walleij 2019-05-24 11:20:19 +0200 210) .date = "20180529", 119f5173628aa drivers/gpu/drm/mediatek/mtk_drm_drv.c 36 (CK Hu 2016-01-04 18:36:34 +0100 34) #define DRIVER_DATE "20150513" 414c453106255 drivers/gpu/drm/mgag200/mgag200_drv.h 34 (Dave Airlie 2012-04-17 15:01:25 +0100 31) #define DRIVER_DATE "20110418" 77145f1cbdf8d drivers/gpu/drm/nouveau/nouveau_drm.h 9 (Ben Skeggs 2012-07-31 16:16:21 +1000 10) #define DRIVER_DATE "20120801" cd5351f4d2b1b drivers/staging/omapdrm/omap_drv.c 27 (Rob Clark 2011-11-12 12:09:40 -0600 31) #define DRIVER_DATE"20110917" 4bdca11507928 drivers/gpu/drm/panthor/panthor_drv.c 1371 (Boris Brezillon 2024-02-29 17:22:25 +0100 1386) .date = "20230801", bed41005e6174 drivers/gpu/drm/pl111/pl111_drv.c 157 (Tom Cooksey 2017-04-12 20:17:46 -0700 222) .date = "20170317", f64122c1f6ade drivers/gpu/drm/qxl/qxl_drv.h 52 (Dave Airlie 2013-02-25 14:47:55 +1000 57) #define DRIVER_DATE "20120117" c0beb2a723d69 drivers/char/drm/radeon_drv.h 41 (Dave Airlie 2008-05-28 13:52:28 +1000 46) #define DRIVER_DATE "20080528" 2048e3286f347 drivers/gpu/drm/rockchip/rockchip_drm_drv.c 34 (Mark Yao 2014-08-22 18:36:26 +0800 41) #define DRIVER_DATE"20140818" a61732e808672 drivers/gpu/drm/solomon/ssd130x.c 38 (Javier Martinez Canillas 2022-02-14 14:37:07
[PATCH] drm: drm_of.c: Using EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL
Linux kernel puts strict limits on which functions and data structures are available to loadable kernel modules; only those that have been explicitly exported with EXPORT_SYMBOL() or EXPORT_SYMBOL_GPL() are accessible. In the case of EXPORT_SYMBOL_GPL(), only modules that declare a GPL-compatible license will be able to see the symbol. Since the whole drm_of.c file is declared with GPL-2.0-only license, so let us keep functions in that source file consistently. Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/drm_of.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c index 177b600895d3..1ca36d654e61 100644 --- a/drivers/gpu/drm/drm_of.c +++ b/drivers/gpu/drm/drm_of.c @@ -44,7 +44,7 @@ uint32_t drm_of_crtc_port_mask(struct drm_device *dev, return 0; } -EXPORT_SYMBOL(drm_of_crtc_port_mask); +EXPORT_SYMBOL_GPL(drm_of_crtc_port_mask); /** * drm_of_find_possible_crtcs - find the possible CRTCs for an encoder port @@ -77,7 +77,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev, return possible_crtcs; } -EXPORT_SYMBOL(drm_of_find_possible_crtcs); +EXPORT_SYMBOL_GPL(drm_of_find_possible_crtcs); /** * drm_of_component_match_add - Add a component helper OF node match rule @@ -181,7 +181,7 @@ int drm_of_component_probe(struct device *dev, return component_master_add_with_match(dev, m_ops, match); } -EXPORT_SYMBOL(drm_of_component_probe); +EXPORT_SYMBOL_GPL(drm_of_component_probe); /* * drm_of_encoder_active_endpoint - return the active encoder endpoint -- 2.34.1
Re: nouveau: r535.c:1266:3: error: label at end of compound statement default: with gcc-8
On Mon, Apr 29, 2024, at 19:08, Timur Tabi wrote: > On Mon, 2024-04-29 at 17:30 +0200, Linux regression tracking (Thorsten > Leemhuis) wrote: >> TWIMC, there is another report about this in this thread (sadly some of >> its post did not make it to lore): >> >> https://lore.kernel.org/all/162ef3c0-1d7b-4220-a21f-b0008657f...@redhat.com/ >> >> Ciao, Thorsten > > This doesn't fail on x86-64 when I build it. I also did a cross-compile to > arm64 with the arm64 defconfig, and it doesn't fail there either. > > I'm guessing this is a compiler version thing. I'm using gcc 11.4. Is that > just too old? It's too new: this is valid syntax in c23 and accepted by newer compilers as an extension to gnu11, but older versions don't like it. gcc-11 and clang-16 are fine, while gcc-10 and clang-15 as well as earlier versions produce this warning. Arnd
[PATCH] drm/panel: ili9341: Remove a superfluous else after return
The else clause after the ruturn clause is not useful. Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c index 3574681891e8..433572c4caf9 100644 --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c @@ -722,7 +722,8 @@ static int ili9341_probe(struct spi_device *spi) if (!strcmp(id->name, "sf-tc240t-9370-t")) return ili9341_dpi_probe(spi, dc, reset); - else if (!strcmp(id->name, "yx240qv29")) + + if (!strcmp(id->name, "yx240qv29")) return ili9341_dbi_probe(spi, dc, reset); return -1; -- 2.34.1
Re: nouveau: r535.c:1266:3: error: label at end of compound statement default: with gcc-8
On Mon, 2024-04-29 at 17:30 +0200, Linux regression tracking (Thorsten Leemhuis) wrote: > TWIMC, there is another report about this in this thread (sadly some of > its post did not make it to lore): > > https://lore.kernel.org/all/162ef3c0-1d7b-4220-a21f-b0008657f...@redhat.com/ > > Ciao, Thorsten This doesn't fail on x86-64 when I build it. I also did a cross-compile to arm64 with the arm64 defconfig, and it doesn't fail there either. I'm guessing this is a compiler version thing. I'm using gcc 11.4. Is that just too old?
Re: drm-misc migration to Gitlab server
On 01/04/2024 13:49, Tvrtko Ursulin wrote: On 12/03/2024 13:56, Maxime Ripard wrote: Hi, On Tue, Feb 20, 2024 at 09:49:25AM +0100, Maxime Ripard wrote: ## Changing the default location repo Dim gets its repos list in the drm-rerere nightly.conf file. We will need to change that file to match the gitlab repo, and drop the old cgit URLs to avoid people pushing to the wrong place once the transition is made. I guess the next merge window is a good time to do so, it's usually a quiet time for us and a small disruption would be easier to handle. I'll be off-duty during that time too, so I'll have time to handle any complication. ## Updating the documentation The documentation currently mentions the old process to request a drm-misc access. It will all go through Gitlab now, so it will change a few things. We will also need to update and move the issue template to the new repo to maintain consistency. I would expect the transition (if everything goes smoothly) to occur in the merge-window time frame (11/03 -> 24/03). The transition just happened. The main drm-misc repo is now on gitlab, with the old cgit repo being setup as a mirror. If there's any issue accessing that gitlab repo, please let me know. No issues accessing the repo just a slight confusion and how to handle the workflow. More specifically, if I have a patch which wants to be merged to drm-misc-next, is the mailing list based worklflow still the way to go, or I should create a merge request, or I should apply for commit access via some new method other than adding permissions to my legacy fdo ssh account? I have this same question, I thought we would keep the workflow with dim tool, but after I pointed drm-misc remote to gitlab, dim is not happy. If I don't point drm-misc to gitlab, dim say it is outdated (but I'm using the last top of the tree of maintainer-tools). So I was wondering if dim requires changes or if the workflow changed. Thanks, Helen Regards, Tvrtko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Hi, On 2024/4/29 19:55, Maxime Ripard wrote: On Sat, Apr 27, 2024 at 01:57:46PM +0800, Sui Jingfeng wrote: Hi, On 2024/4/26 14:23, Maxime Ripard wrote: Hi, On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote: On 2024/4/26 03:10, Andy Shevchenko wrote: On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: On 2024/4/25 22:26, Andy Shevchenko wrote: It seems driver missed the point of proper use of device property APIs. Correct this by updating headers and calls respectively. You are using the 'seems' here exactly saying that you are not 100% sure. Please allow me to tell you the truth: This patch again has ZERO effect. It fix nothing. And this patch is has the risks to be wrong. Huh?! Really, stop commenting the stuff you do not understand. I'm actually a professional display drivers developer at the downstream in the past, despite my contribution to upstream is less. But I believe that all panel driver developers know what I'm talking about. So please have take a look at my replies. Most of the interactions you had in this series has been uncalled for. You might be against a patch, but there's no need to go to such length. As far as I'm concerned, this patch is fine to me in itself, and I don't see anything that would prevent us from merging it. No one is preventing you, as long as don't misunderstanding what other people's technical replies intentionally. I'm just a usual and normal contributor, I hope the world will better than yesterday. You should seriously consider your tone when replying then. Saying such thing to me may not proper, I guess you may want to talk to peoples who has the push rights I think you misunderstood me. My point was that your several rants were uncalled for and aren't the kind of things we're doing here. I know very well how to get a patch merged, thanks. just make sure it isn't a insult to the professionalism of drm bridge community itself though. I'm not sure why you're bringing the bridge community or its professionalism. It's a panel, not a bridge, and I never doubted the professionalism of anyone. I means that the code itself could be adopted, as newer and younger programmer (like Andy) need to be encouraged to contribute. I express no obvious objections, just hints him that something else probably should also be taken into consideration as well. On the other hand, we probably should allow other people participate in discussion so that it is sufficient discussed and ensure that it won't be reverted by someone in the future for some reasons. Backing to out case happens here, we may need to move things forward. Therefore, it definitely deserve to have a try. It is not a big deal even though it gets reverted someday. In the end, I don't mind if you think there is nothing that could prevent you from merge it, but I still suggest you have a glance at peoples siting at the Cc list. I'm busy now and I have a lot of other tasks to do, and may not be able to reply you emails on time. So it up to you and other maintainers to decide. Thank you. Maxime -- Best regards, Sui
Re: [PATCH v4 7/8] drm/v3d: Use gemfs/THP in BO creation if available
Hi Iago,
On 4/29/24 02:22, Iago Toral wrote:
Hi Maíra,
a question below:
El dom, 28-04-2024 a las 09:40 -0300, Maíra Canal escribió:
Although Big/Super Pages could appear naturally, it would be quite
hard
to have 1MB or 64KB allocated contiguously naturally. Therefore, we
can
force the creation of large pages allocated contiguously by using a
mountpoint with "huge=within_size" enabled.
Therefore, as V3D has a mountpoint with "huge=within_size" (if user
has
THP enabled), use this mountpoint for BO creation if available. This
will allow us to create large pages allocated contiguously and make
use
of Big/Super Pages.
Signed-off-by: Maíra Canal
Reviewed-by: Tvrtko Ursulin
---
(...)
@@ -130,10 +140,17 @@ struct v3d_bo *v3d_bo_create(struct drm_device
*dev, struct drm_file *file_priv,
size_t unaligned_size)
{
struct drm_gem_shmem_object *shmem_obj;
+ struct v3d_dev *v3d = to_v3d_dev(dev);
struct v3d_bo *bo;
int ret;
- shmem_obj = drm_gem_shmem_create(dev, unaligned_size);
+ /* Let the user opt out of allocating the BOs with THP */
+ if (v3d->gemfs)
+ shmem_obj = drm_gem_shmem_create_with_mnt(dev,
unaligned_size,
+ v3d-
gemfs);
+ else
+ shmem_obj = drm_gem_shmem_create(dev,
unaligned_size);
+
if (IS_ERR(shmem_obj))
return ERR_CAST(shmem_obj);
bo = to_v3d_bo(_obj->base);
if I read this correctly when we have THP we always allocate with that,
Even objects that are smaller than 64KB. I was wondering if there is
any benefit/downside to this or if the behavior for small allocations
is the same we had without the new mount point.
I'm assuming that your concern is related to memory pressure and memory
fragmentation.
As we are using `huge=within_size`, we only allocate a huge page if it
will be fully within `i_size`. When using `huge=within_size`, we can
optimize the performance for smaller files without forcing larger files
to also use huge pages. I don't understand `huge=within_size` in full
details, but it is possible to check that it is able to avoid the system
(even the RPi) to go OOM. Although it is slightly less performant than
`huge=always` (used in v1), I believe it is more ideal for a system such
as the RPi due to the memory requirements.
Best Regards,
- Maíra
Iago
[PATCH] drm: deprecate driver date
The driver date serves no useful purpose, because it's hardly ever updated. The information is misleading at best. As described in Documentation/gpu/drm-internals.rst: The driver date, formatted as MMDD, is meant to identify the date of the latest modification to the driver. However, as most drivers fail to update it, its value is mostly useless. The DRM core prints it to the kernel log at initialization time and passes it to userspace through the DRM_IOCTL_VERSION ioctl. Stop printing the driver date at init, and start returning the empty string "" as driver date through the DRM_IOCTL_VERSION ioctl. The driver date initialization in drivers and the struct drm_driver date member can be removed in follow-up. Signed-off-by: Jani Nikula --- The below approximates when each driver's date was last updated. $ git grepblame "\(\.date = \".*\"\|#define.*DRIVER_DATE\)" -- drivers/gpu drivers/accel fe77368c0f3e0 drivers/accel/habanalabs/common/habanalabs_drv.c 94 (Tomer Tayar 2023-02-19 11:58:46 +0200 104) .date = "20190505", 35b137630f08d drivers/accel/ivpu/ivpu_drv.h 20 (Jacek Lawrynowicz 2023-01-17 10:27:17 +0100 24) #define DRIVER_DATE "20230117" d38ceaf99ed01 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.h 43 (Alex Deucher 2015-04-20 16:55:21 -0400 43) #define DRIVER_DATE"20150101" 61f1c4a8ab757 drivers/gpu/drm/arm/display/komeda/komeda_kms.c 51 (james qian wang (Arm Technology China) 2019-01-03 11:41:30 + 64) .date = "20181101", 8e22d79240d95 drivers/gpu/drm/arm/hdlcd_drv.c 343 (Liviu Dudau 2015-04-02 19:48:39 +0100 234) .date = "20151021", ad49f8602fe88 drivers/gpu/drm/arm/malidp_drv.c 232 (Liviu Dudau 2016-03-07 10:00:53 + 571) .date = "20160106", 4f2a8f5898ecd drivers/gpu/drm/aspeed/aspeed_gfx_drv.c 208 (Joel Stanley 2019-04-03 10:49:08 +1030 253) .date = "20180319", 312fec1405dd5 drivers/gpu/drm/ast/ast_drv.h 46 (Dave Airlie 2012-02-29 13:40:04 + 46) #define DRIVER_DATE "20120228" 1a396789f65a2 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 504 (Boris Brezillon 2015-01-06 11:13:28 +0100 741) .date = "20141504", 9913f74fe1570 drivers/gpu/drm/exynos/exynos_drm_drv.c 37 (Marek Szyprowski 2018-05-10 08:46:36 +0900 37) #define DRIVER_DATE"20180330" f90cd811ae7a3 drivers/gpu/drm/gma500/psb_drv.h 43 (Arthur Borsboom 2014-03-15 22:12:17 +0100 29) #define DRIVER_DATE "20140314" 1053d01864937 drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c 1070 (Xu YiPing 2019-08-20 23:06:19 + 930).date = "20150718", 76c56a5affeba drivers/gpu/drm/hyperv/hyperv_drm_drv.c 22 (Deepak Rawat 2021-05-27 04:22:28 -0700 22) #define DRIVER_DATE "2020" 3570bd989acc6 drivers/gpu/drm/i915/i915_driver.h 18 (Jani Nikula 2023-09-29 12:43:23 +0300 18) #define DRIVER_DATE "20230929" 4babef0708656 drivers/gpu/drm/imagination/pvr_drv.h 12 (Sarah Walker 2023-11-22 16:34:26 + 12) #define PVR_DRIVER_DATE "20230904" c87e859cdeb5d drivers/gpu/drm/imx/lcdc/imx-lcdc.c 361 (Marian Cichy 2023-03-06 12:52:49 +0100 353) .date = "20200716", eb92830cdbc23 drivers/gpu/drm/kmb/kmb_drv.h 19 (Edmund Dea 2020-08-26 13:17:29 -0700 19) #define DRIVER_DATE"20210223" f39db26c54281 drivers/gpu/drm/loongson/lsdc_drv.c 28 (Sui Jingfeng 2023-06-15 22:36:12 +0800 28) #define DRIVER_DATE "20220701" 5fc537bfd0003 drivers/gpu/drm/mcde/mcde_drv.c 247 (Linus Walleij 2019-05-24 11:20:19 +0200 210) .date = "20180529", 119f5173628aa drivers/gpu/drm/mediatek/mtk_drm_drv.c 36 (CK Hu 2016-01-04 18:36:34 +0100 34) #define DRIVER_DATE "20150513" 414c453106255 drivers/gpu/drm/mgag200/mgag200_drv.h 34 (Dave Airlie 2012-04-17 15:01:25 +0100 31) #define DRIVER_DATE "20110418" 77145f1cbdf8d drivers/gpu/drm/nouveau/nouveau_drm.h 9 (Ben Skeggs 2012-07-31 16:16:21 +1000 10) #define DRIVER_DATE "20120801" cd5351f4d2b1b drivers/staging/omapdrm/omap_drv.c 27 (Rob Clark 2011-11-12 12:09:40 -0600 31) #define DRIVER_DATE"20110917" 4bdca11507928 drivers/gpu/drm/panthor/panthor_drv.c 1371 (Boris Brezillon 2024-02-29 17:22:25 +0100 1386) .date = "20230801", bed41005e6174 drivers/gpu/drm/pl111/pl111_drv.c 157 (Tom Cooksey 2017-04-12 20:17:46 -0700 222) .date = "20170317", f64122c1f6ade drivers/gpu/drm/qxl/qxl_drv.h 52 (Dave Airlie 2013-02-25 14:47:55 +1000 57) #define DRIVER_DATE "20120117" c0beb2a723d69 drivers/char/drm/radeon_drv.h 41 (Dave Airlie 2008-05-28 13:52:28 +1000 46) #define DRIVER_DATE "20080528" 2048e3286f347 drivers/gpu/drm/rockchip/rockchip_drm_drv.c 34 (Mark Yao 2014-08-22 18:36:26 +0800 41) #define DRIVER_DATE"20140818" a61732e808672 drivers/gpu/drm/solomon/ssd130x.c 38 (Javier Martinez Canillas 2022-02-14 14:37:07 +0100 41) #define DRIVER_DATE "20220131" 43531edd53f07 drivers/gpu/drm/sprd/sprd_drm.c 26 (Kevin Tang 2021-12-07 22:27:13 +0800 26) #define DRIVER_DATE "20200201" 9bbf86fe874cc drivers/gpu/drm/sti/sti_drm_drv.c 24 (Benjamin Gaignard
Re: [PATCH] drm/amd/display: Add MSF panel to DPCD 0x317 patch list
Applied. Thanks!
On Fri, Mar 8, 2024 at 8:58 PM wrote:
>
> From: Tobias Jakobi
>
> This 8.4 inch panel is integrated in the Ayaneo Kun handheld
> device. The panel resolution is 2560×1600, i.e. it has
> portrait dimensions.
>
> Decoding the EDID shows:
> Manufacturer: MSF
> Model: 4099
> Display Product Name: 'TV080WUM-NL0 '
>
> Judging from the product name this might be a clone of a
> BOE panel, but with larger dimensions.
>
> Panel frequently shows non-functional backlight control. Adding
> some debug prints to update_connector_ext_caps() shows that
> something the OLED bit of ext_caps is set, and then the driver
> assumes that backlight is controlled via AUX.
>
> Forcing backlight control to PWM via amdgpu.backlight=0 restores
> backlight operation.
>
> Signed-off-by: Tobias Jakobi
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> index 7a09a72e182f..5a017ba94e3c 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c
> @@ -68,6 +68,7 @@ static void apply_edid_quirks(struct edid *edid, struct
> dc_edid_caps *edid_caps)
> case drm_edid_encode_panel_id('A', 'U', 'O', 0xE69B):
> case drm_edid_encode_panel_id('B', 'O', 'E', 0x092A):
> case drm_edid_encode_panel_id('L', 'G', 'D', 0x06D1):
> + case drm_edid_encode_panel_id('M', 'S', 'F', 0x1003):
> DRM_DEBUG_DRIVER("Clearing DPCD 0x317 on monitor with panel
> id %X\n", panel_id);
> edid_caps->panel_patch.remove_sink_ext_caps = true;
> break;
> --
> 2.43.0
>
Re: [PATCH] drm/amd/display: Remove duplicate dcn401/dcn401_clk_mgr.h header
Applied. Thanks! On Wed, Apr 24, 2024 at 11:42 PM Jiapeng Chong wrote: > > ./drivers/gpu/drm/amd/display/dc/clk_mgr/dcn401/dcn401_clk_mgr.c: > dcn401/dcn401_clk_mgr.h is included more than once. > > Reported-by: Abaci Robot > Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8885 > Signed-off-by: Jiapeng Chong > --- > drivers/gpu/drm/amd/display/dc/clk_mgr/dcn401/dcn401_clk_mgr.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn401/dcn401_clk_mgr.c > b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn401/dcn401_clk_mgr.c > index d146c35f6d60..005092b0a0cb 100644 > --- a/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn401/dcn401_clk_mgr.c > +++ b/drivers/gpu/drm/amd/display/dc/clk_mgr/dcn401/dcn401_clk_mgr.c > @@ -21,7 +21,6 @@ > #include "dcn/dcn_4_1_0_offset.h" > #include "dcn/dcn_4_1_0_sh_mask.h" > > -#include "dcn401/dcn401_clk_mgr.h" > #include "dml/dcn401/dcn401_fpu.h" > > #define mmCLK01_CLK0_CLK_PLL_REQ0x16E37 > -- > 2.20.1.7.g153144c >
Re: [PATCH] drm/amd/display: Remove duplicate spl/dc_spl_types.h header
Applied. Thanks! On Wed, Apr 24, 2024 at 9:52 PM Jiapeng Chong wrote: > > ./drivers/gpu/drm/amd/display/dc/inc/hw/transform.h: spl/dc_spl_types.h is > included more than once. > > Reported-by: Abaci Robot > Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8884 > Signed-off-by: Jiapeng Chong > --- > drivers/gpu/drm/amd/display/dc/inc/hw/transform.h | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/inc/hw/transform.h > b/drivers/gpu/drm/amd/display/dc/inc/hw/transform.h > index 5aa2f1a1fb83..28da1dddf0a0 100644 > --- a/drivers/gpu/drm/amd/display/dc/inc/hw/transform.h > +++ b/drivers/gpu/drm/amd/display/dc/inc/hw/transform.h > @@ -31,8 +31,6 @@ > #include "fixed31_32.h" > #include "spl/dc_spl_types.h" > > -#include "spl/dc_spl_types.h" > - > #define CSC_TEMPERATURE_MATRIX_SIZE 12 > > struct bit_depth_reduction_params; > -- > 2.19.1.6.gb485710b >
Re: [PATCH] drm/amdgpu: Fix signedness bug in sdma_v4_0_process_trap_irq()
Applied. Thanks!
On Sun, Apr 28, 2024 at 9:32 PM Zhou, Bob wrote:
>
> [Public]
>
> Reviewed-by: Bob Zhou
>
> Regards,
> Bob
>
> -Original Message-
> From: Dan Carpenter
> Sent: 2024年4月28日 20:57
> To: Zhou, Bob
> Cc: Deucher, Alexander ; Koenig, Christian
> ; Pan, Xinhui ; David Airlie
> ; Daniel Vetter ; Kuehling, Felix
> ; Zhang, Hawking ; Guchun Chen
> ; Ma, Le ; Lazar, Lijo
> ; Sharma, Shashank ;
> amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> linux-ker...@vger.kernel.org; kernel-janit...@vger.kernel.org
> Subject: [PATCH] drm/amdgpu: Fix signedness bug in
> sdma_v4_0_process_trap_irq()
>
> The "instance" variable needs to be signed for the error handling to work.
>
> Fixes: b34ddc71267a ("drm/amdgpu: add error handle to avoid out-of-bounds")
> Signed-off-by: Dan Carpenter
> ---
> drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 101038395c3b..772604feb6ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -2017,7 +2017,7 @@ static int sdma_v4_0_process_trap_irq(struct
> amdgpu_device *adev,
> struct amdgpu_irq_src *source,
> struct amdgpu_iv_entry *entry) {
> - uint32_t instance;
> + int instance;
>
> DRM_DEBUG("IH: SDMA trap\n");
> instance = sdma_v4_0_irq_id_to_seq(entry->client_id);
> --
> 2.43.0
>
Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
On Mon, 29 Apr 2024, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 29, 2024 at 2:38 AM Neil Armstrong
> wrote:
>>
>> > +/**
>> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI
>> > funcs in a row
>> > + * @dsi: Pointer to the MIPI DSI device
>> > + * @accum_err: Storage for the accumulated error over the multiple calls.
>> > Init
>> > + * to 0. If a function encounters an error then the error code will be
>> > + * stored here. If you call a function and this points to a non-zero
>> > + * value then the function will be a noop. This allows calling a
>> > function
>> > + * many times in a row and just checking the error at the end to see if
>> > + * any of them failed.
>> > + */
>> > +
>> > +struct mipi_dsi_multi_context {
>> > + struct mipi_dsi_device *dsi;
>> > + int accum_err;
>> > +};
>>
>> I like the design, but having a context struct seems over-engineered while
>> we could pass
>> a single int over without encapsulating it with mipi_dsi_multi_context.
>>
>> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
>> const void *data, size_t len);
>> vs
>> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int
>> *accum_err,
>> const void *data, size_t len);
>>
>> is the same, and it avoids having to declare a mipi_dsi_multi_context and
>> set ctx->dsi,
>> and I'll find it much easier to migrate, just add a and make sure ret
>> is initialized to 0.
>
> Yeah, I had the same reaction when Jani suggested the context style
> [1] and I actually coded it up exactly as you suggest above. I then
> changed my mind and went with the context. My motivation was that when
> I tested it I found that using the context produced smaller code.
> Specifically, from the description of this patch we see we end up
> with:
>
> Total: Before=10651, After=9663, chg -9.28%
>
> ...when I didn't have the context and I had the accum_err then instead
> of getting ~9% smaller I believe it actually got ~0.5% bigger. This
> makes some sense as the caller has to pass 4 parameters to each call
> instead of 3.
>
> It's not a giant size difference but it was at least some motivation
> that helped push me in this direction. I'd also say that when I looked
> at the code in the end the style grew on me. It's really not too
> terrible to have one line in your functions that looks like:
>
> struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };
>
> ...and if that becomes the canonical way to do it then it's really
> hard to accidentally forget to initialize the error value. With the
> other API it's _very_ easy to forget to initialize the error value and
> the compiler won't yell at you. It also makes it very obvious to the
> caller that this function is doing something a little different than
> most Linux APIs with that error return.
>
> So I guess I'd say that I ended up being pretty happy with the
> "context" even if it does feel a little over engineered and I'd argue
> to keep it that way. That being said, if you feel strongly about it
> then we can perhaps get others to chime in to see which style they
> prefer? Let me know what you think.
>
>
> [1] https://lore.kernel.org/r/8734r85tcf@intel.com
FWIW, I don't feel strongly about this, and I could be persuaded either
way, but I've got this gut feeling that an extensible context parameter
might be benefitial future proofing in this case.
BR,
Jani.
--
Jani Nikula, Intel
Re: [PATCH 1/2] drm: print top commit sha after loading the driver
On Mon, Apr 29, 2024 at 02:02:28PM GMT, Jani Nikula wrote:
On Wed, 24 Apr 2024, Farah Kassabri wrote:
Add the last driver sha to the existing log message
which prints the drm device info.
The commit message fails to answer the most important question: why?
Especially, what makes drm devices such special snowflakes that they'd
need to be the only ones in the kernel to print git commit sha1's?
the closest to what was added here would be srcversion:
$ modinfo build64/drivers/gpu/drm/xe/xe.ko | grep srcversion
srcversion: 0EA08A43AC399A0D942740
which makes more sense and doesn't depend on the git tree checkout and
other possible problems with dirty checkouts. If you want to print it
on module load to be able to tell from a log, you could probably just
access mod->srcversion.
but I'm not sure what we are trying to accomplish here and the commit
message certainly missed that. Please Cc dri-devel on changes outside xe
and provide the motivation in the commit message.
thanks
Lucas De Marchi
BR,
Jani.
Signed-off-by: Farah Kassabri
---
drivers/gpu/drm/drm_drv.c | 6 +++---
include/drm/drm_drv.h | 2 ++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 535b624d4c9d..e0f7af1b6ec3 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -947,10 +947,10 @@ int drm_dev_register(struct drm_device *dev, unsigned
long flags)
}
drm_panic_register(dev);
- DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
+ DRM_INFO("Initialized %s %d.%d.%d%s %s for %s on minor %d\n",
driver->name, driver->major, driver->minor,
-driver->patchlevel, driver->date,
-dev->dev ? dev_name(dev->dev) : "virtual device",
+driver->patchlevel, driver->git_sha ? driver->git_sha : "",
+driver->date, dev->dev ? dev_name(dev->dev) : "virtual device",
dev->primary ? dev->primary->index : dev->accel->index);
goto out_unlock;
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 8878260d7529..7578a1f4ce74 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -407,6 +407,8 @@ struct drm_driver {
int minor;
/** @patchlevel: driver patch level */
int patchlevel;
+ /** @git_sha: driver last commit sha */
+ char *git_sha;
/** @name: driver name */
char *name;
/** @desc: driver description */
--
Jani Nikula, Intel
Re: nouveau: r535.c:1266:3: error: label at end of compound statement default: with gcc-8
On 29.04.24 17:06, Naresh Kamboju wrote:
> Following build warnings / errors noticed on Linux next-20240429 tag on the
> arm64, arm and riscv with gcc-8 and gcc-13 builds pass.
>
> Reported-by: Linux Kernel Functional Testing
>
> Commit id:
> b58a0bc904ff nouveau: add command-line GSP-RM registry support
>
> Buids:
> --
> gcc-8-arm64-defconfig - Fail
> gcc-8-arm-defconfig - Fail
> gcc-8-riscv-defconfig - Fail
>
> Build log:
>
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c: In function 'build_registry':
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1266:3: error: label at
> end of compound statement
>default:
>^~~
> make[7]: *** [scripts/Makefile.build:244:
> drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.o] Error 1
TWIMC, there is another report about this in this thread (sadly some of
its post did not make it to lore):
https://lore.kernel.org/all/162ef3c0-1d7b-4220-a21f-b0008657f...@redhat.com/
Ciao, Thorsten
> metadata:
> git_describe: next-20240429
> git_repo: https://gitlab.com/Linaro/lkft/mirrors/next/linux-next
> git_short_log: b0a2c79c6f35 ("Add linux-next specific files for 20240429")
> arch: arm64, arm, riscv
> toolchain: gcc-8
>
> Steps to reproduce:
>
> # tuxmake --runtime podman --target-arch arm64 --toolchain gcc-8
> --kconfig defconfig
>
> Links:
> -
> https://storage.tuxsuite.com/public/linaro/lkft/builds/2flcoOuqVJfhTvX4AOYsWMd5hqe/
> -
> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20240429/testrun/23704376/suite/build/test/gcc-8-defconfig/history/
> -
> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20240429/testrun/23705756/suite/build/test/gcc-8-defconfig/details/
>
>
> --
> Linaro LKFT
> https://lkft.linaro.org
>
>
nouveau: r535.c:1266:3: error: label at end of compound statement default: with gcc-8
Following build warnings / errors noticed on Linux next-20240429 tag on the
arm64, arm and riscv with gcc-8 and gcc-13 builds pass.
Reported-by: Linux Kernel Functional Testing
Commit id:
b58a0bc904ff nouveau: add command-line GSP-RM registry support
Buids:
--
gcc-8-arm64-defconfig - Fail
gcc-8-arm-defconfig - Fail
gcc-8-riscv-defconfig - Fail
Build log:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c: In function 'build_registry':
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1266:3: error: label at
end of compound statement
default:
^~~
make[7]: *** [scripts/Makefile.build:244:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.o] Error 1
metadata:
git_describe: next-20240429
git_repo: https://gitlab.com/Linaro/lkft/mirrors/next/linux-next
git_short_log: b0a2c79c6f35 ("Add linux-next specific files for 20240429")
arch: arm64, arm, riscv
toolchain: gcc-8
Steps to reproduce:
# tuxmake --runtime podman --target-arch arm64 --toolchain gcc-8
--kconfig defconfig
Links:
-
https://storage.tuxsuite.com/public/linaro/lkft/builds/2flcoOuqVJfhTvX4AOYsWMd5hqe/
-
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20240429/testrun/23704376/suite/build/test/gcc-8-defconfig/history/
-
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20240429/testrun/23705756/suite/build/test/gcc-8-defconfig/details/
--
Linaro LKFT
https://lkft.linaro.org
Re: [PATCH v4 05/16] SoC: mediatek: mt8365: support audio clock control
On Fri, Apr 26, 2024 at 07:22:34PM +0200, Alexandre Mergnat wrote: > Add audio clock wrapper and audio tuner control. Please submit patches using subject lines reflecting the style for the subsystem, this makes it easier for people to identify relevant patches. Look at what existing commits in the area you're changing are doing and make sure your subject lines visually resemble what they're doing. There's no need to resubmit to fix this alone. signature.asc Description: PGP signature
Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
Hi,
On Mon, Apr 29, 2024 at 2:38 AM Neil Armstrong
wrote:
>
> > +/**
> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs
> > in a row
> > + * @dsi: Pointer to the MIPI DSI device
> > + * @accum_err: Storage for the accumulated error over the multiple calls.
> > Init
> > + * to 0. If a function encounters an error then the error code will be
> > + * stored here. If you call a function and this points to a non-zero
> > + * value then the function will be a noop. This allows calling a function
> > + * many times in a row and just checking the error at the end to see if
> > + * any of them failed.
> > + */
> > +
> > +struct mipi_dsi_multi_context {
> > + struct mipi_dsi_device *dsi;
> > + int accum_err;
> > +};
>
> I like the design, but having a context struct seems over-engineered while we
> could pass
> a single int over without encapsulating it with mipi_dsi_multi_context.
>
> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
> const void *data, size_t len);
> vs
> void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int
> *accum_err,
> const void *data, size_t len);
>
> is the same, and it avoids having to declare a mipi_dsi_multi_context and set
> ctx->dsi,
> and I'll find it much easier to migrate, just add a and make sure ret is
> initialized to 0.
Yeah, I had the same reaction when Jani suggested the context style
[1] and I actually coded it up exactly as you suggest above. I then
changed my mind and went with the context. My motivation was that when
I tested it I found that using the context produced smaller code.
Specifically, from the description of this patch we see we end up
with:
Total: Before=10651, After=9663, chg -9.28%
...when I didn't have the context and I had the accum_err then instead
of getting ~9% smaller I believe it actually got ~0.5% bigger. This
makes some sense as the caller has to pass 4 parameters to each call
instead of 3.
It's not a giant size difference but it was at least some motivation
that helped push me in this direction. I'd also say that when I looked
at the code in the end the style grew on me. It's really not too
terrible to have one line in your functions that looks like:
struct mipi_dsi_multi_context ctx = { .dsi = boe->dsi };
...and if that becomes the canonical way to do it then it's really
hard to accidentally forget to initialize the error value. With the
other API it's _very_ easy to forget to initialize the error value and
the compiler won't yell at you. It also makes it very obvious to the
caller that this function is doing something a little different than
most Linux APIs with that error return.
So I guess I'd say that I ended up being pretty happy with the
"context" even if it does feel a little over engineered and I'd argue
to keep it that way. That being said, if you feel strongly about it
then we can perhaps get others to chime in to see which style they
prefer? Let me know what you think.
[1] https://lore.kernel.org/r/8734r85tcf@intel.com
Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
Hi,
On Fri, Apr 26, 2024 at 11:33 PM Sam Ravnborg wrote:
>
> > ---
> > Right now this patch introduces two new functions in
> > drm_mipi_dsi.c. Alternatively we could have changed the prototype of
> > the "chatty" functions and made the deprecated macros adapt to the new
> > prototype. While this sounds nice, it bloated callers of the
> > deprecated functioin a bit because it caused the compiler to emit less
> > optimal code. It doesn't seem terrible to add two more functions, so I
> > went that way.
> The concern here is when it will be cleaned up. As a minimum please
> consider adding an item to todo.rst that details what should be done
> to get rid of the now obsolete chatty functions so someone can pick it
> up.
Sure, I can add something to do TODO. Do folks agree that's the
preferred way to do things? A few thoughts I had:
1. In theory it could be useful to keep _both_ the "chatty" and
"multi" variants of the functions. In at least a few places the
"multi" variant was awkward. The resulting auo_kd101n80_45na_init()
[1] looked awkward. If I was writing just this function I would have
chosen an API more like the "chatty" one, but since I was trying to
make all the init functions similar I kept them all using the "multi"
API. Does it make sense to keep both long term?
[1]
https://lore.kernel.org/all/20240426165839.v2.7.Ib5030ab5cd41b4e08b1958bd7e51571725723008@changeid/
2. Going completely the opposite direction, we could also not bother
saving as much space today and _force_ everyone to move to the new
"multi" API to get the full space savings.
So I guess three options: a) leave my patches the way they are and add
a TODO, b) Keep the "chatty" variants long term and remove my
"after-the-cut", or c) Don't get as much space savings today but don't
introduce a new exported function. Opinions?
> > @@ -792,6 +792,34 @@ int mipi_dsi_generic_write_chatty(struct
> > mipi_dsi_device *dsi,
> > }
> > EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
> >
> > +/**
> > + * mipi_dsi_generic_write_multi() - mipi_dsi_generic_write_chatty() w/
> > accum_err
> > + * @ctx: Context for multiple DSI transactions
> > + * @payload: buffer containing the payload
> > + * @size: size of payload buffer
> > + *
> > + * Like mipi_dsi_generic_write_chatty() but deals with errors in a way that
> > + * makes it convenient to make several calls in a row.
> > + */
> > +void mipi_dsi_generic_write_multi(struct mipi_dsi_multi_context *ctx,
> > + const void *payload, size_t size)
> > +{
> > + struct mipi_dsi_device *dsi = ctx->dsi;
> > + struct device *dev = >dev;
> > + ssize_t ret;
> > +
> > + if (ctx->accum_err)
> > + return;
> > +
> > + ret = mipi_dsi_generic_write(dsi, payload, size);
> > + if (ret < 0) {
> > + ctx->accum_err = ret;
> > + dev_err_ratelimited(dev, "sending generic data %*ph failed:
> > %d\n",
> > + (int)size, payload, ctx->accum_err);
> > + }
> I see no point in using ratelimited and then change it in the next
> patch.
Sure, I'll re-order patches.
> > @@ -197,6 +197,22 @@ struct mipi_dsi_device {
> > struct drm_dsc_config *dsc;
> > };
> >
> > +/**
> > + * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs
> > in a row
> > + * @dsi: Pointer to the MIPI DSI device
> > + * @accum_err: Storage for the accumulated error over the multiple calls.
> > Init
> > + * to 0. If a function encounters an error then the error code will be
> > + * stored here. If you call a function and this points to a non-zero
> > + * value then the function will be a noop. This allows calling a function
> > + * many times in a row and just checking the error at the end to see if
> > + * any of them failed.
> > + */
> > +
> > +struct mipi_dsi_multi_context {
> > + struct mipi_dsi_device *dsi;
> > + int accum_err;
> > +};
> Inline comments is - I think - preferred.
Sure, I'll update in the next version.
RE: [PATCH] drm/i915/gt: Automate CCS Mode setting during engine resets
Tested-by: Krzysztof Gibala
-Original Message-
From: Andi Shyti
Sent: Friday, April 26, 2024 2:07 AM
To: intel-gfx ; dri-devel
Cc: Andi Shyti ; Andi Shyti
; Gnattu OC ; Chris Wilson
; Joonas Lahtinen
; Roper, Matthew D
; sta...@vger.kernel.org
Subject: [PATCH] drm/i915/gt: Automate CCS Mode setting during engine resets
We missed setting the CCS mode during resume and engine resets.
Create a workaround to be added in the engine's workaround list.
This workaround sets the XEHP_CCS_MODE value at every reset.
The issue can be reproduced by running:
$ clpeak --kernel-latency
Without resetting the CCS mode, we encounter a fence timeout:
Fence expiration time out i915-:03:00.0:clpeak[2387]:2!
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10895
Fixes: 6db31251bb26 ("drm/i915/gt: Enable only one CCS for compute workload")
Reported-by: Gnattu OC
Signed-off-by: Andi Shyti
Cc: Chris Wilson
Cc: Joonas Lahtinen
Cc: Matt Roper
Cc: # v6.2+
---
Hi Gnattu,
thanks again for reporting this issue and for your prompt replies on the issue.
Would you give this patch a chance?
Andi
drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c | 6 +++---
drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h | 2 +-
drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 +++-
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
index 044219c5960a..99b71bb7da0a 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
@@ -8,14 +8,14 @@
#include "intel_gt_ccs_mode.h"
#include "intel_gt_regs.h"
-void intel_gt_apply_ccs_mode(struct intel_gt *gt)
+unsigned int intel_gt_apply_ccs_mode(struct intel_gt *gt)
{
int cslice;
u32 mode = 0;
int first_ccs = __ffs(CCS_MASK(gt));
if (!IS_DG2(gt->i915))
- return;
+ return 0;
/* Build the value for the fixed CCS load balancing */
for (cslice = 0; cslice < I915_MAX_CCS; cslice++) { @@ -35,5 +35,5 @@
void intel_gt_apply_ccs_mode(struct intel_gt *gt)
XEHP_CCS_MODE_CSLICE_MASK);
}
- intel_uncore_write(gt->uncore, XEHP_CCS_MODE, mode);
+ return mode;
}
diff --git a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
index 9e5549caeb26..55547f2ff426 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
@@ -8,6 +8,6 @@
struct intel_gt;
-void intel_gt_apply_ccs_mode(struct intel_gt *gt);
+unsigned int intel_gt_apply_ccs_mode(struct intel_gt *gt);
#endif /* __INTEL_GT_CCS_MODE_H__ */
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 68b6aa11bcf7..58693923bf6c 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -2703,6 +2703,7 @@ add_render_compute_tuning_settings(struct intel_gt *gt,
static void ccs_engine_wa_mode(struct intel_engine_cs *engine, struct
i915_wa_list *wal) {
struct intel_gt *gt = engine->gt;
+ u32 mode;
if (!IS_DG2(gt->i915))
return;
@@ -2719,7 +2720,8 @@ static void ccs_engine_wa_mode(struct intel_engine_cs
*engine, struct i915_wa_li
* After having disabled automatic load balancing we need to
* assign all slices to a single CCS. We will call it CCS mode 1
*/
- intel_gt_apply_ccs_mode(gt);
+ mode = intel_gt_apply_ccs_mode(gt);
+ wa_masked_en(wal, XEHP_CCS_MODE, mode);
}
/*
--
2.43.0
-
Intel Technology Poland sp. z o.o.
ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial
Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 |
Kapital zakladowy 200.000 PLN.
Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu ustawy z
dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach
handlowych.
Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i
moze zawierac informacje poufne. W razie przypadkowego otrzymania tej
wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie;
jakiekolwiek przegladanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). If you are not the intended recipient, please
contact the sender and delete all copies; any review or distribution by others
is strictly prohibited.
Re: [PATCH v3] drm/i915/vma: Fix UAF on reopen vs destroy race
On Thu, 2024-04-25 at 20:42 +0200, Janusz Krzysztofik wrote:
> Hi Thomas,
>
> On Tuesday, 16 April 2024 18:40:12 CEST Rodrigo Vivi wrote:
> > On Tue, Apr 16, 2024 at 10:09:46AM +0200, Janusz Krzysztofik wrote:
> > > Hi Rodrigo,
> > >
> > > On Tuesday, 16 April 2024 03:16:31 CEST Rodrigo Vivi wrote:
> > > > On Mon, Apr 15, 2024 at 09:53:09PM +0200, Janusz Krzysztofik
> > > > wrote:
> > > > > We defer actually closing, unbinding and destroying a VMA
> > > > > until next idle
> > > > > point, or until the object is freed in the meantime. By
> > > > > postponing the
> > > > > unbind, we allow for the VMA to be reopened by the client,
> > > > > avoiding the
> > > > > work required to rebind the VMA.
> > > > >
> > > > > It was assumed that as long as a GT is held idle, no VMA
> > > > > would be reopened
> > > > > while we destroy them. That assumption is no longer true in
> > > > > multi-GT
> > > > > configurations, where a VMA we reopen may be handled by a GT
> > > > > different
> > > > > from the one that we already keep active via its engine while
> > > > > we set up
> > > > > an execbuf request.
> > > > >
> > > > > <4> [260.290809] [ cut here ]
> > > > > <4> [260.290988] list_del corruption. prev->next should be
> > > > > 888118c5d990, but was 888118c5a510.
> > > > > (prev=888118c5a510)
> > > > > <4> [260.291004] WARNING: CPU: 2 PID: 1143 at
> > > > > lib/list_debug.c:62
> > > > > __list_del_entry_valid_or_report+0xb7/0xe0
> > > > > ..
> > > > > <4> [260.291055] CPU: 2 PID: 1143 Comm: kms_plane Not tainted
> > > > > 6.9.0-rc2-CI_DRM_14524-ga25d180c6853+ #1
> > > > > <4> [260.291058] Hardware name: Intel Corporation Meteor Lake
> > > > > Client Platform/MTL-P LP5x T3 RVP, BIOS
> > > > > MTLPFWI1.R00.3471.D91.2401310918 01/31/2024
> > > > > <4> [260.291060] RIP:
> > > > > 0010:__list_del_entry_valid_or_report+0xb7/0xe0
> > > > > ...
> > > > > <4> [260.291087] Call Trace:
> > > > > <4> [260.291089]
> > > > > <4> [260.291124] i915_vma_reopen+0x43/0x80 [i915]
> > > > > <4> [260.291298] eb_lookup_vmas+0x9cb/0xcc0 [i915]
> > > > > <4> [260.291579] i915_gem_do_execbuffer+0xc9a/0x26d0 [i915]
> > > > > <4> [260.291883] i915_gem_execbuffer2_ioctl+0x123/0x2a0
> > > > > [i915]
> > > > > ...
> > > > > <4> [260.292301]
> > > > > ...
> > > > > <4> [260.292506] ---[ end trace ]---
> > > > > <4> [260.292782] general protection fault, probably for non-
> > > > > canonical address 0x6b6b6b6b6b6b6ca3: [#1] PREEMPT SMP
> > > > > NOPTI
> > > > > <4> [260.303575] CPU: 2 PID: 1143 Comm: kms_plane Tainted:
> > > > > G W 6.9.0-rc2-CI_DRM_14524-ga25d180c6853+ #1
> > > > > <4> [260.313851] Hardware name: Intel Corporation Meteor Lake
> > > > > Client Platform/MTL-P LP5x T3 RVP, BIOS
> > > > > MTLPFWI1.R00.3471.D91.2401310918 01/31/2024
> > > > > <4> [260.326359] RIP: 0010:eb_validate_vmas+0x114/0xd80
> > > > > [i915]
> > > > > ...
> > > > > <4> [260.428756] Call Trace:
> > > > > <4> [260.431192]
> > > > > <4> [639.283393] i915_gem_do_execbuffer+0xd05/0x26d0 [i915]
> > > > > <4> [639.305245] i915_gem_execbuffer2_ioctl+0x123/0x2a0
> > > > > [i915]
> > > > > ...
> > > > > <4> [639.411134]
> > > > > ...
> > > > > <4> [639.449979] ---[ end trace ]---
> > > > >
> > > > > As soon as we start unbinding and destroying a VMA, marked it
> > > > > as parked,
> > > > > and also keep it marked as closed for the rest of its life.
> > > > > When a VMA
> > > > > to be opened occurs closed, reopen it only if not yet parked.
> > > > >
> > > > > v3: Fix misplaced brackets.
> > > > > v2: Since we no longer re-init the VMA closed list link on
> > > > > VMA park so it
> > > > > looks like still on a list, don't try to delete it from
> > > > > the list again
> > > > > after the VMA has been marked as parked.
> > > > >
> > > > > Fixes: b0647a5e79b1 ("drm/i915: Avoid live-lock with
> > > > > i915_vma_parked()")
> > > >
> > > > what about reverting that?
> > >
> > > I didn't think of that. Why you think that might be a better
> > > approach?
> >
> > well, I thought of that mainly because...
> >
> > >
> > > Anyway, that's a 4 years old patch and a few things have changed
> > > since then,
> > > so simple revert won't work. Moreover, I've just checked that
> > > patch was
> > > supposed to fix another patch, 77853186e547 ("drm/i915: Claim vma
> > > while under
> > > closed_lock in i915_vma_parked()"), which in turn was supposed to
> > > fix
> > > aa5e4453dc05 ("drm/i915/gem: Try to flush pending unbind
> > > events"), and that
> > > one also referenced still another, cb6c3d45f948 ("drm/i915/gem:
> > > Avoid parking
> > > the vma as we unbind") from December 2019, which finally wasn't a
> > > fix but an
> > > improvement.
> >
> > ... because of histories like that ^ and I was afraid of this patch
> > here now
> > just put us into a different corner case.
> >
> > I have a feeling that without locks there we might just hit another
Re: [PATCH] MAINTAINERS: Move the drm-intel repo location to fd.o GitLab
On Fri, Apr 26, 2024 at 05:02:54PM +0100, Tvrtko Ursulin wrote: > > > On 26/04/2024 16:47, Lucas De Marchi wrote: > > On Wed, Apr 24, 2024 at 01:41:59PM GMT, Ryszard Knop wrote: > > > The drm-intel repo is moving from the classic fd.o git host to GitLab. > > > Update its location with a URL matching other fd.o GitLab kernel trees. > > > > > > Signed-off-by: Ryszard Knop > > > > Acked-by: Lucas De Marchi > > > > Also Cc'ing maintainers > > Thanks, > > Acked-by: Tvrtko Ursulin Acked-by: Rodrigo Vivi > > Regards, > > Tvrtko > > > > --- > > > MAINTAINERS | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index d6327dc12cb1..fbf7371a0bb0 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -10854,7 +10854,7 @@ W: > > > https://drm.pages.freedesktop.org/intel-docs/ > > > Q: http://patchwork.freedesktop.org/project/intel-gfx/ > > > B: > > > https://drm.pages.freedesktop.org/intel-docs/how-to-file-i915-bugs.html > > > C: irc://irc.oftc.net/intel-gfx > > > -T: git git://anongit.freedesktop.org/drm-intel > > > +T: git https://gitlab.freedesktop.org/drm/i915/kernel.git > > > F: Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > > F: Documentation/gpu/i915.rst > > > F: drivers/gpu/drm/ci/xfails/i915* > > > -- > > > 2.44.0 > > >
Re: [PATCH] drm/i915/gt: Automate CCS Mode setting during engine resets
On Fri, Apr 26, 2024 at 02:07:23AM +0200, Andi Shyti wrote:
> We missed setting the CCS mode during resume and engine resets.
> Create a workaround to be added in the engine's workaround list.
> This workaround sets the XEHP_CCS_MODE value at every reset.
>
> The issue can be reproduced by running:
>
> $ clpeak --kernel-latency
>
> Without resetting the CCS mode, we encounter a fence timeout:
>
> Fence expiration time out i915-:03:00.0:clpeak[2387]:2!
>
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10895
> Fixes: 6db31251bb26 ("drm/i915/gt: Enable only one CCS for compute workload")
> Reported-by: Gnattu OC
> Signed-off-by: Andi Shyti
> Cc: Chris Wilson
> Cc: Joonas Lahtinen
> Cc: Matt Roper
> Cc: # v6.2+
Reviewed-by: Rodrigo Vivi
> ---
> Hi Gnattu,
>
> thanks again for reporting this issue and for your prompt
> replies on the issue. Would you give this patch a chance?
>
> Andi
>
> drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c | 6 +++---
> drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h | 2 +-
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 4 +++-
> 3 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
> b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
> index 044219c5960a..99b71bb7da0a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.c
> @@ -8,14 +8,14 @@
> #include "intel_gt_ccs_mode.h"
> #include "intel_gt_regs.h"
>
> -void intel_gt_apply_ccs_mode(struct intel_gt *gt)
> +unsigned int intel_gt_apply_ccs_mode(struct intel_gt *gt)
> {
> int cslice;
> u32 mode = 0;
> int first_ccs = __ffs(CCS_MASK(gt));
>
> if (!IS_DG2(gt->i915))
> - return;
> + return 0;
>
> /* Build the value for the fixed CCS load balancing */
> for (cslice = 0; cslice < I915_MAX_CCS; cslice++) {
> @@ -35,5 +35,5 @@ void intel_gt_apply_ccs_mode(struct intel_gt *gt)
>XEHP_CCS_MODE_CSLICE_MASK);
> }
>
> - intel_uncore_write(gt->uncore, XEHP_CCS_MODE, mode);
> + return mode;
> }
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
> b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
> index 9e5549caeb26..55547f2ff426 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_ccs_mode.h
> @@ -8,6 +8,6 @@
>
> struct intel_gt;
>
> -void intel_gt_apply_ccs_mode(struct intel_gt *gt);
> +unsigned int intel_gt_apply_ccs_mode(struct intel_gt *gt);
>
> #endif /* __INTEL_GT_CCS_MODE_H__ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 68b6aa11bcf7..58693923bf6c 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -2703,6 +2703,7 @@ add_render_compute_tuning_settings(struct intel_gt *gt,
> static void ccs_engine_wa_mode(struct intel_engine_cs *engine, struct
> i915_wa_list *wal)
> {
> struct intel_gt *gt = engine->gt;
> + u32 mode;
>
> if (!IS_DG2(gt->i915))
> return;
> @@ -2719,7 +2720,8 @@ static void ccs_engine_wa_mode(struct intel_engine_cs
> *engine, struct i915_wa_li
>* After having disabled automatic load balancing we need to
>* assign all slices to a single CCS. We will call it CCS mode 1
>*/
> - intel_gt_apply_ccs_mode(gt);
> + mode = intel_gt_apply_ccs_mode(gt);
> + wa_masked_en(wal, XEHP_CCS_MODE, mode);
> }
>
> /*
> --
> 2.43.0
>
Re: [PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
On Mon, Apr 29, 2024 at 01:39:06PM +0200, Maxime Ripard wrote: > On Thu, Apr 25, 2024 at 06:04:50PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote: > > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote: > > > > First of all, the driver was introduced when it was already > > > > two drivers available for Ilitek 9341 panels. > > > > > > > > Second, the most recent (fourth!) driver has incorporated this one > > > > and hence, when enabled, it covers the provided functionality. > > > > > > > > Taking into account the above, remove duplicative driver and make > > > > maintenance and support eaiser for everybody. > > > > > > > > Also see discussion [1] for details about Ilitek 9341 duplication > > > > code. > > > > > > > > Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1] > > > > Signed-off-by: Andy Shevchenko > > > > > > I think it should be the other way around and we should remove the > > > mipi-dbi handling from panel/panel-ilitek-ili9341.c > > > > Then please do it! I whining already for a few years about this. > > I have neither the hardware nor the interest to do so. Seems it looks > like you have plenty of the latter at least, I'm sure you'll find some > time to tackle this. Hmm... Since the use of Arduino part in panel IliTek 9341 is clarified in this thread, I won't use that, but I have no time to clean up the mess in DRM in the nearest future, sorry. And TBH it seems you, guys, know much better what you want. FYI: The drivers/gpu/drm/tiny/mi0283qt.c works for me (the plenty of the HW you referred to). TL;DR: consider this as a (bug/feature/cleanup) report. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Sat, Apr 27, 2024 at 01:57:46PM +0800, Sui Jingfeng wrote: > Hi, > > > On 2024/4/26 14:23, Maxime Ripard wrote: > > Hi, > > > > On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote: > > > On 2024/4/26 03:10, Andy Shevchenko wrote: > > > > On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: > > > > > On 2024/4/25 22:26, Andy Shevchenko wrote: > > > > > > It seems driver missed the point of proper use of device property > > > > > > APIs. > > > > > > Correct this by updating headers and calls respectively. > > > > > You are using the 'seems' here exactly saying that you are not 100% > > > > > sure. > > > > > > > > > > Please allow me to tell you the truth: This patch again has ZERO > > > > > effect. > > > > > It fix nothing. And this patch is has the risks to be wrong. > > > > Huh?! Really, stop commenting the stuff you do not understand. > > > I'm actually a professional display drivers developer at the downstream > > > in the past, despite my contribution to upstream is less. But I believe > > > that all panel driver developers know what I'm talking about. So please > > > have take a look at my replies. > > Most of the interactions you had in this series has been uncalled for. > > You might be against a patch, but there's no need to go to such length. > > > > As far as I'm concerned, this patch is fine to me in itself, and I don't > > see anything that would prevent us from merging it. > > No one is preventing you, as long as don't misunderstanding what other > people's technical replies intentionally. I'm just a usual and normal > contributor, I hope the world will better than yesterday. You should seriously consider your tone when replying then. > Saying such thing to me may not proper, I guess you may want to talk > to peoples who has the push rights I think you misunderstood me. My point was that your several rants were uncalled for and aren't the kind of things we're doing here. I know very well how to get a patch merged, thanks. > just make sure it isn't a insult to the professionalism of drm bridge > community itself though. I'm not sure why you're bringing the bridge community or its professionalism. It's a panel, not a bridge, and I never doubted the professionalism of anyone. Maxime signature.asc Description: PGP signature
Re: [PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
On Thu, Apr 25, 2024 at 06:04:50PM +0300, Andy Shevchenko wrote: > On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote: > > Hi, > > > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote: > > > First of all, the driver was introduced when it was already > > > two drivers available for Ilitek 9341 panels. > > > > > > Second, the most recent (fourth!) driver has incorporated this one > > > and hence, when enabled, it covers the provided functionality. > > > > > > Taking into account the above, remove duplicative driver and make > > > maintenance and support eaiser for everybody. > > > > > > Also see discussion [1] for details about Ilitek 9341 duplication > > > code. > > > > > > Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1] > > > Signed-off-by: Andy Shevchenko > > > > I think it should be the other way around and we should remove the > > mipi-dbi handling from panel/panel-ilitek-ili9341.c > > Then please do it! I whining already for a few years about this. I have neither the hardware nor the interest to do so. Seems it looks like you have plenty of the latter at least, I'm sure you'll find some time to tackle this. Maxime signature.asc Description: PGP signature
Re: drm/debugfs: Drop conditionals around of_node pointers
On Sun, Apr 28, 2024 at 04:52:13PM +0800, Sui Jingfeng wrote:
> ping
>
> 在 2024/3/22 06:22, Sui Jingfeng 写道:
> > Having conditional around the of_node pointer of the drm_bridge structure
> > turns out to make driver code use ugly #ifdef blocks.
The code being ugly is an opinion, what problem is it causing exactly?
> Drop the conditionals to simplify debugfs.
What does it simplifies?
> >
> > Fixes: d8dfccde2709 ("drm/bridge: Drop conditionals around of_node
> > pointers")
> > Signed-off-by: Sui Jingfeng
Why do we want to backport that patch to stable?
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] drm/debugfs: Drop conditionals around of_node pointers
On Fri, Mar 22, 2024 at 06:22:58AM +0800, Sui Jingfeng wrote:
> Having conditional around the of_node pointer of the drm_bridge structure
> turns out to make driver code use ugly #ifdef blocks. Drop the conditionals
> to simplify debugfs.
>
> Fixes: d8dfccde2709 ("drm/bridge: Drop conditionals around of_node pointers")
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/drm_debugfs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Reviewed-by: Dmitry Baryshkov
--
With best wishes
Dmitry
Re: [PATCH RESEND] Revert "drm/bridge: ti-sn65dsi83: Fix enable error path"
On Fri, 26 Apr 2024 14:22:59 +0200, Luca Ceresoli wrote: > This reverts commit 8a91b29f1f50ce7742cdbe5cf11d17f128511f3f. > > The regulator_disable() added by the original commit solves one kind of > regulator imbalance but adds another one as it allows the regulator to be > disabled one more time than it is enabled in the following scenario: > > 1. Start video pipeline -> sn65dsi83_atomic_pre_enable -> regulator_enable > 2. PLL lock fails -> regulator_disable > 3. Stop video pipeline -> sn65dsi83_atomic_disable -> regulator_disable > > [...] Applied, thanks! [1/1] Revert "drm/bridge: ti-sn65dsi83: Fix enable error path" https://cgit.freedesktop.org/drm/drm-misc/commit/?id=2940ee03b232 Rob
Re: [PATCH] drm/kmb: Replace of_node_put() with automatic cleanup handler
On 4/10/24 22:45, Javier Carrasco wrote:
> Make use of the __free() cleanup handler to automatically free nodes
> when they get out of scope.
>
> Suggested-by: Julia Lawall
> Signed-off-by: Javier Carrasco
> ---
> The patch is based on the latest linux-next tag (next-20240410).
> ---
> drivers/gpu/drm/kmb/kmb_drv.c | 13 -
> drivers/gpu/drm/kmb/kmb_dsi.c | 11 ---
> 2 files changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index 169b83987ce2..1a743840688a 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
> @@ -480,8 +480,8 @@ static int kmb_probe(struct platform_device *pdev)
> struct device *dev = get_device(>dev);
> struct kmb_drm_private *kmb;
> int ret = 0;
> - struct device_node *dsi_in;
> - struct device_node *dsi_node;
> + struct device_node *dsi_in __free(device_node) =
> + of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> struct platform_device *dsi_pdev;
>
> /* The bridge (ADV 7535) will return -EPROBE_DEFER until it
> @@ -491,28 +491,23 @@ static int kmb_probe(struct platform_device *pdev)
>* and then the rest of the driver initialization can proceed
>* afterwards and the bridge can be successfully attached.
>*/
> - dsi_in = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> if (!dsi_in) {
> DRM_ERROR("Failed to get dsi_in node info from DT");
> return -EINVAL;
> }
> - dsi_node = of_graph_get_remote_port_parent(dsi_in);
> + struct device_node *dsi_node __free(device_node) =
> + of_graph_get_remote_port_parent(dsi_in);
> if (!dsi_node) {
> - of_node_put(dsi_in);
> DRM_ERROR("Failed to get dsi node from DT\n");
> return -EINVAL;
> }
>
> dsi_pdev = of_find_device_by_node(dsi_node);
> if (!dsi_pdev) {
> - of_node_put(dsi_in);
> - of_node_put(dsi_node);
> DRM_ERROR("Failed to get dsi platform device\n");
> return -EINVAL;
> }
>
> - of_node_put(dsi_in);
> - of_node_put(dsi_node);
> ret = kmb_dsi_host_bridge_init(get_device(_pdev->dev));
>
> if (ret == -EPROBE_DEFER) {
> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
> index cf7cf0b07541..61f02462b778 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
> @@ -216,8 +216,6 @@ static const struct mipi_dsi_host_ops kmb_dsi_host_ops = {
>
> int kmb_dsi_host_bridge_init(struct device *dev)
> {
> - struct device_node *encoder_node, *dsi_out;
> -
> /* Create and register MIPI DSI host */
> if (!dsi_host) {
> dsi_host = kzalloc(sizeof(*dsi_host), GFP_KERNEL);
> @@ -239,21 +237,20 @@ int kmb_dsi_host_bridge_init(struct device *dev)
> }
>
> /* Find ADV7535 node and initialize it */
> - dsi_out = of_graph_get_endpoint_by_regs(dev->of_node, 0, 1);
> + struct device_node *dsi_out __free(device_node) =
> + of_graph_get_endpoint_by_regs(dev->of_node, 0, 1);
> if (!dsi_out) {
> DRM_ERROR("Failed to get dsi_out node info from DT\n");
> return -EINVAL;
> }
> - encoder_node = of_graph_get_remote_port_parent(dsi_out);
> + struct device_node *encoder_node __free(device_node) =
> + of_graph_get_remote_port_parent(dsi_out);
> if (!encoder_node) {
> - of_node_put(dsi_out);
> DRM_ERROR("Failed to get bridge info from DT\n");
> return -EINVAL;
> }
> /* Locate drm bridge from the hdmi encoder DT node */
> adv_bridge = of_drm_find_bridge(encoder_node);
> - of_node_put(dsi_out);
> - of_node_put(encoder_node);
> if (!adv_bridge) {
> DRM_DEBUG("Wait for external bridge driver DT\n");
> return -EPROBE_DEFER;
>
> ---
> base-commit: 6ebf211bb11dfc004a2ff73a9de5386fa309c430
> change-id: 20240410-kmb_of_node_put-aaf1c77d9610
>
> Best regards,
Hi,
according to Patchwork, this patch is still marked as "new", but also
"archived", and still did not get any feedback.
Apparently, this new cleanup mechanism has already been applied to the
subsystem's code (at least in drm/exynos/exynos_hdmi.c in linux-next),
and this one would not be the first case anymore.
Thanks and best regards,
Javier Carrasco
Re: [PATCH v2 4/8] drm/mipi-dsi: Introduce mipi_dsi_*_write_seq_multi()
Hello Mister Anderson,
On 27/04/2024 01:58, Douglas Anderson wrote:
The current mipi_dsi_*_write_seq() macros are non-intutitive because
they contain a hidden "return" statement that will return out of the
_caller_ of the macro. Let's mark them as deprecated and instead
introduce some new macros that are more intuitive.
These new macros are less optimal when an error occurs but should
behave more optimally when there is no error. Specifically these new
macros cause smaller code to get generated and the code size savings
(less to fetch from RAM, less cache space used, less RAM used) are
important. Since the error case isn't something we need to optimize
for and these new macros are easier to understand and more flexible,
they should be used.
After converting to use these new functions, one example shows some
nice savings while also being easier to understand.
$ scripts/bloat-o-meter \
...after/panel-novatek-nt36672e.ko \
...ctx/panel-novatek-nt36672e.ko
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-988 (-988)
Function old new delta
nt36672e_1080x2408_60hz_init62365248-988
Total: Before=10651, After=9663, chg -9.28%
Signed-off-by: Douglas Anderson
---
Right now this patch introduces two new functions in
drm_mipi_dsi.c. Alternatively we could have changed the prototype of
the "chatty" functions and made the deprecated macros adapt to the new
prototype. While this sounds nice, it bloated callers of the
deprecated functioin a bit because it caused the compiler to emit less
optimal code. It doesn't seem terrible to add two more functions, so I
went that way.
Changes in v2:
- New
drivers/gpu/drm/drm_mipi_dsi.c | 56 +
include/drm/drm_mipi_dsi.h | 57 ++
2 files changed, 113 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 1e062eda3b88..b7c75a4396e6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -792,6 +792,34 @@ int mipi_dsi_generic_write_chatty(struct mipi_dsi_device
*dsi,
}
EXPORT_SYMBOL(mipi_dsi_generic_write_chatty);
};
+/**
+ * struct mipi_dsi_multi_context - Context to call multiple MIPI DSI funcs in
a row
+ * @dsi: Pointer to the MIPI DSI device
+ * @accum_err: Storage for the accumulated error over the multiple calls. Init
+ * to 0. If a function encounters an error then the error code will be
+ * stored here. If you call a function and this points to a non-zero
+ * value then the function will be a noop. This allows calling a function
+ * many times in a row and just checking the error at the end to see if
+ * any of them failed.
+ */
+
+struct mipi_dsi_multi_context {
+ struct mipi_dsi_device *dsi;
+ int accum_err;
+};
I like the design, but having a context struct seems over-engineered while we
could pass
a single int over without encapsulating it with mipi_dsi_multi_context.
void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_multi_context *ctx,
const void *data, size_t len);
vs
void mipi_dsi_dcs_write_buffer_multi(struct mipi_dsi_device *dsi, int
*accum_err,
const void *data, size_t len);
is the same, and it avoids having to declare a mipi_dsi_multi_context and set
ctx->dsi,
and I'll find it much easier to migrate, just add a and make sure ret is
initialized to 0.
Re: [PATCH] drm/i915/gt: Disarm breadcrumbs if engines are already idle
Hi Andrzej,
On Friday, 26 April 2024 18:13:02 CEST Nirmoy Das wrote:
>
> On 4/23/2024 6:23 PM, Janusz Krzysztofik wrote:
> > From: Chris Wilson
> >
> > The breadcrumbs use a GT wakeref for guarding the interrupt, but are
> > disarmed during release of the engine wakeref. This leaves a hole where
> > we may attach a breadcrumb just as the engine is parking (after it has
> > parked its breadcrumbs), execute the irq worker with some signalers still
> > attached, but never be woken again.
> >
> > That issue manifests itself in CI with IGT runner timeouts while tests
> > are waiting indefinitely for release of all GT wakerefs.
> >
> > <6> [209.151778] i915: Running
> > live_engine_pm_selftests/live_engine_busy_stats
> > <7> [209.231628] i915 :00:02.0: [drm:intel_power_well_disable [i915]]
> > disabling PW_5
> > <7> [209.231816] i915 :00:02.0: [drm:intel_power_well_disable [i915]]
> > disabling PW_4
> > <7> [209.231944] i915 :00:02.0: [drm:intel_power_well_disable [i915]]
> > disabling PW_3
> > <7> [209.232056] i915 :00:02.0: [drm:intel_power_well_disable [i915]]
> > disabling PW_2
> > <7> [209.232166] i915 :00:02.0: [drm:intel_power_well_disable [i915]]
> > disabling DC_off
> > <7> [209.232270] i915 :00:02.0: [drm:skl_enable_dc6 [i915]] Enabling DC6
> > <7> [209.232368] i915 :00:02.0: [drm:gen9_set_dc_state.part.0 [i915]]
> > Setting DC state from 00 to 02
> > <4> [299.356116] [IGT] Inactivity timeout exceeded. Killing the current
> > test with SIGQUIT.
> > ...
> > <6> [299.356526] sysrq: Show State
> > ...
> > <6> [299.373964] task:i915_selftest state:D stack:11784 pid:5578
> > tgid:5578 ppid:873flags:0x4002
> > <6> [299.373967] Call Trace:
> > <6> [299.373968]
> > <6> [299.373970] __schedule+0x3bb/0xda0
> > <6> [299.373974] schedule+0x41/0x110
> > <6> [299.373976] intel_wakeref_wait_for_idle+0x82/0x100 [i915]
> > <6> [299.374083] ? __pfx_var_wake_function+0x10/0x10
> > <6> [299.374087] live_engine_busy_stats+0x9b/0x500 [i915]
> > <6> [299.374173] __i915_subtests+0xbe/0x240 [i915]
> > <6> [299.374277] ? __pfx___intel_gt_live_setup+0x10/0x10 [i915]
> > <6> [299.374369] ? __pfx___intel_gt_live_teardown+0x10/0x10 [i915]
> > <6> [299.374456] intel_engine_live_selftests+0x1c/0x30 [i915]
> > <6> [299.374547] __run_selftests+0xbb/0x190 [i915]
> > <6> [299.374635] i915_live_selftests+0x4b/0x90 [i915]
> > <6> [299.374717] i915_pci_probe+0x10d/0x210 [i915]
> >
> > At the end of the interrupt worker, if there are no more engines awake,
> > disarm the breadcrumb and go to sleep.
> >
> > Fixes: 9d5612ca165a ("drm/i915/gt: Defer enabling the breadcrumb interrupt
> > to after submission")
> > Closes: https://gitlab.freedesktop.org/drm/intel/issues/10026
> > Signed-off-by: Chris Wilson
> > Cc: Andrzej Hajda
> > Cc: # v5.12+
> > Signed-off-by: Janusz Krzysztofik
>
>
> Acked-by: Nirmoy Das
>
> I will let others/Andrzej r-b this as I am not very familiar with the code.
This patch should be familiar to you, could you please take a look?
Thanks,
Janusz
>
>
> Thanks,
>
> Nirmoy
>
> > ---
> > drivers/gpu/drm/i915/gt/intel_breadcrumbs.c | 15 +++
> > 1 file changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > index d650beb8ed22f..20b9b04ec1e0b 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> > @@ -263,8 +263,13 @@ static void signal_irq_work(struct irq_work *work)
> > i915_request_put(rq);
> > }
> >
> > + /* Lazy irq enabling after HW submission */
> > if (!READ_ONCE(b->irq_armed) && !list_empty(>signalers))
> > intel_breadcrumbs_arm_irq(b);
> > +
> > + /* And confirm that we still want irqs enabled before we yield */
> > + if (READ_ONCE(b->irq_armed) && !atomic_read(>active))
> > + intel_breadcrumbs_disarm_irq(b);
> > }
> >
> > struct intel_breadcrumbs *
> > @@ -315,13 +320,7 @@ void __intel_breadcrumbs_park(struct intel_breadcrumbs
> > *b)
> > return;
> >
> > /* Kick the work once more to drain the signalers, and disarm the irq */
> > - irq_work_sync(>irq_work);
> > - while (READ_ONCE(b->irq_armed) && !atomic_read(>active)) {
> > - local_irq_disable();
> > - signal_irq_work(>irq_work);
> > - local_irq_enable();
> > - cond_resched();
> > - }
> > + irq_work_queue(>irq_work);
> > }
> >
> > void intel_breadcrumbs_free(struct kref *kref)
> > @@ -404,7 +403,7 @@ static void insert_breadcrumb(struct i915_request *rq)
> > * the request as it may have completed and raised the interrupt as
> > * we were attaching it into the lists.
> > */
> > - if (!b->irq_armed || __i915_request_is_complete(rq))
> > + if (!READ_ONCE(b->irq_armed) || __i915_request_is_complete(rq))
> > irq_work_queue(>irq_work);
> > }
> >
>
Re: [PATCH v2 0/8] drm/mipi-dsi: Reduce bloat and add funcs for cleaner init seqs
On Fri, 26 Apr 2024, Douglas Anderson wrote: > The consensus of many DRM folks is that we want to move away from DSI > drivers defining tables of init commands. Instead, we want to move to > init functions that can use common DRM functions. The issue thus far > has been that using the macros mipi_dsi_generic_write_seq() and > mipi_dsi_dcs_write_seq() bloats the driver using them. > > While trying to solve bloat, we realized that the majority of the it > was easy to solve. This series solves the bloat for existing drivers > by moving the printout outside of the macro. > > During discussion of my v1 patch to fix the bloat [1], we also decided > that we really want to change the way that drivers deal with init > sequences to make it clearer. In addition to being cleaner, a side > effect of moving drivers to the new style reduces bloat _even more_. > > This series also contains a few minor fixes / cleanups that I found > along the way. FWIW, I like the general approach taken here. Thanks! BR, Jani. -- Jani Nikula, Intel
Re: [PATCH 06/23] drm/xe/svm: Introduce a helper to build sg table from hmm range
On Fri, 2024-04-26 at 13:35 -0300, Jason Gunthorpe wrote: > On Fri, Apr 26, 2024 at 04:49:26PM +0200, Thomas Hellström wrote: > > On Fri, 2024-04-26 at 09:00 -0300, Jason Gunthorpe wrote: > > > On Fri, Apr 26, 2024 at 11:55:05AM +0200, Thomas Hellström wrote: > > > > First, the gpu_vma structure is something that partitions the > > > > gpu_vm > > > > that holds gpu-related range metadata, like what to mirror, > > > > desired > > > > gpu > > > > caching policies etc. These are managed (created, removed and > > > > split) > > > > mainly from user-space. These are stored and looked up from an > > > > rb- > > > > tree. > > > > > > Except we are talking about SVA here, so all of this should not > > > be > > > exposed to userspace. > > > > I think you are misreading. this is on the level "Mirror this > > region of > > the cpu_vm", "prefer this region placed in VRAM", "GPU will do > > atomic > > accesses on this region", very similar to cpu mmap / munmap and > > madvise. What I'm trying to say here is that this does not directly > > affect the SVA except whether to do SVA or not, and in that case > > what > > region of the CPU mm will be mirrored, and in addition, any gpu > > attributes for the mirrored region. > > SVA is you bind the whole MM and device faults dynamically populate > the mirror page table. There aren't non-SVA regions. Meta data, like > you describe, is meta data for the allocation/migration mechanism, > not > for the page table or that has anything to do with the SVA mirror > operation. > > Yes there is another common scheme where you bind a window of CPU to > a > window on the device and mirror a fixed range, but this is a quite > different thing. It is not SVA, it has a fixed range, and it is > probably bound to a single GPU VMA in a multi-VMA device page table. And this above here is exactly what we're implementing, and the GPU page-tables are populated using device faults. Regions (large) of the mirrored CPU mm need to coexist in the same GPU vm as traditional GPU buffer objects. > > SVA is not just a whole bunch of windows being dynamically created by > the OS, that is entirely the wrong mental model. It would be horrible > to expose to userspace something like that as uAPI. Any hidden SVA > granules and other implementation specific artifacts must not be made > visible to userspace!! Implementation-specific artifacts are not to be made visible to user- space. > > > > If you use dma_map_sg you get into the world of wrongness where > > > you > > > have to track ranges and invalidation has to wipe an entire range > > > - > > > because you cannot do a dma unmap of a single page from a > > > dma_map_sg > > > mapping. This is all the wrong way to use hmm_range_fault. > > > > > > hmm_range_fault() is page table mirroring, it fundamentally must > > > be > > > page-by-page. The target page table structure must have similar > > > properties to the MM page table - especially page by page > > > validate/invalidate. Meaning you cannot use dma_map_sg(). > > > > To me this is purely an optimization to make the driver page-table > > and > > hence the GPU TLB benefit from iommu coalescing / large pages and > > large > > driver PTEs. > > This is a different topic. Leon is working on improving the DMA API > to > get these kinds of benifits for HMM users. dma_map_sg is not the path > to get this. Leon's work should be significantly better in terms of > optimizing IOVA contiguity for a GPU use case. You can get a > guaranteed DMA contiguity at your choosen granual level, even up to > something like 512M. > > > It is true that invalidation will sometimes shoot down > > large gpu ptes unnecessarily but it will not put any additional > > burden > > on the core AFAICT. > > In my experience people doing performance workloads don't enable the > IOMMU due to the high performance cost, so while optimizing iommu > coalescing is sort of interesting, it is not as important as using > the > APIs properly and not harming the much more common situation when > there is no iommu and there is no artificial contiguity. > > > on invalidation since zapping the gpu PTEs effectively stops any > > dma > > accesses. The dma mappings are rebuilt on the next gpu pagefault, > > which, as you mention, are considered slow anyway, but will > > probably > > still reuse the same prefault region, hence needing to rebuild the > > dma > > mappings anyway. > > This is bad too. The DMA should not remain mapped after pages have > been freed, it completely destroys the concept of IOMMU enforced DMA > security and the ACPI notion of untrusted external devices. Hmm. Yes, good point. > > > So as long as we are correct and do not adversely affect core mm, > > If > > the gpu performance (for whatever reason) is severely hampered if > > large gpu page-table-entries are not used, couldn't this be > > considered > > left to the driver? > > Please use the APIs properly. We are trying to improve the DMA API to > better support HMM
Re: [PATCH v4 8/8] drm/v3d: Add modparam for turning off Big/Super Pages
On 28/04/2024 13:40, Maíra Canal wrote:
Add a modparam for turning off Big/Super Pages to make sure that if an
user doesn't want Big/Super Pages enabled, it can disabled it by setting
the modparam to false.
Signed-off-by: Maíra Canal
---
drivers/gpu/drm/v3d/v3d_drv.c | 7 +++
drivers/gpu/drm/v3d/v3d_gemfs.c | 5 +
2 files changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index 28b7ddce7747..1a6e01235df6 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -36,6 +36,13 @@
#define DRIVER_MINOR 0
#define DRIVER_PATCHLEVEL 0
+/* Only expose the `super_pages` modparam if THP is enabled. */
+#ifdef CONFIG_TRANSPARENT_HUGEPAGE
+bool super_pages = true;
+module_param_named(super_pages, super_pages, bool, 0400);
+MODULE_PARM_DESC(super_pages, "Enable/Disable Super Pages support.");
+#endif
+
static int v3d_get_param_ioctl(struct drm_device *dev, void *data,
struct drm_file *file_priv)
{
diff --git a/drivers/gpu/drm/v3d/v3d_gemfs.c b/drivers/gpu/drm/v3d/v3d_gemfs.c
index 31cf5bd11e39..0ade02bb7209 100644
--- a/drivers/gpu/drm/v3d/v3d_gemfs.c
+++ b/drivers/gpu/drm/v3d/v3d_gemfs.c
@@ -11,6 +11,7 @@ void v3d_gemfs_init(struct v3d_dev *v3d)
char huge_opt[] = "huge=within_size";
struct file_system_type *type;
struct vfsmount *gemfs;
+ extern bool super_pages;
/*
* By creating our own shmemfs mountpoint, we can pass in
@@ -20,6 +21,10 @@ void v3d_gemfs_init(struct v3d_dev *v3d)
if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
goto err;
+ /* The user doesn't want to enable Super Pages */
+ if (!super_pages)
+ goto err;
+
type = get_fs_type("tmpfs");
if (!type)
goto err;
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
Re: [PATCH] drm/stm: dsi: relax mode_valid clock tolerance
Hi,
On Wed, Apr 24, 2024 at 09:21:17AM UTC, Maxime Ripard wrote:
> Hi,
>
> Sorry, my previous review didn't go through.
>
> On Fri, Mar 22, 2024 at 11:47:31AM +0100, Sean Nyekjaer wrote:
> > When using the DSI interface via DSI2LVDS bridge, it seems a bit harsh
> > to reguire the requested and the actual px clock to be within
> > 50Hz. A typical LVDS display requires the px clock to be within +-10%.
> >
> > In case for HDMI .5% tolerance is required.
> >
> > Fixes: e01356d18273 ("drm/stm: dsi: provide the implementation of
> > mode_valid()")
> > Signed-off-by: Sean Nyekjaer
> > ---
> > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> > b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> > index d5f8c923d7bc..97936b0ef702 100644
> > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c
> > @@ -322,8 +322,6 @@ dw_mipi_dsi_phy_get_timing(void *priv_data, unsigned
> > int lane_mbps,
> > return 0;
> > }
> >
> > -#define CLK_TOLERANCE_HZ 50
> > -
> > static enum drm_mode_status
> > dw_mipi_dsi_stm_mode_valid(void *priv_data,
> >const struct drm_display_mode *mode,
> > @@ -375,9 +373,10 @@ dw_mipi_dsi_stm_mode_valid(void *priv_data,
> > /*
> > * Filter modes according to the clock value, particularly
> > useful for
> > * hdmi modes that require precise pixel clocks.
> > +* Check that px_clock is within .5% tolerance.
> > */
> > - if (px_clock_hz < target_px_clock_hz - CLK_TOLERANCE_HZ ||
> > - px_clock_hz > target_px_clock_hz + CLK_TOLERANCE_HZ)
> > + if (px_clock_hz < mult_frac(target_px_clock_hz, 995, 1000) ||
> > + px_clock_hz > mult_frac(target_px_clock_hz, 1005, 1000))
> > return MODE_CLOCK_RANGE;
>
> I wonder if it's not something that should be made into a helper. We
> have a couple of drivers doing it already, so it might be worth creating
> a function that checks for a given struct clk pointer and pixel clock if
> it's within parameters.
>
> Maxime
Yes agree, if the same calculation is happening other places.
I can't identify where it happens though.
Would that helper function exist in drivers/gpu/drm/drm_hdmi_helper.c ?
I will give the implementation a shot.
/Sean
Re: [PATCH v2 3/6] drm/xe: Add helper to accumulate exec queue runtime
On 26/04/2024 19:59, Umesh Nerlige Ramappa wrote:
On Fri, Apr 26, 2024 at 11:49:32AM +0100, Tvrtko Ursulin wrote:
On 24/04/2024 00:56, Lucas De Marchi wrote:
From: Umesh Nerlige Ramappa
Add a helper to accumulate per-client runtime of all its
exec queues. Currently that is done in 2 places:
1. when the exec_queue is destroyed
2. when the sched job is completed
Signed-off-by: Umesh Nerlige Ramappa
Signed-off-by: Lucas De Marchi
---
drivers/gpu/drm/xe/xe_device_types.h | 9 +++
drivers/gpu/drm/xe/xe_exec_queue.c | 37
drivers/gpu/drm/xe/xe_exec_queue.h | 1 +
drivers/gpu/drm/xe/xe_sched_job.c | 2 ++
4 files changed, 49 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_device_types.h
b/drivers/gpu/drm/xe/xe_device_types.h
index 2e62450d86e1..33d3bf93a2f1 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -547,6 +547,15 @@ struct xe_file {
struct mutex lock;
} exec_queue;
+ /**
+ * @runtime: hw engine class runtime in ticks for this drm client
+ *
+ * Only stats from xe_exec_queue->lrc[0] are accumulated. For
multi-lrc
+ * case, since all jobs run in parallel on the engines, only the
stats
+ * from lrc[0] are sufficient.
Out of curiousity doesn't this mean multi-lrc jobs will be incorrectly
accounted for? (When capacity is considered.)
TBH, I am not sure what the user would like to see here for multi-lrc.
If reporting the capacity, then we may need to use width as a
multiplication factor for multi-lrc. How was this done in i915?
IMO user has to see the real utilisation - so if there are two VCS and
both are busy, 100% should be reported and not 50%. Latter would be
misleading, either with or without cross-checking with physical utilisation.
In i915 with execlists this works correctly and with GuC you would
probably know the answer better than me.
Regards,
Tvrtko
Regards,
Umesh
Regards,
Tvrtko
+ */
+ u64 runtime[XE_ENGINE_CLASS_MAX];
+
/** @client: drm client */
struct xe_drm_client *client;
};
diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
b/drivers/gpu/drm/xe/xe_exec_queue.c
index 395de93579fa..b7b6256cb96a 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.c
+++ b/drivers/gpu/drm/xe/xe_exec_queue.c
@@ -214,6 +214,8 @@ void xe_exec_queue_fini(struct xe_exec_queue *q)
{
int i;
+ xe_exec_queue_update_runtime(q);
+
for (i = 0; i < q->width; ++i)
xe_lrc_finish(q->lrc + i);
if (!(q->flags & EXEC_QUEUE_FLAG_PERMANENT) && (q->flags &
EXEC_QUEUE_FLAG_VM || !q->vm))
@@ -769,6 +771,41 @@ bool xe_exec_queue_is_idle(struct xe_exec_queue *q)
q->lrc[0].fence_ctx.next_seqno - 1;
}
+/**
+ * xe_exec_queue_update_runtime() - Update runtime for this exec
queue from hw
+ * @q: The exec queue
+ *
+ * Update the timestamp saved by HW for this exec queue and save
runtime
+ * calculated by using the delta from last update. On multi-lrc
case, only the
+ * first is considered.
+ */
+void xe_exec_queue_update_runtime(struct xe_exec_queue *q)
+{
+ struct xe_file *xef;
+ struct xe_lrc *lrc;
+ u32 old_ts, new_ts;
+
+ /*
+ * Jobs that are run during driver load may use an exec_queue,
but are
+ * not associated with a user xe file, so avoid accumulating
busyness
+ * for kernel specific work.
+ */
+ if (!q->vm || !q->vm->xef)
+ return;
+
+ xef = q->vm->xef;
+ lrc = >lrc[0];
+
+ new_ts = xe_lrc_update_timestamp(lrc, _ts);
+
+ /*
+ * Special case the very first timestamp: we don't want the
+ * initial delta to be a huge value
+ */
+ if (old_ts)
+ xef->runtime[q->class] += new_ts - old_ts;
+}
+
void xe_exec_queue_kill(struct xe_exec_queue *q)
{
struct xe_exec_queue *eq = q, *next;
diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h
b/drivers/gpu/drm/xe/xe_exec_queue.h
index 02ce8d204622..45b72daa2db3 100644
--- a/drivers/gpu/drm/xe/xe_exec_queue.h
+++ b/drivers/gpu/drm/xe/xe_exec_queue.h
@@ -66,5 +66,6 @@ struct dma_fence
*xe_exec_queue_last_fence_get(struct xe_exec_queue *e,
struct xe_vm *vm);
void xe_exec_queue_last_fence_set(struct xe_exec_queue *e, struct
xe_vm *vm,
struct dma_fence *fence);
+void xe_exec_queue_update_runtime(struct xe_exec_queue *q);
#endif
diff --git a/drivers/gpu/drm/xe/xe_sched_job.c
b/drivers/gpu/drm/xe/xe_sched_job.c
index cd8a2fba5438..6a081a4fa190 100644
--- a/drivers/gpu/drm/xe/xe_sched_job.c
+++ b/drivers/gpu/drm/xe/xe_sched_job.c
@@ -242,6 +242,8 @@ bool xe_sched_job_completed(struct xe_sched_job
*job)
{
struct xe_lrc *lrc = job->q->lrc;
+ xe_exec_queue_update_runtime(job->q);
+
/*
* Can safely check just LRC[0] seqno as that is last seqno
written when
* parallel handshake is done.
[PATCH] drm/exynos: Add check for dma_set_max_seg_size
Add check for the return value of dma_set_max_seg_size() and return
the error if it fails in order to catch the error.
Fixes: ddfd4ab6bb08 ("drm/exynos: Fix dma_parms allocation")
Signed-off-by: Chen Ni
---
drivers/gpu/drm/exynos/exynos_drm_dma.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_dma.c
b/drivers/gpu/drm/exynos/exynos_drm_dma.c
index e2c7373f20c6..0f942186f3ff 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dma.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dma.c
@@ -51,7 +51,10 @@ static int drm_iommu_attach_device(struct drm_device
*drm_dev,
return -EINVAL;
}
- dma_set_max_seg_size(subdrv_dev, DMA_BIT_MASK(32));
+ ret = dma_set_max_seg_size(subdrv_dev, DMA_BIT_MASK(32));
+ if (ret)
+ return ret;
+
if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
/*
* Keep the original DMA mapping of the sub-device and
--
2.25.1
Re: [PATCH] drm/fb_dma: Add checks in drm_fb_dma_get_scanout_buffer()
Am 26.04.24 um 14:10 schrieb Jocelyn Falempe:
plane->state and plane->state->fb can be NULL, so add a check before
dereferencing them.
Found by testing with the imx driver.
Fixes: 879b3b6511fe9 ("drm/fb_dma: Add generic get_scanout_buffer() for
drm_panic")
Signed-off-by: Jocelyn Falempe
---
drivers/gpu/drm/drm_fb_dma_helper.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/drm_fb_dma_helper.c
b/drivers/gpu/drm/drm_fb_dma_helper.c
index 96e5ab960f12..d7bffde94cc5 100644
--- a/drivers/gpu/drm/drm_fb_dma_helper.c
+++ b/drivers/gpu/drm/drm_fb_dma_helper.c
@@ -167,6 +167,9 @@ int drm_fb_dma_get_scanout_buffer(struct drm_plane *plane,
struct drm_gem_dma_object *dma_obj;
struct drm_framebuffer *fb;
+ if (!plane->state || !plane->state->fb)
+ return -ENODEV;
+
It's EINVAL here. With that fixed, free free to add
Reviewed-by: Thomas Zimmermann
Best regards
Thomas
fb = plane->state->fb;
/* Only support linear modifier */
if (fb->modifier != DRM_FORMAT_MOD_LINEAR)
base-commit: 2e3f08a1ac99cb9a19a5cb151593d4f9df5cc6a7
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Re: After suspend/resume cycle ASPEED VGA monitor suffers with "No Signal" state.
Hi Am 23.04.24 um 21:51 schrieb Cary Garrett: Hello, An Aspeed VGA monitor, in my case AST 2400, after a suspend/resume cycle suffers with a "No Signal" state. This is also true of a IPMI/BMC remote console. To restore the "Signal" state requires a reboot or the following workaround. To restore the "Signal" state without rebooting issue the following commands from a SSH session or serial console after every suspend/resume cycle: sudo modprobe -r ast sudo modprobe ast This a home media server which is updated infrequently, so I am unable offer any guidance as to when this issue started occurring. Just to clarify, suspend/resume did restore the display in earlier versions? Best regards Thomas Regards, Cary Garrett Current environment: uname -a: Linux xx-server 6.8.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 17 Apr 2024 15:20:28 + x86_64 GNU/Linux modinfo: filename: /lib/modules/6.8.7-arch1-1/kernel/drivers/gpu/drm/ast/ast.ko.zst license:GPL and additional rights description:AST author: Dave Airlie firmware: ast_dp501_fw.bin srcversion: 7E39455BCA2D11E968D8B2B alias: pci:v1A03d2010sv*sd*bc03sc*i* alias: pci:v1A03d2000sv*sd*bc03sc*i* depends:i2c-algo-bit retpoline: Y intree: Y name: ast vermagic: 6.8.7-arch1-1 SMP preempt mod_unload sig_id: PKCS#7 signer: Build time autogenerated kernel key sig_key:76:06:C7:84:BC:2F:C6:38:38:61:C1:6F:32:D5:6A:03:88:22:68:1C sig_hashalgo: sha512 signature: 30:66:02:31:00:AD:83:EB:D2:9B:91:E6:C3:9B:52:89:51:4B:BB:06: DE:D7:44:A6:6B:07:92:AA:75:2A:0B:20:26:73:58:09:DF:C3:86:C6: FC:B7:D4:13:5F:5D:35:4D:67:89:73:0E:C2:02:31:00:C3:98:99:67: B4:74:02:5C:6D:D3:81:13:D4:9F:B4:F4:CF:37:8A:7C:84:8C:73:BF: DF:4D:D5:34:B0:0A:CE:0E:59:67:28:98:07:BF:D7:FA:68:B3:37:43: 02:1C:59:3E parm: modeset:Disable/Enable modesetting (int) lspci: 04:00.0 VGA compatible controller: ASPEED Technology, Inc. ASPEED Graphics Family (rev 30) (prog-if 00 [VGA controller]) DeviceName: Onboard VGA Subsystem: Super Micro Computer Inc Device 0804 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 Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Re: [PATCH] drm/virtio: fix memory leak of vbuf
… > Therefore, when upload fails，vbuf needs to be free directly. Please choose a corresponding imperative wording for an improved change description. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9-rc5#n94 Regards, Markus
Re: [PATCH v4 03/16] dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC
On 26/04/2024 19:22, Alexandre Mergnat wrote:
>regulators:
> type: object
> $ref: /schemas/regulator/mediatek,mt6357-regulator.yaml
> @@ -83,6 +111,12 @@ examples:
> interrupt-controller;
> #interrupt-cells = <2>;
>
> +audio-codec {
> +mediatek,micbias0-microvolt = <170>;
> +mediatek,micbias1-microvolt = <170>;
> +vaud28-supply = <_vaud28_reg>;
And now you should see how odd it looks. Supplies are part of entire
chip, not subblock, even if they supply dedicated domain within that chip.
That's why I asked to put it in the parent node.
Best regards,
Krzysztof
Re: [PATCH 2/2] drm/nouveau/gsp: Use the sg allocator for level 2 of radix3
> Currently, this can result in runtime PM issues on systems where memory
> Luckily, we don't actually need to allocate coherent memory for the page
> table thanks to being able to pass the GPU a radix3 page table for
> suspend/resume data. So, let's rewrite nvkm_gsp_radix3_sg() to use the sg
> allocator for level 2. We continue using coherent allocations for lvl0 and
> 1, since they only take a single page.
>
> Signed-off-by: Lyude Paul
> Cc: sta...@vger.kernel.org
> ---
> .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 4 +-
> .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c| 71 ---
> 2 files changed, 47 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> index 6f5d376d8fcc1..a11d16a16c3b2 100644
> --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> @@ -15,7 +15,9 @@ struct nvkm_gsp_mem {
> };
>
> struct nvkm_gsp_radix3 {
> - struct nvkm_gsp_mem mem[3];
> + struct nvkm_gsp_mem lvl0;
> + struct nvkm_gsp_mem lvl1;
> + struct sg_table lvl2;
This looks great, could we go a step further and combine lvl0 and lvl1
into a 2 page allocation, I thought we could combine lvl0/lvl1 into a
2 page alloc, but that actually might be a bad idea under memory
pressure.
Dave.
Re: [PATCH v4 02/16] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document
On 26/04/2024 19:22, Alexandre Mergnat wrote: > + link-name: > +description: Indicates dai-link name and PCM stream name > +enum: > + - I2S_IN_BE > + - I2S_OUT_BE > + - PCM1_BE > + - PDM1_BE > + - PDM2_BE > + - PDM3_BE > + - PDM4_BE > + - SPDIF_IN_BE > + - SPDIF_OUT_BE > + - TDM_IN_BE > + - TDM_OUT_BE Feels like BE is redundant. Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
