Re: MAINTAINERS: Update drm-misc entry to match all drivers
Hi, On 2023/9/19 21:12, Maxime Ripard wrote: We've had a number of times when a patch slipped through and we couldn't pick them up either because our MAINTAINERS entry only covers the framework and thus we weren't Cc'd. Let's take another approach where we match everything, and remove all the drivers that are not maintained through drm-misc. Signed-off-by: Maxime Ripard Acked-by: Jani Nikula --- MAINTAINERS | 23 --- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 90f13281d297..757d4f33e158 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -6860,12 +6860,29 @@ M: Thomas Zimmermann S:Maintained W:https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html T:git git://anongit.freedesktop.org/drm/drm-misc +F: Documentation/devicetree/bindings/display/ +F: Documentation/devicetree/bindings/gpu/ F:Documentation/gpu/ -F: drivers/gpu/drm/* +F: drivers/gpu/drm/ F:drivers/gpu/vga/ -F: include/drm/drm* +F: include/drm/drm F:include/linux/vga* -F: include/uapi/drm/drm* +F: include/uapi/drm/ +X: drivers/gpu/drm/amd/ +X: drivers/gpu/drm/armada/ +X: drivers/gpu/drm/etnaviv/ +X: drivers/gpu/drm/exynos/ +X: drivers/gpu/drm/gma500/ +X: drivers/gpu/drm/i915/ +X: drivers/gpu/drm/imx/ +X: drivers/gpu/drm/ingenic/ +X: drivers/gpu/drm/kmb/ +X: drivers/gpu/drm/mediatek/ +X: drivers/gpu/drm/msm/ +X: drivers/gpu/drm/nouveau/ +X: drivers/gpu/drm/radeon/ +X: drivers/gpu/drm/renesas/ +X: drivers/gpu/drm/tegra/ DRM DRIVERS FOR ALLWINNER A10 M:Maxime Ripard Nice patch! Well, I'm just curious about why the drm/ingenic and drm/gma500 are not maintained through drm-misc? As far as I know: 1) the drm/ingenic driver don't have a "T" annotation (location of the link). 2) the "T" of drm/gma500 is "git git://github.com/patjak/drm-gma500", but the code for this link is not up to date. I think at least the drm/ingenic and drm/gma500 drivers are *actually* maintained through drm-misc, So perhaps, these two drivers should not be excluded. Am I correct?
Re: [PATCH 0/5] Add the pci_get_base_class() helper and use it
Hi,
On 2023/8/25 21:18, Deucher, Alexander wrote:
[Public]
-Original Message-
From: amd-gfx On Behalf Of Sui
Jingfeng
Sent: Friday, August 25, 2023 2:27 AM
To: Bjorn Helgaas
Cc: alsa-de...@alsa-project.org; Sui Jingfeng ;
nouv...@lists.freedesktop.org; linux-ker...@vger.kernel.org; dri-
de...@lists.freedesktop.org; amd-...@lists.freedesktop.org; linux-
p...@vger.kernel.org
Subject: [PATCH 0/5] Add the pci_get_base_class() helper and use it
From: Sui Jingfeng
There is no function that can be used to get all PCI(e) devices in a system by
matching against its the PCI base class code only, while keep the sub-class code
and the programming interface ignored. Therefore, add the
pci_get_base_class() function to suit the need.
For example, if an application want to process all PCI(e) display devices in a
system, it can achieve such goal by writing the code as following:
pdev = NULL;
do {
pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev);
if (!pdev)
break;
do_something_for_pci_display_device(pdev);
} while (1);
Sui Jingfeng (5):
PCI: Add the pci_get_base_class() helper
ALSA: hda/intel: Use pci_get_base_class() to reduce duplicated code
drm/nouveau: Use pci_get_base_class() to reduce duplicated code
drm/amdgpu: Use pci_get_base_class() to reduce duplicated code
drm/radeon: Use pci_get_base_class() to reduce duplicated code
Series is:
Reviewed-by: Alex Deucher
Thanks a lot.
What to do next then?
By the way, Bjorn, what's your opinion?
I'm ask because I don't know what to do next with this series.
As they belong to different system of Linux kernel,
the rest of patch (0002 ~ 0005) depend on the first one.
I think, merge the 0001-patch firstly, then wait it arrive at drm-misc, alsa
branch.
Or, to do something else?
Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers
Hi,
On 2023/9/15 21:44, Doug Anderson wrote:
Hi,
On Fri, Sep 15, 2023 at 2:11 AM suijingfeng wrote:
Hi,
On 2023/9/2 07:39, Douglas Anderson wrote:
Based on grepping through the source code these drivers appear to be
missing a call to drm_atomic_helper_shutdown() at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.
The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.
All of the drivers in this patch were fairly straightforward to fix
since they already had a call to drm_atomic_helper_shutdown() at
remove/unbind time but were just lacking one at system shutdown. The
only hitch is that some of these drivers use the component model to
register/unregister their DRM devices. The shutdown callback is part
of the original device. The typical solution here, based on how other
DRM drivers do this, is to keep track of whether the device is bound
based on drvdata. In most cases the drvdata is the drm_device, so we
can just make sure it is NULL when the device is not bound. In some
drivers, this required minor code changes. To make things simpler,
drm_atomic_helper_shutdown() has been modified to consider a NULL
drm_device as a noop in the patch ("drm/atomic-helper:
drm_atomic_helper_shutdown(NULL) should be a noop").
Suggested-by: Maxime Ripard
Signed-off-by: Douglas Anderson
---
I have just tested the whole series, thanks for the patch. For drm/loongson
only:
Reviewed-by: Sui Jingfeng
Tested-by: Sui Jingfeng
Thanks!
By the way, I add 'pr_info("lsdc_pci_shutdown\n");' into the
lsdc_pci_shutdown() function,
And seeing that lsdc_pci_shutdown() will be called when reboot and shutdown the
machine.
I did not witness something weird happen at present. As you have said, this is
useful for
drm panels drivers. But for the rest(drm/hibmc, drm/ast, drm/mgag200 and
drm/loongson etc)
drivers, you didn't mention what's the benefit for those drivers.
I didn't mention it because I have no idea! I presume that for
non-drm_panel use cases it's not a huge deal, otherwise it wouldn't
have been missing from so many drivers. Thus, my "one sentence" reason
for the non-drm_panel case is just "we should do this because the
documentation of the API says we should", which is already in the
commit message. ;-)
OK, this sound fine.
If you have a specific other benefit you'd like me to list then I'm happy to.
You should think about the answer of this question yourself.
But I will not object if you can't find one. OK. :-)
Probably, you can
mention it with at least one sentence at the next version. I also prefer to
alter the
lsdc_pci_shutdown() function as the following pattern:
static void lsdc_pci_shutdown(struct pci_dev *pdev)
{
struct drm_device *ddev = pci_get_drvdata(pdev);
drm_atomic_helper_shutdown(ddev);
}
I was hoping to land this patch without spinning it unless there's a
good reason. How strongly do you feel about needing to change the
above?
Not very strong, this version looks just fine.
I will not object if you keep it as is.
But I will also hear what the others reviewers say.
Thanks for the patch.
-Doug
Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers
Hi,
On 2023/9/2 07:39, Douglas Anderson wrote:
Based on grepping through the source code these drivers appear to be
missing a call to drm_atomic_helper_shutdown() at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.
The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.
All of the drivers in this patch were fairly straightforward to fix
since they already had a call to drm_atomic_helper_shutdown() at
remove/unbind time but were just lacking one at system shutdown. The
only hitch is that some of these drivers use the component model to
register/unregister their DRM devices. The shutdown callback is part
of the original device. The typical solution here, based on how other
DRM drivers do this, is to keep track of whether the device is bound
based on drvdata. In most cases the drvdata is the drm_device, so we
can just make sure it is NULL when the device is not bound. In some
drivers, this required minor code changes. To make things simpler,
drm_atomic_helper_shutdown() has been modified to consider a NULL
drm_device as a noop in the patch ("drm/atomic-helper:
drm_atomic_helper_shutdown(NULL) should be a noop").
Suggested-by: Maxime Ripard
Signed-off-by: Douglas Anderson
---
I have just tested the whole series, thanks for the patch. For drm/loongson
only:
Reviewed-by: Sui Jingfeng
Tested-by: Sui Jingfeng
By the way, I add 'pr_info("lsdc_pci_shutdown\n");' into the
lsdc_pci_shutdown() function,
And seeing that lsdc_pci_shutdown() will be called when reboot and shutdown the
machine.
I did not witness something weird happen at present. As you have said, this is
useful for
drm panels drivers. But for the rest(drm/hibmc, drm/ast, drm/mgag200 and
drm/loongson etc)
drivers, you didn't mention what's the benefit for those drivers. Probably, you
can
mention it with at least one sentence at the next version. I also prefer to
alter the
lsdc_pci_shutdown() function as the following pattern:
static void lsdc_pci_shutdown(struct pci_dev *pdev)
{
struct drm_device *ddev = pci_get_drvdata(pdev);
drm_atomic_helper_shutdown(ddev);
}
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/7 17:08, Christian König wrote: I strongly suggest that you just completely drop this here Drop this is OK, no problem. Then I will go to develop something else. This version is not intended to merge originally, as it's a RFC. Also, the core mechanism already finished, it is the first patch in this series. Things left are just policy (how to specify one and parse the kernel CMD line) and nothing interesting left. It is actually to fulfill my promise at V3 which is to give some examples as usage cases. and go into the AST driver and try to fix it. Well, someone tell me that this is well defined behavior yesterday, which imply that it is not a bug. I'm not going to fix a non-bug. But if thomas ask me to fix it, then I probably have to try to fix. But I suggest if things not broken, don't fix it. Otherwise this may incur more big trouble. For server's single display use case, it is good enough. Thanks.
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/7 20:43, Christian König wrote: Am 07.09.23 um 14:32 schrieb suijingfeng: Hi, On 2023/9/7 17:08, Christian König wrote: Well, I have over 25 years of experience with display hardware and what you describe here was never an issue. I want to give you an example to let you know more. I have a ASRock AD2550B-ITX board[1], When another discrete video card is mounted into it mini PCIe slot or PCI slot, The IGD cannot be the primary display adapter anymore. The display is totally black. I have try to draft a few trivial patch to help fix this[2]. And I want to use the IGD as primary, does this count as an issue? No, this is completely expected behavior and a limitation of the hardware design. As far as I know both AMD and Intel GPUs work the same here. Regards, Christian. [1] https://www.asrock.com/mb/Intel/AD2550-ITX/ [2] https://patchwork.freedesktop.org/series/123073/ Then, I'll give you another example, see below for elaborate description. I have one AMD BC160 GPU, see[1] to get what it looks like. The GPU don't has a display connector interface exported. It actually can be seen as a render-only GPU or compute class GPU for bitcoin. But the firmware of it still acclaim this GPU as VGA compatible. When mount this GPU onto motherboard, the system always select this GPU as primary. But this GPU can't be able to connect with a monitor. Under such a situation, modprobe.blacklist=amdgpu don't works either, because vgaarb always select this GPU as primary, this is a device-level decision. $ dmesg | grep vgaarb: [3.541405] pci :0c:00.0: vgaarb: BAR 0: [mem 0xa000-0xafff 64bit pref] contains firmware FB [0xa000-0xa02f] [3.901448] pci :05:00.0: vgaarb: setting as boot VGA device [3.905375] pci :05:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [3.905382] pci :0c:00.0: vgaarb: setting as boot VGA device (overriding previous) [3.909375] pci :0c:00.0: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none [3.913375] pci :0d:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [3.913377] vgaarb: loaded [ 13.513760] amdgpu :0c:00.0: vgaarb: deactivate vga console [ 19.020992] amdgpu :0c:00.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=none:owns=io+mem I'm using ubuntu 22.04 system, with ast.modeset=10 passed on the cmd line, I still be able to enter the graphics system. And views this GPU as a render-only GPU. Probably continue to examine what's wrong, except this, drm/amdgpu report " *ERROR* IB test failed on sdma0 (-110)" to me. Does this count as problem? Before I could find solution, I have keep this de-fact render only GPU mounted. Because I need recompile kennel module, install the kernel module and testing. All I need is a 2D video card to display something, ast drm is OK, despite simple. It suit the need for my daily usage with VIM, that's enough for me. Now, the real questions that I want ask is: 1) Does the fact that when the kernel driver module got blocked (by modprobe.blacklist=amdgpu), while the vgaarb still select it as primary which leave the X server crash there (because no kennel space driver loaded) count as a problem? 2) Does my approach that mounting another GPU as the primary display adapter, while its real purpose is to solving bugs and development for another GPU, count as a use case? $ cat demsg.txt | grep drm [ 10.099888] ACPI: bus type drm_connector registered [ 11.083920] etnaviv :0d:00.0: [drm] bind etnaviv-display, master name: :0d:00.0 [ 11.084106] [drm] Initialized etnaviv 1.3.0 20151214 for :0d:00.0 on minor 0 [ 13.301702] [drm] amdgpu kernel modesetting enabled. [ 13.359820] [drm] initializing kernel modesetting (NAVI12 0x1002:0x7360 0x1002:0x0A34 0xC7). [ 13.368246] [drm] register mmio base: 0xEB10 [ 13.372861] [drm] register mmio size: 524288 [ 13.380788] [drm] add ip block number 0 [ 13.385661] [drm] add ip block number 1 [ 13.390531] [drm] add ip block number 2 [ 13.395405] [drm] add ip block number 3 [ 13.399760] [drm] add ip block number 4 [ 13.404111] [drm] add ip block number 5 [ 13.408378] [drm] add ip block number 6 [ 13.413249] [drm] add ip block number 7 [ 13.433546] [drm] add ip block number 8 [ 13.433547] [drm] add ip block number 9 [ 13.497757] [drm] VCN decode is enabled in VM mode [ 13.502540] [drm] VCN encode is enabled in VM mode [ 13.508785] [drm] JPEG decode is enabled in VM mode [ 13.529596] [drm] vm size is 262144 GB, 4 levels, block size is 9-bit, fragment size is 9-bit [ 13.564762] [drm] Detected VRAM RAM=8176M, BAR=256M [ 13.569628] [drm] RAM width 2048bits HBM [ 13.574167] [drm] amdgpu: 8176M of VRAM memory ready [ 13.579125] [drm] amdgpu: 15998M of GTT memory ready. [ 13.584184] [drm] GART: num cpu pages 131072, num gpu pages 131072 [ 13.590505] [drm] PCIE GA
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/7 17:08, Christian König wrote: Well, I have over 25 years of experience with display hardware and what you describe here was never an issue. I want to give you an example to let you know more. I have a ASRock AD2550B-ITX board[1], When another discrete video card is mounted into it mini PCIe slot or PCI slot, The IGD cannot be the primary display adapter anymore. The display is totally black. I have try to draft a few trivial patch to help fix this[2]. And I want to use the IGD as primary, does this count as an issue? [1] https://www.asrock.com/mb/Intel/AD2550-ITX/ [2] https://patchwork.freedesktop.org/series/123073/
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/6 16:05, Thomas Zimmermann wrote: Hi Am 05.09.23 um 17:59 schrieb suijingfeng: [...] FYI: per-driver modeset parameters are deprecated and not to be used. Please don't promote them. Well, please wait, I want to explain. drm/nouveau already promote it a little bit. Despite no code of conduct or specification guiding how the modules parameters should be. Noticed that there already have a lot of DRM drivers support the modeset parameters, Please look at the history and discussion around this parameter. To my knowledge, 'modeset' got introduced when modesetting with still done in userspace. It was an easy way of disabling the kernel driver if the system's Xorg did no yet support kernel mode setting. Fast forward a few years and all Linux' use kernel modesetting, which make the modeset parameters obsolete. We discussed and decided to keep them in, because many articles and blog posts refer to them. We didn't want to invalidate them. BUT modeset is deprecated and not allowed in new code. If you look at existing modeset usage, you will eventually come across the comment at [1]. OK, no problem. I agree what you said. There's 'nomodeset', which disables all native drivers. It's useful for debugging or as a quick-fix if the graphics driver breaks. If you want to disable a specific driver, please use one of the options for blacklisting. Yeah, the 'nomodeset' disables all native drivers, this is a good point of it, but this is also the weak point of it. Sometimes, when you are developing a drm driver for a new device. You will see the pain. Its too often a programmer's modification make the entire Linux kernel hang there. The problematic drm driver kernel module already in the initrd. Then, the real need to disable the ill-functional drm driver kernel module only. While what you recommend to disable them all. There are subtle difference. Another limitation of the 'nomodeset' parameter is that it is only available on recent upstream kernel. Low version downstream kernel don't has this parameter supported yet. So this create inconstant developing experience. I believe that there always some people need do back-port and upstream work for various reasons. While (kindly, no offensive) debating, since we have the modprobe.blacklist why we still need the 'nomodeset' parameter ? why not try modprobe.blacklist="amdgpu,radeon,i915,ast,nouveau,gma500_gfx, ..." :-/ But OK in overall, I will listen to your advice. Best regards Thomas [1] https://elixir.bootlin.com/linux/v6.5/source/include/drm/drm_module.h#L83 for the modeset parameter, authors of various device driver try to make the usage not conflict with others. I believe that this is good thing for Linux users. It is probably the responsibility of the drm core maintainers to force various drm drivers to reach a minimal consensus. Probably it pains to do so and doesn't pay off. But reach a minimal consensus do benefit to Linux users. You can use modprobe.blacklist or initcall_blacklist on the kernel command line. There are some cases where the modprobe.blacklist doesn't works, I have come cross several time during the past. Because the device selected by the VGAARB is device-level thing, it is not the driver's problem. Sometimes when VGAARB has a bug, it will select a wrong device as primary. And the X server will use this wrong device as primary and completely crash there, due to lack a driver. Take my old S3 Graphics as an example: $ lspci | grep VGA 00:06.1 VGA compatible controller: Loongson Technology LLC DC (Display Controller) (rev 01) 03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Caicos XT [Radeon HD 7470/8470 / R5 235/310 OEM] 07:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01) 08:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01) Before apply this patch: [ 0.361748] pci :00:06.1: vgaarb: setting as boot VGA device [ 0.361753] pci :00:06.1: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none [ 0.361765] pci :03:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 0.361773] pci :07:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 0.361779] pci :08:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [ 0.361781] vgaarb: loaded [ 0.367838] pci :00:06.1: Overriding boot device as 1002:6778 [ 0.367841] pci :00:06.1: Overriding boot device as 5333:9070 [ 0.367843] pci :00:06.1: Overriding boot device as 5333:9070 For known reason, one of my system select the S3 Graphics as primary GPU. But this S3 Graphics not even have a decent drm upstream driver yet. Under such a case, I begin to believe that only the device who has a driver deserve the primary. Under such a condition, I want to reboot and enter the graphic environment with other working video cards. Eithe
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/6 14:45, Christian König wrote: Am 05.09.23 um 15:30 schrieb suijingfeng: Hi, On 2023/9/5 18:45, Thomas Zimmermann wrote: Hi Am 04.09.23 um 21:57 schrieb Sui Jingfeng: From: Sui Jingfeng On a machine with multiple GPUs, a Linux user has no control over which one is primary at boot time. This series tries to solve above mentioned If anything, the primary graphics adapter is the one initialized by the firmware. I think our boot-up graphics also make this assumption implicitly. Yes, but by the time of DRM drivers get loaded successfully,the boot-up graphics already finished. This is an incorrect assumption. drm_aperture_remove_conflicting_pci_framebuffers() and co don't kill the framebuffer, Well, my original description to this technique point is that 1) "Firmware framebuffer device already get killed by the drm_aperture_remove_conflicting_pci_framebuffers() function (or its siblings)" 2) "By the time of DRM drivers get loaded successfully, the boot-up graphics already finished." The word "killed" here is rough and coarse description about how does the drm device driver take over the firmware framebuffer. Since there seems have something obscure our communication, lets make the things clear. See below for more elaborate description. they just remove the current framebuffer driver to avoid further updates. This statement doesn't sound right, for UEFI environment, a correct description is that they remove the platform device, not the framebuffer driver. For the machines with the UEFI firmware, framebuffer driver here definitely refer to the efifb. The efifb still reside in the system(linux kernel). Please see the aperture_detach_platform_device() function in video/aperture.c So what happens (at least for amdgpu) is that we take over the framebuffer, This statement here is also not an accurate description. Strictly speaking, drm/amdgpu takes over the device (the VRAM hardware), not the framebuffer. The word "take over" here is also dubious, because drm/amdgpu takes over nothing. From the perspective of device-driver model, the GPU hardware *belongs* to the amdgpu drivers. Why you need to take over a thing originally and belong to you? If you could build the drm/amdgpu into the kernel and make it get loaded before the efifb. Then, there no need to use the firmware framebuffer ( the talking is limited to the display boot graphics purpose here). On such a case, the so-called "take over" will not happen. The truth is that the efifb create a platform device, which *occupy* part of the VRAM hardware resource. Thus, the efifb and the drm/amdgpu form the conflict. There are conflict because they share the same hardware resource. It is the hardware resources(address ranges) used by two different driver are conflict. Not the efifb driver itself conflict with drm/amdgpu driver. Thus, drm_aperture_remove_conflicting_xx() function have to kill one of the device are conflicting. Not to kill the driver. Therefore, the correct word would be the "reclaim". drm/amdgpu *reclaim* the hardware resource (vram address range) originally belong to you. The modeset state (including the framebuffer content) still reside in the amdgpu device. You just get the dirty framebuffer image in the framebuffer object. But the framebuffer object already dirty since it in the UEFI firmware stage. In conclusion, *reclaim* is more accurate than the "take over". And as far as I'm understanding, the drm/amdgpu take over nothing, no gains. Well, welcome to correct me if I'm wrong.
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/5 23:05, Thomas Zimmermann wrote: However, on modern Linux systems the primary display does not really exist. 'Primary' is the device that is available via VGA, VESA or EFI. I may miss the point, what do you means by choose the word "modern"? Are you trying to tell me that X server is too old and Wayland is the modern display server? Our drivers don't use these interfaces, but the native registers. Yes and no? Yes for the machine with the UEFI firmware, but I not sure if this statement is true for the machine with the legacy firmware. As the display controller in the ASpeed BMC is VGA compatible. Therefore, in theory, it should works with the VGA console on the machine with another VGA compatible video card. So the ast_vga_set_decode() function provided in the 0007 patch probably useful on legacy firmware environment. To be honest, I have tested this on various machine with UEFI firmware. But I didn't realized that I should do the testing on legacy firmware environment before sending this patch. It seems that the testing effort needed are quite exhausting, since all my machines come with the UEFI firmware. So is it OK to leave the legacy part to someone else who interested in it? Probably Alex is more professional at legacy VGA routing stuff? :-)
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
On 2023/9/5 23:05, Thomas Zimmermann wrote: Hi Am 05.09.23 um 15:30 schrieb suijingfeng: Hi, On 2023/9/5 18:45, Thomas Zimmermann wrote: Hi Am 04.09.23 um 21:57 schrieb Sui Jingfeng: From: Sui Jingfeng On a machine with multiple GPUs, a Linux user has no control over which one is primary at boot time. This series tries to solve above mentioned If anything, the primary graphics adapter is the one initialized by the firmware. I think our boot-up graphics also make this assumption implicitly. Yes, but by the time of DRM drivers get loaded successfully,the boot-up graphics already finished. Firmware framebuffer device already get killed by the drm_aperture_remove_conflicting_pci_framebuffers() function (or its siblings). So, this series is definitely not to interact with the firmware framebuffer Yes and no. The helpers you mention will attempt to remove the firmware framebuffer on the given PCI device. If you have multiple PCI devices, the other devices would not be affected. Yes and no. For the yes part: drm_aperture_remove_conflicting_pci_framebuffers() only kill the conflict one. But for a specific machine with the modern UEFI firmware, there should be only one firmware framebuffer driver. That shoudd be the EFIFB(UEFI GOP). I do have multiple PCI devices, but I don't understand when and why a system will have more than one firmware framebuffer. Even for the machines with the legacy BIOS, the fixed VGA aperture address range can only be owned by one firmware driver. It is just that we need to handle the routing, the ->set_decode() callback of vga_client_register() is used to do such work. Am I correct?
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/5 23:05, Thomas Zimmermann wrote: However, on modern Linux systems the primary display does not really exist. No, it do exist. X server need to know which one is the primary GPU. The '*' character at the of (4@0:0:0) PCI device is the Primary. The '*' denote primary, see the log below. (II) xfree86: Adding drm device (/dev/dri/card2) (II) xfree86: Adding drm device (/dev/dri/card0) (II) Platform probe for /sys/devices/pci:00/:00:1c.5/:003:00.0/:04:00.0/drm/card0 (II) xfree86: Adding drm device (/dev/dri/card3) (II) Platform probe for /sys/devices/pci:00/:00:1c.6/:005:00.0/drm/card3 (--) PCI: (0@0:2:0) 8086:3e91:8086:3e91 rev 0, Mem @ 0xdb00/16216, 0xa000/536870912, I/O @ 0xf000/64, BIOS @ 0x/131072 (--) PCI: (1@0:0:0) 1002:6771:1043:8636 rev 0, Mem @ 0xc000/2688435456, 0xdf22/131072, I/O @ 0xe000/256, BIOS @ 0x/131072 (--) PCI:*(4@0:0:0) 1a03:2000:1a03:2000 rev 48, Mem @ 0xde00/166777216, 0xdf02/131072, I/O @ 0xc000/128, BIOS @ 0x/131072 (--) PCI: (5@0:0:0) 10de:1288:174b:b324 rev 161, Mem @ 0xdc00/116777216, 0xd000/134217728, 0xd800/33554432, I/O @ 0xb000/128, BIOS @@0x/524288 The modesetting driver of X server will create framebuffer on the primary video adapter. If a 2D video adapter (like the aspeed BMC) is not the primary, then it probably will not be used. The only chance to be able to display something is to functional as a output slave. But the output slave technology need the PRIME support for cross driver buffer sharing. So, there do have some difference between the primary and non-primary video adapters. 'Primary' is the device that is available via VGA, VESA or EFI. Our drivers don't use these interfaces, but the native registers. As you said yourself, these firmware devices (VGA, VESA, EFI) are removed ASAP by the native drivers.
Re: [RFC,drm-misc-next v4 3/9] drm/radeon: Implement .be_primary() callback
Hi,
On 2023/9/5 13:50, Christian König wrote:
Am 04.09.23 um 21:57 schrieb Sui Jingfeng:
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over
which one
is primary at boot time.
Question is why is that useful? Should we give users the ability to
control that?
I don't see an use case for this.
On a specific machine with multiple GPUs mounted, only the
primary graphics get POST-ed (initialized) by the firmware.
Therefore the DRM drivers for the rest video cards have to
work without the prerequisite setups done by firmware, This
is called as POST.
One of the use cases is to test if a specific DRM driver
would works properly, under the circumstance of not being
POST-ed, The ast drm driver is the first one which refused
to work if not being POST-ed by the firmware.
Before apply this series, I was unable make drm/ast as the
primary video card easily. The problem is that on a multiple
video card configuration, the monitor connected with my
AST2400 card not light up. While confusing, a naive programmer
may suspect the PRIME is not working.
After applied this series and passing ast.modeset=10 on the
kernel cmd line, I found that the monitor connected with my
ast2400 video card still black, It doesn't display and It
doesn't show image to me.
While in the process of study drm/ast, I know that drm/ast
driver has the POST code shipped, See the ast_post_gpu() function.
Then, I was wondering why this function doesn't works.
After a short-time (hasty) debugging, I found that the ast_post_gpu()
function didn't get run. Because it have something to do with the
ast->config_mode. Without thinking too much, I hardcoded the
ast->config_mode as ast_use_p2a, the key point is to force the
ast_post_gpu() function to run.
```
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -132,6 +132,8 @@ static int ast_device_config_init(struct ast_device
*ast)
}
}
+ ast->config_mode = ast_use_p2a;
+
switch (ast->config_mode) {
case ast_use_defaults:
drm_info(dev, "Using default configuration\n");
```
Then, the monitor light up, it display the Ubuntu greeter to me. Therefore
my patch is useful, at least for the Linux drm driver tester and developer.
It allow programmers to test the specific part of a specific driver without
changing a line of the source code and without the need of sudo authority.
It improves the efficiency of the testing and patch verification. I know
the PrimaryGPU option of Xorg conf, but this approach will remember the
setup have been made, you need modify it with root authority each time
you want to switch the primary. But on the process of rapid developing
and/or testing for multiple video drivers, with only one computer hardware
resource available. What we really want is a one-shot command, as provided
by this series. So, this is the first use case.
The second use case is that sometime the firmware is not reliable.
While there are thousands of ARM64, PowerPC and Mips servers machine,
Most of them don't have a good UEFI firmware support. I haven't test the
drm/amdgpu and drm/radeon at my ARM64 server yet. Because this ARM64
server always use the platform(BMC) integrated display controller as primary.
The UEFI firmware of it does not provide options menu to tune.
So, for the first time, the discrete card because useless, despite more
powerful.
I will take time to carry on the testing, so I will be able to tell more
in the future.
Even on X86, when select the PEG as primary on the UEFI BIOS menu.
There is no way to tell the bios which one of my three
discrete video be the primary. Not to mention some old UEFI
firmware, which doesn't provide a setting at all.
While the benefit of my approach is the flexibility.
Yes the i915, amdgpu and radeon are good quality,
but there may have programmers want to try nouveau.
The third use case is that VGAARB is also not reliable, It will
select a wrong device as primary. Especially on Arm64, Loongarch
and mips arch etc. And the X server will use this wrong device
as primary and completely crash there. Either because of lacking
a driver or the driver has a bug which can not bear the graphic
environment up. VGAARB is firmware dependent.
My patch provide a temporary method to rescue.
The forth is probably the PRIME and reverse PRIME development
and developing driver for new video cards.
Re: [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi, On 2023/9/5 22:52, Alex Williamson wrote: On Tue, 5 Sep 2023 03:57:15 +0800 Sui Jingfeng wrote: From: Sui Jingfeng On a machine with multiple GPUs, a Linux user has no control over which one is primary at boot time. This series tries to solve above mentioned problem by introduced the ->be_primary() function stub. The specific device drivers can provide an implementation to hook up with this stub by calling the vga_client_register() function. Once the driver bound the device successfully, VGAARB will call back to the device driver. To query if the device drivers want to be primary or not. Device drivers can just pass NULL if have no such needs. Please note that: 1) The ARM64, Loongarch, Mips servers have a lot PCIe slot, and I would like to mount at least three video cards. 2) Typically, those non-86 machines don't have a good UEFI firmware support, which doesn't support select primary GPU as firmware stage. Even on x86, there are old UEFI firmwares which already made undesired decision for you. 3) This series is attempt to solve the remain problems at the driver level, while another series[1] of me is target to solve the majority of the problems at device level. Tested (limited) on x86 with four video card mounted, Intel UHD Graphics 630 is the default boot VGA, successfully override by ast2400 with ast.modeset=10 append at the kernel cmd line. $ lspci | grep VGA 00:02.0 VGA compatible controller: Intel Corporation CoffeeLake-S GT2 [UHD Graphics 630] In all my previous experiments with VGA routing and IGD I found that IGD can't actually release VGA routing and Intel confirmed the hardware doesn't have the ability to do so. It will always be primary from a VGA routing perspective. Was this actually tested with non-UEFI? Yes, I have tested on my aspire e471 notebook (i5 5200U), because that notebook using legacy firmware (also have UEFI, double firmware). But this machine have difficult in install ubuntu under UEFI firmware in the past. So I keep it using the legacy firmware. It have two video card, IGD and nvidia video card(GFORCE 840M). nvidia call its video card as 3D controller (pci->class = 0x030200) I have tested this patch and another patch mention at [1] together. I can tell you that the firmware framebuffer of this notebook using vesafb, not efifb. And the framebuffer size (lfb.size) is very small. This is very strange, but I don't have enough time to look in details. But still works. I'm using and tesing my patch whenever and wherever possible. I suspect it might only work in UEFI mode where we probably don't actually have a dependency on VGA routing. This is essentially why vfio requires UEFI ROMs when assigning GPUs to VMs, VGA routing is too broken to use on Intel systems with IGD. Thanks, What you tell me here is the side effect come with the VGA-compatible, but I'm focus on the arbitration itself. I think there no need to keep the VGA routing hardware features nowadays except that hardware vendor want keep the backward compatibility and/or comply the PCI VGA compatible spec. Alex
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
On 2023/9/5 18:49, Thomas Zimmermann wrote: Hi Am 04.09.23 um 21:57 schrieb Sui Jingfeng: From: Sui Jingfeng On a machine with multiple GPUs, a Linux user has no control over which one is primary at boot time. This series tries to solve above mentioned problem by introduced the ->be_primary() function stub. The specific device drivers can provide an implementation to hook up with this stub by calling the vga_client_register() function. Once the driver bound the device successfully, VGAARB will call back to the device driver. To query if the device drivers want to be primary or not. Device drivers can just pass NULL if have no such needs. Please note that: 1) The ARM64, Loongarch, Mips servers have a lot PCIe slot, and I would like to mount at least three video cards. 2) Typically, those non-86 machines don't have a good UEFI firmware support, which doesn't support select primary GPU as firmware stage. Even on x86, there are old UEFI firmwares which already made undesired decision for you. 3) This series is attempt to solve the remain problems at the driver level, while another series[1] of me is target to solve the majority of the problems at device level. Tested (limited) on x86 with four video card mounted, Intel UHD Graphics 630 is the default boot VGA, successfully override by ast2400 with ast.modeset=10 append at the kernel cmd line. FYI: per-driver modeset parameters are deprecated and not to be used. Please don't promote them. Well, please wait, I want to explain. drm/nouveau already promote it a little bit. Despite no code of conduct or specification guiding how the modules parameters should be. Noticed that there already have a lot of DRM drivers support the modeset parameters, for the modeset parameter, authors of various device driver try to make the usage not conflict with others. I believe that this is good thing for Linux users. It is probably the responsibility of the drm core maintainers to force various drm drivers to reach a minimal consensus. Probably it pains to do so and doesn't pay off. But reach a minimal consensus do benefit to Linux users. You can use modprobe.blacklist or initcall_blacklist on the kernel command line. There are some cases where the modprobe.blacklist doesn't works, I have come cross several time during the past. Because the device selected by the VGAARB is device-level thing, it is not the driver's problem. Sometimes when VGAARB has a bug, it will select a wrong device as primary. And the X server will use this wrong device as primary and completely crash there, due to lack a driver. Take my old S3 Graphics as an example: $ lspci | grep VGA 00:06.1 VGA compatible controller: Loongson Technology LLC DC (Display Controller) (rev 01) 03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Caicos XT [Radeon HD 7470/8470 / R5 235/310 OEM] 07:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01) 08:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01) Before apply this patch: [0.361748] pci :00:06.1: vgaarb: setting as boot VGA device [0.361753] pci :00:06.1: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none [0.361765] pci :03:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [0.361773] pci :07:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [0.361779] pci :08:00.0: vgaarb: VGA device added: decodes=io+mem,owns=none,locks=none [0.361781] vgaarb: loaded [0.367838] pci :00:06.1: Overriding boot device as 1002:6778 [0.367841] pci :00:06.1: Overriding boot device as 5333:9070 [0.367843] pci :00:06.1: Overriding boot device as 5333:9070 For known reason, one of my system select the S3 Graphics as primary GPU. But this S3 Graphics not even have a decent drm upstream driver yet. Under such a case, I begin to believe that only the device who has a driver deserve the primary. Under such a condition, I want to reboot and enter the graphic environment with other working video cards. Either platform integrated and discrete GPU. This don't means I should compromise by un-mount the S3 graphics card from the motherboard, this also don't means that I should update my BIOS setting. As sometimes, the BIOS is more worse. With this series applied, all I need to do is to reboot the computer and pass a command line. By force override another video card (who has a decent driver support) as primary, I'm able to do the debugging under graphic environment. I would like to examine what's wrong with the vgaarb on a specific platform under X server graphic environment. Probably try compile a driver for this card and see it works, simply reboot without the need to change anything. It is so efficient. So this is probably the second usage of my patch. It hand the right of control back to the graphic developer.
Re: [Nouveau] [RFC, drm-misc-next v4 0/9] PCI/VGA: Allowing the user to select the primary video adapter at boot time
Hi,
On 2023/9/5 18:45, Thomas Zimmermann wrote:
Hi
Am 04.09.23 um 21:57 schrieb Sui Jingfeng:
From: Sui Jingfeng
On a machine with multiple GPUs, a Linux user has no control over which
one is primary at boot time. This series tries to solve above mentioned
If anything, the primary graphics adapter is the one initialized by
the firmware. I think our boot-up graphics also make this assumption
implicitly.
Yes, but by the time of DRM drivers get loaded successfully,the boot-up
graphics already finished.
Firmware framebuffer device already get killed by the
drm_aperture_remove_conflicting_pci_framebuffers()
function (or its siblings). So, this series is definitely not to interact with
the firmware framebuffer
(or more intelligent framebuffer drivers). It is for user space program, such
as X server and Wayland
compositor. Its for Linux user or drm drivers testers, which allow them to
direct graphic display server
using right hardware of interested as primary video card.
Also, I believe that X server and Wayland compositor are the best test examples.
If a specific DRM driver can't work with X server as a primary,
then there probably have something wrong.
But what's the use case for overriding this setting?
On a specific machine with multiple GPUs mounted,
only the primary graphics get POST-ed (initialized) by the firmware.
Therefore, the DRM drivers for the rest video cards, have to choose to
work without the prerequisite setups done by firmware, This is called as POST.
One of the use cases of this series is to test if a specific DRM driver could
works properly,
even though there is no prerequisite works have been done by firmware at all.
And it seems that the results is not satisfying in all cases.
drm/ast is the first drm drivers which refused to work if not being POST-ed by
the firmware.
Before apply this series, I was unable make drm/ast as the primary video card
easily. On a
multiple video card configuration, the monitor connected with the AST2400 not
light up.
While confusing, a naive programmer may suspect the PRIME is not working.
After applied this series and passing ast.modeset=10 on the kernel cmd line,
I found that the monitor connected with my ast2400 video card still black,
It doesn't display and doesn't show image to me.
While in the process of study drm/ast, I know that drm/ast driver has the POST
code shipped.
See the ast_post_gpu() function, then, I was wondering why this function
doesn't works.
After a short-time (hasty) debugging, I found that the the ast_post_gpu()
function
didn't get run. Because it have something to do with the ast->config_mode.
Without thinking too much, I hardcoded the ast->config_mode as ast_use_p2a to
force the ast_post_gpu() function get run.
```
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -132,6 +132,8 @@ static int ast_device_config_init(struct ast_device
*ast)
}
}
+ ast->config_mode = ast_use_p2a;
+
switch (ast->config_mode) {
case ast_use_defaults:
drm_info(dev, "Using default configuration\n");
```
Then, the monitor light up, it display the Ubuntu greeter to me.
Therefore, my patch is helpful, at lease for the Linux drm driver tester and
developer.
It allow programmers to test the specific part of the specific drive
without changing a line of the source code and without the need of sudo
authority.
It helps to improve efficiency of the testing and patch verification.
I know the PrimaryGPU option of Xorg conf, but this approach will remember the
setup
have been made, you need modify it with root authority each time you want to
switch
the primary. But on rapid developing and/or testing multiple video drivers, with
only one computer hardware resource available. What we really want probably is a
one-shoot command as this series provide.
So, this is the first use case. This probably also help to test full modeset,
PRIME and reverse PRIME on multiple video card machine.
Best regards
Thomas
Re: [PATCH] drm/gma500: Fix call trace when psb_gem_mm_init() fails
Hi, On 2023/8/25 14:54, Patrik Jakobsson wrote: On Fri, Jul 28, 2023 at 02:58:55AM +0800, Sui Jingfeng wrote: Because the gma_irq_install() is call after psb_gem_mm_init() function, when psb_gem_mm_init() fails, the interrupt line haven't been allocated. Yet the gma_irq_uninstall() is called in the psb_driver_unload() function without checking if checking the irq is registered or not. The calltrace is appended as following: [ 20.539253] ioremap memtype_reserve failed -16 [ 20.543895] gma500 :00:02.0: Failure to map stolen base. [ 20.565049] [ cut here ] [ 20.565066] Trying to free already-free IRQ 16 [ 20.565087] WARNING: CPU: 1 PID: 381 at kernel/irq/manage.c:1893 free_irq+0x209/0x370 [ 20.565316] CPU: 1 PID: 381 Comm: systemd-udevd Tainted: G C 6.5.0-rc1+ #368 [ 20.565329] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./IMB-140D Plus, BIOS P1.10 11/18/2013 [ 20.565338] RIP: 0010:free_irq+0x209/0x370 [ 20.565357] Code: 41 5d 41 5e 41 5f 5d 31 d2 89 d1 89 d6 89 d7 41 89 d1 c3 cc cc cc cc 8b 75 d0 48 c7 c7 e0 77 12 9f 4c 89 4d c8 e8 57 fe f4 ff <0f> 0b 48 8b 75 c8 4c 89 f7 e8 29 f3 f1 00 49 8b 47 40 48 8b 40 78 [ 20.565369] RSP: 0018:ae3b40733808 EFLAGS: 00010046 [ 20.565382] RAX: RBX: 9f8082bfe000 RCX: [ 20.565390] RDX: RSI: RDI: [ 20.565397] RBP: ae3b40733840 R08: R09: [ 20.565405] R10: R11: R12: 9f80871c3100 [ 20.565413] R13: 9f80835d3360 R14: 9f80835d32a4 R15: 9f80835d3200 [ 20.565424] FS: 7f13d36458c0() GS:9f813888() knlGS: [ 20.565434] CS: 0010 DS: ES: CR0: 80050033 [ 20.565441] CR2: 7f0d046f3f20 CR3: 06c8c000 CR4: 06e0 [ 20.565450] Call Trace: [ 20.565458] [ 20.565470] ? show_regs+0x72/0x90 [ 20.565488] ? free_irq+0x209/0x370 [ 20.565504] ? __warn+0x8d/0x160 [ 20.565520] ? free_irq+0x209/0x370 [ 20.565536] ? report_bug+0x1bb/0x1d0 [ 20.56] ? handle_bug+0x46/0x90 [ 20.565572] ? exc_invalid_op+0x19/0x80 [ 20.565587] ? asm_exc_invalid_op+0x1b/0x20 [ 20.565607] ? free_irq+0x209/0x370 [ 20.565625] ? free_irq+0x209/0x370 [ 20.565644] gma_irq_uninstall+0x15b/0x1e0 [gma500_gfx] [ 20.565728] psb_driver_unload+0x27/0x190 [gma500_gfx] [ 20.565800] psb_pci_probe+0x5d2/0x790 [gma500_gfx] [ 20.565873] local_pci_probe+0x48/0xb0 [ 20.565892] pci_device_probe+0xc8/0x280 [ 20.565912] really_probe+0x1d2/0x440 [ 20.565929] __driver_probe_device+0x8a/0x190 [ 20.565944] driver_probe_device+0x23/0xd0 [ 20.565957] __driver_attach+0x10f/0x220 [ 20.565971] ? __pfx___driver_attach+0x10/0x10 [ 20.565984] bus_for_each_dev+0x7a/0xe0 [ 20.566002] driver_attach+0x1e/0x30 [ 20.566014] bus_add_driver+0x127/0x240 [ 20.566029] driver_register+0x64/0x140 [ 20.566043] ? __pfx_psb_init+0x10/0x10 [gma500_gfx] [ 20.566111] __pci_register_driver+0x68/0x80 [ 20.566128] psb_init+0x2c/0xff0 [gma500_gfx] [ 20.566194] do_one_initcall+0x46/0x330 [ 20.566214] ? kmalloc_trace+0x2a/0xb0 [ 20.566233] do_init_module+0x6a/0x270 [ 20.566250] load_module+0x207f/0x23a0 [ 20.566278] init_module_from_file+0x9c/0xf0 [ 20.566293] ? init_module_from_file+0x9c/0xf0 [ 20.566315] idempotent_init_module+0x184/0x240 [ 20.566335] __x64_sys_finit_module+0x64/0xd0 [ 20.566352] do_syscall_64+0x59/0x90 [ 20.566366] ? ksys_mmap_pgoff+0x123/0x270 [ 20.566378] ? __secure_computing+0x9b/0x110 [ 20.566392] ? exit_to_user_mode_prepare+0x39/0x190 [ 20.566406] ? syscall_exit_to_user_mode+0x2a/0x50 [ 20.566420] ? do_syscall_64+0x69/0x90 [ 20.566433] ? do_syscall_64+0x69/0x90 [ 20.566445] ? do_syscall_64+0x69/0x90 [ 20.566458] entry_SYSCALL_64_after_hwframe+0x6e/0xd8 [ 20.566472] RIP: 0033:0x7f13d351ea3d [ 20.566485] Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c3 a3 0f 00 f7 d8 64 89 01 48 [ 20.566496] RSP: 002b:7ffe566c1fd8 EFLAGS: 0246 ORIG_RAX: 0139 [ 20.566510] RAX: ffda RBX: 55e66806eec0 RCX: 7f13d351ea3d [ 20.566519] RDX: RSI: 7f13d36d9441 RDI: 0010 [ 20.566527] RBP: 0002 R08: R09: 0002 [ 20.566535] R10: 0010 R11: 0246 R12: 7f13d36d9441 [ 20.566543] R13: 55e6681108c0 R14: 55e66805ba70 R15: 55e66819a9c0 [ 20.566559] [ 20.566566] ---[ end trace ]--- Signed-off-by: Sui Jingfeng Applied to drm-misc-next Thanks a lot. Thanks Patrik --- drivers/gpu/drm/gma500/psb_drv.h | 1 + drivers/gpu/drm/gma500/psb_irq.c | 5 + 2 files changed, 6
Re: [PATCH v2 00/11] Fix typos, comments and copyright
Hi,
Nice cleanup, thanks a lot.
On 2023/8/24 06:29, Bjorn Helgaas wrote:
On Wed, Aug 09, 2023 at 06:34:01AM +0800, Sui Jingfeng wrote:
From: Sui Jingfeng
v1:
* Various improve.
v2:
* More fixes, optimizations and improvements.
Sui Jingfeng (11):
PCI/VGA: Use unsigned type for the io_state variable
PCI: Add the pci_get_class_masked() helper
PCI/VGA: Deal with VGA class devices
I dropped these two patches, at least for now. There's no other use
of pci_get_class_masked(), and the VGA use seems to be mostly
optimization and possibly some behavior change that isn't 100% clear
yet. If it's important, we can look at it again later.
OK, no problem.
PCI/VGA: Drop the inline in the vga_update_device_decodes() function.
PCI/VGA: Move the new_state assignment out of the loop
PCI/VGA: Fix two typos in the comments of pci_notify()
PCI/VGA: vga_client_register() return -ENODEV on failure, not -1
PCI/VGA: Fix a typo to the comment of vga_default
PCI/VGA: Fix a typo to the comments in vga_str_to_iostate() function
PCI/VGA: Tidy up the code and comment format
PCI/VGA: Replace full MIT license text with SPDX identifier
drivers/pci/search.c | 30 ++
drivers/pci/vgaarb.c | 233 +
include/linux/pci.h| 7 ++
include/linux/vgaarb.h | 27 +
4 files changed, 185 insertions(+), 112 deletions(-)
I applied the rest of the patches on pci/vga for v6.5.
I updated the commit logs and tweaked some of the patches.
I squashed all the typo fixes together and added several more that I
had done previously but not posted. The diff between your original v2
posting and the current pci/vga branch is attached. Most of it is
additional typo fixes, but if you look closely you'll see:
- The omission of the pci_get_class_masked() patches (in
vga_arbiter_add_pci_device(), pci_notify(), vga_arb_device_init())
- The tweaks I did to:
vga_update_device_decodes()
vga_client_register()
vga_arbiter_notify_clients()
I dropped the Reviewed-bys from the typo fixes because I didn't want
to extend them to everything that got squashed together. Happy to add
them back if anybody wants to look again.
The branch is at
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/log/?h=vga
Bjorn
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index f1c15aea868b..b4c138a6ec02 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -334,36 +334,6 @@ struct pci_dev *pci_get_device(unsigned int vendor,
unsigned int device,
}
EXPORT_SYMBOL(pci_get_device);
-/**
- * pci_get_class_masked - begin or continue searching for a PCI device by
class and mask
- * @class: search for a PCI device with this class designation
- * @from: Previous PCI device found in search, or %NULL for new search.
- *
- * Iterates through the list of known PCI devices. If a PCI device is
- * found with a matching @class, the reference count to the device is
- * incremented and a pointer to its device structure is returned.
- * Otherwise, %NULL is returned.
- * A new search is initiated by passing %NULL as the @from argument.
- * Otherwise if @from is not %NULL, searches continue from next device
- * on the global list. The reference count for @from is always decremented
- * if it is not %NULL.
- */
-struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask,
-struct pci_dev *from)
-{
- struct pci_device_id id = {
- .vendor = PCI_ANY_ID,
- .device = PCI_ANY_ID,
- .subvendor = PCI_ANY_ID,
- .subdevice = PCI_ANY_ID,
- .class_mask = mask,
- .class = class,
- };
-
- return pci_get_dev_by_id(, from);
-}
-EXPORT_SYMBOL(pci_get_class_masked);
-
/**
* pci_get_class - begin or continue searching for a PCI device by class
* @class: search for a PCI device with this class designation
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index a2f6e0e6b634..5e6b1eb54c64 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: MIT
/*
- * vgaarb.c: Implements the VGA arbitration. For details refer to
+ * vgaarb.c: Implements VGA arbitration. For details refer to
* Documentation/gpu/vgaarbiter.rst
*
* (C) Copyright 2005 Benjamin Herrenschmidt
@@ -42,9 +42,9 @@ static void vga_arbiter_notify_clients(void);
struct vga_device {
struct list_head list;
struct pci_dev *pdev;
- unsigned int decodes; /* what does it decodes */
- unsigned int owns; /* what does it owns */
- unsigned int locks; /* what does it locks */
+ unsigned int decodes; /* what it decodes */
+ unsigned int owns; /* what it owns */
+ unsigned int locks; /* what it locks */
unsigned int io_lock_cnt; /* legacy IO lock
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/22 01:38, Bjorn Helgaas wrote:
On Fri, Aug 18, 2023 at 12:09:29PM +0800, suijingfeng wrote:
On 2023/8/18 06:08, Bjorn Helgaas wrote:
On Wed, Aug 16, 2023 at 06:05:27AM +0800, Sui Jingfeng wrote:
Currently, the vga_is_firmware_default() function only works on x86 and
ia64, it is a no-op on ARM, ARM64, PPC, RISC-V, etc. This patch completes
the implementation for the rest of the architectures. The added code tries
to identify the PCI(e) VGA device that owns the firmware framebuffer
before PCI resource reallocation happens.
As far as I can tell, this is basically identical to the existing
vga_is_firmware_default(), except that this patch funs that code as a
header fixup, so it happens before any PCI BAR reallocations happen.
Yes, what you said is right in overall.
But I think I should mention a few tiny points that make a difference.
1) My version is *less arch-dependent*
Of course. If we make the patch simple and the commit log simple by
removing extraneous details, this will all be obvious.
2) My version focus on the address in ranges, weaken the size parameter.
Which make the code easy to read and follow the canonical convention to
express the address range. while the vga_is_firmware_default() is not.
Whether it's start/size or start/end is a trivial question. We don't
need to waste time on it now.
3) A tiny change make a big difference.
The original vga_is_firmware_default() only works with the assumption
that the PCI resource reallocation won't happens. While I see no clue
that why this is true even on X86 and IA64. The original patch[1] not
mention this assumption explicitly.
[1] 86fd887b7fe3 ('vgaarb: Don't default exclusively to first video device with
mem+io')
That sounds like a good idea, because this is all based on the
framebuffer in screen_info, and screen_info was initialized before PCI
enumeration, and it certainly doesn't account for any BAR changes done
by the PCI core.
Yes.
So why would we keep vga_is_firmware_default() at all? If the header
fixup has already identified the firmware framebuffer, it seems
pointless to look again later.
It need another patch to do the cleanup work, while my patch just
add code to solve the real problem. It focus on provide a solution
for the architectures which have a decent way set up the
screen_info. Other things except that is secondary.
I don't want both mechanisms when only one of them is useful. PCI BAR
reassignment is completely fine, and keeping the assumption in
vga_is_firmware_default() that we can compare reassigned BAR values to
the pre-reassignment screen_info range is a trap that we should
remove.
OK,it's clear now. I know what to do next.
Thanks.
Bjorn
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/22 01:33, Bjorn Helgaas wrote:
On Fri, Aug 18, 2023 at 09:48:46AM +0800, suijingfeng wrote:
On 2023/8/18 06:08, Bjorn Helgaas wrote:
+ if (resource_type(res) != IORESOURCE_MEM)
+ continue;
+
+ if (!res->start || !res->end)
+ continue;
+
+ if (res->start <= fb_start && fb_end <= res->end) {
+ pdev_boot_vga = pdev;
+
+ vgaarb_info(>dev,
+ "BAR %d contains firmware FB\n", i);
Print the BAR with %pR and include the framebuffer region from
screen_info in the same format.
I do remember that you already told me to do this in V3, sorry for not
replying to you at V3. Most of the time, what you tell me is right.But here,
I think I need to explain. Because doing it that way will make the code line
too long,and it will exceed 80 characters in the column if we print too
much.
I believe that the vgaarb_info() at here is already the most compact and
simplest form. Printing the BAR with %pR is not absolute necessary, because
we can get the additional information by: $ lspci | grep VGA
$ dmesg | grep 05:00.0
$ dmesg | grep :03:00.0
$ dmesg | grep PCI
Fair enough. The BAR info is already there. But I don't think the
screen_info framebuffer data is in the dmesg log anywhere, and I think
that would be useful.
It's fine if dmesg lines or kernel printk lines exceed 80 columns.
Actually, I only add something that is absolute necessary.
Printing BAR with %pR and/or Printing the framebuffer region
is consider to only for *debugging* purpose.
I think printing the framebuffer region is important because it makes
it clear *why* we're selecting the device as the default VGA device.
It's more than just debugging; it helps make the system more
transparent and more understandable.
OK, I'm clear what to do next.
The printing will be added at the next version.
Bjorn
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi, On 2023/8/18 18:20, suijingfeng wrote: 1) The "weird" logic completely overrides whatever decision VGAARB ever made. It seems to say that the decision ever made by VGAARB is useless. Well, I think VGAARB shouldn't endure this; VGAARB has to be small. VGAARB have to be smart! The "weird logic" here refer to the weird pci_fixup_vgadev() function.
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi, On 2023/8/18 06:08, Bjorn Helgaas wrote: I guess the point here is that: - 03:00.0 BAR 0 is [mem 0xe005000-0xe005fff] - screen_info says the framebuffer is [mem 0xe005000-0xe005fff] (or part of it) - Therefore, we want 03:00.0 to be the default VGA - PCI core reassigns 03:00.0 BAR 0 to [mem 0xe003000-0xe003fff] - PCI core assigns a 00:06.1 BAR to contain [mem 0xe005000-0xe005fff] - vga_is_firmware_default() incorrectly decides 00:06.1 should be the default VGA because it has a BAR that contains the screen_info address range Is that right? Yes, The 00:06.1 is loongson integrated display controller, integrated in LS7A1000 North bridge. On loongarch, before apply apply any patch, VGAARB always select 00:06.1 as the default boot device. because it is enumerated first. It is always the first VGA compatible device found on our system. Because its PCI domain, bus, function number is smallest. And it own IO and MEM, so the 00:06.1 will always be selected as the default boot device. Even you plug a discrete GPU on the mother board. Therefore we need help the vga_is_firmware_default() to overriding previous. On a multiple GPU co-exist case (on loongson platform), if no "overriding previous" being printed. then there something wrong. On normal case, we need the discrete GPU overriding the integrated one. Because the discrete GPU is more powerful than the platform integrated. But what we want is let the VGAARB determine the primary GPU by referencing the firmware. If firmware put the firmware framebuffer in the VRAM of the integrated display card(00:06.1). then the integrated display card should be the primary GPU. If firmware put the firmware framebuffer in the VRAM of the discrete display card, then the discrete display card should be the primary GPU.
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/18 06:08, Bjorn Helgaas wrote:
+
+/*
+ * Identify the PCI VGA device that contains the firmware framebuffer
+ */
+static void pci_boot_vga_finder(struct pci_dev *pdev)
+{
+ resource_size_t fb_start;
+ resource_size_t fb_end;
+ unsigned int i;
+
+ /* Already found the pdev which has firmware framebuffer ownership */
+ if (pdev_boot_vga)
+ return;
+
+ if (!vga_arb_get_firmware_fb_range(_start, _end))
+ return;
+
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+ struct resource *res = >resource[i];
This is essentially identical to vga_is_firmware_default() so it
should look the same, i.e., it should use pci_dev_for_each_resource().
OK, will be fixed at the next version. Thanks.
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/18 06:08, Bjorn Helgaas wrote:
Please note that before apply this patch, vgaarb can not select the
right boot vga due to weird logic introduced with the commit
57fc7323a8e7c ("LoongArch: Add PCI controller support")
If we need this reference to 57fc7323a8e7c, we need more specifics
about what the "weird logic" is. pci_fixup_vgadev() is the only
obvious VGA connection, so I suppose it's related to that.
Sorry, re-reading your reviews,
I realized that I may miss the point, here, the sentence is not well phased,
Correct expression should be:
Please note that before apply this patch, there don't have a reasonable, system
level solution on loongarch.
VGAARB select a wrong device as primary GPU due to the pci_fixup_vgadev()
function
introduced with the commit 57fc7323a8e7c ("LoongArch: Add PCI controller
support").
Re: [v2,2/2] doc: uapi: Add document describing dma-buf semantics
Hi, On 2023/8/3 23:47, Daniel Stone wrote: Since there's a lot of confusion around this, document both the rules and the best practice around negotiating, allocating, importing, and Probably, best practices? using buffers when crossing context/process/device/subsystem boundaries. This ties up all of dma-buf, formats and modifiers, and their usage. Signed-off-by: Daniel Stone --- Documentation/driver-api/dma-buf.rst | 8 + Documentation/gpu/drm-uapi.rst| 7 + .../userspace-api/dma-buf-alloc-exchange.rst | 384 ++ Documentation/userspace-api/index.rst | 1 + 4 files changed, 400 insertions(+) create mode 100644 Documentation/userspace-api/dma-buf-alloc-exchange.rst v2: - Moved to general uAPI section, cross-referenced from dma-buf/DRM - Added Pekka's suggested glossary with some small changes - Cleanups and clarifications from Simon and James diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index 862dbc2759d0..0c153d79ccc4 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -22,6 +22,14 @@ interact with the three main primitives offered by dma-buf: allowing implicit (kernel-ordered) synchronization of work to preserve the illusion of coherent access + +Userspace API principles and use + + +For more details on how to design your subsystem's API for dma-buf use, please +see Documentation/userspace-api/dma-buf-alloc-exchange.rst. + + Shared DMA Buffers -- diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst index 65fb3036a580..eef5fd19bc92 100644 --- a/Documentation/gpu/drm-uapi.rst +++ b/Documentation/gpu/drm-uapi.rst @@ -486,3 +486,10 @@ and the CRTC index is its position in this array. .. kernel-doc:: include/uapi/drm/drm_mode.h :internal: + + +dma-buf interoperability + + +Please see Documentation/userspace-api/dma-buf-alloc-exchange.rst for +information on how dma-buf is integrated and exposed within DRM. diff --git a/Documentation/userspace-api/dma-buf-alloc-exchange.rst b/Documentation/userspace-api/dma-buf-alloc-exchange.rst new file mode 100644 index ..090453d2ad78 --- /dev/null +++ b/Documentation/userspace-api/dma-buf-alloc-exchange.rst @@ -0,0 +1,384 @@ +.. Copyright 2021-2023 Collabora Ltd. + + +Exchanging pixel buffers + + +As originally designed, the Linux graphics subsystem had extremely limited +support for sharing pixel-buffer allocations between processes, devices, and +subsystems. Modern systems require extensive integration between all three +classes; this document details how applications and kernel subsystems should +approach this sharing for two-dimensional image data. + +It is written with reference to the DRM subsystem for GPU and display devices, +V4L2 for media devices, and also to Vulkan, EGL and Wayland, for userspace +support, however any other subsystems should also follow this design and advice. + + +Glossary of terms += + +.. glossary:: + +image: + Conceptually a two-dimensional array of pixels. The pixels may be stored + in one or more memory buffers. Has width and height in pixels, pixel + format and modifier (implicit or explicit). + +row: + A span along a single y-axis value, e.g. from co-ordinates (0,100) to + (200,100). + +scanline: + Synonym for row. + +column: + A span along a single x-axis value, e.g. from co-ordinates (100,0) to + (100,100). + +memory buffer: + A piece of memory for storing (parts of) pixel data. Has stride and size + in bytes and at least one handle in some API. May contain one or more + planes. + +plane: + A two-dimensional array of some or all of an image's color and alpha + channel values. + +pixel: + A picture element. Has a single color value which is defined by one or + more color channels values, e.g. R, G and B, or Y, Cb and Cr. May also + have an alpha value as an additional channel. + +pixel data: + Bytes or bits that represent some or all of the color/alpha channel values + of a pixel or an image. The data for one pixel may be spread over several + planes or memory buffers depending on format and modifier. + +color value: + A tuple of numbers, representing a color. Each element in the tuple is a + color channel value. + +color channel: + One of the dimensions in a color model. For example, RGB model has + channels R, G, and B. Alpha channel is sometimes counted as a color + channel as well. + +pixel format: + A description of how pixel data represents the pixel's color and alpha + values. + +modifier: + A description of how pixel data is laid out in memory buffers. + +alpha: + A
Re: [06/12] arch: Declare screen_info in
On 2023/8/18 22:04, suijingfeng wrote: Hi, Why this patch get dropped in the end? Since the global screen_info is an arch-specific thing, Whenever an arch-neutral module or subsystem references the global screen_info, There are some complaints from either compile testing robot. There are some complaints from either compile testing robot or domain specific reviewers who doubt why you select the CONFIG_SYSFB not CONFIG_VT or CONFIG_EFI. Well, a programmer may handle it by using the CONFIG_SYSFB guard, but it is not as precise as what this patch provided. Personally, I think this patch is still valuable. I suggest either forcing all other architectures to export screen_info, like the X86 and IA64 arch does, after all the screen_info is a good thing. or provide the fine-control version like this patch does. Because all of the three tokens(CONFIG_SYSFB not CONFIG_VT or CONFIG_EFI.) have no direct relationship with the global screen_info if an arch is not mentioned first. On 2023/6/29 19:45, Thomas Zimmermann wrote: The variable screen_info does not exist on all architectures. Declare it in . All architectures that do declare it will provide it via . Add the Kconfig token ARCH_HAS_SCREEN_INFO to guard against access on architectures that don't provide screen_info. Signed-off-by: Thomas Zimmermann Cc: Richard Henderson Cc: Ivan Kokshaysky Cc: Matt Turner Cc: Russell King Cc: Catalin Marinas Cc: Will Deacon Cc: Guo Ren Cc: Brian Cain Cc: Huacai Chen Cc: WANG Xuerui Cc: Thomas Bogendoerfer Cc: Dinh Nguyen Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Christophe Leroy Cc: Paul Walmsley Cc: Palmer Dabbelt Cc: Albert Ou Cc: Yoshinori Sato Cc: Rich Felker Cc: John Paul Adrian Glaubitz Cc: "David S. Miller" Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: Dave Hansen Cc: x...@kernel.org Cc: "H. Peter Anvin" Cc: Chris Zankel Cc: Max Filippov Cc: Helge Deller Cc: Arnd Bergmann Cc: Kees Cook Cc: "Paul E. McKenney" Cc: Peter Zijlstra Cc: Frederic Weisbecker Cc: Andrew Morton Cc: Ard Biesheuvel Cc: Sami Tolvanen Cc: Juerg Haefliger Cc: Geert Uytterhoeven Cc: Anshuman Khandual Cc: Niklas Schnelle Cc: "Russell King (Oracle)" Cc: Linus Walleij Cc: Sebastian Reichel Cc: "Mike Rapoport (IBM)" Cc: "Kirill A. Shutemov" Cc: Zi Yan Acked-by: WANG Xuerui # loongarch --- arch/Kconfig | 6 ++ arch/alpha/Kconfig | 1 + arch/arm/Kconfig | 1 + arch/arm64/Kconfig | 1 + arch/csky/Kconfig | 1 + arch/hexagon/Kconfig | 1 + arch/ia64/Kconfig | 1 + arch/loongarch/Kconfig | 1 + arch/mips/Kconfig | 1 + arch/nios2/Kconfig | 1 + arch/powerpc/Kconfig | 1 + arch/riscv/Kconfig | 1 + arch/sh/Kconfig | 1 + arch/sparc/Kconfig | 1 + arch/x86/Kconfig | 1 + arch/xtensa/Kconfig | 1 + drivers/video/Kconfig | 3 +++ include/asm-generic/Kbuild | 1 + include/asm-generic/screen_info.h | 12 include/linux/screen_info.h | 2 +- 20 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 include/asm-generic/screen_info.h diff --git a/arch/Kconfig b/arch/Kconfig index 205fd23e0cada..2f58293fd7bcb 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -1466,6 +1466,12 @@ config ARCH_HAS_NONLEAF_PMD_YOUNG address translations. Page table walkers that clear the accessed bit may use this capability to reduce their search space. +config ARCH_HAS_SCREEN_INFO + bool + help + Selected by architectures that provide a global instance of + screen_info. + source "kernel/gcov/Kconfig" source "scripts/gcc-plugins/Kconfig" diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig index a5c2b1aa46b02..d749011d88b14 100644 --- a/arch/alpha/Kconfig +++ b/arch/alpha/Kconfig @@ -4,6 +4,7 @@ config ALPHA default y select ARCH_32BIT_USTAT_F_TINODE select ARCH_HAS_CURRENT_STACK_POINTER + select ARCH_HAS_SCREEN_INFO select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO select ARCH_NO_PREEMPT diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 0fb4b218f6658..a9d01ee67a90e 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -15,6 +15,7 @@ config ARM select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_SPECIAL if ARM_LPAE + select ARCH_HAS_SCREEN_INFO select ARCH_HAS_SETUP_DMA_OPS select ARCH_HAS_SET_MEMORY select ARCH_STACKWALK diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 343e1e1cae10a..21addc4715bb3 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -36,6 +36,7
Re: [06/12] arch: Declare screen_info in
Hi, Why this patch get dropped in the end? Since the global screen_info is an arch-specific thing, Whenever an arch-neutral module or subsystem references the global screen_info, There are some complaints from either compile testing robot. Well, a programmer may handle it by using the CONFIG_SYSFB guard, but it is not as precise as what this patch provided. Personally, I think this patch is still valuable. I suggest either forcing all other architectures to export screen_info, like the X86 and IA64 arch does, after all the screen_info is a good thing. or provide the fine-control version like this patch does. On 2023/6/29 19:45, Thomas Zimmermann wrote: The variable screen_info does not exist on all architectures. Declare it in . All architectures that do declare it will provide it via . Add the Kconfig token ARCH_HAS_SCREEN_INFO to guard against access on architectures that don't provide screen_info. Signed-off-by: Thomas Zimmermann Cc: Richard Henderson Cc: Ivan Kokshaysky Cc: Matt Turner Cc: Russell King Cc: Catalin Marinas Cc: Will Deacon Cc: Guo Ren Cc: Brian Cain Cc: Huacai Chen Cc: WANG Xuerui Cc: Thomas Bogendoerfer Cc: Dinh Nguyen Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Christophe Leroy Cc: Paul Walmsley Cc: Palmer Dabbelt Cc: Albert Ou Cc: Yoshinori Sato Cc: Rich Felker Cc: John Paul Adrian Glaubitz Cc: "David S. Miller" Cc: Thomas Gleixner Cc: Ingo Molnar Cc: Borislav Petkov Cc: Dave Hansen Cc: x...@kernel.org Cc: "H. Peter Anvin" Cc: Chris Zankel Cc: Max Filippov Cc: Helge Deller Cc: Arnd Bergmann Cc: Kees Cook Cc: "Paul E. McKenney" Cc: Peter Zijlstra Cc: Frederic Weisbecker Cc: Andrew Morton Cc: Ard Biesheuvel Cc: Sami Tolvanen Cc: Juerg Haefliger Cc: Geert Uytterhoeven Cc: Anshuman Khandual Cc: Niklas Schnelle Cc: "Russell King (Oracle)" Cc: Linus Walleij Cc: Sebastian Reichel Cc: "Mike Rapoport (IBM)" Cc: "Kirill A. Shutemov" Cc: Zi Yan Acked-by: WANG Xuerui # loongarch --- arch/Kconfig | 6 ++ arch/alpha/Kconfig| 1 + arch/arm/Kconfig | 1 + arch/arm64/Kconfig| 1 + arch/csky/Kconfig | 1 + arch/hexagon/Kconfig | 1 + arch/ia64/Kconfig | 1 + arch/loongarch/Kconfig| 1 + arch/mips/Kconfig | 1 + arch/nios2/Kconfig| 1 + arch/powerpc/Kconfig | 1 + arch/riscv/Kconfig| 1 + arch/sh/Kconfig | 1 + arch/sparc/Kconfig| 1 + arch/x86/Kconfig | 1 + arch/xtensa/Kconfig | 1 + drivers/video/Kconfig | 3 +++ include/asm-generic/Kbuild| 1 + include/asm-generic/screen_info.h | 12 include/linux/screen_info.h | 2 +- 20 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 include/asm-generic/screen_info.h diff --git a/arch/Kconfig b/arch/Kconfig index 205fd23e0cada..2f58293fd7bcb 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -1466,6 +1466,12 @@ config ARCH_HAS_NONLEAF_PMD_YOUNG address translations. Page table walkers that clear the accessed bit may use this capability to reduce their search space. +config ARCH_HAS_SCREEN_INFO + bool + help + Selected by architectures that provide a global instance of + screen_info. + source "kernel/gcov/Kconfig" source "scripts/gcc-plugins/Kconfig" diff --git a/arch/alpha/Kconfig b/arch/alpha/Kconfig index a5c2b1aa46b02..d749011d88b14 100644 --- a/arch/alpha/Kconfig +++ b/arch/alpha/Kconfig @@ -4,6 +4,7 @@ config ALPHA default y select ARCH_32BIT_USTAT_F_TINODE select ARCH_HAS_CURRENT_STACK_POINTER + select ARCH_HAS_SCREEN_INFO select ARCH_MIGHT_HAVE_PC_PARPORT select ARCH_MIGHT_HAVE_PC_SERIO select ARCH_NO_PREEMPT diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 0fb4b218f6658..a9d01ee67a90e 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -15,6 +15,7 @@ config ARM select ARCH_HAS_MEMBARRIER_SYNC_CORE select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_SPECIAL if ARM_LPAE + select ARCH_HAS_SCREEN_INFO select ARCH_HAS_SETUP_DMA_OPS select ARCH_HAS_SET_MEMORY select ARCH_STACKWALK diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 343e1e1cae10a..21addc4715bb3 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -36,6 +36,7 @@ config ARM64 select ARCH_HAS_NON_OVERLAPPING_ADDRESS_SPACE select ARCH_HAS_PTE_DEVMAP select ARCH_HAS_PTE_SPECIAL + select ARCH_HAS_SCREEN_INFO select ARCH_HAS_SETUP_DMA_OPS select ARCH_HAS_SET_DIRECT_MAP select ARCH_HAS_SET_MEMORY diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig index 4df1f8c9d170b..28444e581fc1f 100644 --- a/arch/csky/Kconfig +++ b/arch/csky/Kconfig @@
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/18 06:08, Bjorn Helgaas wrote:
Please note that before apply this patch, vgaarb can not select the
right boot vga due to weird logic introduced with the commit
57fc7323a8e7c ("LoongArch: Add PCI controller support")
If we need this reference to 57fc7323a8e7c, we need more specifics
about what the "weird logic" is. pci_fixup_vgadev() is the only
obvious VGA connection, so I suppose it's related to that.
Yes, you are right.
The pci_fixup_vgadev() function will set the last VGA device enumerated as the
default boot device.
By "the last" VGA device, I mean that this device has the largest PCI bus,
domain, and function triple.
Thus, it is added to vgaarb in the end of all VGA device.
So that logic expresses that the last one added will be the default.
This probably is not what we want.
On the LS3A5000+LS7A1000 platform, the last VGA device is a S3 graphics
(08:00.0). This GPU has two cores. Say the log below:
$ lspci | grep VGA
00:06.1 VGA compatible controller: Loongson Technology LLC DC (Display
Controller) (rev 01)
03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI]
Caicos XT [Radeon HD 7470/8470 / R5 235/310 OEM]
07:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01)
08:00.0 VGA compatible controller: S3 Graphics Ltd. Device 9070 (rev 01)
[0.361781] vgaarb: loaded
[0.367838] pci :00:06.1: Overriding boot device as 1002:6778
[0.367841] pci :00:06.1: Overriding boot device as 5333:9070
[0.367843] pci :00:06.1: Overriding boot device as 5333:9070
1) The "weird" logic completely overrides whatever decision VGAARB ever made.
It seems to say that the decision ever made by VGAARB is useless.
Well, I think VGAARB shouldn't endure this; VGAARB has to be small.
2) The results it gives are not correct either.
In the first testing example in my commit message,
it overrides the S3 graphics as the default boot VGA instead of the AMD/ATI GPU.
Actually, the firmware chooses the AMD/ATI GPU as the "frimware default".
3) It tries to make the decision for the end user instead of the firmware.
Therefore, that function is always wrong. Again, it's a policy, not a mechanism.
Since that already have been merge, I'm fine.
Maybe Huacai is busy, he might don't has the time to carry on a deep thinking.
But I think we should correct the mistake ever made,
let's merge this patch to make vgaarb great again ?
Well, that commit is not a dependency, I don't mind delete the referencing
to that commit. After all, I think my patch will be effective on other
architectures.
Is additional testing on ARM64 and X86 is needed, if so I have to find the
machine to
carry on the testing.
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi, On 2023/8/18 06:08, Bjorn Helgaas wrote: This patch makes the vga_is_firmware_default() function works on whatever arch that has UEFI GOP support. But we make it available only on platforms where PCI resource relocation happens. if the provided method proves to be effective and reliable, it can be expanded to other arch easily. v2: * Fix test robot warnnings and fix typos v3: * Fix linkage problems if the global screen_info is not exported v4: * Handle linkage problems by hiding behind of CONFIG_SYSFB, * Drop side-effects and simplify. The v2, v3, v4 changelog is nice, but we don't need it in the commit log itself, where it will become part of the git history. It should go in a cover letter or after the "---" marker: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.0#n678 Thanks for point it out, now I know. Will be fixed at the next version.
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/18 06:08, Bjorn Helgaas wrote:
On Wed, Aug 16, 2023 at 06:05:27AM +0800, Sui Jingfeng wrote:
Currently, the vga_is_firmware_default() function only works on x86 and
ia64, it is a no-op on ARM, ARM64, PPC, RISC-V, etc. This patch completes
the implementation for the rest of the architectures. The added code tries
to identify the PCI(e) VGA device that owns the firmware framebuffer
before PCI resource reallocation happens.
As far as I can tell, this is basically identical to the existing
vga_is_firmware_default(), except that this patch funs that code as a
header fixup, so it happens before any PCI BAR reallocations happen.
Yes, what you said is right in overall.
But I think I should mention a few tiny points that make a difference.
1) My version is *less arch-dependent*
Again, since the global screen_info is arch-dependent.
The vga_is_firmware_default() mess up the arch-dependent part and
arch-independent part.
It's a mess and it's a bit harder to make the cleanup on the top of it.
While my version is my version split the arch-dependent part and
arch-independent part clearly.
Since we decide to make it less arch-dependent, we have to bear the pain.
Despite all other arches should always export the screen_info like the X86 and
IA64 arch does,
or at least a arch should give a Kconfig token (for example,
CONFIG_ARCH_HAS_SCREEN_INFO) to
demonstrate that an arch has the support for it.
While currently, the fact is that the dependence just populated to everywhere.
I think this is the hard part, you have to investigate how various arches
defines and set up
the screen_info. And then process dependency and the linkage problem across
arch properly.
2) My version focus on the address in ranges, weaken the size parameter.
Which make the code easy to read and follow the canonical convention to
express the address range. while the vga_is_firmware_default() is not.
3) A tiny change make a big difference.
The original vga_is_firmware_default() only works with the assumption
that the PCI resource reallocation won't happens. While I see no clue
that why this is true even on X86 and IA64. The original patch[1] not
mention this assumption explicitly.
[1] 86fd887b7fe3 ('vgaarb: Don't default exclusively to first video device with mem+io')
That sounds like a good idea, because this is all based on the
framebuffer in screen_info, and screen_info was initialized before PCI
enumeration, and it certainly doesn't account for any BAR changes done
by the PCI core.
Yes.
So why would we keep vga_is_firmware_default() at all? If the header
fixup has already identified the firmware framebuffer, it seems
pointless to look again later.
It need another patch to do the cleanup work, while my patch just add code to
solve the real problem.
It focus on provide a solution for the architectures which have a decent way
set up the screen_info.
Other things except that is secondary.
Re: [PATCH v4] PCI/VGA: Make the vga_is_firmware_default() less arch-dependent
Hi,
On 2023/8/18 06:08, Bjorn Helgaas wrote:
+ if (resource_type(res) != IORESOURCE_MEM)
+ continue;
+
+ if (!res->start || !res->end)
+ continue;
+
+ if (res->start <= fb_start && fb_end <= res->end) {
+ pdev_boot_vga = pdev;
+
+ vgaarb_info(>dev,
+ "BAR %d contains firmware FB\n", i);
Print the BAR with %pR and include the framebuffer region from
screen_info in the same format.
I do remember that you already told me to do this in V3, sorry for not
replying to you at V3. Most of the time, what you tell me is right.But
here, I think I need to explain. Because doing it that way will make the
code line too long,and it will exceed 80 characters in the column if we
print too much.
I believe that the vgaarb_info() at here is already the most compact and
simplest form. Printing the BAR with %pR is not absolute necessary,
because we can get the additional information by: $ lspci | grep VGA
$ dmesg | grep 05:00.0
$ dmesg | grep :03:00.0
$ dmesg | grep PCI
Actually, I only add something that is absolute necessary.
Printing BAR with %pR and/or Printing the framebuffer region
is consider to only for *debugging* purpose.
Re: [PATCH next] drm/loongson: Fix error handling in lsdc_pixel_pll_setup()
Hi,
On 2023/7/20 20:39, Harshit Mogalapalli wrote:
There are two problems in lsdc_pixel_pll_setup()
1. If kzalloc() fails then call iounmap() to release the resources.
2. Both kzalloc and ioremap doesnot return error pointers on failure, so
using IS_ERR_OR_NULL() checks is a bit confusing and not very right,
fix this by changing those to NULL checks instead.
Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Signed-off-by: Harshit Mogalapalli
Reviewed-by: Sui Jingfeng
---
This is found with static analysis with smacth and only compile tested.
---
drivers/gpu/drm/loongson/lsdc_pixpll.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/loongson/lsdc_pixpll.c
b/drivers/gpu/drm/loongson/lsdc_pixpll.c
index 04c15b4697e2..2609a2256da4 100644
--- a/drivers/gpu/drm/loongson/lsdc_pixpll.c
+++ b/drivers/gpu/drm/loongson/lsdc_pixpll.c
@@ -120,12 +120,14 @@ static int lsdc_pixel_pll_setup(struct lsdc_pixpll *
const this)
struct lsdc_pixpll_parms *pparms;
this->mmio = ioremap(this->reg_base, this->reg_size);
- if (IS_ERR_OR_NULL(this->mmio))
+ if (!this->mmio)
return -ENOMEM;
pparms = kzalloc(sizeof(*pparms), GFP_KERNEL);
- if (IS_ERR_OR_NULL(pparms))
+ if (!pparms) {
+ iounmap(this->mmio);
return -ENOMEM;
+ }
pparms->ref_clock = LSDC_PLL_REF_CLK_KHZ;
Re: [PATCH v2 02/11] PCI: Add the pci_get_class_masked() helper
Hi,
On 2023/8/9 22:01, Ilpo Järvinen wrote:
On Wed, 9 Aug 2023, Sui Jingfeng wrote:
From: Sui Jingfeng
Because there is no good way to get the mask member used to searching for
devices that conform to a specific PCI class code, an application needs to
process all PCI display devices can achieve its goal as follows:
This is mixing old and new way in a single sentence (which is confusing)?
Thanks for reviewing, but I can't understand this sentence.
Are you telling me that my description have grammar problem or something else?
I means that before apply this patch, we don't have a function can be used
to get all PCI(e) devices in a system by matching against its the PCI base
class code only,
while keep the Sub-Class code and the Programming Interface ignored.
By supply a mask as argument, such thing become possible.
If an application want to process all PCI display devices in the system,
then it can achieve its goal by calling pci_get_class_masked() function.
pdev = NULL;
do {
pdev = pci_get_class_masked(PCI_BASE_CLASS_DISPLAY << 16, 0xFF,
pdev);
if (pdev)
do_something_for_pci_display_device(pdev);
} while (pdev);
While previously, we just can not ignore Sub-Class code and the Programming
cannot
Interface byte when do the searching.
doing the search.
OK, will be fixed at the next version.
Signed-off-by: Sui Jingfeng
---
drivers/pci/search.c | 30 ++
include/linux/pci.h | 7 +++
2 files changed, 37 insertions(+)
diff --git a/drivers/pci/search.c b/drivers/pci/search.c
index b4c138a6ec02..f1c15aea868b 100644
--- a/drivers/pci/search.c
+++ b/drivers/pci/search.c
@@ -334,6 +334,36 @@ struct pci_dev *pci_get_device(unsigned int vendor,
unsigned int device,
}
EXPORT_SYMBOL(pci_get_device);
+/**
+ * pci_get_class_masked - begin or continue searching for a PCI device by
class and mask
+ * @class: search for a PCI device with this class designation
+ * @from: Previous PCI device found in search, or %NULL for new search.
+ *
+ * Iterates through the list of known PCI devices. If a PCI device is
No double spaces in kernel comments. Perhaps your editor might be adding
them on reflow (might be configurable to not do that).
+ * found with a matching @class, the reference count to the device is
+ * incremented and a pointer to its device structure is returned.
+ * Otherwise, %NULL is returned.
+ * A new search is initiated by passing %NULL as the @from argument.
+ * Otherwise if @from is not %NULL, searches continue from next device
+ * on the global list. The reference count for @from is always decremented
+ * if it is not %NULL.
Use kerneldoc's Return: section for describing return value.
+ */
+struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask,
+struct pci_dev *from)
+{
+ struct pci_device_id id = {
+ .vendor = PCI_ANY_ID,
+ .device = PCI_ANY_ID,
+ .subvendor = PCI_ANY_ID,
+ .subdevice = PCI_ANY_ID,
+ .class_mask = mask,
+ .class = class,
+ };
+
+ return pci_get_dev_by_id(, from);
+}
+EXPORT_SYMBOL(pci_get_class_masked);
+
/**
* pci_get_class - begin or continue searching for a PCI device by class
* @class: search for a PCI device with this class designation
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 0ff7500772e6..b20e7ba844bf 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1180,6 +1180,9 @@ struct pci_dev *pci_get_slot(struct pci_bus *bus,
unsigned int devfn);
struct pci_dev *pci_get_domain_bus_and_slot(int domain, unsigned int bus,
unsigned int devfn);
struct pci_dev *pci_get_class(unsigned int class, struct pci_dev *from);
+struct pci_dev *pci_get_class_masked(unsigned int class, unsigned int mask,
+struct pci_dev *from);
+
int pci_dev_present(const struct pci_device_id *ids);
int pci_bus_read_config_byte(struct pci_bus *bus, unsigned int devfn,
@@ -1895,6 +1898,10 @@ static inline struct pci_dev *pci_get_class(unsigned int
class,
struct pci_dev *from)
{ return NULL; }
+static inline struct pci_dev *pci_get_class_masked(unsigned int class,
+ unsigned int mask,
+ struct pci_dev *from)
+{ return NULL; }
static inline int pci_dev_present(const struct pci_device_id *ids)
{ return 0; }
Re: [PATCH v2 06/11] PCI/VGA: Fix two typos in the comments of pci_notify()
Hi,
On 2023/8/9 22:12, Ilpo Järvinen wrote:
On Wed, 9 Aug 2023, Sui Jingfeng wrote:
From: Sui Jingfeng
1) s/intereted/interested
2) s/hotplugable/hot-pluggable
While at it, convert the comments to the conventional multi-line style,
and rewrap to fill 78 columns.
Fixes: deb2d2ecd43d ("PCI/GPU: implement VGA arbitration on Linux")
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 6883067a802a..811510253553 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1535,9 +1535,11 @@ static int pci_notify(struct notifier_block *nb,
unsigned long action,
if (!pci_dev_is_vga(pdev))
return 0;
- /* For now we're only intereted in devices added and removed. I didn't
-* test this thing here, so someone needs to double check for the
-* cases of hotplugable vga cards. */
+ /*
+* For now, we're only interested in devices added and removed.
+* I didn't test this thing here, so someone needs to double check
+* for the cases of hot-pluggable VGA cards.
+*/
if (action == BUS_NOTIFY_ADD_DEVICE)
notify = vga_arbiter_add_pci_device(pdev);
else if (action == BUS_NOTIFY_DEL_DEVICE)
Don't use Fixes tag for comment changes. After removing it, feel free to
add:
OK, I will remove the Fixes tag for comment changes at next version.
Thanks a lot. Also for other patches in this series.
Reviewed-by: Ilpo Järvinen
Re: [PATCH v2 07/11] PCI/VGA: vga_client_register() return -ENODEV on failure, not -1
Hi,
On 2023/8/9 21:52, Ilpo Järvinen wrote:
On Wed, 9 Aug 2023, Sui Jingfeng wrote:
From: Sui Jingfeng
Changelog body is missing.
I thought that probably the Fixes tag could be taken as the body of this commit,
since there are no warnings when I check the whole series with checkpatch.pl.
Fixes: 934f992c763a ("drm/i915: Recognise non-VGA display devices")
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 811510253553..a6b8c0def35d 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -964,7 +964,7 @@ EXPORT_SYMBOL(vga_set_legacy_decoding);
*
* To unregister just call vga_client_unregister().
*
- * Returns: 0 on success, -1 on failure
+ * Returns: 0 on success, -ENODEV on failure
So this is the true substance of this change??
Yes.
It doesn't warrant Fixes tag which requires a real problem to fix. An
incorrect comment is not enough.
I think the shortlog is a bit misleading as is because it doesn't in any
way indicate the problem is only in a comment.
But it's that commit(934f992c763a) alter the return value of
vga_client_register(),
which make the commit and code don't match anymore.
*/
int vga_client_register(struct pci_dev *pdev,
unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
@@ -975,16 +975,13 @@ int vga_client_register(struct pci_dev *pdev,
spin_lock_irqsave(_lock, flags);
vgadev = vgadev_find(pdev);
- if (!vgadev)
- goto bail;
-
- vgadev->set_decode = set_decode;
- ret = 0;
-
-bail:
+ if (vgadev) {
+ vgadev->set_decode = set_decode;
+ ret = 0;
+ }
spin_unlock_irqrestore(_lock, flags);
- return ret;
+ return ret;
No logic changes in this at all? I don't think it belongs to the same
patch. I'm not sure if the new logic is improvement anyway.
Yes, the new logic is just improvement, no function change.
Strict speaking, you are right. One patch do one thing.
I'd prefer to
initialize ret = 0 instead:
int ret = 0;
...
if (!vgadev) {
err = -ENODEV;
goto unlock;
}
...
unlock:
...
But this is same as the original coding style, no fundamental improve.
The key point is to make the wrapped code between the spin_lock_irqsave() and
spin_unlock_irqrestore() compact.
my patch remove the necessary 'goto' statement and the 'bail' label.
After apply my patch, the vga_client_register() function became as this:
int vga_client_register(struct pci_dev *pdev,
unsigned int (*set_decode)(struct pci_dev *pdev, bool decode))
{
int ret = -ENODEV;
struct vga_device *vgadev;
unsigned long flags;
spin_lock_irqsave(_lock, flags);
vgadev = vgadev_find(pdev);
if (vgadev) {
vgadev->set_decode = set_decode;
ret = 0;
}
spin_unlock_irqrestore(_lock, flags);
return ret;
}
Re: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent
Hi, On 2023/8/3 20:25, kernel test robot wrote: Hi Sui, kernel test robot noticed the following build errors: [auto build test ERROR on pci/next] [also build test ERROR on pci/for-linus linus/master v6.5-rc4 next-20230803] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Make-the-vga_is_firmware_default-arch-independent/20230803-161838 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/20230803081758.968742-1-suijingfeng%40loongson.cn patch subject: [PATCH] PCI/VGA: Make the vga_is_firmware_default() arch-independent config: arm64-randconfig-r026-20230731 (https://download.01.org/0day-ci/archive/20230803/202308032022.yizngbbk-...@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a) reproduce: (https://download.01.org/0day-ci/archive/20230803/202308032022.yizngbbk-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202308032022.yizngbbk-...@intel.com/ All errors (new ones prefixed by >>): ld.lld: error: undefined symbol: screen_info >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86) >>> drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a >>> referenced by vgaarb.c:86 (drivers/pci/vgaarb.c:86) >>> drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a >>> referenced by vgaarb.c:88 (drivers/pci/vgaarb.c:88) >>> drivers/pci/vgaarb.o:(vga_arb_firmware_fb_addr_tracker) in archive vmlinux.a >>> referenced 3 more times This is a more like arch-specific problem, It will be pain at many places on platforms that do not export the screen_info symbol. Not only here. I have already explained that screen_info is arch-dependent many times, but no one cares about me. By using (looking at) screen_info, vgaarb gets infected, and becomes arch-dependent as well. vgaarb deals with VGA class (pdev->class == 0x0300XX) devices only, This makes it device-dependent. Hence, It only works correctly for a small set of PCIe devices on x86. arch-dependent, device-dependent, subsystem-dependent (part of it rely on ACPI) and loading order dependent, those dependent itself are the problems. It results in various undefined (uncertain) behaviors on non-x86 architectures. Even on x86, some platform choose to relay on the firmware to solve the multiple GPU coexist problem. so it is also firmware-dependent. This patch solves part of the above problems listed, target at the *device level*, as early as possible. while they still a few problems could be only solved at the *driver level*. For an example, The display controller in Intel N2000 and d2000 series don't has a dedicated VRAM bar. they use the "stolen memory", which is carve out by somebody (either bios or kernel?).
Re: [PATCH] PCI/VGA: Fixup the firmware fb address om demanding time
Hi,
On 2023/8/2 11:40, kernel test robot wrote:
Hi Sui,
kernel test robot noticed the following build errors:
[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.5-rc4 next-20230801]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/PCI-VGA-Fixup-the-firmware-fb-address-om-demanding-time/20230802-023743
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:
https://lore.kernel.org/r/20230801183706.702567-1-suijingfeng%40loongson.cn
patch subject: [PATCH] PCI/VGA: Fixup the firmware fb address om demanding time
config: parisc64-defconfig
(https://download.01.org/0day-ci/archive/20230802/202308021153.w0leladx-...@intel.com/config)
compiler: hppa64-linux-gcc (GCC) 12.3.0
reproduce:
(https://download.01.org/0day-ci/archive/20230802/202308021153.w0leladx-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202308021153.w0leladx-...@intel.com/
All errors (new ones prefixed by >>):
hppa64-linux-ld: drivers/pci/vgaarb.o: in function
`vga_arb_firmware_fb_addr_tracker':
(.text+0x1d0): undefined reference to `screen_info'
hppa64-linux-ld: (.text+0x1d4): undefined reference to `screen_info'
How does this problem related to my patch?
The dependency was introduce since the commit 86fd887b7fe3
('vgaarb: Don't default exclusively to first video device with mem+io')
Re: [v1,v1,5/7] drm/vs: Register DRM device
Hi, On 2023/8/1 21:40, suijingfeng wrote: +#define DRV_DATE "202305161" The date is not correct here, generally it should have 8 numbers, while you have 9 digits, why you are so special ? I'm also interesting in RISCV arch, I got attracted by your patch. I just want to join into the discussion at here (at my spare time), So when you see my comments, I hoping that you will not interpret it as hostility. Welcome contributing. :-)
Re: [v1,v1,5/7] drm/vs: Register DRM device
Hi, On 2023/8/1 21:40, suijingfeng wrote: +if DRM_VERISILICON + +config STARFIVE_HDMI + bool "Starfive specific extensions HDMI" + help + This selects support for StarFive SoC specific extensions + for the Innosilicon HDMI driver. If you want to enable + HDMI on JH7110 based SoC, you should select this option. + + To compile this driver as a module, choose M here. +endif Why not use Why not use the 'depends on DRM_VERISILICON' here ? ``` config STARFIVE_HDMI depends on DRM_VERISILICON bool "Starfive specific extensions HDMI" help ``` I see the Kconfig of VC4 using the 'depends on', and most driver using the 'depends on'
Re: [v1,v1,5/7] drm/vs: Register DRM device
Hi, On 2023/8/1 21:40, suijingfeng wrote: So, you patch will be pass the compile test, I guess. You patch will *NOT* pass the compile test, I guess.
Re: [v1,v1,5/7] drm/vs: Register DRM device
Hi,
On 2023/8/1 18:10, Keith Zhao wrote:
Implement drm device registration interface
Signed-off-by: Keith Zhao
---
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/verisilicon/Kconfig | 25 ++
drivers/gpu/drm/verisilicon/Makefile | 13 +
drivers/gpu/drm/verisilicon/vs_drv.c | 273 +
drivers/gpu/drm/verisilicon/vs_drv.h | 54
drivers/gpu/drm/verisilicon/vs_gem.c | 298 +++
drivers/gpu/drm/verisilicon/vs_gem.h | 50
drivers/gpu/drm/verisilicon/vs_modeset.c | 92 +++
drivers/gpu/drm/verisilicon/vs_modeset.h | 13 +
include/uapi/drm/vs_drm.h| 50
11 files changed, 871 insertions(+)
create mode 100644 drivers/gpu/drm/verisilicon/Kconfig
create mode 100644 drivers/gpu/drm/verisilicon/Makefile
create mode 100644 drivers/gpu/drm/verisilicon/vs_drv.c
create mode 100644 drivers/gpu/drm/verisilicon/vs_drv.h
create mode 100644 drivers/gpu/drm/verisilicon/vs_gem.c
create mode 100644 drivers/gpu/drm/verisilicon/vs_gem.h
create mode 100644 drivers/gpu/drm/verisilicon/vs_modeset.c
create mode 100644 drivers/gpu/drm/verisilicon/vs_modeset.h
create mode 100644 include/uapi/drm/vs_drm.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index afb3b2f5f..9ede80ef9 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -363,6 +363,8 @@ source "drivers/gpu/drm/solomon/Kconfig"
source "drivers/gpu/drm/sprd/Kconfig"
+source "drivers/gpu/drm/verisilicon/Kconfig"
+
config DRM_HYPERV
tristate "DRM Support for Hyper-V synthetic video device"
depends on DRM && PCI && MMU && HYPERV
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 7a09a89b4..6db3bc397 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -194,3 +194,4 @@ obj-y += gud/
obj-$(CONFIG_DRM_HYPERV) += hyperv/
obj-y += solomon/
obj-$(CONFIG_DRM_SPRD) += sprd/
+obj-$(CONFIG_DRM_VERISILICON) += verisilicon/
diff --git a/drivers/gpu/drm/verisilicon/Kconfig
b/drivers/gpu/drm/verisilicon/Kconfig
new file mode 100644
index 0..fcc39dded
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/Kconfig
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: GPL-2.0
+config DRM_VERISILICON
+ tristate "DRM Support for VeriSilicon"
+ depends on DRM
+ select DRM_KMS_HELPER
+ select DRM_GEM_DMA_HELPER
+ select CMA
+ select DMA_CMA
+ help
+ Choose this option if you have a VeriSilicon soc chipset.
+ This driver provides VeriSilicon kernel mode
+ setting and buffer management. It does not
+ provide 2D or 3D acceleration.
+
+if DRM_VERISILICON
+
+config STARFIVE_HDMI
+ bool "Starfive specific extensions HDMI"
+ help
+ This selects support for StarFive SoC specific extensions
+ for the Innosilicon HDMI driver. If you want to enable
+ HDMI on JH7110 based SoC, you should select this option.
+
+ To compile this driver as a module, choose M here.
+endif
Why not use
diff --git a/drivers/gpu/drm/verisilicon/Makefile
b/drivers/gpu/drm/verisilicon/Makefile
new file mode 100644
index 0..26cc7e21b
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/Makefile
@@ -0,0 +1,13 @@
+# SPDX-License-Identifier: GPL-2.0
+
+vs_drm-objs := vs_dc_hw.o \
+ vs_dc.o \
+ vs_crtc.o \
+ vs_drv.o \
+ vs_modeset.o \
+ vs_gem.o \
+ vs_plane.o
+
+vs_drm-$(CONFIG_STARFIVE_HDMI) += starfive_hdmi.o
+obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o
+
diff --git a/drivers/gpu/drm/verisilicon/vs_drv.c
b/drivers/gpu/drm/verisilicon/vs_drv.c
new file mode 100644
index 0..69591e640
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/vs_drv.c
@@ -0,0 +1,273 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "vs_drv.h"
+#include "vs_modeset.h"
+#include "vs_gem.h"
+
+#define DRV_NAME "starfive"
+#define DRV_DESC "Starfive DRM driver"
+#define DRV_DATE "202305161"
The date is not correct here, generally it should have 8 numbers,
while you have 9 digits, why you are so special ?
+#define DRV_MAJOR 1
+#define DRV_MINOR 0
+
+static struct platform_driver vs_drm_platform_driver;
+
+static const struct file_operations fops = {
+ .owner = THIS_MODULE,
+ .open = drm_open,
+ .release= drm_release,
+ .unlocked_ioctl = drm_ioctl,
+ .compat_ioctl =
Re: [1/2] drm/tegra: Return an error code if fails
Hi, Gentle ping for this series. On 2023/6/26 22:33, Sui Jingfeng wrote: Return -ENOMEM if tegra_bo_mmap() fails. Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/tegra/gem.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index dea38892d6e6..0ce22935fbd3 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -710,6 +710,8 @@ static int tegra_gem_prime_vmap(struct dma_buf *buf, struct iosys_map *map) void *vaddr; vaddr = tegra_bo_mmap(>base); + if (!vaddr) + return -ENOMEM; if (IS_ERR(vaddr)) return PTR_ERR(vaddr);
Re: [PATCH v2] drm/mediatek: Fix potential memory leak if vmap() fail
Hi,
On 2023/7/27 17:22, AngeloGioacchino Del Regno wrote:
Il 06/07/23 15:40, Sui Jingfeng ha scritto:
Also return -ENOMEM if such a failure happens, the implement should take
responsibility for the error handling.
Fixes: 3df64d7b0a4f ("drm/mediatek: Implement gem prime vmap/vunmap
function")
Reviewed-by: Matthias Brugger
Reviewed-by: Alexandre Mergnat
Signed-off-by: Sui Jingfeng
Reviewed-by: AngeloGioacchino Del Regno
Yeah! I got so many R-B!
So surprise, So happy!
Hopefully someone will merge/apply this patch someday, thanks.
Re: [PATCH v2] drm/mediatek: Fix potential memory leak if vmap() fail
Hi,
Thanks a lot!
On 2023/7/27 09:47, CK Hu (胡俊光) wrote:
Hi, Jingfeng:
On Thu, 2023-07-06 at 21:40 +0800, Sui Jingfeng wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> Also return -ENOMEM if such a failure happens, the implement should
> take
> responsibility for the error handling.
Reviewed-by: CK Hu
>
> Fixes: 3df64d7b0a4f ("drm/mediatek: Implement gem prime vmap/vunmap
> function")
> Reviewed-by: Matthias Brugger
> Reviewed-by: Alexandre Mergnat
> Signed-off-by: Sui Jingfeng
> ---
> drivers/gpu/drm/mediatek/mtk_drm_gem.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index a25b28d3ee90..9f364df52478 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -247,7 +247,11 @@ int mtk_drm_gem_prime_vmap(struct drm_gem_object
> *obj, struct iosys_map *map)
>
> mtk_gem->kvaddr = vmap(mtk_gem->pages, npages, VM_MAP,
> pgprot_writecombine(PAGE_KERNEL));
> -
> +if (!mtk_gem->kvaddr) {
> +kfree(sgt);
> +kfree(mtk_gem->pages);
> +return -ENOMEM;
> +}
> out:
> kfree(sgt);
> iosys_map_set_vaddr(map, mtk_gem->kvaddr);
> --
> 2.34.1
* MEDIATEK Confidentiality Notice
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe
that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
Re: drm/ast: Do not enable PCI resources multiple times
Hi,
On 2023/7/25 02:34, Thomas Zimmermann wrote:
Hi
Am 18.07.23 um 07:40 schrieb suijingfeng:
Hi,
Actually, I'm only a little bit worry about the ast_pm_thaw() code
path.
|- ast_pm_thaw()
|-- ast_drm_thaw()
|--- ast_post_gpu()
I'm not quite sure what mean here, because the post-GPU code is not
involved in this patch.
Ah, I'm get confused with a previous patch.
Previously, the drm/ast driver call ast_init_pci_config() function in
ast_post_gpu().
I also don't know why it need to do so.
Ok then, just remove it.
All this patch does is to remove duplicated code.
Yes, this patch has nothing to do with the ast_post_gpu() function.
Is there a bug in the post-GPU handling?
No, I'm not find one so far.
Best regards
Thomas
Except this, all other code path have pci_enable_device() or
pcim_enable_device() called.
So, this patch seems OK.
On 2023/7/12 21:08, Thomas Zimmermann wrote:
Remove ast_init_pci_config() as the ast driver already enables the PCI
resources by calling pcim_enable_device().
Suggested-by: Sui Jingfeng
Signed-off-by: Thomas Zimmermann
Reviewed-by: Jocelyn Falempe
---
drivers/gpu/drm/ast/ast_main.c | 21 -
1 file changed, 21 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c
b/drivers/gpu/drm/ast/ast_main.c
index 8bfbdfd86d77..dae365ed3969 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,23 +35,6 @@
#include "ast_drv.h"
-static int ast_init_pci_config(struct pci_dev *pdev)
-{
- int err;
- u16 pcis04;
-
- err = pci_read_config_word(pdev, PCI_COMMAND, );
- if (err)
- goto out;
-
- pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-
- err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
-
-out:
- return pcibios_err_to_errno(err);
-}
-
static bool ast_is_vga_enabled(struct drm_device *dev)
{
struct ast_device *ast = to_ast_device(dev);
@@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const
struct drm_driver *drv,
return ERR_PTR(-EIO);
}
- ret = ast_init_pci_config(pdev);
- if (ret)
- return ERR_PTR(ret);
-
if (!ast_is_vga_enabled(dev)) {
drm_info(dev, "VGA not enabled on entry, requesting chip
POST\n");
need_post = true;
Re: [PATCH 3/6] PCI/VGA: drop the inline of vga_update_device_decodes() function
PING, please !
On 2023/7/11 21:43, Sui Jingfeng wrote:
From: Sui Jingfeng
The vga_update_device_decodes() function is NOT a trivial function, It is
not performance critical either. So, drop the inline.
This patch also make the parameter and argument consistent, use unsigned
int type instead of the signed type to store the decode. Change the second
argument of vga_update_device_decodes() function as 'unsigned int' type.
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 021116ed61cb..668139f7c247 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -860,24 +860,24 @@ static bool vga_arbiter_del_pci_device(struct pci_dev
*pdev)
return ret;
}
-/* this is called with the lock */
-static inline void vga_update_device_decodes(struct vga_device *vgadev,
-int new_decodes)
+/* This is called with the lock */
+static void vga_update_device_decodes(struct vga_device *vgadev,
+ unsigned int new_decodes)
{
struct device *dev = >pdev->dev;
- int old_decodes, decodes_removed, decodes_unlocked;
+ unsigned int old_decodes = vgadev->decodes;
+ unsigned int decodes_removed = ~new_decodes & old_decodes;
+ unsigned int decodes_unlocked = vgadev->locks & decodes_removed;
- old_decodes = vgadev->decodes;
- decodes_removed = ~new_decodes & old_decodes;
- decodes_unlocked = vgadev->locks & decodes_removed;
vgadev->decodes = new_decodes;
- vgaarb_info(dev, "changed VGA decodes: olddecodes=%s,decodes=%s:owns=%s\n",
- vga_iostate_to_str(old_decodes),
- vga_iostate_to_str(vgadev->decodes),
- vga_iostate_to_str(vgadev->owns));
+ vgaarb_info(dev,
+ "VGA decodes changed: olddecodes=%s,decodes=%s:owns=%s\n",
+ vga_iostate_to_str(old_decodes),
+ vga_iostate_to_str(vgadev->decodes),
+ vga_iostate_to_str(vgadev->owns));
- /* if we removed locked decodes, lock count goes to zero, and release */
+ /* If we removed locked decodes, lock count goes to zero, and release */
if (decodes_unlocked) {
if (decodes_unlocked & VGA_RSRC_LEGACY_IO)
vgadev->io_lock_cnt = 0;
Re: [PATCH 4/6] PCI/VGA: Move the new_state assignment out the loop
PING, please !
On 2023/7/11 21:43, Sui Jingfeng wrote:
From: Sui Jingfeng
In the vga_arbiter_notify_clients() function, the value of the 'new_state'
variable will be 'false' on systems that have more than one PCI VGA card.
Its value will be 'true' on systems that have one or no PCI VGA compatible
card. In other words, its value is not relevant to the iteration, so move
the assignment () out of the loop.
For a system with multiple video cards, this patch saves the redundant
assignment.
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index 668139f7c247..4c448c758bab 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -1468,22 +1468,20 @@ static void vga_arbiter_notify_clients(void)
{
struct vga_device *vgadev;
unsigned long flags;
- uint32_t new_decodes;
- bool new_state;
+ bool state;
if (!vga_arbiter_used)
return;
+ state = (vga_count > 1) ? false : true;
+
spin_lock_irqsave(_lock, flags);
list_for_each_entry(vgadev, _list, list) {
- if (vga_count > 1)
- new_state = false;
- else
- new_state = true;
if (vgadev->set_decode) {
- new_decodes = vgadev->set_decode(vgadev->pdev,
-new_state);
- vga_update_device_decodes(vgadev, new_decodes);
+ unsigned int decodes;
+
+ decodes = vgadev->set_decode(vgadev->pdev, state);
+ vga_update_device_decodes(vgadev, decodes);
}
}
spin_unlock_irqrestore(_lock, flags);
Re: [PATCH v3 0/9] PCI/VGA: Improve the default VGA device selection
Hi,
On 2023/7/20 03:32, Bjorn Helgaas wrote:
"drm/loongson: Add an implement for ..." also solves a problem, but it
lacks a commit log, so I don't know what the problem is.
I have already telling you one yeas ago.
I want remove the pci_fixup_vgadev() function in arch/loongarch/pci/pci.c
I was the original author of this workaround at our downstream kernel.
While the time is not mature until this patch set be merged.
I don't want mention this, as you asked this question.
So, I think I have to explain.
-static void pci_fixup_vgadev(struct pci_dev *pdev)
-{
- struct pci_dev *devp = NULL;
-
- while ((devp = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, devp))) {
- if (devp->vendor != PCI_VENDOR_ID_LOONGSON) {
- vga_set_default_device(devp);
- dev_info(>dev,
- "Overriding boot device as %X:%X\n",
- devp->vendor, devp->device);
- }
- }
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON,
PCI_DEVICE_ID_LOONGSON_DC1, pci_fixup_vgadev);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_LOONGSON,
PCI_DEVICE_ID_LOONGSON_DC2, pci_fixup_vgadev);
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
Hi,
Thanks for you noticed my change.
On 2023/7/20 03:32, Bjorn Helgaas wrote:
@@ -1509,13 +1543,24 @@ static int pci_notify(struct notifier_block *nb,
unsigned long action,
* cases of hotplugable vga cards.
*/
- if (action == BUS_NOTIFY_ADD_DEVICE)
+ switch (action) {
+ case BUS_NOTIFY_ADD_DEVICE:
notify = vga_arbiter_add_pci_device(pdev);
- else if (action == BUS_NOTIFY_DEL_DEVICE)
+ if (notify)
+ vga_arbiter_notify_clients();
+ break;
+ case BUS_NOTIFY_DEL_DEVICE:
notify = vga_arbiter_del_pci_device(pdev);
+ if (notify)
+ vga_arbiter_notify_clients();
+ break;
+ case BUS_NOTIFY_BOUND_DRIVER:
+ vga_arbiter_do_arbitration(pdev);
+ break;
+ default:
+ break;
+ }
Changing from if/else to switch makes the patch bigger than necessary
for no real benefit and obscures what is really changing.
Actually, the logic become more clear after this patch applied.
```
switch (action) {
case BUS_NOTIFY_ADD_DEVICE:
notify = vga_arbiter_add_pci_device(pdev);
if (notify)
vga_arbiter_notify_clients();
break;
case BUS_NOTIFY_DEL_DEVICE:
notify = vga_arbiter_del_pci_device(pdev);
if (notify)
vga_arbiter_notify_clients();
break;
case BUS_NOTIFY_BOUND_DRIVER:
vga_arbiter_do_arbitration(pdev);
break;
default:
break;
}
```
Because we only need call vga_arbiter_notify_clients() when action ==
BUS_NOTIFY_ADD_DEVICE or action == BUS_NOTIFY_DEL_DEVICE,
But *NOT* when the action equals to BUS_NOTIFY_BOUND_DRIVER.
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
Hi, On 2023/7/20 03:32, Bjorn Helgaas wrote: 2) It does not take the PCI Bar may get relocated into consideration. 3) It is not effective for the PCI device without a dedicated VRAM Bar. 4) It is device-agnostic, thus it has to waste the effort to iterate all of the PCI Bar to find the VRAM aperture. 5) It has invented lots of methods to determine which one is the default boot device, but this is still a policy because it doesn't give the user a choice to override. I don't think we need a list of*potential* problems. We need an example of the specific problem this will solve, i.e., what currently does not work? This version do allow the arbitration service works on non-x86 arch, which also allow me remove a arch-specific workaround. I will give more detail at the next version. But I want to provide one more drawback of vgaarb here: (6) It does not works for non VGA-compatible PCI(e) display controllers. Because, currently, vgaarb deal with PCI VGA compatible devices only. See another my patch set [1] for more elaborate discussion. It also ignore PCI_CLASS_NOT_DEFINED_VGA as Maciej puts it[2]. While my approach do not required the display controller to be VGA-compatible to enjoy the arbitration service. What do you think then? [1] https://patchwork.freedesktop.org/patch/546690/?series=120548=1 [2] https://lkml.org/lkml/2023/6/18/315
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
Hi, I was too hurry reply to you. I'm may miss the point for part of your reviews, Sorry. On 2023/7/20 03:32, Bjorn Helgaas wrote: CONFIG_DRM_AST is a tristate. We're talking about identifying the boot-time console device. Yes, my patch will only works *after* the module gets loaded successfully. But generally, vgaarb will select a default boot device before my patch taking into effect. I means that vgaarb will select a default boot device by calling vga_arbiter_add_pci_device() function. In practice, I still not notice any obvious problems. I'm lack the knowledge about the boot-time console, what is the potential problems with such a condition? So if CONFIG_DRM_AST=m, I guess we don't get the benefit of the new callback unless the module gets loaded? Yes, my approach will not works until the device driver kernel module gets loaded successfully. So what's the problem with such a situation, do you see something weird ?
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
Hi, On 2023/7/20 02:26, Bjorn Helgaas wrote: Optimization is fine, but the most important thing here is to be clear about what functional change this patch makes. As I mentioned at [1], if this patch affects the class codes accepted, please make that clear here. Reviewed-by: Mario Limonciello Signed-off-by: Sui Jingfeng I do not see Mario's Reviewed-by on the list. I do see Mario's Reviewed-by [2] for a previous version, but that version added this in pci_notify(): + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8) + return 0; while this version adds: + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA) + return 0; It's OK to carry a review to future versions if there are insignificant changes, but this is a functional change that seems significant to me. The first matches only 0x03, while the second discards the low eight bits so it matches 0x0300XX. [1]https://lore.kernel.org/r/20230718231400.GA496927@bhelgaas [2]https://lore.kernel.org/all/5b6fdf65-b354-94a9-f883-be820157e...@amd.com/ Yes, you are right. As you already told me at [1]: According to the "PCI Code and ID Assignment" spec, r1.15, sec 1.4, only mentions 0x0300 programming interface 0x00 as decoding the legacy VGA addresses. If the programming interface is 0x01, then it is a 8514-compatible controller. It is petty old card, about 30 years old(I think, it is nearly obsolete for now). I never have a chance to see such a card in real life. Yes, we should adopt first matches method here. That is: + if (pdev->class != PCI_CLASS_DISPLAY_VGA << 8) + return 0; It seems that we are more rigorous to deal the VGA-compatible devices as illustrated by above code here. But who the still have that card (8514-compatible) and the hardware to using such a card today ? Please consider that the pci_dev_attrs_are_visible() function[3] also ignore the programming interface (the least significant 8 bits). Therefore, at this version of my vgaarb cleanup patch set. I choose to keep the original filtering rule, but do the necessary optimization only which I think is meaningful. In the future, we may want to expand VGAARB to deal all PCI display class devices, with another patch. if (pdev->class >> 16 == PCI_BASE_CLASS_DISPLAY) // accept else // return immediately. Then, we will have a more good chance to clarify the programmer interface. Is this explanation feasible and reasonable, Bjorn and Mario ? [3] https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-sysfs.c#L1551
Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx
Hi,
On 2023/7/20 18:07, Maxime Ripard wrote:
On Wed, Jul 19, 2023 at 09:12:14PM +0200, Javier Martinez Canillas wrote:
suijingfeng writes:
Hi,
On 2023/7/10 15:47, Maxime Ripard wrote:
[...]
+
+/**
+ * drm_kunit_helper_context_alloc - Allocates an acquire context
+ * @test: The test context object
+ *
+ * Allocates and initializes a modeset acquire context.
+ *
+ * The context is tied to the kunit test context, so we must not call
+ * drm_modeset_acquire_fini() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated context otherwise
+ */
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
+{
+ struct drm_modeset_acquire_ctx *ctx;
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+ drm_modeset_acquire_init(ctx, 0);
+
+ ret = kunit_add_action_or_reset(test,
+ action_drm_release_context,
+ ctx);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+
I think all of the patch inside this series are quite well.
Personally, I can't find problems in it.
But I still want to ask a question:
Should the managed functions you introduced be prefixed with drmm_
(instead of drm_) ?
That's a good question. But personally I think that the drmm_ prefix
should be reserved for drm_device managed resources and helpers.
Yeah, to me drmm functions are meant for resources that are tied to the
DRM device lifetime. These resources are tied to the test lifetime, so
they shouldn't share the same prefix.
Okay then,
Thanks for reply.
As mindless programmer may still want to call drm_modeset_acquire_fini()
on the pointer returned by
drm_kunit_helper_acquire_ctx_alloc()?
The function kernel-doc already mentions that there's no need to do that
and that will be done automatically by kunit. So shouldn't be different of
other functions helper where the programmer didn't read the documentation.
Right
Maxime
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
On 2023/7/20 03:32, Bjorn Helgaas wrote: but I think it's just confusing to mention this in the commit log, so I would just remove it. Ok, will be done at the next version.
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
Hi, On 2023/7/20 03:32, Bjorn Helgaas wrote: [+cc linux-pci (please cc in the future since the bulk of this patch is in drivers/pci/)] On Wed, Jul 12, 2023 at 12:43:05AM +0800, Sui Jingfeng wrote: From: Sui Jingfeng Currently, the strategy of selecting the default boot on a multiple video card coexistence system is not perfect. Potential problems are: 1) This function is a no-op on non-x86 architectures. Which function in particular is a no-op for non-x86? I refer to the vga_is_firmware_default() function, I will improve the commit message at the next version. (To make it more human readable). Thanks you point it out. 2) It does not take the PCI Bar may get relocated into consideration. 3) It is not effective for the PCI device without a dedicated VRAM Bar. 4) It is device-agnostic, thus it has to waste the effort to iterate all of the PCI Bar to find the VRAM aperture. 5) It has invented lots of methods to determine which one is the default boot device, but this is still a policy because it doesn't give the user a choice to override. I don't think we need a list of *potential* problems. We need an example of the specific problem this will solve, i.e., what currently does not work? 1) The selection of primary GPU on Non-x86 platform. (Arm64, risc-v, powerpc etc) Mostly server platforms have equipped with aspeed bmc, and such hardware platforms have a lot PCIe slot. So I think, aspeed bmc V.S (P.K) radeon(or amdgpu) is very common. 2) The ability to pass the control back to the end user. Convert the *device driven* to the "driver driven" or "human driven". Currently, it is the machine making the decision. Emm, I probably will be able to give some examples at the next version. The drm/ast and maybe drm/loongson patches are the only ones that use the new callback, so I assume there are real problems with those drivers. CONFIG_DRM_AST is a tristate. We're talking about identifying the boot-time console device. So if CONFIG_DRM_AST=m, I guess we don't get the benefit of the new callback unless the module gets loaded? Since, this patch set is mostly for the user of X server. It is actually okey if CONFIG_DRM_AST=m. (it will be works no matter CONFIG_DRM_AST=m or CONFIG_DRM_AST=y) As the device and the driver bound at a latter time. So we are lucky, we need this behavior to implement the override.
Re: [Intel-gfx] [PATCH v3 3/9] PCI/VGA: Switch to aperture_contain_firmware_fb_nonreloc()
Hi,
On 2023/7/20 04:43, Bjorn Helgaas wrote:
[+cc linux-pci; I don't apply or ack PCI patches unless they appear there]
On Wed, Jul 12, 2023 at 12:43:04AM +0800, Sui Jingfeng wrote:
From: Sui Jingfeng
The observation behind this is that we should avoid accessing the global
screen_info directly. Call the aperture_contain_firmware_fb_nonreloc()
function to implement the detection of whether an aperture contains the
firmware FB.
Because it's better to access the global screen_info from
aperture_contain_firmware_fb_nonreloc()? The reasoning here is not
super clear to me.
Yes, honestly the benefits of this patch is not obvious.
But I do have some (may not practical) ideas in my mind when I create
this patch.
See my explanation at the end.
This patch helps to decouple the determination from the implementation.
Or, in other words, we intend to make the determination opaque to the
caller. The determination may choose to be arch-dependent or
arch-independent. But vgaarb, as a consumer of the determination,
shouldn't care how the does determination is implemented.
"how the determination ..." (drop the "does")
Ok, will be fixed at the next version.
Are you saying that aperture_contain_firmware_fb_nonreloc() might be
arch-dependent? Are there multiple callers? Or does this just move
code from one place to a more appropriate place?
1) To form a unify approach, and drop the screen_info.h header.
There are similar cleanup patch at patchwork.
screen_info.h is definitely arch-dependent, while vgaarb is just
device-dependent.
I think, they do have subtle difference.
2) Convert the *device driven* to the "driver driven".
Move it from vgaarb.c to video/apperture allow code sharing.
While this function are not going to be shared in vgaarb.
Previous it is the device make the decision,
after applied this patch it allow driver make the decision.
They do have subtle difference.
Emm, I will try to give some examples at the next version.
3) I was imagine to drag platform display controllers in (get platform
devices involved in the arbitration).
As Alex seem hint to implement something platform-independent.
The aperture_contain_firmware_fb_nonreloc() actually is possible be shared.
The aperture of platform device will be not moved.
So it seems that platform device driver could call this function to do
something else.
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index bf96e085751d..953daf731b2c 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -14,6 +14,7 @@
#define vgaarb_info(dev, fmt, arg...) dev_info(dev, "vgaarb: " fmt, ##arg)
#define vgaarb_err(dev, fmt, arg...) dev_err(dev, "vgaarb: " fmt, ##arg)
+#include
#include
#include
#include
@@ -26,7 +27,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -558,20 +558,11 @@ void vga_put(struct pci_dev *pdev, unsigned int rsrc)
}
EXPORT_SYMBOL(vga_put);
+/* Select the device owning the boot framebuffer if there is one */
static bool vga_is_firmware_default(struct pci_dev *pdev)
{
#if defined(CONFIG_X86) || defined(CONFIG_IA64)
- u64 base = screen_info.lfb_base;
- u64 size = screen_info.lfb_size;
struct resource *r;
- u64 limit;
-
- /* Select the device owning the boot framebuffer if there is one */
-
- if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
- base |= (u64)screen_info.ext_lfb_base << 32;
-
- limit = base + size;
/* Does firmware framebuffer belong to us? */
pci_dev_for_each_resource(pdev, r) {
@@ -581,10 +572,8 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
if (!r->start || !r->end)
continue;
- if (base < r->start || limit >= r->end)
- continue;
-
- return true;
+ if (aperture_contain_firmware_fb_nonreloc(r->start, r->end))
+ return true;
}
#endif
return false;
--
2.25.1
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
Hi, On 2023/7/20 05:13, Sui Jingfeng wrote: Otherwise there 30+ noisy(useless) events got snooped. See below: ``` [ 0.246077] pci :01:00.0: vgaarb: setting as boot VGA device [ 0.246077] pci :01:00.0: vgaarb: bridge control possible [ 0.246077] pci :01:00.0: vgaarb: VGA device added: decodes=io+mem,owns=io+mem,locks=none [ 0.246077] vgaarb: loaded [ 0.294169] skl_uncore :00:00.0: vgaarb: pci_notify: action=3 [ 0.294182] skl_uncore :00:00.0: vgaarb: pci_notify: action=4 [ 0.301297] pcieport :00:01.0: vgaarb: pci_notify: action=3 [ 0.301482] pcieport :00:01.0: vgaarb: pci_notify: action=4 [ 0.301488] pcieport :00:1c.0: vgaarb: pci_notify: action=3 [ 0.301705] pcieport :00:1c.0: vgaarb: pci_notify: action=4 [ 1.806445] xhci_hcd :00:14.0: vgaarb: pci_notify: action=3 [ 1.810976] ahci :00:17.0: vgaarb: pci_notify: action=3 [ 1.824383] xhci_hcd :00:14.0: vgaarb: pci_notify: action=4 [ 1.857470] ahci :00:17.0: vgaarb: pci_notify: action=4 [ 4.692700] intel_pch_thermal :00:14.2: vgaarb: pci_notify: action=3 [ 4.693110] intel_pch_thermal :00:14.2: vgaarb: pci_notify: action=4 [ 4.746712] i801_smbus :00:1f.4: vgaarb: pci_notify: action=3 [ 4.747212] pci :00:1f.1: vgaarb: pci_notify: action=0 [ 4.747227] pci :00:1f.1: vgaarb: pci_notify: action=1 [ 4.747250] pci :00:1f.1: vgaarb: pci_notify: action=2 [ 4.749098] i801_smbus :00:1f.4: vgaarb: pci_notify: action=4 [ 4.799217] mei_me :00:16.0: vgaarb: pci_notify: action=3 [ 4.802503] mei_me :00:16.0: vgaarb: pci_notify: action=4 [ 4.874880] intel-lpss :00:15.0: vgaarb: pci_notify: action=3 [ 4.881227] intel-lpss :00:15.0: vgaarb: pci_notify: action=4 [ 4.881240] intel-lpss :00:15.1: vgaarb: pci_notify: action=3 [ 4.887578] intel-lpss :00:15.1: vgaarb: pci_notify: action=4 [ 4.985796] r8169 :02:00.0: vgaarb: pci_notify: action=3 [ 4.991862] r8169 :02:00.0: vgaarb: pci_notify: action=4 [ 5.404835] snd_hda_intel :00:1f.3: vgaarb: pci_notify: action=3 [ 5.405175] snd_hda_intel :00:1f.3: vgaarb: pci_notify: action=4 [ 5.405401] snd_hda_intel :01:00.1: vgaarb: pci_notify: action=3 [ 5.405973] snd_hda_intel :01:00.1: vgaarb: pci_notify: action=4 [ 10.793665] i915 :00:02.0: vgaarb: pci_notify: action=3 [ 11.201384] i915 :00:02.0: vgaarb: pci_notify: action=4 [ 16.135842] amdgpu :01:00.0: vgaarb: pci_notify: action=3 [ 16.140458] amdgpu :01:00.0: vgaarb: deactivate vga console [ 16.638564] amdgpu :01:00.0: vgaarb: pci_notify: action=4 ``` After apply my patch, this events are still will notify me, it is just that if we found it is irrelevant, we will return immediately. No further process is needed.
Re: [PATCH v3 1/9] video/aperture: Add a helper to detect if an aperture contains firmware FB
Hi, On 2023/7/20 04:43, Bjorn Helgaas wrote: On Wed, Jul 12, 2023 at 12:43:02AM +0800, Sui Jingfeng wrote: From: Sui Jingfeng This patch adds the aperture_contain_firmware_fb() function to do the determination. Unfortunately, due to the fact that the apertures list will be freed dynamically, the location and size information of the firmware FB will be lost after dedicated drivers call aperture_remove_conflicting_devices(), aperture_remove_conflicting_pci_devices() or aperture_remove_all_conflicting_devices() functions We solve this problem by introducing two static variables that record the firmware framebuffer's start addrness and end addrness. It assumes that the system has only one active firmware framebuffer driver at a time. We don't use the global structure screen_info here, because PCI resources may get reallocated (the VRAM BAR could be moved) during the kernel boot stage. s/addrness/address/ (twice) Will be fixed at the next version, thanks.
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
On 2023/7/20 03:58, Sui Jingfeng wrote: On the other hand, even though the lest significant 8 but if pdev->class is really matter. If the low eight bits of pdev->class is really matters, maybe we should wait the potential problems became severe. Currently, it is not obvious.
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
On 2023/7/20 03:58, Sui Jingfeng wrote: My explanation about the minor tweak being made before this version and previous version is that I want to keep my patch *less distraction*. The minor tweak being made between this version and previous version is to keep my patch *less distraction*.
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
On 2023/7/20 03:58, Sui Jingfeng wrote: What this version adds here is *same* before this patch set is applied. The filter method is *same* , in the cases of before this patch is applied and after this patch is applied.
Re: [PATCH v2] drm/loongson: Add a check for lsdc_bo_create() errors
Hi,
This version is ok, I'll apply this patch within a few days.
On 2023/7/19 16:45, Dan Carpenter wrote:
This code doesn't check for lsdc_bo_create() failure and it could lead
to a crash. It can fail for a variety of reasons, but the most common
cause would be low memory. Add a check.
Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Signed-off-by: Dan Carpenter
Reviewed-by: Sui Jingfeng
---
v2: change subject and re-word the commit message
drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c
b/drivers/gpu/drm/loongson/lsdc_ttm.c
index bb0c8fd43a75..bf79dc55afa4 100644
--- a/drivers/gpu/drm/loongson/lsdc_ttm.c
+++ b/drivers/gpu/drm/loongson/lsdc_ttm.c
@@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct
drm_device *ddev,
int ret;
lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL);
+ if (IS_ERR(lbo))
+ return ERR_CAST(lbo);
ret = lsdc_bo_reserve(lbo);
if (unlikely(ret)) {
Re: [PATCH] drm: loongson: Add a check for lsdc_bo_create() errors
Hi, I still remember you are helps to review the drm/lsdc patch one years ago, see [1]. drm/lsdc is the former version of drm/loongson, originally drm/lsdc are embedded SoCs of Loongson. [1] https://patchwork.freedesktop.org/patch/471997/?series=99512=5 I haven't forget about you. On 2023/7/18 21:59, Dan Carpenter wrote: Even if the fail happened, your patch is not fixing the root problem. What is the correct fix then? lsdc_bo_create_kernel_pinned() is intend to be used when you do the self test (cat benchmark) which is using to testing the hardware bandwidth via debugfs. Another potential usage is to implement built in fbdev emulation. I admit the error handling is necessary, but it's a kind of costuming. To be honest, I have also post similar patches to other DRM drivers.:-) So, that is okay. But let's back to the technique, when the kzalloc() fails, who will survives? If the the kzalloc() fail, then I think the implement of kzalloc() should be blamed first. while developing this driver nearly about two years, it rare happens. For my drivers, kzalloc() fails is one sort of error, lsdc_bo_create() could fail when no enough VRAM is another. The dumb_buffer test if IGT would helps to probe such failures. But this is a kind of limitation of the hardware itself. Its a resource limitation, even the drm/radeon could probably fail. So, How does your patch could resolve the root problems that caused by no enough resource available?
Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()
Hi,
On 2023/7/18 16:12, Lucas Stach wrote:
Hi Jingfeng,
Am Dienstag, dem 18.07.2023 um 02:34 +0800 schrieb suijingfeng:
Hi, Lucas
Thanks for you guidance!
On 2023/7/17 17:51, Lucas Stach wrote:
Hi Jingfeng,
Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
From: Sui Jingfeng
Because it is not used by the etnaviv_gem_new_impl() function,
no functional change.
I think it would make sense to move into the opposite direction: in
both callsites of etnaviv_gem_new_impl we immediately call
drm_gem_object_init with the size.
Really?
But there are multiple call path to the etnaviv_gem_new_impl() function.
Code path 1 (PRIME):
- etnaviv_gem_prime_import_sg_table()
-- etnaviv_gem_new_private()
--- etnaviv_gem_new_impl(dev, size, flags, ops, )
--- drm_gem_private_object_init(dev, obj, size)
Code path 2 (USERPTR):
- etnaviv_gem_new_userptr()
-- etnaviv_gem_new_private()
--- etnaviv_gem_new_impl(dev, size, flags, ops, )
--- drm_gem_private_object_init(dev, obj, size)
Code path 3 (construct a GEM buffer object for the user-space):
- etnaviv_ioctl_gem_new()
-- etnaviv_gem_new_handle()
--- etnaviv_gem_new_impl(dev, size, flags, _gem_shmem_ops, );
--- drm_gem_object_init(dev, obj, size);
If I understand this correctly:
Code path 1 is for cross device (and cross driver) buffer-sharing,
Code path 2 is going to share the buffer the userspace,
*Only* the code path 3 is to construct a GEM buffer object for the
user-space the userspace,
that is say, *only* the code path 3 need to do the backing memory
allocation work for the userspace.
thus it need to call drm_gem_object_init() function, which really the
shmem do the backing memory
allocation.
The code path 1 and the code path 2 do not need the kernel space
allocate the backing memory.
Because they are going to share the buffer already allocated by others.
thus, code path 2 and code path 3 should call drm_gem_private_object_init(),
*not* the drm_gem_object_init() function.
When import buffer from the a specific KMS driver,
then etnaviv_gem_prime_import_sg_table() will be called.
I guess you means that drm_gem_private_object_init() (not the
drm_gem_object_init() function)here ?
A better cleanup would be to make
use of the size parameter and move this object init call into
etnaviv_gem_new_impl.
If I following you guidance, how do I differentiate the cases
when to call drm_gem_private_object_init() not drm_gem_object_init() ?
and when call drm_gem_object_init() not drm_gem_private_object_init()?
I don't think you are right here.
Yes, clearly I was not taking into account the differences between
drm_gem_private_object_init and drm_gem_object_init properly. Please
disregard my comment, this patch is good as-is.
I have study your patch in the past frequently.
As you could solve very complex(and difficulty) bugs.
So I still believe that you know everything about etnaviv.
I'm just wondering that you are designing the traps. But I'm not sure.
Okay, still acceptable.
Because communicate will you is interesting.
Thank you.
Regards,
Lucas
Regards,
Lucas
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index b5f73502e3dd..be2f459c66b5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs
etnaviv_gem_object_funcs = {
.vm_ops = _ops,
};
-static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
+static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
{
struct etnaviv_gem_object *etnaviv_obj;
@@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct
drm_file *file,
size = PAGE_ALIGN(size);
- ret = etnaviv_gem_new_impl(dev, size, flags,
- _gem_shmem_ops, );
+ ret = etnaviv_gem_new_impl(dev, flags, _gem_shmem_ops, );
if (ret)
goto fail;
@@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
struct drm_gem_object *obj;
int ret;
- ret = etnaviv_gem_new_impl(dev, size, flags, ops, );
+ ret = etnaviv_gem_new_impl(dev, flags, ops, );
if (ret)
return ret;
Re: [PATCH] drm: loongson: Add a check for lsdc_bo_create() errors
Hi, On 2023/7/18 21:59, Dan Carpenter wrote: People have suggested that I misread this and that "bare brain" means through code review instead of testing. In context that seems to make sense. Sorry. Sorry for my broken English, that's really a misunderstanding. Anyway, the fixes tag is warranted. Okay, I'll accept this if no other experts object. To follow the convention of the DRM world, please rename the commit title by "drm/loongson: Add a check for lsdc_bo_create() errors". Reviewed-by: Sui Jingfeng With this small problem solved.
Re: [PATCH] drm: loongson: Add a check for lsdc_bo_create() errors
Hi,
On 2023/7/18 21:27, Dan Carpenter wrote:
Basically everything in this email was wrong to a kind of shocking
degree. For example, ignoring kmalloc() failure is a bug so the fixes
tag is definitely warranted. But then you called me "bare brained"
which seems like a personal attack
Sorry, that's a misunderstanding
Sorry for my broken English.
by "bare brain", I means that by using the brains(pure code review) only,
I conjure up this adjective from the word "bare metal".
When I reply you email, I lack a word to describe this.
I believe that many experts have this sort of ability,
they could create a patch by simply give a glimpse of the code.
because they know how does the code run at the very low level.
so I'm going to report this as a code
of conduct violation.
Sorry Dan, you are welcomed.
Please withdraw this report.
I don't know why are you angry.
Because our hardware is rare,
Originally, by using the words "bare brain", I means by "pure brain",
I means that you probably without a hardware platform to do verification.
I want to know that if you have tested your patch on the board.
Or, I want to probe that if you have our hardware.
I will consider to send you one if you are long time contributor and if
you are really interested.
I have a lot of boards, now I'm feel a little bit tired by developing
drivers for all of them.
Please withdraw that report, personal attack tend to continues(across)
to multiple thread.
Sorry for my broken English. I will improve my written skill in the long
term.
Thanks for you contribution.
regards,
dan carpenter
On Tue, Jul 18, 2023 at 08:32:02PM +0800, suijingfeng wrote:
Hi,
Thanks for the patch.
The commit title generally should be 'drm/looongson: Add a check for
lsdc_bo_create() errors'
not 'drm: loongson: '
On 2023/7/18 15:01, Dan Carpenter wrote:
The lsdc_bo_create() function can fail so add a check for that.
Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Please drop this Fixes tag, because you patch just add a error handling.
Yes, the lsdc_bo_create() function can fail, but this is generally not
happen.
Even if the fail happened, your patch is not fixing the root problem.
I know that you create this patch with the bare brain,
I would like hear more practical usage or bugs of this driver.
And mention more in the commit message is preferred.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c
b/drivers/gpu/drm/loongson/lsdc_ttm.c
index bb0c8fd43a75..bf79dc55afa4 100644
--- a/drivers/gpu/drm/loongson/lsdc_ttm.c
+++ b/drivers/gpu/drm/loongson/lsdc_ttm.c
@@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct
drm_device *ddev,
int ret;
lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL);
+ if (IS_ERR(lbo))
+ return ERR_CAST(lbo);
ret = lsdc_bo_reserve(lbo);
if (unlikely(ret)) {
Re: [PATCH] drm: loongson: Add a check for lsdc_bo_create() errors
"drm/loongson: Add a check for lsdc_bo_create() errors"
On 2023/7/18 15:01, Dan Carpenter wrote:
The lsdc_bo_create() function can fail so add a check for that.
Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c
b/drivers/gpu/drm/loongson/lsdc_ttm.c
index bb0c8fd43a75..bf79dc55afa4 100644
--- a/drivers/gpu/drm/loongson/lsdc_ttm.c
+++ b/drivers/gpu/drm/loongson/lsdc_ttm.c
@@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct
drm_device *ddev,
int ret;
lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL);
+ if (IS_ERR(lbo))
+ return ERR_CAST(lbo);
ret = lsdc_bo_reserve(lbo);
if (unlikely(ret)) {
Re: [PATCH] drm: loongson: Add a check for lsdc_bo_create() errors
Hi,
Thanks for the patch.
The commit title generally should be 'drm/looongson: Add a check for
lsdc_bo_create() errors'
not 'drm: loongson: '
On 2023/7/18 15:01, Dan Carpenter wrote:
The lsdc_bo_create() function can fail so add a check for that.
Fixes: f39db26c5428 ("drm: Add kms driver for loongson display controller")
Please drop this Fixes tag, because you patch just add a error handling.
Yes, the lsdc_bo_create() function can fail, but this is generally not
happen.
Even if the fail happened, your patch is not fixing the root problem.
I know that you create this patch with the bare brain,
I would like hear more practical usage or bugs of this driver.
And mention more in the commit message is preferred.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/loongson/lsdc_ttm.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/loongson/lsdc_ttm.c
b/drivers/gpu/drm/loongson/lsdc_ttm.c
index bb0c8fd43a75..bf79dc55afa4 100644
--- a/drivers/gpu/drm/loongson/lsdc_ttm.c
+++ b/drivers/gpu/drm/loongson/lsdc_ttm.c
@@ -496,6 +496,8 @@ struct lsdc_bo *lsdc_bo_create_kernel_pinned(struct
drm_device *ddev,
int ret;
lbo = lsdc_bo_create(ddev, domain, size, true, NULL, NULL);
+ if (IS_ERR(lbo))
+ return ERR_CAST(lbo);
ret = lsdc_bo_reserve(lbo);
if (unlikely(ret)) {
Re: drm/ast: Do not enable PCI resources multiple times
Hi,
Actually, I'm only a little bit worry about the ast_pm_thaw() code path.
|- ast_pm_thaw()
|-- ast_drm_thaw()
|--- ast_post_gpu()
Except this, all other code path have pci_enable_device() or
pcim_enable_device() called.
So, this patch seems OK.
On 2023/7/12 21:08, Thomas Zimmermann wrote:
Remove ast_init_pci_config() as the ast driver already enables the PCI
resources by calling pcim_enable_device().
Suggested-by: Sui Jingfeng
Signed-off-by: Thomas Zimmermann
Reviewed-by: Jocelyn Falempe
---
drivers/gpu/drm/ast/ast_main.c | 21 -
1 file changed, 21 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 8bfbdfd86d77..dae365ed3969 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,23 +35,6 @@
#include "ast_drv.h"
-static int ast_init_pci_config(struct pci_dev *pdev)
-{
- int err;
- u16 pcis04;
-
- err = pci_read_config_word(pdev, PCI_COMMAND, );
- if (err)
- goto out;
-
- pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-
- err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
-
-out:
- return pcibios_err_to_errno(err);
-}
-
static bool ast_is_vga_enabled(struct drm_device *dev)
{
struct ast_device *ast = to_ast_device(dev);
@@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct
drm_driver *drv,
return ERR_PTR(-EIO);
}
- ret = ast_init_pci_config(pdev);
- if (ret)
- return ERR_PTR(ret);
-
if (!ast_is_vga_enabled(dev)) {
drm_info(dev, "VGA not enabled on entry, requesting chip
POST\n");
need_post = true;
Re: drm/ast: Do not enable PCI resources multiple times
Hi,
I have tested this patch on my x86-64(i3-8100, H110 D4L board) + ast2400
discrete BMC card just now,
drm/ast still works on normal case.
But originally this function is called in ast_post_gpu() function.
ast_post_gpu() doesn't happen on my test case.
I know something about the POST (Power On Self Test),
In the past, my mips and loongarch have difficulty to run x86 VBIOS code.
So part of my radeon and amdgpu gpu card will just hang(stall) at the
bios/vbios boot stage.
When we debugging, we hasty solve this problem by disabling (comment out)
the VBIOS part initialization code of the BIOS.
The results is that the screen will not light-up during the boot stage.
We relay the kernel-space drivers (drm/radeon and drm/amdgpu) to
initialize the GPU.
In this case, (drm/radeon and drm/amdgpu) driver log(dmesg) will print :
"need POST ...".
So, from my past experience, I think the "need POST" will only happens
when there no firmware support for a specific machine + BMC combination.
But this is not a normal case.
So, I think we probably could try this, if there no objections from
experienced reviewers.
On 2023/7/12 21:08, Thomas Zimmermann wrote:
Remove ast_init_pci_config() as the ast driver already enables the PCI
resources by calling pcim_enable_device().
Suggested-by: Sui Jingfeng
Signed-off-by: Thomas Zimmermann
Reviewed-by: Jocelyn Falempe
Tested-by: Sui Jingfeng
---
drivers/gpu/drm/ast/ast_main.c | 21 -
1 file changed, 21 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 8bfbdfd86d77..dae365ed3969 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -35,23 +35,6 @@
#include "ast_drv.h"
-static int ast_init_pci_config(struct pci_dev *pdev)
-{
- int err;
- u16 pcis04;
-
- err = pci_read_config_word(pdev, PCI_COMMAND, );
- if (err)
- goto out;
-
- pcis04 |= PCI_COMMAND_MEMORY | PCI_COMMAND_IO;
-
- err = pci_write_config_word(pdev, PCI_COMMAND, pcis04);
-
-out:
- return pcibios_err_to_errno(err);
-}
-
static bool ast_is_vga_enabled(struct drm_device *dev)
{
struct ast_device *ast = to_ast_device(dev);
@@ -483,10 +466,6 @@ struct ast_device *ast_device_create(const struct
drm_driver *drv,
return ERR_PTR(-EIO);
}
- ret = ast_init_pci_config(pdev);
- if (ret)
- return ERR_PTR(ret);
-
if (!ast_is_vga_enabled(dev)) {
drm_info(dev, "VGA not enabled on entry, requesting chip
POST\n");
need_post = true;
Re: [PATCH v1 3/8] drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl()
Hi, Lucas
Thanks for you guidance!
On 2023/7/17 17:51, Lucas Stach wrote:
Hi Jingfeng,
Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
From: Sui Jingfeng
Because it is not used by the etnaviv_gem_new_impl() function,
no functional change.
I think it would make sense to move into the opposite direction: in
both callsites of etnaviv_gem_new_impl we immediately call
drm_gem_object_init with the size.
Really?
But there are multiple call path to the etnaviv_gem_new_impl() function.
Code path 1 (PRIME):
|- etnaviv_gem_prime_import_sg_table()
|-- etnaviv_gem_new_private()
|--- etnaviv_gem_new_impl(dev, size, flags, ops, )
|--- drm_gem_private_object_init(dev, obj, size)
Code path 2 (USERPTR):
|- etnaviv_gem_new_userptr()
|-- etnaviv_gem_new_private()
|--- etnaviv_gem_new_impl(dev, size, flags, ops, )
|--- drm_gem_private_object_init(dev, obj, size)
Code path 3 (construct a GEM buffer object for the user-space):
|- etnaviv_ioctl_gem_new()
|-- etnaviv_gem_new_handle()
|--- etnaviv_gem_new_impl(dev, size, flags, _gem_shmem_ops, );
|--- drm_gem_object_init(dev, obj, size);
If I understand this correctly:
Code path 1 is for cross device (and cross driver) buffer-sharing,
Code path 2 is going to share the buffer the userspace,
*Only* the code path 3 is to construct a GEM buffer object for the
user-space the userspace,
that is say, *only* the code path 3 need to do the backing memory
allocation work for the userspace.
thus it need to call drm_gem_object_init() function, which really the
shmem do the backing memory
allocation.
The code path 1 and the code path 2 do not need the kernel space
allocate the backing memory.
Because they are going to share the buffer already allocated by others.
thus, code path 2 and code path 3 should call drm_gem_private_object_init(),
*not* the drm_gem_object_init() function.
When import buffer from the a specific KMS driver,
then etnaviv_gem_prime_import_sg_table() will be called.
I guess you means that drm_gem_private_object_init() (not the
drm_gem_object_init() function)here ?
A better cleanup would be to make
use of the size parameter and move this object init call into
etnaviv_gem_new_impl.
If I following you guidance, how do I differentiate the cases
when to call drm_gem_private_object_init() not drm_gem_object_init() ?
and when call drm_gem_object_init() not drm_gem_private_object_init()?
I don't think you are right here.
Regards,
Lucas
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index b5f73502e3dd..be2f459c66b5 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -542,7 +542,7 @@ static const struct drm_gem_object_funcs
etnaviv_gem_object_funcs = {
.vm_ops = _ops,
};
-static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
+static int etnaviv_gem_new_impl(struct drm_device *dev, u32 flags,
const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
{
struct etnaviv_gem_object *etnaviv_obj;
@@ -591,8 +591,7 @@ int etnaviv_gem_new_handle(struct drm_device *dev, struct
drm_file *file,
size = PAGE_ALIGN(size);
- ret = etnaviv_gem_new_impl(dev, size, flags,
- _gem_shmem_ops, );
+ ret = etnaviv_gem_new_impl(dev, flags, _gem_shmem_ops, );
if (ret)
goto fail;
@@ -627,7 +626,7 @@ int etnaviv_gem_new_private(struct drm_device *dev, size_t size, u32 flags,
struct drm_gem_object *obj;
int ret;
- ret = etnaviv_gem_new_impl(dev, size, flags, ops, );
+ ret = etnaviv_gem_new_impl(dev, flags, ops, );
if (ret)
return ret;
Re: [09/11] drm/vc4: tests: pv-muxing: Switch to managed locking init
Hi,
On 2023/7/10 15:47, Maxime Ripard wrote:
The new helper to init the locking context allows to remove some
boilerplate.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 42 --
1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
index 776a7b01608f..ff1deaed0cab 100644
--- a/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
+++ b/drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c
@@ -20,7 +20,6 @@
struct pv_muxing_priv {
struct vc4_dev *vc4;
- struct drm_modeset_acquire_ctx ctx;
struct drm_atomic_state *state;
};
@@ -725,6 +724,7 @@ static void drm_vc4_test_pv_muxing_invalid(struct kunit *test)
static int vc4_pv_muxing_test_init(struct kunit *test)
{
const struct pv_muxing_param *params = test->param_value;
+ struct drm_modeset_acquire_ctx *ctx;
struct drm_atomic_state *state;
struct pv_muxing_priv *priv;
struct drm_device *drm;
@@ -738,13 +738,14 @@ static int vc4_pv_muxing_test_init(struct kunit *test)
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
priv->vc4 = vc4;
- drm_modeset_acquire_init(>ctx, 0);
+ ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
The pointer returned by drm_kunit_helper_acquire_ctx_alloc() function
can't be NULL,
if ctx is NULL, the current kthread will be terminated by the
KUNIT_ASSERT_NOT_NULL() in the drm_kunit_helper_acquire_ctx_alloc().
so only a PTR_ERR is possible, right?
If so, probably invent a KUNIT_ASSERT_NOT_ERR() function to call is enough.
I'm fine with this patch, but I feel the checking if the ctx is NULL is
redundant.
drm = >base;
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx = >ctx;
+ state->acquire_ctx = ctx;
priv->state = state;
@@ -757,8 +758,6 @@ static void vc4_pv_muxing_test_exit(struct kunit *test)
struct drm_atomic_state *state = priv->state;
drm_atomic_state_put(state);
- drm_modeset_drop_locks(>ctx);
- drm_modeset_acquire_fini(>ctx);
}
static struct kunit_case vc4_pv_muxing_tests[] = {
@@ -798,7 +797,7 @@ static struct kunit_suite vc5_pv_muxing_test_suite = {
*/
static void drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit
*test)
{
- struct drm_modeset_acquire_ctx ctx;
+ struct drm_modeset_acquire_ctx *ctx;
struct drm_atomic_state *state;
struct vc4_crtc_state *new_vc4_crtc_state;
struct vc4_hvs_state *new_hvs_state;
@@ -811,13 +810,14 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
vc4 = vc5_mock_device(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
- drm_modeset_acquire_init(, 0);
+ ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = >base;
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx =
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -844,7 +844,7 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx =
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI1);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -866,13 +866,11 @@ static void
drm_test_vc5_pv_muxing_bugs_subsequent_crtc_enable(struct kunit *tes
KUNIT_EXPECT_NE(test, hdmi0_channel, hdmi1_channel);
drm_atomic_state_put(state);
- drm_modeset_drop_locks();
- drm_modeset_acquire_fini();
}
static void drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
{
- struct drm_modeset_acquire_ctx ctx;
+ struct drm_modeset_acquire_ctx *ctx;
struct drm_atomic_state *state;
struct vc4_crtc_state *new_vc4_crtc_state;
struct vc4_hvs_state *new_hvs_state;
@@ -885,13 +883,14 @@ static void
drm_test_vc5_pv_muxing_bugs_stable_fifo(struct kunit *test)
vc4 = vc5_mock_device(test);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, vc4);
- drm_modeset_acquire_init(, 0);
+ ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+ KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
drm = >base;
state = drm_atomic_state_alloc(drm);
KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
- state->acquire_ctx =
+ state->acquire_ctx = ctx;
ret = vc4_mock_atomic_add_output(test, state, VC4_ENCODER_TYPE_HDMI0);
KUNIT_ASSERT_EQ(test, ret, 0);
@@ -929,7 +928,7 @@ static void
Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx
Hi,
On 2023/7/10 15:47, Maxime Ripard wrote:
As we get more and more tests, the locking context initialisation
creates more and more boilerplate, both at creation and destruction.
Let's create a helper that will allocate, initialise a context, and
register kunit actions to clean up once the test is done.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++
include/drm/drm_kunit_helpers.h | 2 ++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 38211fea9ae6..40a27c78d692 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -124,5 +124,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct
kunit *test,
}
EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
+static void action_drm_release_context(void *ptr)
+{
+ struct drm_modeset_acquire_ctx *ctx = ptr;
+
+ drm_modeset_drop_locks(ctx);
+ drm_modeset_acquire_fini(ctx);
+}
+
+/**
+ * drm_kunit_helper_context_alloc - Allocates an acquire context
+ * @test: The test context object
+ *
+ * Allocates and initializes a modeset acquire context.
+ *
+ * The context is tied to the kunit test context, so we must not call
+ * drm_modeset_acquire_fini() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated context otherwise
+ */
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
+{
+ struct drm_modeset_acquire_ctx *ctx;
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
Because kunit_kzalloc() is also managed,
Is there any possibility that kfree(ctx) get called before
action_drm_release_context(ctx) ?
Currently, I can't find where the order is guaranteed.
+ KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+ drm_modeset_acquire_init(ctx, 0);
+
+ ret = kunit_add_action_or_reset(test,
+ action_drm_release_context,
+ ctx);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+
MODULE_AUTHOR("Maxime Ripard ");
MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index ed013fdcc1ff..4ba5e10653c6 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
sizeof(_type),
\
offsetof(_type, _member),
\
_feat))
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
#endif // DRM_KUNIT_HELPERS_H_
Re: [05/11] drm/tests: helpers: Create an helper to allocate a locking ctx
Hi,
On 2023/7/10 15:47, Maxime Ripard wrote:
As we get more and more tests, the locking context initialisation
creates more and more boilerplate, both at creation and destruction.
Let's create a helper that will allocate, initialise a context, and
register kunit actions to clean up once the test is done.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/tests/drm_kunit_helpers.c | 41 +++
include/drm/drm_kunit_helpers.h | 2 ++
2 files changed, 43 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
b/drivers/gpu/drm/tests/drm_kunit_helpers.c
index 38211fea9ae6..40a27c78d692 100644
--- a/drivers/gpu/drm/tests/drm_kunit_helpers.c
+++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
@@ -124,5 +124,46 @@ __drm_kunit_helper_alloc_drm_device_with_driver(struct
kunit *test,
}
EXPORT_SYMBOL_GPL(__drm_kunit_helper_alloc_drm_device_with_driver);
+static void action_drm_release_context(void *ptr)
+{
+ struct drm_modeset_acquire_ctx *ctx = ptr;
+
+ drm_modeset_drop_locks(ctx);
+ drm_modeset_acquire_fini(ctx);
+}
+
+/**
+ * drm_kunit_helper_context_alloc - Allocates an acquire context
+ * @test: The test context object
+ *
+ * Allocates and initializes a modeset acquire context.
+ *
+ * The context is tied to the kunit test context, so we must not call
+ * drm_modeset_acquire_fini() on it, it will be done so automatically.
+ *
+ * Returns:
+ * An ERR_PTR on error, a pointer to the newly allocated context otherwise
+ */
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test)
+{
+ struct drm_modeset_acquire_ctx *ctx;
+ int ret;
+
+ ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+ drm_modeset_acquire_init(ctx, 0);
+
+ ret = kunit_add_action_or_reset(test,
+ action_drm_release_context,
+ ctx);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(drm_kunit_helper_acquire_ctx_alloc);
+
I think all of the patch inside this series are quite well.
Personally, I can't find problems in it.
But I still want to ask a question:
Should the managed functions you introduced be prefixed with drmm_
(instead of drm_) ?
As mindless programmer may still want to call drm_modeset_acquire_fini()
on the pointer returned by
drm_kunit_helper_acquire_ctx_alloc()?
MODULE_AUTHOR("Maxime Ripard ");
MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_kunit_helpers.h b/include/drm/drm_kunit_helpers.h
index ed013fdcc1ff..4ba5e10653c6 100644
--- a/include/drm/drm_kunit_helpers.h
+++ b/include/drm/drm_kunit_helpers.h
@@ -87,5 +87,7 @@ __drm_kunit_helper_alloc_drm_device(struct kunit *test,
sizeof(_type),
\
offsetof(_type, _member),
\
_feat))
+struct drm_modeset_acquire_ctx *
+drm_kunit_helper_acquire_ctx_alloc(struct kunit *test);
#endif // DRM_KUNIT_HELPERS_H_
Re: [PATCH 00/11] drm: kunit: Switch to kunit actions
Hi, On 2023/7/10 15:47, Maxime Ripard wrote: Hi, Since v6.5-rc1, kunit gained a devm/drmm-like mechanism that makes tests resources much easier to cleanup. This series converts the existing tests to use those new actions were relevant. Is the word 'were' here means that 'where' relevant ? Or it is means that it were relevant, after applied you patch it is not relevant anymore ? Let me know what you think, Maxime Signed-off-by: Maxime Ripard --- Maxime Ripard (11): drm/tests: helpers: Switch to kunit actions drm/tests: client-modeset: Remove call to drm_kunit_helper_free_device() drm/tests: modes: Remove call to drm_kunit_helper_free_device() drm/tests: probe-helper: Remove call to drm_kunit_helper_free_device() drm/tests: helpers: Create an helper to allocate a locking ctx drm/tests: helpers: Create an helper to allocate an atomic state a helper or an helper ? Should this two patch be re-titled as following ? I search it on the internet[1], mostly using a helper. drm/tests: helpers: Create a helper to allocate a locking ctx drm/tests: helpers: Create a helper to allocate an atomic state [1] https://www.a-or-an.com/a_an/helper Sorry about the noise if I'm wrong. drm/vc4: tests: pv-muxing: Remove call to drm_kunit_helper_free_device() drm/vc4: tests: mock: Use a kunit action to unregister DRM device drm/vc4: tests: pv-muxing: Switch to managed locking init drm/vc4: tests: Switch to atomic state allocation helper drm/vc4: tests: pv-muxing: Document test scenario drivers/gpu/drm/tests/drm_client_modeset_test.c | 8 -- drivers/gpu/drm/tests/drm_kunit_helpers.c | 112 ++- drivers/gpu/drm/tests/drm_modes_test.c | 8 -- drivers/gpu/drm/tests/drm_probe_helper_test.c | 8 -- drivers/gpu/drm/vc4/tests/vc4_mock.c| 5 ++ drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 115 +--- include/drm/drm_kunit_helpers.h | 7 ++ 7 files changed, 162 insertions(+), 101 deletions(-) --- base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 change-id: 20230710-kms-kunit-actions-rework-5d163762c93b Best regards,
Re: [PATCH v3 4/9] PCI/VGA: Improve the default VGA device selection
Hi,
Fixes: f6b1772b2555 ('vgaarb: remove the unused irq_set_state argument
to vga_client_register')
Because after applied that patch, there have only one callback mechanism
we can use, not two anymore.
On 2023/7/12 00:43, Sui Jingfeng wrote:
From: Sui Jingfeng
Currently, the strategy of selecting the default boot on a multiple video
card coexistence system is not perfect. Potential problems are:
1) This function is a no-op on non-x86 architectures.
2) It does not take the PCI Bar may get relocated into consideration.
3) It is not effective for the PCI device without a dedicated VRAM Bar.
4) It is device-agnostic, thus it has to waste the effort to iterate all
of the PCI Bar to find the VRAM aperture.
5) It has invented lots of methods to determine which one is the default
boot device, but this is still a policy because it doesn't give the
user a choice to override.
With the observation that device drivers may have better knowledge about
which PCI bar contains the firmware FB. This patch tries to solve the above
problems by introducing a function callback to the vga_client_register()
function interface. DRM device drivers for the PCI device could provide
a xx_vga_is_primary_gpu() function callback during the driver loading time.
Once the driver binds the device successfully, VRAARB will call back to
the driver. This gives the device drivers a chance to provide accurate
boot device identification. Which in turn unlock the abitration service
to non-x86 architectures. A device driver can also just pass a NULL pointer
to keep the original behavior.
This patch is intended to introducing the mechanism only, the specific
implementation is left to the authors of various device driver. Also honor
the comment: "Clients have *TWO* callback mechanisms they can use"
Cc: Alex Deucher
Cc: Christian Konig
Cc: Pan Xinhui
Cc: David Airlie
Cc: Daniel Vetter
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Rodrigo Vivi
Cc: Tvrtko Ursulin
Cc: Ben Skeggs
Cc: Karol Herbst
Cc: Lyude Paul
Cc: Bjorn Helgaas
Cc: Alex Williamson
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: Hawking Zhang
Cc: Mario Limonciello
Cc: Lijo Lazar
Cc: YiPeng Chai
Cc: Bokun Zhang
Cc: Likun Gao
Cc: Ville Syrjala
Cc: Jason Gunthorpe
CC: Kevin Tian
Cc: Cornelia Huck
Cc: Yishai Hadas
Cc: Abhishek Sahu
Cc: Yi Liu
Acked-by: Jani Nikula # i915
Reviewed-by: Lyude Paul # nouveau
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
drivers/gpu/drm/i915/display/intel_vga.c | 3 +-
drivers/gpu/drm/loongson/lsdc_drv.c| 2 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 2 +-
drivers/gpu/drm/radeon/radeon_device.c | 2 +-
drivers/pci/vgaarb.c | 55 --
drivers/vfio/pci/vfio_pci_core.c | 2 +-
include/linux/vgaarb.h | 8 ++--
8 files changed, 61 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a92c6189b4b6..d98f0801ac77 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4103,7 +4103,7 @@ int amdgpu_device_init(struct amdgpu_device *adev,
/* this will fail for cards that aren't VGA class devices, just
* ignore it */
if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
- vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
+ vga_client_register(adev->pdev, amdgpu_device_vga_set_decode,
NULL);
px = amdgpu_device_supports_px(ddev);
diff --git a/drivers/gpu/drm/i915/display/intel_vga.c b/drivers/gpu/drm/i915/display/intel_vga.c
index 286a0bdd28c6..98d7d4dffe9f 100644
--- a/drivers/gpu/drm/i915/display/intel_vga.c
+++ b/drivers/gpu/drm/i915/display/intel_vga.c
@@ -115,7 +115,6 @@ intel_vga_set_decode(struct pci_dev *pdev, bool
enable_decode)
int intel_vga_register(struct drm_i915_private *i915)
{
-
struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
int ret;
@@ -127,7 +126,7 @@ int intel_vga_register(struct drm_i915_private *i915)
* then we do not take part in VGA arbitration and the
* vga_client_register() fails with -ENODEV.
*/
- ret = vga_client_register(pdev, intel_vga_set_decode);
+ ret = vga_client_register(pdev, intel_vga_set_decode, NULL);
if (ret && ret != -ENODEV)
return ret;
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c b/drivers/gpu/drm/loongson/lsdc_drv.c
index 188ec82afcfb..d10a28c2c494 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -289,7 +289,7 @@ static int lsdc_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
pci_set_drvdata(pdev, ddev);
- vga_client_register(pdev, lsdc_vga_set_decode);
+ vga_client_register(pdev, lsdc_vga_set_decode, NULL);
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
On 2023/7/17 21:17, Sui Jingfeng wrote: So without we can craft something new easily without significant change. Therefore, We can *NOT* craft something new easily without significant change. Especially userspace changes. So my current patch choose to keep the original behavior. At the same time, it optimize and cleanup the vgaarb.c a lot. Thanks.
Re: [PATCH v1 8/8] drm/etnaviv: Add a helper to get a pointer to the first available node
Hi,
On 2023/7/17 18:07, Lucas Stach wrote:
Am Freitag, dem 23.06.2023 um 18:08 +0800 schrieb Sui Jingfeng:
From: Sui Jingfeng
This make the code in etnaviv_pdev_probe() less twisted, drop the reference
to device node after finished. Also kill a double blank line.
I can't spot the double blank line you claim to remove.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 32 ++-
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 7d0eeab3e8b7..3446f8eabf59 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -27,6 +27,19 @@
* DRM operations:
*/
+/* If the DT contains at least one available GPU, return a pointer to it */
+
Here is the double blank line my patch remove, it (a blank line) is occupied by
the comment of "/* If the DT contains at least one available GPU, return a pointer
to it */"
I think the code in the function is simple enough that we don't need a
comment explaining what it does.
Ok, then I'll remove the comment at the next version. Thanks
Regards,
Lucas
+static struct device_node *etnaviv_of_first_node(void)
+{
+ struct device_node *np;
+
+ for_each_compatible_node(np, NULL, "vivante,gc") {
+ if (of_device_is_available(np))
+ return np;
+ }
+
+ return NULL;
+}
static void load_gpu(struct drm_device *dev)
{
@@ -587,7 +600,7 @@ static const struct component_master_ops etnaviv_master_ops
= {
static int etnaviv_pdev_probe(struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *first_node = NULL;
+ struct device_node *first_node;
struct component_match *match = NULL;
if (!dev->platform_data) {
@@ -597,11 +610,10 @@ static int etnaviv_pdev_probe(struct platform_device
*pdev)
if (!of_device_is_available(core_node))
continue;
- if (!first_node)
- first_node = core_node;
-
drm_of_component_match_add(>dev, ,
component_compare_of,
core_node);
+
+ of_node_put(core_node);
}
} else {
char **names = dev->platform_data;
@@ -634,8 +646,11 @@ static int etnaviv_pdev_probe(struct platform_device *pdev)
* device as the GPU we found. This assumes that all Vivante
* GPUs in the system share the same DMA constraints.
*/
- if (first_node)
+ first_node = etnaviv_of_first_node();
+ if (first_node) {
of_dma_configure(>dev, first_node, true);
+ of_node_put(first_node);
+ }
return component_master_add_with_match(dev, _master_ops, match);
}
@@ -709,17 +724,14 @@ static int __init etnaviv_init(void)
* If the DT contains at least one available GPU device, instantiate
* the DRM platform device.
*/
- for_each_compatible_node(np, NULL, "vivante,gc") {
- if (!of_device_is_available(np))
- continue;
+ np = etnaviv_of_first_node();
+ if (np) {
of_node_put(np);
ret = etnaviv_create_platform_device("etnaviv",
_platform_device);
if (ret)
goto unregister_platform_driver;
-
- break;
}
return 0;
Re: [PATCH 2/6] PCI/VGA: Deal with PCI VGA compatible devices only
Hi,
On 2023/7/11 21:43, Sui Jingfeng wrote:
From: Sui Jingfeng
Currently, vgaarb only cares about PCI VGA-compatible class devices.
While vga_arbiter_del_pci_device() gets called unbalanced when some PCI
device is about to be removed. This happens even during the boot process.
Another reason is that the vga_arb_device_init() function is not efficient.
Since we only care about VGA-compatible devices (pdev->class == 0x03),
We could filter the unqualified devices out in the vga_arb_device_init()
function. While the current implementation is to search all PCI devices
in a system, this is not necessary.
Reviewed-by: Mario Limonciello
Signed-off-by: Sui Jingfeng
---
drivers/pci/vgaarb.c | 25 +
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
index c1bc6c983932..021116ed61cb 100644
--- a/drivers/pci/vgaarb.c
+++ b/drivers/pci/vgaarb.c
@@ -754,10 +754,6 @@ static bool vga_arbiter_add_pci_device(struct pci_dev
*pdev)
struct pci_dev *bridge;
u16 cmd;
- /* Only deal with VGA class devices */
- if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
- return false;
-
/* Allocate structure */
vgadev = kzalloc(sizeof(struct vga_device), GFP_KERNEL);
if (vgadev == NULL) {
@@ -1502,6 +1498,10 @@ static int pci_notify(struct notifier_block *nb,
unsigned long action,
vgaarb_dbg(dev, "%s\n", __func__);
+ /* Deal with VGA compatible devices only */
+ if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+ return 0;
+
/* For now we're only intereted in devices added and removed. I didn't
* test this thing here, so someone needs to double check for the
* cases of hotplugable vga cards. */
@@ -1534,8 +1534,8 @@ static struct miscdevice vga_arb_device = {
static int __init vga_arb_device_init(void)
{
+ struct pci_dev *pdev = NULL;
int rc;
- struct pci_dev *pdev;
rc = misc_register(_arb_device);
if (rc < 0)
@@ -1543,13 +1543,14 @@ static int __init vga_arb_device_init(void)
bus_register_notifier(_bus_type, _notifier);
- /* We add all PCI devices satisfying VGA class in the arbiter by
-* default */
- pdev = NULL;
- while ((pdev =
- pci_get_subsys(PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
- PCI_ANY_ID, pdev)) != NULL)
- vga_arbiter_add_pci_device(pdev);
+ /*
+* We add all PCI VGA compatible devices in the arbiter by default
+*/
+ do {
+ pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev);
+ if (pdev)
+ vga_arbiter_add_pci_device(pdev);
+ } while (pdev);
I suddenly remember one more thing that I forget to explain.
In a previous thread[1] of previous version of this series,
Maciej seems argue that PCI_CLASS_NOT_DEFINED_VGA should be handled also.
But, even I try to handled PCI_CLASS_NOT_DEFINED_VGA at here,
this card still will not be annotate as boot_vga,
because the pci_dev_attrs_are_visible() function only consider VGA compatible
devices
which (pdev->class >> 8 == PCI_CLASS_DISPLAY_VGA) is true. See [2].
Intel integrated GPU is more intelligent,
which could change their class of the PCI(e) device to 0x038000 when
multiple GPU co-exist. Even though the GPU can display. This is configurable by
the firmware, but this is trying to escape the arbitration by changing is PCI
id.
So, PCI devices belong to the PCI_CLASS_DISPLAY_OTHER, PCI_CLASS_DISPLAY_3D and
PCI_CLASS_DISPLAY_XGA
can not participate in the arbitration. They are all will be get filtered.
So, this is a limitation of the vgaarb itself.
While I could help to improve it in the future, what do you think?
Is my express clear?
[1]
https://lkml.kernel.org/nouveau/alpine.deb.2.21.2306190339590.14...@angie.orcam.me.uk/#t
[2]
https://elixir.bootlin.com/linux/latest/source/drivers/pci/pci-sysfs.c#L1544
pr_info("loaded\n");
return rc;
Re: [PATCH v1 0/8] drm/etnaviv: Various cleanup
Hi, Dear etnaviv folks Would you like to review this cleanup patch set ? I am asking because I'm wondering that if I should re-spin my other patch from the code base which *with* this series applied or from the code base *without* this series applied. I think this series looks fine, is it acceptable? On 2023/6/23 18:08, Sui Jingfeng wrote: From: Sui Jingfeng No functional change. Sui Jingfeng (8): drm/etnaviv: Using the size_t variable to store the number of pages drm/etnaviv: Using the unsigned int type to count the number of pages drm/etnaviv: Drop the second argument of the etnaviv_gem_new_impl() drm/etnaviv: Remove surplus else after return drm/etnaviv: Keep the curly brace aligned drm/etnaviv: No indentation by double tabs drm/etnaviv: Add dedicated functions to create and destroy platform device drm/etnaviv: Add a helper to get a pointer to the first available node drivers/gpu/drm/etnaviv/etnaviv_drv.c | 100 +--- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 14 +-- drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 7 +- 3 files changed, 77 insertions(+), 44 deletions(-)
Re: [PATCH] fbdev/hyperv_fb: Include
Hi,
On 2023/7/10 15:58, Thomas Zimmermann wrote:
Include to get the global screen_info state.
Fixes the following errors:
drivers/video/fbdev/hyperv_fb.c:1033:10: error: use of undeclared identifier
'screen_info'
1033 | base = screen_info.lfb_base;
|^
drivers/video/fbdev/hyperv_fb.c:1034:10: error: use of undeclared
identifier 'screen_info'
1034 | size = screen_info.lfb_size;
|^
drivers/video/fbdev/hyperv_fb.c:1080:3: error: must use 'struct' tag to refer
to type 'screen_info'
1080 | screen_info.lfb_size = 0;
| ^
| struct
drivers/video/fbdev/hyperv_fb.c:1080:14: error: expected identifier or '('
1080 | screen_info.lfb_size = 0;
|^
drivers/video/fbdev/hyperv_fb.c:1081:3: error: must use 'struct' tag to
refer to type 'screen_info'
1081 | screen_info.lfb_base = 0;
| ^
| struct
drivers/video/fbdev/hyperv_fb.c:1081:14: error: expected identifier or '('
1081 | screen_info.lfb_base = 0;
|^
drivers/video/fbdev/hyperv_fb.c:1082:3: error: must use 'struct' tag to
refer to type 'screen_info'
1082 | screen_info.orig_video_isVGA = 0;
| ^
| struct
drivers/video/fbdev/hyperv_fb.c:1082:14: error: expected identifier or '('
1082 | screen_info.orig_video_isVGA = 0;
|^
8 errors generated.
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202307101042.rqehuauj-...@intel.com/
Fixes: 8b0d13545b09 ("efi: Do not include from EFI
header")
Signed-off-by: Thomas Zimmermann
Reviewed-by: Sui Jingfeng
Cc: "K. Y. Srinivasan" (supporter:Hyper-V/Azure CORE AND
DRIVERS)
Cc: Haiyang Zhang (supporter:Hyper-V/Azure CORE AND
DRIVERS)
Cc: Wei Liu (supporter:Hyper-V/Azure CORE AND DRIVERS)
Cc: Dexuan Cui (supporter:Hyper-V/Azure CORE AND DRIVERS)
Cc: Helge Deller (maintainer:FRAMEBUFFER LAYER)
Cc: Javier Martinez Canillas
Cc: Sui Jingfeng
Cc: Ard Biesheuvel
Cc: Russell King
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Arnd Bergmann
Cc: linux-...@vger.kernel.org
Cc: linux-hyp...@vger.kernel.org (open list:Hyper-V/Azure CORE AND DRIVERS)
Cc: linux-fb...@vger.kernel.org (open list:FRAMEBUFFER LAYER)
Cc: dri-devel@lists.freedesktop.org (open list:FRAMEBUFFER LAYER)
---
drivers/video/fbdev/hyperv_fb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
index 1ae35ab62b29..b331452aab4f 100644
--- a/drivers/video/fbdev/hyperv_fb.c
+++ b/drivers/video/fbdev/hyperv_fb.c
@@ -48,6 +48,7 @@
#include
#include
#include
+#include
#include
#include
#include
Ah, I also overlook this one. :-)
Re: [PATCH] drm/loongson: Fix two warnings because of passing wrong type
Hi, On 2023/7/10 18:26, Jani Nikula wrote: On Mon, 10 Jul 2023, Sui Jingfeng wrote: When accessing I/O memory, we should pass '__iomem *' type instead of 'void *' simply, otherwise sparse tests will complain. After applied this patch, the following two sparse warnings got fixed. Usually the commit message should explain why it's okay to cast away the warning. Because realistically this doesn't "fix" the warning, this merely hides it. The reason why we don't fix this at the very beginning is that we are following the ttm_kmap_obj_virtual() and the ttm_bo_kmap() function. Our lsdc_bo_kmap() is implemented with the ttm_bo_kmap() function. Another reason is that this warning don't emerge when compile with W=1, at least this is true on our platform. We don't think this warning is harmful, because implicit cast will do the cast for us. It is just that we need eliminate the noise as a programmer. Again, for the code at here, before you do the de-reference operation, As long as a address is really(originally) point to the I/O memory, cast it to 'void __iomem *' is OK. As long as a address is really(originally) point to the system memory, cast it to 'void *' is OK. BR, Jani. 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35: sparse: expected void volatile [noderef] __iomem * sparse: got void *kptr 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51: sparse: expected void const volatile [noderef] __iomem * sparse: got void *kptr Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-...@intel.com/ Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c index b088646a2ff9..36e352820bdb 100644 --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo, lsdc_bo_kmap(dst_bo); while (n--) - memcpy_toio(dst_bo->kptr, src_bo->kptr, size); + memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size); lsdc_bo_kunmap(src_bo); lsdc_bo_kunmap(dst_bo); @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo, lsdc_bo_kmap(dst_bo); while (n--) - memcpy_fromio(dst_bo->kptr, src_bo->kptr, size); + memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size); lsdc_bo_kunmap(src_bo); lsdc_bo_kunmap(dst_bo);
Re: [PATCH] drm/loongson: Fix two warnings because of passing wrong type
Hi, On 2023/7/10 18:26, Jani Nikula wrote: On Mon, 10 Jul 2023, Sui Jingfeng wrote: When accessing I/O memory, we should pass '__iomem *' type instead of 'void *' simply, otherwise sparse tests will complain. After applied this patch, the following two sparse warnings got fixed. Usually the commit message should explain why it's okay to cast away the warning. Because realistically this doesn't "fix" the warning, this merely hides it. My understanding is that a point itself is just a variable where store a address, if this address originally point to I/O memory, then, we can other cast it to u64 type, then cast it back to '__iomem *' again. as long as the type's bit-width is width enough to store this address, we won't lost the information. 'void *' or 'u64' is just a intermediate represent of the address. we can other cast it to u64 type, then cast it back to 'void __iomem *' or 'void *' again. Why it's okay ? My answer is that As long as a address is really point to the I/O memory, cast it to 'void __iomem *' is OK. As long as a address is really point to the system memory, cast it to 'void *' is OK. BR, Jani. 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35: sparse: expected void volatile [noderef] __iomem * sparse: got void *kptr 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51: sparse: expected void const volatile [noderef] __iomem * sparse: got void *kptr Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-...@intel.com/ Signed-off-by: Sui Jingfeng --- drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c index b088646a2ff9..36e352820bdb 100644 --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo, lsdc_bo_kmap(dst_bo); while (n--) - memcpy_toio(dst_bo->kptr, src_bo->kptr, size); + memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size); lsdc_bo_kunmap(src_bo); lsdc_bo_kunmap(dst_bo); @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo, lsdc_bo_kmap(dst_bo); while (n--) - memcpy_fromio(dst_bo->kptr, src_bo->kptr, size); + memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size); lsdc_bo_kunmap(src_bo); lsdc_bo_kunmap(dst_bo);
Re: [PATCH] drm/loongson: Remove a useless check in cursor_plane_atomic_async_check()
Hi,
On 2023/7/10 18:39, Thomas Zimmermann wrote:
Am 10.07.23 um 12:24 schrieb Sui Jingfeng:
Because smatch warnings:
drivers/gpu/drm/loongson/lsdc_plane.c:199
lsdc_cursor_plane_atomic_async_check()
warn: variable dereferenced before check 'state' (see line 180)
vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
174 static int
lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
175 struct drm_atomic_state
*state)
176 {
177 struct drm_plane_state *new_state;
178 struct drm_crtc_state *crtc_state;
179
180 new_state = drm_atomic_get_new_plane_state(state, plane);
^
state is dereferenced inside this function
181
182 if (!plane->state || !plane->state->fb) {
183 drm_dbg(plane->dev, "%s: state is NULL\n", plane->name);
184 return -EINVAL;
185 }
186
187 if (new_state->crtc_w != new_state->crtc_h) {
188 drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
189 new_state->crtc_w, new_state->crtc_h);
190 return -EINVAL;
191 }
192
193 if (new_state->crtc_w != 64 && new_state->crtc_w != 32) {
194 drm_dbg(plane->dev, "unsupported cursor size: %ux%u\n",
195 new_state->crtc_w, new_state->crtc_h);
196 return -EINVAL;
197 }
198
199 if (state) {
^
Checked too late!
Reported-by: Dan Carpenter
Closes: https://lore.kernel.org/r/202307100423.rv7d05uq-...@intel.com/
Signed-off-by: Sui Jingfeng
Acked-by: Thomas Zimmermann
BTW, you're posting these patches for loongson,
I'm posting these patches for the drm/loongson driver in drm-misc and/or
drm-tip branch,
what do you means for *loongson*,
but that driver is not yet in our tree?
I already applied(push) drm/loongson driver to drm-misc-next branch,
What do you means that by "not yet in our tree", linux kernel side?
Am I missing something ?
Best regards
Thomas
---
drivers/gpu/drm/loongson/lsdc_plane.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/loongson/lsdc_plane.c
b/drivers/gpu/drm/loongson/lsdc_plane.c
index 2ab3db982aa3..0d5094633222 100644
--- a/drivers/gpu/drm/loongson/lsdc_plane.c
+++ b/drivers/gpu/drm/loongson/lsdc_plane.c
@@ -196,13 +196,7 @@ static int
lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
return -EINVAL;
}
- if (state) {
- crtc_state = drm_atomic_get_existing_crtc_state(state,
new_state->crtc);
- } else {
- crtc_state = plane->crtc->state;
- drm_dbg(plane->dev, "%s: atomic state is NULL\n", plane->name);
- }
-
+ crtc_state = drm_atomic_get_existing_crtc_state(state,
new_state->crtc);
if (!crtc_state->active)
return -EINVAL;
Re: [drm-misc:drm-misc-next 2/3] drivers/gpu/drm/loongson/lsdc_plane.c:199 lsdc_cursor_plane_atomic_async_check() warn: variable dereferenced before check 'state' (see line 180)
Hi,
On 2023/7/10 15:19, Thomas Zimmermann wrote:
Hi
Am 10.07.23 um 09:02 schrieb suijingfeng:
Hi,
Thanks for testing,
What do you means about tell me this?
I means that would you like to help fixing this warning?
The code in drm_atomic_get_new_plane_state() dereferences the state
parameter. Later in your function, you test for state to be non-NULL.
That's what the warning is about.
You should be able to silence this warning by removing the state test
from your function (and also delete that else branch). There should
always be an atomic state present and the atomic-check callbacks
should not be called without a state. If not, the driver most likely
failed to initialize correctly.
Yes, I will create a patch to fix this warning before tonight.
The driver works very well, it never print with kernel 'cmd drm.debug=0x02'.
I just feel interesting, thanks.
Best regards
Thomas
Or otherwise, I will fix this someday.
On 2023/7/10 14:29, Dan Carpenter wrote:
tree: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
head: 8d1077cf2e43b15fefd76ebec2b71541eb27ef2c
commit: f39db26c54281da6a785259498ca74b5e470476f [2/3] drm: Add kms
driver for loongson display controller
config: i386-randconfig-m021-20230710
(https://download.01.org/0day-ci/archive/20230710/202307100423.rv7d05uq-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230710/202307100423.rv7d05uq-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new
version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes:
https://lore.kernel.org/r/202307100423.rv7d05uq-...@intel.com/
smatch warnings:
drivers/gpu/drm/loongson/lsdc_plane.c:199
lsdc_cursor_plane_atomic_async_check() warn: variable dereferenced
before check 'state' (see line 180)
vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
f39db26c54281d Sui Jingfeng 2023-06-15 174 static int
lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
f39db26c54281d Sui Jingfeng 2023-06-15 175
struct drm_atomic_state *state)
f39db26c54281d Sui Jingfeng 2023-06-15 176 {
f39db26c54281d Sui Jingfeng 2023-06-15 177 struct
drm_plane_state *new_state;
f39db26c54281d Sui Jingfeng 2023-06-15 178 struct
drm_crtc_state *crtc_state;
f39db26c54281d Sui Jingfeng 2023-06-15 179
f39db26c54281d Sui Jingfeng 2023-06-15 @180 new_state =
drm_atomic_get_new_plane_state(state, plane);
^
state is dereferenced inside this function
f39db26c54281d Sui Jingfeng 2023-06-15 181
f39db26c54281d Sui Jingfeng 2023-06-15 182 if (!plane->state
|| !plane->state->fb) {
f39db26c54281d Sui Jingfeng 2023-06-15 183 drm_dbg(plane->dev, "%s:
state is NULL\n", plane->name);
f39db26c54281d Sui Jingfeng 2023-06-15 184 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 185 }
f39db26c54281d Sui Jingfeng 2023-06-15 186
f39db26c54281d Sui Jingfeng 2023-06-15 187 if
(new_state->crtc_w != new_state->crtc_h) {
f39db26c54281d Sui Jingfeng 2023-06-15 188 drm_dbg(plane->dev,
"unsupported cursor size: %ux%u\n",
f39db26c54281d Sui Jingfeng 2023-06-15 189 new_state->crtc_w,
new_state->crtc_h);
f39db26c54281d Sui Jingfeng 2023-06-15 190 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 191 }
f39db26c54281d Sui Jingfeng 2023-06-15 192
f39db26c54281d Sui Jingfeng 2023-06-15 193 if
(new_state->crtc_w != 64 && new_state->crtc_w != 32) {
f39db26c54281d Sui Jingfeng 2023-06-15 194 drm_dbg(plane->dev,
"unsupported cursor size: %ux%u\n",
f39db26c54281d Sui Jingfeng 2023-06-15 195 new_state->crtc_w,
new_state->crtc_h);
f39db26c54281d Sui Jingfeng 2023-06-15 196 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 197 }
f39db26c54281d Sui Jingfeng 2023-06-15 198
f39db26c54281d Sui Jingfeng 2023-06-15 @199 if (state) {
^
Checked too late
f39db26c54281d Sui Jingfeng 2023-06-15 200 crtc_state =
drm_atomic_get_existing_crtc_state(state, new_state->crtc);
f39db26c54281d Sui Jingfeng 2023-06-15 201 } else {
f39db26c54281d Sui Jingfeng 2023-06-15 202 crtc_state =
plane->crtc->state;
f39db26c54281d Sui Jingfeng 2023-06-15 203 drm_dbg(plane->dev, "%s:
atomic state is NULL\n", plane->name);
f39db26c54281d Sui Jingfeng 2023-06-15 204 }
f39db26c54281d Sui Jingfeng 2023-06-15 205
f39db26c54281d Sui Jingfeng 2023-06-15 206 if
(!crtc_state->active)
f39db26c54281d Sui Jingfeng 2023-06-15 207 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 208
f39db26c54281d Sui Jingfeng 2023-06-15 209 if
(plane->state->crtc != new_state->crtc ||
f39db26c54281d Sui Jingfeng 2023-06-15 210 p
Re: [drm-misc:drm-misc-next 2/3] drivers/gpu/drm/loongson/lsdc_plane.c:199 lsdc_cursor_plane_atomic_async_check() warn: variable dereferenced before check 'state' (see line 180)
Hi,
On 2023/7/10 14:29, Dan Carpenter wrote:
tree: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
head: 8d1077cf2e43b15fefd76ebec2b71541eb27ef2c
commit: f39db26c54281da6a785259498ca74b5e470476f [2/3] drm: Add kms driver for
loongson display controller
config: i386-randconfig-m021-20230710
(https://download.01.org/0day-ci/archive/20230710/202307100423.rv7d05uq-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230710/202307100423.rv7d05uq-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202307100423.rv7d05uq-...@intel.com/
smatch warnings:
drivers/gpu/drm/loongson/lsdc_plane.c:199
lsdc_cursor_plane_atomic_async_check() warn: variable dereferenced before check
'state' (see line 180)
vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
f39db26c54281d Sui Jingfeng 2023-06-15 174 static int
lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
f39db26c54281d Sui Jingfeng 2023-06-15 175
struct drm_atomic_state *state)
f39db26c54281d Sui Jingfeng 2023-06-15 176 {
f39db26c54281d Sui Jingfeng 2023-06-15 177 struct drm_plane_state
*new_state;
f39db26c54281d Sui Jingfeng 2023-06-15 178 struct drm_crtc_state
*crtc_state;
f39db26c54281d Sui Jingfeng 2023-06-15 179
f39db26c54281d Sui Jingfeng 2023-06-15 @180 new_state =
drm_atomic_get_new_plane_state(state, plane);
^
state is dereferenced inside this function
f39db26c54281d Sui Jingfeng 2023-06-15 181
f39db26c54281d Sui Jingfeng 2023-06-15 182 if (!plane->state ||
!plane->state->fb) {
f39db26c54281d Sui Jingfeng 2023-06-15 183 drm_dbg(plane->dev, "%s: state
is NULL\n", plane->name);
f39db26c54281d Sui Jingfeng 2023-06-15 184 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 185 }
f39db26c54281d Sui Jingfeng 2023-06-15 186
f39db26c54281d Sui Jingfeng 2023-06-15 187 if (new_state->crtc_w !=
new_state->crtc_h) {
f39db26c54281d Sui Jingfeng 2023-06-15 188 drm_dbg(plane->dev,
"unsupported cursor size: %ux%u\n",
f39db26c54281d Sui Jingfeng 2023-06-15 189 new_state->crtc_w,
new_state->crtc_h);
f39db26c54281d Sui Jingfeng 2023-06-15 190 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 191 }
f39db26c54281d Sui Jingfeng 2023-06-15 192
f39db26c54281d Sui Jingfeng 2023-06-15 193 if (new_state->crtc_w != 64 &&
new_state->crtc_w != 32) {
f39db26c54281d Sui Jingfeng 2023-06-15 194 drm_dbg(plane->dev,
"unsupported cursor size: %ux%u\n",
f39db26c54281d Sui Jingfeng 2023-06-15 195 new_state->crtc_w,
new_state->crtc_h);
f39db26c54281d Sui Jingfeng 2023-06-15 196 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 197 }
f39db26c54281d Sui Jingfeng 2023-06-15 198
f39db26c54281d Sui Jingfeng 2023-06-15 @199 if (state) {
^
Checked too late
Yes, there no need to check here, simply writing the code as following
is OK.
```
crtc_state = drm_atomic_get_existing_crtc_state(state,
new_state->crtc);
```
f39db26c54281d Sui Jingfeng 2023-06-15 200 crtc_state =
drm_atomic_get_existing_crtc_state(state, new_state->crtc);
f39db26c54281d Sui Jingfeng 2023-06-15 201 } else {
f39db26c54281d Sui Jingfeng 2023-06-15 202 crtc_state =
plane->crtc->state;
f39db26c54281d Sui Jingfeng 2023-06-15 203 drm_dbg(plane->dev, "%s: atomic
state is NULL\n", plane->name);
f39db26c54281d Sui Jingfeng 2023-06-15 204 }
f39db26c54281d Sui Jingfeng 2023-06-15 205
f39db26c54281d Sui Jingfeng 2023-06-15 206 if (!crtc_state->active)
f39db26c54281d Sui Jingfeng 2023-06-15 207 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 208
f39db26c54281d Sui Jingfeng 2023-06-15 209 if (plane->state->crtc !=
new_state->crtc ||
f39db26c54281d Sui Jingfeng 2023-06-15 210 plane->state->src_w !=
new_state->src_w ||
f39db26c54281d Sui Jingfeng 2023-06-15 211 plane->state->src_h !=
new_state->src_h ||
f39db26c54281d Sui Jingfeng 2023-06-15 212 plane->state->crtc_w !=
new_state->crtc_w ||
f39db26c54281d Sui Jingfeng 2023-06-15 213 plane->state->crtc_h !=
new_state->crtc_h)
f39db26c54281d Sui Jingfeng 2023-06-15 214 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 215
f39db26c54281d Sui Jingfeng 2023-06-15 216 if (new_state->visible !=
plane->state->visible)
f39db26c54281d Sui Jingfeng 2023-06-15 217 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 218
f39db26c54281d
Re: [drm-misc:drm-misc-next 2/3] drivers/gpu/drm/loongson/lsdc_plane.c:199 lsdc_cursor_plane_atomic_async_check() warn: variable dereferenced before check 'state' (see line 180)
Hi,
Thanks for testing,
What do you means about tell me this?
I means that would you like to help fixing this warning?
Or otherwise, I will fix this someday.
On 2023/7/10 14:29, Dan Carpenter wrote:
tree: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
head: 8d1077cf2e43b15fefd76ebec2b71541eb27ef2c
commit: f39db26c54281da6a785259498ca74b5e470476f [2/3] drm: Add kms driver for
loongson display controller
config: i386-randconfig-m021-20230710
(https://download.01.org/0day-ci/archive/20230710/202307100423.rv7d05uq-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230710/202307100423.rv7d05uq-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202307100423.rv7d05uq-...@intel.com/
smatch warnings:
drivers/gpu/drm/loongson/lsdc_plane.c:199
lsdc_cursor_plane_atomic_async_check() warn: variable dereferenced before check
'state' (see line 180)
vim +/state +199 drivers/gpu/drm/loongson/lsdc_plane.c
f39db26c54281d Sui Jingfeng 2023-06-15 174 static int
lsdc_cursor_plane_atomic_async_check(struct drm_plane *plane,
f39db26c54281d Sui Jingfeng 2023-06-15 175
struct drm_atomic_state *state)
f39db26c54281d Sui Jingfeng 2023-06-15 176 {
f39db26c54281d Sui Jingfeng 2023-06-15 177 struct drm_plane_state
*new_state;
f39db26c54281d Sui Jingfeng 2023-06-15 178 struct drm_crtc_state
*crtc_state;
f39db26c54281d Sui Jingfeng 2023-06-15 179
f39db26c54281d Sui Jingfeng 2023-06-15 @180 new_state =
drm_atomic_get_new_plane_state(state, plane);
^
state is dereferenced inside this function
f39db26c54281d Sui Jingfeng 2023-06-15 181
f39db26c54281d Sui Jingfeng 2023-06-15 182 if (!plane->state ||
!plane->state->fb) {
f39db26c54281d Sui Jingfeng 2023-06-15 183 drm_dbg(plane->dev, "%s: state
is NULL\n", plane->name);
f39db26c54281d Sui Jingfeng 2023-06-15 184 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 185 }
f39db26c54281d Sui Jingfeng 2023-06-15 186
f39db26c54281d Sui Jingfeng 2023-06-15 187 if (new_state->crtc_w !=
new_state->crtc_h) {
f39db26c54281d Sui Jingfeng 2023-06-15 188 drm_dbg(plane->dev,
"unsupported cursor size: %ux%u\n",
f39db26c54281d Sui Jingfeng 2023-06-15 189 new_state->crtc_w,
new_state->crtc_h);
f39db26c54281d Sui Jingfeng 2023-06-15 190 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 191 }
f39db26c54281d Sui Jingfeng 2023-06-15 192
f39db26c54281d Sui Jingfeng 2023-06-15 193 if (new_state->crtc_w != 64 &&
new_state->crtc_w != 32) {
f39db26c54281d Sui Jingfeng 2023-06-15 194 drm_dbg(plane->dev,
"unsupported cursor size: %ux%u\n",
f39db26c54281d Sui Jingfeng 2023-06-15 195 new_state->crtc_w,
new_state->crtc_h);
f39db26c54281d Sui Jingfeng 2023-06-15 196 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 197 }
f39db26c54281d Sui Jingfeng 2023-06-15 198
f39db26c54281d Sui Jingfeng 2023-06-15 @199 if (state) {
^
Checked too late
f39db26c54281d Sui Jingfeng 2023-06-15 200 crtc_state =
drm_atomic_get_existing_crtc_state(state, new_state->crtc);
f39db26c54281d Sui Jingfeng 2023-06-15 201 } else {
f39db26c54281d Sui Jingfeng 2023-06-15 202 crtc_state =
plane->crtc->state;
f39db26c54281d Sui Jingfeng 2023-06-15 203 drm_dbg(plane->dev, "%s: atomic
state is NULL\n", plane->name);
f39db26c54281d Sui Jingfeng 2023-06-15 204 }
f39db26c54281d Sui Jingfeng 2023-06-15 205
f39db26c54281d Sui Jingfeng 2023-06-15 206 if (!crtc_state->active)
f39db26c54281d Sui Jingfeng 2023-06-15 207 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 208
f39db26c54281d Sui Jingfeng 2023-06-15 209 if (plane->state->crtc !=
new_state->crtc ||
f39db26c54281d Sui Jingfeng 2023-06-15 210 plane->state->src_w !=
new_state->src_w ||
f39db26c54281d Sui Jingfeng 2023-06-15 211 plane->state->src_h !=
new_state->src_h ||
f39db26c54281d Sui Jingfeng 2023-06-15 212 plane->state->crtc_w !=
new_state->crtc_w ||
f39db26c54281d Sui Jingfeng 2023-06-15 213 plane->state->crtc_h !=
new_state->crtc_h)
f39db26c54281d Sui Jingfeng 2023-06-15 214 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 215
f39db26c54281d Sui Jingfeng 2023-06-15 216 if (new_state->visible !=
plane->state->visible)
f39db26c54281d Sui Jingfeng 2023-06-15 217 return -EINVAL;
f39db26c54281d Sui Jingfeng 2023-06-15 218
f39db26c54281d Sui Jingfeng
Re: [PATCH] drm/hyperv: Fix a compilation issue because of not including screen_info.h
OK, thanks a lot,
done!
On 2023/7/10 13:20, Michael Kelley (LINUX) wrote:
From: Sui Jingfeng Sent: Sunday, July 9, 2023 3:05 AM
drivers/video/fbdev/hyperv_fb.c: In function 'hvfb_getmem':
drivers/video/fbdev/hyperv_fb.c:1033:24: error: 'screen_info' undeclared (first
use
in this function)
1033 | base = screen_info.lfb_base;
|^~~
drivers/video/fbdev/hyperv_fb.c:1033:24: note: each undeclared identifier
is reported
only once for each function it appears in
--
drivers/gpu/drm/hyperv/hyperv_drm_drv.c: In function 'hyperv_setup_vram':
drivers/gpu/drm/hyperv/hyperv_drm_drv.c:75:54: error: 'screen_info' undeclared
(first use in this function)
75 |
drm_aperture_remove_conflicting_framebuffers(screen_info.lfb_base,
| ^~~
drivers/gpu/drm/hyperv/hyperv_drm_drv.c:75:54: note: each undeclared
identifier is
reported only once for each function it appears in
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202307090823.nxnt8kk5-...@intel.com/
Fixes: 81d2393485f0 ("fbdev/hyperv-fb: Do not set struct fb_info.apertures")
Fixes: 8b0d13545b09 ("efi: Do not include from EFI
header")
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index a7d2c92d6c6a..8026118c6e03 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -7,6 +7,7 @@
#include
#include
#include
+#include
#include
#include
--
2.25.1
Reviewed-by: Michael Kelley
Re: [drm-misc:drm-misc-next 1/15] drivers/video/fbdev/hyperv_fb.c:1033:10: error: use of undeclared identifier 'screen_info'
Hi, Deepak and Michael
Would you like to review this trivial patch ?
Please help to push this to drm/hyperv, because kernel test robots are
crying.
On 2023/7/10 10:57, kernel test robot wrote:
tree: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
head: 70d1ace56db6c79d39dbe9c0d5244452b67e2fde
commit: 8b0d13545b091729e0aa05ff9981e2d06c1e2ee5 [1/15] efi: Do not include
from EFI header
config: arm64-allmodconfig
(https://download.01.org/0day-ci/archive/20230710/202307101042.rqehuauj-...@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git
4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce:
(https://download.01.org/0day-ci/archive/20230710/202307101042.rqehuauj-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202307101042.rqehuauj-...@intel.com/
All errors (new ones prefixed by >>):
drivers/video/fbdev/hyperv_fb.c:1033:10: error: use of undeclared identifier
'screen_info'
1033 | base = screen_info.lfb_base;
|^
drivers/video/fbdev/hyperv_fb.c:1034:10: error: use of undeclared
identifier 'screen_info'
1034 | size = screen_info.lfb_size;
|^
drivers/video/fbdev/hyperv_fb.c:1080:3: error: must use 'struct' tag to refer
to type 'screen_info'
1080 | screen_info.lfb_size = 0;
| ^
| struct
drivers/video/fbdev/hyperv_fb.c:1080:14: error: expected identifier or '('
1080 | screen_info.lfb_size = 0;
|^
drivers/video/fbdev/hyperv_fb.c:1081:3: error: must use 'struct' tag to
refer to type 'screen_info'
1081 | screen_info.lfb_base = 0;
| ^
| struct
drivers/video/fbdev/hyperv_fb.c:1081:14: error: expected identifier or '('
1081 | screen_info.lfb_base = 0;
|^
drivers/video/fbdev/hyperv_fb.c:1082:3: error: must use 'struct' tag to
refer to type 'screen_info'
1082 | screen_info.orig_video_isVGA = 0;
| ^
| struct
drivers/video/fbdev/hyperv_fb.c:1082:14: error: expected identifier or '('
1082 | screen_info.orig_video_isVGA = 0;
|^
8 errors generated.
vim +/screen_info +1033 drivers/video/fbdev/hyperv_fb.c
3a6fb6c4255c38 drivers/video/fbdev/hyperv_fb.c Wei Hu2019-12-09
988
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
989
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
990 /* Get framebuffer memory from Hyper-V video pci space */
3546448338e76a drivers/video/fbdev/hyperv_fb.c Jake Oshins 2015-08-05
991 static int hvfb_getmem(struct hv_device *hdev, struct fb_info *info)
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
992 {
9069fd54960304 drivers/video/hyperv_fb.c Gerd Hoffmann 2014-02-26 993
struct hvfb_par *par = info->par;
9069fd54960304 drivers/video/hyperv_fb.c Gerd Hoffmann 2014-02-26
994 struct pci_dev *pdev = NULL;
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
995 void __iomem *fb_virt;
9069fd54960304 drivers/video/hyperv_fb.c Gerd Hoffmann 2014-02-26
996 int gen2vm = efi_enabled(EFI_BOOT);
81d2393485f099 drivers/video/fbdev/hyperv_fb.c Thomas Zimmermann 2022-12-19
997 resource_size_t base, size;
3a6fb6c4255c38 drivers/video/fbdev/hyperv_fb.c Wei Hu2019-12-09
998 phys_addr_t paddr;
9069fd54960304 drivers/video/hyperv_fb.c Gerd Hoffmann 2014-02-26
999 int ret;
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
1000
3a6fb6c4255c38 drivers/video/fbdev/hyperv_fb.c Wei Hu2019-12-09
1001 if (!gen2vm) {
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
1002 pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
1003 PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
1004 if (!pdev) {
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29 1005
pr_err("Unable to find PCI Hyper-V video\n");
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
1006 return -ENODEV;
68a2d20b79b105 drivers/video/hyperv_fb.c Haiyang Zhang 2013-04-29
Re: [PATCH] drm/mediatek: Fix potential memory leak if vmap() fail
Hi,
On 2023/7/6 20:47, AngeloGioacchino Del Regno wrote:
Il 26/06/23 20:58, Sui Jingfeng ha scritto:
Also return -ENOMEM if such a failure happens, the implement should take
responsibility for the error handling.
Signed-off-by: Sui Jingfeng
This commit needs a Fixes tag. Please add the relevant one and resend.
Done at [1],
Thanks for point this out, I got it.
[1] https://patchwork.freedesktop.org/patch/545843/?series=119885=2
Thanks,
Angelo
---
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index a25b28d3ee90..9f364df52478 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -247,7 +247,11 @@ int mtk_drm_gem_prime_vmap(struct drm_gem_object
*obj, struct iosys_map *map)
mtk_gem->kvaddr = vmap(mtk_gem->pages, npages, VM_MAP,
pgprot_writecombine(PAGE_KERNEL));
-
+ if (!mtk_gem->kvaddr) {
+ kfree(sgt);
+ kfree(mtk_gem->pages);
+ return -ENOMEM;
+ }
out:
kfree(sgt);
iosys_map_set_vaddr(map, mtk_gem->kvaddr);
Re: [PATCH] drm/mediatek: Fix potential memory leak if vmap() fail
Hi, thanks a lot! On 2023/7/6 20:13, Alexandre Mergnat wrote: On 26/06/2023 20:58, Sui Jingfeng wrote: Also return -ENOMEM if such a failure happens, the implement should take responsibility for the error handling. Reviewed-by: Alexandre Mergnat
Re: [PATCH] drm/mediatek: Fix potential memory leak if vmap() fail
Hi, Thanks a lot!
On 2023/7/6 19:39, Matthias Brugger wrote:
On 26/06/2023 20:58, Sui Jingfeng wrote:
Also return -ENOMEM if such a failure happens, the implement should take
responsibility for the error handling.
Signed-off-by: Sui Jingfeng
---
drivers/gpu/drm/mediatek/mtk_drm_gem.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
index a25b28d3ee90..9f364df52478 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
@@ -247,7 +247,11 @@ int mtk_drm_gem_prime_vmap(struct drm_gem_object
*obj, struct iosys_map *map)
mtk_gem->kvaddr = vmap(mtk_gem->pages, npages, VM_MAP,
pgprot_writecombine(PAGE_KERNEL));
-
+ if (!mtk_gem->kvaddr) {
+ kfree(sgt);
+ kfree(mtk_gem->pages);
+ return -ENOMEM;
+ }
Reviewed-by: Matthias Brugger
out:
kfree(sgt);
iosys_map_set_vaddr(map, mtk_gem->kvaddr);
Re: [32/39] drm: renesas: shmobile: Shutdown the display on remove
Hi,
On 2023/7/5 18:29, Geert Uytterhoeven wrote:
Hi Sui,
On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng wrote:
On 2023/6/22 17:21, Geert Uytterhoeven wrote:
When the device is unbound from the driver, the display may be active.
Make sure it gets shut down.
would you mind to give a short description why this is necessary.
That's a good comment.
It turned out that this is not really necessary here, but to avoid a regression
with "[PATCH 34/39] drm: renesas: shmobile: Atomic conversion part 1", where
it is needed to call drm_atomic_helper_shutdown().
As the comments for drm_atomic_helper_shutdown() says it is the
atomic version of drm_helper_force_disable_all(), I figured I had to
introduce a call to the latter first, before doing the atomic conversion.
Does that make sense?
I'm just noticed that I'm actually ask a duplicated question.
I didn't notice Laurent's remark about this patch.
I'm actually agree with Laurent that this function should be turned
into drm_atomic_helper_shutdown() finally.
Yes, I do noticed that you replace this function with in [PATCH 34/39],
Originally, I thought this patch could possibly merged with the [PATCH
34/39].
then, the net result of the merged two patch is equivalent to
adding drm_atomic_helper_shutdown() function only in the
shmob_drm_remove() function.
I also realized that it is necessary that disable the CRTC when the
driver going to leave.
Otherwise there are some plane resource still being referenced.
OK, I'm satisfy about you answer (if no other reviewers complains).
Thanks for you answer. :-)
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Laurent Pinchart
--- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
@@ -16,6 +16,7 @@
#include
#include
+#include
#include
#include
#include
@@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev)
struct drm_device *ddev = >ddev;
drm_dev_unregister(ddev);
+ drm_helper_force_disable_all(ddev);
Is it that the DRM core recommend us to use
drm_atomic_helper_disable_all() ?
Well, drm_atomic_helper_shutdown() is a convenience wrapper
around drm_atomic_helper_disable_all()... But we can't call any
atomic helpers yet, before the conversion to atomic modesetting.
drm_kms_helper_poll_fini(ddev);
return 0;
}
Gr{oetje,eeting}s,
Geert
Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register
Hi, On 2023/6/30 01:44, Limonciello, Mario wrote: [Public] -Original Message- From: 15330273...@189.cn <15330273...@189.cn> Sent: Thursday, June 29, 2023 12:00 PM To: Bjorn Helgaas ; Sui Jingfeng Cc: Bjorn Helgaas ; linux-fb...@vger.kernel.org; Cornelia Huck ; Karol Herbst ; nouv...@lists.freedesktop.org; Joonas Lahtinen ; dri-devel@lists.freedesktop.org; Chai, Thomas ; Limonciello, Mario ; Gao, Likun ; David Airlie ; Ville Syrjala ; Yi Liu ; k...@vger.kernel.org; amd-...@lists.freedesktop.org; Jason Gunthorpe ; Ben Skeggs ; linux- p...@vger.kernel.org; Kevin Tian ; Lazar, Lijo ; Thomas Zimmermann ; Zhang, Bokun ; intel-...@lists.freedesktop.org; Maarten Lankhorst ; Jani Nikula ; Alex Williamson ; Abhishek Sahu ; Maxime Ripard ; Rodrigo Vivi ; Tvrtko Ursulin ; Yishai Hadas ; Pan, Xinhui ; linux- ker...@vger.kernel.org; Daniel Vetter ; Deucher, Alexander ; Koenig, Christian ; Zhang, Hawking Subject: Re: [PATCH v7 6/8] PCI/VGA: Introduce is_boot_device function callback to vga_client_register Hi, On 2023/6/29 23:54, Bjorn Helgaas wrote: On Thu, Jun 22, 2023 at 01:08:15PM +0800, Sui Jingfeng wrote: Hi, A nouveau developer(Lyude) from redhat send me a R-B, Thanks for the developers of nouveau project. Please allow me add a link[1] here. [1] https://lore.kernel.org/all/0afadc69f99a36bc9d03ecf54ff25859dbc10e28.ca m...@redhat.com/ 1) Thanks for this. If you post another version of this series, please pick up Lyude's Reviewed-by and include it in the relevant patches (as long as you haven't made significant changes to the code Lyude reviewed). Yes, no significant changes. Just fix typo. I also would like to add support for other DRM drivers. But I think this deserve another patch. Whoever applies this should automatically pick up Reviewed-by/Ack/etc that are replies to the version being applied, but they won't go through previous revisions to find them. 2) Please mention the commit to which the series applies. I tried to apply this on v6.4-rc1, but it doesn't apply cleanly. Since I'm a graphic driver developer, I'm using drm-tip. I just have already pulled, it still apply cleanly on drm-tip. 3) Thanks for including cover letters in your postings. Please include a little changelog in the cover letter so we know what changed between v6 and v7, etc. No change between v6 and v7, it seems that it is because the mailbox don't allow me to sending too many mails a day. so some of the patch is failed to delivery because out of quota. 4) Right now we're in the middle of the v6.5 merge window, so new content, e.g., this series, is too late for v6.5. Most maintainers, including me, wait to merge new content until the merge window closes and a new -rc1 is tagged. This merge window should close on July 9, and people will start merging content for v6.6, typically based on v6.5-rc1. I'm wondering Would you will merge all of the patches in this series (e.g. including the patch for drm/amdgpu(7/8) and drm/radeon(8/8)) ? Or just part of them? Emm, I don't know because my patch seems across different subsystem of Linux kernel. There is also a developer for AMDGPU (Mario) give me a R-B for the patch-0002 of this series. So, at least, PATCH-0001, PATCH-0002, PATCH-0003, PATCH-0004, PATCH- 0006 are already OK(got reviewed by). Those 5 patch are already qualified to be merged, I think. I think what you can do is pick up all the tags in your next version. Once the whole series has tags we can discuss how it merges. Thanks a lot, Mario. Is it possible to merge the PCI/VGA part as fast as possible, especially the PATCH-0006 PCI/VGA: Introduce is_boot_device function callback to vga_client_register As this patch is fundamental, it introduce no functional change, as long as the drm driver side don't introduce a callback. I'm not hurry, but drm driver-side's patch have a dependency on this patch, I think it is better the PCI/VGA-side's patch got merge first. At least for get the first four cleanup(0001 ~ 0004) patch merged first, so that I don't have to send so much on the next version on one series. Being exposed so far, there no obvious objection. It saying that other people also want it got merged. Bjorn, is this OK ? I means that if you could merge those 5 patch first, then there no need to send another version again. I will refine the rest patch with more details and description. I'm fear of making too much noise. Bjorn
[PATCH] ttm/ttm_device.h: fix a trival typo
should replace '@' with '*'
Signed-off-by: suijingfeng
---
include/drm/ttm/ttm_device.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/drm/ttm/ttm_device.h b/include/drm/ttm/ttm_device.h
index 4f3e81eac6f3..56e82ba2d046 100644
--- a/include/drm/ttm/ttm_device.h
+++ b/include/drm/ttm/ttm_device.h
@@ -141,7 +141,7 @@ struct ttm_device_funcs {
* the graphics address space
* @ctx: context for this move with parameters
* @new_mem: the new memory region receiving the buffer
-@ @hop: placement for driver directed intermediate hop
+* @hop: placement for driver directed intermediate hop
*
* Move a buffer between two memory regions.
* Returns errno -EMULTIHOP if driver requests a hop
--
2.25.1
[PATCH] dma-buf/dma-buf.c: add a blank line between the two adjoining functions
Signed-off-by: suijingfeng <15330273...@189.cn>
---
drivers/dma-buf/dma-buf.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 757c0fb77a6c..45d56aa4319c 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -777,10 +777,10 @@ static void mangle_sg_table(struct sg_table *sg_table)
for_each_sgtable_sg(sg_table, sg, i)
sg->page_link ^= ~0xffUL;
#endif
-
}
-static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach,
- enum dma_data_direction direction)
+
+static struct sg_table *__map_dma_buf(struct dma_buf_attachment *attach,
+ enum dma_data_direction direction)
{
struct sg_table *sg_table;
signed long ret;
--
2.25.1
[PATCH v6 2/2] MAINTAINERS: add maintainers for DRM LOONGSON driver
From: suijingfeng This patch add myself as maintainer to fix following warning when run ./scripts/checkpatch.pl WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? Signed-off-by: suijingfeng Signed-off-by: suijingfeng <15330273...@189.cn> --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 97d814a19475..bb1d84a9ec73 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7022,6 +7022,13 @@ T: git git://anongit.freedesktop.org/drm/drm-misc F: drivers/gpu/drm/lima/ F: include/uapi/drm/lima_drm.h +DRM DRIVERS FOR LOONGSON LSDC +M: suijingfeng +L: dri-devel@lists.freedesktop.org +S: Maintained +T: git git://anongit.freedesktop.org/drm/drm-misc +F: drivers/gpu/drm/lsdc/ + DRM DRIVERS FOR MEDIATEK M: Chun-Kuang Hu M: Philipp Zabel -- 2.25.1
[PATCH v6 1/2] drm: add kms driver for loongson display controller
From: suijingfeng Loongson display controller IP has been integrated in both Loongson North Bridge chipset(ls7a1000 and ls7a2000) and Loongson SoCs(ls2k1000 and ls2k2000 etc), it even has been included in Loongson BMC products. This display controller is a PCI device, it has two display pipe. For the DC in LS7A1000 and LS2K1000 each way has a DVO output interface which provide RGB888 signals, vertical & horizontal synchronisations, and the pixel clock. Each CRTC is able to support 1920x1080@60Hz, the maximum resolution is 2048x2048 according to the hardware spec. For the DC in LS7A2000, each display pipe is equipped with a built-in HDMI encoder which is compliant with HDMI 1.4 specification, thus it support 3840x2160@30Hz. The first display pipe is also equipped with a transparent vga encoder which is parallel with the HDMI encoder. The DC in LS7A2000 is more complete, besides above feature, it has two hardware cursors, two hardware vblank counter and two scanout position recorders. v1 -> v2: 1) Use hpd status reg when polling for ls7a2000 2) Fix all warnings emerged when compile with W=1 v2 -> v3: 1) Add COMPILE_TEST in Kconfig and make the driver off by default 2) Alphabetical sorting headers (Thomas) 3) Untangle register access functions as much as possible (Thomas) 4) Switch to TTM based memory manager and prefer cached mapping for Loongson SoC (Thomas) 5) Add chip id detection method, now all models are distinguishable. 6) Revise builtin HDMI phy driver, nearly all main stream mode below 4K@30Hz is tested, this driver supported these mode very well including clone display mode and extend display mode. v3 -> v4: 1) Quickly fix a small mistake. v4 -> v5: 1) Drop potential support for Loongson 2K series SoC temporary, this part should be resend with the DT binding patch in the future. 2) Add per display pipe debugfs support to the builtin HDMI encoder. 3) Rewrite atomic_update() for hardware cursors plane(Thomas) 4) Rewrite encoder and connector initialization part, untangle it according to the chip(Thomas). v5 -> v6: 1) Remove stray code which didn't get used, say lsdc_of_get_reserved_ram 2) Fix all typos I counld found, make sentences and code more readable 3) Untange lsdc_hdmi*_connector_detect() function according to the pipe 4) After a serious consideration, we rename this driver as loongson. Because we also have drivers toward the LoongGPU IP in LS7A2000 and LS2K2000. Besides, there are also drivers about the external encoder, HDMI audio driver and vbios support etc. This patch only provide DC driver part, my teammate Li Yi believe that loongson is more suitable for various Loongson Graphics related hardware than lsdc in the long run. loongson.ko = LSDC + LoongGPU + encoders driver + vbios/DT ... As a basic KMS driver, the user experience is merely enough when using with X server under mate and xfce desktop environment. This dirver is ready to be merged, and We will take the responsibility if there are any bug happen. Signed-off-by: Li Yi Signed-off-by: suijingfeng Signed-off-by: suijingfeng <15330273...@189.cn> --- drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/loongson/Kconfig| 17 + drivers/gpu/drm/loongson/Makefile | 15 + drivers/gpu/drm/loongson/lsdc_crtc.c| 374 drivers/gpu/drm/loongson/lsdc_debugfs.c | 236 ++ drivers/gpu/drm/loongson/lsdc_drv.c | 495 + drivers/gpu/drm/loongson/lsdc_drv.h | 314 + drivers/gpu/drm/loongson/lsdc_i2c.c | 172 +++ drivers/gpu/drm/loongson/lsdc_irq.c | 84 drivers/gpu/drm/loongson/lsdc_output.c | 568 drivers/gpu/drm/loongson/lsdc_output.h | 14 + drivers/gpu/drm/loongson/lsdc_plane.c | 431 ++ drivers/gpu/drm/loongson/lsdc_pll.c | 338 ++ drivers/gpu/drm/loongson/lsdc_pll.h | 76 drivers/gpu/drm/loongson/lsdc_probe.c | 84 drivers/gpu/drm/loongson/lsdc_regs.h| 353 +++ drivers/gpu/drm/loongson/lsdc_ttm.c | 445 +++ drivers/gpu/drm/loongson/lsdc_ttm.h | 64 +++ 19 files changed, 4083 insertions(+) create mode 100644 drivers/gpu/drm/loongson/Kconfig create mode 100644 drivers/gpu/drm/loongson/Makefile create mode 100644 drivers/gpu/drm/loongson/lsdc_crtc.c create mode 100644 drivers/gpu/drm/loongson/lsdc_debugfs.c create mode 100644 drivers/gpu/drm/loongson/lsdc_drv.c create mode 100644 drivers/gpu/drm/loongson/lsdc_drv.h create mode 100644 drivers/gpu/drm/loongson/lsdc_i2c.c create mode 100644 drivers/gpu/drm/loongson/lsdc_irq.c create mode 100644 drivers/gpu/drm/loongson/lsdc_output.c create mode 100644 drivers/gpu/drm/loongson/lsdc_output.h create mode 100644 drivers/gpu/drm/loongson/lsdc_plane.c create mode 100644
[PATCH v5 1/2] drm: add kms driver for loongson display controller
From: suijingfeng Loongson display controller IP has been integrated in both Loongson North Bridge chipset(ls7a1000 and ls7a2000) and Loongson SoCs(ls2k1000 and ls2k2000 etc), it even has been included in Loongson BMC products. This display controller is a PCI device, it has two display pipe. For the DC in LS7A1000 and LS2K1000 each way has a DVO output interface which provide RGB888 signals, vertical & horizontal synchronisations, and the pixel clock. Each CRTC is able to support 1920x1080@60Hz, the maximum resolution is 2048x2048 according to the hardware spec. For the DC in LS7A2000, each display pipe is equipped with a built-in HDMI encoder which is compliant with HDMI 1.4 specification, thus it support 3840x2160@30Hz. The first display pipe is also equipped with a transparent vga encoder which is parallel with the HDMI encoder. The DC in LS7A2000 is more complete, besides above feature, it has two hardware cursors, two hardware vblank counter and two scanout position recorders. v1 -> v2: 1) Use hpd status reg when polling for ls7a2000 2) Fix all warnings emerged when compile with W=1 v2 -> v3: 1) Add COMPILE_TEST in Kconfig and make the driver off by default 2) Alphabetical sorting headers (Thomas) 3) Untangle register access functions as much as possible (Thomas) 4) Switch to TTM based memory manager and prefer cached mapping for Loongson SoC (Thomas) 5) Add chip id detection method, now all models are distinguishable. 6) Revise builtin HDMI phy driver, nearly all main stream mode below 4K@30Hz is tested, this driver supported these mode very well including clone display mode and extend display mode. v3 -> v4: 1) Quickly fix a small mistake. v4 -> v5: 1) Drop potential support for Loongson 2K series SoC temporary, Maybe this part should be resend with the dt binding patch in the future. 2) Add per display pipe debugfs support to the builtin HDMI encoder. 3) Rewrite atomic_update() for hardware cursors(Thomas) 4) Rewrite encoder and connector initialization part, untangle it according to the chip(Thomas). As a basic KMS driver, the user experience is good enough when using with X server under mate and xfce desktop environment. This dirver is ready to be merged, and We will take the responsibility if there are any bug happen. Signed-off-by: Li Yi Signed-off-by: suijingfeng Signed-off-by: suijingfeng <15330273...@189.cn> --- drivers/gpu/drm/Kconfig | 2 + drivers/gpu/drm/Makefile| 1 + drivers/gpu/drm/lsdc/Kconfig| 16 + drivers/gpu/drm/lsdc/Makefile | 15 + drivers/gpu/drm/lsdc/lsdc_crtc.c| 374 ++ drivers/gpu/drm/lsdc/lsdc_debugfs.c | 236 drivers/gpu/drm/lsdc/lsdc_drv.c | 529 ++ drivers/gpu/drm/lsdc/lsdc_drv.h | 308 +++ drivers/gpu/drm/lsdc/lsdc_i2c.c | 172 + drivers/gpu/drm/lsdc/lsdc_irq.c | 86 + drivers/gpu/drm/lsdc/lsdc_output.c | 570 drivers/gpu/drm/lsdc/lsdc_output.h | 14 + drivers/gpu/drm/lsdc/lsdc_plane.c | 431 + drivers/gpu/drm/lsdc/lsdc_pll.c | 336 drivers/gpu/drm/lsdc/lsdc_pll.h | 76 drivers/gpu/drm/lsdc/lsdc_probe.c | 88 + drivers/gpu/drm/lsdc/lsdc_regs.h| 353 + drivers/gpu/drm/lsdc/lsdc_ttm.c | 451 ++ drivers/gpu/drm/lsdc/lsdc_ttm.h | 64 19 files changed, 4122 insertions(+) create mode 100644 drivers/gpu/drm/lsdc/Kconfig create mode 100644 drivers/gpu/drm/lsdc/Makefile create mode 100644 drivers/gpu/drm/lsdc/lsdc_crtc.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_debugfs.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_drv.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_i2c.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_irq.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_output.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_output.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_plane.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_pll.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_probe.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_regs.h create mode 100644 drivers/gpu/drm/lsdc/lsdc_ttm.c create mode 100644 drivers/gpu/drm/lsdc/lsdc_ttm.h diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index dc0f94f02a82..3baecd48930b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -367,6 +367,8 @@ source "drivers/gpu/drm/solomon/Kconfig" source "drivers/gpu/drm/sprd/Kconfig" +source "drivers/gpu/drm/lsdc/Kconfig" + config DRM_HYPERV tristate "DRM Support for Hyper-V synthetic video device" depends on DRM && PCI && MMU && HYPERV diff --git a/drivers/gpu/drm/Makefile b/
[PATCH v5 2/2] MAINTAINERS: add maintainers for DRM LSDC driver
From: suijingfeng This patch add myself as maintainer to fix following warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? Signed-off-by: suijingfeng Signed-off-by: suijingfeng <15330273...@189.cn> --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 97d814a19475..bb1d84a9ec73 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7022,6 +7022,13 @@ T: git git://anongit.freedesktop.org/drm/drm-misc F: drivers/gpu/drm/lima/ F: include/uapi/drm/lima_drm.h +DRM DRIVERS FOR LOONGSON LSDC +M: suijingfeng +L: dri-devel@lists.freedesktop.org +S: Maintained +T: git git://anongit.freedesktop.org/drm/drm-misc +F: drivers/gpu/drm/lsdc/ + DRM DRIVERS FOR MEDIATEK M: Chun-Kuang Hu M: Philipp Zabel -- 2.25.1
