[PATCH 2/4] drm/etnaviv: add pci device driver support
From: suijingfeng
There is a Vivante GC1000 V5037 in LS2K1000 and LS7A1000,
the gpu in those chip is a PCI device and it have 2D and 3D
in the same core. Therefore, this patch try to provide PCI
device driver wrapper for it by mimic the platform counterpart
one.
LS7A1000 is a bridge chip, this bridge chip typically use
with LS3A4000 (4 core 1.8gHz, Mips64r5) and LS3A5000 (4 core
loongarch 2.5Ghz). While LS2K1000 is a double core 1.0Ghz
Mips64r2 SoC.
loongson CPU's cache coherency is maintained by the hardware.
Both LS7A1000 and LS2K1000 have a display controller integrated,
named lsdc. By using KMS-RO framework, lsdc and gc1000 made a
compatible pair.
Signed-off-by: suijingfeng
Signed-off-by: Sui Jingfeng <15330273...@189.cn>
---
drivers/gpu/drm/etnaviv/Kconfig | 12 ++
drivers/gpu/drm/etnaviv/Makefile | 2 +
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 113 +---
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 8 +
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 28 ++-
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 94 ++
drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 6 +
drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 203 ++
include/uapi/drm/etnaviv_drm.h| 11 +-
9 files changed, 409 insertions(+), 68 deletions(-)
create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
diff --git a/drivers/gpu/drm/etnaviv/Kconfig b/drivers/gpu/drm/etnaviv/Kconfig
index faa7fc68b009..5858eec59025 100644
--- a/drivers/gpu/drm/etnaviv/Kconfig
+++ b/drivers/gpu/drm/etnaviv/Kconfig
@@ -15,6 +15,18 @@ config DRM_ETNAVIV
help
DRM driver for Vivante GPUs.
+config DRM_ETNAVIV_PCI_DRIVER
+ bool "Enable PCI device driver support"
+ depends on DRM_ETNAVIV
+ depends on PCI
+ depends on MACH_LOONGSON64
+ default y
+ help
+ DRM PCI driver for the Vivante GPU in LS7A1000 north bridge
+ and LS2K1000 SoC. The GC1000 in LS2K1000 and LS7A1000 is a
+ PCI device.
+ If in doubt, say "n".
+
config DRM_ETNAVIV_THERMAL
bool "enable ETNAVIV thermal throttling"
depends on DRM_ETNAVIV
diff --git a/drivers/gpu/drm/etnaviv/Makefile b/drivers/gpu/drm/etnaviv/Makefile
index 46e5ffad69a6..6829e1ebf2db 100644
--- a/drivers/gpu/drm/etnaviv/Makefile
+++ b/drivers/gpu/drm/etnaviv/Makefile
@@ -16,4 +16,6 @@ etnaviv-y := \
etnaviv_perfmon.o \
etnaviv_sched.o
+etnaviv-$(CONFIG_DRM_ETNAVIV_PCI_DRIVER) += etnaviv_pci_drv.o
+
obj-$(CONFIG_DRM_ETNAVIV) += etnaviv.o
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 7dcc6392792d..dc2bb3a6efe6 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -8,6 +8,9 @@
#include
#include
#include
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+#include
+#endif
#include
#include
@@ -72,7 +75,7 @@ static int etnaviv_open(struct drm_device *dev, struct
drm_file *file)
drm_sched_entity_init(>sched_entity[i],
DRM_SCHED_PRIORITY_NORMAL, ,
1, NULL);
- }
+ }
}
file->driver_priv = ctx;
@@ -470,7 +473,7 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = {
DEFINE_DRM_GEM_FOPS(fops);
-static const struct drm_driver etnaviv_drm_driver = {
+const struct drm_driver etnaviv_drm_driver = {
.driver_features= DRIVER_GEM | DRIVER_RENDER,
.open = etnaviv_open,
.postclose = etnaviv_postclose,
@@ -491,6 +494,36 @@ static const struct drm_driver etnaviv_drm_driver = {
.minor = 3,
};
+
+/*
+ * PCI device driver:
+ */
+
+#ifdef CONFIG_DRM_ETNAVIV_PCI_DRIVER
+enum vivante_gpu_pci_family {
+ GC1000_IN_LS7A1000 = 0,
+ GC1000_IN_LS2K1000 = 1,
+ CHIP_LAST,
+};
+
+static const struct pci_device_id etnaviv_pci_id_lists[] = {
+ {0x0014, 0x7a15, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS7A1000},
+ {0x0014, 0x7a05, PCI_ANY_ID, PCI_ANY_ID, 0, 0, GC1000_IN_LS2K1000},
+ {0, 0, 0, 0, 0, 0, 0}
+};
+
+static struct pci_driver etnaviv_pci_driver = {
+ .name = "etnaviv",
+ .id_table = etnaviv_pci_id_lists,
+ .probe = etnaviv_pci_probe,
+ .remove = etnaviv_pci_remove,
+ .driver = {
+ .name = "etnaviv",
+ },
+};
+#endif
+
+
/*
* Platform driver:
*/
@@ -629,10 +662,40 @@ static struct platform_driver etnaviv_platform_driver = {
static struct platform_device *etnaviv_drm;
-static int __init etnaviv_init(void)
+static int etnaviv_create_platform_device(const char *name, struct device_node
*np)
{
struct platform_device *pdev;
int ret;
+
+ pdev = platform_device_alloc(name, -1);
+ if (!pdev)
+ return -ENOMEM;
+
+ pdev->dev.coherent_dma_mask = DMA_BIT_MASK(40);
[PATCH 1/4] dt-bindings: ls2k1000: add gpu device node
From: suijingfeng
There is a vivante gpu (GC1000 V5037) in ls2k1000,
but it is pci device not platform device.
ls2k1000 is dual-core mips64 cpu made by loongson.
Signed-off-by: suijingfeng
Signed-off-by: Sui Jingfeng <15330273...@189.cn>
---
arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi | 11 +++
1 file changed, 11 insertions(+)
diff --git a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
index bfc3d3243ee7..f1feffac78a6 100644
--- a/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
+++ b/arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
@@ -193,6 +193,17 @@
interrupt-parent = <>;
};
+ gpu@5,0 {
+ compatible = "pci0014,7a05.0",
+ "pci0014,7a05",
+ "pciclass030200",
+ "pciclass0302";
+
+ reg = <0x2800 0x0 0x0 0x0 0x0>;
+ interrupts = <29 IRQ_TYPE_LEVEL_LOW>;
+ interrupt-parent = <>;
+ };
+
pci_bridge@9,0 {
compatible = "pci0014,7a19.0",
"pci0014,7a19",
--
2.20.1
drm/etnaviv: add pci device driver support for gpu in LS7A1000 and LS2K1000
There is a Vivante GC1000 V5037 in LS2K1000 and LS7A1000, the gpu is a PCI device and it have 2D and 3D in the same core. Therefore, this patch try to provide PCI device driver wrapper for it by mimic the platform counterpart. LS7A1000 is a bridge chip, this bridge chip typically use with LS3A4000 (4 core 1.8gHz, Mips64r5) and LS3A5000 (4 core loongarch 2.5Ghz). While LS2K1000 is a double core 1.0Ghz Mips64r2 SoC. loongson CPU's cache coherency is maintained by the hardware. Both LS7A1000 and LS2K1000 have a display controller integrated, named lsdc. The drm driver of this display controller is not upstream yet, but we have a demo version and it works. By using KMS-RO framework, lsdc and gc1000 made a compatible pair. suijingfeng (4): dt-bindings: ls2k1000: add gpu device node drm/etnaviv: add pci device driver support loongson3_defconfig: enable etnaviv drm driver on default loongson2_defconfig: enable etnaviv drm driver on default .../boot/dts/loongson/loongson64-2k1000.dtsi | 11 + arch/mips/configs/loongson2k_defconfig| 1 + arch/mips/configs/loongson3_defconfig | 1 + drivers/gpu/drm/etnaviv/Kconfig | 12 ++ drivers/gpu/drm/etnaviv/Makefile | 2 + drivers/gpu/drm/etnaviv/etnaviv_drv.c | 113 +++--- drivers/gpu/drm/etnaviv/etnaviv_drv.h | 8 + drivers/gpu/drm/etnaviv/etnaviv_gem.c | 28 ++- drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 94 +--- drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 6 + drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 203 ++ include/uapi/drm/etnaviv_drm.h| 11 +- 12 files changed, 422 insertions(+), 68 deletions(-) create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c -- 2.20.1
Re: [PATCH bpf] treewide: add missing includes masked by cgroup -> bpf dependency
Hi Jakub, On Fri, 19 Nov 2021 19:52:53 -0800 Jakub Kicinski wrote: > cgroup.h (therefore swap.h, therefore half of the universe) > includes bpf.h which in turn includes module.h and slab.h. > Since we're about to get rid of that dependency we need > to clean things up. > > Signed-off-by: Jakub Kicinski Acked-by: SeongJae Park for DAMON part. Thanks, SJ [...]
RE: [PATCH] i2c: tegra: Add ACPI support
>
>
> On Fri, Nov 19, 2021 at 3:37 PM Akhil R wrote:
> >
> > Add support for ACPI based device registration so that the driver can
> > be also enabled through ACPI table.
>
> the ACPI
>
> ...
>
> > + if (has_acpi_companion(i2c_dev->dev)) {
>
> You are checkin for the companion and using a handle, why not check for a
> handle explicitly?
Okay.
>
> > + acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
> > +NULL, NULL);
> > + } else {
> > + err = reset_control_reset(i2c_dev->rst);
> > + WARN_ON_ONCE(err);
> > + }
>
> ...
>
> > + if (i2c_dev->nclocks == 0)
> > + return;
>
> Why? Make clocks optional.
>
> ...
>
> > - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev,
> > "i2c");
> > - if (IS_ERR(i2c_dev->rst)) {
>
> > - dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> > - "failed to get reset control\n");
> > - return PTR_ERR(i2c_dev->rst);
>
> Besides the fact this should be as simple as
>
> return dev_err_probe(...)
>
> > - }
>
> > + if (!has_acpi_companion(>dev)) {
>
> ...why do you do this?
The thought was to call out the error when using device tree and to ignore if
using ACPI table.
We are expecting the clocks to be initialized from the bootloader and to use
the _RST method
(instead of reset_control), when an ACPI table is used.
The problem I thought when making it optional is that an error could go
unnoticed when using a
device tree as well.
Best Regards,
Akhil
Re: [PATCH bpf] treewide: add missing includes masked by cgroup -> bpf dependency
On 21-11-19 19:52:53, Jakub Kicinski wrote: > cgroup.h (therefore swap.h, therefore half of the universe) > includes bpf.h which in turn includes module.h and slab.h. > Since we're about to get rid of that dependency we need > to clean things up. > > Signed-off-by: Jakub Kicinski > --- > static inline struct inode *bdev_file_inode(struct file *file) > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c > b/drivers/gpu/drm/drm_gem_shmem_helper.c > index 7b9f69f21f1e..bca0de92802e 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -9,6 +9,7 @@ > #include > #include > #include > +#include > > #ifdef CONFIG_X86 > #include > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c > b/drivers/gpu/drm/i915/gt/intel_gtt.c > index 67d14afa6623..b67f620c3d93 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gtt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c > @@ -6,6 +6,7 @@ > #include /* fault-inject.h is not standalone! */ > > #include > +#include > > #include "gem/i915_gem_lmem.h" > #include "i915_trace.h" > diff --git a/drivers/gpu/drm/i915/i915_request.c > b/drivers/gpu/drm/i915/i915_request.c > index 820a1f38b271..89cccefeea63 100644 > --- a/drivers/gpu/drm/i915/i915_request.c > +++ b/drivers/gpu/drm/i915/i915_request.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > > #include "gem/i915_gem_context.h" > #include "gt/intel_breadcrumbs.h" > diff --git a/drivers/gpu/drm/lima/lima_device.c > b/drivers/gpu/drm/lima/lima_device.c > index 65fdca366e41..f74f8048af8f 100644 > --- a/drivers/gpu/drm/lima/lima_device.c > +++ b/drivers/gpu/drm/lima/lima_device.c > @@ -4,6 +4,7 @@ > #include > #include > #include > +#include > #include > #include > > diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c > b/drivers/gpu/drm/msm/msm_gem_shrinker.c > index 4a1420b05e97..086dacf2f26a 100644 > --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c > +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c > @@ -5,6 +5,7 @@ > */ > > #include > +#include > > #include "msm_drv.h" > #include "msm_gem.h" > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c > index 7e83c00a3f48..79c870a3bef8 100644 > --- a/drivers/gpu/drm/ttm/ttm_tt.c > +++ b/drivers/gpu/drm/ttm/ttm_tt.c > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include > #include > #include > > diff --git a/drivers/net/ethernet/huawei/hinic/hinic_sriov.c > b/drivers/net/ethernet/huawei/hinic/hinic_sriov.c > index a78c398bf5b2..01e7d3c0b68e 100644 > --- a/drivers/net/ethernet/huawei/hinic/hinic_sriov.c > +++ b/drivers/net/ethernet/huawei/hinic/hinic_sriov.c > @@ -8,6 +8,7 @@ > #include > #include > #include > +#include > > #include "hinic_hw_dev.h" > #include "hinic_dev.h" > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c > b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c > index 0ef68fdd1f26..61c20907315f 100644 > --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c > @@ -5,6 +5,8 @@ > * > */ > > +#include > + > #include "otx2_common.h" > #include "otx2_ptp.h" > > diff --git a/drivers/pci/controller/dwc/pci-exynos.c > b/drivers/pci/controller/dwc/pci-exynos.c > index c24dab383654..722dacdd5a17 100644 > --- a/drivers/pci/controller/dwc/pci-exynos.c > +++ b/drivers/pci/controller/dwc/pci-exynos.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > > #include "pcie-designware.h" > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c > b/drivers/pci/controller/dwc/pcie-qcom-ep.c > index 7b17da2f9b3f..cfe66bf04c1d 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > > #include "pcie-designware.h" > > diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c > index 84dadfa726aa..9643b905e2d8 100644 > --- a/drivers/usb/cdns3/host.c > +++ b/drivers/usb/cdns3/host.c > @@ -10,6 +10,7 @@ > */ > > #include > +#include Should be "#include "? -- Thanks, Peter Chen
[PATCH bpf] treewide: add missing includes masked by cgroup -> bpf dependency
cgroup.h (therefore swap.h, therefore half of the universe) includes bpf.h which in turn includes module.h and slab.h. Since we're about to get rid of that dependency we need to clean things up. Signed-off-by: Jakub Kicinski --- CC: ax...@kernel.dk CC: maarten.lankho...@linux.intel.com CC: mrip...@kernel.org CC: tzimmerm...@suse.de CC: airl...@linux.ie CC: dan...@ffwll.ch CC: jani.nik...@linux.intel.com CC: joonas.lahti...@linux.intel.com CC: rodrigo.v...@intel.com CC: yuq...@gmail.com CC: robdcl...@gmail.com CC: s...@poorly.run CC: christian.koe...@amd.com CC: ray.hu...@amd.com CC: sgout...@marvell.com CC: gak...@marvell.com CC: sbha...@marvell.com CC: hke...@marvell.com CC: jingooh...@gmail.com CC: lorenzo.pieral...@arm.com CC: r...@kernel.org CC: k...@linux.com CC: bhelg...@google.com CC: krzysztof.kozlow...@canonical.com CC: m...@kernel.org CC: paw...@cadence.com CC: peter.c...@kernel.org CC: rog...@kernel.org CC: a-govindr...@ti.com CC: gre...@linuxfoundation.org CC: a...@kernel.org CC: dan...@iogearbox.net CC: and...@kernel.org CC: ka...@fb.com CC: songliubrav...@fb.com CC: y...@fb.com CC: john.fastab...@gmail.com CC: kpsi...@kernel.org CC: s...@kernel.org CC: a...@linux-foundation.org CC: thomas.hellst...@linux.intel.com CC: matthew.a...@intel.com CC: colin.k...@intel.com CC: ge...@linux-m68k.org CC: linux-bl...@vger.kernel.org CC: dri-devel@lists.freedesktop.org CC: intel-...@lists.freedesktop.org CC: l...@lists.freedesktop.org CC: linux-arm-...@vger.kernel.org CC: freedr...@lists.freedesktop.org CC: linux-...@vger.kernel.org CC: linux-arm-ker...@lists.infradead.org CC: linux-samsung-...@vger.kernel.org CC: linux-...@vger.kernel.org CC: b...@vger.kernel.org CC: linux...@kvack.org Well, let's see if this makes it thru email servers... --- block/fops.c | 1 + drivers/gpu/drm/drm_gem_shmem_helper.c| 1 + drivers/gpu/drm/i915/gt/intel_gtt.c | 1 + drivers/gpu/drm/i915/i915_request.c | 1 + drivers/gpu/drm/lima/lima_device.c| 1 + drivers/gpu/drm/msm/msm_gem_shrinker.c| 1 + drivers/gpu/drm/ttm/ttm_tt.c | 1 + drivers/net/ethernet/huawei/hinic/hinic_sriov.c | 1 + drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c | 2 ++ drivers/pci/controller/dwc/pci-exynos.c | 1 + drivers/pci/controller/dwc/pcie-qcom-ep.c | 1 + drivers/usb/cdns3/host.c | 1 + include/linux/device/driver.h | 1 + include/linux/filter.h| 2 +- mm/damon/vaddr.c | 1 + mm/memory_hotplug.c | 1 + mm/swap_slots.c | 1 + 17 files changed, 18 insertions(+), 1 deletion(-) diff --git a/block/fops.c b/block/fops.c index ad732a36f9b3..3cb1e81929bc 100644 --- a/block/fops.c +++ b/block/fops.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "blk.h" static inline struct inode *bdev_file_inode(struct file *file) diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 7b9f69f21f1e..bca0de92802e 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -9,6 +9,7 @@ #include #include #include +#include #ifdef CONFIG_X86 #include diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c b/drivers/gpu/drm/i915/gt/intel_gtt.c index 67d14afa6623..b67f620c3d93 100644 --- a/drivers/gpu/drm/i915/gt/intel_gtt.c +++ b/drivers/gpu/drm/i915/gt/intel_gtt.c @@ -6,6 +6,7 @@ #include /* fault-inject.h is not standalone! */ #include +#include #include "gem/i915_gem_lmem.h" #include "i915_trace.h" diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c index 820a1f38b271..89cccefeea63 100644 --- a/drivers/gpu/drm/i915/i915_request.c +++ b/drivers/gpu/drm/i915/i915_request.c @@ -29,6 +29,7 @@ #include #include #include +#include #include "gem/i915_gem_context.h" #include "gt/intel_breadcrumbs.h" diff --git a/drivers/gpu/drm/lima/lima_device.c b/drivers/gpu/drm/lima/lima_device.c index 65fdca366e41..f74f8048af8f 100644 --- a/drivers/gpu/drm/lima/lima_device.c +++ b/drivers/gpu/drm/lima/lima_device.c @@ -4,6 +4,7 @@ #include #include #include +#include #include #include diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c b/drivers/gpu/drm/msm/msm_gem_shrinker.c index 4a1420b05e97..086dacf2f26a 100644 --- a/drivers/gpu/drm/msm/msm_gem_shrinker.c +++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c @@ -5,6 +5,7 @@ */ #include +#include #include "msm_drv.h" #include "msm_gem.h" diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c index 7e83c00a3f48..79c870a3bef8 100644 --- a/drivers/gpu/drm/ttm/ttm_tt.c +++ b/drivers/gpu/drm/ttm/ttm_tt.c @@ -34,6 +34,7 @@ #include #include #include
Re: [PATCH 1/3] drm/simpledrm: Bind to OF framebuffers in /chosen
On 18/11/2021 18.19, Thomas Zimmermann wrote:
Hi
Am 17.11.21 um 15:58 schrieb Hector Martin:
@@ -897,5 +898,21 @@ static struct platform_driver simpledrm_platform_driver = {
module_platform_driver(simpledrm_platform_driver);
+static int __init simpledrm_init(void)
+{
+ struct device_node *np;
+
+ if (IS_ENABLED(CONFIG_OF_ADDRESS) && of_chosen) {
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
+ of_platform_device_create(np, NULL, NULL);
+ }
+ }
+
+ return 0;
+}
+
+fs_initcall(simpledrm_init);
+
Simpledrm is just a driver, but this is platform setup code. Why is this
code located here and not under arch/ or drivers/firmware/?
I know that other drivers do similar things, it doesn't seem to belong here.
This definitely doesn't belong in either of those, since it is not arch-
or firmware-specific. It is implementing support for the standard
simple-framebuffer OF binding, which specifies that it must be located
within the /chosen node (and thus the default OF setup code won't do the
matching for you); this applies to all OF platforms [1]
Adding Rob; do you think this should move from simplefb/simpledrm to
common OF code? (where?)
[1] Documentation/devicetree/bindings/display/simple-framebuffer.yaml
--
Hector Martin (mar...@marcan.st)
Public Key: https://mrcn.st/pub
Re: [PATCH v3 11/12] drm/virtio: implement context init: add virtio_gpu_fence_event
On Fri, Nov 19, 2021 at 9:38 AM Rob Clark wrote: > On Thu, Nov 18, 2021 at 12:53 AM Daniel Vetter wrote: > > > > On Tue, Nov 16, 2021 at 06:31:10PM -0800, Gurchetan Singh wrote: > > > On Tue, Nov 16, 2021 at 7:43 AM Daniel Vetter wrote: > > > > > > > On Mon, Nov 15, 2021 at 07:26:14PM +, Kasireddy, Vivek wrote: > > > > > Hi Daniel, Greg, > > > > > > > > > > If it is the same or a similar crash reported here: > > > > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2021-November/330018.html > > > > > and here: > > > > > https://lists.freedesktop.org/archives/dri-devel/2021-November/330212.html > > > > > then the fix is already merged: > > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d89c0c8322ecdc9a2ec84b959b6f766be082da76 > > > > > > Yeah but that still leaves the problem of why exaxtly virtgpu is > > > > reinventing drm_poll here? > > > > > > > > > > Can you please replace it with drm_poll like all other drivers, > including > > > > the ones that have private events? > > > > > > > > > > Hi Daniel, > > > > > > Allow me to explain the use case a bit. It's for when virtgpu KMS is > not > > > used, but a special Wayland compositor does wayland passthrough > instead: > > > > > > > https://www.youtube.com/watch?v=WwrXqDERFm8https://www.youtube.com/watch?v=EkNBsBx501Q > > > > > > This technique has gained much popularity in the virtualized laptop > > > space, where it offers better performance/user experience than virtgpu > > > KMS. The relevant paravirtualized userspace is "Sommelier": > > > > > > > https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/sommelier/ > > > > https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/sommelier/virtualization/virtgpu_channel.cc > > > > > > Previously, we were using the out-of-tree virtio-wl device and there > > > were many discussions on how we could get this upstream: > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2017-December/160309.html > > > https://lists.oasis-open.org/archives/virtio-dev/202002/msg5.html > > > > > > Extending virtgpu was deemed the least intrusive option: > > > > > > https://www.spinics.net/lists/kvm/msg159206.html > > > > > > We ultimately settled on the context type abstraction and used > > > virtio_gpu_poll to tell the guest "hey, we have a Wayland event". The > > > host response is actually in a buffer of type BLOB_MEM_GUEST. > > > > > > It is possible to use drm_poll(..), but that would have to be > > > accompanied by a drm_read(..). You'll need to define a dummy > > > VIRTGPU_EVENT_FENCE_SIGNALED in the uapi too. > > > > > > That's originally how I did it, but some pointed out that's > > > unnecessary since the host response is in the BLOB_MEM_GUEST buffer > > > and virtgpu event is a dummy event. So we decided just to modify > > > virtio_gpu_poll(..) to have the desired semantics in that case. > > > > > > For the regular virtio-gpu KMS path, things remain unchanged. > > > > > > There are of course other ways to do it (perhaps polling a dma_fence), > > > but that was the cleanest way we could find. > > > > > > It's not rare for virtio to "special things" (see virtio_dma_buf_ops, > > > virtio_dma_ops), since they are in fake devices. > > > > These are all internal interfaces, not uapi. > > > > > We're open to other ideas, but hopefully that answers some of your > > > questions. > > > > Well for one, why does the commit message not explain any of this. You're > > building uapi, which is forever, it's paramount all considerations are > > properly explained. > > > > Second, I really don't like that youre redefining poll semantics in > > incompatible ways from all other drm drivers. If you want special poll > > semantics then just create a sperate fd for that (or a dma_fence or > > whatever, maybe that saves some typing), but bending the drm fd semantics > > is no good at all. We have tons of different fd with their dedicated > > semantics in drm, trying to shoehorn it all into one just isn't very good > > design. > > > > Or do the dummy event which is just the event code, but does not contain > > any data. Either is fine with me. > > > > Can you pls do this asap? I really don't want to bake this in as uapi > > which we then have to quirk and support forever. I'd say revert for -rc2 > > of these two and then maybe sort it out properly in -next. > > I think as a general rule, if there is not consensus about uabi > change, even if it is just a semantic change, then revert and reland > later is ok.. > > As far as dummy VIRTGPU_EVENT_FENCE_SIGNALED.. that doesn't entirely > sound like a bad thing to me. Actually, it sounds like a good thing.. > it makes it more explicit what is going on. And would avoid confusing > a different userspace polling for kms related events expecting to be > able to do a read. > If dummy events work, then it's actually not a big change to make. Expect patches in the upcoming business days. >
Re: [PATCH] drm/hyperv: Fix device removal on Gen1 VMs
Thanks for the patch.
Reviewed-by: Deepak Rawat
I will push this to drm-fixes, let me know otherwise.
Deepak
On Fri, 2021-11-19 at 12:29 +0100, Mohammed Gamal wrote:
> The Hyper-V DRM driver tries to free MMIO region on removing
> the device regardless of VM type, while Gen1 VMs don't use MMIO
> and hence causing the kernel to crash on a NULL pointer dereference.
>
> Fix this by making deallocating MMIO only on Gen2 machines and
> implement
> removal for Gen1
>
> Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic
> video device")
>
> Signed-off-by: Mohammed Gamal
> ---
> drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 19 ++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> index cd818a629183..9f923beb7d8d 100644
> --- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> +++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
> @@ -225,12 +225,29 @@ static int hyperv_vmbus_remove(struct hv_device
> *hdev)
> {
> struct drm_device *dev = hv_get_drvdata(hdev);
> struct hyperv_drm_device *hv = to_hv(dev);
> + struct pci_dev *pdev;
>
> drm_dev_unplug(dev);
> drm_atomic_helper_shutdown(dev);
> vmbus_close(hdev->channel);
> hv_set_drvdata(hdev, NULL);
> - vmbus_free_mmio(hv->mem->start, hv->fb_size);
> +
> + /*
> + * Free allocated MMIO memory only on Gen2 VMs.
> + * On Gen1 VMs, release the PCI device
> + */
> + if (efi_enabled(EFI_BOOT)) {
> + vmbus_free_mmio(hv->mem->start, hv->fb_size);
> + } else {
> + pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
> + PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> + if (!pdev) {
> + drm_err(dev, "Unable to find PCI Hyper-V
> video\n");
> + return -ENODEV;
> + }
> + pci_release_region(pdev, 0);
> + pci_dev_put(pdev);
> + }
>
> return 0;
> }
Re: [PATCH 2/2] drm/msm/gpu: Respect PM QoS constraints
Hi,
On Fri, Nov 19, 2021 at 2:47 PM Rob Clark wrote:
>
> +void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> +{
> + struct msm_gpu_devfreq *df = >devfreq;
> + unsigned long freq;
> +
> + freq = get_freq(gpu);
> + freq *= factor;
> + freq /= HZ_PER_KHZ;
Should it do the divide first? I don't know for sure, but it feels
like GPU frequency could conceivably be near-ish the u32 overflow? (~4
GHz). Better to be safe and do the / 1000 first?
> @@ -201,26 +217,14 @@ static void msm_devfreq_idle_work(struct kthread_work
> *work)
> struct msm_gpu_devfreq *df = container_of(work,
> struct msm_gpu_devfreq, idle_work.work);
> struct msm_gpu *gpu = container_of(df, struct msm_gpu, devfreq);
> - unsigned long idle_freq, target_freq = 0;
>
> if (!df->devfreq)
> return;
Why does the msm_devfreq_idle_work() need a check for "!df->devfreq"
but the boost work doesn't? Maybe you don't need it anymore now that
you're not reaching into the mutex? ...or maybe the boost work does
need it?
...and if "df->devfreq" is NULL then doesn't it mean that
msm_hrtimer_work_init() was never called? That seems bad...
-Doug
Re: [PATCH 1/2] drm/msm/gpu: Fix idle_work time
Hi, On Fri, Nov 19, 2021 at 2:47 PM Rob Clark wrote: > > From: Rob Clark > > This was supposed to be a relative timer, not absolute. > > Signed-off-by: Rob Clark > --- > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Douglas Anderson
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On Fri, 19 Nov 2021 15:46:31 -0700 jim.cro...@gmail.com wrote: > > So I could see us supporting subsystem specific trace buffer output > > via dynamic debug here. We could add new dev_debug() variants that > > allow say a trace buffer to be supplied. So in that way subsystems > > could 'opt-out' of having their data put into the global trace buffer. > > And perhaps some subsystems we would want to allow output to both > > buffers? The subsystem specific one and the global one? > > > > * trace_array_printk - Print a message to a specific instance > * @tr: The instance trace_array descriptor > * @ip: The instruction pointer that this is called from. > * @fmt: The format to print (printf format) > * > > what happens when @tr == NULL ? It does nothing, but perhaps crash the kernel. > It could allow up-flow of events to the global instance Absolutely not! Then it's just a reimplementation of trace_printk(). Which I refuse to have. Nothing should just dump to the main instance. Once we allow that, then everyone will be dumping there and you will no longer be able to trace anything because it will be filled with noise. What is allowed is an event that acts like a trace_printk() but is an event, which you can turn off (have default off), and even pick which instance to go to. > > > Thanks, > > > > -Jason > > > > > > So I wonder, is there any conceptual utility to this ? > > echo 1 > instances/foo/filter_up # enable event upflow (or query-time > merging?) > > Maybe enabling this causes other files (the ones missing from > instances/foo) to magically appear > so all those filtering capacities also appear. I've been busy doing other things so I haven't been keeping up with this thread (which I need to go back and read). Perhaps it was already stated, but I don't know why you want that. trace-cmd can read several instances (including the top level one) and interleave them nicely, if that is what you are looking for. So can KernelShark. -- Steve
Re: [PATCH 1/2] drm/msm/gpu: Fix idle_work time
On Fri, Nov 19, 2021 at 2:46 PM Rob Clark wrote:
>
> From: Rob Clark
>
> This was supposed to be a relative timer, not absolute.
>
Fixes: 658f4c829688 ("drm/msm/devfreq: Add 1ms delay before clamping freq")
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index 43468919df61..7285041c737e 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -228,5 +228,5 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> struct msm_gpu_devfreq *df = >devfreq;
>
> msm_hrtimer_queue_work(>idle_work, ms_to_ktime(1),
> - HRTIMER_MODE_ABS);
> + HRTIMER_MODE_REL);
> }
> --
> 2.33.1
>
Re: [PATCH 2/2] dt-bindings: panel: Introduce a panel-lvds binding
Hi Maxime, Thank you for the patch. On Tue, Nov 16, 2021 at 03:35:03PM +0100, Maxime Ripard wrote: > Following the previous patch, let's introduce a generic panel-lvds > binding that documents the panels that don't have any particular > constraint documented. > > Signed-off-by: Maxime Ripard > --- > .../bindings/display/panel/panel-lvds.yaml| 56 +++ > 1 file changed, 56 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/panel/panel-lvds.yaml > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml > b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml > new file mode 100644 > index ..f6ce8e29391e > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/panel/panel-lvds.yaml > @@ -0,0 +1,56 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/panel/panel-lvds.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Generic LVDS Display Panel Device Tree Bindings > + > +maintainers: > + - Lad Prabhakar > + - Thierry Reding > + > +allOf: > + - $ref: panel-common.yaml# > + - $ref: lvds.yaml# > + > +select: > + properties: > +compatible: > + contains: > +const: panel-lvds > + > + not: > +properties: > + compatible: > +contains: > + enum: > + - advantech,idk-1110wr > + - innolux,ee101ia-01d > + - mitsubishi,aa104xd12 > + - mitsubishi,aa121td01 > + - sgd,gktw70sdae4se This will be annoying to maintain, I'm pretty sure authors will forget to update this file when adding bindings for other panels. Is there any way we could select this binding with a positive rule instead of a negative rule ? > + > + required: > +- compatible > + > +properties: > + compatible: > +items: > + - enum: > + - auo,b101ew05 > + - tbs,a711-panel > + > + - const: panel-lvds > + > +unevaluatedProperties: false > + > +required: > + - compatible > + - data-mapping > + - width-mm > + - height-mm > + - panel-timing > + - port > + > +... -- Regards, Laurent Pinchart
[PATCH 1/2] drm/msm/gpu: Fix idle_work time
From: Rob Clark This was supposed to be a relative timer, not absolute. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_gpu_devfreq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c index 43468919df61..7285041c737e 100644 --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c @@ -228,5 +228,5 @@ void msm_devfreq_idle(struct msm_gpu *gpu) struct msm_gpu_devfreq *df = >devfreq; msm_hrtimer_queue_work(>idle_work, ms_to_ktime(1), - HRTIMER_MODE_ABS); + HRTIMER_MODE_REL); } -- 2.33.1
[PATCH 2/2] drm/msm/gpu: Respect PM QoS constraints
From: Rob Clark
Re-work the boost and idle clamping to use PM QoS requests instead, so
they get aggreggated with other requests (such as cooling device).
This does have the minor side-effect that devfreq sysfs min_freq/
max_freq files now reflect the boost and idle clamping, as they show
(despite what they are documented to show) the aggregated min/max freq.
Fixing that in devfreq does not look straightforward after considering
that OPPs can be dynamically added/removed. However writes to the
sysfs files still behave as expected.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gpu.h | 33 +++
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 82 ++-
2 files changed, 66 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 59cdd00b69d0..96d8d37dd5b7 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -87,6 +87,21 @@ struct msm_gpu_devfreq {
/** devfreq: devfreq instance */
struct devfreq *devfreq;
+ /**
+* idle_constraint:
+*
+* A PM QoS constraint to limit max freq while the GPU is idle.
+*/
+ struct dev_pm_qos_request idle_freq;
+
+ /**
+* boost_constraint:
+*
+* A PM QoS constraint to boost min freq for a period of time
+* until the boost expires.
+*/
+ struct dev_pm_qos_request boost_freq;
+
/**
* busy_cycles:
*
@@ -103,22 +118,19 @@ struct msm_gpu_devfreq {
ktime_t idle_time;
/**
-* idle_freq:
+* idle_work:
*
-* Shadow frequency used while the GPU is idle. From the PoV of
-* the devfreq governor, we are continuing to sample busyness and
-* adjust frequency while the GPU is idle, but we use this shadow
-* value as the GPU is actually clamped to minimum frequency while
-* it is inactive.
+* Used to delay clamping to idle freq on active->idle transition.
*/
- unsigned long idle_freq;
+ struct msm_hrtimer_work idle_work;
/**
-* idle_work:
+* boost_work:
*
-* Used to delay clamping to idle freq on active->idle transition.
+* Used to reset the boost_constraint after the boost period has
+* elapsed
*/
- struct msm_hrtimer_work idle_work;
+ struct msm_hrtimer_work boost_work;
};
struct msm_gpu {
@@ -498,6 +510,7 @@ void msm_devfreq_init(struct msm_gpu *gpu);
void msm_devfreq_cleanup(struct msm_gpu *gpu);
void msm_devfreq_resume(struct msm_gpu *gpu);
void msm_devfreq_suspend(struct msm_gpu *gpu);
+void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor);
void msm_devfreq_active(struct msm_gpu *gpu);
void msm_devfreq_idle(struct msm_gpu *gpu);
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index 7285041c737e..ff668e431cee 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -9,6 +9,7 @@
#include
#include
+#include
/*
* Power Management:
@@ -22,15 +23,6 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
opp = devfreq_recommended_opp(dev, freq, flags);
- /*
-* If the GPU is idle, devfreq is not aware, so just ignore
-* it's requests
-*/
- if (gpu->devfreq.idle_freq) {
- gpu->devfreq.idle_freq = *freq;
- return 0;
- }
-
if (IS_ERR(opp))
return PTR_ERR(opp);
@@ -48,9 +40,6 @@ static int msm_devfreq_target(struct device *dev, unsigned
long *freq,
static unsigned long get_freq(struct msm_gpu *gpu)
{
- if (gpu->devfreq.idle_freq)
- return gpu->devfreq.idle_freq;
-
if (gpu->funcs->gpu_get_freq)
return gpu->funcs->gpu_get_freq(gpu);
@@ -88,6 +77,7 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
.get_cur_freq = msm_devfreq_get_cur_freq,
};
+static void msm_devfreq_boost_work(struct kthread_work *work);
static void msm_devfreq_idle_work(struct kthread_work *work);
void msm_devfreq_init(struct msm_gpu *gpu)
@@ -98,6 +88,12 @@ void msm_devfreq_init(struct msm_gpu *gpu)
if (!gpu->funcs->gpu_busy)
return;
+ dev_pm_qos_add_request(>pdev->dev, >idle_freq,
+ DEV_PM_QOS_MAX_FREQUENCY,
+ PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
+ dev_pm_qos_add_request(>pdev->dev, >boost_freq,
+ DEV_PM_QOS_MIN_FREQUENCY, 0);
+
msm_devfreq_profile.initial_freq = gpu->fast_rate;
/*
@@ -128,13 +124,19 @@ void msm_devfreq_init(struct msm_gpu *gpu)
gpu->cooling = NULL;
}
+ msm_hrtimer_work_init(>boost_work, gpu->worker,
msm_devfreq_boost_work,
+ CLOCK_MONOTONIC,
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On Fri, Nov 19, 2021 at 9:21 AM Jason Baron wrote: > > > > On 11/18/21 10:24 AM, Pekka Paalanen wrote: > > On Thu, 18 Nov 2021 09:29:27 -0500 > > Jason Baron wrote: > > > >> On 11/16/21 3:46 AM, Pekka Paalanen wrote: > >>> On Fri, 12 Nov 2021 10:08:41 -0500 > >>> Jason Baron wrote: > >>> > On 11/12/21 6:49 AM, Vincent Whitchurch wrote: > > On Thu, Nov 11, 2021 at 03:02:04PM -0700, Jim Cromie wrote: > >> Sean Paul proposed, in: > >> https://urldefense.com/v3/__https://patchwork.freedesktop.org/series/78133/__;!!GjvTz_vk!HcKnMRByYkIdyF1apqQjlN5aBIomzJR1an3YWXM6KXs0EftVMQdrewRA8Dki4A$ > >> drm/trace: Mirror DRM debug logs to tracefs > >> > >> His patchset's objective is to be able to independently steer some of > >> the drm.debug stream to an alternate tracing destination, by splitting > >> drm_debug_enabled() into syslog & trace flavors, and enabling them > >> separately. 2 advantages were identified: > >> > >> 1- syslog is heavyweight, tracefs is much lighter > >> 2- separate selection of enabled categories means less traffic > >> > >> Dynamic-Debug can do 2nd exceedingly well: > >> > >> A- all work is behind jump-label's NOOP, zero off cost. > >> B- exact site selectivity, precisely the useful traffic. > >>can tailor enabled set interactively, at shell. > >> > >> Since the tracefs interface is effective for drm (the threads suggest > >> so), adding that interface to dynamic-debug has real potential for > >> everyone including drm. > >> > >> if CONFIG_TRACING: > >> > >> Grab Sean's trace_init/cleanup code, use it to provide tracefs > >> available by default to all pr_debugs. This will likely need some > >> further per-module treatment; perhaps something reflecting hierarchy > >> of module,file,function,line, maybe with a tuned flattening. > >> > >> endif CONFIG_TRACING > >> > >> Add a new +T flag to enable tracing, independent of +p, and add and > >> use 3 macros: dyndbg_site_is_enabled/logging/tracing(), to encapsulate > >> the flag checks. Existing code treats T like other flags. > > > > I posted a patchset a while ago to do something very similar, but that > > got stalled for some reason and I unfortunately didn't follow it up: > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/lkml/20200825153338.17061-1-vincent.whitchu...@axis.com/__;!!GjvTz_vk!HcKnMRByYkIdyF1apqQjlN5aBIomzJR1an3YWXM6KXs0EftVMQdrewRGytKHPg$ > > > > A key difference between that patchset and this patch (besides that > > small fact that I used +x instead of +T) was that my patchset allowed > > the dyndbg trace to be emitted to the main buffer and did not force them > > to be in an instance-specific buffer. > > Yes, I agree I'd prefer that we print here to the 'main' buffer - it > seems to keep things simpler and easier to combine the output from > different sources as you mentioned. > >>> > >>> Hi, > >>> > >>> I'm not quite sure I understand this discussion, but I would like to > >>> remind you all of what Sean's original work is about: > >>> > >>> Userspace configures DRM tracing into a flight recorder buffer (I guess > >>> this is what you refer to "instance-specific buffer"). > >>> > >>> Userspace runs happily for months, and then hits a problem: a failure > >>> in the DRM sub-system most likely, e.g. an ioctl that should never > >>> fail, failed. Userspace handles that failure by dumping the flight > >>> recorder buffer into a file and saving or sending a bug report. The > >>> flight recorder contents give a log of all relevant DRM in-kernel > >>> actions leading to the unexpected failure to help developers debug it. > >>> > >>> I don't mind if one can additionally send the flight recorder stream to > >>> the main buffer, but I do want the separate flight recorder buffer to > >>> be an option so that a) unrelated things cannot flood the interesting > >>> bits out of it, and b) the scope of collected information is relevant. > >>> > >>> The very reason for this work is problems that are very difficult to > >>> reproduce in practice, either because the problem itself is triggered > >>> very rarely and randomly, or because the end users of the system have > >>> either no knowledge or no access to reconfigure debug logging and then > >>> reproduce the problem with good debug logs. > >>> > >>> Thank you very much for pushing this work forward! > >>> > >>> > >> > >> So I think Vincent (earlier in the thread) was saying that he finds it > >> very helpful have dynamic debug output go to the 'main' trace buffer, > >> while you seem to be saying you'd prefer it just go to dynamic debug > >> specific trace buffer. > > > > Seems like we have different use cases: traditional debugging, and > > in-production flight recorder for problem reporting. I'm not surprised > > if they need different treatment. > > > >> So we
Re: [PATCH 1/2] dt-bindings: display: Turn lvds.yaml into a generic schema
Hi Maxime,
Thank you for the patch.
On Tue, Nov 16, 2021 at 03:35:02PM +0100, Maxime Ripard wrote:
> The lvds.yaml file so far was both defining the generic LVDS properties
> (such as data-mapping) that could be used for any LVDS sink, but also
> the panel-lvds binding.
>
> That last binding was to describe LVDS panels simple enough, and had a
> number of other bindings using it as a base to specialise it further.
>
> However, this situation makes it fairly hard to extend and reuse both
> the generic parts, and the panel-lvds itself.
>
> Let's remove the panel-lvds parts and leave only the generic LVDS
> properties.
>
> Signed-off-by: Maxime Ripard
> ---
> .../display/panel/advantech,idk-1110wr.yaml | 17 ++-
> .../display/panel/innolux,ee101ia-01d.yaml| 21 +-
> .../bindings/display/panel/lvds.yaml | 29 +--
> .../display/panel/mitsubishi,aa104xd12.yaml | 17 ++-
> .../display/panel/mitsubishi,aa121td01.yaml | 17 ++-
> .../display/panel/sgd,gktw70sdae4se.yaml | 17 ++-
> 6 files changed, 85 insertions(+), 33 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/panel/advantech,idk-1110wr.yaml
> b/Documentation/devicetree/bindings/display/panel/advantech,idk-1110wr.yaml
> index 93878c2cd370..f27cd2038636 100644
> ---
> a/Documentation/devicetree/bindings/display/panel/advantech,idk-1110wr.yaml
> +++
> b/Documentation/devicetree/bindings/display/panel/advantech,idk-1110wr.yaml
> @@ -11,13 +11,23 @@ maintainers:
>- Thierry Reding
>
> allOf:
> + - $ref: panel-common.yaml#
>- $ref: lvds.yaml#
>
> +select:
> + properties:
> +compatible:
> + contains:
> +const: advantech,idk-1110wr
> +
> + required:
> +- compatible
I've never encountered this before, what does it do ?
> +
> properties:
>compatible:
> items:
>- const: advantech,idk-1110wr
> - - {} # panel-lvds, but not listed here to avoid false select
> + - const: panel-lvds
>
>data-mapping:
> const: jeida-24
> @@ -35,6 +45,11 @@ additionalProperties: false
>
> required:
>- compatible
> + - data-mapping
> + - width-mm
> + - height-mm
> + - panel-timing
> + - port
>
> examples:
>- |+
> diff --git
> a/Documentation/devicetree/bindings/display/panel/innolux,ee101ia-01d.yaml
> b/Documentation/devicetree/bindings/display/panel/innolux,ee101ia-01d.yaml
> index a69681e724cb..6e06eecac14e 100644
> --- a/Documentation/devicetree/bindings/display/panel/innolux,ee101ia-01d.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/innolux,ee101ia-01d.yaml
> @@ -11,15 +11,26 @@ maintainers:
>- Thierry Reding
>
> allOf:
> + - $ref: panel-common.yaml#
>- $ref: lvds.yaml#
>
> +select:
> + properties:
> +compatible:
> + contains:
> +const: innolux,ee101ia-01d
> +
> + required:
> +- compatible
> +
> properties:
>compatible:
> items:
>- const: innolux,ee101ia-01d
> - - {} # panel-lvds, but not listed here to avoid false select
> + - const: panel-lvds
>
>backlight: true
> + data-mapping: true
>enable-gpios: true
>power-supply: true
>width-mm: true
> @@ -27,5 +38,13 @@ properties:
>panel-timing: true
>port: true
>
> +required:
> + - compatible
> + - data-mapping
> + - width-mm
> + - height-mm
> + - panel-timing
> + - port
> +
> additionalProperties: false
> ...
> diff --git a/Documentation/devicetree/bindings/display/panel/lvds.yaml
> b/Documentation/devicetree/bindings/display/panel/lvds.yaml
> index 49460c9dceea..5281a75c8bb5 100644
> --- a/Documentation/devicetree/bindings/display/panel/lvds.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/lvds.yaml
> @@ -4,7 +4,7 @@
> $id: http://devicetree.org/schemas/display/panel/lvds.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: LVDS Display Panel
> +title: LVDS Display Common Properties
Maybe
title: LVDS Display Panel Common Properties
or do you foresee this being useful for non-panel LBDS sinks too ? In
that case the title is fine, but the file could be moved in the parent
directory.
I'm also wondering what we should do with the data-mapping and
data-mirror properties. For an LVDS panel they're fine at the device
level, but for an LVDS sink, they may be better placed at the port or
endpoint level.
>
> maintainers:
>- Laurent Pinchart
> @@ -26,18 +26,7 @@ description: |+
>Device compatible with those specifications have been marketed under the
>FPD-Link and FlatLink brands.
>
> -allOf:
> - - $ref: panel-common.yaml#
> -
> properties:
> - compatible:
> -contains:
> - const: panel-lvds
> -description:
> - Shall contain "panel-lvds" in addition to a mandatory panel-specific
> - compatible string defined in individual panel bindings. The
> "panel-lvds"
> - value shall never be used on its own.
> -
>data-mapping:
>
Re: [git pull] drm fixes for 5.16-rc2
The pull request you sent on Fri, 19 Nov 2021 15:32:38 +1000: > git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2021-11-19 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/ad44518affc66611644654cec9c165eb4e848030 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi Daniel, On Fri, Nov 19, 2021 at 11:01:18AM +0100, Daniel Vetter wrote: > On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote: > > On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > > > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > > > --- a/drivers/gpu/drm/Kconfig > > > > +++ b/drivers/gpu/drm/Kconfig > > > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > > > > > If in doubt, say "N". > > > > > > > > +config DRM_INPUT_HELPER > > > > + def_bool y > > > > + depends on DRM_KMS_HELPER > > > > + depends on INPUT > > > > > > Uh please no configs for each thing, it just makes everything more > > > complex. Do we _really_ need this? > > > > First, it's not a configurable option (a user will never see this nor > > have to answer Y/N to it); it only serves as an intermediary to express > > the CONFIG_INPUT dependency (which is necessary) without making > > DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run > > display stacks without the input subsystem.) > > I'm not so much worried about the user cost, but the maintenance cost. > Kbuild config complexity is ridiculous, anything that adds even a bit is > really silly. > > > The closest alternative I can think of with fewer Kconfig symbols is to > > just use CONFIG_INPUT directly in the code, to decide whether to provide > > the helpers or else just stub them out. But that has a problem of not > > properly expressing the =m vs. =y necessity: if, for example, > > CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker > > issues. > > Usually this is done by providing static inline dummy implementations in > the headers. That avoids having to sprinkle new Kconfig symbols all over. Right, I already did that, and I'm not sprinkling CONFIG_DRM_INPUT_HELPER much. (I do include one around the module parameter, because it doesn't make much sense to have the module parameter even exist, if the underlying feature is stubbed out.) But that doesn't solve the problem in my last sentence, involving tristates. The "stub inline" approach only works well for boolean features -- either built-in, or disabled. Once your feature is in a module, you need to ensure that no built-in code depends on it. Do you want DRM_KMS_HELPER to unconditionally depend on CONFIG_INPUT? If so, I can just add a 'select' or 'depend' and drop this intermediate symbol. If not, then what do you expect to happen with DRM_KMS_HELPER=y and CONFIG_INPUT=m? Brian
Re: [PATCH v3 11/12] drm/virtio: implement context init: add virtio_gpu_fence_event
On Thu, Nov 18, 2021 at 12:53 AM Daniel Vetter wrote: > > On Tue, Nov 16, 2021 at 06:31:10PM -0800, Gurchetan Singh wrote: > > On Tue, Nov 16, 2021 at 7:43 AM Daniel Vetter wrote: > > > > > On Mon, Nov 15, 2021 at 07:26:14PM +, Kasireddy, Vivek wrote: > > > > Hi Daniel, Greg, > > > > > > > > If it is the same or a similar crash reported here: > > > > > > > https://lists.freedesktop.org/archives/dri-devel/2021-November/330018.html > > > > and here: > > > https://lists.freedesktop.org/archives/dri-devel/2021-November/330212.html > > > > then the fix is already merged: > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d89c0c8322ecdc9a2ec84b959b6f766be082da76 > > > > Yeah but that still leaves the problem of why exaxtly virtgpu is > > > reinventing drm_poll here? > > > > > > > Can you please replace it with drm_poll like all other drivers, including > > > the ones that have private events? > > > > > > > Hi Daniel, > > > > Allow me to explain the use case a bit. It's for when virtgpu KMS is not > > used, but a special Wayland compositor does wayland passthrough instead: > > > > https://www.youtube.com/watch?v=WwrXqDERFm8https://www.youtube.com/watch?v=EkNBsBx501Q > > > > This technique has gained much popularity in the virtualized laptop > > space, where it offers better performance/user experience than virtgpu > > KMS. The relevant paravirtualized userspace is "Sommelier": > > > > https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/sommelier/ > > https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/sommelier/virtualization/virtgpu_channel.cc > > > > Previously, we were using the out-of-tree virtio-wl device and there > > were many discussions on how we could get this upstream: > > > > https://lists.freedesktop.org/archives/dri-devel/2017-December/160309.html > > https://lists.oasis-open.org/archives/virtio-dev/202002/msg5.html > > > > Extending virtgpu was deemed the least intrusive option: > > > > https://www.spinics.net/lists/kvm/msg159206.html > > > > We ultimately settled on the context type abstraction and used > > virtio_gpu_poll to tell the guest "hey, we have a Wayland event". The > > host response is actually in a buffer of type BLOB_MEM_GUEST. > > > > It is possible to use drm_poll(..), but that would have to be > > accompanied by a drm_read(..). You'll need to define a dummy > > VIRTGPU_EVENT_FENCE_SIGNALED in the uapi too. > > > > That's originally how I did it, but some pointed out that's > > unnecessary since the host response is in the BLOB_MEM_GUEST buffer > > and virtgpu event is a dummy event. So we decided just to modify > > virtio_gpu_poll(..) to have the desired semantics in that case. > > > > For the regular virtio-gpu KMS path, things remain unchanged. > > > > There are of course other ways to do it (perhaps polling a dma_fence), > > but that was the cleanest way we could find. > > > > It's not rare for virtio to "special things" (see virtio_dma_buf_ops, > > virtio_dma_ops), since they are in fake devices. > > These are all internal interfaces, not uapi. > > > We're open to other ideas, but hopefully that answers some of your > > questions. > > Well for one, why does the commit message not explain any of this. You're > building uapi, which is forever, it's paramount all considerations are > properly explained. > > Second, I really don't like that youre redefining poll semantics in > incompatible ways from all other drm drivers. If you want special poll > semantics then just create a sperate fd for that (or a dma_fence or > whatever, maybe that saves some typing), but bending the drm fd semantics > is no good at all. We have tons of different fd with their dedicated > semantics in drm, trying to shoehorn it all into one just isn't very good > design. > > Or do the dummy event which is just the event code, but does not contain > any data. Either is fine with me. > > Can you pls do this asap? I really don't want to bake this in as uapi > which we then have to quirk and support forever. I'd say revert for -rc2 > of these two and then maybe sort it out properly in -next. I think as a general rule, if there is not consensus about uabi change, even if it is just a semantic change, then revert and reland later is ok.. As far as dummy VIRTGPU_EVENT_FENCE_SIGNALED.. that doesn't entirely sound like a bad thing to me. Actually, it sounds like a good thing.. it makes it more explicit what is going on. And would avoid confusing a different userspace polling for kms related events expecting to be able to do a read. BR, -R > Cheers, Daniel > > > > > > > Thanks, Daniel > > > > > > > > > > > Thanks, > > > > Vivek > > > > > > > > > On Sat, Nov 13, 2021 at 03:51:48PM +0100, Greg KH wrote: > > > > > > On Tue, Sep 21, 2021 at 04:20:23PM -0700, Gurchetan Singh wrote: > > > > > > > Similar to DRM_VMW_EVENT_FENCE_SIGNALED. Sends a pollable event > > > > > > > to the DRM file descriptor when a fence on a
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Rename gt to gt0
On Wed, Nov 17, 2021 at 02:16:13PM +, Chris Wilson wrote:
Quoting Andi Shyti (2021-11-17 13:34:56)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index a9727447c0379..4bfedc04f5c70 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -936,7 +936,7 @@ hsw_gt_workarounds_init(struct intel_gt *gt, struct
i915_wa_list *wal)
static void
gen9_wa_init_mcr(struct drm_i915_private *i915, struct i915_wa_list *wal)
{
- const struct sseu_dev_info *sseu = >gt.info.sseu;
+ const struct sseu_dev_info *sseu = >gt0.info.sseu;
This feels like it should be pulling from uncore->gt, since the MCR is
across an uncore.
Overall though, rather than introduce bare >gt0, how about pulling in
the patch for to_gt(i915)?
agreed, why are we going a different route rather than using the to_gt()
patches Michal prepared? If the to_gt() is not what we want, then please
sync with Michal on the proper direction
Lucas De Marchi
Re: [PATCH v4 6/6] drm/i915/ttm: Update i915_gem_obj_copy_ttm() to be asynchronous
On 18/11/2021 13:02, Thomas Hellström wrote: Update the copy function i915_gem_obj_copy_ttm() to be asynchronous for future users and update the only current user to sync the objects as needed after this function. Signed-off-by: Thomas Hellström Reviewed-by: Matthew Auld
Re: [PATCH v4 5/6] drm/i915/ttm: Implement asynchronous TTM moves
On 18/11/2021 13:02, Thomas Hellström wrote:
Don't wait sync while migrating, but rather make the GPU blit await the
dependencies and add a moving fence to the object.
This also enables asynchronous VRAM management in that on eviction,
rather than waiting for the moving fence to expire before freeing VRAM,
it is freed immediately and the fence is stored with the VRAM manager and
handed out to newly allocated objects to await before clears and swapins,
or for kernel objects before setting up gpu vmas or mapping.
To collect dependencies before migrating, add a set of utilities that
coalesce these to a single dma_fence.
What is still missing for fully asynchronous operation is asynchronous vma
unbinding, which is still to be implemented.
This commit substantially reduces execution time in the gem_lmem_swapping
test.
v2:
- Make a couple of functions static.
v4:
- Fix some style issues (Matthew Auld)
- Audit and add more checks for ghost objects (Matthew Auld)
- Add more documentation for the i915_deps utility (Mattew Auld)
- Simplify the i915_deps_sync() function
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 32 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 338 +--
drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +-
4 files changed, 344 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index e37157b080e4..81e84c1763de 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -248,10 +248,13 @@ static struct ttm_tt *i915_ttm_tt_create(struct
ttm_buffer_object *bo,
struct ttm_resource_manager *man =
ttm_manager_type(bo->bdev, bo->resource->mem_type);
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- enum ttm_caching caching = i915_ttm_select_tt_caching(obj);
+ enum ttm_caching caching;
struct i915_ttm_tt *i915_tt;
int ret;
+ if (!obj)
+ return NULL;
+
i915_tt = kzalloc(sizeof(*i915_tt), GFP_KERNEL);
if (!i915_tt)
return NULL;
@@ -260,6 +263,7 @@ static struct ttm_tt *i915_ttm_tt_create(struct
ttm_buffer_object *bo,
man->use_tt)
page_flags |= TTM_TT_FLAG_ZERO_ALLOC;
+ caching = i915_ttm_select_tt_caching(obj);
if (i915_gem_object_is_shrinkable(obj) && caching == ttm_cached) {
page_flags |= TTM_TT_FLAG_EXTERNAL |
TTM_TT_FLAG_EXTERNAL_MAPPABLE;
@@ -326,6 +330,9 @@ static bool i915_ttm_eviction_valuable(struct
ttm_buffer_object *bo,
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
+ if (!obj)
+ return false;
+
/*
* EXTERNAL objects should never be swapped out by TTM, instead we need
* to handle that ourselves. TTM will already skip such objects for us,
@@ -552,8 +559,12 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
static void i915_ttm_swap_notify(struct ttm_buffer_object *bo)
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- int ret = i915_ttm_move_notify(bo);
+ int ret;
+ if (!obj)
+ return;
+
+ ret = i915_ttm_move_notify(bo);
GEM_WARN_ON(ret);
GEM_WARN_ON(obj->ttm.cached_io_rsgt);
if (!ret && obj->mm.madv != I915_MADV_WILLNEED)
@@ -575,17 +586,23 @@ static unsigned long i915_ttm_io_mem_pfn(struct
ttm_buffer_object *bo,
unsigned long page_offset)
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- unsigned long base = obj->mm.region->iomap.base -
obj->mm.region->region.start;
struct scatterlist *sg;
+ unsigned long base;
unsigned int ofs;
+ GEM_BUG_ON(!obj);
GEM_WARN_ON(bo->ttm);
+ base = obj->mm.region->iomap.base - obj->mm.region->region.start;
sg = __i915_gem_object_get_sg(obj, >ttm.get_io_page, page_offset,
, true);
return ((base + sg_dma_address(sg)) >> PAGE_SHIFT) + ofs;
}
+/*
+ * All callbacks need to take care not to downcast a struct ttm_buffer_object
+ * without checking its subclass, since it might be a TTM ghost object.
+ */
static struct ttm_device_funcs i915_ttm_bo_driver = {
.ttm_tt_create = i915_ttm_tt_create,
.ttm_tt_populate = i915_ttm_tt_populate,
@@ -847,13 +864,16 @@ static void i915_ttm_delayed_free(struct
drm_i915_gem_object *obj)
static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
{
struct vm_area_struct *area = vmf->vma;
- struct drm_i915_gem_object *obj =
- i915_ttm_to_gem(area->vm_private_data);
- struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
+ struct ttm_buffer_object *bo = area->vm_private_data;
struct drm_device *dev = bo->base.dev;
+ struct drm_i915_gem_object *obj;
Re: [PATCH v3 5/5] drm/i915/dg2: extend Wa_1409120013 to DG2
On Fri, Nov 19, 2021 at 08:36:56AM -0800, Souza, Jose wrote:
> On Tue, 2021-11-16 at 09:48 -0800, Matt Roper wrote:
> > From: Matt Atwood
> >
> > Extend existing workaround 1409120013 to DG2.
> >
> > Cc: José Roberto de Souza
> > Signed-off-by: Matt Atwood
> > Signed-off-by: Matt Roper
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 89dc7f69baf3..e721c421cc58 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -7444,9 +7444,9 @@ static void icl_init_clock_gating(struct
> > drm_i915_private *dev_priv)
> >
> > static void gen12lp_init_clock_gating(struct drm_i915_private *dev_priv)
> > {
> > - /* Wa_1409120013:tgl,rkl,adl-s,dg1 */
> > + /* Wa_1409120013:tgl,rkl,adl-s,dg1,dg2 */
>
> I'm not finding this workaround in the DG2 WA spec page, maybe it was removed
> because it is not necessary anymore?
Ville raised the same question; I believe this is just an issue with the
query that generates the bspec page from the database; here's my earlier
response:
>> This seems to be problem with the DG2 query since for some
>> reason they marked this workaround as
>> 'driver_change_required' rather than 'driver_permanent_wa' in
>> the database and that prevents it from showing up in some of
>> the queries properly. The DG2-specific ID number to check is
>> 140975.
Since it appears this is currently needed on every version 12 and
version 13 display platform _except_ for ADL-P, I did send a question to
the HW team to confirm that the lack of ADL-P isn't an oversight, but I
haven't heard back yet.
Matt
>
> > if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> > - IS_ALDERLAKE_S(dev_priv) || IS_DG1(dev_priv))
> > + IS_ALDERLAKE_S(dev_priv) || IS_DG1(dev_priv) || IS_DG2(dev_priv))
> > intel_uncore_write(_priv->uncore, ILK_DPFC_CHICKEN,
> >DPFC_CHICKEN_COMP_DUMMY_PIXEL);
> >
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
Re: Sparsely populated TTM bos
On Fri, Nov 19, 2021 at 05:35:53PM +0100, Christian König wrote: > Hi Thomas, > > Am 19.11.21 um 15:28 schrieb Thomas Hellström: > > Hi, Christian, > > > > We have an upcoming use-case in i915 where one solution would be > > sparsely populated TTM bos. > > > > We had that at one point where ttm_tt pages were allocated on demand, > > but this time we'd rather be looking at multiple struct ttm_resources > > per bo and those resources could be from different managers. > > > > There might theoretically be other ways we can handle this use-case but > > I wanted to check with you whether this is something AMD is already > > looking into and if not, your general opinion. > > oh, yes I've looked into this as well a very long time ago. > > At that point the basic blocker was that we couldn't have different cache > setting for the same VMA, but I think that's fixed by now. I think for cpu mmap we might just disallow them. Or we just migrate them back into so that cpu access is always done in the same (or at least a compatible) cache domain. We can't really talk yet about what this thing is for, but "entire ttm_bo cpu mmap must have same caching mode" shouldn't be a real limitation for what we want to do here. > Another thing is that you essentially need to move the LRU handling into the > resource like I already planned to do anyway. Yeah, hence why I suggested going ttm_bo : ttm_resource 1:N might be a good idea in general, and we could piggy-pack on top of this. If you're all on board then I guess we'll try to prototype something and maybe if you're bored we could resurrect some of the patches to move lru/dma_resv and whatever else from ttm_bo to ttm_resource? Just to see how much this would impact. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v4] drm/msm/dp: do not initialize phy until plugin interrupt received
On 11/18/2021 5:20 PM, Stephen Boyd wrote:
Quoting Kuogee Hsieh (2021-11-09 13:38:13)
From: Kuogee Hsieh
Current DP drivers have regulators, clocks, irq and phy are grouped
together within a function and executed not in a symmetric manner.
This increase difficulty of code maintenance and limited code scalability.
This patch divided the driver life cycle of operation into four states,
resume (including booting up), dongle plugin, dongle unplugged and suspend.
Regulators, core clocks and irq are grouped together and enabled at resume
(or booting up) so that the DP controller is armed and ready to receive HPD
plugin interrupts. HPD plugin interrupt is generated when a dongle plugs
into DUT (device under test). Once HPD plugin interrupt is received, DP
controller will initialize phy so that dpcd read/write will function and
following link training can be proceeded successfully. DP phy will be
disabled after main link is teared down at end of unplugged HPD interrupt
handle triggered by dongle unplugged out of DUT. Finally regulators, code
clocks and irq are disabled at corresponding suspension.
Changes in V2:
-- removed unnecessary dp_ctrl NULL check
-- removed unnecessary phy init_count and power_count DRM_DEBUG_DP logs
-- remove flip parameter out of dp_ctrl_irq_enable()
-- add fixes tag
Changes in V3:
-- call dp_display_host_phy_init() instead of dp_ctrl_phy_init() at
dp_display_host_init() for eDP
Changes in V4:
-- rewording commit text to match this commit changes
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
---
What commit is this patch based on?
It base on Bjorn's patch,
https://patchwork.freedesktop.org/series/92992/
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi, On Fri, Nov 19, 2021 at 8:39 AM Rob Clark wrote: > > On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen wrote: > > > > On Thu, 18 Nov 2021 15:30:38 -0800 > > Rob Clark wrote: > > > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen > > > wrote: > > > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > > Brian Norris wrote: > > > > > > > > > A variety of applications have found it useful to listen to > > > > > user-initiated input events to make decisions within a DRM driver, > > > > > given > > > > > that input events are often the first sign that we're going to start > > > > > doing latency-sensitive activities: > > > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > >Rockchip eDP) is especially latency sensitive. In some cases, it > > > > > can > > > > >take 10s of milliseconds for a panel to exit self-refresh, which > > > > > can > > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always > > > > > shipped > > > > >with an input_handler boost, that preemptively exits self-refresh > > > > >whenever there is input activity. > > > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > >render new frames immediately after user activity. Powering up the > > > > >GPU can take enough time that it is worthwhile to start this > > > > > process > > > > >as soon as there is input activity. Many Chrome OS systems also > > > > > ship > > > > >with an input_handler boost that powers up the GPU. > > > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > > input-subsystem details around picking which devices to listen to, and > > > > > some other boilerplate. This will be used in the next patch to > > > > > implement > > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS > > > > > kernels > > > > > have been carrying for a while. > > > > > > > > > > Signed-off-by: Brian Norris > > > > > --- > > > > > > > > Thanks Simon for the CC. > > > > > > > > Hi Brian, > > > > > > > > while this feature in general makes sense and sounds good, to start > > > > warming up display hardware early when something might start to happen, > > > > this particular proposal has many problems from UAPI perspective (as it > > > > has none). Comments below. > > > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > > from this input event watching? I would imagine the improvement to not > > > > be noticeable. > > > > > > > > I think some numbers about how much this feature helps would be really > > > > good, even if they are quite specific use cases. You also need to > > > > identify the userspace components, because I think different display > > > > servers are very different in their reaction speed. > > > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > > there might be a delay worth avoiding. But then, is it worth > > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > > userspace could hit to start the warming up process? > > > > > > In my measurements, it takes userspace a frame or two to respond and > > > get to the point of starting to build cmdstream (before eventually > > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > > a similar boost mechanism to spool up cpufreq > > > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > > cumulation of a long sequence of events.. input-boost is letting > > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > > that long sequence. > > > > Right, exactly. That is why I was musing about a *new* ioctl that > > userspace could hit as soon as any input device fd (or network fd!) > > shows signs of life. Would that be enough, avoiding all the annoying > > questions about which input and DRM devices should participate here > > (and what about non-input devices that still want to trigger the > > warm-up, like network traffic, e.g. remote control?), or does it really > > need to be kernel internal to be fast enough? > > > > As Brian wrote about his quick hack to test that via debugfs, sounds > > like the userspace solution would be totally sufficient. > > > > The question is, I think we want to use this for at least a couple > different things.. PSR exit, and/or early GPU wakeup/boost. So I'm > not quite sure where/what this ioctl should be. Different drivers may > have different uses. Also, because of the CrOS userspace sandbox > architecture, the place that could do a kms ioctl isn't really the > place that would know when to call the ioctl. > > But if we want to move the policy out of the kernel, one approach > would just be to have some sysfs where userspace could configure the >
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen wrote: > > On Thu, 18 Nov 2021 15:30:38 -0800 > Rob Clark wrote: > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > >with an input_handler boost, that preemptively exits self-refresh > > > >whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > >render new frames immediately after user activity. Powering up the > > > >GPU can take enough time that it is worthwhile to start this process > > > >as soon as there is input activity. Many Chrome OS systems also ship > > > >with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > In my measurements, it takes userspace a frame or two to respond and > > get to the point of starting to build cmdstream (before eventually > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > a similar boost mechanism to spool up cpufreq > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > cumulation of a long sequence of events.. input-boost is letting > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > that long sequence. > > Right, exactly. That is why I was musing about a *new* ioctl that > userspace could hit as soon as any input device fd (or network fd!) > shows signs of life. Would that be enough, avoiding all the annoying > questions about which input and DRM devices should participate here > (and what about non-input devices that still want to trigger the > warm-up, like network traffic, e.g. remote control?), or does it really > need to be kernel internal to be fast enough? > > As Brian wrote about his quick hack to test that via debugfs, sounds > like the userspace solution would be totally sufficient. > The question is, I think we want to use this for at least a couple different things.. PSR exit, and/or early GPU wakeup/boost. So I'm not quite sure where/what this ioctl should be. Different drivers may have different uses. Also, because of the CrOS userspace sandbox architecture, the place that could do a kms ioctl isn't really the place that would know when to call the ioctl. But if we want to move the policy out of the kernel, one approach would just be to have some sysfs where userspace could configure the input_device_id[] filter.. BR, -R
Re: [PATCH v3 5/5] drm/i915/dg2: extend Wa_1409120013 to DG2
On Tue, 2021-11-16 at 09:48 -0800, Matt Roper wrote:
> From: Matt Atwood
>
> Extend existing workaround 1409120013 to DG2.
>
> Cc: José Roberto de Souza
> Signed-off-by: Matt Atwood
> Signed-off-by: Matt Roper
> ---
> drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 89dc7f69baf3..e721c421cc58 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7444,9 +7444,9 @@ static void icl_init_clock_gating(struct
> drm_i915_private *dev_priv)
>
> static void gen12lp_init_clock_gating(struct drm_i915_private *dev_priv)
> {
> - /* Wa_1409120013:tgl,rkl,adl-s,dg1 */
> + /* Wa_1409120013:tgl,rkl,adl-s,dg1,dg2 */
I'm not finding this workaround in the DG2 WA spec page, maybe it was removed
because it is not necessary anymore?
> if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
> - IS_ALDERLAKE_S(dev_priv) || IS_DG1(dev_priv))
> + IS_ALDERLAKE_S(dev_priv) || IS_DG1(dev_priv) || IS_DG2(dev_priv))
> intel_uncore_write(_priv->uncore, ILK_DPFC_CHICKEN,
> DPFC_CHICKEN_COMP_DUMMY_PIXEL);
>
Re: Sparsely populated TTM bos
Hi Thomas, Am 19.11.21 um 15:28 schrieb Thomas Hellström: Hi, Christian, We have an upcoming use-case in i915 where one solution would be sparsely populated TTM bos. We had that at one point where ttm_tt pages were allocated on demand, but this time we'd rather be looking at multiple struct ttm_resources per bo and those resources could be from different managers. There might theoretically be other ways we can handle this use-case but I wanted to check with you whether this is something AMD is already looking into and if not, your general opinion. oh, yes I've looked into this as well a very long time ago. At that point the basic blocker was that we couldn't have different cache setting for the same VMA, but I think that's fixed by now. Another thing is that you essentially need to move the LRU handling into the resource like I already planned to do anyway. Regards, Christian. Thanks, Thomas
Re: [PATCH v10 08/10] dyndbg: add print-to-tracefs, selftest with it - RFC
On 11/18/21 10:24 AM, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 09:29:27 -0500 > Jason Baron wrote: > >> On 11/16/21 3:46 AM, Pekka Paalanen wrote: >>> On Fri, 12 Nov 2021 10:08:41 -0500 >>> Jason Baron wrote: >>> On 11/12/21 6:49 AM, Vincent Whitchurch wrote: > On Thu, Nov 11, 2021 at 03:02:04PM -0700, Jim Cromie wrote: >> Sean Paul proposed, in: >> https://urldefense.com/v3/__https://patchwork.freedesktop.org/series/78133/__;!!GjvTz_vk!HcKnMRByYkIdyF1apqQjlN5aBIomzJR1an3YWXM6KXs0EftVMQdrewRA8Dki4A$ >> >> drm/trace: Mirror DRM debug logs to tracefs >> >> His patchset's objective is to be able to independently steer some of >> the drm.debug stream to an alternate tracing destination, by splitting >> drm_debug_enabled() into syslog & trace flavors, and enabling them >> separately. 2 advantages were identified: >> >> 1- syslog is heavyweight, tracefs is much lighter >> 2- separate selection of enabled categories means less traffic >> >> Dynamic-Debug can do 2nd exceedingly well: >> >> A- all work is behind jump-label's NOOP, zero off cost. >> B- exact site selectivity, precisely the useful traffic. >>can tailor enabled set interactively, at shell. >> >> Since the tracefs interface is effective for drm (the threads suggest >> so), adding that interface to dynamic-debug has real potential for >> everyone including drm. >> >> if CONFIG_TRACING: >> >> Grab Sean's trace_init/cleanup code, use it to provide tracefs >> available by default to all pr_debugs. This will likely need some >> further per-module treatment; perhaps something reflecting hierarchy >> of module,file,function,line, maybe with a tuned flattening. >> >> endif CONFIG_TRACING >> >> Add a new +T flag to enable tracing, independent of +p, and add and >> use 3 macros: dyndbg_site_is_enabled/logging/tracing(), to encapsulate >> the flag checks. Existing code treats T like other flags. > > I posted a patchset a while ago to do something very similar, but that > got stalled for some reason and I unfortunately didn't follow it up: > > > https://urldefense.com/v3/__https://lore.kernel.org/lkml/20200825153338.17061-1-vincent.whitchu...@axis.com/__;!!GjvTz_vk!HcKnMRByYkIdyF1apqQjlN5aBIomzJR1an3YWXM6KXs0EftVMQdrewRGytKHPg$ > > > A key difference between that patchset and this patch (besides that > small fact that I used +x instead of +T) was that my patchset allowed > the dyndbg trace to be emitted to the main buffer and did not force them > to be in an instance-specific buffer. Yes, I agree I'd prefer that we print here to the 'main' buffer - it seems to keep things simpler and easier to combine the output from different sources as you mentioned. >>> >>> Hi, >>> >>> I'm not quite sure I understand this discussion, but I would like to >>> remind you all of what Sean's original work is about: >>> >>> Userspace configures DRM tracing into a flight recorder buffer (I guess >>> this is what you refer to "instance-specific buffer"). >>> >>> Userspace runs happily for months, and then hits a problem: a failure >>> in the DRM sub-system most likely, e.g. an ioctl that should never >>> fail, failed. Userspace handles that failure by dumping the flight >>> recorder buffer into a file and saving or sending a bug report. The >>> flight recorder contents give a log of all relevant DRM in-kernel >>> actions leading to the unexpected failure to help developers debug it. >>> >>> I don't mind if one can additionally send the flight recorder stream to >>> the main buffer, but I do want the separate flight recorder buffer to >>> be an option so that a) unrelated things cannot flood the interesting >>> bits out of it, and b) the scope of collected information is relevant. >>> >>> The very reason for this work is problems that are very difficult to >>> reproduce in practice, either because the problem itself is triggered >>> very rarely and randomly, or because the end users of the system have >>> either no knowledge or no access to reconfigure debug logging and then >>> reproduce the problem with good debug logs. >>> >>> Thank you very much for pushing this work forward! >>> >>> >> >> So I think Vincent (earlier in the thread) was saying that he finds it >> very helpful have dynamic debug output go to the 'main' trace buffer, >> while you seem to be saying you'd prefer it just go to dynamic debug >> specific trace buffer. > > Seems like we have different use cases: traditional debugging, and > in-production flight recorder for problem reporting. I'm not surprised > if they need different treatment. > >> So we certainly can have dynamic output potentially go to both places - >> although I think this would mean two tracepoints? But I really wonder >> if we really need a separate tracing buffer for dynamic debug when >>
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 04:04:28PM +, Simon Ser wrote: > On Friday, November 19th, 2021 at 16:53, Daniel Vetter > wrote: > > > Random idea ... should we perhaps let userspace connect the boosting? I.e. > > we do a bunch of standardized boost targets (render clocks, display sr > > exit), and userspace can then connect it to whichever input device it > > wants to? > > On IRC we discussed having user-space hand over a FD to the kernel. When the > FD > becomes readable, the kernel triggers the boost. > > This would let user-space use e.g. an input device, an eventfd, or an epoll FD > with any combination of these as the boost signal. Can userspace filter eventfd appropriately like we do here? And can they get at that maybe 2nd eventfd from logind or whatever there is on distros where /dev access is locked down for compositors/users. I do agree that if we can do this generically maybe we should, but also the use-case for input boosting is pretty well defined. I think it's just about making sure that compositors is in control, and that we don't make it worse (e.g. with the sr exit adding latency when the compositor can redraw quickly enough). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Friday, November 19th, 2021 at 16:53, Daniel Vetter wrote: > Random idea ... should we perhaps let userspace connect the boosting? I.e. > we do a bunch of standardized boost targets (render clocks, display sr > exit), and userspace can then connect it to whichever input device it > wants to? On IRC we discussed having user-space hand over a FD to the kernel. When the FD becomes readable, the kernel triggers the boost. This would let user-space use e.g. an input device, an eventfd, or an epoll FD with any combination of these as the boost signal.
Re: [PATCH] drm/i915: Don't disable interrupts and pretend a lock as been acquired in __timeline_mark_lock().
On Thu, Nov 18, 2021 at 05:59:14PM +0100, Sebastian Andrzej Siewior wrote:
> This is a revert of commits
>d67739268cf0e ("drm/i915/gt: Mark up the nested engine-pm timeline lock as
> irqsafe")
>6c69a45445af9 ("drm/i915/gt: Mark context->active_count as protected by
> timeline->mutex")
>
> The existing code leads to a different behaviour depending on whether
> lockdep is enabled or not. Any following lock that is acquired without
> disabling interrupts (but needs to) will not be noticed by lockdep.
>
> This it not just a lockdep annotation but is used but an actual mutex_t
> that is properly used as a lock but in case of __timeline_mark_lock()
> lockdep is only told that it is acquired but no lock has been acquired.
>
> It appears that its purpose is just satisfy the lockdep_assert_held()
> check in intel_context_mark_active(). The other problem with disabling
> interrupts is that on PREEMPT_RT interrupts are also disabled which
> leads to problems for instance later during memory allocation.
>
> Add a CONTEXT_IS_PARKED bit to intel_engine_cs and set_bit/clear_bit it
> instead of mutex_acquire/mutex_release. Use test_bit in the two
> identified spots which relied on the lockdep annotation.
>
> Cc: Peter Zijlstra
> Signed-off-by: Sebastian Andrzej Siewior
Yeah if we can simplify this with reverts then I'm all for this.
Acked-by: Daniel Vetter
I've asked drm/i915 maintainers to check
-Daniel
> ---
> drivers/gpu/drm/i915/gt/intel_context.h |3 +-
> drivers/gpu/drm/i915/gt/intel_context_types.h |1
> drivers/gpu/drm/i915/gt/intel_engine_pm.c | 38
> +-
> drivers/gpu/drm/i915/i915_request.h |3 +-
> 4 files changed, 7 insertions(+), 38 deletions(-)
>
> --- a/drivers/gpu/drm/i915/gt/intel_context.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context.h
> @@ -211,7 +211,8 @@ static inline void intel_context_enter(s
>
> static inline void intel_context_mark_active(struct intel_context *ce)
> {
> - lockdep_assert_held(>timeline->mutex);
> + lockdep_assert(lockdep_is_held(>timeline->mutex) ||
> +test_bit(CONTEXT_IS_PARKED, >flags));
> ++ce->active_count;
> }
>
> --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> @@ -118,6 +118,7 @@ struct intel_context {
> #define CONTEXT_LRCA_DIRTY 9
> #define CONTEXT_GUC_INIT 10
> #define CONTEXT_PERMA_PIN11
> +#define CONTEXT_IS_PARKED12
>
> struct {
> u64 timeout_us;
> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> @@ -80,39 +80,6 @@ static int __engine_unpark(struct intel_
> return 0;
> }
>
> -#if IS_ENABLED(CONFIG_LOCKDEP)
> -
> -static unsigned long __timeline_mark_lock(struct intel_context *ce)
> -{
> - unsigned long flags;
> -
> - local_irq_save(flags);
> - mutex_acquire(>timeline->mutex.dep_map, 2, 0, _THIS_IP_);
> -
> - return flags;
> -}
> -
> -static void __timeline_mark_unlock(struct intel_context *ce,
> -unsigned long flags)
> -{
> - mutex_release(>timeline->mutex.dep_map, _THIS_IP_);
> - local_irq_restore(flags);
> -}
> -
> -#else
> -
> -static unsigned long __timeline_mark_lock(struct intel_context *ce)
> -{
> - return 0;
> -}
> -
> -static void __timeline_mark_unlock(struct intel_context *ce,
> -unsigned long flags)
> -{
> -}
> -
> -#endif /* !IS_ENABLED(CONFIG_LOCKDEP) */
> -
> static void duration(struct dma_fence *fence, struct dma_fence_cb *cb)
> {
> struct i915_request *rq = to_request(fence);
> @@ -159,7 +126,6 @@ static bool switch_to_kernel_context(str
> {
> struct intel_context *ce = engine->kernel_context;
> struct i915_request *rq;
> - unsigned long flags;
> bool result = true;
>
> /*
> @@ -214,7 +180,7 @@ static bool switch_to_kernel_context(str
>* engine->wakeref.count, we may see the request completion and retire
>* it causing an underflow of the engine->wakeref.
>*/
> - flags = __timeline_mark_lock(ce);
> + set_bit(CONTEXT_IS_PARKED, >flags);
> GEM_BUG_ON(atomic_read(>timeline->active_count) < 0);
>
> rq = __i915_request_create(ce, GFP_NOWAIT);
> @@ -246,7 +212,7 @@ static bool switch_to_kernel_context(str
>
> result = false;
> out_unlock:
> - __timeline_mark_unlock(ce, flags);
> + clear_bit(CONTEXT_IS_PARKED, >flags);
> return result;
> }
>
> --- a/drivers/gpu/drm/i915/i915_request.h
> +++ b/drivers/gpu/drm/i915/i915_request.h
> @@ -642,7 +642,8 @@ i915_request_timeline(const struct i915_
> {
> /* Valid only while the request is being constructed (or retired). */
> return rcu_dereference_protected(rq->timeline,
> -
> lockdep_is_held(_access_pointer(rq->timeline)->mutex));
> +
Re: [PATCH v2 00/13] drm: Add generic helpers for HDMI scrambling
On Thu, Nov 18, 2021 at 11:38:01AM +0100, Maxime Ripard wrote: > Hi, > > This is a follow-up of the work to support the interactions between the > hotplug > and the scrambling support for vc4: > > https://lore.kernel.org/dri-devel/20210507150515.257424-11-max...@cerno.tech/ > https://lore.kernel.org/dri-devel/20211025152903.1088803-10-max...@cerno.tech/ > > Ville feedback was that the same discussion happened some time ago for i915, > and resulted in a function to do an full disable/enable cycle on reconnection > to avoid breaking the HDMI 2.0 spec. > > This series improves the current scrambling support by adding generic helpers > for usual scrambling-related operations, and builds upon them to provide a > generic alternative to intel_hdmi_reset_link. Out of curiosity, can we rebuild intel_hdmi_reset_link on top of these? Always better to have two drivers to actually show the helpers help, than just one. -Daniel > > Let me know what you think, > Maxime > > Changes from v1: > - Dropped the 340MHz define > - Make drm_mode_hdmi_requires_scrambling use the bpc > - Make more drm_display_mode const in vc4 > - Dropped the tegra conversion > - Added more comments > > Maxime Ripard (13): > drm/connector: Add helper to check if a mode requires scrambling > drm/atomic: Add HDMI scrambler state helper > drm/atomic: Add HDMI reset link helper > drm/scdc: Document hotplug gotchas > drm/vc4: hdmi: Constify drm_display_mode > drm/vc4: hdmi: Remove unused argument in vc4_hdmi_supports_scrambling > drm/vc4: hdmi: Remove mutex in detect > drm/vc4: hdmi: Remove HDMI flag from encoder > drm/vc4: hdmi: Simplify the hotplug handling > drm/vc4: hdmi: Simplify the connector state retrieval > drm/vc4: hdmi: Switch to detect_ctx > drm/vc4: hdmi: Leverage new SCDC atomic_check > drm/vc4: hdmi: Reset link on hotplug > > drivers/gpu/drm/drm_atomic_helper.c | 109 + > drivers/gpu/drm/drm_atomic_state_helper.c | 58 + > drivers/gpu/drm/drm_scdc_helper.c | 13 ++ > drivers/gpu/drm/vc4/vc4_hdmi.c| 257 ++ > drivers/gpu/drm/vc4/vc4_hdmi.h| 19 +- > include/drm/drm_atomic_helper.h | 3 + > include/drm/drm_atomic_state_helper.h | 3 + > include/drm/drm_connector.h | 25 +++ > include/drm/drm_modes.h | 20 ++ > 9 files changed, 353 insertions(+), 154 deletions(-) > > -- > 2.33.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm/sun4i: remove no need type conversion to bool
Hi Bernard!
Dne torek, 16. november 2021 ob 03:14:49 CET je Bernard Zhao napisal(a):
> This change is to cleanup the code a bit.
>
> Signed-off-by: Bernard Zhao
Acked-by: Jernej Skrabec
Best regards,
Jernej
> ---
> drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/
sun4i/sun4i_hdmi_tmds_clk.c
> index fbf7da9d9592..25e6f85fed0b 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c
> @@ -49,7 +49,7 @@ static unsigned long sun4i_tmds_calc_divider(unsigned long
rate,
> (rate - tmp_rate) < (rate - best_rate))
{
> best_rate = tmp_rate;
> best_m = m;
> - is_double = (d == 2) ? true :
false;
> + is_double = (d == 2);
> }
> }
> }
> --
> 2.33.1
>
>
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 17:46:10 -0800 > Brian Norris wrote: > > > Hi Pekka, > > > > Thanks for the thoughts and review. I've tried to respond below: > > > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > >with an input_handler boost, that preemptively exits self-refresh > > > >whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > >render new frames immediately after user activity. Powering up the > > > >GPU can take enough time that it is worthwhile to start this process > > > >as soon as there is input activity. Many Chrome OS systems also ship > > > >with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > Patch 2 has details. It's not really about precisely how slow PSR is, > > but how much foresight we can gain: in patch 2, I note that with my > > particular user space and system, I can start PSR-exit 50ms earlier than > > I would otherweise. (FWIW, this measurement is exactly the same it was > > with the original version written 4 years ago.) > > > > For how long PSR-exit takes: the measurements I'm able to do (via > > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > > manages to hide nearly 100% of that latency. > > > > Typical use cases where one notices PSR latency (and where this 35-55ms > > matters) involve simply moving a cursor; it's very noticeable when you > > have more than a few frames of latency to "get started". > > Hi Brian, > > that is very interesting, thanks. > > I would never have expected to have userspace take *that* long to > react. But, that sounds like it could be just your userspace software > stack. In the other subthread we're talking about making this more explicit. Maybe we need to combine this with a "I expect to take this many milliseconds to get the first frame out" value. That way compositors which take 50ms (which frankly is shocking slow) can set that, and kms can enable sr exit (since sr exit will actually help here). But other compositors which expect to get the first frame out in maybe 20 can spec that, and then the driver will not sr exit (because too high chances we'll just make shit slower), and instead will only boost render clocks. Thoughts? -Daniel > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > If my email address isn't obvious, I'm testing Chrome OS. I'm frankly > > not that familiar with the user space display stack, but for what I > > know, it's rather custom, developed within the Chromium project. Others > > on CC here could probably give you more detail, if you want specific > > answers, besides docs like this: > > > > https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > >
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 11:54:19AM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 15:30:38 -0800 > Rob Clark wrote: > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > >with an input_handler boost, that preemptively exits self-refresh > > > >whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > >render new frames immediately after user activity. Powering up the > > > >GPU can take enough time that it is worthwhile to start this process > > > >as soon as there is input activity. Many Chrome OS systems also ship > > > >with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > In my measurements, it takes userspace a frame or two to respond and > > get to the point of starting to build cmdstream (before eventually > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > a similar boost mechanism to spool up cpufreq > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > cumulation of a long sequence of events.. input-boost is letting > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > that long sequence. > > Right, exactly. That is why I was musing about a *new* ioctl that > userspace could hit as soon as any input device fd (or network fd!) > shows signs of life. Would that be enough, avoiding all the annoying > questions about which input and DRM devices should participate here > (and what about non-input devices that still want to trigger the > warm-up, like network traffic, e.g. remote control?), or does it really > need to be kernel internal to be fast enough? > > As Brian wrote about his quick hack to test that via debugfs, sounds > like the userspace solution would be totally sufficient. Random idea ... should we perhaps let userspace connect the boosting? I.e. we do a bunch of standardized boost targets (render clocks, display sr exit), and userspace can then connect it to whichever input device it wants to? That also avoids the multi-user lol of us boosting the wrong seat, we could do a drm ioctl where you pass it an eventfd and essentially say "listen to this mkay?" That way the boosting would also neatly get passed along with compositors as we vt switch them, in case you have one that's all tablet, and another one (console emulation) that's kbd only. Also this avoids the latency problem perhaps of a compositor which just dumbly paints every frame because it's VR or something like that, so never any sr exit possible. Just an idea, compositor people pls shred it :-) -Daniel -- Daniel Vetter Software Engineer, Intel
[PATCH v5 6/6] drm/i915/ttm: Update i915_gem_obj_copy_ttm() to be asynchronous
Update the copy function i915_gem_obj_copy_ttm() to be asynchronous for
future users and update the only current user to sync the objects
as needed after this function.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 40 ++--
drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c | 2 +
2 files changed, 30 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index 38623fde170a..d377c86232f1 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -822,33 +822,49 @@ int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
.interruptible = intr,
};
struct i915_refct_sgt *dst_rsgt;
- struct dma_fence *copy_fence;
- int ret;
+ struct dma_fence *copy_fence, *dep_fence;
+ struct i915_deps deps;
+ int ret, shared_err;
assert_object_held(dst);
assert_object_held(src);
+ i915_deps_init(, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
/*
-* Sync for now. This will change with async moves.
+* We plan to add a shared fence only for the source. If that
+* fails, we await all source fences before commencing
+* the copy instead of only the exclusive.
*/
- ret = ttm_bo_wait_ctx(dst_bo, );
+ shared_err = dma_resv_reserve_shared(src_bo->base.resv, 1);
+ ret = i915_deps_add_resv(, dst_bo->base.resv, true, false, );
if (!ret)
- ret = ttm_bo_wait_ctx(src_bo, );
+ ret = i915_deps_add_resv(, src_bo->base.resv,
+!!shared_err, false, );
if (ret)
return ret;
+ dep_fence = i915_deps_to_fence(, );
+ if (IS_ERR(dep_fence))
+ return PTR_ERR(dep_fence);
+
dst_rsgt = i915_ttm_resource_get_st(dst, dst_bo->resource);
copy_fence = __i915_ttm_move(src_bo, false, dst_bo->resource,
-dst_bo->ttm, dst_rsgt, allow_accel, NULL);
+dst_bo->ttm, dst_rsgt, allow_accel,
+dep_fence);
i915_refct_sgt_put(dst_rsgt);
- if (IS_ERR(copy_fence))
- return PTR_ERR(copy_fence);
+ if (IS_ERR_OR_NULL(copy_fence))
+ return PTR_ERR_OR_ZERO(copy_fence);
- if (copy_fence) {
- dma_fence_wait(copy_fence, false);
- dma_fence_put(copy_fence);
- }
+ dma_resv_add_excl_fence(dst_bo->base.resv, copy_fence);
+
+ /* If we failed to reserve a shared slot, add an exclusive fence */
+ if (shared_err)
+ dma_resv_add_excl_fence(src_bo->base.resv, copy_fence);
+ else
+ dma_resv_add_shared_fence(src_bo->base.resv, copy_fence);
+
+ dma_fence_put(copy_fence);
return 0;
}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
index 60d10ab55d1e..9aad84059d56 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c
@@ -80,6 +80,7 @@ static int i915_ttm_backup(struct i915_gem_apply_to_region
*apply,
err = i915_gem_obj_copy_ttm(backup, obj, pm_apply->allow_gpu, false);
GEM_WARN_ON(err);
+ ttm_bo_wait_ctx(backup_bo, );
obj->ttm.backup = backup;
return 0;
@@ -170,6 +171,7 @@ static int i915_ttm_restore(struct i915_gem_apply_to_region
*apply,
err = i915_gem_obj_copy_ttm(obj, backup, pm_apply->allow_gpu,
false);
GEM_WARN_ON(err);
+ ttm_bo_wait_ctx(backup_bo, );
obj->ttm.backup = NULL;
err = 0;
--
2.31.1
[PATCH v5 5/6] drm/i915/ttm: Implement asynchronous TTM moves
Don't wait sync while migrating, but rather make the GPU blit await the
dependencies and add a moving fence to the object.
This also enables asynchronous VRAM management in that on eviction,
rather than waiting for the moving fence to expire before freeing VRAM,
it is freed immediately and the fence is stored with the VRAM manager and
handed out to newly allocated objects to await before clears and swapins,
or for kernel objects before setting up gpu vmas or mapping.
To collect dependencies before migrating, add a set of utilities that
coalesce these to a single dma_fence.
What is still missing for fully asynchronous operation is asynchronous vma
unbinding, which is still to be implemented.
This commit substantially reduces execution time in the gem_lmem_swapping
test.
v2:
- Make a couple of functions static.
v4:
- Fix some style issues (Matthew Auld)
- Audit and add more checks for ghost objects (Matthew Auld)
- Add more documentation for the i915_deps utility (Mattew Auld)
- Simplify the i915_deps_sync() function
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 32 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 338 +--
drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +-
4 files changed, 344 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index e37157b080e4..81e84c1763de 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -248,10 +248,13 @@ static struct ttm_tt *i915_ttm_tt_create(struct
ttm_buffer_object *bo,
struct ttm_resource_manager *man =
ttm_manager_type(bo->bdev, bo->resource->mem_type);
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- enum ttm_caching caching = i915_ttm_select_tt_caching(obj);
+ enum ttm_caching caching;
struct i915_ttm_tt *i915_tt;
int ret;
+ if (!obj)
+ return NULL;
+
i915_tt = kzalloc(sizeof(*i915_tt), GFP_KERNEL);
if (!i915_tt)
return NULL;
@@ -260,6 +263,7 @@ static struct ttm_tt *i915_ttm_tt_create(struct
ttm_buffer_object *bo,
man->use_tt)
page_flags |= TTM_TT_FLAG_ZERO_ALLOC;
+ caching = i915_ttm_select_tt_caching(obj);
if (i915_gem_object_is_shrinkable(obj) && caching == ttm_cached) {
page_flags |= TTM_TT_FLAG_EXTERNAL |
TTM_TT_FLAG_EXTERNAL_MAPPABLE;
@@ -326,6 +330,9 @@ static bool i915_ttm_eviction_valuable(struct
ttm_buffer_object *bo,
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
+ if (!obj)
+ return false;
+
/*
* EXTERNAL objects should never be swapped out by TTM, instead we need
* to handle that ourselves. TTM will already skip such objects for us,
@@ -552,8 +559,12 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj,
static void i915_ttm_swap_notify(struct ttm_buffer_object *bo)
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- int ret = i915_ttm_move_notify(bo);
+ int ret;
+ if (!obj)
+ return;
+
+ ret = i915_ttm_move_notify(bo);
GEM_WARN_ON(ret);
GEM_WARN_ON(obj->ttm.cached_io_rsgt);
if (!ret && obj->mm.madv != I915_MADV_WILLNEED)
@@ -575,17 +586,23 @@ static unsigned long i915_ttm_io_mem_pfn(struct
ttm_buffer_object *bo,
unsigned long page_offset)
{
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
- unsigned long base = obj->mm.region->iomap.base -
obj->mm.region->region.start;
struct scatterlist *sg;
+ unsigned long base;
unsigned int ofs;
+ GEM_BUG_ON(!obj);
GEM_WARN_ON(bo->ttm);
+ base = obj->mm.region->iomap.base - obj->mm.region->region.start;
sg = __i915_gem_object_get_sg(obj, >ttm.get_io_page, page_offset,
, true);
return ((base + sg_dma_address(sg)) >> PAGE_SHIFT) + ofs;
}
+/*
+ * All callbacks need to take care not to downcast a struct ttm_buffer_object
+ * without checking its subclass, since it might be a TTM ghost object.
+ */
static struct ttm_device_funcs i915_ttm_bo_driver = {
.ttm_tt_create = i915_ttm_tt_create,
.ttm_tt_populate = i915_ttm_tt_populate,
@@ -847,13 +864,16 @@ static void i915_ttm_delayed_free(struct
drm_i915_gem_object *obj)
static vm_fault_t vm_fault_ttm(struct vm_fault *vmf)
{
struct vm_area_struct *area = vmf->vma;
- struct drm_i915_gem_object *obj =
- i915_ttm_to_gem(area->vm_private_data);
- struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
+ struct ttm_buffer_object *bo = area->vm_private_data;
struct drm_device *dev = bo->base.dev;
+ struct drm_i915_gem_object *obj;
vm_fault_t ret;
int
[PATCH v5 3/6] drm/i915/ttm: Drop region reference counting
There is an interesting refcounting loop:
struct intel_memory_region has a struct ttm_resource_manager,
ttm_resource_manager->move may hold a reference to i915_request,
i915_request may hold a reference to intel_context,
intel_context may hold a reference to drm_i915_gem_object,
drm_i915_gem_object may hold a reference to intel_memory_region.
Break this loop by dropping region reference counting.
In addition, Have regions with a manager moving fence make sure
that all region objects are released before freeing the region.
Signed-off-by: Thomas Hellström
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_region.c| 4 +--
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 3 +-
drivers/gpu/drm/i915/gem/i915_gem_stolen.c| 6 ++--
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 3 +-
.../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +-
drivers/gpu/drm/i915/gt/intel_region_lmem.c | 10 --
drivers/gpu/drm/i915/intel_memory_region.c| 26 --
drivers/gpu/drm/i915/intel_memory_region.h| 9 ++---
drivers/gpu/drm/i915/intel_region_ttm.c | 35 +--
drivers/gpu/drm/i915/intel_region_ttm.h | 2 +-
.../drm/i915/selftests/intel_memory_region.c | 8 ++---
drivers/gpu/drm/i915/selftests/mock_region.c | 7 ++--
12 files changed, 69 insertions(+), 46 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c
b/drivers/gpu/drm/i915/gem/i915_gem_region.c
index a016ccec36f3..a4350227e9ae 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
@@ -11,7 +11,7 @@
void i915_gem_object_init_memory_region(struct drm_i915_gem_object *obj,
struct intel_memory_region *mem)
{
- obj->mm.region = intel_memory_region_get(mem);
+ obj->mm.region = mem;
mutex_lock(>objects.lock);
list_add(>mm.region_link, >objects.list);
@@ -25,8 +25,6 @@ void i915_gem_object_release_memory_region(struct
drm_i915_gem_object *obj)
mutex_lock(>objects.lock);
list_del(>mm.region_link);
mutex_unlock(>objects.lock);
-
- intel_memory_region_put(mem);
}
struct drm_i915_gem_object *
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 4a88c89b7a14..cc9fe258fba7 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -664,9 +664,10 @@ static int init_shmem(struct intel_memory_region *mem)
return 0; /* Don't error, we can simply fallback to the kernel mnt */
}
-static void release_shmem(struct intel_memory_region *mem)
+static int release_shmem(struct intel_memory_region *mem)
{
i915_gemfs_fini(mem->i915);
+ return 0;
}
static const struct intel_memory_region_ops shmem_region_ops = {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
index ddd37ccb1362..80680395bb3b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
@@ -720,9 +720,10 @@ static int init_stolen_smem(struct intel_memory_region
*mem)
return i915_gem_init_stolen(mem);
}
-static void release_stolen_smem(struct intel_memory_region *mem)
+static int release_stolen_smem(struct intel_memory_region *mem)
{
i915_gem_cleanup_stolen(mem->i915);
+ return 0;
}
static const struct intel_memory_region_ops i915_region_stolen_smem_ops = {
@@ -759,10 +760,11 @@ static int init_stolen_lmem(struct intel_memory_region
*mem)
return err;
}
-static void release_stolen_lmem(struct intel_memory_region *mem)
+static int release_stolen_lmem(struct intel_memory_region *mem)
{
io_mapping_fini(>iomap);
i915_gem_cleanup_stolen(mem->i915);
+ return 0;
}
static const struct intel_memory_region_ops i915_region_stolen_lmem_ops = {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 537a81445b90..350bf1a23db5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -997,7 +997,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region
*mem,
i915_gem_object_init(obj, _gem_ttm_obj_ops, _class, flags);
/* Don't put on a region list until we're either locked or fully
initialized. */
- obj->mm.region = intel_memory_region_get(mem);
+ obj->mm.region = mem;
INIT_LIST_HEAD(>mm.region_link);
INIT_RADIX_TREE(>ttm.get_io_page.radix, GFP_KERNEL | __GFP_NOWARN);
@@ -1044,6 +1044,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region
*mem,
static const struct intel_memory_region_ops ttm_system_region_ops = {
.init_object = __i915_gem_ttm_object_init,
+ .release = intel_region_ttm_fini,
};
struct intel_memory_region *
diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
[PATCH v5 4/6] drm/i915/ttm: Correctly handle waiting for gpu when shrinking
With async migration, the shrinker may end up wanting to release the
pages of an object while the migration blit is still running, since
the GT migration code doesn't set up VMAs and the shrinker is thus
oblivious to the fact that the GPU is still using the pages.
Add waiting for gpu in the shrinker_release_pages() op and an
argument to that function indicating whether the shrinker expects it
to not wait for gpu. In the latter case the shrinker_release_pages()
op will return -EBUSY if the object is not idle.
Signed-off-by: Thomas Hellström
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 1 +
drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 1 +
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 7 ++-
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
index 604ed5ad77f5..f9f7e44099fe 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
@@ -59,6 +59,7 @@ struct drm_i915_gem_object_ops {
int (*truncate)(struct drm_i915_gem_object *obj);
void (*writeback)(struct drm_i915_gem_object *obj);
int (*shrinker_release_pages)(struct drm_i915_gem_object *obj,
+ bool no_gpu_wait,
bool should_writeback);
int (*pread)(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
index dde0a5c232f8..8b4b5f3a432a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c
@@ -60,6 +60,7 @@ static int try_to_writeback(struct drm_i915_gem_object *obj,
unsigned int flags)
{
if (obj->ops->shrinker_release_pages)
return obj->ops->shrinker_release_pages(obj,
+ !(flags &
I915_SHRINK_ACTIVE),
flags &
I915_SHRINK_WRITEBACK);
switch (obj->mm.madv) {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 350bf1a23db5..e37157b080e4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -418,6 +418,7 @@ int i915_ttm_purge(struct drm_i915_gem_object *obj)
}
static int i915_ttm_shrinker_release_pages(struct drm_i915_gem_object *obj,
+ bool no_wait_gpu,
bool should_writeback)
{
struct ttm_buffer_object *bo = i915_gem_to_ttm(obj);
@@ -425,7 +426,7 @@ static int i915_ttm_shrinker_release_pages(struct
drm_i915_gem_object *obj,
container_of(bo->ttm, typeof(*i915_tt), ttm);
struct ttm_operation_ctx ctx = {
.interruptible = true,
- .no_wait_gpu = false,
+ .no_wait_gpu = no_wait_gpu,
};
struct ttm_placement place = {};
int ret;
@@ -438,6 +439,10 @@ static int i915_ttm_shrinker_release_pages(struct
drm_i915_gem_object *obj,
if (!i915_tt->filp)
return 0;
+ ret = ttm_bo_wait_ctx(bo, );
+ if (ret)
+ return ret;
+
switch (obj->mm.madv) {
case I915_MADV_DONTNEED:
return i915_ttm_purge(obj);
--
2.31.1
[PATCH v5 1/6] drm/i915: Add support for moving fence waiting
From: Maarten Lankhorst
For now, we will only allow async migration when TTM is used,
so the paths we care about are related to TTM.
The mmap path is handled by having the fence in ttm_bo->moving,
when pinning, the binding only becomes available after the moving
fence is signaled, and pinning a cpu map will only work after
the moving fence signals.
This should close all holes where userspace can read a buffer
before it's fully migrated.
v2:
- Fix a couple of SPARSE warnings
v3:
- Fix a NULL pointer dereference
v4:
- Ditch the moving fence waiting for i915_vma_pin_iomap() and
replace with a verification that the vma is already bound.
(Matthew Auld)
- Squash with a previous patch introducing moving fence waiting and
accessing interfaces (Matthew Auld)
- Rename to indicated that we also add support for sync waiting.
v5:
- Fix check for NULL and unreferencing i915_vma_verify_bind_complete()
(Matthew Auld)
- Fix compilation failure if !CONFIG_DRM_I915_DEBUG_GEM
- Fix include ordering. (Matthew Auld)
Co-developed-by: Thomas Hellström
Signed-off-by: Thomas Hellström
Signed-off-by: Maarten Lankhorst
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 52 ++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 6 +++
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 6 +++
drivers/gpu/drm/i915/i915_vma.c| 39 +++-
4 files changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 591ee3cb7275..24f83c432350 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -31,6 +31,7 @@
#include "i915_gem_context.h"
#include "i915_gem_mman.h"
#include "i915_gem_object.h"
+#include "i915_gem_ttm.h"
#include "i915_memcpy.h"
#include "i915_trace.h"
@@ -726,6 +727,57 @@ static const struct drm_gem_object_funcs
i915_gem_object_funcs = {
.export = i915_gem_prime_export,
};
+/**
+ * i915_gem_object_get_moving_fence - Get the object's moving fence if any
+ * @obj: The object whose moving fence to get.
+ *
+ * A non-signaled moving fence means that there is an async operation
+ * pending on the object that needs to be waited on before setting up
+ * any GPU- or CPU PTEs to the object's pages.
+ *
+ * Return: A refcounted pointer to the object's moving fence if any,
+ * NULL otherwise.
+ */
+struct dma_fence *
+i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj)
+{
+ return dma_fence_get(i915_gem_to_ttm(obj)->moving);
+}
+
+/**
+ * i915_gem_object_wait_moving_fence - Wait for the object's moving fence if
any
+ * @obj: The object whose moving fence to wait for.
+ * @intr: Whether to wait interruptible.
+ *
+ * If the moving fence signaled without an error, it is detached from the
+ * object and put.
+ *
+ * Return: 0 if successful, -ERESTARTSYS if the wait was interrupted,
+ * negative error code if the async operation represented by the
+ * moving fence failed.
+ */
+int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj,
+ bool intr)
+{
+ struct dma_fence *fence = i915_gem_to_ttm(obj)->moving;
+ int ret;
+
+ assert_object_held(obj);
+ if (!fence)
+ return 0;
+
+ ret = dma_fence_wait(fence, intr);
+ if (ret)
+ return ret;
+
+ if (fence->error)
+ return fence->error;
+
+ i915_gem_to_ttm(obj)->moving = NULL;
+ dma_fence_put(fence);
+ return 0;
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
#include "selftests/huge_gem_object.c"
#include "selftests/huge_pages.c"
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 133963b46135..66f20b803b01 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -517,6 +517,12 @@ i915_gem_object_finish_access(struct drm_i915_gem_object
*obj)
i915_gem_object_unpin_pages(obj);
}
+struct dma_fence *
+i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj);
+
+int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj,
+ bool intr);
+
void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
unsigned int cache_level);
bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj);
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index c4f684b7cc51..49c6e55c68ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -418,6 +418,12 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object
*obj,
}
if (!ptr) {
+ err = i915_gem_object_wait_moving_fence(obj, true);
+ if (err) {
+ ptr = ERR_PTR(err);
+
[PATCH v5 2/6] drm/i915/ttm: Move the i915_gem_obj_copy_ttm() function
Move the i915_gem_obj_copy_ttm() function to i915_gem_ttm_move.h.
This will help keep a number of functions static when introducing
async moves.
Signed-off-by: Thomas Hellström
Reviewed-by: Matthew Auld
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 47 ---
drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 4 --
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 63
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 10 ++--
drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c | 1 +
5 files changed, 56 insertions(+), 69 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 68cfe6e9ceab..537a81445b90 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1063,50 +1063,3 @@ i915_gem_ttm_system_setup(struct drm_i915_private *i915,
intel_memory_region_set_name(mr, "system-ttm");
return mr;
}
-
-/**
- * i915_gem_obj_copy_ttm - Copy the contents of one ttm-based gem object to
- * another
- * @dst: The destination object
- * @src: The source object
- * @allow_accel: Allow using the blitter. Otherwise TTM memcpy is used.
- * @intr: Whether to perform waits interruptible:
- *
- * Note: The caller is responsible for assuring that the underlying
- * TTM objects are populated if needed and locked.
- *
- * Return: Zero on success. Negative error code on error. If @intr == true,
- * then it may return -ERESTARTSYS or -EINTR.
- */
-int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
- struct drm_i915_gem_object *src,
- bool allow_accel, bool intr)
-{
- struct ttm_buffer_object *dst_bo = i915_gem_to_ttm(dst);
- struct ttm_buffer_object *src_bo = i915_gem_to_ttm(src);
- struct ttm_operation_ctx ctx = {
- .interruptible = intr,
- };
- struct i915_refct_sgt *dst_rsgt;
- int ret;
-
- assert_object_held(dst);
- assert_object_held(src);
-
- /*
-* Sync for now. This will change with async moves.
-*/
- ret = ttm_bo_wait_ctx(dst_bo, );
- if (!ret)
- ret = ttm_bo_wait_ctx(src_bo, );
- if (ret)
- return ret;
-
- dst_rsgt = i915_ttm_resource_get_st(dst, dst_bo->resource);
- __i915_ttm_move(src_bo, false, dst_bo->resource, dst_bo->ttm,
- dst_rsgt, allow_accel);
-
- i915_refct_sgt_put(dst_rsgt);
-
- return 0;
-}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
index 074a7c08ff31..82cdabb542be 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.h
@@ -49,10 +49,6 @@ int __i915_gem_ttm_object_init(struct intel_memory_region
*mem,
resource_size_t page_size,
unsigned int flags);
-int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
- struct drm_i915_gem_object *src,
- bool allow_accel, bool intr);
-
/* Internal I915 TTM declarations and definitions below. */
#define I915_PL_LMEM0 TTM_PL_PRIV
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index ef22d4ed66ad..f35b386c56ca 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -378,18 +378,10 @@ i915_ttm_memcpy_work_arm(struct i915_ttm_memcpy_work
*work,
return >fence;
}
-/**
- * __i915_ttm_move - helper to perform TTM moves or clears.
- * @bo: The source buffer object.
- * @clear: Whether this is a clear operation.
- * @dst_mem: The destination ttm resource.
- * @dst_ttm: The destination ttm page vector.
- * @dst_rsgt: The destination refcounted sg-list.
- * @allow_accel: Whether to allow acceleration.
- */
-void __i915_ttm_move(struct ttm_buffer_object *bo, bool clear,
-struct ttm_resource *dst_mem, struct ttm_tt *dst_ttm,
-struct i915_refct_sgt *dst_rsgt, bool allow_accel)
+static void __i915_ttm_move(struct ttm_buffer_object *bo, bool clear,
+ struct ttm_resource *dst_mem,
+ struct ttm_tt *dst_ttm,
+ struct i915_refct_sgt *dst_rsgt, bool allow_accel)
{
struct i915_ttm_memcpy_work *copy_work = NULL;
struct i915_ttm_memcpy_arg _arg, *arg = &_arg;
@@ -521,3 +513,50 @@ int i915_ttm_move(struct ttm_buffer_object *bo, bool evict,
i915_ttm_adjust_gem_after_move(obj);
return 0;
}
+
+/**
+ * i915_gem_obj_copy_ttm - Copy the contents of one ttm-based gem object to
+ * another
+ * @dst: The destination object
+ * @src: The source object
+ * @allow_accel: Allow using the blitter. Otherwise TTM memcpy is used.
+ * @intr: Whether to perform waits interruptible:
+ *
+ * Note: The caller is responsible for assuring that the underlying
+ * TTM
[PATCH v5 0/6] drm/i915/ttm: Async migration
This patch series deals with async migration and async vram management. It still leaves an important part out, which is async unbinding which will reduce latency further, at least when trying to migrate already active objects. Patches 1/6 deals with accessing and waiting for the TTM moving fence from i915 GEM. Patch 2 is pure code reorganization, no functional change. Patch 3 breaks a refcounting loop involving the TTM moving fence. Patch 4 makes the i915 TTM shinking code handle async moves. Patch 5 uses TTM to implement the ttm move() callback async, it also introduces a utility to collect dependencies and turn them into a single dma_fence, which is needed for the intel_migrate code. This also affects the gem object migrate code. Patch 6 makes the object copy utility async as well, mainly for future users since the only current user, suspend backup and restore, typically will want to sync anyway. v2: - Fix a couple of SPARSE warnings. v3: - Fix a NULL pointer dereference. v4: - Squash what was previously patch 1 and 2 to patch1 - Ditch the moving fence waiting in i915_vma_pin_iomap() - Rework how the refcounting loop is broken in patch 3. Drop region reference counting. - Break what is now patch 4 out of patch 5. Add support for avoiding waiting for gpu when shrinking. - A number of changes in patch 5. See the commit message for details. v5: - Some fixes to i915_vma_verify_bind_complete() (Matthew Auld) - Update patches with R-B. Maarten Lankhorst (1): drm/i915: Add support for moving fence waiting Thomas Hellström (5): drm/i915/ttm: Move the i915_gem_obj_copy_ttm() function drm/i915/ttm: Drop region reference counting drm/i915/ttm: Correctly handle waiting for gpu when shrinking drm/i915/ttm: Implement asynchronous TTM moves drm/i915/ttm: Update i915_gem_obj_copy_ttm() to be asynchronous drivers/gpu/drm/i915/gem/i915_gem_object.c| 52 +++ drivers/gpu/drm/i915/gem/i915_gem_object.h| 6 + .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 + drivers/gpu/drm/i915/gem/i915_gem_pages.c | 6 + drivers/gpu/drm/i915/gem/i915_gem_region.c| 4 +- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 3 +- drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 1 + drivers/gpu/drm/i915/gem/i915_gem_stolen.c| 6 +- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 89 ++-- drivers/gpu/drm/i915/gem/i915_gem_ttm.h | 6 +- drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 405 -- drivers/gpu/drm/i915/gem/i915_gem_ttm_move.h | 10 +- drivers/gpu/drm/i915/gem/i915_gem_ttm_pm.c| 3 + drivers/gpu/drm/i915/gem/i915_gem_wait.c | 4 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- drivers/gpu/drm/i915/gt/intel_region_lmem.c | 10 +- drivers/gpu/drm/i915/i915_vma.c | 39 +- drivers/gpu/drm/i915/intel_memory_region.c| 26 +- drivers/gpu/drm/i915/intel_memory_region.h| 9 +- drivers/gpu/drm/i915/intel_region_ttm.c | 35 +- drivers/gpu/drm/i915/intel_region_ttm.h | 2 +- .../drm/i915/selftests/intel_memory_region.c | 8 +- drivers/gpu/drm/i915/selftests/mock_region.c | 7 +- 23 files changed, 591 insertions(+), 143 deletions(-) -- 2.31.1
Re: [PATCH v2 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function
Il 19/11/21 14:19, Robert Foss ha scritto:
Hey Angelo,
On Wed, 10 Nov 2021 at 13:46, AngeloGioacchino Del Regno
wrote:
Il 10/11/21 13:44, Dafna Hirschfeld ha scritto:
On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
In preparation for varying the poweron error handling in function
ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
and also move the actual logic to power off the chip to a new
__ps8640_bridge_poweroff() function.
Signed-off-by: AngeloGioacchino Del Regno
---
drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++
1 file changed, 20 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8c5402947b3c..41f5d511d516 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640
*ps_bridge,
return 0;
}
+static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
+{
+gpiod_set_value(ps_bridge->gpio_reset, 1);
+gpiod_set_value(ps_bridge->gpio_powerdown, 1);
+if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
+ ps_bridge->supplies)) {
+DRM_ERROR("cannot disable regulators\n");
+}
That '{' is redundant
Thanks,
Danfa
Hi Dafna,
the braces were added as a way to increase human readability.
Not to bikeshed this, but the kernel style guide is clear about this.
No unneeded braces should be used where a single statement will do.
Hey Robert!
That's right, style rules come before personal preferences.
I'll send a v3 without the braces as soon as possible!
Cheers,
- Angelo
[PATCH 2/2] drm/bridge: chipone-icn6211: Add mode_set API
Get the display mode settings via mode_set bridge
function instead of explicitly de-reference.
Signed-off-by: Jagan Teki
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c
b/drivers/gpu/drm/bridge/chipone-icn6211.c
index 77b3e2c29461..e8f36dca56b3 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -31,6 +31,7 @@
struct chipone {
struct device *dev;
struct drm_bridge bridge;
+ struct drm_display_mode mode;
struct drm_bridge *panel_bridge;
struct gpio_desc *enable_gpio;
struct regulator *vdd1;
@@ -43,11 +44,6 @@ static inline struct chipone *bridge_to_chipone(struct
drm_bridge *bridge)
return container_of(bridge, struct chipone, bridge);
}
-static struct drm_display_mode *bridge_to_mode(struct drm_bridge *bridge)
-{
- return >encoder->crtc->state->adjusted_mode;
-}
-
static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
size_t len)
{
@@ -66,7 +62,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
- struct drm_display_mode *mode = bridge_to_mode(bridge);
+ struct drm_display_mode *mode = >mode;
ICN6211_DSI(icn, 0x7a, 0xc1);
@@ -165,6 +161,15 @@ static void chipone_atomic_post_disable(struct drm_bridge
*bridge,
gpiod_set_value(icn->enable_gpio, 0);
}
+static void chipone_mode_set(struct drm_bridge *bridge,
+const struct drm_display_mode *mode,
+const struct drm_display_mode *adjusted_mode)
+{
+ struct chipone *icn = bridge_to_chipone(bridge);
+
+ drm_mode_copy(>mode, adjusted_mode);
+}
+
static int chipone_attach(struct drm_bridge *bridge, enum
drm_bridge_attach_flags flags)
{
struct chipone *icn = bridge_to_chipone(bridge);
@@ -179,6 +184,7 @@ static const struct drm_bridge_funcs chipone_bridge_funcs =
{
.atomic_pre_enable = chipone_atomic_pre_enable,
.atomic_enable = chipone_atomic_enable,
.atomic_post_disable= chipone_atomic_post_disable,
+ .mode_set = chipone_mode_set,
.attach = chipone_attach,
};
--
2.25.1
[PATCH 1/2] drm/bridge: chipone-icn6211: Switch to atomic operations
Replace atomic version of the pre_enable/enable/post_disable
operations to continue the transition to the atomic API.
Also added default drm atomic operations for duplicate, destroy
and reset state API's in order to have smooth transition on
atomic API's.
Tested on Allwinner R16/R40 DSI.
Signed-off-by: Jagan Teki
---
drivers/gpu/drm/bridge/chipone-icn6211.c | 21 ++---
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c
b/drivers/gpu/drm/bridge/chipone-icn6211.c
index a6151db95586..77b3e2c29461 100644
--- a/drivers/gpu/drm/bridge/chipone-icn6211.c
+++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
@@ -4,6 +4,7 @@
* Author: Jagan Teki
*/
+#include
#include
#include
#include
@@ -61,7 +62,8 @@ static inline int chipone_dsi_write(struct chipone *icn,
const void *seq,
chipone_dsi_write(icn, d, ARRAY_SIZE(d)); \
}
-static void chipone_enable(struct drm_bridge *bridge)
+static void chipone_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
struct drm_display_mode *mode = bridge_to_mode(bridge);
@@ -114,7 +116,8 @@ static void chipone_enable(struct drm_bridge *bridge)
usleep_range(1, 11000);
}
-static void chipone_pre_enable(struct drm_bridge *bridge)
+static void chipone_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
int ret;
@@ -145,7 +148,8 @@ static void chipone_pre_enable(struct drm_bridge *bridge)
usleep_range(1, 11000);
}
-static void chipone_post_disable(struct drm_bridge *bridge)
+static void chipone_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct chipone *icn = bridge_to_chipone(bridge);
@@ -169,10 +173,13 @@ static int chipone_attach(struct drm_bridge *bridge, enum
drm_bridge_attach_flag
}
static const struct drm_bridge_funcs chipone_bridge_funcs = {
- .attach = chipone_attach,
- .post_disable = chipone_post_disable,
- .pre_enable = chipone_pre_enable,
- .enable = chipone_enable,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_pre_enable = chipone_atomic_pre_enable,
+ .atomic_enable = chipone_atomic_enable,
+ .atomic_post_disable= chipone_atomic_post_disable,
+ .attach = chipone_attach,
};
static int chipone_parse_dt(struct chipone *icn)
--
2.25.1
Re: [PATCH] i2c: tegra: Add ACPI support
On Fri, Nov 19, 2021 at 3:37 PM Akhil R wrote:
>
> Add support for ACPI based device registration so that the driver
> can be also enabled through ACPI table.
the ACPI
...
> + if (has_acpi_companion(i2c_dev->dev)) {
You are checkin for the companion and using a handle, why not check
for a handle explicitly?
> + acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
> +NULL, NULL);
> + } else {
> + err = reset_control_reset(i2c_dev->rst);
> + WARN_ON_ONCE(err);
> + }
...
> + if (i2c_dev->nclocks == 0)
> + return;
Why? Make clocks optional.
...
> - i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> - if (IS_ERR(i2c_dev->rst)) {
> - dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> - "failed to get reset control\n");
> - return PTR_ERR(i2c_dev->rst);
Besides the fact this should be as simple as
return dev_err_probe(...)
> - }
> + if (!has_acpi_companion(>dev)) {
...why do you do this?
> + i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev,
> "i2c");
> + if (IS_ERR(i2c_dev->rst)) {
> + dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> + "failed to get reset control\n");
> + return PTR_ERR(i2c_dev->rst);
> + }
...
> +static const struct acpi_device_id tegra_i2c_acpi_match[] = {
> + {.id = "NVDA0101", .driver_data = (kernel_ulong_t)_i2c_hw},
> + {.id = "NVDA0201", .driver_data = (kernel_ulong_t)_i2c_hw},
> + {.id = "NVDA0301", .driver_data = (kernel_ulong_t)_i2c_hw},
> + { },
No comma for the terminator entry.
> +};
--
With Best Regards,
Andy Shevchenko
[Bug 211807] [drm:drm_dp_mst_dpcd_read] *ERROR* mstb 000000004e6288dd port 3: DPCD read on addr 0x60 for 1 bytes NAKed
https://bugzilla.kernel.org/show_bug.cgi?id=211807 --- Comment #15 from Michel Dänzer (mic...@daenzer.net) --- (In reply to Daan from comment #14) > I also had this in my logs yesterday, right before my system locked > completely (had to do a hard reset). That's probably coincidence. I get these messages on a regular basis, without any bad behaviour. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH 1/3] drm/fourcc: Add packed 10bit YUV 4:2:0 format
On Wed, 17 Nov 2021 15:08:58 +0100
Maxime Ripard wrote:
> From: Dave Stevenson
>
> Adds a format that is 3 10bit YUV 4:2:0 samples packed into
> a 32bit work (with 2 spare bits).
>
> Supported on Broadcom BCM2711 chips.
>
> Signed-off-by: Dave Stevenson
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/drm_fourcc.c | 3 +++
> include/uapi/drm/drm_fourcc.h | 11 +++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 25837b1d6639..07741b678798 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -269,6 +269,9 @@ const struct drm_format_info *__drm_format_info(u32
> format)
> .num_planes = 3, .char_per_block = { 2, 2, 2 },
> .block_w = { 1, 1, 1 }, .block_h = { 1, 1, 1 }, .hsub = 0,
> .vsub = 0, .is_yuv = true },
> + { .format = DRM_FORMAT_P030,.depth = 0,
> .num_planes = 2,
> + .char_per_block = { 4, 8, 0 }, .block_w = { 3, 3, 0 },
> .block_h = { 1, 1, 0 },
> + .hsub = 2, .vsub = 2, .is_yuv = true},
> };
>
> unsigned int i;
> diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> index 7f652c96845b..2e6d2ecae45f 100644
> --- a/include/uapi/drm/drm_fourcc.h
> +++ b/include/uapi/drm/drm_fourcc.h
> @@ -330,6 +330,13 @@ extern "C" {
> */
> #define DRM_FORMAT_Q401 fourcc_code('Q', '4', '0', '1')
>
> +/*
> + * 2 plane YCbCr MSB aligned, 3 pixels packed into 4 bytes.
Hi,
what does "MSB aligned" mean? How widely used term is that?
> + * index 0 = Y plane, [31:0] x:Y2:Y1:Y0 2:10:10:10 little endian
Because if I had to say, this looks like LSB aligned?
> + * index 1 = Cr:Cb plane, [63:0] x:Cr2:Cb2:Cr1:x:Cb1:Cr0:Cb0
> [2:10:10:10:2:10:10:10] little endian
And this is not really either, I guess.
Thanks,
pq
> + */
> +#define DRM_FORMAT_P030 fourcc_code('P', '0', '3', '0') /* 2x2
> subsampled Cr:Cb plane 10 bits per channel packed */
> +
> /*
> * 3 plane YCbCr
> * index 0: Y plane, [7:0] Y
> @@ -854,6 +861,10 @@ drm_fourcc_canonicalize_nvidia_format_mod(__u64 modifier)
> * and UV. Some SAND-using hardware stores UV in a separate tiled
> * image from Y to reduce the column height, which is not supported
> * with these modifiers.
> + *
> + * The DRM_FORMAT_MOD_BROADCOM_SAND128_COL_HEIGHT modifier is also
> + * supported for DRM_FORMAT_P030 where the columns remain as 128 bytes
> + * wide, but as this is a 10 bpp format that translates to 96 pixels.
> */
>
> #define DRM_FORMAT_MOD_BROADCOM_SAND32_COL_HEIGHT(v) \
pgpL__XkFZI12.pgp
Description: OpenPGP digital signature
Sparsely populated TTM bos
Hi, Christian, We have an upcoming use-case in i915 where one solution would be sparsely populated TTM bos. We had that at one point where ttm_tt pages were allocated on demand, but this time we'd rather be looking at multiple struct ttm_resources per bo and those resources could be from different managers. There might theoretically be other ways we can handle this use-case but I wanted to check with you whether this is something AMD is already looking into and if not, your general opinion. Thanks, Thomas
Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero
On Fri, 19 Nov 2021, Daniel Vetter wrote:
> On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
>> On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
>> > On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
>> > > On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
>> > > > Do a sanity check on struct drm_format_info hsub and vsub values to
>> > > > avoid divide by zero.
>> > > >
>> > > > Syzkaller reported a divide error in framebuffer_check() when the
>> > > > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via
>> > > > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for
>> > > > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0".
>> > > > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses
>> > > > vsub as a divisor. These divisors need to be sanity checked for
>> > > > zero before use.
>> > > >
>> > > > divide error: [#1] SMP KASAN NOPTI
>> > > > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1
>> > > > Hardware name: Red Hat KVM, BIOS 1.13.0-2
>> > > > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199
>> > > > [inline]
>> > > > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90
>> > > > drivers/gpu/drm/drm_framebuffer.c:317
>> > > >
>> > > > Call Trace:
>> > > > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355
>> > > > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391
>> > > > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795
>> > > > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898
>> > > > vfs_ioctl fs/ioctl.c:51 [inline]
>> > > > __do_sys_ioctl fs/ioctl.c:874 [inline]
>> > > > __se_sys_ioctl fs/ioctl.c:860 [inline]
>> > > > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860
>> > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>> > > > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80
>> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
>> > > >
>> > > > Signed-off-by: George Kennedy
>> > > > ---
>> > > > drivers/gpu/drm/drm_framebuffer.c | 10 ++
>> > > > 1 file changed, 10 insertions(+)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c
>> > > > b/drivers/gpu/drm/drm_framebuffer.c
>> > > > index 07f5abc..a146e4b 100644
>> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > > > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device
>> > > > *dev,
>> > > >/* now let the driver pick its own format info */
>> > > >info = drm_get_format_info(dev, r);
>> > > >
>> > > > + if (info->hsub == 0) {
>> > > > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor
>> > > > %u\n", info->hsub);
>> > > > + return -EINVAL;
>> > > > + }
>> > > > +
>> > > > + if (info->vsub == 0) {
>> > > > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor
>> > > > %u\n", info->vsub);
>> > > > + return -EINVAL;
>> > > > + }
>> > >
>> > > Looks like duct tape to me. I think we need to either fix those formats
>> > > to have valid format info, or just revert the whole patch that added such
>> > > broken things.
>> >
>> > Yeah maybe even a compile-time check of the format table(s) to validate
>> > them properly and scream ... Or at least a selftest.
>>
>> I really wish C had (even very limited) compile time evaluation
>> so one could actually loop over arrays like at compile time to
>> check each element. As it stands you either have to check each
>> array element by hand, or you do some cpp macro horrors to
>> pretend you're iterating the array.
>
> Python preprocess or so seems to be the usual answer, and that then just
> generates the C table after it's all checked.
>
> Or a post-processor which fishes the table out from the .o (or just links
> against it).
>
> But yeah doing this in cpp isn't going to work, aside from it'd be really
> ugly.
Kbuild does have support for hostprogs which are typically used in the
build. The obvious idea is to use that for code generation, but it would
also be interesting to see how that could be used for compile-time
evaluation of sorts. Kind of like compile-time selftests? And, of
course, how badly that would be frowned upon.
git grep says there are only four hostprogs users in drivers/, so it
certainly isn't a popularity contest winner. (One of them is
"mkregtable" in radeon.)
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
[Bug 213391] AMDGPU retries page fault with some specific processes amdgpu and sometimes followed [gfxhub0] retry page fault until *ERROR* ring gfx timeout, but soft recovered
https://bugzilla.kernel.org/show_bug.cgi?id=213391 Lahfa Samy (s...@lahfa.xyz) changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |UNREPRODUCIBLE -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 2/3] drm/bridge: parade-ps8640: Move real poweroff action to new function
Hey Angelo,
On Wed, 10 Nov 2021 at 13:46, AngeloGioacchino Del Regno
wrote:
>
> Il 10/11/21 13:44, Dafna Hirschfeld ha scritto:
> >
> >
> > On 02.11.21 11:36, AngeloGioacchino Del Regno wrote:
> >> In preparation for varying the poweron error handling in function
> >> ps8640_bridge_poweron(), move function ps8640_bridge_poweroff() up
> >> and also move the actual logic to power off the chip to a new
> >> __ps8640_bridge_poweroff() function.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno
> >>
> >> ---
> >> drivers/gpu/drm/bridge/parade-ps8640.c | 37 ++
> >> 1 file changed, 20 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> >> b/drivers/gpu/drm/bridge/parade-ps8640.c
> >> index 8c5402947b3c..41f5d511d516 100644
> >> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> >> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> >> @@ -293,6 +293,26 @@ static int ps8640_bridge_vdo_control(struct ps8640
> >> *ps_bridge,
> >> return 0;
> >> }
> >> +static void __ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
> >> +{
> >> +gpiod_set_value(ps_bridge->gpio_reset, 1);
> >> +gpiod_set_value(ps_bridge->gpio_powerdown, 1);
> >> +if (regulator_bulk_disable(ARRAY_SIZE(ps_bridge->supplies),
> >> + ps_bridge->supplies)) {
> >> +DRM_ERROR("cannot disable regulators\n");
> >> +}
> >
> > That '{' is redundant
> >
> > Thanks,
> > Danfa
> >
>
> Hi Dafna,
> the braces were added as a way to increase human readability.
Not to bikeshed this, but the kernel style guide is clear about this.
No unneeded braces should be used where a single statement will do.
[PATCH 1/2] i915/gvt: Introduce the mmio_info_table.c to support VFIO new mdev API
From: Zhi Wang
To support the new mdev interfaces and the re-factor patches from
Christoph, which moves the GVT-g code into a dedicated module, the GVT-g
initialization path has to be seperated into two phases:
a) Early initialization.
The early initialization of GVT requires to be done when loading i915.
Mostly it's because the inital clean HW state needs to be saved before
i915 touches the HW.
b) Late initalization.
This phases of initalization will setup the rest components of GVT-g,
which can be done later when the dedicated module is being loaded.
v2:
- Implement a mmio table instead of generating it by marco in i915. (Jani)
Cc: Christoph Hellwig
Cc: Jason Gunthorpe
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Vivi Rodrigo
Cc: Zhenyu Wang
Cc: Zhi Wang
Tested-by: Terrence Xu
Signed-off-by: Zhi Wang
---
drivers/gpu/drm/i915/Makefile |2 +-
drivers/gpu/drm/i915/gvt/gvt.c | 38 +-
drivers/gpu/drm/i915/gvt/gvt.h |1 +
drivers/gpu/drm/i915/gvt/handlers.c| 1780 +++-
drivers/gpu/drm/i915/gvt/mmio.h|5 +-
drivers/gpu/drm/i915/gvt/mmio_info_table.c | 1461
drivers/gpu/drm/i915/gvt/mmio_info_table.h | 35 +
drivers/gpu/drm/i915/gvt/reg.h |9 +-
drivers/gpu/drm/i915/intel_gvt.c | 42 +-
9 files changed, 1775 insertions(+), 1598 deletions(-)
create mode 100644 drivers/gpu/drm/i915/gvt/mmio_info_table.c
create mode 100644 drivers/gpu/drm/i915/gvt/mmio_info_table.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index cdc244bbbfc1..55603aebe3e4 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -309,7 +309,7 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
i915-y += i915_vgpu.o
ifeq ($(CONFIG_DRM_I915_GVT),y)
-i915-y += intel_gvt.o
+i915-y += intel_gvt.o gvt/mmio_info_table.o
include $(src)/gvt/Makefile
endif
diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
index cbac409f6c8a..c7580db355c0 100644
--- a/drivers/gpu/drm/i915/gvt/gvt.c
+++ b/drivers/gpu/drm/i915/gvt/gvt.c
@@ -63,23 +63,6 @@ static const struct intel_gvt_ops intel_gvt_ops = {
.emulate_hotplug = intel_vgpu_emulate_hotplug,
};
-static void init_device_info(struct intel_gvt *gvt)
-{
- struct intel_gvt_device_info *info = >device_info;
- struct pci_dev *pdev = to_pci_dev(gvt->gt->i915->drm.dev);
-
- info->max_support_vgpus = 8;
- info->cfg_space_size = PCI_CFG_SPACE_EXP_SIZE;
- info->mmio_size = 2 * 1024 * 1024;
- info->mmio_bar = 0;
- info->gtt_start_offset = 8 * 1024 * 1024;
- info->gtt_entry_size = 8;
- info->gtt_entry_size_shift = 3;
- info->gmadr_bytes_in_cmd = 8;
- info->max_surface_size = 36 * 1024 * 1024;
- info->msi_cap_offset = pdev->msi_cap;
-}
-
static void intel_gvt_test_and_emulate_vblank(struct intel_gvt *gvt)
{
struct intel_vgpu *vgpu;
@@ -169,7 +152,6 @@ void intel_gvt_clean_device(struct drm_i915_private *i915)
intel_gvt_clean_workload_scheduler(gvt);
intel_gvt_clean_gtt(gvt);
intel_gvt_free_firmware(gvt);
- intel_gvt_clean_mmio_info(gvt);
idr_destroy(>vgpu_idr);
kfree(i915->gvt);
@@ -188,16 +170,12 @@ void intel_gvt_clean_device(struct drm_i915_private *i915)
*/
int intel_gvt_init_device(struct drm_i915_private *i915)
{
- struct intel_gvt *gvt;
+ struct intel_gvt *gvt = i915->gvt;
struct intel_vgpu *vgpu;
int ret;
- if (drm_WARN_ON(>drm, i915->gvt))
- return -EEXIST;
-
- gvt = kzalloc(sizeof(struct intel_gvt), GFP_KERNEL);
- if (!gvt)
- return -ENOMEM;
+ if (drm_WARN_ON(>drm, !i915->gvt))
+ return -ENODEV;
gvt_dbg_core("init gvt device\n");
@@ -205,12 +183,8 @@ int intel_gvt_init_device(struct drm_i915_private *i915)
spin_lock_init(>scheduler.mmio_context_lock);
mutex_init(>lock);
mutex_init(>sched_lock);
- gvt->gt = >gt;
- i915->gvt = gvt;
-
- init_device_info(gvt);
- ret = intel_gvt_setup_mmio_info(gvt);
+ ret = intel_gvt_setup_mmio_handlers(gvt);
if (ret)
goto out_clean_idr;
@@ -218,7 +192,7 @@ int intel_gvt_init_device(struct drm_i915_private *i915)
ret = intel_gvt_load_firmware(gvt);
if (ret)
- goto out_clean_mmio_info;
+ goto out_clean_idr;
ret = intel_gvt_init_irq(gvt);
if (ret)
@@ -277,8 +251,6 @@ int intel_gvt_init_device(struct drm_i915_private *i915)
intel_gvt_clean_gtt(gvt);
out_free_firmware:
intel_gvt_free_firmware(gvt);
-out_clean_mmio_info:
- intel_gvt_clean_mmio_info(gvt);
out_clean_idr:
idr_destroy(>vgpu_idr);
kfree(gvt);
diff --git a/drivers/gpu/drm/i915/gvt/gvt.h b/drivers/gpu/drm/i915/gvt/gvt.h
index 0c0615602343..76b192563212 100644
---
[PATCH 2/2] i915/gvt: save the MMIO snapshot in the early init of GVT-g
From: Zhi Wang
To support the early init of GVT-g, which will be put in i915, after the
GVT-g is moved into a dedicated module, we need to save the MMIO snapshot
in the early init of GVT-g, when the HW hasn't been touched.
Cc: Christoph Hellwig
Cc: Jason Gunthorpe
Cc: Jani Nikula
Cc: Joonas Lahtinen
Cc: Vivi Rodrigo
Cc: Zhenyu Wang
Cc: Zhi Wang
Tested-by: Terrence Xu
Signed-off-by: Zhi Wang
---
drivers/gpu/drm/i915/gvt/firmware.c| 36 +--
drivers/gpu/drm/i915/gvt/handlers.c| 39
drivers/gpu/drm/i915/gvt/mmio_info_table.c | 72 +-
drivers/gpu/drm/i915/gvt/mmio_info_table.h | 3 +
4 files changed, 76 insertions(+), 74 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/firmware.c
b/drivers/gpu/drm/i915/gvt/firmware.c
index 1a8274a3f4b1..dd5d4dd7a8cf 100644
--- a/drivers/gpu/drm/i915/gvt/firmware.c
+++ b/drivers/gpu/drm/i915/gvt/firmware.c
@@ -66,12 +66,6 @@ static struct bin_attribute firmware_attr = {
.mmap = NULL,
};
-static int mmio_snapshot_handler(struct intel_gvt *gvt, u32 offset, void *data)
-{
- *(u32 *)(data + offset) = intel_uncore_read_notrace(gvt->gt->uncore,
- _MMIO(offset));
- return 0;
-}
static int expose_firmware_sysfs(struct intel_gvt *gvt)
{
@@ -99,17 +93,11 @@ static int expose_firmware_sysfs(struct intel_gvt *gvt)
p = firmware + h->cfg_space_offset;
- for (i = 0; i < h->cfg_space_size; i += 4)
- pci_read_config_dword(pdev, i, p + i);
-
- memcpy(gvt->firmware.cfg_space, p, info->cfg_space_size);
+ memcpy(p, gvt->firmware.cfg_space, info->cfg_space_size);
p = firmware + h->mmio_offset;
- /* Take a snapshot of hw mmio registers. */
- intel_gvt_for_each_tracked_mmio(gvt, mmio_snapshot_handler, p);
-
- memcpy(gvt->firmware.mmio, p, info->mmio_size);
+ memcpy(p, gvt->firmware.mmio, info->mmio_size);
crc32_start = offsetof(struct gvt_firmware_header, crc32) + 4;
h->crc32 = crc32_le(0, firmware + crc32_start, size - crc32_start);
@@ -142,9 +130,6 @@ void intel_gvt_free_firmware(struct intel_gvt *gvt)
{
if (!gvt->firmware.firmware_loaded)
clean_firmware_sysfs(gvt);
-
- kfree(gvt->firmware.cfg_space);
- vfree(gvt->firmware.mmio);
}
static int verify_firmware(struct intel_gvt *gvt,
@@ -217,23 +202,6 @@ int intel_gvt_load_firmware(struct intel_gvt *gvt)
if (!path)
return -ENOMEM;
- mem = kmalloc(info->cfg_space_size, GFP_KERNEL);
- if (!mem) {
- kfree(path);
- return -ENOMEM;
- }
-
- firmware->cfg_space = mem;
-
- mem = vmalloc(info->mmio_size);
- if (!mem) {
- kfree(path);
- kfree(firmware->cfg_space);
- return -ENOMEM;
- }
-
- firmware->mmio = mem;
-
sprintf(path, "%s/vid_0x%04x_did_0x%04x_rid_0x%02x.golden_hw_state",
GVT_FIRMWARE_PATH, pdev->vendor, pdev->device,
pdev->revision);
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
b/drivers/gpu/drm/i915/gvt/handlers.c
index 2c064da3db6d..ba7b330a2c71 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -2406,45 +2406,6 @@ int intel_gvt_setup_mmio_handlers(struct intel_gvt *gvt)
return ret;
}
-/**
- * intel_gvt_for_each_tracked_mmio - iterate each tracked mmio
- * @gvt: a GVT device
- * @handler: the handler
- * @data: private data given to handler
- *
- * Returns:
- * Zero on success, negative error code if failed.
- */
-int intel_gvt_for_each_tracked_mmio(struct intel_gvt *gvt,
- int (*handler)(struct intel_gvt *gvt, u32 offset, void *data),
- void *data)
-{
- struct gvt_mmio_block *block = gvt->mmio.mmio_block;
- struct intel_gvt_mmio_info *e;
- int i, j, ret;
-
- hash_for_each(gvt->mmio.mmio_info_table, i, e, node) {
- ret = handler(gvt, e->offset, data);
- if (ret)
- return ret;
- }
-
- for (i = 0; i < gvt->mmio.num_mmio_block; i++, block++) {
- /* pvinfo data doesn't come from hw mmio */
- if (i915_mmio_reg_offset(block->offset) == VGT_PVINFO_PAGE)
- continue;
-
- for (j = 0; j < block->size; j += 4) {
- ret = handler(gvt,
- i915_mmio_reg_offset(block->offset) + j,
- data);
- if (ret)
- return ret;
- }
- }
- return 0;
-}
-
/**
* intel_vgpu_default_mmio_read - default MMIO read handler
* @vgpu: a vGPU
diff --git a/drivers/gpu/drm/i915/gvt/mmio_info_table.c
b/drivers/gpu/drm/i915/gvt/mmio_info_table.c
index 723190c25313..76535e3cc9ba 100644
---
Re: [PATCH 07/11] dmaengine: qcom-adm: stop abusing slave_id config
Hi Arnd,
I love your patch! Perhaps something to improve:
[auto build test WARNING on vkoul-dmaengine/next]
[also build test WARNING on tiwai-sound/for-next staging/staging-testing
linus/master v5.16-rc1 next-2028]
[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]
url:
https://github.com/0day-ci/linux/commits/Arnd-Bergmann/dmaengine-kill-off-dma_slave_config-slave_id/2025-165731
base: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next
config: xtensa-buildonly-randconfig-r005-2025 (attached as .config)
compiler: xtensa-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/f2e7f9ee67ce784864f75db39f20d1060c932279
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Arnd-Bergmann/dmaengine-kill-off-dma_slave_config-slave_id/2025-165731
git checkout f2e7f9ee67ce784864f75db39f20d1060c932279
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross
ARCH=xtensa
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/dma/qcom/qcom_adm.c:712:18: warning: no previous prototype for
>> 'adm_dma_xlate' [-Wmissing-prototypes]
712 | struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
| ^
vim +/adm_dma_xlate +712 drivers/dma/qcom/qcom_adm.c
700
701 /**
702 * adm_dma_xlate
703 * @dma_spec: pointer to DMA specifier as found in the device tree
704 * @ofdma: pointer to DMA controller data
705 *
706 * This can use either 1-cell or 2-cell formats, the first cell
707 * identifies the slave device, while the optional second cell
708 * contains the crci value.
709 *
710 * Returns pointer to appropriate dma channel on success or NULL on
error.
711 */
> 712 struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
713 struct of_dma *ofdma)
714 {
715 struct dma_device *dev = ofdma->of_dma_data;
716 struct dma_chan *chan, *candidate = NULL;
717 struct adm_chan *achan;
718
719 if (!dev || dma_spec->args_count > 2)
720 return NULL;
721
722 list_for_each_entry(chan, >channels, device_node)
723 if (chan->chan_id == dma_spec->args[0]) {
724 candidate = chan;
725 break;
726 }
727
728 if (!candidate)
729 return NULL;
730
731 achan = to_adm_chan(candidate);
732 if (dma_spec->args_count == 2)
733 achan->crci = dma_spec->args[1];
734 else
735 achan->crci = 0;
736
737 return dma_get_slave_channel(candidate);
738 }
739
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[Bug 211807] [drm:drm_dp_mst_dpcd_read] *ERROR* mstb 000000004e6288dd port 3: DPCD read on addr 0x60 for 1 bytes NAKed
https://bugzilla.kernel.org/show_bug.cgi?id=211807 Daan (daandelomba...@gmail.com) changed: What|Removed |Added CC||daandelomba...@gmail.com --- Comment #14 from Daan (daandelomba...@gmail.com) --- I also had this in my logs yesterday, right before my system locked completely (had to do a hard reset). I also have a Lenovo Thinkpad Thunderbolt Dock, which connects my Thinkpad T480 with two external monitors (one Hdmi, one Displayport). I didn't have this lockup before, in the past both monitors were hdmi so maybe Displayport is the culprit here? Exact message: i915 :00:02.0: [drm] *ERROR* mstb afedfd20 port 3: DPCD read on addr 0x4b0 for 1 bytes NAKed Setup: Thinkpad T480, i7, UHD Graphics 620 + Nvidia Mx150 Fedora (5.14.16-201.fc34.x86_64) -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[PATCH] drm/hyperv: Fix device removal on Gen1 VMs
The Hyper-V DRM driver tries to free MMIO region on removing
the device regardless of VM type, while Gen1 VMs don't use MMIO
and hence causing the kernel to crash on a NULL pointer dereference.
Fix this by making deallocating MMIO only on Gen2 machines and implement
removal for Gen1
Fixes: 76c56a5affeb ("drm/hyperv: Add DRM driver for hyperv synthetic video
device")
Signed-off-by: Mohammed Gamal
---
drivers/gpu/drm/hyperv/hyperv_drm_drv.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index cd818a629183..9f923beb7d8d 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -225,12 +225,29 @@ static int hyperv_vmbus_remove(struct hv_device *hdev)
{
struct drm_device *dev = hv_get_drvdata(hdev);
struct hyperv_drm_device *hv = to_hv(dev);
+ struct pci_dev *pdev;
drm_dev_unplug(dev);
drm_atomic_helper_shutdown(dev);
vmbus_close(hdev->channel);
hv_set_drvdata(hdev, NULL);
- vmbus_free_mmio(hv->mem->start, hv->fb_size);
+
+ /*
+* Free allocated MMIO memory only on Gen2 VMs.
+* On Gen1 VMs, release the PCI device
+*/
+ if (efi_enabled(EFI_BOOT)) {
+ vmbus_free_mmio(hv->mem->start, hv->fb_size);
+ } else {
+ pdev = pci_get_device(PCI_VENDOR_ID_MICROSOFT,
+ PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
+ if (!pdev) {
+ drm_err(dev, "Unable to find PCI Hyper-V video\n");
+ return -ENODEV;
+ }
+ pci_release_region(pdev, 0);
+ pci_dev_put(pdev);
+ }
return 0;
}
--
2.33.1
Re: [PATCH v5 3/6] drm: Move nomodeset kernel parameter to the DRM subsystem
On Fri, Nov 19, 2021 at 11:20:13AM +0100, Javier Martinez Canillas wrote: > [ adding gregkh to Cc list for drivers/video/console ] > > On 11/12/21 14:32, Javier Martinez Canillas wrote: > > The "nomodeset" kernel cmdline parameter is handled by the vgacon driver > > but the exported vgacon_text_force() symbol is only used by DRM drivers. > > > > It makes much more sense for the parameter logic to be in the subsystem > > of the drivers that are making use of it. > > > > Let's move the vgacon_text_force() function and related logic to the DRM > > subsystem. While doing that, rename it to drm_firmware_drivers_only() and > > make it return true if "nomodeset" was used and false otherwise. This is > > a better description of the condition that the drivers are testing for. > > > > Suggested-by: Daniel Vetter > > Signed-off-by: Javier Martinez Canillas > > Acked-by: Thomas Zimmermann > > Acked-by: Jani Nikula > > Acked-by: Pekka Paalanen > > --- > > > > Greg, could I please get your ack for this patch ? Acked-by: Greg Kroah-Hartman
Re: (subset) [PATCH] drm/vc4: fix error code in vc4_create_object()
On Thu, 18 Nov 2021 14:14:16 +0300, Dan Carpenter wrote: > The ->gem_create_object() functions are supposed to return NULL if there > is an error. None of the callers expect error pointers so returing one > will lead to an Oops. See drm_gem_vram_create(), for example. > > Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
Re: (subset) [PATCH] drm/aspeed: Fix vga_pw sysfs output
On Wed, 17 Nov 2021 09:01:45 +0800, Joel Stanley wrote: > Before the drm driver had support for this file there was a driver that > exposed the contents of the vga password register to userspace. It would > present the entire register instead of interpreting it. > > The drm implementation chose to mask of the lower bit, without explaining > why. This breaks the existing userspace, which is looking for 0xa8 in > the lower byte. > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, 18 Nov 2021 17:46:10 -0800 Brian Norris wrote: > Hi Pekka, > > Thanks for the thoughts and review. I've tried to respond below: > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > On Wed, 17 Nov 2021 14:48:40 -0800 > > Brian Norris wrote: > > > > > A variety of applications have found it useful to listen to > > > user-initiated input events to make decisions within a DRM driver, given > > > that input events are often the first sign that we're going to start > > > doing latency-sensitive activities: > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > >with an input_handler boost, that preemptively exits self-refresh > > >whenever there is input activity. > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > >render new frames immediately after user activity. Powering up the > > >GPU can take enough time that it is worthwhile to start this process > > >as soon as there is input activity. Many Chrome OS systems also ship > > >with an input_handler boost that powers up the GPU. > > > > > > This patch provides a small helper library that abstracts some of the > > > input-subsystem details around picking which devices to listen to, and > > > some other boilerplate. This will be used in the next patch to implement > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > have been carrying for a while. > > > > > > Signed-off-by: Brian Norris > > > --- > > > > Thanks Simon for the CC. > > > > Hi Brian, > > > > while this feature in general makes sense and sounds good, to start > > warming up display hardware early when something might start to happen, > > this particular proposal has many problems from UAPI perspective (as it > > has none). Comments below. > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > from this input event watching? I would imagine the improvement to not > > be noticeable. > > Patch 2 has details. It's not really about precisely how slow PSR is, > but how much foresight we can gain: in patch 2, I note that with my > particular user space and system, I can start PSR-exit 50ms earlier than > I would otherweise. (FWIW, this measurement is exactly the same it was > with the original version written 4 years ago.) > > For how long PSR-exit takes: the measurements I'm able to do (via > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > manages to hide nearly 100% of that latency. > > Typical use cases where one notices PSR latency (and where this 35-55ms > matters) involve simply moving a cursor; it's very noticeable when you > have more than a few frames of latency to "get started". Hi Brian, that is very interesting, thanks. I would never have expected to have userspace take *that* long to react. But, that sounds like it could be just your userspace software stack. > > I think some numbers about how much this feature helps would be really > > good, even if they are quite specific use cases. You also need to > > identify the userspace components, because I think different display > > servers are very different in their reaction speed. > > If my email address isn't obvious, I'm testing Chrome OS. I'm frankly > not that familiar with the user space display stack, but for what I > know, it's rather custom, developed within the Chromium project. Others > on CC here could probably give you more detail, if you want specific > answers, besides docs like this: > > https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > > > If KMS gets a pageflip or modeset in no time after an input event, then > > what's the gain. OTOH, if the display server is locking on to vblank, > > there might be a delay worth avoiding. But then, is it worth > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > userspace could hit to start the warming up process? > > Rob responded to the first part to some extent (there is definitely gain > to be had). > > To the last part: I wrote a simple debugfs hook to allow user space to > force a PSR exit, and then a simple user space program to read input > events and smash that debugfs file whenever it sees one. Testing in the > same scenarios, this appears to lose less than 100 microseconds versus > the in-kernel approach, which is negligible for this use case. (I'm not > sure about the other use cases.) > > So, this is technically doable in user space. This is crucial information I would like you to include in some commit message. I think it is
Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero
On Fri, Nov 19, 2021 at 12:03:00PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
> > On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
> > > On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
> > > > Do a sanity check on struct drm_format_info hsub and vsub values to
> > > > avoid divide by zero.
> > > >
> > > > Syzkaller reported a divide error in framebuffer_check() when the
> > > > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via
> > > > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for
> > > > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0".
> > > > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses
> > > > vsub as a divisor. These divisors need to be sanity checked for
> > > > zero before use.
> > > >
> > > > divide error: [#1] SMP KASAN NOPTI
> > > > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1
> > > > Hardware name: Red Hat KVM, BIOS 1.13.0-2
> > > > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199
> > > > [inline]
> > > > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90
> > > > drivers/gpu/drm/drm_framebuffer.c:317
> > > >
> > > > Call Trace:
> > > > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355
> > > > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391
> > > > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795
> > > > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898
> > > > vfs_ioctl fs/ioctl.c:51 [inline]
> > > > __do_sys_ioctl fs/ioctl.c:874 [inline]
> > > > __se_sys_ioctl fs/ioctl.c:860 [inline]
> > > > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860
> > > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > > > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80
> > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > >
> > > > Signed-off-by: George Kennedy
> > > > ---
> > > > drivers/gpu/drm/drm_framebuffer.c | 10 ++
> > > > 1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> > > > b/drivers/gpu/drm/drm_framebuffer.c
> > > > index 07f5abc..a146e4b 100644
> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device
> > > > *dev,
> > > > /* now let the driver pick its own format info */
> > > > info = drm_get_format_info(dev, r);
> > > >
> > > > + if (info->hsub == 0) {
> > > > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor
> > > > %u\n", info->hsub);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (info->vsub == 0) {
> > > > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor
> > > > %u\n", info->vsub);
> > > > + return -EINVAL;
> > > > + }
> > >
> > > Looks like duct tape to me. I think we need to either fix those formats
> > > to have valid format info, or just revert the whole patch that added such
> > > broken things.
> >
> > Yeah maybe even a compile-time check of the format table(s) to validate
> > them properly and scream ... Or at least a selftest.
>
> I really wish C had (even very limited) compile time evaluation
> so one could actually loop over arrays like at compile time to
> check each element. As it stands you either have to check each
> array element by hand, or you do some cpp macro horrors to
> pretend you're iterating the array.
Python preprocess or so seems to be the usual answer, and that then just
generates the C table after it's all checked.
Or a post-processor which fishes the table out from the .o (or just links
against it).
But yeah doing this in cpp isn't going to work, aside from it'd be really
ugly.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH v2 0/6] drm/vc4: kms: Misc fixes for HVS commits
Maxime Ripard 於 2021年11月18日 週四 下午6:40寫道： > > On Thu, Nov 18, 2021 at 02:42:58PM +0800, Jian-Hong Pan wrote: > > Maxime Ripard 於 2021年11月17日 週三 下午5:45寫道： > > > The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: > > > Convert to > > > atomic helpers") introduced a number of issues in corner cases, most of > > > them > > > showing themselves in the form of either a vblank timeout or > > > use-after-free > > > error. > > > > > > These patches should fix most of them, some of them still being debugged. > > > > > > Maxime > > > > > > Changes from v1: > > > - Prevent a null pointer dereference > > > > > > Maxime Ripard (6): > > > drm/vc4: kms: Wait for the commit before increasing our clock rate > > > drm/vc4: kms: Fix return code check > > > drm/vc4: kms: Add missing drm_crtc_commit_put > > > drm/vc4: kms: Clear the HVS FIFO commit pointer once done > > > drm/vc4: kms: Don't duplicate pending commit > > > drm/vc4: kms: Fix previous HVS commit wait > > > > > > drivers/gpu/drm/vc4/vc4_kms.c | 42 --- > > > 1 file changed, 19 insertions(+), 23 deletions(-) > > > > I tested the v2 patches based on latest mainline kernel with RPi 4B. > > System can boot up into desktop environment. > > So the thing that was broken initially isn't anymore? No. It does not appear anymore. > > Although it still hit the bug [1], which might be under debugging, I > > think these patches LGTM. > > The vblank timeout stuff is a symptom of various different bugs. Can you > share your setup, your config.txt, and what you're doing to trigger it? I get the RPi boot firmware files from raspberrypi's repository at tag 1.20211029 [1] And, make system boots: RPi firmware -> U-Boot -> Linux kernel (aarch64) with corresponding DTB The config.txt only has: enable_uart=1 arm_64bit=1 kernel=uboot.bin This bug can be reproduced with es2gears command easily. May need to wait it running a while. Mesa: 21.2.2 libdrm: 2.4.107 xserver/wayland: 1.20.11 Using wayland This bug happens with direct boot path as well: RPi firmware -> Linux kernel (aarch64) with corresponding DTB I tried with Endless OS and Ubuntu's RPi 4B images. An easy setup is using Ubuntu 21.10 RPi 4B image [2]. Then, replace the kernel & device tree blob and edit the config.txt. [1] https://github.com/raspberrypi/firmware/tree/1.20211029/boot [2] https://ubuntu.com/download/raspberry-pi Jian-Hong Pan > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=214991 > > > > Tested-by: Jian-Hong Pan > > Thanks! > Maxime
Re: [PATCH v5 3/6] drm: Move nomodeset kernel parameter to the DRM subsystem
[ adding gregkh to Cc list for drivers/video/console ]
On 11/12/21 14:32, Javier Martinez Canillas wrote:
> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
> but the exported vgacon_text_force() symbol is only used by DRM drivers.
>
> It makes much more sense for the parameter logic to be in the subsystem
> of the drivers that are making use of it.
>
> Let's move the vgacon_text_force() function and related logic to the DRM
> subsystem. While doing that, rename it to drm_firmware_drivers_only() and
> make it return true if "nomodeset" was used and false otherwise. This is
> a better description of the condition that the drivers are testing for.
>
> Suggested-by: Daniel Vetter
> Signed-off-by: Javier Martinez Canillas
> Acked-by: Thomas Zimmermann
> Acked-by: Jani Nikula
> Acked-by: Pekka Paalanen
> ---
>
Greg, could I please get your ack for this patch ?
> Changes in v5:
> - Rename drm_get_modeset() to drm_firmware_drivers_only().
>
> Changes in v3:
> - Drop the drm_drv_enabled() function and just call to drm_get_modeset().
> - Make drm_get_modeset() just a getter function and don't return an error.
> - Move independent cleanups in drivers as separate preparatory patches.
>
> Changes in v2:
> - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
> - Squash patches to move nomodeset logic to DRM and do the renaming.
> - Name the function drm_check_modeset() and make it return -ENODEV.
> - Squash patch to add drm_drv_enabled() and make drivers use it.
> - Make the drivers changes before moving nomodeset logic to DRM.
> - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
> - Remove debug and error messages in drivers.
>
> drivers/gpu/drm/Makefile| 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +--
> drivers/gpu/drm/ast/ast_drv.c | 3 +--
> drivers/gpu/drm/drm_nomodeset.c | 26 +
> drivers/gpu/drm/i915/i915_module.c | 4 ++--
> drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +--
> drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++--
> drivers/gpu/drm/qxl/qxl_drv.c | 3 +--
> drivers/gpu/drm/radeon/radeon_drv.c | 3 +--
> drivers/gpu/drm/tiny/bochs.c| 3 +--
> drivers/gpu/drm/tiny/cirrus.c | 4 ++--
> drivers/gpu/drm/vboxvideo/vbox_drv.c| 3 +--
> drivers/gpu/drm/virtio/virtgpu_drv.c| 3 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +--
> drivers/video/console/vgacon.c | 21
> include/drm/drm_drv.h | 5 +
> include/linux/console.h | 6 --
> 17 files changed, 48 insertions(+), 51 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>
> diff --git drivers/gpu/drm/Makefile drivers/gpu/drm/Makefile
> index 1c41156deb5f..c74810c285af 100644
> --- drivers/gpu/drm/Makefile
> +++ drivers/gpu/drm/Makefile
> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
> drm_privacy_screen_x86.
>
> obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>
> +obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
> +
> drm_cma_helper-y := drm_gem_cma_helper.o
> obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
>
> diff --git drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 289d04999ced..fc761087c358 100644
> --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -31,7 +31,6 @@
> #include "amdgpu_drv.h"
>
> #include
> -#include
> #include
> #include
> #include
> @@ -2514,7 +2513,7 @@ static int __init amdgpu_init(void)
> {
> int r;
>
> - if (vgacon_text_force())
> + if (drm_firmware_drivers_only())
> return -EINVAL;
>
> r = amdgpu_sync_init();
> diff --git drivers/gpu/drm/ast/ast_drv.c drivers/gpu/drm/ast/ast_drv.c
> index 86d5cd7b6318..6d8613f6fe1c 100644
> --- drivers/gpu/drm/ast/ast_drv.c
> +++ drivers/gpu/drm/ast/ast_drv.c
> @@ -26,7 +26,6 @@
> * Authors: Dave Airlie
> */
>
> -#include
> #include
> #include
>
> @@ -233,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
>
> static int __init ast_init(void)
> {
> - if (vgacon_text_force() && ast_modeset == -1)
> + if (drm_firmware_drivers_only() && ast_modeset == -1)
> return -EINVAL;
>
> if (ast_modeset == 0)
> diff --git drivers/gpu/drm/drm_nomodeset.c drivers/gpu/drm/drm_nomodeset.c
> new file mode 100644
> index ..287edfb18b5d
> --- /dev/null
> +++ drivers/gpu/drm/drm_nomodeset.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include
> +#include
> +
> +static bool drm_nomodeset;
> +
> +bool drm_firmware_drivers_only(void)
> +{
> + return drm_nomodeset;
> +}
> +EXPORT_SYMBOL(drm_firmware_drivers_only);
> +
> +static int __init disable_modeset(char *str)
> +{
> + drm_nomodeset = true;
> +
> + pr_warn("You have booted with nomodeset. This means your GPU
[syzbot] KASAN: vmalloc-out-of-bounds Write in imageblit (2)
Hello, syzbot found the following issue on: HEAD commit:fa55b7dcdc43 Linux 5.16-rc1 git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=15fe2569b0 kernel config: https://syzkaller.appspot.com/x/.config?x=6d3b8fd1977c1e73 dashboard link: https://syzkaller.appspot.com/bug?extid=14b0e8f3fd1612e35350 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 userspace arch: i386 Unfortunately, I don't have any reproducer for this issue yet. IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+14b0e8f3fd1612e35...@syzkaller.appspotmail.com 524155 pages RAM 0 pages HighMem/MovableOnly 163742 pages reserved 0 pages cma reserved == BUG: KASAN: vmalloc-out-of-bounds in fast_imageblit drivers/video/fbdev/core/sysimgblt.c:229 [inline] BUG: KASAN: vmalloc-out-of-bounds in sys_imageblit+0x12f4/0x1430 drivers/video/fbdev/core/sysimgblt.c:275 Write of size 4 at addr c90004631000 by task syz-executor.0/7913 CPU: 0 PID: 7913 Comm: syz-executor.0 Not tainted 5.16.0-rc1-syzkaller #0 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 print_address_description.constprop.0.cold+0xf/0x320 mm/kasan/report.c:247 __kasan_report mm/kasan/report.c:433 [inline] kasan_report.cold+0x83/0xdf mm/kasan/report.c:450 fast_imageblit drivers/video/fbdev/core/sysimgblt.c:229 [inline] sys_imageblit+0x12f4/0x1430 drivers/video/fbdev/core/sysimgblt.c:275 drm_fb_helper_sys_imageblit drivers/gpu/drm/drm_fb_helper.c:794 [inline] drm_fbdev_fb_imageblit+0x15c/0x350 drivers/gpu/drm/drm_fb_helper.c:2282 bit_putcs_unaligned drivers/video/fbdev/core/bitblit.c:124 [inline] bit_putcs+0x6e1/0xd20 drivers/video/fbdev/core/bitblit.c:173 fbcon_putcs+0x353/0x440 drivers/video/fbdev/core/fbcon.c:1277 do_update_region+0x399/0x630 drivers/tty/vt/vt.c:676 redraw_screen+0x61f/0x740 drivers/tty/vt/vt.c:1035 fbcon_modechanged+0x58c/0x6c0 drivers/video/fbdev/core/fbcon.c:2182 fbcon_update_vcs+0x3a/0x50 drivers/video/fbdev/core/fbcon.c:2227 do_fb_ioctl+0x62e/0x690 drivers/video/fbdev/core/fbmem.c:1114 fb_compat_ioctl+0x17e/0x610 drivers/video/fbdev/core/fbmem.c:1313 __do_compat_sys_ioctl+0x1c7/0x290 fs/ioctl.c:972 do_syscall_32_irqs_on arch/x86/entry/common.c:112 [inline] __do_fast_syscall_32+0x65/0xf0 arch/x86/entry/common.c:178 do_fast_syscall_32+0x2f/0x70 arch/x86/entry/common.c:203 entry_SYSENTER_compat_after_hwframe+0x4d/0x5c RIP: 0023:0xf6e67549 Code: 03 74 c0 01 10 05 03 74 b8 01 10 06 03 74 b4 01 10 07 03 74 b0 01 10 08 03 74 d8 01 00 00 00 00 00 51 52 55 89 e5 0f 34 cd 80 <5d> 5a 59 c3 90 90 90 90 8d b4 26 00 00 00 00 8d b4 26 00 00 00 00 RSP: 002b:f44615fc EFLAGS: 0296 ORIG_RAX: 0036 RAX: ffda RBX: 0005 RCX: 4601 RDX: 2000 RSI: RDI: RBP: R08: R09: R10: R11: R12: R13: R14: R15: Memory state around the buggy address: c90004630f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 c90004630f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 >c90004631000: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 ^ c90004631080: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 c90004631100: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 == Code disassembly (best guess): 0: 03 74 c0 01 add0x1(%rax,%rax,8),%esi 4: 10 05 03 74 b8 01 adc%al,0x1b87403(%rip)# 0x1b8740d a: 10 06 adc%al,(%rsi) c: 03 74 b4 01 add0x1(%rsp,%rsi,4),%esi 10: 10 07 adc%al,(%rdi) 12: 03 74 b0 01 add0x1(%rax,%rsi,4),%esi 16: 10 08 adc%cl,(%rax) 18: 03 74 d8 01 add0x1(%rax,%rbx,8),%esi 1c: 00 00 add%al,(%rax) 1e: 00 00 add%al,(%rax) 20: 00 51 52add%dl,0x52(%rcx) 23: 55 push %rbp 24: 89 e5 mov%esp,%ebp 26: 0f 34 sysenter 28: cd 80 int$0x80 * 2a: 5d pop%rbp <-- trapping instruction 2b: 5a pop%rdx 2c: 59 pop%rcx 2d: c3 retq 2e: 90 nop 2f: 90 nop 30: 90 nop 31: 90 nop 32: 8d b4 26 00 00 00 00lea0x0(%rsi,%riz,1),%esi
Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero
On Fri, Nov 19, 2021 at 10:40:38AM +0100, Daniel Vetter wrote:
> On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
> > On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
> > > Do a sanity check on struct drm_format_info hsub and vsub values to
> > > avoid divide by zero.
> > >
> > > Syzkaller reported a divide error in framebuffer_check() when the
> > > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via
> > > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for
> > > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0".
> > > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses
> > > vsub as a divisor. These divisors need to be sanity checked for
> > > zero before use.
> > >
> > > divide error: [#1] SMP KASAN NOPTI
> > > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1
> > > Hardware name: Red Hat KVM, BIOS 1.13.0-2
> > > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline]
> > > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90
> > > drivers/gpu/drm/drm_framebuffer.c:317
> > >
> > > Call Trace:
> > > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355
> > > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391
> > > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795
> > > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898
> > > vfs_ioctl fs/ioctl.c:51 [inline]
> > > __do_sys_ioctl fs/ioctl.c:874 [inline]
> > > __se_sys_ioctl fs/ioctl.c:860 [inline]
> > > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860
> > > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80
> > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > >
> > > Signed-off-by: George Kennedy
> > > ---
> > > drivers/gpu/drm/drm_framebuffer.c | 10 ++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> > > b/drivers/gpu/drm/drm_framebuffer.c
> > > index 07f5abc..a146e4b 100644
> > > --- a/drivers/gpu/drm/drm_framebuffer.c
> > > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev,
> > > /* now let the driver pick its own format info */
> > > info = drm_get_format_info(dev, r);
> > >
> > > + if (info->hsub == 0) {
> > > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n",
> > > info->hsub);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + if (info->vsub == 0) {
> > > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n",
> > > info->vsub);
> > > + return -EINVAL;
> > > + }
> >
> > Looks like duct tape to me. I think we need to either fix those formats
> > to have valid format info, or just revert the whole patch that added such
> > broken things.
>
> Yeah maybe even a compile-time check of the format table(s) to validate
> them properly and scream ... Or at least a selftest.
I really wish C had (even very limited) compile time evaluation
so one could actually loop over arrays like at compile time to
check each element. As it stands you either have to check each
array element by hand, or you do some cpp macro horrors to
pretend you're iterating the array.
--
Ville Syrjälä
Intel
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote:
> Hi Daniel,
>
> Thanks for the review. Lots to address elsewhere, but I can respond
> here first:
>
> On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST
> > >
> > > If in doubt, say "N".
> > >
> > > +config DRM_INPUT_HELPER
> > > + def_bool y
> > > + depends on DRM_KMS_HELPER
> > > + depends on INPUT
> >
> > Uh please no configs for each thing, it just makes everything more
> > complex. Do we _really_ need this?
>
> First, it's not a configurable option (a user will never see this nor
> have to answer Y/N to it); it only serves as an intermediary to express
> the CONFIG_INPUT dependency (which is necessary) without making
> DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run
> display stacks without the input subsystem.)
I'm not so much worried about the user cost, but the maintenance cost.
Kbuild config complexity is ridiculous, anything that adds even a bit is
really silly.
> The closest alternative I can think of with fewer Kconfig symbols is to
> just use CONFIG_INPUT directly in the code, to decide whether to provide
> the helpers or else just stub them out. But that has a problem of not
> properly expressing the =m vs. =y necessity: if, for example,
> CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker
> issues.
Usually this is done by providing static inline dummy implementations in
the headers. That avoids having to sprinkle new Kconfig symbols all over.
> In short, yes, I think we really need this. But I'm not a Kbuild expert.
>
> > > diff --git a/include/drm/drm_input_helper.h
> > > b/include/drm/drm_input_helper.h
> > > new file mode 100644
> > > index ..7904f397b934
> > > --- /dev/null
> > > +++ b/include/drm/drm_input_helper.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2021 Google, Inc.
> > > + */
> > > +#ifndef __DRM_INPUT_HELPER_H__
> > > +#define __DRM_INPUT_HELPER_H__
> > > +
> > > +#include
> > > +
> > > +struct drm_device;
> > > +
> > > +struct drm_input_handler {
> > > + /*
> > > + * Callback to call for input activity. Will be called in an atomic
> > > + * context.
> >
> > How atomic? Like hardirq, and nasty spinlocks held?
>
> Maybe I should have just cribbed off the doc:
>
> * @event: event handler. This method is being called by input core with
> * interrupts disabled and dev->event_lock spinlock held and so
> * it may not sleep
>
> I probably don't want to propagate the subsystem details about which
> locks, but I guess I can be specific about "interrupts disabled" and
> "don't sleep".
You can also do hyperlinks in the generated htmldocs and just reference
that:
https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#highlights-and-cross-references
>
> > > + */
> > > + void (*callback)(struct drm_input_handler *handler);
> > > +
> > > + struct input_handler handler;
> > > +};
> > > +
> > > +#if defined(CONFIG_DRM_INPUT_HELPER)
> > > +
> > > +int drm_input_handle_register(struct drm_device *dev,
> > > + struct drm_input_handler *handler);
> > > +void drm_input_handle_unregister(struct drm_input_handler *handler);
> > > +
> > > +#else /* !CONFIG_DRM_INPUT_HELPER */
> > > +
> > > +static inline int drm_input_handle_register(struct drm_device *dev,
> > > + struct drm_input_handler *handler)
> > > +{
> > > + return 0;
> > > +}
> >
> > I guess the reason behind the helper is that you also want to use this in
> > drivers or maybe drm/sched?
>
> I think my reasoning is heavily described in both the cover letter and
> the commit message. If that's not clear, can you point out which part?
> I'd gladly improve it :)
>
> But specifically, see the 2nd bullet from the commit message, which I've
> re-quoted down here:
>
> > > * GPU drivers: on GPU-accelerated desktop systems, we may need to
> > >render new frames immediately after user activity. Powering up the
> > >GPU can take enough time that it is worthwhile to start this process
> > >as soon as there is input activity. Many Chrome OS systems also ship
> > >with an input_handler boost that powers up the GPU.
>
> Rob Clark has patches to drm/msm to boost GPU power-up via a similar
> helper.
Yeah this question was just for confirmation, might be good to include
that other patch set too for the full picture.
> > Anyway I think it looks all reasonable. Definitely need an ack from input
> > people
>
> I realized I failed to carry Dmitry's Ack from version 1 [1]. If this
> has a v3 in similar form, I'll carry it there.
>
> > that the event list you have is a good choice, I have no idea what
> > that all does. Maybe also document that part a bit more.
>
RE: [PATCH v3] drm/i915/rpm: Enable runtime pm autosuspend by default
> -Original Message-
> From: Tangudu, Tilak
> Sent: Tuesday, November 16, 2021 9:23 PM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Tangudu, Tilak ; Ewins, Jon
> ; Vivi, Rodrigo ; Nilawar, Badal
> ; Gupta, Anshuman ;
> Syrjala, Ville
> Subject: [PATCH v3] drm/i915/rpm: Enable runtime pm autosuspend by default
>
> v1: Enable runtime pm autosuspend by default for Gen12 and later versions.
>
> v2: Enable runtime pm autosuspend by default for all platforms(Syrjala Ville)
>
> v3: Change commit message(Nikula Jani)
> Let's enable runtime pm autosuspend by default everywhere.
> So, we can allow D3hot and bigger power savings on idle scenarios.
>
> But at this time let's not touch the autosuspend_delay time, what caused some
> regression on our previous attempt.
>
> Also, the latest identified issue on GuC PM has been fixed by commit
> 1a52faed3131 ("drm/i915/guc: Take GT PM ref when deregistering
> context")
>
> Signed-off-by: Tilak Tangudu
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 0d85f3c5c526..22dab36afcb6 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -590,6 +590,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm
> *rpm)
> pm_runtime_use_autosuspend(kdev);
> }
>
> + /* Enable by default */
> + pm_runtime_allow(kdev);
> +
BAT is failing due to soft lockup on SKL.
How about to enable the runtime PM only for discrete platforms till we fixes
all issues on Gen9 for hybrid gfx use cases.
(when discrete card will used only for rendering)
Thanks,
Anshuman Gupta.
> /*
>* The core calls the driver load handler with an RPM reference held.
>* We drop that here and will reacquire it during unloading in
> --
> 2.25.1
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, 18 Nov 2021 15:30:38 -0800 Rob Clark wrote: > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > Brian Norris wrote: > > > > > A variety of applications have found it useful to listen to > > > user-initiated input events to make decisions within a DRM driver, given > > > that input events are often the first sign that we're going to start > > > doing latency-sensitive activities: > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > >with an input_handler boost, that preemptively exits self-refresh > > >whenever there is input activity. > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > >render new frames immediately after user activity. Powering up the > > >GPU can take enough time that it is worthwhile to start this process > > >as soon as there is input activity. Many Chrome OS systems also ship > > >with an input_handler boost that powers up the GPU. > > > > > > This patch provides a small helper library that abstracts some of the > > > input-subsystem details around picking which devices to listen to, and > > > some other boilerplate. This will be used in the next patch to implement > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > have been carrying for a while. > > > > > > Signed-off-by: Brian Norris > > > --- > > > > Thanks Simon for the CC. > > > > Hi Brian, > > > > while this feature in general makes sense and sounds good, to start > > warming up display hardware early when something might start to happen, > > this particular proposal has many problems from UAPI perspective (as it > > has none). Comments below. > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > from this input event watching? I would imagine the improvement to not > > be noticeable. > > > > I think some numbers about how much this feature helps would be really > > good, even if they are quite specific use cases. You also need to > > identify the userspace components, because I think different display > > servers are very different in their reaction speed. > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > what's the gain. OTOH, if the display server is locking on to vblank, > > there might be a delay worth avoiding. But then, is it worth > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > userspace could hit to start the warming up process? > > In my measurements, it takes userspace a frame or two to respond and > get to the point of starting to build cmdstream (before eventually > doing atomic/pageflip ioctl).. possibly longer if you don't also have > a similar boost mechanism to spool up cpufreq > > But the important thing, IMO, is that atomic/pageflip ioctl is the > cumulation of a long sequence of events.. input-boost is letting > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > that long sequence. Right, exactly. That is why I was musing about a *new* ioctl that userspace could hit as soon as any input device fd (or network fd!) shows signs of life. Would that be enough, avoiding all the annoying questions about which input and DRM devices should participate here (and what about non-input devices that still want to trigger the warm-up, like network traffic, e.g. remote control?), or does it really need to be kernel internal to be fast enough? As Brian wrote about his quick hack to test that via debugfs, sounds like the userspace solution would be totally sufficient. Thanks, pq pgpzvK14VdrPL.pgp Description: OpenPGP digital signature
Re: [PATCH v2] drm/fb-helper: Call drm_fb_helper_hotplug_event() when releasing drm master
On Thu, Nov 18, 2021 at 11:23:32AM +0100, Michel Dänzer wrote:
> On 2021-11-08 17:00, Daniel Vetter wrote:
> > On Mon, Nov 08, 2021 at 04:34:53PM +0100, Jocelyn Falempe wrote:
> >> When using Xorg/Logind and an external monitor connected with an MST dock.
> >> After disconnecting the external monitor, switching to VT may not work,
> >> the (internal) monitor sill display Xorg, and you can't see what you are
> >> typing in the VT.
> >>
> >> This is related to commit e2809c7db818 ("drm/fb_helper: move deferred fb
> >> checking into restore mode (v2)")
> >>
> >> When switching to VT, with Xorg and logind, if there
> >> are pending hotplug event (like MST unplugged), the hotplug path
> >> may not be fulfilled, because logind may drop the master a bit later.
> >> It leads to the console not showing up on the monitor.
> >>
> >> So when dropping the drm master, call the delayed hotplug function if
> >> needed.
> >>
> >> v2: rewrote the patch by calling the hotplug function after dropping master
> >>
> >> Signed-off-by: Jocelyn Falempe
> >
> > Lastclose console restore is a very gross hack, and generally doesn't work
> > well.
> >
> > The way this is supposed to work is:
> > - userspace drops drm master (because drm master always wins)
> > - userspace switches the console back to text mode (which will restore the
> > console)
> >
> > I guess we could also do this from dropmaster once more (like from
> > lastclose), but that really feels like papering over userspace bugs. And
> > given what a massive mess this entire area is already, I'm not eager to
> > add more hacks here.
> >
> > So ... can't we fix userspace?
>
> Sounds like the good old UMS days, when VT switching relied on user space
> doing the right things in the right order.
>
> With KMS, surely the kernel needs to be able to get to a known good
> state from scratch when it's in control of the VT, no matter what state
> user space left things in.
Unfortunately not. When your in KD_GRAPHICS mode we explicitly tell fbcon
to shut up and not restore itself, and it shouldn't ever do that.
And afaik there's not really a holder concept for KD_TEXT/GRAPHICS, unlike
the drm master which signifies ownership of kms resources.
Which sucks ofc, but fixing this would mean we need to retrofit ownership
into VT somehow, so that ownership is auto-dropped like drm_master on
close()/exit(). Not sure that's possible without breaking uapi (e.g. with
logind I think it's logind also doing the KD_TEXT/GRAPHICS dance, but
didn't check).
But if we'd have some kind of real ownership for KD_GRAPHICS then we could
tie that to the implied/weak drm_master status of fbdev emulation and make
this work correctly. But as-is the kernel simply doesn't have enough
information to dtrt. Or at least I'm not seeing how exactly, without just
trying to make guesses what userspace wants to do.
Either way I think we need to really clearly spell out how this is all
supposed to work, and not just add random bandaids justified with "works
for me". It is already a byzantine mess as-is.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero
On Thu, Nov 18, 2021 at 03:00:15PM -0500, George Kennedy wrote: > > > On 10/29/2021 10:14 AM, Brian Starkey wrote: > > Hi, > > > > On Fri, Oct 29, 2021 at 09:15:28AM -0400, George Kennedy wrote: > > > Asking if you have any input on how to deal with hsub and vsub = zero? > > That's just a straight mistake on those formats - they should > > be 1. My bad for not spotting it in review. > > > > On the one hand, having formats in this table is a nice > > machine-readable way to describe them. On the other, as drm_fourcc is > > being used as the canonical repository for formats, including ones > > not used in DRM, we can end up with situations like this. > > (R10/R12 being another example of formats not used in DRM: > > 20211027233140.12268-1-laurent.pinch...@ideasonboard.com) > > Wondering if there is an alternate fix to the one proposed? I think if the cost of defining formats correctly for everyone is that drm carries a bunch of nice machine-readable entries in its tables that it never uses, then we should do that. The very few bytes saved aren't worth any headaches (because on any soc system you anyway have tons more formats than what your driver is using). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm: check drm_format_info hsub and vsub to avoid divide by zero
On Thu, Oct 28, 2021 at 05:04:19PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 28, 2021 at 08:57:17AM -0500, George Kennedy wrote:
> > Do a sanity check on struct drm_format_info hsub and vsub values to
> > avoid divide by zero.
> >
> > Syzkaller reported a divide error in framebuffer_check() when the
> > DRM_FORMAT_Q410 or DRM_FORMAT_Q401 pixel_format is passed in via
> > the DRM_IOCTL_MODE_ADDFB2 ioctl. The drm_format_info struct for
> > the DRM_FORMAT_Q410 pixel_pattern has ".hsub = 0" and ".vsub = 0".
> > fb_plane_width() uses hsub as a divisor and fb_plane_height() uses
> > vsub as a divisor. These divisors need to be sanity checked for
> > zero before use.
> >
> > divide error: [#1] SMP KASAN NOPTI
> > CPU: 0 PID: 14995 Comm: syz-executor709 Not tainted 5.15.0-rc6-syzk #1
> > Hardware name: Red Hat KVM, BIOS 1.13.0-2
> > RIP: 0010:framebuffer_check drivers/gpu/drm/drm_framebuffer.c:199 [inline]
> > RIP: 0010:drm_internal_framebuffer_create+0x604/0xf90
> > drivers/gpu/drm/drm_framebuffer.c:317
> >
> > Call Trace:
> > drm_mode_addfb2+0xdc/0x320 drivers/gpu/drm/drm_framebuffer.c:355
> > drm_mode_addfb2_ioctl+0x2a/0x40 drivers/gpu/drm/drm_framebuffer.c:391
> > drm_ioctl_kernel+0x23a/0x2e0 drivers/gpu/drm/drm_ioctl.c:795
> > drm_ioctl+0x589/0xac0 drivers/gpu/drm/drm_ioctl.c:898
> > vfs_ioctl fs/ioctl.c:51 [inline]
> > __do_sys_ioctl fs/ioctl.c:874 [inline]
> > __se_sys_ioctl fs/ioctl.c:860 [inline]
> > __x64_sys_ioctl+0x19d/0x220 fs/ioctl.c:860
> > do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > do_syscall_64+0x3a/0x80 arch/x86/entry/common.c:80
> > entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > Signed-off-by: George Kennedy
> > ---
> > drivers/gpu/drm/drm_framebuffer.c | 10 ++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_framebuffer.c
> > b/drivers/gpu/drm/drm_framebuffer.c
> > index 07f5abc..a146e4b 100644
> > --- a/drivers/gpu/drm/drm_framebuffer.c
> > +++ b/drivers/gpu/drm/drm_framebuffer.c
> > @@ -195,6 +195,16 @@ static int framebuffer_check(struct drm_device *dev,
> > /* now let the driver pick its own format info */
> > info = drm_get_format_info(dev, r);
> >
> > + if (info->hsub == 0) {
> > + DRM_DEBUG_KMS("bad horizontal chroma subsampling factor %u\n",
> > info->hsub);
> > + return -EINVAL;
> > + }
> > +
> > + if (info->vsub == 0) {
> > + DRM_DEBUG_KMS("bad vertical chroma subsampling factor %u\n",
> > info->vsub);
> > + return -EINVAL;
> > + }
>
> Looks like duct tape to me. I think we need to either fix those formats
> to have valid format info, or just revert the whole patch that added such
> broken things.
Yeah maybe even a compile-time check of the format table(s) to validate
them properly and scream ... Or at least a selftest.
-Daniel
>
> > +
> > for (i = 0; i < info->num_planes; i++) {
> > unsigned int width = fb_plane_width(r->width, info, i);
> > unsigned int height = fb_plane_height(r->height, info, i);
> > --
> > 1.8.3.1
>
> --
> Ville Syrjälä
> Intel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: [PATCH] drm/aspeed: Fix vga_pw sysfs output
Hi Joel, > Before the drm driver had support for this file there was a driver > that exposed the contents of the vga password register to userspace. > It would present the entire register instead of interpreting it. > > The drm implementation chose to mask of the lower bit, without > explaining why. This breaks the existing userspace, which is looking > for 0xa8 in the lower byte. > > Change our implementation to expose the entire register. As a userspace consumer of this: Reviewed-by: Jeremy Kerr Thanks! Jeremy
