Re: [Linaro-mm-sig] [PATCH next] dma-buf/sync-file: do not allow zero size allocations
That problem is already fixed with patch 21d139d73f77 dma-buf/sync-file:
fix logic error in new fence merge code.
Am 30.03.22 um 00:14 schrieb Pavel Skripkin:
syzbot reported GPF in dma_fence_array_first(), which is caused by
dereferencing ZERO_PTR in dma-buf internals.
ZERO_PTR was generated in sync_file_merge(). This functuion tries to
reduce allocation size, but does not check if it reducing to 0.
This is actually perfectly ok. The code above should have prevented the
size to become 0.
Regards,
Christian.
Fix reported bug by validating `index` value before passing it to
krealloc_array().
Fail log:
general protection fault, probably for non-canonical address
0xdc02: [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0010-0x0017]
CPU: 1 PID: 3595 Comm: syz-executor814 Not tainted
5.17.0-next-20220328-syzkaller #0
...
RIP: 0010:dma_fence_array_first+0x78/0xb0 drivers/dma-buf/dma-fence-array.c:234
...
Call Trace:
__dma_fence_unwrap_array include/linux/dma-fence-unwrap.h:42 [inline]
dma_fence_unwrap_first include/linux/dma-fence-unwrap.h:57 [inline]
sync_file_ioctl_fence_info drivers/dma-buf/sync_file.c:414 [inline]
sync_file_ioctl+0x248/0x22c0 drivers/dma-buf/sync_file.c:477
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
There was same problem with initial kcalloc() allocation in same
function, so it's fixed as well.
Reported-and-tested-by: syzbot+5c943fe38e86d615c...@syzkaller.appspotmail.com
Fixes: 519f490db07e ("dma-buf/sync-file: fix warning about fence containers")
Signed-off-by: Pavel Skripkin
---
drivers/dma-buf/sync_file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index b8dea4ec123b..aa744f017008 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -212,7 +212,7 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
dma_fence_unwrap_for_each(b_fence, &b_iter, b->fence)
++num_fences;
- if (num_fences > INT_MAX)
+ if (num_fences > INT_MAX || !num_fences)
goto err_free_sync_file;
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL);
@@ -264,7 +264,7 @@ static struct sync_file *sync_file_merge(const char *name,
struct sync_file *a,
if (index == 0)
add_fence(fences, &index, dma_fence_get_stub());
- if (num_fences > index) {
+ if (index && num_fences > index) {
struct dma_fence **tmp;
/* Keep going even when reducing the size failed */
Re: [PATCH 6/8] drm/display: Move HDCP helpers into display-helper module
On Tue, 22 Mar 2022, Thomas Zimmermann wrote: > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c > b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 21f16394012f..0ad78c73af7c 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -23,11 +23,11 @@ > > #include > #include > +#include This helper helper crept in a few places. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 3/3] arm64: dts: qcom: sdm845-xiaomi-beryllium: enable qcom wled backlight and link to panel
On 2022-03-30 12:26:39, Joel Selvaraj wrote:
> Xiaomi Poco F1 uses the QCOM WLED driver for backlight control.
> Enable and link it to the panel to use it.
>
> Signed-off-by: Joel Selvaraj
> ---
> .../arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> index 798fc72578a7..3ebb0f9905d3 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
> @@ -231,6 +231,7 @@ panel@0 {
> #address-cells = <1>;
> #size-cells = <0>;
>
> + backlight = <&pmi8998_wled>;
> reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
>
> port {
> @@ -314,6 +315,18 @@ vol_up_pin_a: vol-up-active {
> };
> };
>
> +&pmi8998_wled {
> + status = "okay";
> + qcom,current-boost-limit = <970>;
> + qcom,ovp-millivolt = <29600>;
> + qcom,current-limit-microamp = <2>;
> + qcom,enabled-strings = <0 1>;
> + qcom,num-strings = <2>;
No need to set both nowadays, the driver will even print a warning in
this case:
https://lore.kernel.org/linux-arm-msm/2025203459.1634079-6-marijn.suij...@somainline.org/
Sticking with qcom,num-strings is probably the right choice here.
- Marijn
> + qcom,switching-freq = <600>;
> + qcom,external-pfet;
> + qcom,cabc;
> +};
> +
> &pm8998_pon {
> resin {
> compatible = "qcom,pm8941-resin";
> --
> 2.35.1
>
Re: [PATCH v9 00/23] drm/rockchip: RK356x VOP2 support
Hi Piotr, On Tue, Mar 29, 2022 at 09:31:01AM +0200, Piotr Oniszczuk wrote: > > > > Wiadomość napisana przez Sascha Hauer w dniu > > 28.03.2022, o godz. 17:10: > > > > > > Changes since v8: > > - make hclk_vo a critical clock instead of enabling it in the hdmi driver > > - Fix vop2_setup_layer_mixer(), reported by Andy Yan > > - Limit planes possible_crtcs to actually existing crtcs > > > > > > Sascha, > > FYI: > I was hoping v9 will fix green screen issue i see when video player wants to > draw to nv12 capable drm plane. > It look issue is still present :-( > > You can easily reproduce with modetest utility: > > modetest -P 43@67:1920x1080@NV12 This only sets the overlay, but how did you get something on the screen initially? I did with "modetest -s 69@67:1920x1080 -d" and with this it works as expected, I can't reproduce any green screen issue here. I found another problem though which might or might not be related with your issue. I saw that the overlay is not exactly centered as it ought to be. This goes down to wrong delay settings for the overlay, the following patch fixes this. Sascha -8<--- >From f9a92401344e8aa3203fca2236dd4a40cc8690f6 Mon Sep 17 00:00:00 2001 From: Sascha Hauer Date: Wed, 30 Mar 2022 09:22:26 +0200 Subject: [PATCH] fixup! drm: rockchip: Add VOP2 driver --- drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c index 69e9870d5f2dc..7dba7b9b63dc6 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c @@ -1979,10 +1979,10 @@ static void vop2_setup_dly_for_windows(struct vop2 *vop2) sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__ESMART1, dly); break; case ROCKCHIP_VOP2_SMART0: - sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART1, dly); + sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART0, dly); break; case ROCKCHIP_VOP2_SMART1: - sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART0, dly); + sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART1, dly); break; } } -- 2.30.2 -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH 3/3] arm64: dts: qcom: sdm845-xiaomi-beryllium: enable qcom wled backlight and link to panel
Xiaomi Poco F1 uses the QCOM WLED driver for backlight control.
Enable and link it to the panel to use it.
Signed-off-by: Joel Selvaraj
---
.../arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts | 13 +
1 file changed, 13 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
index 798fc72578a7..3ebb0f9905d3 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
@@ -231,6 +231,7 @@ panel@0 {
#address-cells = <1>;
#size-cells = <0>;
+ backlight = <&pmi8998_wled>;
reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
port {
@@ -314,6 +315,18 @@ vol_up_pin_a: vol-up-active {
};
};
+&pmi8998_wled {
+ status = "okay";
+ qcom,current-boost-limit = <970>;
+ qcom,ovp-millivolt = <29600>;
+ qcom,current-limit-microamp = <2>;
+ qcom,enabled-strings = <0 1>;
+ qcom,num-strings = <2>;
+ qcom,switching-freq = <600>;
+ qcom,external-pfet;
+ qcom,cabc;
+};
+
&pm8998_pon {
resin {
compatible = "qcom,pm8941-resin";
--
2.35.1
[PATCH 1/3] drm/panel: nt36672a: add backlight support
Add support for backlight. This panel supports backlight control through the QCOM WLED driver in Xiaomi Poco F1 device. Signed-off-by: Joel Selvaraj --- drivers/gpu/drm/panel/panel-novatek-nt36672a.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c index 231f371901e8..6d6ce42787e2 100644 --- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c +++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c @@ -628,6 +628,10 @@ static int nt36672a_panel_add(struct nt36672a_panel *pinfo) drm_panel_init(&pinfo->base, dev, &panel_funcs, DRM_MODE_CONNECTOR_DSI); + ret = drm_panel_of_backlight(&pinfo->base); + if (ret) + return dev_err_probe(dev, ret, "Failed to get backlight\n"); + drm_panel_add(&pinfo->base); return 0; -- 2.35.1
[syzbot] general protection fault in dma_fence_array_first
Hello,
syzbot found the following issue on:
HEAD commit:8515d05bf6bc Add linux-next specific files for 20220328
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=1694e21b70
kernel config: https://syzkaller.appspot.com/x/.config?x=530c68bef4e2b8a8
dashboard link: https://syzkaller.appspot.com/bug?extid=5c943fe38e86d615cac2
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for
Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1467313b70
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=121b7cb970
The issue was bisected to:
commit 519f490db07e1a539490612f376487f61e48e39c
Author: Christian König
Date: Fri Mar 11 09:32:26 2022 +
dma-buf/sync-file: fix warning about fence containers
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1058277d70
final oops: https://syzkaller.appspot.com/x/report.txt?x=1258277d70
console output: https://syzkaller.appspot.com/x/log.txt?x=1458277d70
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+5c943fe38e86d615c...@syzkaller.appspotmail.com
Fixes: 519f490db07e ("dma-buf/sync-file: fix warning about fence containers")
general protection fault, probably for non-canonical address
0xdc02: [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0010-0x0017]
CPU: 1 PID: 3595 Comm: syz-executor814 Not tainted
5.17.0-next-20220328-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
01/01/2011
RIP: 0010:dma_fence_array_first+0x78/0xb0 drivers/dma-buf/dma-fence-array.c:234
Code: 00 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c 02 00 75 43 48 8b 9b 88 00 00
00 48 b8 00 00 00 00 00 fc ff df 48 89 da 48 c1 ea 03 <80> 3c 02 00 75 1b 4c 8b
23 e8 fa a9 e0 fc 4c 89 e0 5b 41 5c c3 45
RSP: 0018:c90003a4fd48 EFLAGS: 00010202
RAX: dc00 RBX: 0010 RCX:
RDX: 0002 RSI: 84980052 RDI: 888015c76388
RBP: 888015c76300 R08: R09: 888015c7633b
R10: 8498f6ba R11: R12: 888015c76300
R13: 888015c76690 R14: c0383e04 R15: 20001840
FS: 56872300() GS:8880b9d0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 20001528 CR3: 1e82f000 CR4: 003506e0
DR0: DR1: DR2:
DR3: DR6: fffe0ff0 DR7: 0400
Call Trace:
__dma_fence_unwrap_array include/linux/dma-fence-unwrap.h:42 [inline]
dma_fence_unwrap_first include/linux/dma-fence-unwrap.h:57 [inline]
sync_file_ioctl_fence_info drivers/dma-buf/sync_file.c:414 [inline]
sync_file_ioctl+0x248/0x22c0 drivers/dma-buf/sync_file.c:477
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:870 [inline]
__se_sys_ioctl fs/ioctl.c:856 [inline]
__x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0x80 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f6aae8951b9
Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 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 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:7ffedd290238 EFLAGS: 0246 ORIG_RAX: 0010
RAX: ffda RBX: RCX: 7f6aae8951b9
RDX: 20001840 RSI: c0383e04 RDI: 0007
RBP: 7f6aae8591a0 R08: R09:
R10: R11: 0246 R12: 7f6aae859230
R13: R14: R15:
Modules linked in:
---[ end trace ]---
RIP: 0010:dma_fence_array_first+0x78/0xb0 drivers/dma-buf/dma-fence-array.c:234
Code: 00 00 fc ff df 48 89 fa 48 c1 ea 03 80 3c 02 00 75 43 48 8b 9b 88 00 00
00 48 b8 00 00 00 00 00 fc ff df 48 89 da 48 c1 ea 03 <80> 3c 02 00 75 1b 4c 8b
23 e8 fa a9 e0 fc 4c 89 e0 5b 41 5c c3 45
RSP: 0018:c90003a4fd48 EFLAGS: 00010202
RAX: dc00 RBX: 0010 RCX:
RDX: 0002 RSI: 84980052 RDI: 888015c76388
RBP: 888015c76300 R08: R09: 888015c7633b
R10: 8498f6ba R11: R12: 888015c76300
R13: 888015c76690 R14: c0383e04 R15: 20001840
FS: 56872300() GS:8880b9d0() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 20001528 CR3: 1e82f000 CR4: 003506e0
DR0: DR1: DR2:
DR3: DR6: fffe0ff0 DR7: 0400
Code disassembly (best guess), 4 bytes skipped:
0: df 48 89fisttps -0x77(%rax)
3: fa
[PATCH 2/3] dt-bindings: display: novatek, nt36672a: add backlight property
Add backlight property and update example to include it.
Signed-off-by: Joel Selvaraj
---
.../devicetree/bindings/display/panel/novatek,nt36672a.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git
a/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
b/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
index 563766d283f6..41ee3157a1cd 100644
--- a/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
+++ b/Documentation/devicetree/bindings/display/panel/novatek,nt36672a.yaml
@@ -46,6 +46,7 @@ properties:
reg: true
port: true
+ backlight: true
required:
- compatible
@@ -73,6 +74,7 @@ examples:
vddpos-supply = <&lab>;
vddneg-supply = <&ibb>;
+backlight = <&pmi8998_wled>;
reset-gpios = <&tlmm 6 GPIO_ACTIVE_HIGH>;
port {
--
2.35.1
Re: [syzbot] general protection fault in dma_fence_array_first
Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+5c943fe38e86d615c...@syzkaller.appspotmail.com Tested on: commit: c2528a0c Add linux-next specific files for 20220329 git tree: linux-next kernel config: https://syzkaller.appspot.com/x/.config?x=88d1370cc1f241e6 dashboard link: https://syzkaller.appspot.com/bug?extid=5c943fe38e86d615cac2 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 patch: https://syzkaller.appspot.com/x/patch.diff?x=128372e770 Note: testing is done by a robot and is best-effort only.
[PATCH 0/3] drm/panel: nt36672a: add backlight support
Joel Selvaraj (3): drm/panel: nt36672a: add backlight support dt-bindings: display: novatek,nt36672a: add backlight property arm64: dts: qcom: sdm845-xiaomi-beryllium: enable qcom wled backlight and link to panel .../bindings/display/panel/novatek,nt36672a.yaml| 2 ++ .../arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts | 13 + drivers/gpu/drm/panel/panel-novatek-nt36672a.c | 4 3 files changed, 19 insertions(+) -- 2.35.1
Re: [PATCH v6 02/12] clk: Introduce Kunit Tests for the framework
Hi Daniel, On Mon, Mar 28, 2022 at 02:36:25PM -0500, Daniel Latypov wrote: > On Mon, Mar 28, 2022 at 2:57 AM Maxime Ripard wrote: > > On Fri, Mar 25, 2022 at 05:36:25PM -0500, Daniel Latypov wrote: > > > On Mon, Feb 28, 2022 at 4:47 AM Maxime Ripard wrote: > > > > > > > > On Fri, Feb 25, 2022 at 01:29:03PM -0800, Daniel Latypov wrote: > > > > > On Fri, Feb 25, 2022 at 5:23 AM Maxime Ripard > > > > > wrote: > > > > > > > > > > > > Hi Daniel, > > > > > > > > > > > > On Wed, Feb 23, 2022 at 02:50:59PM -0800, Daniel Latypov wrote: > > > > > > > On Wed, Feb 23, 2022 at 2:56 AM Maxime Ripard > > > > > > > wrote: > > > > > > > > > > > > > > > > Let's test various parts of the rate-related clock API with the > > > > > > > > kunit > > > > > > > > testing framework. > > > > > > > > > > > > > > > > Cc: kunit-...@googlegroups.com > > > > > > > > Suggested-by: Stephen Boyd > > > > > > > > Signed-off-by: Maxime Ripard > > > > > > > > > > > > > > Tested-by: Daniel Latypov > > > > > > > > > > > > > > Looks good to me on the KUnit side. > > > > > > > Two small nits below. > > > > > > > > > > > > > > FYI, I computed the incremental coverage for this series, i.e.: > > > > > > > 1) applied the full series > > > > > > > 2) computed the absolute coverage > > > > > > > > > > > > > > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk > > > > > > > --make_options=CC=/usr/bin/gcc-6 > > > > > > > --kconfig_add=CONFIG_DEBUG_KERNEL=y > > > > > > > --kconfig_add=CONFIG_DEBUG_INFO=y --kconfig_add=CONFIG_GCOV=y > > > > > > > > > > > > I built a docker container based on ubuntu 18.04 to have gcc6 and > > > > > > python3.7, but this doesn't seem to be working, I'm not entirely > > > > > > sure why: > > > > > > > > > > > > [13:11:22] Configuring KUnit Kernel ... > > > > > > Regenerating .config ... > > > > > > Populating config with: > > > > > > $ make ARCH=um olddefconfig CC=/usr/bin/gcc-6 O=.kunit > > > > > > ERROR:root:Not all Kconfig options selected in kunitconfig were in > > > > > > the generated .config. > > > > > > This is probably due to unsatisfied dependencies. > > > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y > > > > > > Note: many Kconfig options aren't available on UML. You can try > > > > > > running on a different architecture with something like > > > > > > "--arch=x86_64". > > > > > > > > > > Did you perhaps drop CONFIG_DEBUG_KERNEL=y? > > > > > Need to add 3 config options in total for coverage. > > > > > > > > > > If I tweak the command I ran above but drop CONFIG_DEBUG_KERNEL=y, I > > > > > get the error message you get: > > > > > > > > > > $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/clk > > > > > --make_options=CC=/usr/bin/gcc-6 --kconfig_add=CONFIG_DEBUG_INFO=y > > > > > --kconfig_add=CONFIG_GCOV=y > > > > > ... > > > > > Missing: CONFIG_DEBUG_INFO=y, CONFIG_GCOV=y > > > > > Note: many Kconfig options aren't available on UML. You can try > > > > > running on a different architecture with something like > > > > > "--arch=x86_64". > > > > > > > > It looks to me that it's more that DEBUG_INFO isn't enabled. > > > > > > Sorry for the very delayed response. > > > I was largely getting internet over mobile data around when this email > > > came in and didn't want to try and download docker images over that. > > > > > > It looks like that there was another change that is now merged into > > > Linus' tree that causes this. > > > > > > I found that adding this helped (thanks David Gow) > > > --kconfig_add=DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT > > > > > > Running against --kunitconfig=lib/kunit, my final coverage result is > > > > > > Overall coverage rate: > > > lines..: 13.6% (18004 of 132055 lines) > > > functions..: 15.7% (1885 of 12010 functions) > > > > > > Can you give that a shot and see if it works? > > > > It does fix the configuration issue, but I'm not able to run the tests > > either: > > > > [07:53:51] Configuring KUnit Kernel ... > > Generating .config ... > > Populating config with: > > $ make ARCH=um olddefconfig O=/home/max/out > > [07:53:53] Building KUnit Kernel ... > > Populating config with: > > $ make ARCH=um olddefconfig O=/home/max/out > > Building with: > > $ make ARCH=um --jobs=16 O=/home/max/out > > [07:54:09] Starting KUnit Kernel (1/1)... > > [07:54:09] > > [07:54:09] [ERROR] Test : invalid KTAP input! > > [07:54:09] > > [07:54:09] Testing complete. Passed: 0, Failed: 0, Crashed: 0, Skipped: 0, > > Errors: 1 > > [07:54:09] Elapsed time: 18.486s total, 2.430s configuring, 16.052s > > building, 0.003s running > > > > > > I've tried to remove all the coverage from the equation, and I get the > > same issue if I only run kunit run from inside the container, but it > > works fine outside. So I guess it's my setup that is broken. Is there > > some way to debug what could be going wrong there? > > kunit.py is failing to f
[PATCH 1/1] drm/amdkfd: Create file descriptor after client is added to smi_clients list
This ensures userspace cannot prematurely clean-up the client before
it is fully initialised which has been proven to cause issues in the
past.
Cc: Felix Kuehling
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: amd-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Lee Jones
---
drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
index e4beebb1c80a2..c5d5398d45cbf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
@@ -247,15 +247,6 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
return ret;
}
- ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
- O_RDWR);
- if (ret < 0) {
- kfifo_free(&client->fifo);
- kfree(client);
- return ret;
- }
- *fd = ret;
-
init_waitqueue_head(&client->wait_queue);
spin_lock_init(&client->lock);
client->events = 0;
@@ -265,5 +256,14 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
list_add_rcu(&client->list, &dev->smi_clients);
spin_unlock(&dev->smi_lock);
+ ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
+ O_RDWR);
+ if (ret < 0) {
+ kfifo_free(&client->fifo);
+ kfree(client);
+ return ret;
+ }
+ *fd = ret;
+
return 0;
}
--
2.35.1.1021.g381101b075-goog
Re: [PATCH 6/8] drm/display: Move HDCP helpers into display-helper module
Hi Jani Am 30.03.22 um 09:12 schrieb Jani Nikula: On Tue, 22 Mar 2022, Thomas Zimmermann wrote: diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c index 21f16394012f..0ad78c73af7c 100644 --- a/drivers/gpu/drm/bridge/analogix/anx7625.c +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c @@ -23,11 +23,11 @@ #include #include +#include This helper helper crept in a few places. Thanks for reporting. I'll try to enable more drivers for the patches' next iteration in order to find such issues. Best regards Thomas BR, Jani. -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 1/8] drm: Put related statements next to each other in Makefile
On Tue, 22 Mar 2022, Thomas Zimmermann wrote: > Give the Makefile a bit more structure by putting rules for core, > helpers, drivers, etc next to each other. If you're up for it, I think it would be time to split these one per line, in alphabetical order, to make the diffs nicer: drm-y := \ drm_aperture.o \ drm_auth.o \ ... Sure it takes up a lot of vertical screen estate, but IMO makes life easier in the long run. Definitely can be a follow-up, I don't really want to make the series harder to land than it already is. BR, Jani. > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/Makefile | 23 +-- > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index c2ef5f9fce54..e5929437e13c 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -18,7 +18,6 @@ drm-y := drm_aperture.o drm_auth.o drm_cache.o \ > drm_syncobj.o drm_lease.o drm_writeback.o drm_client.o \ > drm_client_modeset.o drm_atomic_uapi.o \ > drm_managed.o drm_vblank_work.o > - > drm-$(CONFIG_DRM_LEGACY) += drm_agpsupport.o drm_bufs.o drm_context.o > drm_dma.o \ > drm_hashtab.o drm_irq.o drm_legacy_misc.o > drm_lock.o \ > drm_memory.o drm_scatter.o drm_vm.o > @@ -30,8 +29,16 @@ drm-$(CONFIG_PCI) += drm_pci.o > drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o > drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o > drm_privacy_screen_x86.o > +obj-$(CONFIG_DRM)+= drm.o > > obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o > +obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o > + > +# > +# Memory-management helpers > +# > + > +obj-$(CONFIG_DRM_BUDDY) += drm_buddy.o > > drm_cma_helper-y := drm_gem_cma_helper.o > drm_cma_helper-$(CONFIG_DRM_KMS_HELPER) += drm_fb_cma_helper.o > @@ -40,14 +47,16 @@ obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o > drm_shmem_helper-y := drm_gem_shmem_helper.o > obj-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_shmem_helper.o > > -obj-$(CONFIG_DRM_BUDDY) += drm_buddy.o > - > drm_vram_helper-y := drm_gem_vram_helper.o > obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o > > drm_ttm_helper-y := drm_gem_ttm_helper.o > obj-$(CONFIG_DRM_TTM_HELPER) += drm_ttm_helper.o > > +# > +# Modesetting helpers > +# > + > drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o \ > drm_dsc.o drm_encoder_slave.o drm_flip_work.o drm_hdcp.o \ > drm_probe_helper.o \ > @@ -60,14 +69,16 @@ drm_kms_helper-y := drm_bridge_connector.o > drm_crtc_helper.o \ > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > - > obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o > + > +# > +# Drivers and the rest > +# > + > obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/ > > -obj-$(CONFIG_DRM)+= drm.o > obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o > obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o > -obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o > obj-y+= arm/ > obj-y+= dp/ > obj-$(CONFIG_DRM_TTM)+= ttm/ -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 1/8] drm: Put related statements next to each other in Makefile
Hello Thomas, On 3/22/22 20:27, Thomas Zimmermann wrote: > Give the Makefile a bit more structure by putting rules for core, > helpers, drivers, etc next to each other. > > Signed-off-by: Thomas Zimmermann > --- This is a nice cleanup. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
RE: [PATCH v2] drm/amdgpu: fix that issue that the number of the crtc of the 3250c is not correct
>Hi Chandan,
>
>This issue we found on the Zork project which uses the kernel 5.4 on. So I
>just implemented it on kernel 5.4.
>For finding out which is 3250c, I referenced the function which is implemented
>from another function.
>Below is the part where I found it.
>
>drivers/gpu/drm/amd/amdgpu/amdgpu_device.c-
> case CHIP_RAVEN:
> if (adev->rev_id >= 8)
> chip_name = "raven2";
> else if (adev->pdev->device == 0x15d8)
> chip_name = "picasso";
> else
> chip_name = "raven";
> break;
>
>BR,
>Ryan Lin.
Suggest you to rebase to amd-staging-drm-next tip and update your changes
accordingly and re-submit.
>
>Hi Ryan,
>
>Is this change applicable on a specific kernel version?
>On latest I see IP DISCOVERY based impl for CHIP_RAVEN.
>
>>[Why]
>>External displays take priority over internal display when there are fewer
>>display controllers than displays.
>>
>> [How]
>>The root cause is because of that number of the crtc is not correct.
>>The number of the crtc on the 3250c is 3, but on the 3500c is 4.
>>On the source code, we can see that number of the crtc has been fixed at 4.
>>Needs to set the num_crtc to 3 for 3250c platform.
>>
>>v2:
>> - remove unnecessary comments and Id
>>
>>Signed-off-by: Ryan Lin
>>
>>---
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 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 40c91b448f7da..455a2c45e8cda 100644
>>--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>@@ -2738,9 +2738,15 @@ static int dm_early_init(void *handle)
>> break;
>> #if defined(CONFIG_DRM_AMD_DC_DCN1_0)
>> case CHIP_RAVEN:
>>- adev->mode_info.num_crtc = 4;
>>- adev->mode_info.num_hpd = 4;
>>- adev->mode_info.num_dig = 4;
>>+ if (adev->rev_id >= 8) {
>
>May I know what this ">=8" indicate? Also, should it be external_rev_id if its
>based on old version?
>
>>+ adev->mode_info.num_crtc = 3;
>>+ adev->mode_info.num_hpd = 3;
>>+ adev->mode_info.num_dig = 3;
>>+ } else {
>>+ adev->mode_info.num_crtc = 4;
>>+ adev->mode_info.num_hpd = 4;
>>+ adev->mode_info.num_dig = 4;
>>+ }
>> break;
>> #endif
>> #if defined(CONFIG_DRM_AMD_DC_DCN2_0)
>>--
>>2.25.1
>>
>
>BR,
>Chandan V N
Re: (subset) [PATCH v3] drm: of: Properly try all possible cases for bridge/panel detection
On Tue, 29 Mar 2022 15:27:32 +0200, Paul Kocialkowski wrote: > While bridge/panel detection was initially relying on the usual > port/ports-based of graph detection, it was recently changed to > perform the lookup on any child node that is not port/ports > instead when such a node is available, with no fallback on the > usual way. > > This results in breaking detection when a child node is present > but does not contain any panel or bridge node, even when the > usual port/ports-based of graph is there. > > [...] Applied to drm/drm-misc (drm-misc-next-fixes). Thanks! Maxime
Re: [PATCH 3/3] arm64: dts: qcom: sdm845-xiaomi-beryllium: enable qcom wled backlight and link to panel
Hi Marijn,
On 30/03/22 12:47, Marijn Suijten wrote:
> On 2022-03-30 12:26:39, Joel Selvaraj wrote:
>> Xiaomi Poco F1 uses the QCOM WLED driver for backlight control.
>> Enable and link it to the panel to use it.
>>
>> Signed-off-by: Joel Selvaraj
>> ---
>> .../arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts | 13 +
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
>> index 798fc72578a7..3ebb0f9905d3 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium.dts
>> @@ -231,6 +231,7 @@ panel@0 {
>>#address-cells = <1>;
>>#size-cells = <0>;
>>
>> + backlight = <&pmi8998_wled>;
>>reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
>>
>>port {
>> @@ -314,6 +315,18 @@ vol_up_pin_a: vol-up-active {
>>};
>> };
>>
>> +&pmi8998_wled {
>> + status = "okay";
>> + qcom,current-boost-limit = <970>;
>> + qcom,ovp-millivolt = <29600>;
>> + qcom,current-limit-microamp = <2>;
>> + qcom,enabled-strings = <0 1>;
>> + qcom,num-strings = <2>;
>
> No need to set both nowadays, the driver will even print a warning in
> this case:
>
>
https://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-arm-msm%2F2025203459.1634079-6-marijn.suijten%40somainline.org%2F&data=04%7C01%7C%7C2104b54ac0f54308dd1208da121d706a%7C84df9e7fe9f640afb435%7C1%7C0%7C637842214894184949%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=GYmzHoy6tTAE7ZNTqclfCZH5Fnu%2Bh5d5JHOwsm4wVro%3D&reserved=0
>
> Sticking with qcom,num-strings is probably the right choice here.
Ok. Thanks. Will fix it in the next version.
Regards
Joel
>
> - Marijn
>
>> + qcom,switching-freq = <600>;
>> + qcom,external-pfet;
>> + qcom,cabc;
>> +};
>> +
>> &pm8998_pon {
>>resin {
>>compatible = "qcom,pm8941-resin";
>> --
>> 2.35.1
>>
> .
>
Re: [PATCH v9 00/23] drm/rockchip: RK356x VOP2 support
> Wiadomość napisana przez Sascha Hauer w dniu > 30.03.2022, o godz. 09:28: > >> >> You can easily reproduce with modetest utility: >> >> modetest -P 43@67:1920x1080@NV12 > > This only sets the overlay, but how did you get something on the screen > initially? > I'm not sure that above command only sets plane. On other SoCs i’m testing it gives expected results: diagonal colored stripes. There is single exception: rk356x with vop2 - where screen is green unless i „fix/enable” by playing with plane #69 > I did with "modetest -s 69@67:1920x1080 -d" and with this it works as > expected, I can't reproduce any green screen issue here. I see you are using plane #69. Why not #43? Is plane #43 working ok for you? I’m using plane #43 because: application (player) - at start - queries all planes and selects first plane offering format being within offered formats by provider (video decoder; NV12 from rk356x hantro video decoder). pls look on app log regarding planes discovery and election: https://pastebin.com/edAhbcvU Now - looking what VOP2 reports: https://pastebin.com/8ujkaV9n is see first plane accepting NV12 is #43 - so my app is electing this plane to use for displaying video. This strategy works well for all 13 platforms i’m supporting (only 13 i have in my testbed). If this approach is - by Yours VOP2 patches goal - is not supported - then OK. I understand this :-) But - if You want to support DRM features in the same way like other SOC are doing (and working well with KODI/MythTV/mpv/etc) - then i think: 1\ DRM plane #43 not supports NV12 - but code wrongly reports NV12 format is supported, or 2\ DRM plane #43 is supported - but code has bug resulting with green screen. Pls let me know what you think! > > I found another problem though which might or might not be related with > your issue. I saw that the overlay is not exactly centered as it ought > to be. This goes down to wrong delay settings for the overlay, the > following patch fixes this. > > Sascha > > -8<--- > > From f9a92401344e8aa3203fca2236dd4a40cc8690f6 Mon Sep 17 00:00:00 2001 > From: Sascha Hauer > Date: Wed, 30 Mar 2022 09:22:26 +0200 > Subject: [PATCH] fixup! drm: rockchip: Add VOP2 driver > > --- > drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > index 69e9870d5f2dc..7dba7b9b63dc6 100644 > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c > @@ -1979,10 +1979,10 @@ static void vop2_setup_dly_for_windows(struct vop2 > *vop2) > sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__ESMART1, dly); > break; > case ROCKCHIP_VOP2_SMART0: > - sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART1, dly); > + sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART0, dly); > break; > case ROCKCHIP_VOP2_SMART1: > - sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART0, dly); > + sdly |= FIELD_PREP(RK3568_SMART_DLY_NUM__SMART1, dly); > break; > } > } > -- > 2.30.2 > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: DSI Bridge switching
On Tue, Mar 29, 2022 at 07:39:21PM +0530, Jagan Teki wrote: > On Thu, Mar 10, 2022 at 4:05 PM Maxime Ripard wrote: > > > > On Wed, Mar 09, 2022 at 06:45:10PM -0600, Adam Ford wrote: > > > On Wed, Mar 9, 2022 at 1:11 PM Jagan Teki > > > wrote: > > > > > > > > or a Hi All, > > > > > > > > On Thu, Oct 14, 2021 at 6:45 PM Jagan Teki > > > > wrote: > > > > > > > > > > Hi Laurent, > > > > > > > > > > On Fri, Oct 8, 2021 at 7:07 PM Laurent Pinchart > > > > > wrote: > > > > > > > > > > > > Hello, > > > > > > > > > > > > On Fri, Oct 08, 2021 at 03:27:43PM +0200, Andrzej Hajda wrote: > > > > > > > Hi, > > > > > > > > > > > > > > Removed my invalid email (I will update files next week). > > > > > > > > > > > > > > On 08.10.2021 13:14, Jagan Teki wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > I think this seems to be a known use case for industrial these > > > > > > > > days with i.mx8m. > > > > > > > > > > > > > > > > The host DSI would configure with two bridges one for DSI to > > > > > > > > LVDS > > > > > > > > (SN65DSI83) and another for DSI to HDMI Out (ADV7535). > > > > > > > > Technically we > > > > > > > > can use only one bridge at a time as host DSI support single > > > > > > > > out port. > > > > > > > > So we can have two separate device tree files for LVDS and HDMI > > > > > > > > and > > > > > > > > load them static. > > > > > > > > > > > > > > > > But, one of the use cases is to support both of them in single > > > > > > > > dts, and > > > > > > > > - Turn On LVDS (default) > > > > > > > > - Turn Off LVDS then Turn On HDMI when cable plug-in > > > > > > > > > > > > > > Are you sure it will work from hardware PoV? Do you have some > > > > > > > demuxer? > > > > > > > isolation of pins? > > > > > > > > > > > > It may be in the category of "you shouldn't do this, but it actually > > > > > > works". I've seen the same being done with two CSI-2 camera sensors > > > > > > connected to the same receiver, with one of them being held in > > > > > > reset at > > > > > > all times. > > > > > > > > > > Yes. Here the design has 2 MIPI D-PHY switches. Each switch take 2 > > > > > input data lanes and 1 clock lane from SoC and produces 4 data lanes > > > > > and 2 clock lanes and from switch output 2 lanes and 1 clock are > > > > > inputting to HDMI bridge and other 2 lanes and 1 clock is inputting to > > > > > LVDS. So 1st pair of 1st switch and 1st pair of 2nd switch goes to > > > > > HDMI and 2nd pair of 1st switch and 2nd pair of 2nd switch does to > > > > > LVDS. > > > > > > > > > > However, routing of these lanes are controlled by SEL, OE GPIO pins. > > > > > So at a time we can access only single bridge. > > > > > > > > > > > > > > > > > > > The HDMI event can be detected via some HDMI-INT GPIO on-board > > > > > > > > design. > > > > > > > > > > > > > > > > The possible solution, I'm thinking of adding LVDS on port 1, > > > > > > > > HDMI on > > > > > > > > port 2 in the DSI host node, and trying to attach the respective > > > > > > > > bridge based on HDMI-INT like repeating the bridge attachment > > > > > > > > cycle > > > > > > > > based on the HDMI-INT. > > > > > > > > > > > > > > I think more appropriate would be to share the same port, but > > > > > > > provide > > > > > > > two endpoints inside this port - we have two hardware sharing the > > > > > > > same > > > > > > > physical port. > > > > > > > > > > > > That sounds like the correct DT description to me. > > > > > > > > > > > > > > Can it be possible to do bridge attachment at runtime? > > > > > > > > something like > > > > > > > > a bridge hotplug event? or any other possible solutions? > > > > > > > > > > > > > > > > Any suggestions? > > > > > > > > > > > > > > Practically it is possible, see exynos_dsi + panels, or > > > > > > > exynos_dsi + > > > > > > > some toshiba bridge - panel and bridge are dynamically 'plugged' > > > > > > > and > > > > > > > 'unplugged' from exynos_drm, but they do not use bridge chain for > > > > > > > this > > > > > > > and some other reasons. (un|re|)plugging should be performed of > > > > > > > course > > > > > > > when pipeline is off (connector disconnected). I am not sure about > > > > > > > bridges added to bridge chain - you need to inspect all opses to > > > > > > > ensure > > > > > > > it can be done safely. > > > > > > > > > > > > > > And the main issue: Daniel does not like it :) > > > > > > > > > > > > Neither do I :-) Could it be handled with two DRM connectors that > > > > > > are > > > > > > mutually exclusive ? > > > > > > > > > > How about adding lvds-connector, hdmi-connector on the pipeline and > > > > > select them based on the switch SEL GPIO? does it make sense to do > > > > > this implementation via display-connector.c > > > > > > > > I have somehow managed to make runtime switching possible between LVDS > > > > and HDMI with the help of DRM bridges. > > > > > > > > | => ADV7535=> > > > > HDMI-A Connect
Re: DRM Master ignoring hotplug event during display switching (QT)
On Tue, Mar 29, 2022 at 11:38:32PM +0530, Jagan Teki wrote: > Hi all, > > I have implemented runtime display switching in the MIPI switch design > where LVDS and HDMI bridges are selected with the help of runtime > GPIO. > > Initial discussion on the same can be found here, > https://www.spinics.net/lists/dri-devel/msg318524.html > > The implementation has been done by creating each connector at > runtime. The default boot will create the LVDS connector and based on > the HDMI plug-in the ISR. > > 1. forcing the LVDS to disconnect > 2. call drm_kms_helper_hotplug_event > 3. detach the bridge chain > 4. attach the new bridge chain (HDMI) > 5. call drm_kms_helper_hotplug_event > > do the reverse when we unplug the HDMI cable. > > So, the bridge chains are attached and detached based on GPIO > Interrupt which is indeed identified based on the physical HDMIA > connector. > > Pipeline for LVDS, > mxfsb => nwl-dsi => display-switch => sn65dsi83=> panel-bridge > > Pipeline for HDMI, > mxfsb => nwl-dsi => display-switch => adv7511 => display-connector > > With this, implementation and I can able switch the displays with > default DRM (without specific DRM applications) where the LVDS is ON > by default and when HDMI plug-in the LVDS OFF/HDMI ON, and when HDMI > unplug the HDMI OFF/LVDS ON. > > However, with QT5 I can see the DRM Master ignoring hotplug event by > returning 0 on drm_master_internal_acquire in > drm_fb_helper_hotplug_event. With this the hotplug returned early so > it cannot able to disconnect and connect the new switching connector. > > Any help? I'm not sure why you started another discussion with pretty much the same content, but you can't rely on userspace handling the hotplug event. You'll have to deal with the case where it just doesn't. Maxime signature.asc Description: PGP signature
Re: BUG: KASAN: use-after-free in drm_atomic_helper_wait_for_vblanks()
On Tue, Mar 15, 2022 at 12:53:30AM +0300, Dmitry Osipenko wrote: > On 3/11/22 17:22, Maxime Ripard wrote: > > On Thu, Mar 10, 2022 at 03:33:07AM +0300, Dmitry Osipenko wrote: > >> I was playing/testing SuperTuxKart using VirtIO-GPU driver and spotted a > >> UAF bug in drm_atomic_helper_wait_for_vblanks(). > >> > >> SuperTuxKart can use DRM directly, i.e. you can run game in VT without > >> Xorg or Wayland, this is where bugs happens. SuperTuxKart uses a > >> non-blocking atomic page flips and UAF happens when a new atomic state > >> is committed while there is a previous page flip still in-fly. > >> > >> What happens is that the new and old atomic states refer to the same > >> CRTC state somehow. Once the older atomic state is destroyed, the CRTC > >> state is freed and the newer atomic state continues to use the freed > >> CRTC state. > > > > I'm not sure what you mean by "the new and old atomic states refer to > > the same CRTC state", are those the same pointers? > > Yes, the pointers are the same. I'd assume that the newer atomic state > should duplicate CRTC state, but apparently it doesn't happen. Yeah, I don't think this is right either > >> The bug is easily reproducible (at least by me) by playing SuperTuxKart > >> for a minute. It presents on latest -next and 5.17-rc7, I haven't > >> checked older kernel versions. > >> > >> I'm not an expert of the non-blocking code paths in DRM, so asking for > >> suggestions about where the root of the problem could be. > > > > Does it occur with other platforms? Can you easily test on something else? > > Shouldn't be easy to replicate this on other platforms, but I'll try. By replicating I meant running SuperTuxKart on a platform with a different KMS driver than virtio-gpu. So any ARM SBC with a GPU will do for example. That will allow us to see if it's a bug in virtio-gpu or in the helpers/core. Maxime signature.asc Description: PGP signature
Re: [PATCH v13 3/6] dt-bindings: display: Add Loongson display controller
在2022年3月30日三月 上午4:46，Sui Jingfeng写道： > On 2022/3/29 21:27, Rob Herring wrote: >> On Sun, Mar 27, 2022 at 9:29 PM Sui Jingfeng <15330273...@189.cn> wrote: >>> Add DT bindings and simple usages for Loongson display controller >>> found in LS7A1000 bridge chip and LS2k1000 SoC. >>> >>> Signed-off-by: Sui Jingfeng <15330273...@189.cn> >>> --- >>> .../loongson/loongson,display-controller.yaml | 321 ++ >>> 1 file changed, 321 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml >>> >>> diff --git >>> a/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml >>> >>> b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml >>> new file mode 100644 >>> index ..34060ed55a25 >>> --- /dev/null >>> +++ >>> b/Documentation/devicetree/bindings/display/loongson/loongson,display-controller.yaml >>> @@ -0,0 +1,321 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: >>> http://devicetree.org/schemas/display/loongson/loongson,display-controller.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Loongson LS7A1000/LS2K1000/LS2K0500 Display Controller Device Tree >>> Bindings >>> + >>> +maintainers: >>> + - Sui Jingfeng >>> + >>> +description: |+ >>> + >>> + Loongson display controllers are simple which require scanout buffers >>> + to be physically contiguous. LS2K1000/LS2K0500 is a SOC, only system >>> + memory is available. LS7A1000/LS7A2000 is bridge chip which is equipped >>> + with a dedicated video RAM which is 64MB or more, precise size can be >>> + read from the PCI BAR 2 of the GPU device(0x0014:0x7A15) in the bridge >>> + chip. >>> + >>> + LSDC has two display pipes, each way has a DVO interface which provide >>> + RGB888 signals, vertical & horizontal synchronisations, data enable and >>> + the pixel clock. LSDC has two CRTC, each CRTC is able to scanout from >>> + 1920x1080 resolution at 60Hz. Each CRTC has two FB address registers. >>> + >>> + For LS7A1000, there are 4 dedicated GPIOs whose control register is >>> + located at the DC register space. They are used to emulate two way i2c, >>> + One for DVO0, another for DVO1. >>> + >>> + LS2K1000 and LS2K0500 SoC grab i2c adapter from other module, either >>> + general purpose GPIO emulated i2c or hardware i2c in the SoC. >>> + >>> + LSDC's display pipeline have several components as below description, >>> + >>> + The display controller in LS7A1000: >>> + ___ _ >>> +|---| | | >>> +| CRTC0 --> | DVO0 > Encoder0 ---> Connector0 ---> | Monitor | >>> +| _ _ ---|^ ^|_| >>> +| | | | |---|| | >>> +| |_| |_|| i2c0 <+-+ >>> +|---| >>> +| DC IN LS7A1000 | >>> +| _ _ ---| >>> +| | | | || i2c1 <+-+ >>> +| |_| |_|---|| | _ >>> +|---|| || | >>> +| CRTC1 --> | DVO1 > Encoder1 ---> Connector1 ---> | Panel | >>> +|---| |_| >>> +|___| >>> + >>> + Simple usage of LS7A1000 with LS3A4000 CPU: >>> + >>> ++--+++ >>> +| DDR4 || +---+ | >>> ++--+| | PCIe Root complex | LS7A1000 | >>> + || MC0 | +--++-+++ | >>> + +--+ HT 3.0 | || || | >>> + | LS3A4000 |<>| +---++---+ +--++--+ +-+ +--+ >>> + | CPU|<>| | GC1000 | | LSDC |<--->| DDR3 MC |<->| VRAM | >>> + +--+ | ++ +-+--+-+ +-+ +--+ >>> + || MC1 +---|--|-+ >>> ++--+| | >>> +| DDR4 | +---+ DVO0 | | DVO1 +--+ >>> ++--+ VGA <--|ADV7125|<+ +>|TFP410|--> DVI/HDMI >>> + +---+ +--+ >>> + >>> + The display controller in LS2K1000/LS2K0500: >>> + ___ _ >>> +|---| | | >>> +| CRTC0 --> | DVO0 > Encoder0 ---> Connector0 ---> | Monitor | >>> +| _ _ ---|^ ^ |_| >>> +| | | | | || | >>> +| |_| |_| | +--+ | >>> +| <>| i2c0 |<--
Re: [PATCH 2/8] drm: Rename dp/ to display/
On 3/22/22 20:27, Thomas Zimmermann wrote: > Rename dp/ to display/ to account for additional display-related > helpers, such as HDMI. Update all related include statements. No > functional changes. > > Various drivers, such as i915 and amdgpu, use similar naming scheme > by putting code for video-output standards into a local display/ > directory. The new directory's name is aligned with that policy. > It is really a policy or just a convention ? > Signed-off-by: Thomas Zimmermann > --- > Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH] drm: round_up the size to the alignment value
Round up the size value to the min_page_size and trim the last block to
the required size.
This solves a bug detected when size is not aligned with the min_page_size.
Unigine Heaven has allocation requests for example required pages are 257
and alignment request is 256. To allocate the left over 1 page, continues
the iteration to find the order value which is 0 and when it compares with
min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue
we round_up the size value to the min_page_size and trim the last block to
the computed required size value.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/drm_buddy.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..98d7ec359b08 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
unsigned int min_order, order;
unsigned long pages;
LIST_HEAD(allocated);
+ u64 cur_size;
int err;
if (size < mm->chunk_size)
@@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
if (start + size == end)
return __drm_buddy_alloc_range(mm, start, size, blocks);
+ cur_size = size;
+
+ if (!IS_ALIGNED(size, min_page_size))
+ size = round_up(size, min_page_size);
+
pages = size >> ilog2(mm->chunk_size);
order = fls(pages) - 1;
min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
@@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
break;
} while (1);
+
+ /*
+* If size value rounded up to min_page_size, trim the last block
+* to the required size
+*/
+ if (cur_size != size) {
+ struct drm_buddy_block *trim_block;
+ LIST_HEAD(trim_list);
+ u64 required_size;
+
+ trim_block = list_last_entry(&allocated, typeof(*trim_block),
link);
+ list_move_tail(&trim_block->link, &trim_list);
+ /*
+* Compute the required_size value by subtracting the last
block size
+* with (aligned size - original size)
+*/
+ required_size = drm_buddy_block_size(mm, trim_block) - (size -
cur_size);
+
+ drm_buddy_block_trim(mm,
+required_size,
+&trim_list);
+
+ list_splice_tail(&trim_list, &allocated);
+ }
+
list_splice_tail(&allocated, blocks);
return 0;
base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b
--
2.25.1
Re: [PATCH v2] drm: add a check to verify the size alignment
On 29/03/22 4:54 pm, Matthew Auld wrote: > On Tue, 29 Mar 2022 at 12:17, Arunpravin Paneer Selvam > wrote: >> >> >> >> On 23/03/22 1:15 pm, Christian König wrote: >>> Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam: Add a simple check to reject any size not aligned to the min_page_size. handle instances when size is not aligned with the min_page_size. Unigine Heaven has allocation requests for example required pages are 257 and alignment request is 256. To allocate the left over 1 page, continues the iteration to find the order value which is 0 and when it compares with min_order = 8, triggers the BUG_ON(order < min_order). To avoid this problem, we added a simple check to return -EINVAL if size is not aligned to the min_page_size. v2: Added more details to the commit description Signed-off-by: Arunpravin Paneer Selvam Suggested-by: Matthew Auld --- drivers/gpu/drm/drm_buddy.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 72f52f293249..b503c88786b0 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, if (range_overflows(start, size, mm->size)) return -EINVAL; +if (WARN_ON(!IS_ALIGNED(size, min_page_size))) +return -EINVAL; + >>> >>> I'm not that happy with the handling here. >>> >>> See a minimum page size larger than the requested size is perfectly >>> valid, it just means that the remaining pages needs to be trimmed. >>> >>> In other words when the request is to allocate 1 page with an alignment >>> of 256 we just need to give the remaining 255 pages back to the allocator. >> >> In one of the previous mail Matthew explained that i915 expects to >> return -EINVAL error code if size is not aligned to min_page_size. > > We could also move the WARN_ON() into i915 as a separate patch, and > just change the default buddy behaviour to transparently handle the > rounding + trim, if you prefer. I don't have a strong opinion. ok, I sent a patch handling rounding + trim in drm_buddy_alloc_blocks() function. Please review the patch. > >> >> can we just modify in amdgpu code where we round_up the size to the >> min_page_size value and keep this error handling in drm_buddy.c? >>> >>> Regards, >>> Christian. >>> /* Actual range allocation */ if (start + size == end) return __drm_buddy_alloc_range(mm, start, size, blocks); base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b >>>
Re: [PATCH] drm: round_up the size to the alignment value
Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam:
Round up the size value to the min_page_size and trim the last block to
the required size.
This solves a bug detected when size is not aligned with the min_page_size.
Unigine Heaven has allocation requests for example required pages are 257
and alignment request is 256. To allocate the left over 1 page, continues
the iteration to find the order value which is 0 and when it compares with
min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue
we round_up the size value to the min_page_size and trim the last block to
the computed required size value.
Well, Matthew and you convinced me to *not* do it like this.
Has that conclusion changed somehow?
Regards,
Christian.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/drm_buddy.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..98d7ec359b08 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
unsigned int min_order, order;
unsigned long pages;
LIST_HEAD(allocated);
+ u64 cur_size;
int err;
if (size < mm->chunk_size)
@@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
if (start + size == end)
return __drm_buddy_alloc_range(mm, start, size, blocks);
+ cur_size = size;
+
+ if (!IS_ALIGNED(size, min_page_size))
+ size = round_up(size, min_page_size);
+
pages = size >> ilog2(mm->chunk_size);
order = fls(pages) - 1;
min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
@@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
break;
} while (1);
+
+ /*
+* If size value rounded up to min_page_size, trim the last block
+* to the required size
+*/
+ if (cur_size != size) {
+ struct drm_buddy_block *trim_block;
+ LIST_HEAD(trim_list);
+ u64 required_size;
+
+ trim_block = list_last_entry(&allocated, typeof(*trim_block),
link);
+ list_move_tail(&trim_block->link, &trim_list);
+ /*
+* Compute the required_size value by subtracting the last
block size
+* with (aligned size - original size)
+*/
+ required_size = drm_buddy_block_size(mm, trim_block) - (size -
cur_size);
+
+ drm_buddy_block_trim(mm,
+required_size,
+&trim_list);
+
+ list_splice_tail(&trim_list, &allocated);
+ }
+
list_splice_tail(&allocated, blocks);
return 0;
base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b
Re: [PATCH] drm: round_up the size to the alignment value
On 30/03/22 2:37 pm, Christian König wrote:
> Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam:
>> Round up the size value to the min_page_size and trim the last block to
>> the required size.
>>
>> This solves a bug detected when size is not aligned with the min_page_size.
>> Unigine Heaven has allocation requests for example required pages are 257
>> and alignment request is 256. To allocate the left over 1 page, continues
>> the iteration to find the order value which is 0 and when it compares with
>> min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue
>> we round_up the size value to the min_page_size and trim the last block to
>> the computed required size value.
>
> Well, Matthew and you convinced me to *not* do it like this.
>
> Has that conclusion changed somehow?
>
Yes, now he is ok to handle rounding + trimming in drm buddy
> Regards,
> Christian.
>
>>
>> Signed-off-by: Arunpravin Paneer Selvam
>> ---
>> drivers/gpu/drm/drm_buddy.c | 31 +++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
>> index 72f52f293249..98d7ec359b08 100644
>> --- a/drivers/gpu/drm/drm_buddy.c
>> +++ b/drivers/gpu/drm/drm_buddy.c
>> @@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>> unsigned int min_order, order;
>> unsigned long pages;
>> LIST_HEAD(allocated);
>> +u64 cur_size;
>> int err;
>>
>> if (size < mm->chunk_size)
>> @@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>> if (start + size == end)
>> return __drm_buddy_alloc_range(mm, start, size, blocks);
>>
>> +cur_size = size;
>> +
>> +if (!IS_ALIGNED(size, min_page_size))
>> +size = round_up(size, min_page_size);
>> +
>> pages = size >> ilog2(mm->chunk_size);
>> order = fls(pages) - 1;
>> min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
>> @@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
>> break;
>> } while (1);
>>
>> +
>> +/*
>> + * If size value rounded up to min_page_size, trim the last block
>> + * to the required size
>> + */
>> +if (cur_size != size) {
>> +struct drm_buddy_block *trim_block;
>> +LIST_HEAD(trim_list);
>> +u64 required_size;
>> +
>> +trim_block = list_last_entry(&allocated, typeof(*trim_block),
>> link);
>> +list_move_tail(&trim_block->link, &trim_list);
>> +/*
>> + * Compute the required_size value by subtracting the last
>> block size
>> + * with (aligned size - original size)
>> + */
>> +required_size = drm_buddy_block_size(mm, trim_block) - (size -
>> cur_size);
>> +
>> +drm_buddy_block_trim(mm,
>> + required_size,
>> + &trim_list);
>> +
>> +list_splice_tail(&trim_list, &allocated);
>> +}
>> +
>> list_splice_tail(&allocated, blocks);
>> return 0;
>>
>>
>> base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b
>
Re: DRM Master ignoring hotplug event during display switching (QT)
On Wed, Mar 30, 2022 at 2:22 PM Maxime Ripard wrote: > > On Tue, Mar 29, 2022 at 11:38:32PM +0530, Jagan Teki wrote: > > Hi all, > > > > I have implemented runtime display switching in the MIPI switch design > > where LVDS and HDMI bridges are selected with the help of runtime > > GPIO. > > > > Initial discussion on the same can be found here, > > https://www.spinics.net/lists/dri-devel/msg318524.html > > > > The implementation has been done by creating each connector at > > runtime. The default boot will create the LVDS connector and based on > > the HDMI plug-in the ISR. > > > > 1. forcing the LVDS to disconnect > > 2. call drm_kms_helper_hotplug_event > > 3. detach the bridge chain > > 4. attach the new bridge chain (HDMI) > > 5. call drm_kms_helper_hotplug_event > > > > do the reverse when we unplug the HDMI cable. > > > > So, the bridge chains are attached and detached based on GPIO > > Interrupt which is indeed identified based on the physical HDMIA > > connector. > > > > Pipeline for LVDS, > > mxfsb => nwl-dsi => display-switch => sn65dsi83=> panel-bridge > > > > Pipeline for HDMI, > > mxfsb => nwl-dsi => display-switch => adv7511 => display-connector > > > > With this, implementation and I can able switch the displays with > > default DRM (without specific DRM applications) where the LVDS is ON > > by default and when HDMI plug-in the LVDS OFF/HDMI ON, and when HDMI > > unplug the HDMI OFF/LVDS ON. > > > > However, with QT5 I can see the DRM Master ignoring hotplug event by > > returning 0 on drm_master_internal_acquire in > > drm_fb_helper_hotplug_event. With this the hotplug returned early so > > it cannot able to disconnect and connect the new switching connector. > > > > Any help? > > I'm not sure why you started another discussion with pretty much the > same content, but you can't rely on userspace handling the hotplug > event. You'll have to deal with the case where it just doesn't. I've added clear details here - thought that it reaches more people. Okay, I will continue on the old thread. Jagan.
Re: [PATCH] drm: round_up the size to the alignment value
Am 30.03.22 um 11:20 schrieb Arunpravin Paneer Selvam:
On 30/03/22 2:37 pm, Christian König wrote:
Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam:
Round up the size value to the min_page_size and trim the last block to
the required size.
This solves a bug detected when size is not aligned with the min_page_size.
Unigine Heaven has allocation requests for example required pages are 257
and alignment request is 256. To allocate the left over 1 page, continues
the iteration to find the order value which is 0 and when it compares with
min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue
we round_up the size value to the min_page_size and trim the last block to
the computed required size value.
Well, Matthew and you convinced me to *not* do it like this.
Has that conclusion changed somehow?
Yes, now he is ok to handle rounding + trimming in drm buddy
Yeah, but I'm no longer :)
How do we then handle the detection of contiguous allocation?
As I said we can do that like:
1. alloc
2. check if we only have a single node
3. trim
But if we include the trim here we can't do it any more.
Only alternative would then be to inspect each node and see if it
follows directly behind the predecessor.
Regards,
Christian.
Regards,
Christian.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/drm_buddy.c | 31 +++
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..98d7ec359b08 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
unsigned int min_order, order;
unsigned long pages;
LIST_HEAD(allocated);
+ u64 cur_size;
int err;
if (size < mm->chunk_size)
@@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
if (start + size == end)
return __drm_buddy_alloc_range(mm, start, size, blocks);
+ cur_size = size;
+
+ if (!IS_ALIGNED(size, min_page_size))
+ size = round_up(size, min_page_size);
+
pages = size >> ilog2(mm->chunk_size);
order = fls(pages) - 1;
min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
@@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
break;
} while (1);
+
+ /*
+* If size value rounded up to min_page_size, trim the last block
+* to the required size
+*/
+ if (cur_size != size) {
+ struct drm_buddy_block *trim_block;
+ LIST_HEAD(trim_list);
+ u64 required_size;
+
+ trim_block = list_last_entry(&allocated, typeof(*trim_block),
link);
+ list_move_tail(&trim_block->link, &trim_list);
+ /*
+* Compute the required_size value by subtracting the last
block size
+* with (aligned size - original size)
+*/
+ required_size = drm_buddy_block_size(mm, trim_block) - (size -
cur_size);
+
+ drm_buddy_block_trim(mm,
+required_size,
+&trim_list);
+
+ list_splice_tail(&trim_list, &allocated);
+ }
+
list_splice_tail(&allocated, blocks);
return 0;
base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b
Re: [PATCH v2 00/12] iio: buffer-dma: write() and new DMABUF based API
On Tue, Mar 29, 2022 at 06:16:56PM +0100, Paul Cercueil wrote: > Hi Daniel, > > Le mar., mars 29 2022 at 16:10:44 +0200, Daniel Vetter a > écrit : > > On Tue, Mar 29, 2022 at 10:11:14AM +0100, Paul Cercueil wrote: > > > Hi Daniel, > > > > > > Le mar., mars 29 2022 at 10:33:32 +0200, Daniel Vetter > > > a > > > écrit : > > > > On Tue, Feb 15, 2022 at 05:43:35PM +, Paul Cercueil wrote: > > > > > Hi Jonathan, > > > > > > > > > > Le dim., févr. 13 2022 at 18:46:16 +, Jonathan Cameron > > > > > a écrit : > > > > > > On Mon, 7 Feb 2022 12:59:21 + > > > > > > Paul Cercueil wrote: > > > > > > > > > > > > > Hi Jonathan, > > > > > > > > > > > > > > This is the V2 of my patchset that introduces a new > > > userspace > > > > > > > interface > > > > > > > based on DMABUF objects to complement the fileio API, and > > > adds > > > > > > > write() > > > > > > > support to the existing fileio API. > > > > > > > > > > > > Hi Paul, > > > > > > > > > > > > It's been a little while. Perhaps you could summarize the > > > various > > > > > view > > > > > > points around the appropriateness of using DMABUF for this? > > > > > > I appreciate it is a tricky topic to distil into a brief > > > summary > > > > > but > > > > > > I know I would find it useful even if no one else does! > > > > > > > > > > So we want to have a high-speed interface where buffers of > > > samples > > > > > are > > > > > passed around between IIO devices and other devices (e.g. USB > > > or > > > > > network), > > > > > or made available to userspace without copying the data. > > > > > > > > > > DMABUF is, at least in theory, exactly what we need. Quoting > > > the > > > > > documentation > > > > > > > > (https://www.kernel.org/doc/html/v5.15/driver-api/dma-buf.html): > > > > > "The dma-buf subsystem provides the framework for sharing > > > buffers > > > > > for > > > > > hardware (DMA) access across multiple device drivers and > > > > > subsystems, and for > > > > > synchronizing asynchronous hardware access. This is used, for > > > > > example, by > > > > > drm “prime” multi-GPU support, but is of course not limited to > > > GPU > > > > > use > > > > > cases." > > > > > > > > > > The problem is that right now DMABUF is only really used by > > > DRM, > > > > > and to > > > > > quote Daniel, "dma-buf looks like something super generic and > > > > > useful, until > > > > > you realize that there's a metric ton of gpu/accelerator bagage > > > > > piled in". > > > > > > > > > > Still, it seems to be the only viable option. We could add a > > > custom > > > > > buffer-passing interface, but that would mean implementing the > > > same > > > > > buffer-passing interface on the network and USB stacks, and > > > before > > > > > we know > > > > > it we re-invented DMABUFs. > > > > > > > > dma-buf also doesn't support sharing with network and usb stacks, > > > so I'm > > > > a > > > > bit confused why exactly this is useful? > > > > > > There is an attempt to get dma-buf support in the network stack, > > > called > > > "zctap". Last patchset was sent last november. USB stack does not > > > support > > > dma-buf, but we can add it later I guess. > > > > > > > So yeah unless there's some sharing going on with gpu stuff (for > > > data > > > > processing maybe) I'm not sure this makes a lot of sense really. > > > Or at > > > > least some zero-copy sharing between drivers, but even that would > > > > minimally require a dma-buf import ioctl of some sorts. Which I > > > either > > > > missed or doesn't exist. > > > > > > We do want zero-copy between drivers, the network stack, and the > > > USB stack. > > > It's not just about having a userspace interface. > > > > I think in that case we need these other pieces too. And we need acks > > from > > relevant subsystems that these other pieces are a) ready for upstream > > merging and also that the dma-buf side of things actually makes sense. > > Ok... > > > > > If there's none of that then just hand-roll your buffer handling > > > code > > > > (xarray is cheap to use in terms of code for this), you can > > > always add > > > > dma-buf import/export later on when the need arises. > > > > > > > > Scrolling through patches you only have dma-buf export, but no > > > > importing, > > > > so the use-case that works is with one of the existing subsystems > > > that > > > > supporting dma-buf importing. > > > > > > > > I think minimally we need the use-case (in form of code) that > > > needs the > > > > buffer sharing here. > > > > > > I'll try with zctap and report back. > > > > Do you have a link for this? I just checked dri-devel on lore, and it's > > not there. Nor anywhere else. > > The code is here: https://github.com/jlemon/zctap_kernel > > I know Jonathan Lemon (Cc'd) was working on upstreaming it, I saw a few > patchsets. Yeah if the goal here is to zero-copy from iio to
Re: [PATCH v2 12/12] Documentation: iio: Document high-speed DMABUF based API
On Tue, Mar 29, 2022 at 06:34:58PM +0100, Paul Cercueil wrote: > > > Le mar., mars 29 2022 at 16:07:21 +0200, Daniel Vetter a > écrit : > > On Tue, Mar 29, 2022 at 10:47:23AM +0100, Paul Cercueil wrote: > > > Hi Daniel, > > > > > > Le mar., mars 29 2022 at 10:54:43 +0200, Daniel Vetter > > > a > > > écrit : > > > > On Mon, Feb 07, 2022 at 01:01:40PM +, Paul Cercueil wrote: > > > > > Document the new DMABUF based API. > > > > > > > > > > v2: - Explicitly state that the new interface is optional and > > > is > > > > >not implemented by all drivers. > > > > > - The IOCTLs can now only be called on the buffer FD > > > returned by > > > > >IIO_BUFFER_GET_FD_IOCTL. > > > > > - Move the page up a bit in the index since it is core > > > stuff > > > > > and not > > > > >driver-specific. > > > > > > > > > > Signed-off-by: Paul Cercueil > > > > > --- > > > > > Documentation/driver-api/dma-buf.rst | 2 + > > > > > Documentation/iio/dmabuf_api.rst | 94 > > > > > > > > > > Documentation/iio/index.rst | 2 + > > > > > 3 files changed, 98 insertions(+) > > > > > create mode 100644 Documentation/iio/dmabuf_api.rst > > > > > > > > > > diff --git a/Documentation/driver-api/dma-buf.rst > > > > > b/Documentation/driver-api/dma-buf.rst > > > > > index 2cd7db82d9fe..d3c9b58d2706 100644 > > > > > --- a/Documentation/driver-api/dma-buf.rst > > > > > +++ b/Documentation/driver-api/dma-buf.rst > > > > > @@ -1,3 +1,5 @@ > > > > > +.. _dma-buf: > > > > > + > > > > > Buffer Sharing and Synchronization > > > > > == > > > > > > > > > > diff --git a/Documentation/iio/dmabuf_api.rst > > > > > b/Documentation/iio/dmabuf_api.rst > > > > > new file mode 100644 > > > > > index ..43bb2c1b9fdc > > > > > --- /dev/null > > > > > +++ b/Documentation/iio/dmabuf_api.rst > > > > > @@ -0,0 +1,94 @@ > > > > > +=== > > > > > +High-speed DMABUF interface for IIO > > > > > +=== > > > > > + > > > > > +1. Overview > > > > > +=== > > > > > + > > > > > +The Industrial I/O subsystem supports access to buffers > > > through a > > > > > file-based > > > > > +interface, with read() and write() access calls through the > > > IIO > > > > > device's dev > > > > > +node. > > > > > + > > > > > +It additionally supports a DMABUF based interface, where the > > > > > userspace > > > > > +application can allocate and append DMABUF objects to the > > > buffer's > > > > > queue. > > > > > +This interface is however optional and is not available in all > > > > > drivers. > > > > > + > > > > > +The advantage of this DMABUF based interface vs. the read() > > > > > +interface, is that it avoids an extra copy of the data > > > between the > > > > > +kernel and userspace. This is particularly useful for > > > high-speed > > > > > +devices which produce several megabytes or even gigabytes of > > > data > > > > > per > > > > > +second. > > > > > + > > > > > +The data in this DMABUF interface is managed at the > > > granularity of > > > > > +DMABUF objects. Reducing the granularity from byte level to > > > block > > > > > level > > > > > +is done to reduce the userspace-kernelspace synchronization > > > > > overhead > > > > > +since performing syscalls for each byte at a few Mbps is just > > > not > > > > > +feasible. > > > > > + > > > > > +This of course leads to a slightly increased latency. For this > > > > > reason an > > > > > +application can choose the size of the DMABUFs as well as how > > > many > > > > > it > > > > > +allocates. E.g. two DMABUFs would be a traditional double > > > buffering > > > > > +scheme. But using a higher number might be necessary to avoid > > > > > +underflow/overflow situations in the presence of scheduling > > > > > latencies. > > > > > > > > So this reads a lot like reinventing io-uring with pre-registered > > > > O_DIRECT > > > > memory ranges. Except it's using dma-buf and hand-rolling a lot of > > > > pieces > > > > instead of io-uring and O_DIRECT. > > > > > > I don't see how io_uring would help us. It's an async I/O > > > framework, does it > > > allow us to access a kernel buffer without copying the data? Does > > > it allow > > > us to zero-copy the data to a network interface? > > > > With networking, do you mean rdma, or some other kind of networking? > > Anything else than rdma doesn't support dma-buf, and I don't think it > > will > > likely ever do so. Similar it's really tricky to glue dma-buf support > > into > > the block layer. > > By networking I mean standard sockets. If I'm not mistaken, Jonathan Lemon's > work on zctap was to add dma-buf import/export support to standard sockets. > > > Wrt io_uring, yes it's async, but that's not the point. The point is > > that > > with i
Re: [PATCH 3/8] drm/display: Introduce a DRM display-helper module
On 3/22/22 20:27, Thomas Zimmermann wrote: > Replace the DP-helper module with a display-helper module. Update > all related Kconfig and Makefile rules. > > Besides the existing code for DisplayPort, the new module will > contain helpers for other video-output standards, such as HDMI. > Drivers will still be able to select the required video-output > helpers. Linking all such code into a single module avoids the > proliferation of small kernel modules. > > Signed-off-by: Thomas Zimmermann > --- [snip] > +config DRM_DISPLAY_HELPER > + tristate > + depends on DRM > + help > + DRM helpers for display adapters. > + > config DRM_DP_HELPER > tristate > depends on DRM > + select DRM_DISPLAY_HELPER > help > DRM helpers for DisplayPort. > I was about to ask why this would still be needed but then re-read the commit message that says drivers will still be able to select required video-output helpers. That makes sense since the fact that all helpers will be in the same module would be transparent to drivers. > diff --git a/drivers/gpu/drm/display/Makefile > b/drivers/gpu/drm/display/Makefile > index 75faffc706b1..90f12e9b4735 100644 > --- a/drivers/gpu/drm/display/Makefile > +++ b/drivers/gpu/drm/display/Makefile > @@ -2,8 +2,9 @@ > > obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o > > -drm_dp_helper-y := drm_dp.o drm_dp_dual_mode_helper.o drm_dp_helper_mod.o > drm_dp_mst_topology.o > -drm_dp_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > -drm_dp_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o > +drm_display_helper-y := drm_display_helper_mod.o > +drm_display_helper-$(CONFIG_DRM_DP_HELPER) := drm_dp_helper.o > drm_dp_dual_mode_helper.o drm_dp_mst_topology.o > +drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > +drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o > > -obj-$(CONFIG_DRM_DP_HELPER) += drm_dp_helper.o > +obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o The drm_dp_helper.ko module has some parameters and this change will break existing kernel cmdline that are using it: $ modinfo drivers/gpu/drm/dp/drm_dp_helper.ko | grep parm | cut -d : -f2 drm_dp_cec_unregister_delay dp_aux_i2c_speed_khz dp_aux_i2c_transfer_size I don't know whether those are considered a kernel ABI or not though, and some already changed when the DP helpers were moved from drm_kms_helper.ko -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 2/8] drm: Rename dp/ to display/
Hi Am 30.03.22 um 11:04 schrieb Javier Martinez Canillas: On 3/22/22 20:27, Thomas Zimmermann wrote: Rename dp/ to display/ to account for additional display-related helpers, such as HDMI. Update all related include statements. No functional changes. Various drivers, such as i915 and amdgpu, use similar naming scheme by putting code for video-output standards into a local display/ directory. The new directory's name is aligned with that policy. It is really a policy or just a convention ? Convention, I think. I'll update the wording. Best regards Thomas Signed-off-by: Thomas Zimmermann --- Reviewed-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: DSI Bridge switching
On Wed, Mar 30, 2022 at 2:20 PM Maxime Ripard wrote: > > On Tue, Mar 29, 2022 at 07:39:21PM +0530, Jagan Teki wrote: > > On Thu, Mar 10, 2022 at 4:05 PM Maxime Ripard wrote: > > > > > > On Wed, Mar 09, 2022 at 06:45:10PM -0600, Adam Ford wrote: > > > > On Wed, Mar 9, 2022 at 1:11 PM Jagan Teki > > > > wrote: > > > > > > > > > > or a Hi All, > > > > > > > > > > On Thu, Oct 14, 2021 at 6:45 PM Jagan Teki > > > > > wrote: > > > > > > > > > > > > Hi Laurent, > > > > > > > > > > > > On Fri, Oct 8, 2021 at 7:07 PM Laurent Pinchart > > > > > > wrote: > > > > > > > > > > > > > > Hello, > > > > > > > > > > > > > > On Fri, Oct 08, 2021 at 03:27:43PM +0200, Andrzej Hajda wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > Removed my invalid email (I will update files next week). > > > > > > > > > > > > > > > > On 08.10.2021 13:14, Jagan Teki wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > I think this seems to be a known use case for industrial > > > > > > > > > these days with i.mx8m. > > > > > > > > > > > > > > > > > > The host DSI would configure with two bridges one for DSI to > > > > > > > > > LVDS > > > > > > > > > (SN65DSI83) and another for DSI to HDMI Out (ADV7535). > > > > > > > > > Technically we > > > > > > > > > can use only one bridge at a time as host DSI support single > > > > > > > > > out port. > > > > > > > > > So we can have two separate device tree files for LVDS and > > > > > > > > > HDMI and > > > > > > > > > load them static. > > > > > > > > > > > > > > > > > > But, one of the use cases is to support both of them in > > > > > > > > > single dts, and > > > > > > > > > - Turn On LVDS (default) > > > > > > > > > - Turn Off LVDS then Turn On HDMI when cable plug-in > > > > > > > > > > > > > > > > Are you sure it will work from hardware PoV? Do you have some > > > > > > > > demuxer? > > > > > > > > isolation of pins? > > > > > > > > > > > > > > It may be in the category of "you shouldn't do this, but it > > > > > > > actually > > > > > > > works". I've seen the same being done with two CSI-2 camera > > > > > > > sensors > > > > > > > connected to the same receiver, with one of them being held in > > > > > > > reset at > > > > > > > all times. > > > > > > > > > > > > Yes. Here the design has 2 MIPI D-PHY switches. Each switch take 2 > > > > > > input data lanes and 1 clock lane from SoC and produces 4 data lanes > > > > > > and 2 clock lanes and from switch output 2 lanes and 1 clock are > > > > > > inputting to HDMI bridge and other 2 lanes and 1 clock is inputting > > > > > > to > > > > > > LVDS. So 1st pair of 1st switch and 1st pair of 2nd switch goes to > > > > > > HDMI and 2nd pair of 1st switch and 2nd pair of 2nd switch does to > > > > > > LVDS. > > > > > > > > > > > > However, routing of these lanes are controlled by SEL, OE GPIO pins. > > > > > > So at a time we can access only single bridge. > > > > > > > > > > > > > > > > > > > > > > The HDMI event can be detected via some HDMI-INT GPIO > > > > > > > > > on-board design. > > > > > > > > > > > > > > > > > > The possible solution, I'm thinking of adding LVDS on port 1, > > > > > > > > > HDMI on > > > > > > > > > port 2 in the DSI host node, and trying to attach the > > > > > > > > > respective > > > > > > > > > bridge based on HDMI-INT like repeating the bridge attachment > > > > > > > > > cycle > > > > > > > > > based on the HDMI-INT. > > > > > > > > > > > > > > > > I think more appropriate would be to share the same port, but > > > > > > > > provide > > > > > > > > two endpoints inside this port - we have two hardware sharing > > > > > > > > the same > > > > > > > > physical port. > > > > > > > > > > > > > > That sounds like the correct DT description to me. > > > > > > > > > > > > > > > > Can it be possible to do bridge attachment at runtime? > > > > > > > > > something like > > > > > > > > > a bridge hotplug event? or any other possible solutions? > > > > > > > > > > > > > > > > > > Any suggestions? > > > > > > > > > > > > > > > > Practically it is possible, see exynos_dsi + panels, or > > > > > > > > exynos_dsi + > > > > > > > > some toshiba bridge - panel and bridge are dynamically > > > > > > > > 'plugged' and > > > > > > > > 'unplugged' from exynos_drm, but they do not use bridge chain > > > > > > > > for this > > > > > > > > and some other reasons. (un|re|)plugging should be performed of > > > > > > > > course > > > > > > > > when pipeline is off (connector disconnected). I am not sure > > > > > > > > about > > > > > > > > bridges added to bridge chain - you need to inspect all opses > > > > > > > > to ensure > > > > > > > > it can be done safely. > > > > > > > > > > > > > > > > And the main issue: Daniel does not like it :) > > > > > > > > > > > > > > Neither do I :-) Could it be handled with two DRM connectors that > > > > > > > are > > > > > > > mutually exclusive ? > > > > > > > > > > > > How about adding lvds-connector, hdmi-connector on the pipeline and > > > > > > sel
Re: BUG: KASAN: use-after-free in drm_atomic_helper_wait_for_vblanks()
On Tue, Mar 15, 2022 at 12:53:30AM +0300, Dmitry Osipenko wrote: > On 3/11/22 17:22, Maxime Ripard wrote: > > Hi Dmitry, > > > > On Thu, Mar 10, 2022 at 03:33:07AM +0300, Dmitry Osipenko wrote: > >> I was playing/testing SuperTuxKart using VirtIO-GPU driver and spotted a > >> UAF bug in drm_atomic_helper_wait_for_vblanks(). > >> > >> SuperTuxKart can use DRM directly, i.e. you can run game in VT without > >> Xorg or Wayland, this is where bugs happens. SuperTuxKart uses a > >> non-blocking atomic page flips and UAF happens when a new atomic state > >> is committed while there is a previous page flip still in-fly. > >> > >> What happens is that the new and old atomic states refer to the same > >> CRTC state somehow. Once the older atomic state is destroyed, the CRTC > >> state is freed and the newer atomic state continues to use the freed > >> CRTC state. > > > > I'm not sure what you mean by "the new and old atomic states refer to > > the same CRTC state", are those the same pointers? > > Yes, the pointers are the same. I'd assume that the newer atomic state > should duplicate CRTC state, but apparently it doesn't happen. The legacy cursor hack stuff does this, and it pretty fundamentally breaks everything. Might be good to retest with that disabled: https://lore.kernel.org/dri-devel/20201023123925.2374863-1-daniel.vet...@ffwll.ch/ The problem is a bit that this might cause some regressions, for drivers which don't yet have the fancy new cursor fastpath for plane updates. -Daniel > >> The bug is easily reproducible (at least by me) by playing SuperTuxKart > >> for a minute. It presents on latest -next and 5.17-rc7, I haven't > >> checked older kernel versions. > >> > >> I'm not an expert of the non-blocking code paths in DRM, so asking for > >> suggestions about where the root of the problem could be. > > > > Does it occur with other platforms? Can you easily test on something else? > > Shouldn't be easy to replicate this on other platforms, but I'll try. -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] dt-bindings: display: bridge: Drop requirement on input port for DSI devices
On Thu, Mar 24, 2022 at 04:50:04PM +0200, Laurent Pinchart wrote: > Hi Maxime, > > On Thu, Mar 24, 2022 at 03:23:24PM +0100, Maxime Ripard wrote: > > On Thu, Mar 24, 2022 at 03:43:42PM +0200, Laurent Pinchart wrote: > > > On Thu, Mar 24, 2022 at 09:18:19AM +0100, Maxime Ripard wrote: > > > > On Wed, Mar 23, 2022 at 10:38:19PM +0200, Laurent Pinchart wrote: > > > > > Hi Maxime, > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > Thank you for the patch. > > > > > > > > > > On Wed, Mar 23, 2022 at 04:48:23PM +0100, Maxime Ripard wrote: > > > > > > MIPI-DSI devices, if they are controlled through the bus itself, > > > > > > have to > > > > > > be described as a child node of the controller they are attached to. > > > > > > > > > > > > Thus, there's no requirement on the controller having an OF-Graph > > > > > > output > > > > > > port to model the data stream: it's assumed that it would go from > > > > > > the > > > > > > parent to the child. > > > > > > > > > > > > However, some bridges controlled through the DSI bus still require > > > > > > an > > > > > > input OF-Graph port, thus requiring a controller with an OF-Graph > > > > > > output > > > > > > port. This prevents those bridges from being used with the > > > > > > controllers > > > > > > that do not have one without any particular reason to. > > > > > > > > > > > > Let's drop that requirement. > > > > > > > > > > I'm sure this won't come as a surprise, I'm very much opposed to this > > > > > change, for two reasons. > > > > > > > > > > First, ports are part of the hardware, even if they're not connected. > > > > > It > > > > > thus simplifies handling in drivers if they're always present. > > > > > > > > > > Then, and that's the most important reason, I think it's a mistake not > > > > > to model the DSI data connection using OF graph unconditionally, even > > > > > when the DSI sink device is also controlled through the DSI bus (using > > > > > DCS) and is in that case a child of the DSI source device in the DT > > > > > hierarchy. > > > > > > > > That's the way we do for any other device though. You never addressed > > > > that comment, but it's very much the same that occurs for i2c or spi > > > > controllers and their device. They all get their data from the parent > > > > bus. I don't see you advocate for using OF-Graph for those devices. > > > > > > Those are different, there's no data stream independent of the control > > > communications. > > > > Fine, then you have Ethernet PHYs, or any MMIO device that does DMA. > > > > > > > The device tree describes a control hierarchy between devices. OF > > > > > graph > > > > > overlays on top of that a data transfer graph. The two are different > > > > > concepts, and the fact that DSI can sometimes be used as a control bus > > > > > doesn't change the concept. Using OF graph unconditionally to describe > > > > > the data connections for DSI leads to less variation in the device > > > > > tree > > > > > structure, and thus less complexity in the implementation. We're > > > > > suffering from the fact we haven't made it a requirement in the first > > > > > place, which can't be fixed due to ABI breakage constraints, but let's > > > > > not acknowledge it as a good idea. > > > > > > > > Honestly, it doesn't matter one bit. > > > > > > > > We have a huge discrepancy here today, and only a couple of bridges have > > > > that arbitrary restriction. The situation you don't want to acknowledge > > > > is the de-facto standard, by the generic binding and by what all the > > > > bridges and panels are implementing. Even panel-simple-dsi is doing it. > > > > So it's very much there already. > > > > > > It's here, and I think we should move away from it for new DSI sinks. > > > I'd like OF graph to be used consistently for new drivers. We can't > > > change existing DT bindings and drivers to drop support for the > > > non-OF-graph description due to ABI stability, but we can avoid > > > repeating the mistake going forward. > > > > > > > What I'm trying to address here is that some controllers that do > > > > everything right can't be used because that restriction is completely > > > > arbitrary and in opposition to the consensus. And they can't be used > > > > *today*. > > > > > > > > If we want to change that consensus, fine, but we should still have one. > > > > Having some bridges enforcing custom rules for no reason is very much > > > > unacceptable. > > > > > > > > And changing that consensus won't happen overtime, we'll have to take > > > > care of the backward compatibility, etc. So it won't fix the issue that > > > > we can't use any bridge with any controller any time soon. > > > > > > I don't think that's the issue at hand here. You can still use a > > > non-OF-graph DT event if the nodes for the two bridges affected by this > > > patch define a port@0. It can just be left unconnected. > > > > > > I do agree it will cause some DT bindings for DCS-based DSI sinks to >
Re: [PATCH v9 00/23] drm/rockchip: RK356x VOP2 support
On Wed, Mar 30, 2022 at 10:41:56AM +0200, piotro.oniszc...@google.com wrote: > > > > Wiadomość napisana przez Sascha Hauer w dniu > > 30.03.2022, o godz. 09:28: > > > >> > >> You can easily reproduce with modetest utility: > >> > >> modetest -P 43@67:1920x1080@NV12 > > > > This only sets the overlay, but how did you get something on the screen > > initially? Let me rephrase this: The above sets a plane, but it doesn't set a mode on the crtc. When my system boots up then the output of modetest looks like this: Encoders: id crtctypepossible crtcs possible clones 68 0 TMDS0x0001 0x0001 Connectors: id encoder status namesize (mm) modes encoders 69 0 connected HDMI-A-1530x300 9 68 CRTCs: id fb pos size 67 0 (0,0) (0x0) #0 nan 0 0 0 0 0 0 0 0 0 flags: ; type: No mode is set on the CRTC and the encoder/connector/crtc are not bound to each other, consequently the screen is in standby. "modetest -P 43@67:1920x1080@NV12" doesn't change this, still no mode set. Hence my question: How did you set a mode initially? > > > > I'm not sure that above command only sets plane. > On other SoCs i’m testing it gives expected results: diagonal colored stripes. > There is single exception: rk356x with vop2 - where screen is green unless i > „fix/enable” by playing with plane #69 > > > I did with "modetest -s 69@67:1920x1080 -d" and with this it works as > > expected, I can't reproduce any green screen issue here. > > I see you are using plane #69. > Why not #43? I used "modetest -s 69@67:1920x1080 -d" to set a mode. The '69' is the connector id, not a plane. > Is plane #43 working ok for you? Yes. > > I’m using plane #43 because: application (player) - at start - queries all > planes and selects first plane offering format being within offered formats > by provider (video decoder; NV12 from rk356x hantro video decoder). > > pls look on app log regarding planes discovery and election: > https://pastebin.com/edAhbcvU > > Now - looking what VOP2 reports: https://pastebin.com/8ujkaV9n > is see first plane accepting NV12 is #43 - so my app is electing this plane > to use for displaying video. > > This strategy works well for all 13 platforms i’m supporting (only 13 i have > in my testbed). > > If this approach is - by Yours VOP2 patches goal - is not supported - then OK. > I understand this :-) > > But - if You want to support DRM features in the same way like other SOC are > doing (and working well with KODI/MythTV/mpv/etc) - then i think: > > 1\ DRM plane #43 not supports NV12 - but code wrongly reports NV12 format is > supported, or > 2\ DRM plane #43 is supported - but code has bug resulting with green screen. plane #43 should support NV12 and it seems to work fine here. I believe you that there's a problem, but I can't reproduce it here and I might need further assistence to reproduce it. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event
On Tue, Mar 29, 2022 at 12:25:55PM -0400, Marek Olšák wrote: > I don't know what iris does, but I would guess that the same problems as > with AMD GPUs apply, making GPUs resets very fragile. iris_batch_check_for_reset -> replace_kernel_ctx -> iris_lost_context_state is I think the main call chain of how this is handled/detected. There's also a side-chain which handles -EIO from execbuf. Also this is using non-recoverable contexts, i.e. any time they suffer from a gpu reset (either because guilty themselves, or collateral damage of a reset that shot more than just the guilty context) the context stops entirely and refuses any further execbuf with -EIO. Cheers, Daniel > > Marek > > On Tue., Mar. 29, 2022, 08:14 Christian König, > wrote: > > > My main question is what does the iris driver better than radeonsi when > > the client doesn't support the robustness extension? > > > > From Daniels description it sounds like they have at least a partial > > recovery mechanism in place. > > > > Apart from that I completely agree to what you said below. > > > > Christian. > > > > Am 26.03.22 um 01:53 schrieb Olsak, Marek: > > > > [AMD Official Use Only] > > > > amdgpu has 2 resets: soft reset and hard reset. > > > > The soft reset is able to recover from an infinite loop and even some GPU > > hangs due to bad shaders or bad states. The soft reset uses a signal that > > kills all currently-running shaders of a certain process (VM context), > > which unblocks the graphics pipeline, so draws and command buffers finish > > but are not correctly. This can then cause a hard hang if the shader was > > supposed to signal work completion through a shader store instruction and a > > non-shader consumer is waiting for it (skipping the store instruction by > > killing the shader won't signal the work, and thus the consumer will be > > stuck, requiring a hard reset). > > > > The hard reset can recover from other hangs, which is great, but it may > > use a PCI reset, which erases VRAM on dGPUs. APUs don't lose memory > > contents, but we should assume that any process that had running jobs on > > the GPU during a GPU reset has its memory resources in an inconsistent > > state, and thus following command buffers can cause another GPU hang. The > > shader store example above is enough to cause another hard hang due to > > incorrect content in memory resources, which can contain synchronization > > primitives that are used internally by the hardware. > > > > Asking the driver to replay a command buffer that caused a hang is a sure > > way to hang it again. Unrelated processes can be affected due to lost VRAM > > or the misfortune of using the GPU while the GPU hang occurred. The window > > system should recreate GPU resources and redraw everything without > > affecting applications. If apps use GL, they should do the same. Processes > > that can't recover by redrawing content can be terminated or left alone, > > but they shouldn't be allowed to submit work to the GPU anymore. > > > > dEQP only exercises the soft reset. I think WebGL is only able to trigger > > a soft reset at this point, but Vulkan can also trigger a hard reset. > > > > Marek > > -- > > *From:* Koenig, Christian > > > > *Sent:* March 23, 2022 11:25 > > *To:* Daniel Vetter ; Daniel Stone > > ; Olsak, Marek > > ; Grodzovsky, Andrey > > > > *Cc:* Rob Clark ; Rob Clark > > ; Sharma, Shashank > > ; Christian König > > ; > > Somalapuram, Amaranath > > ; Abhinav Kumar > > ; dri-devel > > ; amd-gfx list > > ; Deucher, > > Alexander ; > > Shashank Sharma > > > > *Subject:* Re: [PATCH v2 1/2] drm: Add GPU reset sysfs event > > > > [Adding Marek and Andrey as well] > > > > Am 23.03.22 um 16:14 schrieb Daniel Vetter: > > > On Wed, 23 Mar 2022 at 15:07, Daniel Stone > > wrote: > > >> Hi, > > >> > > >> On Mon, 21 Mar 2022 at 16:02, Rob Clark > > wrote: > > >>> On Mon, Mar 21, 2022 at 2:30 AM Christian König > > >>> wrote: > > Well you can, it just means that their contexts are lost as well. > > >>> Which is rather inconvenient when deqp-egl reset tests, for example, > > >>> take down your compositor ;-) > > >> Yeah. Or anything WebGL. > > >> > > >> System-wide collateral damage is definitely a non-starter. If that > > >> means that the userspace driver has to do what iris does and ensure > > >> everything's recreated and resubmitted, that works too, just as long > > >> as the response to 'my adblocker didn't detect a crypto miner ad' is > > >> something better than 'shoot the entire user session'. > > > Not sure where that idea came from, I thought at least I made it clear > > > that legacy gl _has_ to recover. It's only vk and arb_robustness gl > > > which should die without recovery attempt. > > > > > > The entire discussion here is who should be responsible for replay and > > > at least if you can decide the uapi, then punting that entirely to > > > userspace is a good approach. > > > > Yes, completely agree. We
Re: DRM Master ignoring hotplug event during display switching (QT)
On Wed, Mar 30, 2022 at 10:52:54AM +0200, Maxime Ripard wrote: > On Tue, Mar 29, 2022 at 11:38:32PM +0530, Jagan Teki wrote: > > Hi all, > > > > I have implemented runtime display switching in the MIPI switch design > > where LVDS and HDMI bridges are selected with the help of runtime > > GPIO. > > > > Initial discussion on the same can be found here, > > https://www.spinics.net/lists/dri-devel/msg318524.html > > > > The implementation has been done by creating each connector at > > runtime. The default boot will create the LVDS connector and based on > > the HDMI plug-in the ISR. > > > > 1. forcing the LVDS to disconnect > > 2. call drm_kms_helper_hotplug_event > > 3. detach the bridge chain > > 4. attach the new bridge chain (HDMI) > > 5. call drm_kms_helper_hotplug_event > > > > do the reverse when we unplug the HDMI cable. > > > > So, the bridge chains are attached and detached based on GPIO > > Interrupt which is indeed identified based on the physical HDMIA > > connector. > > > > Pipeline for LVDS, > > mxfsb => nwl-dsi => display-switch => sn65dsi83=> panel-bridge > > > > Pipeline for HDMI, > > mxfsb => nwl-dsi => display-switch => adv7511 => display-connector > > > > With this, implementation and I can able switch the displays with > > default DRM (without specific DRM applications) where the LVDS is ON > > by default and when HDMI plug-in the LVDS OFF/HDMI ON, and when HDMI > > unplug the HDMI OFF/LVDS ON. > > > > However, with QT5 I can see the DRM Master ignoring hotplug event by > > returning 0 on drm_master_internal_acquire in > > drm_fb_helper_hotplug_event. With this the hotplug returned early so > > it cannot able to disconnect and connect the new switching connector. > > > > Any help? > > I'm not sure why you started another discussion with pretty much the > same content, but you can't rely on userspace handling the hotplug > event. You'll have to deal with the case where it just doesn't. Well I missed the old thread, so I'm replying here. You should not handle this at all from a hotplug. The way kms works is roughly as follows: 1. hw output state changes 2. driver detects this (either through hpd interrupt or polling) 3. driver sends out hotplug uevent That's it. Nothing else, no bridge rebinding, no link retaining is required. Then either userspace or fbcon emulation reacts to this hotplug event by doing an atomic modeset, where it hopefully disables the old output and re-enables the new output. Your atomic_check needs to validate that everything is all right (i.e. not enabling both at the same time). Note that if you change stuff underneath, then that tends to seriously upset atomic users. Also you should try to continue supporting at least page flips with the wrong config, compositors otherwise tend to crash. This also means that if userspace doesn't handle hotplug events, then you might end up with a black screen. That's ok. We try to avoid that when it's practical (e.g. dp sst link retraining), but not when it's too hard (dp mst hot-replug relies on userspace restoring everything). Finally exchanging the bridge chain isn't supported, there's no locking for that since it's assumed to be invariant over the lifetim of the drm_device instance. The simplest way to make that happen right now is to have 2 drm_encoder instances, one with the lvds bridge chain, the other with the hdmi bridge chain, and select the right encoder/bridge chain depending upon which output userspace picks. Also ofc your atomic_check needs to make sure that they're not both enabled at the same time :-) I wouldn't try to make bridge chains exchangeable instead, that's headaches - e.g. with dp mst we've also opted for a bunch of fake drm_encoders to model that kind of switching. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] dt-bindings: display: bridge: Drop requirement on input port for DSI devices
Hi Tomi, On Fri, Mar 25, 2022 at 12:42:15PM +0200, Tomi Valkeinen wrote: > On 24/03/2022 16:23, Maxime Ripard wrote: > > On Thu, Mar 24, 2022 at 03:43:42PM +0200, Laurent Pinchart wrote: > > > On Thu, Mar 24, 2022 at 09:18:19AM +0100, Maxime Ripard wrote: > > > > On Wed, Mar 23, 2022 at 10:38:19PM +0200, Laurent Pinchart wrote: > > > > > Hi Maxime, > > > > > > > > > > (CC'ing Sakari) > > > > > > > > > > Thank you for the patch. > > > > > > > > > > On Wed, Mar 23, 2022 at 04:48:23PM +0100, Maxime Ripard wrote: > > > > > > MIPI-DSI devices, if they are controlled through the bus itself, > > > > > > have to > > > > > > be described as a child node of the controller they are attached to. > > > > > > > > > > > > Thus, there's no requirement on the controller having an OF-Graph > > > > > > output > > > > > > port to model the data stream: it's assumed that it would go from > > > > > > the > > > > > > parent to the child. > > > > > > > > > > > > However, some bridges controlled through the DSI bus still require > > > > > > an > > > > > > input OF-Graph port, thus requiring a controller with an OF-Graph > > > > > > output > > > > > > port. This prevents those bridges from being used with the > > > > > > controllers > > > > > > that do not have one without any particular reason to. > > > > > > > > > > > > Let's drop that requirement. > > > > > > > > > > I'm sure this won't come as a surprise, I'm very much opposed to this > > > > > change, for two reasons. > > > > > > > > > > First, ports are part of the hardware, even if they're not connected. > > > > > It > > > > > thus simplifies handling in drivers if they're always present. > > > > > > > > > > Then, and that's the most important reason, I think it's a mistake not > > > > > to model the DSI data connection using OF graph unconditionally, even > > > > > when the DSI sink device is also controlled through the DSI bus (using > > > > > DCS) and is in that case a child of the DSI source device in the DT > > > > > hierarchy. > > > > > > > > That's the way we do for any other device though. You never addressed > > > > that comment, but it's very much the same that occurs for i2c or spi > > > > controllers and their device. They all get their data from the parent > > > > bus. I don't see you advocate for using OF-Graph for those devices. > > > > > > Those are different, there's no data stream independent of the control > > > communications. > > > > Fine, then you have Ethernet PHYs, or any MMIO device that does DMA. > > Have those devices had the need for OF graphs? No, they don't. It's kind of my point actually :) > For display and capture we have a clear need. I don't think we should > sometimes use OF graphs and sometimes not, but rather use them > consistently at least in any new driver. There's a clear need when the data don't follow the obvious path, ie when there's a direct FIFO between a CRTC and its connector. If it was going through that obvious path, like an Ethernet PHY, then we wouldn't need it. A device being controlled through the DSI bus has that obvious path. If it's taking its data through the DSI bus, but is being controlled by an i2c bus, then it needs an OF-graph node... > > > > > The device tree describes a control hierarchy between devices. OF > > > > > graph > > > > > overlays on top of that a data transfer graph. The two are different > > > > > concepts, and the fact that DSI can sometimes be used as a control bus > > > > > doesn't change the concept. Using OF graph unconditionally to describe > > > > > the data connections for DSI leads to less variation in the device > > > > > tree > > > > > structure, and thus less complexity in the implementation. We're > > > > > suffering from the fact we haven't made it a requirement in the first > > > > > place, which can't be fixed due to ABI breakage constraints, but let's > > > > > not acknowledge it as a good idea. > > > > > > > > Honestly, it doesn't matter one bit. > > > > > > > > We have a huge discrepancy here today, and only a couple of bridges have > > > > that arbitrary restriction. The situation you don't want to acknowledge > > > > is the de-facto standard, by the generic binding and by what all the > > > > bridges and panels are implementing. Even panel-simple-dsi is doing it. > > > > So it's very much there already. > > > > > > It's here, and I think we should move away from it for new DSI sinks. > > > I'd like OF graph to be used consistently for new drivers. We can't > > > change existing DT bindings and drivers to drop support for the > > > non-OF-graph description due to ABI stability, but we can avoid > > > repeating the mistake going forward. > > > > > > > What I'm trying to address here is that some controllers that do > > > > everything right can't be used because that restriction is completely > > > > arbitrary and in opposition to the consensus. And they can't be used > > > > *today*. > > > > > > > > If we want to change that consensus, fine, but
Re: [PATCH v9 00/23] drm/rockchip: RK356x VOP2 support
> Wiadomość napisana przez Sascha Hauer w dniu > 30.03.2022, o godz. 11:45: > > On Wed, Mar 30, 2022 at 10:41:56AM +0200, piotro.oniszc...@google.com wrote: > > Let me rephrase this: The above sets a plane, but it doesn't set a mode > on the crtc. When my system boots up then the output of modetest looks > like this: > > Encoders: > id crtctypepossible crtcs possible clones > 68 0 TMDS0x0001 0x0001 > Connectors: > id encoder status namesize (mm) modes > encoders > 69 0 connected HDMI-A-1530x300 9 68 > CRTCs: > id fb pos size > 67 0 (0,0) (0x0) > #0 nan 0 0 0 0 0 0 0 0 0 flags: ; type: > > No mode is set on the CRTC and the encoder/connector/crtc are not bound > to each other, consequently the screen is in standby. "modetest -P > 43@67:1920x1080@NV12" doesn't change this, still no mode set. Hence my > question: How did you set a mode initially? Ah ok. I see your point. mode is set by app (player). Sequence was like this: -boot board -start app -on UI select playback -playback has green screen -exit app -run modetest -P 43@67:1920x1080@NV12 (the same green screen like in playback) -run modetest -P 49@67:1920x1080@NV12 (works ok) -run modetest -P 43@67:1920x1080@NV12 (now works ok) > >>> >> >> I'm not sure that above command only sets plane. >> On other SoCs i’m testing it gives expected results: diagonal colored >> stripes. >> There is single exception: rk356x with vop2 - where screen is green unless i >> „fix/enable” by playing with plane #69 >> >>> I did with "modetest -s 69@67:1920x1080 -d" and with this it works as >>> expected, I can't reproduce any green screen issue here. >> >> I see you are using plane #69. >> Why not #43? > > I used "modetest -s 69@67:1920x1080 -d" to set a mode. The '69' is the > connector id, not a plane. ack. typo from my side. it was modetest -P 49@67:1920x1080@NV12 > >> Is plane #43 working ok for you? > > Yes. So it looks your testing method of #43 is not meaningful for verifying issue we are discussing here. In my case: 12 SOC (except rk356x VOP2) gives me: -boot board -start app -on UI select playback -playback is ok -exit app -run modetest -P XX@YY:1920x1080@NV12 (diagonal stripes) (XX/YY are plane/connector elected by app: plane@conector with format matching provider format) rk356x with vop2 v9: -boot board -start app -on UI select playback -playback has green screen -exit app -run modetest -P 43@67:1920x1080@NV12 (the same green screen like in playback) -run modetest -P 49@67:1920x1080@NV12 (works ok) -run modetest -P 43@67:1920x1080@NV12 (now works ok)
Re: [PATCH v9 00/23] drm/rockchip: RK356x VOP2 support
On Wed, Mar 30, 2022 at 12:01:05PM +0200, piotro.oniszc...@google.com wrote: > > > > Wiadomość napisana przez Sascha Hauer w dniu > > 30.03.2022, o godz. 11:45: > > > > On Wed, Mar 30, 2022 at 10:41:56AM +0200, piotro.oniszc...@google.com wrote: > > > > Let me rephrase this: The above sets a plane, but it doesn't set a mode > > on the crtc. When my system boots up then the output of modetest looks > > like this: > > > > Encoders: > > id crtctypepossible crtcs possible clones > > 68 0 TMDS0x0001 0x0001 > > Connectors: > > id encoder status namesize (mm) modes > > encoders > > 69 0 connected HDMI-A-1530x300 9 68 > > CRTCs: > > id fb pos size > > 67 0 (0,0) (0x0) > > #0 nan 0 0 0 0 0 0 0 0 0 flags: ; type: > > > > No mode is set on the CRTC and the encoder/connector/crtc are not bound > > to each other, consequently the screen is in standby. "modetest -P > > 43@67:1920x1080@NV12" doesn't change this, still no mode set. Hence my > > question: How did you set a mode initially? > > Ah ok. I see your point. > mode is set by app (player). > > Sequence was like this: > -boot board > -start app > -on UI select playback > -playback has green screen > -exit app > -run modetest -P 43@67:1920x1080@NV12 (the same green screen like in playback) > -run modetest -P 49@67:1920x1080@NV12 (works ok) > -run modetest -P 43@67:1920x1080@NV12 (now works ok) > > > > >>> > >> > >> I'm not sure that above command only sets plane. > >> On other SoCs i’m testing it gives expected results: diagonal colored > >> stripes. > >> There is single exception: rk356x with vop2 - where screen is green unless > >> i „fix/enable” by playing with plane #69 > >> > >>> I did with "modetest -s 69@67:1920x1080 -d" and with this it works as > >>> expected, I can't reproduce any green screen issue here. > >> > >> I see you are using plane #69. > >> Why not #43? > > > > I used "modetest -s 69@67:1920x1080 -d" to set a mode. The '69' is the > > connector id, not a plane. > > ack. > typo from my side. > > it was > modetest -P 49@67:1920x1080@NV12 > > > > > >> Is plane #43 working ok for you? > > > > Yes. > > So it looks your testing method of #43 is not meaningful for verifying issue > we are discussing here. > > In my case: > 12 SOC (except rk356x VOP2) gives me: > -boot board > -start app > -on UI select playback > -playback is ok > -exit app > -run modetest -P XX@YY:1920x1080@NV12 (diagonal stripes) > > (XX/YY are plane/connector elected by app: plane@conector with format > matching provider format) > > rk356x with vop2 v9: > -boot board > -start app > -on UI select playback > -playback has green screen > -exit app > -run modetest -P 43@67:1920x1080@NV12 (the same green screen like in playback) > -run modetest -P 49@67:1920x1080@NV12 (works ok) > -run modetest -P 43@67:1920x1080@NV12 (now works ok) Does it change anything if you do a "modetest -s 69@67:1920x1080" before starting the app? Or if you run "modetest -P 43@67:1920x1080@NV12" before starting the app? Or other combinations thereof? Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [PATCH 3/8] drm/display: Introduce a DRM display-helper module
Hi Am 30.03.22 um 11:23 schrieb Javier Martinez Canillas: On 3/22/22 20:27, Thomas Zimmermann wrote: Replace the DP-helper module with a display-helper module. Update all related Kconfig and Makefile rules. Besides the existing code for DisplayPort, the new module will contain helpers for other video-output standards, such as HDMI. Drivers will still be able to select the required video-output helpers. Linking all such code into a single module avoids the proliferation of small kernel modules. Signed-off-by: Thomas Zimmermann --- [snip] +config DRM_DISPLAY_HELPER + tristate + depends on DRM + help + DRM helpers for display adapters. + config DRM_DP_HELPER tristate depends on DRM + select DRM_DISPLAY_HELPER help DRM helpers for DisplayPort. I was about to ask why this would still be needed but then re-read the commit message that says drivers will still be able to select required video-output helpers. That makes sense since the fact that all helpers will be in the same module would be transparent to drivers. diff --git a/drivers/gpu/drm/display/Makefile b/drivers/gpu/drm/display/Makefile index 75faffc706b1..90f12e9b4735 100644 --- a/drivers/gpu/drm/display/Makefile +++ b/drivers/gpu/drm/display/Makefile @@ -2,8 +2,9 @@ obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o -drm_dp_helper-y := drm_dp.o drm_dp_dual_mode_helper.o drm_dp_helper_mod.o drm_dp_mst_topology.o -drm_dp_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o -drm_dp_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o +drm_display_helper-y := drm_display_helper_mod.o +drm_display_helper-$(CONFIG_DRM_DP_HELPER) := drm_dp_helper.o drm_dp_dual_mode_helper.o drm_dp_mst_topology.o +drm_display_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o +drm_display_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o -obj-$(CONFIG_DRM_DP_HELPER) += drm_dp_helper.o +obj-$(CONFIG_DRM_DISPLAY_HELPER) += drm_display_helper.o The drm_dp_helper.ko module has some parameters and this change will break existing kernel cmdline that are using it: $ modinfo drivers/gpu/drm/dp/drm_dp_helper.ko | grep parm | cut -d : -f2 drm_dp_cec_unregister_delay dp_aux_i2c_speed_khz dp_aux_i2c_transfer_size I don't know whether those are considered a kernel ABI or not though, and some already changed when the DP helpers were moved from drm_kms_helper.ko Good point. I'll mention it in the commit message andcheck the documentation as well. At least, no one complained when these functions moved from kms helpers into dp helpers. Moving them again is unfortunate, but I hope that the new library will stick. I somehow expected that HDMI, HDCP et al would require their own libraries. But introducing several new and tiny kernel modules for such small helpers wasn't worth it. Hence, there's the display library that can collect all such helpers in a single place. It looks like MIPI DSI could be another candidate to be moved into the display library; at least partially. I have go through the codebase to see if there are drivers that would benefit from such a change. Best regards Thomas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 7/8] drm/display: Move HDMI helpers into display-helper module
On Tue, 22 Mar 2022, Thomas Zimmermann wrote: > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h > index 144c495b99c4..e6e9e4557067 100644 > --- a/include/drm/drm_edid.h > +++ b/include/drm/drm_edid.h > @@ -391,33 +391,6 @@ drm_load_edid_firmware(struct drm_connector *connector) > > bool drm_edid_are_equal(const struct edid *edid1, const struct edid *edid2); > > -int > -drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame, > - const struct drm_connector *connector, > - const struct drm_display_mode *mode); > -int > -drm_hdmi_vendor_infoframe_from_display_mode(struct hdmi_vendor_infoframe > *frame, > - const struct drm_connector > *connector, > - const struct drm_display_mode > *mode); > - > -void > -drm_hdmi_avi_infoframe_colorimetry(struct hdmi_avi_infoframe *frame, > -const struct drm_connector_state > *conn_state); > - > -void > -drm_hdmi_avi_infoframe_bars(struct hdmi_avi_infoframe *frame, > - const struct drm_connector_state *conn_state); > - > -void > -drm_hdmi_avi_infoframe_quant_range(struct hdmi_avi_infoframe *frame, > -const struct drm_connector *connector, > -const struct drm_display_mode *mode, > -enum hdmi_quantization_range > rgb_quant_range); > - > -int > -drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame, > - const struct drm_connector_state > *conn_state); > - > /** > * drm_eld_mnl - Get ELD monitor name length in bytes. > * @eld: pointer to an eld memory structure with mnl set > @@ -587,6 +560,10 @@ void drm_edid_get_monitor_name(struct edid *edid, char > *name, > struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev, > int hsize, int vsize, int fresh, > bool rb); > + > +u8 drm_match_hdmi_mode(const struct drm_display_mode *to_match); > +enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code); > +enum hdmi_picture_aspect drm_get_hdmi_aspect_ratio(const u8 video_code); I think these were fine as static, but not really great interfaces to export. There's zero input checking on the vic in the latter, because internally we could be sure they were fine. I also wish we could limit the usage to the module you're adding; this is now available to all drivers which should be discouraged. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v2] drm/qxl: fix qxl can't use in arm64
I'd like to make sure this has no side effects on x86 guests, it
probably is safe, but keep an eye for regression reports.
Reviewed-by: Dave Airlie
Dave.
On Wed, Mar 30, 2022 at 8:20 PM Cong Liu wrote:
>
> any suggestions or extra test I can do now?
>
> Regards,
> Cong
>
> On 2022/3/25 15:45, Christian König wrote:
> > Am 24.03.22 um 11:49 schrieb Cong Liu:
> >> qxl use ioremap to map ram_header and rom, in the arm64 implementation,
> >> the device is mapped as DEVICE_nGnRE, it can not support unaligned
> >> access. and qxl is a virtual device, it can be treated more like RAM
> >> than actual MMIO registers. use ioremap_wc() replace it.
> >>
> >> Signed-off-by: Cong Liu
> >
> > Looks sane to me, but I'm really not involved enough to fully judge.
> >
> > Acked-by: Christian König
> >
> >> ---
> >> drivers/gpu/drm/qxl/qxl_kms.c | 4 ++--
> >> drivers/gpu/drm/qxl/qxl_ttm.c | 4 ++--
> >> 2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c
> >> b/drivers/gpu/drm/qxl/qxl_kms.c
> >> index 4dc5ad13f12c..a054e4a00fe8 100644
> >> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> >> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> >> @@ -165,7 +165,7 @@ int qxl_device_init(struct qxl_device *qdev,
> >>(int)qdev->surfaceram_size / 1024,
> >>(sb == 4) ? "64bit" : "32bit");
> >> -qdev->rom = ioremap(qdev->rom_base, qdev->rom_size);
> >> +qdev->rom = ioremap_wc(qdev->rom_base, qdev->rom_size);
> >> if (!qdev->rom) {
> >> pr_err("Unable to ioremap ROM\n");
> >> r = -ENOMEM;
> >> @@ -183,7 +183,7 @@ int qxl_device_init(struct qxl_device *qdev,
> >> goto rom_unmap;
> >> }
> >> -qdev->ram_header = ioremap(qdev->vram_base +
> >> +qdev->ram_header = ioremap_wc(qdev->vram_base +
> >> qdev->rom->ram_header_offset,
> >> sizeof(*qdev->ram_header));
> >> if (!qdev->ram_header) {
> >> diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c
> >> b/drivers/gpu/drm/qxl/qxl_ttm.c
> >> index b2e33d5ba5d0..95df5750f47f 100644
> >> --- a/drivers/gpu/drm/qxl/qxl_ttm.c
> >> +++ b/drivers/gpu/drm/qxl/qxl_ttm.c
> >> @@ -82,13 +82,13 @@ int qxl_ttm_io_mem_reserve(struct ttm_device *bdev,
> >> case TTM_PL_VRAM:
> >> mem->bus.is_iomem = true;
> >> mem->bus.offset = (mem->start << PAGE_SHIFT) + qdev->vram_base;
> >> -mem->bus.caching = ttm_cached;
> >> +mem->bus.caching = ttm_write_combined;
> >> break;
> >> case TTM_PL_PRIV:
> >> mem->bus.is_iomem = true;
> >> mem->bus.offset = (mem->start << PAGE_SHIFT) +
> >> qdev->surfaceram_base;
> >> -mem->bus.caching = ttm_cached;
> >> +mem->bus.caching = ttm_write_combined;
> >> break;
> >> default:
> >> return -EINVAL;
> >
>
Re: Allow ttm_buffer_object without resource
Am 29.03.22 um 16:02 schrieb Daniel Vetter: On Tue, Mar 29, 2022 at 01:02:32PM +0200, Christian König wrote: Hi guys, this patch set cleans up the handling of TTM buffer objects quite a bit by allowing to create them without allocating a ttm_resource as well. That's not only cleaner in general, but also a necessary prerequisite for quite a number of related work. Maybe there's some threads I missed, but I can't really guess what this could be useful for without even a hint. Well about a week ago Bob send me a partial implementation of this and said he needs it for i915. What exactly i915 needs here I'm not sure about either. I have this cleanup in the pipeline because amdgpu wants to improve his page table handling with this and independent of those driver use case patches #10 and #11 drop allocating dummy resources for two use cases in TTM. Christian. -Daniel
Re: [PATCH 5/9] drm/msm: Drop msm_gem_iova()
On Wed, 30 Mar 2022 at 02:00, Rob Clark wrote:
>
> From: Rob Clark
>
> There was only a single user, which could just as easily stash the iova
> when pinning.
>
> Signed-off-by: Rob Clark
Reviewed-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/msm_fb.c | 16 ++--
> drivers/gpu/drm/msm/msm_gem.c | 16
> drivers/gpu/drm/msm/msm_gem.h | 2 --
> 3 files changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c
> index 7137492fe78e..d4eef66e29dc 100644
> --- a/drivers/gpu/drm/msm/msm_fb.c
> +++ b/drivers/gpu/drm/msm/msm_fb.c
> @@ -21,6 +21,9 @@ struct msm_framebuffer {
>
> /* Count of # of attached planes which need dirtyfb: */
> refcount_t dirtyfb;
> +
> + /* Framebuffer per-plane address, if pinned, else zero: */
> + uint64_t iova[DRM_FORMAT_MAX_PLANES];
> };
> #define to_msm_framebuffer(x) container_of(x, struct msm_framebuffer, base)
>
> @@ -76,14 +79,14 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb,
> {
> struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
> int ret, i, n = fb->format->num_planes;
> - uint64_t iova;
>
> if (needs_dirtyfb)
> refcount_inc(&msm_fb->dirtyfb);
>
> for (i = 0; i < n; i++) {
> - ret = msm_gem_get_and_pin_iova(fb->obj[i], aspace, &iova);
> - drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)",
> fb->base.id, i, iova, ret);
> + ret = msm_gem_get_and_pin_iova(fb->obj[i], aspace,
> &msm_fb->iova[i]);
> + drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)",
> + fb->base.id, i, msm_fb->iova[i], ret);
> if (ret)
> return ret;
> }
> @@ -103,14 +106,15 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb,
>
> for (i = 0; i < n; i++)
> msm_gem_unpin_iova(fb->obj[i], aspace);
> +
> + memset(msm_fb->iova, 0, sizeof(msm_fb->iova));
> }
>
> uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb,
> struct msm_gem_address_space *aspace, int plane)
> {
> - if (!fb->obj[plane])
> - return 0;
> - return msm_gem_iova(fb->obj[plane], aspace) + fb->offsets[plane];
> + struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb);
> + return msm_fb->iova[plane];
> }
>
> struct drm_gem_object *msm_framebuffer_bo(struct drm_framebuffer *fb, int
> plane)
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index f4b68bb28a4d..deafae6feaa8 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -509,22 +509,6 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
> return ret;
> }
>
> -/* get iova without taking a reference, used in places where you have
> - * already done a 'msm_gem_get_and_pin_iova' or 'msm_gem_get_iova'
> - */
> -uint64_t msm_gem_iova(struct drm_gem_object *obj,
> - struct msm_gem_address_space *aspace)
> -{
> - struct msm_gem_vma *vma;
> -
> - msm_gem_lock(obj);
> - vma = lookup_vma(obj, aspace);
> - msm_gem_unlock(obj);
> - GEM_WARN_ON(!vma);
> -
> - return vma ? vma->iova : 0;
> -}
> -
> /*
> * Locked variant of msm_gem_unpin_iova()
> */
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 090c3b1a6d9a..772de010a669 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -142,8 +142,6 @@ int msm_gem_get_and_pin_iova_locked(struct drm_gem_object
> *obj,
> struct msm_gem_address_space *aspace, uint64_t *iova);
> int msm_gem_get_and_pin_iova(struct drm_gem_object *obj,
> struct msm_gem_address_space *aspace, uint64_t *iova);
> -uint64_t msm_gem_iova(struct drm_gem_object *obj,
> - struct msm_gem_address_space *aspace);
> void msm_gem_unpin_iova_locked(struct drm_gem_object *obj,
> struct msm_gem_address_space *aspace);
> void msm_gem_unpin_iova(struct drm_gem_object *obj,
> --
> 2.35.1
>
--
With best wishes
Dmitry
Re: [PATCH 3/9] drm/msm/gem: Split out inuse helper
On Wed, 30 Mar 2022 at 02:00, Rob Clark wrote:
>
> From: Rob Clark
>
> Prep for a following patch. While we are at it, convert a few remaining
> WARN_ON()s to GEM_WARN_ON().
Well... GEM_WARN_ON doesn't really look like a 'while we are at it'.
It might be better to split it into a separate commit.
>
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem.c | 2 +-
> drivers/gpu/drm/msm/msm_gem.h | 1 +
> drivers/gpu/drm/msm/msm_gem_vma.c | 15 ++-
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index a4f61972667b..f96d1dc72021 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -938,7 +938,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct
> seq_file *m,
> name, comm ? ":" : "", comm ? comm : "",
> vma->aspace, vma->iova,
> vma->mapped ? "mapped" : "unmapped",
> - vma->inuse);
> + msm_gem_vma_inuse(vma));
> kfree(comm);
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 947ff7d9b471..1b7f0f0b88bf 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -61,6 +61,7 @@ struct msm_gem_vma {
> int msm_gem_init_vma(struct msm_gem_address_space *aspace,
> struct msm_gem_vma *vma, int npages,
> u64 range_start, u64 range_end);
> +bool msm_gem_vma_inuse(struct msm_gem_vma *vma);
> void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
> struct msm_gem_vma *vma);
> void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c
> b/drivers/gpu/drm/msm/msm_gem_vma.c
> index f914ddbaea89..dc2ae097805e 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -37,6 +37,11 @@ msm_gem_address_space_get(struct msm_gem_address_space
> *aspace)
> return aspace;
> }
>
> +bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
> +{
> + return !!vma->inuse;
> +}
It almost asks to be a static inline. The patch 04 makes it complex,
so it's probably not that important.
> +
> /* Actually unmap memory for the vma */
> void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
> struct msm_gem_vma *vma)
> @@ -44,7 +49,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
> unsigned size = vma->node.size << PAGE_SHIFT;
>
> /* Print a message if we try to purge a vma in use */
> - if (WARN_ON(vma->inuse > 0))
> + if (GEM_WARN_ON(msm_gem_vma_inuse(vma)))
> return;
>
> /* Don't do anything if the memory isn't mapped */
> @@ -61,7 +66,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
> void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
> struct msm_gem_vma *vma)
> {
> - if (!WARN_ON(!vma->iova))
> + if (!GEM_WARN_ON(!vma->iova))
> vma->inuse--;
> }
>
> @@ -73,7 +78,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
> unsigned size = npages << PAGE_SHIFT;
> int ret = 0;
>
> - if (WARN_ON(!vma->iova))
> + if (GEM_WARN_ON(!vma->iova))
> return -EINVAL;
>
> /* Increase the usage counter */
> @@ -100,7 +105,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
> void msm_gem_close_vma(struct msm_gem_address_space *aspace,
> struct msm_gem_vma *vma)
> {
> - if (WARN_ON(vma->inuse > 0 || vma->mapped))
> + if (GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped))
> return;
>
> spin_lock(&aspace->lock);
> @@ -120,7 +125,7 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace,
> {
> int ret;
>
> - if (WARN_ON(vma->iova))
> + if (GEM_WARN_ON(vma->iova))
> return -EBUSY;
>
> spin_lock(&aspace->lock);
> --
> 2.35.1
>
--
With best wishes
Dmitry
RX 5500 XT: PCIe link speed stuck at Gen1 2.5GT/s by default
See https://gitlab.freedesktop.org/drm/amd/-/issues/1447 This report was against DRM, but I'm mentioning it here because we've been talking about some link speed init issues lately, and AFAICT the gitlab reports don't show up anywhere in lore. Robert Hancock reported: > I'm using a RX 5500 XT card on an Asus PRIME H270-PRO motherboard, > Intel i5-7500 CPU, with kernel 5.10.9 under Fedora 33. I noticed > that in Linux, "lspci -vv" always showed the GPU PCIe link running > at 2.5GT/s link speed and never seemed to change regardless of the > application being run, while in Windows, GPU-Z shows the link > running at the max supported 8GT/s speed when under graphical load. > > It seems like the driver thinks that 2.5GT/s is the max allowable > speed, based on the pp_dpm_pcie file: > > > cd > /sys/devices/pci:00/:00:01.0/:01:00.0/:02:00.0/:03:00.0/ > > cat pp_dpm_pcie > 0: 2.5GT/s, x8 81Mhz * > 1: 2.5GT/s, x8 619Mhz * > > I'm assuming that something is going wrong with the PCIe link speed > detection in the driver. Using the "amdgpu.pcie_gen_cap=0x70007" > kernel command line option seems to result in the driver detecting > the proper 8GT/s maximum speed. > > lspci -vv output from booting without overriding the speed is > attached.
[PATCH AUTOSEL 5.17 14/66] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 5.16 14/59] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 5.15 13/50] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 5.10 06/37] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 5.4 02/25] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 4.19 01/22] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 4.14 01/20] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH AUTOSEL 4.9 01/17] video: fbdev: nvidiafb: Use strscpy() to prevent buffer overflow
From: Tim Gardner
[ Upstream commit 37a1a2e6eeeb101285cd34e12e48a881524701aa ]
Coverity complains of a possible buffer overflow. However,
given the 'static' scope of nvidia_setup_i2c_bus() it looks
like that can't happen after examiniing the call sites.
CID 19036 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
1. fixed_size_dest: You might overrun the 48-character fixed-size string
chan->adapter.name by copying name without checking the length.
2. parameter_as_source: Note: This defect has an elevated risk because the
source argument is a parameter of the current function.
89strcpy(chan->adapter.name, name);
Fix this warning by using strscpy() which will silence the warning and
prevent any future buffer overflows should the names used to identify the
channel become much longer.
Cc: Antonino Daplas
Cc: linux-fb...@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Tim Gardner
Signed-off-by: Helge Deller
Signed-off-by: Sasha Levin
---
drivers/video/fbdev/nvidia/nv_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/nvidia/nv_i2c.c
b/drivers/video/fbdev/nvidia/nv_i2c.c
index d7994a173245..0b48965a6420 100644
--- a/drivers/video/fbdev/nvidia/nv_i2c.c
+++ b/drivers/video/fbdev/nvidia/nv_i2c.c
@@ -86,7 +86,7 @@ static int nvidia_setup_i2c_bus(struct nvidia_i2c_chan *chan,
const char *name,
{
int rc;
- strcpy(chan->adapter.name, name);
+ strscpy(chan->adapter.name, name, sizeof(chan->adapter.name));
chan->adapter.owner = THIS_MODULE;
chan->adapter.class = i2c_class;
chan->adapter.algo_data = &chan->algo;
--
2.34.1
[PATCH 0/5] fix missing break in list_or_each_entry
Instead of exiting the loop as expected when an entry is found, the list_for_each_entry() continues until the traversal is complete. It could lead to invalid reference or set 'is_*' flags mistakely. To fix this, when an entry is found, add a break to exit the loop. Xiaomeng Tong (5): gma500: fix a missing break in oaktrail_crtc_mode_set gma500: fix a missing break in cdv_intel_crtc_mode_set gma500: fix a missing break in psb_intel_crtc_mode_set gma500: fix a missing break in cdv_intel_dp_set_m_n gma500: fix a missing break in psb_driver_load drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++ drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++ drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++ drivers/gpu/drm/gma500/psb_drv.c | 2 ++ drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++ 5 files changed, 10 insertions(+) -- 2.17.1
[PATCH 1/5] gma500: fix a missing break in oaktrail_crtc_mode_set
Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
will certainly lead to a invalid reference to list itereator variable
'connector' after the loop pointing an bogus address at an offset
from the list head, and could lead to multiple 'is_*' flags being set
with true mistakely too.
The invalid reference to list itereator is here:
drm_object_property_get_value(&connector->base,
To fix this, when found the entry, add a break after the switch
statement.
Fixes: a69ac9ea85d87 ("drm/gma500: drm_connector_property ->
drm_object_property")
Signed-off-by: Xiaomeng Tong
---
drivers/gpu/drm/gma500/oaktrail_crtc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c
b/drivers/gpu/drm/gma500/oaktrail_crtc.c
index 36c7c2686c90..eb2d79872bd5 100644
--- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
+++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
@@ -409,6 +409,8 @@ static int oaktrail_crtc_mode_set(struct drm_crtc *crtc,
is_mipi = true;
break;
}
+
+ break;
}
/* Disable the VGA plane that we never use */
--
2.17.1
[PATCH 2/5] gma500: fix a missing break in cdv_intel_crtc_mode_set
Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could lead to a invalid reference to 'ddi_select' after the loop, and
could lead to multiple 'is_*' flags being set with true mistakely, too.
The invalid reference to 'ddi_select' is here:
cdv_dpll_set_clock_cdv(dev, crtc, &clock, is_lvds, ddi_select);
To fix this, when found the entry, add a break after the switch statement.
Fixes: d66760962d75 ("gma500: Program the DPLL lane based on the selected
digitial port")
Signed-off-by: Xiaomeng Tong
---
drivers/gpu/drm/gma500/cdv_intel_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_display.c
b/drivers/gpu/drm/gma500/cdv_intel_display.c
index 94ebc48a4349..3e93019b17cb 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_display.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_display.c
@@ -616,6 +616,8 @@ static int cdv_intel_crtc_mode_set(struct drm_crtc *crtc,
DRM_ERROR("invalid output type.\n");
return 0;
}
+
+ break;
}
if (dev_priv->dplla_96mhz)
--
2.17.1
[PATCH 3/5] gma500: fix a missing break in psb_intel_crtc_mode_set
Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could result in multiple 'is_*' flags being set with true mistakely.
To fix this, when found the entry, add a break after the switch
statement.
Fixes: 89c78134cc54d (" gma500: Add Poulsbo support")
Signed-off-by: Xiaomeng Tong
---
drivers/gpu/drm/gma500/psb_intel_display.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c
b/drivers/gpu/drm/gma500/psb_intel_display.c
index 42d1a733e124..85fc61bf333a 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -134,6 +134,8 @@ static int psb_intel_crtc_mode_set(struct drm_crtc *crtc,
is_tv = true;
break;
}
+
+ break;
}
refclk = 96000;
--
2.17.1
[PATCH 4/5] gma500: fix a missing break in cdv_intel_dp_set_m_n
Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. It
could lead to a invalid reference to 'lane_count/bpp' after the loop.
The invalid reference to 'lane_count/bpp' is here:
cdv_intel_dp_compute_m_n(bpp, lane_count,
To fix this, when found the entry, add a break after the switch statement.
Fixes: 8695b61294356 ("gma500: Add the support of display port on CDV")
Signed-off-by: Xiaomeng Tong
---
drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c
b/drivers/gpu/drm/gma500/cdv_intel_dp.c
index ba6ad1466374..e6473b8da296 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
@@ -1016,6 +1016,8 @@ cdv_intel_dp_set_m_n(struct drm_crtc *crtc, struct
drm_display_mode *mode,
bpp = dev_priv->edp.bpp;
break;
}
+
+ break;
}
/*
--
2.17.1
[PATCH 5/5] gma500: fix a missing break in psb_driver_load
Instead of exiting the loop as expected when an entry is found, the
list_for_each_entry() continues until the traversal is complete. To
avoid potential executing 'ret = gma_backlight_init(dev);' repeatly,
add a break after the switch statement.
Fixes: 5c49fd3aa0ab0 ("gma500: Add the core DRM files and headers")
Signed-off-by: Xiaomeng Tong
---
drivers/gpu/drm/gma500/psb_drv.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 65cf1c79dd7c..d65a68811bf7 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -398,6 +398,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned
long flags)
ret = gma_backlight_init(dev);
break;
}
+
+ break;
}
if (ret)
--
2.17.1
Re: [PATCH v2] drm/amd/display: don't ignore alpha property on pre-multiplied mode
On 2022-03-29 16:18, Melissa Wen wrote:
"Pre-multiplied" is the default pixel blend mode for KMS/DRM, as
documented in supported_modes of drm_plane_create_blend_mode_property():
https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_blend.c
In this mode, both 'pixel alpha' and 'plane alpha' participate in the
calculation, as described by the pixel blend mode formula in KMS/DRM
documentation:
out.rgb = plane_alpha * fg.rgb +
(1 - (plane_alpha * fg.alpha)) * bg.rgb
Considering the blend config mechanisms we have in the driver so far,
the alpha mode that better fits this blend mode is the
_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN, where the value for global_gain
is the plane alpha (global_alpha).
With this change, alpha property stops to be ignored. It also addresses
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1734
v2:
* keep the 8-bit value for global_alpha_value (Nicholas)
* correct the logical ordering for combined global gain (Nicholas)
* apply to dcn10 too (Nicholas)
Signed-off-by: Melissa Wen
---
.../drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 14 +-
drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c | 14 +-
2 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
index ad757b59e00e..b1034e6258c8 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
@@ -2528,14 +2528,18 @@ void dcn10_update_mpcc(struct dc *dc, struct pipe_ctx
*pipe_ctx)
struct mpc *mpc = dc->res_pool->mpc;
struct mpc_tree *mpc_tree_params =
&(pipe_ctx->stream_res.opp->mpc_tree_params);
- if (per_pixel_alpha)
- blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
- else
- blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
-
blnd_cfg.overlap_only = false;
blnd_cfg.global_gain = 0xff;
+ if (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) {
+ blnd_cfg.alpha_mode =
MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
+ blnd_cfg.global_gain =
pipe_ctx->plane_state->global_alpha_value;
+ } else if (per_pixel_alpha) {
+ blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
+ } else {
+ blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
+ }
+
if (pipe_ctx->plane_state->global_alpha)
blnd_cfg.global_alpha =
pipe_ctx->plane_state->global_alpha_value;
else
diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
index 4290eaf11a04..b627c41713cc 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
@@ -2344,14 +2344,18 @@ void dcn20_update_mpcc(struct dc *dc, struct pipe_ctx
*pipe_ctx)
struct mpc *mpc = dc->res_pool->mpc;
struct mpc_tree *mpc_tree_params =
&(pipe_ctx->stream_res.opp->mpc_tree_params);
- if (per_pixel_alpha)
- blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
- else
- blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
-
blnd_cfg.overlap_only = false;
blnd_cfg.global_gain = 0xff;
+ if (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) {
+ blnd_cfg.alpha_mode =
MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
+ blnd_cfg.global_gain =
pipe_ctx->plane_state->global_alpha_value;
+ } else if (per_pixel_alpha) {
+ blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
+ } else {
+ blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
+ }
+
Hi Melissa,
Thanks a lot for this patch. I run your patch in our CI, and everything
looks good from the IGT test result.
In this sense:
Tested-by: Rodrigo Siqueira
However, I think it is still necessary to have someone else review.
Harry, Nick, or Zhan, what do you think about this change?
Thanks
Siqueira
if (pipe_ctx->plane_state->global_alpha)
blnd_cfg.global_alpha =
pipe_ctx->plane_state->global_alpha_value;
else
Re: regression: NULL pointer dereference due to 27599aacbaef ("fbdev: Hot-unplug firmware fb devices on forced removal")
[TLDR: I'm adding the regression report below to regzbot, the Linux
kernel regression tracking bot; all text you find below is compiled from
a few templates paragraphs you might have encountered already already
from similar mails.]
[adding fbdev and dri-devel lists, and the egression mailing list, as it
should be in the loop for all regressions]
Hi, this is your Linux kernel regression tracker. Top-posting for once,
to make this easily accessible to everyone.
Thanks for the report. Thomas, Zack, do you know what's up here?
Note to everyone: the culprit of this regression is marked for backporting.
Anyway: To be sure below issue doesn't fall through the cracks
unnoticed, I'm adding it to regzbot, my Linux kernel regression tracking
bot:
#regzbot ^introduced 27599aacbaef
#regzbot title drm: fbdev: NULL pointer dereference due to 27599aacbaef
("fbdev: Hot-unplug firmware fb devices on forced removal")
#regzbot ignore-activity
If it turns out this isn't a regression, free free to remove it from the
tracking by sending a reply to this thread containing a paragraph like
"#regzbot invalid: reason why this is invalid" (without the quotes).
Reminder for developers: when fixing the issue, please add a 'Link:'
tags pointing to the report (the mail quoted above) using
lore.kernel.org/r/, as explained in
'Documentation/process/submitting-patches.rst' and
'Documentation/process/5.Posting.rst'. Regzbot needs them to
automatically connect reports with fixes, but they are useful in
general, too.
I'm sending this to everyone that got the initial report, to make
everyone aware of the tracking. I also hope that messages like this
motivate people to directly get at least the regression mailing list and
ideally even regzbot involved when dealing with regressions, as messages
like this wouldn't be needed then. And don't worry, if I need to send
other mails regarding this regression only relevant for regzbot I'll
send them to the regressions lists only (with a tag in the subject so
people can filter them away). With a bit of luck no such messages will
be needed anyway.
Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
P.S.: As the Linux kernel's regression tracker I'm getting a lot of
reports on my table. I can only look briefly into most of them and lack
knowledge about most of the areas they concern. I thus unfortunately
will sometimes get things wrong or miss something important. I hope
that's not the case here; if you think it is, don't hesitate to tell me
in a public reply, it's in everyone's interest to set the public record
straight.
On 28.03.22 17:41, Sudip Mukherjee wrote:
> Hi Thomas,
>
> We usually run boot tests with linux mainline HEAD commit almost every
> night on ppc64 qemu. And my tests had been failing for last few days.
> Testing locally gave me:
>
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.038791] BUG: Kernel NULL pointer
> dereference on read at 0x0060
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.038995] Faulting instruction
> address: 0xc080dfa4
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.039572] Oops: Kernel access of
> bad area, sig: 11 [#1]
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.039723] LE PAGE_SIZE=64K MMU=Hash
> SMP NR_CPUS=2048 NUMA pSeries
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.040012] Modules linked in:
> bochs(+) drm_vram_helper xhci_pci drm_kms_helper syscopyarea sysfillrect
> sysimgblt fb_sys_fops drm_ttm_helper sr_mod ttm cdrom xhci_hcd virtio_net
> virtio_console net_failover virtio_blk virtio_scsi failover ibmvscsi
> scsi_transport_srp virtio_pci virtio virtio_pci_legacy_dev
> virtio_pci_modern_dev usbcore drm drm_panel_orientation_quirks virtio_ring
> usb_common
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.040918] CPU: 2 PID: 139 Comm:
> systemd-udevd Not tainted 5.17.0-ae085d7f9365 #1
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.041245] NIP: c080dfa4
> LR: c080df9c CTR: c0797430
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.041376] REGS: c4132fe0
> TRAP: 0300 Not tainted (5.17.0-ae085d7f9365)
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.041528] MSR: 82009033
> CR: 28228282 XER: 2000
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.042017] CFAR: c000c80c
> DAR: 0060 DSISR: 4000 IRQMASK: 0
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.042017] GPR00: c080df9c
> c4133280 c169d200 0029
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.042017] GPR04: efff
> c4132f90 c4132f88
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.042017] GPR08: c15658f8
> c15cd200 c14f57d0 48228283
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.042017] GPR12:
> c0003fffe300 2000
> Mar 28 13:16:36 debian-ppc64 kernel: [ 11.042017] GPR16:
> 000113fc4a40 0005 000113
Re: [PATCH 0/2] remove DC_FP_* wrappers in dml files
On 2022-03-26 16:24, Melissa Wen wrote: From FPU documentation, developers must not use DC_FP_START/END in dml files, but invoke it when calling FPU-associated functions (isolated in dml folder). Therefore, the first patch renames dcn10_validate_bandwidth in dml/calcs to dcn_ for generalization, declares dcn10_validate_bandwidth in dcn10 - that calls dcn_validate_bandwidth and wraps with DC_FP_* accordingly. The second patch removes invocations of DC_FP_* from dml files and properly wraps FPU functions in dc code outside dml folder. Melissa Wen (2): drm/amd/display: detach fpu operations from dcn10_validate_bandwidth in calcs drm/amd/display: remove DC_FP_* wrapper from dml folder .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 10 -- .../drm/amd/display/dc/dcn10/dcn10_resource.c | 16 .../drm/amd/display/dc/dml/calcs/dcn_calcs.c | 19 +-- .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c | 2 -- .../gpu/drm/amd/display/dc/inc/dcn_calcs.h| 2 +- 5 files changed, 26 insertions(+), 23 deletions(-) Hi, Thanks a lot for your patch! I reviewed and tested this series and lgtm. Applied to amd-staging-drm-next. Btw, I agree with Christian. Can you try to find a way to add a compilation error or warning if the developer tries to add DC_FP_* inside DML? Also, about the question of recursive calling to DC_FP_*, it should be safe if using DC_FP_*. Thanks Siqueira
Re: [PATCH v9 20/23] drm/rockchip: Make VOP driver optional
Hi Sascha:
On 3/30/22 14:39, Sascha Hauer wrote:
Hi Andy,
On Tue, Mar 29, 2022 at 07:56:27PM +0800, Andy Yan wrote:
Hi Sascha:
On 3/28/22 23:11, Sascha Hauer wrote:
With upcoming VOP2 support VOP won't be the only choice anymore, so make
the VOP driver optional.
Signed-off-by: Sascha Hauer
---
drivers/gpu/drm/rockchip/Kconfig| 8
drivers/gpu/drm/rockchip/Makefile | 3 ++-
drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 2 +-
3 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index fa5cfda4e90e3..7d22e2997a571 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -23,8 +23,16 @@ config DRM_ROCKCHIP
if DRM_ROCKCHIP
+config ROCKCHIP_VOP
+ bool "Rockchip VOP driver"
+ default y
+ help
+ This selects support for the VOP driver. You should enable it
+ on all older SoCs up to RK3399.
That reminds me that I wanted to rephrase this. Will change in next
round.
+
config ROCKCHIP_ANALOGIX_DP
bool "Rockchip specific extensions for Analogix DP driver"
+ depends on ROCKCHIP_VOP
Aanlogix dp is also on vop2 base soc such as rk356x and rk3588 .
I added the dependency because analogix_dp-rockchip.c calls
rockchip_drm_wait_vact_end() which is implemented in the VOP driver,
so this driver currenty can't work with the VOP2 driver and can't
be linked without the VOP driver being present.
I'll add a few words to the commit message.
Maybe a better direction is move rockchip_drm_wait_vact_end from the VOP
driver to rockchip_drm_drv.c
int rockchip_drm_wait_vact_end(struct drm_crtc *crtc, unsigned int
mstimeout)
{
struct rockchip_drm_private *priv;
int pipe, ret = 0;
if (!crtc)
return -ENODEV;
if (mstimeout <= 0)
return -EINVAL;
priv = crtc->dev->dev_private;
pipe = drm_crtc_index(crtc);
if (priv->crtc_funcs[pipe] &&
priv->crtc_funcs[pipe]->wait_vact_end)
ret = priv->crtc_funcs[pipe]->wait_vact_end(crtc,
mstimeout);
return ret;
}
EXPORT_SYMBOL(rockchip_drm_wait_vact_end);
Sascha
Re: [Intel-gfx] [RFC v2 2/2] drm/doc/rfc: VM_BIND uapi definition
On Mon, Mar 07, 2022 at 12:31:46PM -0800, Niranjana Vishwanathapura wrote:
> VM_BIND und related uapi definitions
>
> Signed-off-by: Niranjana Vishwanathapura
> ---
> Documentation/gpu/rfc/i915_vm_bind.h | 176 +++
Maybe as the top level comment: The point of documenting uapi isn't to
just spell out all the fields, but to define _how_ and _why_ things work.
This part is completely missing from these docs here.
> 1 file changed, 176 insertions(+)
> create mode 100644 Documentation/gpu/rfc/i915_vm_bind.h
>
> diff --git a/Documentation/gpu/rfc/i915_vm_bind.h
> b/Documentation/gpu/rfc/i915_vm_bind.h
> new file mode 100644
> index ..80f00ee6c8a1
> --- /dev/null
> +++ b/Documentation/gpu/rfc/i915_vm_bind.h
You need to incldue this somewhere so it's rendered, see the previous
examples.
> @@ -0,0 +1,176 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +/* VM_BIND feature availability through drm_i915_getparam */
> +#define I915_PARAM_HAS_VM_BIND 57
Needs to be kernel-docified, which means we need a prep patch that fixes
up the existing mess.
> +
> +/* VM_BIND related ioctls */
> +#define DRM_I915_GEM_VM_BIND 0x3d
> +#define DRM_I915_GEM_VM_UNBIND 0x3e
> +#define DRM_I915_GEM_WAIT_USER_FENCE 0x3f
> +
> +#define DRM_IOCTL_I915_GEM_VM_BIND DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_BIND, struct drm_i915_gem_vm_bind)
> +#define DRM_IOCTL_I915_GEM_VM_UNBIND DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_UNBIND, struct drm_i915_gem_vm_bind)
> +#define DRM_IOCTL_I915_GEM_WAIT_USER_FENCE DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_WAIT_USER_FENCE, struct drm_i915_gem_wait_user_fence)
> +
> +/**
> + * struct drm_i915_gem_vm_bind - VA to object/buffer mapping to [un]bind.
Both binding and unbinding need to specify in excruciating detail what
happens if there's overlaps (existing mappings, or unmapping a range which
has no mapping, or only partially full of maps or different objects) and
fun stuff like that.
> + */
> +struct drm_i915_gem_vm_bind {
> + /** vm to [un]bind */
> + __u32 vm_id;
> +
> + /**
> + * BO handle or file descriptor.
> + * 'fd' value of -1 is reserved for system pages (SVM)
> + */
> + union {
> + __u32 handle; /* For unbind, it is reserved and must be 0 */
I think it'd be a lot cleaner if we do a bind and an unbind struct for
these, instead of mixing it up.
Also I thought mesa requested to be able to unmap an object from a vm
without a range. Has that been dropped, and confirmed to not be needed.
> + __s32 fd;
If we don't need it right away then don't add it yet. If it's planned to
be used then it needs to be documented, but I kinda have no idea why you'd
need an fd for svm?
> + }
> +
> + /** VA start to [un]bind */
> + __u64 start;
> +
> + /** Offset in object to [un]bind */
> + __u64 offset;
> +
> + /** VA length to [un]bind */
> + __u64 length;
> +
> + /** Flags */
> + __u64 flags;
> + /** Bind the mapping immediately instead of during next submission */
This aint kerneldoc.
Also this needs to specify in much more detail what exactly this means,
and also how it interacts with execbuf.
So the patch here probably needs to include the missing pieces on the
execbuf side of things. Like how does execbuf work when it's used with a
vm_bind managed vm? That means:
- document the pieces that are there
- then add a patch to document how that all changes with vm_bind
And do that for everything execbuf can do.
> +#define I915_GEM_VM_BIND_IMMEDIATE (1 << 0)
> + /** Read-only mapping */
> +#define I915_GEM_VM_BIND_READONLY(1 << 1)
> + /** Capture this mapping in the dump upon GPU error */
> +#define I915_GEM_VM_BIND_CAPTURE (1 << 2)
> +
> + /** Zero-terminated chain of extensions */
> + __u64 extensions;
> +};
> +
> +/**
> + * struct drm_i915_vm_bind_ext_user_fence - Bind completion signaling
> extension.
> + */
> +struct drm_i915_vm_bind_ext_user_fence {
> +#define I915_VM_BIND_EXT_USER_FENCE 0
> + /** @base: Extension link. See struct i915_user_extension. */
> + struct i915_user_extension base;
> +
> + /** User/Memory fence qword alinged process virtual address */
> + __u64 addr;
> +
> + /** User/Memory fence value to be written after bind completion */
> + __u64 val;
> +
> + /** Reserved for future extensions */
> + __u64 rsvd;
> +};
> +
> +/**
> + * struct drm_i915_gem_execbuffer_ext_user_fence - First level batch
> completion
> + * signaling extension.
> + *
> + * This extension allows user to attach a user fence ( pair) to
> an
> + * execbuf to be signaled by the command streamer after the completion of 1st
> + * level batch, by writing the at specified and triggering an
> + * interrupt.
> + * User can either poll for this user fence to signal or can also wait on it
> + * with i915_gem
Re: [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()
On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
> Mixing u8 * and struct edid * is confusing, switch to the latter.
>
> Cc: Ville Syrjälä
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/drm_edid.c | 31 +++
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d79b06f7f34c..0650b9217aa2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
> void *data)
> {
> int i, j = 0, valid_extensions = 0;
> - u8 *edid, *new;
> - struct edid *override;
> + struct edid *edid, *new, *override;
>
> override = drm_get_override_edid(connector);
> if (override)
> return override;
>
> - edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block,
> data);
> + edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
> if (!edid)
> return NULL;
>
> /* if there's no extensions or no connector, we're done */
> - valid_extensions = edid[0x7e];
> + valid_extensions = edid->extensions;
> if (valid_extensions == 0)
> - return (struct edid *)edid;
> + return edid;
>
> new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> if (!new)
> goto out;
> edid = new;
>
> - for (j = 1; j <= edid[0x7e]; j++) {
> - u8 *block = edid + j * EDID_LENGTH;
> + for (j = 1; j <= edid->extensions; j++) {
> + void *block = edid + j;
>
> for (i = 0; i < 4; i++) {
> if (get_edid_block(data, block, j, EDID_LENGTH))
> @@ -2026,13 +2025,13 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
> valid_extensions--;
> }
>
> - if (valid_extensions != edid[0x7e]) {
> - u8 *base;
> + if (valid_extensions != edid->extensions) {
> + struct edid *base;
This one points to extension blocks too so using
struct edid doesn't seem entirely appropriate.
>
> - connector_bad_edid(connector, edid, edid[0x7e] + 1);
> + connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
>
> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> - edid[0x7e] = valid_extensions;
> + edid->checksum += edid->extensions - valid_extensions;
> + edid->extensions = valid_extensions;
>
> new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
> GFP_KERNEL);
> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
> goto out;
>
> base = new;
> - for (i = 0; i <= edid[0x7e]; i++) {
> - u8 *block = edid + i * EDID_LENGTH;
> + for (i = 0; i <= edid->extensions; i++) {
> + void *block = edid + i;
Hmm. This code seems very broken to me. We read each block
into its expected offset based on the original base block extension
count, but here we only iterate up to the new ext block count. So
if we had eg. a 4 block EDID where block 2 is busted, we set
the new ext count to 2, copy over blocks 0 and 1, skip block 2,
and then terminate the loop. So instead of copying block 3 from
the orignal EDID into block 2 of the new EDID, we leave the
original garbage block 2 in place.
Also this memcpy() business seems entirely poinless in the sense
that we could just read each ext block into the final offset
directly AFAICS.
>
> if (!drm_edid_block_valid(block, i, false, NULL))
> continue;
>
> memcpy(base, block, EDID_LENGTH);
> - base += EDID_LENGTH;
> + base++;
> }
>
> kfree(edid);
> edid = new;
> }
>
> - return (struct edid *)edid;
> + return edid;
>
> out:
> kfree(edid);
> --
> 2.30.2
--
Ville Syrjälä
Intel
Re: [PATCH 2/8] drm: Rename dp/ to display/
On Tue, Mar 22, 2022 at 3:28 PM Thomas Zimmermann wrote: > > Rename dp/ to display/ to account for additional display-related > helpers, such as HDMI. Update all related include statements. No > functional changes. > > Various drivers, such as i915 and amdgpu, use similar naming scheme > by putting code for video-output standards into a local display/ > directory. The new directory's name is aligned with that policy. > > Signed-off-by: Thomas Zimmermann Reviewed-by: Alex Deucher
Re: [PATCH 0/2] remove DC_FP_* wrappers in dml files
On 03/30, Rodrigo Siqueira Jordao wrote: > > > On 2022-03-26 16:24, Melissa Wen wrote: > > From FPU documentation, developers must not use DC_FP_START/END in dml > > files, but invoke it when calling FPU-associated functions (isolated in > > dml folder). Therefore, the first patch renames dcn10_validate_bandwidth > > in dml/calcs to dcn_ for generalization, declares dcn10_validate_bandwidth > > in dcn10 - that calls dcn_validate_bandwidth and wraps with DC_FP_* > > accordingly. The second patch removes invocations of DC_FP_* from dml > > files and properly wraps FPU functions in dc code outside dml folder. > > > > Melissa Wen (2): > >drm/amd/display: detach fpu operations from dcn10_validate_bandwidth > > in calcs > >drm/amd/display: remove DC_FP_* wrapper from dml folder > > > > .../amd/display/dc/dcn10/dcn10_hw_sequencer.c | 10 -- > > .../drm/amd/display/dc/dcn10/dcn10_resource.c | 16 > > .../drm/amd/display/dc/dml/calcs/dcn_calcs.c | 19 +-- > > .../drm/amd/display/dc/dml/dcn20/dcn20_fpu.c | 2 -- > > .../gpu/drm/amd/display/dc/inc/dcn_calcs.h| 2 +- > > 5 files changed, 26 insertions(+), 23 deletions(-) > > > > > Hi, > > Thanks a lot for your patch! > > I reviewed and tested this series and lgtm. Applied to amd-staging-drm-next. > > Btw, I agree with Christian. Can you try to find a way to add a compilation > error or warning if the developer tries to add DC_FP_* inside DML? Yes, I agree too. I'll try to address it as soon as possible. > > Also, about the question of recursive calling to DC_FP_*, it should be safe > if using DC_FP_*. Thanks, Melissa > > Thanks > Siqueira signature.asc Description: PGP signature
Re: [PATCH v9 00/23] drm/rockchip: RK356x VOP2 support
> Wiadomość napisana przez Sascha Hauer w dniu > 30.03.2022, o godz. 12:20: > > Does it change anything if you do a "modetest -s 69@67:1920x1080" before > starting the app? Or if you run "modetest -P 43@67:1920x1080@NV12" > before starting the app? Or other combinations thereof? So i tried following combinations 1. -boot -start app -stop app -modetest -s 69@67:1920x1080 -> ok -modetest -P 43@67:1920x1080@NV12 -> green screen 2. -boot -modetest -s 69@67:1920x1080 -> ok -modetest -P 43@67:1920x1080@NV12 -> green screen 3. -boot -modetest -s 69@67:1920x1080 -> ok -start app -playback in app -> green screen -stop app -modetest -P 43@67:1920x1080@NV12 -> green screen 4. -boot -modetest -s 69@67:1920x1080 -> ok -start app -playback in app -> green screen -stop app -modetest -s 69@67:1920x1080 -> ok -modetest -P 43@67:1920x1080@NV12 -> green screen br
Re: [PATCH v2] drm/amd/display: don't ignore alpha property on pre-multiplied mode
On 2022-03-30 08:30, Rodrigo Siqueira Jordao wrote:
>
>
> On 2022-03-29 16:18, Melissa Wen wrote:
>> "Pre-multiplied" is the default pixel blend mode for KMS/DRM, as
>> documented in supported_modes of drm_plane_create_blend_mode_property():
>> https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_blend.c
>>
>> In this mode, both 'pixel alpha' and 'plane alpha' participate in the
>> calculation, as described by the pixel blend mode formula in KMS/DRM
>> documentation:
>>
>> out.rgb = plane_alpha * fg.rgb +
>> (1 - (plane_alpha * fg.alpha)) * bg.rgb
>>
>> Considering the blend config mechanisms we have in the driver so far,
>> the alpha mode that better fits this blend mode is the
>> _PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN, where the value for global_gain
>> is the plane alpha (global_alpha).
>>
>> With this change, alpha property stops to be ignored. It also addresses
>> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1734
>>
>> v2:
>> * keep the 8-bit value for global_alpha_value (Nicholas)
>> * correct the logical ordering for combined global gain (Nicholas)
>> * apply to dcn10 too (Nicholas)
>>
>> Signed-off-by: Melissa Wen
>> ---
>> .../drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 14 +-
>> drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c | 14 +-
>> 2 files changed, 18 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
>> b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
>> index ad757b59e00e..b1034e6258c8 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c
>> @@ -2528,14 +2528,18 @@ void dcn10_update_mpcc(struct dc *dc, struct
>> pipe_ctx *pipe_ctx)
>> struct mpc *mpc = dc->res_pool->mpc;
>> struct mpc_tree *mpc_tree_params =
>> &(pipe_ctx->stream_res.opp->mpc_tree_params);
>> - if (per_pixel_alpha)
>> - blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
>> - else
>> - blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>> -
>> blnd_cfg.overlap_only = false;
>> blnd_cfg.global_gain = 0xff;
>> + if (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) {
>> + blnd_cfg.alpha_mode =
>> MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
>> + blnd_cfg.global_gain = pipe_ctx->plane_state->global_alpha_value;
>> + } else if (per_pixel_alpha) {
>> + blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
>> + } else {
>> + blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>> + }
>> +
>> if (pipe_ctx->plane_state->global_alpha)
>> blnd_cfg.global_alpha = pipe_ctx->plane_state->global_alpha_value;
>> else
>> diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>> b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>> index 4290eaf11a04..b627c41713cc 100644
>> --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>> +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c
>> @@ -2344,14 +2344,18 @@ void dcn20_update_mpcc(struct dc *dc, struct
>> pipe_ctx *pipe_ctx)
>> struct mpc *mpc = dc->res_pool->mpc;
>> struct mpc_tree *mpc_tree_params =
>> &(pipe_ctx->stream_res.opp->mpc_tree_params);
>> - if (per_pixel_alpha)
>> - blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
>> - else
>> - blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>> -
>> blnd_cfg.overlap_only = false;
>> blnd_cfg.global_gain = 0xff;
>> + if (per_pixel_alpha && pipe_ctx->plane_state->global_alpha) {
>> + blnd_cfg.alpha_mode =
>> MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA_COMBINED_GLOBAL_GAIN;
>> + blnd_cfg.global_gain = pipe_ctx->plane_state->global_alpha_value;
>> + } else if (per_pixel_alpha) {
>> + blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_PER_PIXEL_ALPHA;
>> + } else {
>> + blnd_cfg.alpha_mode = MPCC_ALPHA_BLEND_MODE_GLOBAL_ALPHA;
>> + }
>> +
>
> Hi Melissa,
>
> Thanks a lot for this patch. I run your patch in our CI, and everything looks
> good from the IGT test result.
>
> In this sense:
> Tested-by: Rodrigo Siqueira
>
> However, I think it is still necessary to have someone else review.
>
> Harry, Nick, or Zhan, what do you think about this change?
>
Looks good to me.
Reviewed-by: Harry Wentland
Harry
> Thanks
> Siqueira
>
>> if (pipe_ctx->plane_state->global_alpha)
>> blnd_cfg.global_alpha = pipe_ctx->plane_state->global_alpha_value;
>> else
>
Re: [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()
On Wed, 30 Mar 2022, Ville Syrjälä wrote:
> On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
>> Mixing u8 * and struct edid * is confusing, switch to the latter.
>>
>> Cc: Ville Syrjälä
>> Signed-off-by: Jani Nikula
>> ---
>> drivers/gpu/drm/drm_edid.c | 31 +++
>> 1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index d79b06f7f34c..0650b9217aa2 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct drm_connector
>> *connector,
>> void *data)
>> {
>> int i, j = 0, valid_extensions = 0;
>> -u8 *edid, *new;
>> -struct edid *override;
>> +struct edid *edid, *new, *override;
>>
>> override = drm_get_override_edid(connector);
>> if (override)
>> return override;
>>
>> -edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block,
>> data);
>> +edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
>> if (!edid)
>> return NULL;
>>
>> /* if there's no extensions or no connector, we're done */
>> -valid_extensions = edid[0x7e];
>> +valid_extensions = edid->extensions;
>> if (valid_extensions == 0)
>> -return (struct edid *)edid;
>> +return edid;
>>
>> new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>> if (!new)
>> goto out;
>> edid = new;
>>
>> -for (j = 1; j <= edid[0x7e]; j++) {
>> -u8 *block = edid + j * EDID_LENGTH;
>> +for (j = 1; j <= edid->extensions; j++) {
>> +void *block = edid + j;
>>
>> for (i = 0; i < 4; i++) {
>> if (get_edid_block(data, block, j, EDID_LENGTH))
>> @@ -2026,13 +2025,13 @@ struct edid *drm_do_get_edid(struct drm_connector
>> *connector,
>> valid_extensions--;
>> }
>>
>> -if (valid_extensions != edid[0x7e]) {
>> -u8 *base;
>> +if (valid_extensions != edid->extensions) {
>> +struct edid *base;
>
> This one points to extension blocks too so using
> struct edid doesn't seem entirely appropriate.
So I've gone back and forth with this. I think I want to get rid of u8*
no matter what, because it always requires casting. I've used void* here
and there to allow mixed use, internally in drm_edid.c while
transitioning, and in public interfaces due to usage all over the place.
OTOH I don't much like arithmetics on void*. It's a gcc extension.
struct edid * is useful for e.g. ->checksum and arithmetics. In many
places I've named it struct edid *block to distinguish. We could have a
struct edid_block too, which could have ->tag and ->checksum members,
for example, but then it would require casting or a function for "safe"
typecasting.
I've also gone back and forth with the helpers for getting a pointer to
a block. For usage like this, kind of need both const and non-const
versions. And, with the plans I have for future, I'm not sure I want to
promote any EDID parsing outside of drm_edid.c, so maybe they should be
static.
Undecided. C is a bit clunky here.
>
>>
>> -connector_bad_edid(connector, edid, edid[0x7e] + 1);
>> +connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
>>
>> -edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
>> -edid[0x7e] = valid_extensions;
>> +edid->checksum += edid->extensions - valid_extensions;
>> +edid->extensions = valid_extensions;
>>
>> new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
>> GFP_KERNEL);
>> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct drm_connector
>> *connector,
>> goto out;
>>
>> base = new;
>> -for (i = 0; i <= edid[0x7e]; i++) {
>> -u8 *block = edid + i * EDID_LENGTH;
>> +for (i = 0; i <= edid->extensions; i++) {
>> +void *block = edid + i;
>
> Hmm. This code seems very broken to me. We read each block
> into its expected offset based on the original base block extension
> count, but here we only iterate up to the new ext block count. So
> if we had eg. a 4 block EDID where block 2 is busted, we set
> the new ext count to 2, copy over blocks 0 and 1, skip block 2,
> and then terminate the loop. So instead of copying block 3 from
> the orignal EDID into block 2 of the new EDID, we leave the
> original garbage block 2 in place.
Ugh. I end up fixing this in the series, in "drm/edid: split out invalid
block filtering to a separate function", but I don't mention it
anywhere.
Looks like it's been broken for 5+ years since commit 14544d0937bf
("drm/edid: Only print the bad edid when aborting").
Which really makes you wonder about the usefulness of trying to "fix"
th
Re: [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()
On Wed, Mar 30, 2022 at 06:16:17PM +0300, Jani Nikula wrote:
> On Wed, 30 Mar 2022, Ville Syrjälä wrote:
> > On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
> >> Mixing u8 * and struct edid * is confusing, switch to the latter.
> >>
> >> Cc: Ville Syrjälä
> >> Signed-off-by: Jani Nikula
> >> ---
> >> drivers/gpu/drm/drm_edid.c | 31 +++
> >> 1 file changed, 15 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index d79b06f7f34c..0650b9217aa2 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct drm_connector
> >> *connector,
> >>void *data)
> >> {
> >>int i, j = 0, valid_extensions = 0;
> >> - u8 *edid, *new;
> >> - struct edid *override;
> >> + struct edid *edid, *new, *override;
> >>
> >>override = drm_get_override_edid(connector);
> >>if (override)
> >>return override;
> >>
> >> - edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block,
> >> data);
> >> + edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
> >>if (!edid)
> >>return NULL;
> >>
> >>/* if there's no extensions or no connector, we're done */
> >> - valid_extensions = edid[0x7e];
> >> + valid_extensions = edid->extensions;
> >>if (valid_extensions == 0)
> >> - return (struct edid *)edid;
> >> + return edid;
> >>
> >>new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
> >>if (!new)
> >>goto out;
> >>edid = new;
> >>
> >> - for (j = 1; j <= edid[0x7e]; j++) {
> >> - u8 *block = edid + j * EDID_LENGTH;
> >> + for (j = 1; j <= edid->extensions; j++) {
> >> + void *block = edid + j;
> >>
> >>for (i = 0; i < 4; i++) {
> >>if (get_edid_block(data, block, j, EDID_LENGTH))
> >> @@ -2026,13 +2025,13 @@ struct edid *drm_do_get_edid(struct drm_connector
> >> *connector,
> >>valid_extensions--;
> >>}
> >>
> >> - if (valid_extensions != edid[0x7e]) {
> >> - u8 *base;
> >> + if (valid_extensions != edid->extensions) {
> >> + struct edid *base;
> >
> > This one points to extension blocks too so using
> > struct edid doesn't seem entirely appropriate.
>
> So I've gone back and forth with this. I think I want to get rid of u8*
> no matter what, because it always requires casting. I've used void* here
> and there to allow mixed use, internally in drm_edid.c while
> transitioning, and in public interfaces due to usage all over the place.
>
> OTOH I don't much like arithmetics on void*. It's a gcc extension.
>
> struct edid * is useful for e.g. ->checksum and arithmetics. In many
> places I've named it struct edid *block to distinguish. We could have a
> struct edid_block too, which could have ->tag and ->checksum members,
> for example, but then it would require casting or a function for "safe"
> typecasting.
>
> I've also gone back and forth with the helpers for getting a pointer to
> a block. For usage like this, kind of need both const and non-const
> versions. And, with the plans I have for future, I'm not sure I want to
> promote any EDID parsing outside of drm_edid.c, so maybe they should be
> static.
>
> Undecided. C is a bit clunky here.
>
> >
> >>
> >> - connector_bad_edid(connector, edid, edid[0x7e] + 1);
> >> + connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
> >>
> >> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> >> - edid[0x7e] = valid_extensions;
> >> + edid->checksum += edid->extensions - valid_extensions;
> >> + edid->extensions = valid_extensions;
> >>
> >>new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
> >>GFP_KERNEL);
> >> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct drm_connector
> >> *connector,
> >>goto out;
> >>
> >>base = new;
> >> - for (i = 0; i <= edid[0x7e]; i++) {
> >> - u8 *block = edid + i * EDID_LENGTH;
> >> + for (i = 0; i <= edid->extensions; i++) {
> >> + void *block = edid + i;
> >
> > Hmm. This code seems very broken to me. We read each block
> > into its expected offset based on the original base block extension
> > count, but here we only iterate up to the new ext block count. So
> > if we had eg. a 4 block EDID where block 2 is busted, we set
> > the new ext count to 2, copy over blocks 0 and 1, skip block 2,
> > and then terminate the loop. So instead of copying block 3 from
> > the orignal EDID into block 2 of the new EDID, we leave the
> > original garbage block 2 in place.
>
> Ugh. I end up fixing this in the series, in "drm/edid: split out invalid
> block filtering to a separate function", but I don't mention it
>
Re: [PATCH] drm/i915/guc: Use iosys_map interface to update lrc_desc
Sorry, only just seen this patch.
Please do not do this!
The entire lrc_desc_pool entity is being dropped as part of the update
to GuC v70. That's why there was a recent patch set to significantly
re-organise how/where it is used. That patch set explicitly said - this
is all in preparation for removing the desc pool entirely.
Merging this change would just cause unnecessary churn and rebase
conflicts with the v70 update patches that I am working on. Please wait
until that lands and then see if there is anything left that you think
still needs to be updated.
John.
On 3/8/2022 08:47, Balasubramani Vivekanandan wrote:
This patch is continuation of the effort to move all pointers in i915,
which at any point may be pointing to device memory or system memory, to
iosys_map interface.
More details about the need of this change is explained in the patch
series which initiated this task
https://patchwork.freedesktop.org/series/99711/
This patch converts all access to the lrc_desc through iosys_map
interfaces.
Cc: Lucas De Marchi
Cc: John Harrison
Cc: Matthew Brost
Cc: Umesh Nerlige Ramappa
Signed-off-by: Balasubramani Vivekanandan
---
drivers/gpu/drm/i915/gt/uc/intel_guc.h| 2 +-
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 68 ---
2 files changed, 43 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
index e439e6c1ac8b..cbbc24dbaf0f 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
@@ -168,7 +168,7 @@ struct intel_guc {
/** @lrc_desc_pool: object allocated to hold the GuC LRC descriptor
pool */
struct i915_vma *lrc_desc_pool;
/** @lrc_desc_pool_vaddr: contents of the GuC LRC descriptor pool */
- void *lrc_desc_pool_vaddr;
+ struct iosys_map lrc_desc_pool_vaddr;
/**
* @context_lookup: used to resolve intel_context from guc_id, if a
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 9ec03234d2c2..84b17ded886a 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -467,13 +467,14 @@ static u32 *get_wq_pointer(struct guc_process_desc *desc,
return &__get_parent_scratch(ce)->wq[ce->parallel.guc.wqi_tail /
sizeof(u32)];
}
-static struct guc_lrc_desc *__get_lrc_desc(struct intel_guc *guc, u32 index)
+static void __write_lrc_desc(struct intel_guc *guc, u32 index,
+struct guc_lrc_desc *desc)
{
- struct guc_lrc_desc *base = guc->lrc_desc_pool_vaddr;
+ unsigned int size = sizeof(struct guc_lrc_desc);
GEM_BUG_ON(index >= GUC_MAX_CONTEXT_ID);
- return &base[index];
+ iosys_map_memcpy_to(&guc->lrc_desc_pool_vaddr, index * size, desc,
size);
}
static inline struct intel_context *__get_context(struct intel_guc *guc, u32 id)
@@ -489,20 +490,28 @@ static int guc_lrc_desc_pool_create(struct intel_guc *guc)
{
u32 size;
int ret;
+ void *addr;
size = PAGE_ALIGN(sizeof(struct guc_lrc_desc) *
GUC_MAX_CONTEXT_ID);
ret = intel_guc_allocate_and_map_vma(guc, size, &guc->lrc_desc_pool,
-(void
**)&guc->lrc_desc_pool_vaddr);
+&addr);
+
if (ret)
return ret;
+ if (i915_gem_object_is_lmem(guc->lrc_desc_pool->obj))
+ iosys_map_set_vaddr_iomem(&guc->lrc_desc_pool_vaddr,
+ (void __iomem *)addr);
+ else
+ iosys_map_set_vaddr(&guc->lrc_desc_pool_vaddr, addr);
+
return 0;
}
static void guc_lrc_desc_pool_destroy(struct intel_guc *guc)
{
- guc->lrc_desc_pool_vaddr = NULL;
+ iosys_map_clear(&guc->lrc_desc_pool_vaddr);
i915_vma_unpin_and_release(&guc->lrc_desc_pool, I915_VMA_RELEASE_MAP);
}
@@ -513,9 +522,11 @@ static inline bool guc_submission_initialized(struct intel_guc *guc)
static inline void _reset_lrc_desc(struct intel_guc *guc, u32 id)
{
- struct guc_lrc_desc *desc = __get_lrc_desc(guc, id);
+ unsigned int size = sizeof(struct guc_lrc_desc);
- memset(desc, 0, sizeof(*desc));
+ GEM_BUG_ON(id >= GUC_MAX_CONTEXT_ID);
+
+ iosys_map_memset(&guc->lrc_desc_pool_vaddr, id * size, 0, size);
}
static inline bool ctx_id_mapped(struct intel_guc *guc, u32 id)
@@ -2233,7 +2244,7 @@ static void prepare_context_registration_info(struct
intel_context *ce)
struct intel_engine_cs *engine = ce->engine;
struct intel_guc *guc = &engine->gt->uc.guc;
u32 ctx_id = ce->guc_id.id;
- struct guc_lrc_desc *desc;
+ struct guc_lrc_desc desc;
struct intel_context *child;
GEM_BUG_ON(!engine->mask);
@@ -2245,13 +2256,13 @@ static void prepare_context_regis
[PATCH v6 0/8] Add support for the eDP panel over aux_bus
This series adds support for generic eDP panel over aux_bus. These changes are dependent on the following series in order: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=620127&state=* https://patchwork.kernel.org/project/linux-arm-msm/list/?series=616587&state=* https://patchwork.kernel.org/project/linux-arm-msm/list/?series=613654&state=* Sankeerth Billakanti (8): drm/msm/dp: Add eDP support via aux_bus drm/msm/dp: wait for hpd high before aux transaction drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP drm/msm/dp: avoid handling masked interrupts drm/msm/dp: prevent multiple votes for dp resources drm/msm/dp: remove unnecessary delay during boot drm/msm/dp: Support edp/dp without hpd drm/msm/dp: Handle eDP mode_valid differently from dp drivers/gpu/drm/msm/dp/dp_aux.c | 13 - drivers/gpu/drm/msm/dp/dp_aux.h | 3 +- drivers/gpu/drm/msm/dp/dp_catalog.c | 35 ++--- drivers/gpu/drm/msm/dp/dp_catalog.h | 1 + drivers/gpu/drm/msm/dp/dp_display.c | 99 ++--- drivers/gpu/drm/msm/dp/dp_drm.c | 10 ++-- drivers/gpu/drm/msm/dp/dp_parser.c | 21 +--- drivers/gpu/drm/msm/dp/dp_parser.h | 1 + 8 files changed, 143 insertions(+), 40 deletions(-) -- 2.7.4
[PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
This patch adds support for generic eDP sink through aux_bus. The eDP/DP
controller driver should support aux transactions originating from the
panel-edp driver and hence should be initialized and ready.
The panel bridge supporting the panel should be ready before the bridge
connector is initialized. The generic panel probe needs the controller
resources to be enabled to support the aux transactions originating from
the panel probe.
Signed-off-by: Sankeerth Billakanti
---
Changes in v6:
- Remove initialization
- Fix aux_bus node leak
- Split the patches
drivers/gpu/drm/msm/dp/dp_display.c | 54 +++--
drivers/gpu/drm/msm/dp/dp_drm.c | 10 ---
drivers/gpu/drm/msm/dp/dp_parser.c | 21 +--
drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
4 files changed, 60 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 382b3aa..e082d02 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include "msm_drv.h"
#include "msm_kms.h"
@@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct
device *master,
goto end;
}
- dp->dp_display.next_bridge = dp->parser->next_bridge;
-
dp->aux->drm_dev = drm;
rc = dp_aux_register(dp->aux);
if (rc) {
@@ -1524,6 +1523,53 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display,
struct drm_minor *minor)
}
}
+static int dp_display_get_next_bridge(struct msm_dp *dp)
+{
+ int rc;
+ struct dp_display_private *dp_priv;
+ struct device_node *aux_bus;
+ struct device *dev;
+
+ dp_priv = container_of(dp, struct dp_display_private, dp_display);
+ dev = &dp_priv->pdev->dev;
+ aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
+
+ if (aux_bus) {
+ dp_display_host_init(dp_priv);
+ dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ enable_irq(dp_priv->irq);
+ dp_display_host_phy_init(dp_priv);
+
+ devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
+
+ disable_irq(dp_priv->irq);
+ of_node_put(aux_bus);
+ }
+
+ /*
+* External bridges are mandatory for eDP interfaces: one has to
+* provide at least an eDP panel (which gets wrapped into panel-bridge).
+*
+* For DisplayPort interfaces external bridges are optional, so
+* silently ignore an error if one is not present (-ENODEV).
+*/
+ rc = dp_parser_find_next_bridge(dp_priv->parser);
+ if (rc == -ENODEV) {
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
+ DRM_ERROR("eDP: next bridge is not present\n");
+ return rc;
+ }
+ } else if (rc) {
+ if (rc != -EPROBE_DEFER)
+ DRM_ERROR("DP: error parsing next bridge: %d\n", rc);
+ return rc;
+ }
+
+ dp->next_bridge = dp_priv->parser->next_bridge;
+
+ return 0;
+}
+
int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder)
{
@@ -1547,6 +1593,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display,
struct drm_device *dev,
dp_display->encoder = encoder;
+ ret = dp_display_get_next_bridge(dp_display);
+ if (ret)
+ return ret;
+
dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
if (IS_ERR(dp_display->bridge)) {
ret = PTR_ERR(dp_display->bridge);
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 7ce1aca..5254bd6 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp
*dp_display, struct drm_device *
bridge->funcs = &dp_bridge_ops;
bridge->type = dp_display->connector_type;
- bridge->ops =
- DRM_BRIDGE_OP_DETECT |
- DRM_BRIDGE_OP_HPD |
- DRM_BRIDGE_OP_MODES;
+ if (bridge->type == DRM_MODE_CONNECTOR_DisplayPort) {
+ bridge->ops =
+ DRM_BRIDGE_OP_DETECT |
+ DRM_BRIDGE_OP_HPD |
+ DRM_BRIDGE_OP_MODES;
+ }
rc = drm_bridge_attach(encoder, bridge, NULL,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (rc) {
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 1056b8d..6317dce 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser)
return 0;
}
-static int dp_parser_find_next_bridge(struct dp_parser *parser)
+int dp_parser_find_next_bridge(struct dp_parser
[PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction
The source device should ensure the sink is ready before proceeding to
read the sink capability or performing any aux transactions. The sink
will indicate its readiness by asserting the HPD line. The controller
driver needs to wait for the hpd line to be asserted by the sink before
performing any aux transactions.
The eDP sink is assumed to be always connected. It needs power from the
source and its HPD line will be asserted only after the panel is powered
on. The panel power will be enabled from the panel-edp driver and only
after that, the hpd line will be asserted.
Whereas for DP, the sink can be hotplugged and unplugged anytime. The hpd
line gets asserted to indicate the sink is connected and ready. Hence
there is no need to wait for the hpd line to be asserted for a DP sink.
Signed-off-by: Sankeerth Billakanti
---
Changes in v6:
- Wait for hpd high only for eDP
- Split into smaller patches
drivers/gpu/drm/msm/dp/dp_aux.c | 13 -
drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +
drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
5 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
index 6d36f63..a217c80 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.c
+++ b/drivers/gpu/drm/msm/dp/dp_aux.c
@@ -36,6 +36,7 @@ struct dp_aux_private {
bool initted;
u32 offset;
u32 segment;
+ bool is_edp;
struct drm_dp_aux dp_aux;
};
@@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
goto exit;
}
+ if (aux->is_edp) {
+ ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
+ if (ret) {
+ DRM_DEBUG_DP("Panel not ready for aux transactions\n");
+ goto exit;
+ }
+ }
+
dp_aux_update_offset_and_segment(aux, msg);
dp_aux_transfer_helper(aux, msg, true);
@@ -491,7 +500,8 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux)
drm_dp_aux_unregister(dp_aux);
}
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog)
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp)
{
struct dp_aux_private *aux;
@@ -506,6 +516,7 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct
dp_catalog *catalog)
init_completion(&aux->comp);
aux->cmd_busy = false;
+ aux->is_edp = is_edp;
mutex_init(&aux->mutex);
aux->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h b/drivers/gpu/drm/msm/dp/dp_aux.h
index 82afc8d..c99aeec 100644
--- a/drivers/gpu/drm/msm/dp/dp_aux.h
+++ b/drivers/gpu/drm/msm/dp/dp_aux.h
@@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux);
void dp_aux_deinit(struct drm_dp_aux *dp_aux);
void dp_aux_reconfig(struct drm_dp_aux *dp_aux);
-struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog);
+struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
+ bool is_edp);
void dp_aux_put(struct drm_dp_aux *aux);
#endif /*__DP_AUX_H_*/
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index fac815f..b6add4e 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -242,6 +242,19 @@ void dp_catalog_aux_update_cfg(struct dp_catalog
*dp_catalog)
phy_calibrate(phy);
}
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
+{
+ u32 state;
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ /* poll for hpd connected status every 2ms and timeout after 500ms */
+ return readl_poll_timeout(catalog->io->dp_controller.aux.base +
+ REG_DP_DP_HPD_INT_STATUS,
+ state, state & DP_DP_HPD_STATE_STATUS_CONNECTED,
+ 2000, 50);
+}
+
static void dump_regs(void __iomem *base, int len)
{
int i;
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h
b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 7dea101..45140a3 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -84,6 +84,7 @@ int dp_catalog_aux_clear_hw_interrupts(struct dp_catalog
*dp_catalog);
void dp_catalog_aux_reset(struct dp_catalog *dp_catalog);
void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
+int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
/* DP Controller APIs */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/dr
[PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
The panel-edp enables the eDP panel power during probe, get_modes
and enable. The eDP connect and disconnect interrupts for the eDP/DP
controller are directly dependent on panel power. As eDP display can be
assumed as always connected, the controller driver can skip the eDP
connect and disconnect interrupts. Any disruption in the link status
will be indicated via the IRQ_HPD interrupts.
So, the eDP controller driver can just enable the IRQ_HPD and replug
interrupts. The DP controller driver still needs to enable all the
interrupts.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 4
drivers/gpu/drm/msm/dp/dp_display.c | 24 ++--
2 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index b6add4e..3c16f95 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -582,10 +582,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
*dp_catalog)
u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
- /* enable HPD plug and unplug interrupts */
- dp_catalog_hpd_config_intr(dp_catalog,
- DP_DP_HPD_PLUG_INT_MASK | DP_DP_HPD_UNPLUG_INT_MASK, true);
-
/* Configure REFTIMER and enable it */
reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 274bbcf..888ff03 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -677,7 +677,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
dp_display_handle_plugged_change(&dp->dp_display, false);
/* enable HDP plug interrupt to prepare for next plugin */
- dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
+ if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+ dp_catalog_hpd_config_intr(dp->catalog,
DP_DP_HPD_PLUG_INT_MASK, true);
DRM_DEBUG_DP("After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
@@ -1087,10 +1088,17 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state,
struct msm_dp *dp)
static void dp_display_config_hpd(struct dp_display_private *dp)
{
-
dp_display_host_init(dp);
+
dp_catalog_ctrl_hpd_config(dp->catalog);
+ /* Enable plug and unplug interrupts only for external DisplayPort */
+ if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
+ if (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_DisplayPort)
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
+
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
* set sink to normal operation mode -- D0
@@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
return;
}
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
+ dp_hpd_plug_handle(dp_display, 0);
+
mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
@@ -1703,6 +1720,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
dp_display = container_of(dp, struct dp_display_private, dp_display);
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
+ dp_hpd_unplug_handle(dp_display, 0);
+
mutex_lock(&dp_display->event_mutex);
/* stop sentinel checking */
--
2.7.4
[PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
The interrupt register will still reflect the connect and disconnect
interrupt status without generating an actual HW interrupt.
The controller driver should not handle those masked interrupts.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 3c16f95..1809ce2 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog
*dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
- int isr = 0;
+ int isr, mask;
isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
(isr & DP_DP_HPD_INT_MASK));
+ mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
- return isr;
+ return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
}
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
--
2.7.4
[PATCH v6 5/8] drm/msm/dp: prevent multiple votes for dp resources
The aux_bus support with the dp_display driver will enable the dp
resources during msm_dp_modeset_init. The host_init has to return early
if the core is already initialized to prevent putting an additional vote
for the dp controller resources.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_display.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 888ff03..798b30b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -420,6 +420,11 @@ static void dp_display_host_init(struct dp_display_private
*dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
+ if (dp->core_initialized) {
+ DRM_DEBUG_DP("DP core already initialized\n");
+ return;
+ }
+
dp_power_init(dp->power, false);
dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
dp_aux_init(dp->aux);
@@ -432,6 +437,11 @@ static void dp_display_host_deinit(struct
dp_display_private *dp)
dp->dp_display.connector_type, dp->core_initialized,
dp->phy_initialized);
+ if (!dp->core_initialized) {
+ DRM_DEBUG_DP("DP core not initialized\n");
+ return;
+ }
+
dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
dp_aux_deinit(dp->aux);
dp_power_deinit(dp->power);
--
2.7.4
[PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot
Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP event. Signed-off-by: Sankeerth Billakanti --- drivers/gpu/drm/msm/dp/dp_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 798b30b..8bafdd0 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1526,7 +1526,7 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display) dp_hpd_event_setup(dp); - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0); } void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) -- 2.7.4
[PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd
Some eDP sinks or platform boards will not support hpd.
This patch adds support for those cases.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 1809ce2..8f1fc71 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -244,10 +244,17 @@ void dp_catalog_aux_update_cfg(struct dp_catalog
*dp_catalog)
int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
{
- u32 state;
+ u32 state, hpd_en;
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
+ hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL);
+ hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
+
+ /* no-hpd case */
+ if (!hpd_en)
+ return 0;
+
/* poll for hpd connected status every 2ms and timeout after 500ms */
return readl_poll_timeout(catalog->io->dp_controller.aux.base +
REG_DP_DP_HPD_INT_STATUS,
@@ -586,8 +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
*dp_catalog)
reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
- /* Enable HPD */
- dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
+ /* Enable HPD if supported*/
+ if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))
+ dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
+ DP_DP_HPD_CTRL_HPD_EN);
}
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
--
2.7.4
[PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
The panel-edp driver modes needs to be validated differently from DP
because the link capabilities are not available for EDP by that time.
Signed-off-by: Sankeerth Billakanti
---
drivers/gpu/drm/msm/dp/dp_display.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 8bafdd0..f9c7d9a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1003,6 +1003,12 @@ enum drm_mode_status dp_bridge_mode_valid(struct
drm_bridge *bridge,
return -EINVAL;
}
+ if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
+ if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
+ return MODE_CLOCK_HIGH;
+ return MODE_OK;
+ }
+
if ((dp->max_pclk_khz <= 0) ||
(dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
(mode->clock > dp->max_pclk_khz))
--
2.7.4
[PATCH] drm/panel-edp: Fix AUO B133UAN01 panel id
Two digits are swapped in the AUO B133UAN01 panel id (0x8495 instead
of 0x8594). This went initially unnoticed because the panel is still
detected, though it is set up with a conservative default timing. Fix
the digit swap.
Fixes: ec57376fba5a ("drm/panel-edp: Add AUO B133UAN01")
Signed-off-by: Matthias Kaehlcke
---
drivers/gpu/drm/panel/panel-edp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c
b/drivers/gpu/drm/panel/panel-edp.c
index db4eea1d7f67..1732b4f56e38 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1847,7 +1847,7 @@ static const struct panel_delay delay_100_500_e200 = {
static const struct edp_panel_entry edp_panels[] = {
EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay,
"B116XAK01"),
EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50,
"B116XAN06.1"),
- EDP_PANEL_ENTRY('A', 'U', 'O', 0x8495, &delay_200_500_e50,
"B133UAN01.0"),
+ EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50,
"B133UAN01.0"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x0786, &delay_200_500_p2e80,
"NV116WHM-T01"),
EDP_PANEL_ENTRY('B', 'O', 'E', 0x07d1, &boe_nv133fhm_n61.delay,
"NV133FHM-N61"),
--
2.35.1.1094.g7c7d902a7c-goog
Re: [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()
On Wed, 30 Mar 2022, Ville Syrjälä wrote:
> On Wed, Mar 30, 2022 at 06:16:17PM +0300, Jani Nikula wrote:
>> On Wed, 30 Mar 2022, Ville Syrjälä wrote:
>> > On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
>> >> Mixing u8 * and struct edid * is confusing, switch to the latter.
>> >>
>> >> Cc: Ville Syrjälä
>> >> Signed-off-by: Jani Nikula
>> >> ---
>> >> drivers/gpu/drm/drm_edid.c | 31 +++
>> >> 1 file changed, 15 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> >> index d79b06f7f34c..0650b9217aa2 100644
>> >> --- a/drivers/gpu/drm/drm_edid.c
>> >> +++ b/drivers/gpu/drm/drm_edid.c
>> >> @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct drm_connector
>> >> *connector,
>> >> void *data)
>> >> {
>> >> int i, j = 0, valid_extensions = 0;
>> >> - u8 *edid, *new;
>> >> - struct edid *override;
>> >> + struct edid *edid, *new, *override;
>> >>
>> >> override = drm_get_override_edid(connector);
>> >> if (override)
>> >> return override;
>> >>
>> >> - edid = (u8 *)drm_do_get_edid_base_block(connector, get_edid_block,
>> >> data);
>> >> + edid = drm_do_get_edid_base_block(connector, get_edid_block, data);
>> >> if (!edid)
>> >> return NULL;
>> >>
>> >> /* if there's no extensions or no connector, we're done */
>> >> - valid_extensions = edid[0x7e];
>> >> + valid_extensions = edid->extensions;
>> >> if (valid_extensions == 0)
>> >> - return (struct edid *)edid;
>> >> + return edid;
>> >>
>> >> new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH, GFP_KERNEL);
>> >> if (!new)
>> >> goto out;
>> >> edid = new;
>> >>
>> >> - for (j = 1; j <= edid[0x7e]; j++) {
>> >> - u8 *block = edid + j * EDID_LENGTH;
>> >> + for (j = 1; j <= edid->extensions; j++) {
>> >> + void *block = edid + j;
>> >>
>> >> for (i = 0; i < 4; i++) {
>> >> if (get_edid_block(data, block, j, EDID_LENGTH))
>> >> @@ -2026,13 +2025,13 @@ struct edid *drm_do_get_edid(struct drm_connector
>> >> *connector,
>> >> valid_extensions--;
>> >> }
>> >>
>> >> - if (valid_extensions != edid[0x7e]) {
>> >> - u8 *base;
>> >> + if (valid_extensions != edid->extensions) {
>> >> + struct edid *base;
>> >
>> > This one points to extension blocks too so using
>> > struct edid doesn't seem entirely appropriate.
>>
>> So I've gone back and forth with this. I think I want to get rid of u8*
>> no matter what, because it always requires casting. I've used void* here
>> and there to allow mixed use, internally in drm_edid.c while
>> transitioning, and in public interfaces due to usage all over the place.
>>
>> OTOH I don't much like arithmetics on void*. It's a gcc extension.
>>
>> struct edid * is useful for e.g. ->checksum and arithmetics. In many
>> places I've named it struct edid *block to distinguish. We could have a
>> struct edid_block too, which could have ->tag and ->checksum members,
>> for example, but then it would require casting or a function for "safe"
>> typecasting.
>>
>> I've also gone back and forth with the helpers for getting a pointer to
>> a block. For usage like this, kind of need both const and non-const
>> versions. And, with the plans I have for future, I'm not sure I want to
>> promote any EDID parsing outside of drm_edid.c, so maybe they should be
>> static.
>>
>> Undecided. C is a bit clunky here.
>>
>> >
>> >>
>> >> - connector_bad_edid(connector, edid, edid[0x7e] + 1);
>> >> + connector_bad_edid(connector, (u8 *)edid, edid->extensions + 1);
>> >>
>> >> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
>> >> - edid[0x7e] = valid_extensions;
>> >> + edid->checksum += edid->extensions - valid_extensions;
>> >> + edid->extensions = valid_extensions;
>> >>
>> >> new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
>> >> GFP_KERNEL);
>> >> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct drm_connector
>> >> *connector,
>> >> goto out;
>> >>
>> >> base = new;
>> >> - for (i = 0; i <= edid[0x7e]; i++) {
>> >> - u8 *block = edid + i * EDID_LENGTH;
>> >> + for (i = 0; i <= edid->extensions; i++) {
>> >> + void *block = edid + i;
>> >
>> > Hmm. This code seems very broken to me. We read each block
>> > into its expected offset based on the original base block extension
>> > count, but here we only iterate up to the new ext block count. So
>> > if we had eg. a 4 block EDID where block 2 is busted, we set
>> > the new ext count to 2, copy over blocks 0 and 1, skip block 2,
>> > and then terminate the loop. So instead of copying block 3 from
>> > the orignal EDID into block 2 of the new EDID, we leave the
>> > original garbage block 2 in place.
>>
>> Ugh. I end up fixing this
Re: [PATCH 1/1] drm/amdkfd: Create file descriptor after client is added to smi_clients list
Am 2022-03-30 um 03:51 schrieb Lee Jones:
This ensures userspace cannot prematurely clean-up the client before
it is fully initialised which has been proven to cause issues in the
past.
Cc: Felix Kuehling
Cc: Alex Deucher
Cc: "Christian König"
Cc: "Pan, Xinhui"
Cc: David Airlie
Cc: Daniel Vetter
Cc: amd-...@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Lee Jones
---
drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
index e4beebb1c80a2..c5d5398d45cbf 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
@@ -247,15 +247,6 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
return ret;
}
- ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
- O_RDWR);
- if (ret < 0) {
- kfifo_free(&client->fifo);
- kfree(client);
- return ret;
- }
- *fd = ret;
-
init_waitqueue_head(&client->wait_queue);
spin_lock_init(&client->lock);
client->events = 0;
@@ -265,5 +256,14 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t *fd)
list_add_rcu(&client->list, &dev->smi_clients);
spin_unlock(&dev->smi_lock);
+ ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
+ O_RDWR);
+ if (ret < 0) {
Thank you for the patch. This looks like the correct solution. But you
also need to remove the client from the dev->smi_clients list here
before kfree(client). With that fixed, the patch is
Reviewed-by: Felix Kuehling
+ kfifo_free(&client->fifo);
+ kfree(client);
+ return ret;
+ }
+ *fd = ret;
+
return 0;
}
Re: [PATCH] fbdev: defio: fix the pagelist corruption
[Cc: -jay...@intworks.biz as it bounces]
Dear Chuansheng,
Am 29.03.22 um 01:58 schrieb Liu, Chuansheng:
-Original Message-
From: Paul Menzel
Sent: Monday, March 28, 2022 2:15 PM
Am 28.03.22 um 02:58 schrieb Liu, Chuansheng:
-Original Message-
Sent: Saturday, March 26, 2022 4:11 PM
Am 17.03.22 um 06:46 schrieb Chuansheng Liu:
Easily hit the below list corruption:
==
list_add corruption. prev->next should be next (c0ceb090), but
was ec604507edc8. (prev=ec604507edc8).
WARNING: CPU: 65 PID: 3959 at lib/list_debug.c:26
__list_add_valid+0x53/0x80
CPU: 65 PID: 3959 Comm: fbdev Tainted: G U
RIP: 0010:__list_add_valid+0x53/0x80
Call Trace:
fb_deferred_io_mkwrite+0xea/0x150
do_page_mkwrite+0x57/0xc0
do_wp_page+0x278/0x2f0
__handle_mm_fault+0xdc2/0x1590
handle_mm_fault+0xdd/0x2c0
do_user_addr_fault+0x1d3/0x650
exc_page_fault+0x77/0x180
? asm_exc_page_fault+0x8/0x30
asm_exc_page_fault+0x1e/0x30
RIP: 0033:0x7fd98fc8fad1
==
Figure out the race happens when one process is adding &page->lru into
the pagelist tail in fb_deferred_io_mkwrite(), another process is
re-initializing the same &page->lru in fb_deferred_io_fault(), which is
not protected by the lock.
This fix is to init all the page lists one time during initialization,
it not only fixes the list corruption, but also avoids INIT_LIST_HEAD()
redundantly.
Fixes: 105a940416fc ("fbdev/defio: Early-out if page is already enlisted")
Cc: Thomas Zimmermann
Signed-off-by: Chuansheng Liu
---
drivers/video/fbdev/core/fb_defio.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index 98b0f23bf5e2..eafb66ca4f28 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -59,7 +59,6 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
printk(KERN_ERR "no mapping available\n");
BUG_ON(!page->mapping);
- INIT_LIST_HEAD(&page->lru);
page->index = vmf->pgoff;
vmf->page = page;
@@ -220,6 +219,8 @@ static void fb_deferred_io_work(struct work_struct *work)
void fb_deferred_io_init(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
+ struct page *page;
+ int i;
BUG_ON(!fbdefio);
mutex_init(&fbdefio->lock);
@@ -227,6 +228,12 @@ void fb_deferred_io_init(struct fb_info *info)
INIT_LIST_HEAD(&fbdefio->pagelist);
if (fbdefio->delay == 0) /* set a default of 1 s */
fbdefio->delay = HZ;
+
+ /* initialize all the page lists one time */
+ for (i = 0; i < info->fix.smem_len; i += PAGE_SIZE) {
+ page = fb_deferred_io_page(info, i);
+ INIT_LIST_HEAD(&page->lru);
+ }
}
EXPORT_SYMBOL_GPL(fb_deferred_io_init);
Applying your patch on top of current Linus’ master branch, tty0 is
unusable and looks frozen. Sometimes network card still works, sometimes
not.
I don't see how the patch would cause below BUG call stack, need some time to
debug. Just few comments:
1. Will the system work well without this patch?
Yes, the framebuffer works well without the patch.
2. When you are sure the patch causes the regression you saw, please get free
to submit one reverted patch, thanks : )
I think you for patch wasn’t submitted yet – at least not pulled by Linus.
The patch has been in drm-tip, could you have a try with the latest drm-tip to
see if the
Framebuffer works well, in that case, we could revert it in drm-tip then.
With drm-tip (drm-tip: 2022y-03m-29d-13h-14m-35s UTC integration
manifest) everything works fine. (I had to disable amdgpu driver, as it
failed to build.) Is anyone able to explain that?
Kind regards,
Paul
Re: [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()
On Wed, Mar 30, 2022 at 07:28:56PM +0300, Jani Nikula wrote:
> On Wed, 30 Mar 2022, Ville Syrjälä wrote:
> > On Wed, Mar 30, 2022 at 06:16:17PM +0300, Jani Nikula wrote:
> >> On Wed, 30 Mar 2022, Ville Syrjälä wrote:
> >> > On Tue, Mar 29, 2022 at 09:42:08PM +0300, Jani Nikula wrote:
> >> >> Mixing u8 * and struct edid * is confusing, switch to the latter.
> >> >>
> >> >> Cc: Ville Syrjälä
> >> >> Signed-off-by: Jani Nikula
> >> >> ---
> >> >> drivers/gpu/drm/drm_edid.c | 31 +++
> >> >> 1 file changed, 15 insertions(+), 16 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> >> index d79b06f7f34c..0650b9217aa2 100644
> >> >> --- a/drivers/gpu/drm/drm_edid.c
> >> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> >> @@ -1991,29 +1991,28 @@ struct edid *drm_do_get_edid(struct
> >> >> drm_connector *connector,
> >> >> void *data)
> >> >> {
> >> >> int i, j = 0, valid_extensions = 0;
> >> >> - u8 *edid, *new;
> >> >> - struct edid *override;
> >> >> + struct edid *edid, *new, *override;
> >> >>
> >> >> override = drm_get_override_edid(connector);
> >> >> if (override)
> >> >> return override;
> >> >>
> >> >> - edid = (u8 *)drm_do_get_edid_base_block(connector,
> >> >> get_edid_block, data);
> >> >> + edid = drm_do_get_edid_base_block(connector, get_edid_block,
> >> >> data);
> >> >> if (!edid)
> >> >> return NULL;
> >> >>
> >> >> /* if there's no extensions or no connector, we're done */
> >> >> - valid_extensions = edid[0x7e];
> >> >> + valid_extensions = edid->extensions;
> >> >> if (valid_extensions == 0)
> >> >> - return (struct edid *)edid;
> >> >> + return edid;
> >> >>
> >> >> new = krealloc(edid, (valid_extensions + 1) * EDID_LENGTH,
> >> >> GFP_KERNEL);
> >> >> if (!new)
> >> >> goto out;
> >> >> edid = new;
> >> >>
> >> >> - for (j = 1; j <= edid[0x7e]; j++) {
> >> >> - u8 *block = edid + j * EDID_LENGTH;
> >> >> + for (j = 1; j <= edid->extensions; j++) {
> >> >> + void *block = edid + j;
> >> >>
> >> >> for (i = 0; i < 4; i++) {
> >> >> if (get_edid_block(data, block, j, EDID_LENGTH))
> >> >> @@ -2026,13 +2025,13 @@ struct edid *drm_do_get_edid(struct
> >> >> drm_connector *connector,
> >> >> valid_extensions--;
> >> >> }
> >> >>
> >> >> - if (valid_extensions != edid[0x7e]) {
> >> >> - u8 *base;
> >> >> + if (valid_extensions != edid->extensions) {
> >> >> + struct edid *base;
> >> >
> >> > This one points to extension blocks too so using
> >> > struct edid doesn't seem entirely appropriate.
> >>
> >> So I've gone back and forth with this. I think I want to get rid of u8*
> >> no matter what, because it always requires casting. I've used void* here
> >> and there to allow mixed use, internally in drm_edid.c while
> >> transitioning, and in public interfaces due to usage all over the place.
> >>
> >> OTOH I don't much like arithmetics on void*. It's a gcc extension.
> >>
> >> struct edid * is useful for e.g. ->checksum and arithmetics. In many
> >> places I've named it struct edid *block to distinguish. We could have a
> >> struct edid_block too, which could have ->tag and ->checksum members,
> >> for example, but then it would require casting or a function for "safe"
> >> typecasting.
> >>
> >> I've also gone back and forth with the helpers for getting a pointer to
> >> a block. For usage like this, kind of need both const and non-const
> >> versions. And, with the plans I have for future, I'm not sure I want to
> >> promote any EDID parsing outside of drm_edid.c, so maybe they should be
> >> static.
> >>
> >> Undecided. C is a bit clunky here.
> >>
> >> >
> >> >>
> >> >> - connector_bad_edid(connector, edid, edid[0x7e] + 1);
> >> >> + connector_bad_edid(connector, (u8 *)edid,
> >> >> edid->extensions + 1);
> >> >>
> >> >> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> >> >> - edid[0x7e] = valid_extensions;
> >> >> + edid->checksum += edid->extensions - valid_extensions;
> >> >> + edid->extensions = valid_extensions;
> >> >>
> >> >> new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
> >> >> GFP_KERNEL);
> >> >> @@ -2040,21 +2039,21 @@ struct edid *drm_do_get_edid(struct
> >> >> drm_connector *connector,
> >> >> goto out;
> >> >>
> >> >> base = new;
> >> >> - for (i = 0; i <= edid[0x7e]; i++) {
> >> >> - u8 *block = edid + i * EDID_LENGTH;
> >> >> + for (i = 0; i <= edid->extensions; i++) {
> >> >> +
[PATCH] drm/edid: fix invalid EDID extension block filtering
The invalid EDID block filtering uses the number of valid EDID
extensions instead of all EDID extensions for looping the extensions in
the copy. This is fine, by coincidence, if all the invalid blocks are at
the end of the EDID. However, it's completely broken if there are
invalid extensions in the middle; the invalid blocks are included and
valid blocks are excluded.
Fix it by modifying the base block after, not before, the copy.
Fixes: 14544d0937bf ("drm/edid: Only print the bad edid when aborting")
Reported-by: Ville Syrjälä
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/drm_edid.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index d79b06f7f34c..8829120470ab 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2031,9 +2031,6 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
connector_bad_edid(connector, edid, edid[0x7e] + 1);
- edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
- edid[0x7e] = valid_extensions;
-
new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
GFP_KERNEL);
if (!new)
@@ -2050,6 +2047,9 @@ struct edid *drm_do_get_edid(struct drm_connector
*connector,
base += EDID_LENGTH;
}
+ new[EDID_LENGTH - 1] += new[0x7e] - valid_extensions;
+ new[0x7e] = valid_extensions;
+
kfree(edid);
edid = new;
}
--
2.30.2
Re: [Intel-gfx] [PATCH 01/12] drm/edid: use struct edid * in drm_do_get_edid()
On Wed, 30 Mar 2022, Ville Syrjälä wrote: > I'd fix this up front so we don't end having to backport the whole > thing if/when some security scan gizmo stumbles on this. Sent separately [1]. I'll rebase this series on top once that gets merged, but the conflict is trivial so I think the first round of review could go on here in parallel. BR, Jani. [1] https://patchwork.freedesktop.org/patch/msgid/20220330170426.349248-1-jani.nik...@intel.com -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/edid: fix invalid EDID extension block filtering
On Wed, Mar 30, 2022 at 08:04:26PM +0300, Jani Nikula wrote:
> The invalid EDID block filtering uses the number of valid EDID
> extensions instead of all EDID extensions for looping the extensions in
> the copy. This is fine, by coincidence, if all the invalid blocks are at
> the end of the EDID. However, it's completely broken if there are
> invalid extensions in the middle; the invalid blocks are included and
> valid blocks are excluded.
>
> Fix it by modifying the base block after, not before, the copy.
>
> Fixes: 14544d0937bf ("drm/edid: Only print the bad edid when aborting")
> Reported-by: Ville Syrjälä
> Signed-off-by: Jani Nikula
Reviewed-by: Ville Syrjälä
> ---
> drivers/gpu/drm/drm_edid.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index d79b06f7f34c..8829120470ab 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2031,9 +2031,6 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
>
> connector_bad_edid(connector, edid, edid[0x7e] + 1);
>
> - edid[EDID_LENGTH-1] += edid[0x7e] - valid_extensions;
> - edid[0x7e] = valid_extensions;
> -
> new = kmalloc_array(valid_extensions + 1, EDID_LENGTH,
> GFP_KERNEL);
> if (!new)
> @@ -2050,6 +2047,9 @@ struct edid *drm_do_get_edid(struct drm_connector
> *connector,
> base += EDID_LENGTH;
> }
>
> + new[EDID_LENGTH - 1] += new[0x7e] - valid_extensions;
> + new[0x7e] = valid_extensions;
> +
> kfree(edid);
> edid = new;
> }
> --
> 2.30.2
--
Ville Syrjälä
Intel
Re: [PATCH 3/9] drm/msm/gem: Split out inuse helper
On Wed, Mar 30, 2022 at 4:32 AM Dmitry Baryshkov
wrote:
>
> On Wed, 30 Mar 2022 at 02:00, Rob Clark wrote:
> >
> > From: Rob Clark
> >
> > Prep for a following patch. While we are at it, convert a few remaining
> > WARN_ON()s to GEM_WARN_ON().
>
> Well... GEM_WARN_ON doesn't really look like a 'while we are at it'.
> It might be better to split it into a separate commit.
it was a bit lazy.. I'll split it out
> >
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/msm_gem.c | 2 +-
> > drivers/gpu/drm/msm/msm_gem.h | 1 +
> > drivers/gpu/drm/msm/msm_gem_vma.c | 15 ++-
> > 3 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > index a4f61972667b..f96d1dc72021 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > @@ -938,7 +938,7 @@ void msm_gem_describe(struct drm_gem_object *obj,
> > struct seq_file *m,
> > name, comm ? ":" : "", comm ? comm : "",
> > vma->aspace, vma->iova,
> > vma->mapped ? "mapped" : "unmapped",
> > - vma->inuse);
> > + msm_gem_vma_inuse(vma));
> > kfree(comm);
> > }
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> > index 947ff7d9b471..1b7f0f0b88bf 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.h
> > +++ b/drivers/gpu/drm/msm/msm_gem.h
> > @@ -61,6 +61,7 @@ struct msm_gem_vma {
> > int msm_gem_init_vma(struct msm_gem_address_space *aspace,
> > struct msm_gem_vma *vma, int npages,
> > u64 range_start, u64 range_end);
> > +bool msm_gem_vma_inuse(struct msm_gem_vma *vma);
> > void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
> > struct msm_gem_vma *vma);
> > void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
> > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c
> > b/drivers/gpu/drm/msm/msm_gem_vma.c
> > index f914ddbaea89..dc2ae097805e 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> > @@ -37,6 +37,11 @@ msm_gem_address_space_get(struct msm_gem_address_space
> > *aspace)
> > return aspace;
> > }
> >
> > +bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
> > +{
> > + return !!vma->inuse;
> > +}
>
> It almost asks to be a static inline. The patch 04 makes it complex,
> so it's probably not that important.
yeah, that is the reason I didn't make it static inline
BR,
-R
> > +
> > /* Actually unmap memory for the vma */
> > void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
> > struct msm_gem_vma *vma)
> > @@ -44,7 +49,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space
> > *aspace,
> > unsigned size = vma->node.size << PAGE_SHIFT;
> >
> > /* Print a message if we try to purge a vma in use */
> > - if (WARN_ON(vma->inuse > 0))
> > + if (GEM_WARN_ON(msm_gem_vma_inuse(vma)))
> > return;
> >
> > /* Don't do anything if the memory isn't mapped */
> > @@ -61,7 +66,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space
> > *aspace,
> > void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
> > struct msm_gem_vma *vma)
> > {
> > - if (!WARN_ON(!vma->iova))
> > + if (!GEM_WARN_ON(!vma->iova))
> > vma->inuse--;
> > }
> >
> > @@ -73,7 +78,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
> > unsigned size = npages << PAGE_SHIFT;
> > int ret = 0;
> >
> > - if (WARN_ON(!vma->iova))
> > + if (GEM_WARN_ON(!vma->iova))
> > return -EINVAL;
> >
> > /* Increase the usage counter */
> > @@ -100,7 +105,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace,
> > void msm_gem_close_vma(struct msm_gem_address_space *aspace,
> > struct msm_gem_vma *vma)
> > {
> > - if (WARN_ON(vma->inuse > 0 || vma->mapped))
> > + if (GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped))
> > return;
> >
> > spin_lock(&aspace->lock);
> > @@ -120,7 +125,7 @@ int msm_gem_init_vma(struct msm_gem_address_space
> > *aspace,
> > {
> > int ret;
> >
> > - if (WARN_ON(vma->iova))
> > + if (GEM_WARN_ON(vma->iova))
> > return -EBUSY;
> >
> > spin_lock(&aspace->lock);
> > --
> > 2.35.1
> >
>
>
> --
> With best wishes
> Dmitry
Re: [PATCH 1/1] drm/amdkfd: Create file descriptor after client is added to smi_clients list
On Wed, 30 Mar 2022, Felix Kuehling wrote:
>
> Am 2022-03-30 um 03:51 schrieb Lee Jones:
> > This ensures userspace cannot prematurely clean-up the client before
> > it is fully initialised which has been proven to cause issues in the
> > past.
> >
> > Cc: Felix Kuehling
> > Cc: Alex Deucher
> > Cc: "Christian König"
> > Cc: "Pan, Xinhui"
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: amd-...@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Lee Jones
> > ---
> > drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c | 18 +-
> > 1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> > b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> > index e4beebb1c80a2..c5d5398d45cbf 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_smi_events.c
> > @@ -247,15 +247,6 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t
> > *fd)
> > return ret;
> > }
> > - ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
> > - O_RDWR);
> > - if (ret < 0) {
> > - kfifo_free(&client->fifo);
> > - kfree(client);
> > - return ret;
> > - }
> > - *fd = ret;
> > -
> > init_waitqueue_head(&client->wait_queue);
> > spin_lock_init(&client->lock);
> > client->events = 0;
> > @@ -265,5 +256,14 @@ int kfd_smi_event_open(struct kfd_dev *dev, uint32_t
> > *fd)
> > list_add_rcu(&client->list, &dev->smi_clients);
> > spin_unlock(&dev->smi_lock);
> > + ret = anon_inode_getfd(kfd_smi_name, &kfd_smi_ev_fops, (void *)client,
> > + O_RDWR);
> > + if (ret < 0) {
>
> Thank you for the patch. This looks like the correct solution. But you also
> need to remove the client from the dev->smi_clients list here before
> kfree(client). With that fixed, the patch is
Yes, that makes perfect sense.
> Reviewed-by: Felix Kuehling
Thanks Felix. I will provide a follow-up tomorrow.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
[PATCH] drm/msm/gpu: Avoid -Wunused-function with !CONFIG_PM_SLEEP
When building with CONFIG_PM=y and CONFIG_PM_SLEEP=n (such as ARCH=riscv
allmodconfig), the following warnings/errors occur:
drivers/gpu/drm/msm/adreno/adreno_device.c:679:12: error:
'adreno_system_resume' defined but not used [-Werror=unused-function]
679 | static int adreno_system_resume(struct device *dev)
|^~~~
drivers/gpu/drm/msm/adreno/adreno_device.c:655:12: error:
'adreno_system_suspend' defined but not used [-Werror=unused-function]
655 | static int adreno_system_suspend(struct device *dev)
|^
cc1: all warnings being treated as errors
These functions are only used in SET_SYSTEM_SLEEP_PM_OPS(), which
evaluates to empty when CONFIG_PM_SLEEP is not set, making these
functions unused.
Traditionally, these functions are marked as __maybe_unused but in this
case, there is already an '#ifdef CONFIG_PM' in the code, so just do the
same thing with CONFIG_PM_SLEEP to resolve the warning.
Fixes: 7e4167c9e021 ("drm/msm/gpu: Park scheduler threads for system suspend")
Signed-off-by: Nathan Chancellor
---
drivers/gpu/drm/msm/adreno/adreno_device.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c
b/drivers/gpu/drm/msm/adreno/adreno_device.c
index 661dfa7681fb..b25915230bab 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_device.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
@@ -621,6 +621,7 @@ static int adreno_runtime_suspend(struct device *dev)
return gpu->funcs->pm_suspend(gpu);
}
+#ifdef CONFIG_PM_SLEEP
static void suspend_scheduler(struct msm_gpu *gpu)
{
int i;
@@ -681,8 +682,8 @@ static int adreno_system_resume(struct device *dev)
resume_scheduler(dev_to_gpu(dev));
return pm_runtime_force_resume(dev);
}
-
-#endif
+#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
static const struct dev_pm_ops adreno_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(adreno_system_suspend, adreno_system_resume)
base-commit: 05241de1f69eb7f56b0a5e0bec96a7752fad1b2f
--
2.35.1
Re: [Linaro-mm-sig] [PATCH next] dma-buf/sync-file: do not allow zero size allocations
Hi Christian, On 3/30/22 10:09, Christian König wrote: That problem is already fixed with patch 21d139d73f77 dma-buf/sync-file: fix logic error in new fence merge code. Am 30.03.22 um 00:14 schrieb Pavel Skripkin: syzbot reported GPF in dma_fence_array_first(), which is caused by dereferencing ZERO_PTR in dma-buf internals. ZERO_PTR was generated in sync_file_merge(). This functuion tries to reduce allocation size, but does not check if it reducing to 0. This is actually perfectly ok. The code above should have prevented the size to become 0. Regards, Christian. Thanks for your reply! I see that 21d139d73f77 fixes GPF in dma_fence_array_first(), but what about this part: - if (num_fences > INT_MAX) + if (num_fences > INT_MAX || !num_fences) goto err_free_sync_file; fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); @@ -264,7 +264,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, if (index == 0) If num_fences is equal to zero then fences dereference will cause an oops. Or this one is also fixed in your tree? Thanks! With regards, Pavel Skripkin
Re: DRM Master ignoring hotplug event during display switching (QT)
On Wed, Mar 30, 2022 at 3:27 PM Daniel Vetter wrote: > > On Wed, Mar 30, 2022 at 10:52:54AM +0200, Maxime Ripard wrote: > > On Tue, Mar 29, 2022 at 11:38:32PM +0530, Jagan Teki wrote: > > > Hi all, > > > > > > I have implemented runtime display switching in the MIPI switch design > > > where LVDS and HDMI bridges are selected with the help of runtime > > > GPIO. > > > > > > Initial discussion on the same can be found here, > > > https://www.spinics.net/lists/dri-devel/msg318524.html > > > > > > The implementation has been done by creating each connector at > > > runtime. The default boot will create the LVDS connector and based on > > > the HDMI plug-in the ISR. > > > > > > 1. forcing the LVDS to disconnect > > > 2. call drm_kms_helper_hotplug_event > > > 3. detach the bridge chain > > > 4. attach the new bridge chain (HDMI) > > > 5. call drm_kms_helper_hotplug_event > > > > > > do the reverse when we unplug the HDMI cable. > > > > > > So, the bridge chains are attached and detached based on GPIO > > > Interrupt which is indeed identified based on the physical HDMIA > > > connector. > > > > > > Pipeline for LVDS, > > > mxfsb => nwl-dsi => display-switch => sn65dsi83=> panel-bridge > > > > > > Pipeline for HDMI, > > > mxfsb => nwl-dsi => display-switch => adv7511 => display-connector > > > > > > With this, implementation and I can able switch the displays with > > > default DRM (without specific DRM applications) where the LVDS is ON > > > by default and when HDMI plug-in the LVDS OFF/HDMI ON, and when HDMI > > > unplug the HDMI OFF/LVDS ON. > > > > > > However, with QT5 I can see the DRM Master ignoring hotplug event by > > > returning 0 on drm_master_internal_acquire in > > > drm_fb_helper_hotplug_event. With this the hotplug returned early so > > > it cannot able to disconnect and connect the new switching connector. > > > > > > Any help? > > > > I'm not sure why you started another discussion with pretty much the > > same content, but you can't rely on userspace handling the hotplug > > event. You'll have to deal with the case where it just doesn't. > > Well I missed the old thread, so I'm replying here. > > You should not handle this at all from a hotplug. Just to be clear. ISR is handling bridge detach and attach management and call hotplug not the hotplug don't know anything about bridges here. > > The way kms works is roughly as follows: > > 1. hw output state changes > 2. driver detects this (either through hpd interrupt or polling) > 3. driver sends out hotplug uevent > > That's it. Nothing else, no bridge rebinding, no link retaining is > required. > > Then either userspace or fbcon emulation reacts to this hotplug event by > doing an atomic modeset, where it hopefully disables the old output and > re-enables the new output. Your atomic_check needs to validate that > everything is all right (i.e. not enabling both at the same time). Does it mean the userspace knows when to disconnect and connect the LVDS or HDMI? What if display-switch ISR will disconnect LVDS and connect HDMI when HPD is On and connect LVDS and disconnect HDMI when HDP is Off of-course it makes only one enable at a time. > > Note that if you change stuff underneath, then that tends to seriously > upset atomic users. Also you should try to continue supporting at least > page flips with the wrong config, compositors otherwise tend to crash. > > This also means that if userspace doesn't handle hotplug events, then you > might end up with a black screen. That's ok. We try to avoid that when > it's practical (e.g. dp sst link retraining), but not when it's too hard > (dp mst hot-replug relies on userspace restoring everything). This is what I'm not sure about it as normal FB testing without any specific applications like QT - I can still see the switching works well without any issues. However, QT applications seem to control the hotplug by acquiring DRM master. is there any way from kernel side to ignore those application control over hotplug so that I can switch even the QT application as normal FB does? just to understand my testing and flow? > > Finally exchanging the bridge chain isn't supported, there's no locking > for that since it's assumed to be invariant over the lifetim of the > drm_device instance. The simplest way to make that happen right now is to > have 2 drm_encoder instances, one with the lvds bridge chain, the other > with the hdmi bridge chain, and select the right encoder/bridge chain > depending upon which output userspace picks. Does it mean to initialize to encoder instances and start attaching those to respective bridge pipelines? > > Also ofc your atomic_check needs to make sure that they're not both > enabled at the same time :-) > > I wouldn't try to make bridge chains exchangeable instead, that's > headaches - e.g. with dp mst we've also opted for a bunch of fake > drm_encoders to model that kind of switching. Can you link some references in the source tree for it to make a quick check? Thanks
[PATCH 3/4] drm: ssd130x: Support page addressing mode
From: Chen-Yu Tsai
On the SINO WEALTH SH1106, which is mostly compatible with the SSD1306,
only the basic page addressing mode is supported. This addressing mode
is not as easy to use compared to the currently supported horizontal
addressing mode, as the page address has to be set prior to writing
out each page, and each page must be written out separately as a result.
Also, there is no way to force the column address to wrap around early,
thus the column address must also be reset for each page to be accurate.
Add support for this addressing mode, with a flag to choose it. This
flag is designed to be set from the device info data structure, but
can be extended to be explicitly forced on through a device tree
property if such a need arises.
Signed-off-by: Chen-Yu Tsai
---
drivers/gpu/drm/solomon/ssd130x.c | 72 ---
drivers/gpu/drm/solomon/ssd130x.h | 2 +
2 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x.c
b/drivers/gpu/drm/solomon/ssd130x.c
index d08d86ef07bc..21040d8bf9d1 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -42,6 +42,8 @@
#define SSD130X_DATA 0x40
#define SSD130X_COMMAND0x80
+#define SSD130X_PAGE_COL_START_LOW 0x00
+#define SSD130X_PAGE_COL_START_HIGH0x10
#define SSD130X_SET_ADDRESS_MODE 0x20
#define SSD130X_SET_COL_RANGE 0x21
#define SSD130X_SET_PAGE_RANGE 0x22
@@ -61,6 +63,10 @@
#define SSD130X_SET_COM_PINS_CONFIG0xda
#define SSD130X_SET_VCOMH 0xdb
+#define SSD130X_PAGE_COL_START_MASKGENMASK(3, 0)
+#define SSD130X_PAGE_COL_START_SET(val)
FIELD_PREP(SSD130X_PAGE_COL_START_MASK, (val))
+#define SSD130X_START_PAGE_ADDRESS_MASKGENMASK(2, 0)
+#define SSD130X_START_PAGE_ADDRESS_SET(val)
FIELD_PREP(SSD130X_START_PAGE_ADDRESS_MASK, (val))
#define SSD130X_SET_SEG_REMAP_MASK GENMASK(0, 0)
#define SSD130X_SET_SEG_REMAP_SET(val)
FIELD_PREP(SSD130X_SET_SEG_REMAP_MASK, (val))
#define SSD130X_SET_COM_SCAN_DIR_MASK GENMASK(3, 3)
@@ -130,6 +136,7 @@ static int ssd130x_write_cmd(struct ssd130x_device
*ssd130x, int count,
return ret;
}
+/* Set address range for horizontal/vertical addressing modes */
static int ssd130x_set_col_range(struct ssd130x_device *ssd130x,
u8 col_start, u8 cols)
{
@@ -166,6 +173,26 @@ static int ssd130x_set_page_range(struct ssd130x_device
*ssd130x,
return 0;
}
+/* Set page and column start address for page addressing mode */
+static int ssd130x_set_page_pos(struct ssd130x_device *ssd130x,
+ u8 page_start, u8 col_start)
+{
+ int ret;
+ u32 page, col_low, col_high;
+
+ page = SSD130X_START_PAGE_ADDRESS |
+ SSD130X_START_PAGE_ADDRESS_SET(page_start);
+ col_low = SSD130X_PAGE_COL_START_LOW |
+ SSD130X_PAGE_COL_START_SET(col_start & 0xf);
+ col_high = SSD130X_PAGE_COL_START_HIGH |
+ SSD130X_PAGE_COL_START_SET((col_start >> 4) & 0xf);
+ ret = ssd130x_write_cmd(ssd130x, 3, page, col_low, col_high);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
{
struct device *dev = ssd130x->dev;
@@ -342,6 +369,11 @@ static int ssd130x_init(struct ssd130x_device *ssd130x)
}
}
+ /* Switch to page addressing mode */
+ if (ssd130x->page_address_mode)
+ return ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_ADDRESS_MODE,
+SSD130X_SET_ADDRESS_MODE_PAGE);
+
/* Switch to horizontal addressing mode */
return ssd130x_write_cmd(ssd130x, 2, SSD130X_SET_ADDRESS_MODE,
SSD130X_SET_ADDRESS_MODE_HORIZONTAL);
@@ -393,13 +425,16 @@ static int ssd130x_update_rect(struct ssd130x_device
*ssd130x, u8 *buf,
* (5) A4 B4 C4 D4 E4 F4 G4 H4
*/
- ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width);
- if (ret < 0)
- goto out_free;
+ if (!ssd130x->page_address_mode) {
+ /* Set address range for horizontal addressing mode */
+ ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x,
width);
+ if (ret < 0)
+ goto out_free;
- ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y / 8,
pages);
- if (ret < 0)
- goto out_free;
+ ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y
/ 8, pages);
+ if (ret < 0)
+ goto out_free;
+ }
for (i = y / 8; i < y / 8 + pages; i++) {
int m = 8;
@@ -418,9 +453,29 @@ static
