Re: [PATCH v2 1/6] dt-bindings: display: Add yamls for JH7110 display system
On 29/11/2023 04:13, Keith Zhao wrote: > > > On 2023/10/25 20:50, Krzysztof Kozlowski wrote: >> On 25/10/2023 12:39, Keith Zhao wrote: >>> StarFive SoCs JH7110 display system: >> >> A nit, subject: drop second/last, redundant "yamls for". The >> "dt-bindings" prefix is already stating that these are bindings, so >> format is fixed. >> >>> lcd-controller bases verisilicon dc8200 IP, >>> and hdmi bases Innosilicon IP. Add bindings for them. >> >> Please make it a proper sentences, with proper wrapping. >> >>> >>> also update MAINTAINERS for dt-bindings >> >> Not a sentence, but also not really needed.ok I see. >> >>> >>> about this patch, I tested the dtbs_check and dt_binding_check >>> with the result pass. >>> Based on the feedback of the previous version, the corresponding >>> arrangement is made >> >> Not relevant, so not really suitable for commit msg. >> >>> >>> Signed-off-by: Keith Zhao >>> --- >>> .../starfive/starfive,display-subsystem.yaml | 41 +++ >>> .../starfive/starfive,jh7110-dc8200.yaml | 109 ++ >>> .../starfive/starfive,jh7110-inno-hdmi.yaml | 85 ++ >>> MAINTAINERS | 7 ++ >>> 4 files changed, 242 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml >>> create mode 100644 >>> Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml >>> create mode 100644 >>> Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml >>> >>> diff --git >>> a/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml >>> >>> b/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml >>> new file mode 100644 >>> index 0..f45b97b08 >>> --- /dev/null >>> +++ >>> b/Documentation/devicetree/bindings/display/starfive/starfive,display-subsystem.yaml >>> @@ -0,0 +1,41 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: >>> http://devicetree.org/schemas/display/starfive/starfive,display-subsystem.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Starfive DRM master device >> >> What is DRM in hardware? I know Digital Rights Management, but then >> subsystem seems wrong. If you mean Linux DRM, then Linux is not a >> hardware, so drop all Linuxisms and describe hardware. > ok , will only keep hardware describe in my next version >> >> >>> + >>> +maintainers: >>> + - Keith Zhao >>> + - ShengYang Chen >>> + >>> +description: >>> + The Starfive DRM master device is a virtual device needed to list all >> >> Virtual device? Then not suitable for bindings, sorry. >> >>> + display controller or other display interface nodes that comprise the >>> + graphics subsystem. >>> + >>> +properties: >>> + compatible: >>> +const: starfive,display-subsystem >>> + >>> + ports: >>> +$ref: /schemas/types.yaml#/definitions/phandle-array >> >> No, ports is not phandle-array. ports is object, always. >> >>> +description: >>> + Should contain a list of phandles pointing to display interface ports >>> + of display controller devices. Display controller definitions as >>> defined >>> + in Documentation/devicetree/bindings/display/starfive/ >>> + starfive,jh7110-dc8200.yaml >> >> Use standard graph ports, not some own, custom property. >> >> Anyway, entire binding should be dropped. You do not need it even. > Hi Krzysztof: > Virtual device is not suitable for bindings, matbe I need associate it with > the real hardware. > such as the top clocks & reset , irq , etc. > Currently I configure them in another yaml file. Logically speaking, this is > more suitable. > > Can adding the corresponding hardware description change its fate of being > deleted? I am not sure if I follow. Bindings and DTS describe the hardware, so if you configure device A clocks in a device B node, then it is not correct. If you add binding for something not being a real device, it is not correct. Feel free to bring proper hardware description, not Linux. This entire binding was written to describe Linux driver, which is not correct. Best regards, Krzysztof
RE: [RFC PATCH 0/6] Supporting GMEM (generalized memory management) for external memory devices
Glad to hear that more sharable code is desirable. IMHO, for a common MM subsystem, it is more beneficial for GMEM to extend core MM instead of building a separate one. As stated in the beginning of my RFC letter, MM systems are large and similar. Even a sophisticated one like Linux MM that has evolved over decades still suffers from an increasing number of bugs[1]. So, directly extending core MM to support devices not only avoids opening a new box of bugs, but also allows the community to concentrate on maintaining one single MM system. On the other side, GMEM does no hurt to core MM If a CPU process is not attached with device contexts. @Christian, could you provide more information on what AMD proposed with KFD and why it was rejected? [1] Huang, Jian, Moinuddin K. Qureshi, and Karsten Schwan. "An evolutionary study of linux memory management for fun and profit." 2016 USENIX Annual Technical Conference (USENIX ATC 16). 2016. Thanks, Weixi -Original Message- From: Dave Airlie Sent: Wednesday, November 29, 2023 1:15 PM To: Christian König Cc: zhuweixi ; linux...@kvack.org; linux-ker...@vger.kernel.org; a...@linux-foundation.org; weixi@openeuler.sh; mgor...@suse.de; jgli...@redhat.com; rcampb...@nvidia.com; jhubb...@nvidia.com; apop...@nvidia.com; mhairgr...@nvidia.com; z...@nvidia.com; alexander.deuc...@amd.com; xinhui@amd.com; amd-...@lists.freedesktop.org; felix.kuehl...@amd.com; ogab...@kernel.org; dri-devel@lists.freedesktop.org; j...@nvidia.com; leo...@nvidia.com; zhen...@linux.intel.com; zhi.a.w...@intel.com; intel-gvt-...@lists.freedesktop.org; intel-...@lists.freedesktop.org; jani.nik...@linux.intel.com; joonas.lahti...@linux.intel.com; rodrigo.v...@intel.com; tvrtko.ursu...@linux.intel.com Subject: Re: [RFC PATCH 0/6] Supporting GMEM (generalized memory management) for external memory devices On Tue, 28 Nov 2023 at 23:07, Christian König wrote: > > Am 28.11.23 um 13:50 schrieb Weixi Zhu: > > The problem: > > > > Accelerator driver developers are forced to reinvent external MM subsystems > > case by case, because Linux core MM only considers host memory resources. > > These reinvented MM subsystems have similar orders of magnitude of LoC as > > Linux MM (80K), e.g. Nvidia-UVM has 70K, AMD GPU has 14K and Huawei NPU has > > 30K. Meanwhile, more and more vendors are implementing their own > > accelerators, e.g. Microsoft's Maia 100. At the same time, > > application-level developers suffer from poor programmability -- they must > > consider parallel address spaces and be careful about the limited device > > DRAM capacity. This can be alleviated if a malloc()-ed virtual address can > > be shared by the accelerator, or the abundant host DRAM can further > > transparently backup the device local memory. > > > > These external MM systems share similar mechanisms except for the > > hardware-dependent part, so reinventing them is effectively introducing > > redundant code (14K~70K for each case). Such developing/maintaining is not > > cheap. Furthermore, to share a malloc()-ed virtual address, device drivers > > need to deeply interact with Linux MM via low-level MM APIs, e.g. MMU > > notifiers/HMM. This raises the bar for driver development, since developers > > must understand how Linux MM works. Further, it creates code maintenance > > problems -- any changes to Linux MM potentially require coordinated changes > > to accelerator drivers using low-level MM APIs. > > > > Putting a cache-coherent bus between host and device will not make these > > external MM subsystems disappear. For example, a throughput-oriented > > accelerator will not tolerate executing heavy memory access workload with > > a host MMU/IOMMU via a remote bus. Therefore, devices will still have > > their own MMU and pick a simpler page table format for lower address > > translation overhead, requiring external MM subsystems. > > > > > > > > What GMEM (Generalized Memory Management [1]) does: > > > > GMEM extends Linux MM to share its machine-independent MM code. Only > > high-level interface is provided for device drivers. This prevents > > accelerator drivers from reinventing the wheel, but relies on drivers to > > implement their hardware-dependent functions declared by GMEM. GMEM's key > > interface include gm_dev_create(), gm_as_create(), gm_as_attach() and > > gm_dev_register_physmem(). Here briefly describe how a device driver > > utilizes them: > > 1. At boot time, call gm_dev_create() and registers the implementation of > > hardware-dependent functions as declared in struct gm_mmu. > > - If the device has local DRAM, call gm_dev_register_physmem() to > > register available physical addresses. > > 2. When a device context is initialized (e.g. triggered by ioctl), check if > > the current CPU process has been attached to a gmem address space > > (struct gm_as). If not, call gm_as_create() and point current->mm->gm_as > > to it. > > 3.
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi,
On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> wrote:
> > Geert Uytterhoeven writes:
> > > The Imagination Technologies PowerVR Series 6 GPU is currently only
> > > supported on Texas Instruments K3 AM62x SoCs. Hence add a dependency on
> > > ARCH_K3, to prevent asking the user about this driver when configuring a
> > > kernel without Texas Instruments K3 Multicore SoC support.
> > >
> > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR driver")
> > > Signed-off-by: Geert Uytterhoeven
> > > ---
> >
> > Indeed. Although I wonder what is the supposed policy since for example
> > the DRM_PANFROST symbol only depends on ARM || ARM64 and others such as
>
> I think ARM Mali is sufficiently ubiquitous on ARM/ARM64 systems to
> have just an ARM/ARM64 dependency...
>
> > DRM_ETNAVIV don't even have an SoC or architecture dependency.
>
> Vivante GPUs are found in DTS files on at least 4 architectures.
> Might be worthwhile to add some dependencies, though...
>
> > In any case, I agree with you that restricting to only K3 makes sense.
>
> I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> eventually ;-)
I disagree. This is to handle a generic IP, just like panfrost, lima, or
etnaviv, and we certaintly don't want to maintain the Kconfig list of
every possible architecture and SoC family it might or might not be
found.
GPUs supposed to be handled are spread across 4 architectures (x86,
riscv, arm, arm64, mips?), and in arm/arm64 alone we have at least 5
platforms that might use it (allwinner, ti, mediatek, renesas, rockchip)
It didn't make sense for panfrost, or etnaviv. It doesn't make sense for
that driver either. Especially for something that olddefconfig can
handle just fine.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 10/12] drm/rockchip: vop2: Add support for rk3588
Hi Sascha,
On 11/27/23 19:19, Sascha Hauer wrote:
Hi Andy,
Looks good overall, two small things inside.
On Wed, Nov 22, 2023 at 08:55:44PM +0800, Andy Yan wrote:
+#define vop2_output_if_is_hdmi(x) (x == ROCKCHIP_VOP2_EP_HDMI0 || x == ROCKCHIP_VOP2_EP_HDMI1)
+#define vop2_output_if_is_dp(x)(x == ROCKCHIP_VOP2_EP_DP0 || x
== ROCKCHIP_VOP2_EP_DP1)
+#define vop2_output_if_is_edp(x) (x == ROCKCHIP_VOP2_EP_EDP0 || x ==
ROCKCHIP_VOP2_EP_EDP1)
+#define vop2_output_if_is_mipi(x) (x == ROCKCHIP_VOP2_EP_MIPI0 || x ==
ROCKCHIP_VOP2_EP_MIPI1)
+#define vop2_output_if_is_lvds(x) (x == ROCKCHIP_VOP2_EP_LVDS0 || x ==
ROCKCHIP_VOP2_EP_LVDS1)
+#define vop2_output_if_is_dpi(x) (x == ROCKCHIP_VOP2_EP_RGB0)
Not that it matters in practice here, but you should add braces around
the x argument in the macros usage, i.e. ((x) == ROCKCHIP_VOP2_EP_RGB0)
Okay , will do.
+static unsigned long rk3588_set_intf_mux(struct vop2_video_port *vp, int id,
u32 polflags)
+{
+ struct vop2 *vop2 = vp->vop2;
+ int dclk_core_div, dclk_out_div, if_pixclk_div, if_dclk_div;
+ unsigned long clock;
+ u32 die, dip, div, vp_clk_div, val;
+
+ clock = rk3588_calc_cru_cfg(vp, id, &dclk_core_div, &dclk_out_div,
+ &if_pixclk_div, &if_dclk_div);
+ if (!clock)
+ return 0;
+
+ vp_clk_div = FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_CORE_DIV,
dclk_core_div);
+ vp_clk_div |= FIELD_PREP(RK3588_VP_CLK_CTRL__DCLK_OUT_DIV,
dclk_out_div);
+
+ die = vop2_readl(vop2, RK3568_DSP_IF_EN);
+ dip = vop2_readl(vop2, RK3568_DSP_IF_POL);
+ div = vop2_readl(vop2, RK3568_DSP_IF_CTRL);
+
+ switch (id) {
+ case ROCKCHIP_VOP2_EP_HDMI0:
+ div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_DCLK_DIV,
if_dclk_div);
you should clear the bits of a mask before setting them again. The same
goes for several other bits modified in this switch/case.
Thanks for catching this, will fixed in next version.
+ div |= FIELD_PREP(RK3588_DSP_IF_EDP_HDMI0_PCLK_DIV,
if_pixclk_div);
+ die &= ~RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX;
+ die |= RK3588_SYS_DSP_INFACE_EN_HDMI0 |
+ FIELD_PREP(RK3588_SYS_DSP_INFACE_EN_EDP_HDMI0_MUX,
vp->id);
+ val = rk3588_get_hdmi_pol(polflags);
+ regmap_write(vop2->vop_grf, RK3588_GRF_VOP_CON2,
HIWORD_UPDATE(1, 1, 1));
+ regmap_write(vop2->vo1_grf, RK3588_GRF_VO1_CON0,
HIWORD_UPDATE(val, 6, 5));
+ break;
Sascha
[PATCH v2 RESEND] drm/panel: starry-2081101qfh032011-53g: Fine tune the panel power sequence
For the "starry, 2081101qfh032011-53g" panel, it is stipulated in the
panel spec that MIPI needs to keep the LP11 state before the
lcm_reset pin is pulled high.
Fixes: 6069b66cd962 ("drm/panel: support for STARRY 2081101QFH032011-53G
MIPI-DSI panel")
Signed-off-by: xiazhengqiao
Reviewed-by: Jessica Zhang
---
drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 4f370bc6dca8..4ed8c2e28d37 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -1765,6 +1765,7 @@ static const struct panel_desc starry_qfh032011_53g_desc
= {
.mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
MIPI_DSI_MODE_LPM,
.init_cmds = starry_qfh032011_53g_init_cmd,
+ .lp11_before_reset = true,
};
static const struct drm_display_mode starry_himax83102_j02_default_mode = {
--
2.17.1
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Maxime Ripard writes:
Hello Maxime,
> Hi,
>
> On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
>> On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
>> wrote:
>> > Geert Uytterhoeven writes:
>> > > The Imagination Technologies PowerVR Series 6 GPU is currently only
>> > > supported on Texas Instruments K3 AM62x SoCs. Hence add a dependency on
>> > > ARCH_K3, to prevent asking the user about this driver when configuring a
>> > > kernel without Texas Instruments K3 Multicore SoC support.
>> > >
>> > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR driver")
>> > > Signed-off-by: Geert Uytterhoeven
>> > > ---
>> >
>> > Indeed. Although I wonder what is the supposed policy since for example
>> > the DRM_PANFROST symbol only depends on ARM || ARM64 and others such as
>>
>> I think ARM Mali is sufficiently ubiquitous on ARM/ARM64 systems to
>> have just an ARM/ARM64 dependency...
>>
>> > DRM_ETNAVIV don't even have an SoC or architecture dependency.
>>
>> Vivante GPUs are found in DTS files on at least 4 architectures.
>> Might be worthwhile to add some dependencies, though...
>>
>> > In any case, I agree with you that restricting to only K3 makes sense.
>>
>> I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
>> eventually ;-)
>
> I disagree. This is to handle a generic IP, just like panfrost, lima, or
> etnaviv, and we certaintly don't want to maintain the Kconfig list of
> every possible architecture and SoC family it might or might not be
> found.
>
Thanks for the clarification. Then the policy is to have a depends on
ARCH_$FOO if the IP block is tied to a particular SoC or SoC family ?
For example, DRM_V3D has:
depends on ARCH_BCM || ARCH_BRCMSTB || ARCH_BCM2835 || COMPILE_TEST
If the IP block is generic and could be integrated with any SoC, then it
should not have a dependency as you said.
> GPUs supposed to be handled are spread across 4 architectures (x86,
> riscv, arm, arm64, mips?), and in arm/arm64 alone we have at least 5
> platforms that might use it (allwinner, ti, mediatek, renesas, rockchip)
>
> It didn't make sense for panfrost, or etnaviv. It doesn't make sense for
> that driver either. Especially for something that olddefconfig can
> handle just fine.
>
I think then that we should drop the arch and SoC dependency for these GPU
drivers and just leave the symbols they really depend on (e.g: DRM, MMU) ?
> Maxime
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [RFC PATCH 2/6] mm/gmem: add arch-independent abstraction to track address mapping status
On 11/28/23 07:50, Weixi Zhu wrote:
This patch adds an abstraction layer, struct vm_object, that maintains
per-process virtual-to-physical mapping status stored in struct gm_mapping.
For example, a virtual page may be mapped to a CPU physical page or to a
device physical page. Struct vm_object effectively maintains an
arch-independent page table, which is defined as a "logical page table".
While arch-dependent page table used by a real MMU is named a "physical
page table". The logical page table is useful if Linux core MM is extended
to handle a unified virtual address space with external accelerators using
customized MMUs.
In this patch, struct vm_object utilizes a radix
tree (xarray) to track where a virtual page is mapped to. This adds extra
memory consumption from xarray, but provides a nice abstraction to isolate
mapping status from the machine-dependent layer (PTEs). Besides supporting
accelerators with external MMUs, struct vm_object is planned to further
union with i_pages in struct address_mapping for file-backed memory.
The idea of struct vm_object is originated from FreeBSD VM design, which
provides a unified abstraction for anonymous memory, file-backed memory,
page cache and etc[1].
Currently, Linux utilizes a set of hierarchical page walk functions to
abstract page table manipulations of different CPU architecture. The
problem happens when a device wants to reuse Linux MM code to manage its
page table -- the device page table may not be accessible to the CPU.
Existing solution like Linux HMM utilizes the MMU notifier mechanisms to
invoke device-specific MMU functions, but relies on encoding the mapping
status on the CPU page table entries. This entangles machine-independent
code with machine-dependent code, and also brings unnecessary restrictions.
The PTE size and format vary arch by arch, which harms the extensibility.
[1] https://docs.freebsd.org/en/articles/vm-design/
Signed-off-by: Weixi Zhu
---
include/linux/gmem.h | 120 +
include/linux/mm_types.h | 4 +
mm/Makefile | 2 +-
mm/vm_object.c | 184 +++
4 files changed, 309 insertions(+), 1 deletion(-)
create mode 100644 mm/vm_object.c
diff --git a/include/linux/gmem.h b/include/linux/gmem.h
index fff877873557..529ff6755a99 100644
--- a/include/linux/gmem.h
+++ b/include/linux/gmem.h
@@ -9,11 +9,131 @@
#ifndef _GMEM_H
#define _GMEM_H
+#include
+
#ifdef CONFIG_GMEM
+
+#define GM_PAGE_CPU0x10 /* Determines whether page is a pointer or a pfn
number. */
+#define GM_PAGE_DEVICE 0x20
+#define GM_PAGE_NOMAP 0x40
+#define GM_PAGE_WILLNEED 0x80
+
+#define GM_PAGE_TYPE_MASK (GM_PAGE_CPU | GM_PAGE_DEVICE | GM_PAGE_NOMAP)
+
+struct gm_mapping {
+ unsigned int flag;
+
+ union {
+ struct page *page; /* CPU node */
+ struct gm_dev *dev; /* hetero-node. TODO: support multiple
devices */
+ unsigned long pfn;
+ };
+
+ struct mutex lock;
+};
+
+static inline void gm_mapping_flags_set(struct gm_mapping *gm_mapping, int
flags)
+{
+ if (flags & GM_PAGE_TYPE_MASK)
+ gm_mapping->flag &= ~GM_PAGE_TYPE_MASK;
+
+ gm_mapping->flag |= flags;
+}
+
+static inline void gm_mapping_flags_clear(struct gm_mapping *gm_mapping, int
flags)
+{
+ gm_mapping->flag &= ~flags;
+}
+
+static inline bool gm_mapping_cpu(struct gm_mapping *gm_mapping)
+{
+ return !!(gm_mapping->flag & GM_PAGE_CPU);
+}
+
+static inline bool gm_mapping_device(struct gm_mapping *gm_mapping)
+{
+ return !!(gm_mapping->flag & GM_PAGE_DEVICE);
+}
+
+static inline bool gm_mapping_nomap(struct gm_mapping *gm_mapping)
+{
+ return !!(gm_mapping->flag & GM_PAGE_NOMAP);
+}
+
+static inline bool gm_mapping_willneed(struct gm_mapping *gm_mapping)
+{
+ return !!(gm_mapping->flag & GM_PAGE_WILLNEED);
+}
+
/* h-NUMA topology */
void __init hnuma_init(void);
+
+/* vm object */
+/*
+ * Each per-process vm_object tracks the mapping status of virtual pages from
+ * all VMAs mmap()-ed with MAP_PRIVATE | MAP_PEER_SHARED.
+ */
+struct vm_object {
+ spinlock_t lock;
+
+ /*
+* The logical_page_table is a container that holds the mapping
+* information between a VA and a struct page.
+*/
+ struct xarray *logical_page_table;
+ atomic_t nr_pages;
+};
+
+int __init vm_object_init(void);
+struct vm_object *vm_object_create(struct mm_struct *mm);
+void vm_object_drop_locked(struct mm_struct *mm);
+
+struct gm_mapping *alloc_gm_mapping(void);
+void free_gm_mappings(struct vm_area_struct *vma);
+struct gm_mapping *vm_object_lookup(struct vm_object *obj, unsigned long va);
+void vm_object_mapping_create(struct vm_object *obj, unsigned long start);
+void unmap_gm_mappings_range(struct vm_area_struct *vma, unsigned long start,
+unsigned long end);
+void munmap_in_peer_devices(struct mm_struct *m
Re: [PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support
On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
>Hi Sascha:
>
>thanks for you review.
>
>On 11/27/23 18:13, Sascha Hauer wrote:
>
> On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
>
> From: Andy Yan [1]
>
> /sys/kernel/debug/dri/vop2/summary: dump vop display state
> /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
> /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
> activated modules
>
> Signed-off-by: Andy Yan [2]
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++
> 1 file changed, 399 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 9eecbe1f71f9..4a2342209c15 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -187,6 +188,7 @@ struct vop2 {
> */
> u32 registered_num_wins;
>
> + struct resource *res;
> void __iomem *regs;
> struct regmap *map;
>
> @@ -228,6 +230,44 @@ struct vop2 {
> #define vop2_output_if_is_lvds(x) (x == ROCKCHIP_VOP2_EP_LVDS0 || x ==
> ROCKCHIP_VOP2_EP_LVDS1)
> #define vop2_output_if_is_dpi(x) (x == ROCKCHIP_VOP2_EP_RGB0)
>
> +struct vop2_regs_dump {
> + const char *name;
> + u32 base;
> + u32 en_reg;
> + u32 en_val;
> + u32 en_mask;
> +};
> +
> +/*
> + * bus-format types.
> + */
> +struct drm_bus_format_enum_list {
> + int type;
> + const char *name;
> +};
> +
> +static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
> + { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
> + { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
> + { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
> + { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
> + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
> + { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
> + { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
> + { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
> + { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
> + { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
> + { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
> + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
> + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
> + { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
> + { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
> + { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
> + { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
> + { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
> +};
> +static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
> +
> static const struct regmap_config vop2_regmap_config;
>
> static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
> @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs
> vop2_crtc_helper_funcs = {
> .atomic_disable = vop2_crtc_atomic_disable,
> };
>
> +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct
> seq_file *s)
> +{
> + struct drm_connector_list_iter conn_iter;
> + struct drm_connector *connector;
> +
> + drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + if (crtc->state->connector_mask &
> drm_connector_mask(connector))
> + seq_printf(s, "Connector: %s\n",
> connector->name);
> +
> + }
> + drm_connector_list_iter_end(&conn_iter);
> +}
> +
> +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane
> *plane)
> +{
> + struct vop2_win *win = to_vop2_win(plane);
> + struct drm_plane_state *pstate = plane->state;
> + struct drm_rect *src, *dst;
> + struct drm_framebuffer *fb;
> + struct drm_gem_object *obj;
> + struct rockchip_gem_object *rk_obj;
> + bool xmirror;
> + bool ymirror;
> + bool rotate_270;
> + bool rotate_90;
> + dma_addr_t fb_addr;
> + int i;
> +
> + seq_printf(s, "%s: %s\n", win->data->name, pstate->crtc ?
> "ACTIVE" : "DISABLED");
> + if (!pstate || !pstate->fb)
> + return 0;
> +
> + fb = pstate->fb;
> + src = &pstate->src;
> + dst = &pstate->dst;
> + xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
> + ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
> + rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
> + rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
> +
> + seq_printf(s, "\twin_id: %d\n", win->win_id);
> +
> +
Re: [RFC PATCH 03/10] drm/mipi-dsi: add API for manual control over the DSI link power state
On 08/11/2023 16:58, Laurent Pinchart wrote: On Wed, Nov 08, 2023 at 04:34:39PM +0100, Maxime Ripard wrote: On Tue, Nov 07, 2023 at 04:26:34PM +0100, Greg Kroah-Hartman wrote: On Tue, Nov 07, 2023 at 01:18:14PM +0100, Maxime Ripard wrote: On Tue, Nov 07, 2023 at 12:22:21PM +0100, Greg Kroah-Hartman wrote: On Tue, Nov 07, 2023 at 11:57:49AM +0100, Maxime Ripard wrote: +GKH Why? I don't see a question for me here, sorry. I guess the question is: we have a bus with various power states (powered off, low power, high speed) Great, have fun! And is this per-device or per-bus-instance? Per bus instance To be precise, those power states are link states. They don't necessarily translate directly to device power states, and they're not so much about power management than speed (and bus turn-around for reads) management. So the DSI core should support handling and tracking the current DSI link state, and DSI devices should be able to request for a particular link state. Also, while DSI allows for multiple peripherals on a bus, the link is point-to-point, with the peripherals being all behind a single DSI RX. > low power is typically used to send commands to a device, high speed to transmit pixels, but still allows to send commands. Low power (LP) is a link state where commands can be transmitted at a low speed, as opposed to the high speed (HS) link state that is used to transmit both video data and commands at high speed. Any device-to-host data transfer (in response to read commands) occurs exclusively in LP mode (at least with DSI v1.3, I don't have acces to newer specifications). Depending on the devices, there's different requirements about the state devices expect the bus to be in to send commands. Some will need to send all the commands in the low power state, some don't care, etc. See the mail I was replying too for more details. We've tried so far to model that in KMS itself, so the framework the drivers would register too, but we're kind of reaching the limits of what we can do there. It also feels to me that "the driver can't access its device" is more of a problem for the bus to solve rather than the framework. This is up to the specific bus to resolve, there's nothing special needed in the driver core for it, right? Yeah, we weren't really looking to handle this into the driver core, but rather if there was a set of guidelines or feedback on implementing those kind of features for a bus. Do you agree? Are you aware of any other bus in Linux with similar requirements we could look at? Or any suggestion on how to solve it? There might be others, yes, look at how the dynamic power management works for different devices on most busses, that might help you out here. Thanks for the pointers, we'll have a look
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Maxime,
On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard wrote:
> On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > wrote:
> > > Geert Uytterhoeven writes:
> > > > The Imagination Technologies PowerVR Series 6 GPU is currently only
> > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a dependency on
> > > > ARCH_K3, to prevent asking the user about this driver when configuring a
> > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > >
> > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR driver")
> > > > Signed-off-by: Geert Uytterhoeven
> > > In any case, I agree with you that restricting to only K3 makes sense.
> >
> > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> > eventually ;-)
>
> I disagree. This is to handle a generic IP, just like panfrost, lima, or
> etnaviv, and we certaintly don't want to maintain the Kconfig list of
> every possible architecture and SoC family it might or might not be
> found.
While PowerVR is a generic IP, I believe it needs a non-generic
firmware, which is currently only available for AM62x SoCs.
Once it becomes truly generic, I'm happy to drop all platform
dependencies. Until then, there is no point in asking everyone who
configures an arm64 kernel about this driver, unless they also enabled
K3 support.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH v6] Documentation/gpu: VM_BIND locking document
Add the first version of the VM_BIND locking document which is intended to be part of the xe driver upstreaming agreement. The document describes and discuss the locking used during exec- functions, evicton and for userptr gpu-vmas. Intention is to be using the same nomenclature as the drm-vm-bind-async.rst. v2: - s/gvm/gpu_vm/g (Rodrigo Vivi) - Clarify the userptr seqlock with a pointer to mm/mmu_notifier.c (Rodrigo Vivi) - Adjust commit message accordingly. - Add SPDX license header. v3: - Large update to align with the drm_gpuvm manager locking - Add "Efficient userptr gpu_vma exec function iteration" section - Add "Locking at bind- and unbind time" section. v4: - Fix tabs vs space errors by untabifying (Rodrigo Vivi) - Minor style fixes and typos (Rodrigo Vivi) - Clarify situations where stale GPU mappings are occurring and how access through these mappings are blocked. (Rodrigo Vivi) - Insert into the toctree in implementation_guidelines.rst v5: - Add a section about recoverable page-faults. - Use local references to other documentation where possible (Bagas Sanjaya) - General documentation fixes and typos (Danilo Krummrich and Boris Brezillon) - Improve the documentation around locks that need to be grabbed from the dm-fence critical section (Boris Brezillon) - Add more references to the DRM GPUVM helpers (Danilo Krummrich and Boriz Brezillon) - Update the rfc/xe.rst document. v6: - Rework wording to improve readability (Boris Brezillon, Rodrigo Vivi, Bagas Sanjaya) - Various minor fixes across the document (Boris Brezillon) Cc: Rodrigo Vivi Signed-off-by: Thomas Hellström Reviewed-by: Boris Brezillon Reviewed-by: Rodrigo Vivi Reviewed-by: Danilo Krummrich --- Documentation/core-api/pin_user_pages.rst | 2 + Documentation/gpu/drm-mm.rst | 4 + Documentation/gpu/drm-vm-bind-locking.rst | 582 ++ .../gpu/implementation_guidelines.rst | 1 + Documentation/gpu/rfc/xe.rst | 5 + 5 files changed, 594 insertions(+) create mode 100644 Documentation/gpu/drm-vm-bind-locking.rst diff --git a/Documentation/core-api/pin_user_pages.rst b/Documentation/core-api/pin_user_pages.rst index d3c1f6d8c0e0..6b5f7e6e7155 100644 --- a/Documentation/core-api/pin_user_pages.rst +++ b/Documentation/core-api/pin_user_pages.rst @@ -153,6 +153,8 @@ NOTE: Some pages, such as DAX pages, cannot be pinned with longterm pins. That's because DAX pages do not have a separate page cache, and so "pinning" implies locking down file system blocks, which is not (yet) supported in that way. +.. _mmu-notifier-registration-case: + CASE 3: MMU notifier registration, with or without page faulting hardware - Device drivers can pin pages via get_user_pages*(), and register for mmu diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index acc5901ac840..d55751cad67c 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -466,6 +466,8 @@ DRM MM Range Allocator Function References .. kernel-doc:: drivers/gpu/drm/drm_mm.c :export: +.. _drm_gpuvm: + DRM GPUVM = @@ -481,6 +483,8 @@ Split and Merge .. kernel-doc:: drivers/gpu/drm/drm_gpuvm.c :doc: Split and Merge +.. _drm_gpuvm_locking: + Locking --- diff --git a/Documentation/gpu/drm-vm-bind-locking.rst b/Documentation/gpu/drm-vm-bind-locking.rst new file mode 100644 index ..a345aa513d12 --- /dev/null +++ b/Documentation/gpu/drm-vm-bind-locking.rst @@ -0,0 +1,582 @@ +.. SPDX-License-Identifier: (GPL-2.0+ OR MIT) + +=== +VM_BIND locking +=== + +This document attempts to describe what's needed to get VM_BIND locking right, +including the userptr mmu_notifier locking. It also discusses some +optimizations to get rid of the looping through of all userptr mappings and +external / shared object mappings that is needed in the simplest +implementation. In addition, there is a section describing the VM_BIND locking +required for implementing recoverable pagefaults. + +The DRM GPUVM set of helpers + + +There is a set of helpers for drivers implementing VM_BIND, and this +set of helpers implements much, but not all of the locking described +in this document. In particular, it is currently lacking a userptr +implementation. This document does not intend to describe the DRM GPUVM +implementation in detail, but it is covered in :ref:`its own +documentation `. It is highly recommended for any driver +implementing VM_BIND to use the DRM GPUVM helpers and to extend it if +common functionality is missing. + +Nomenclature + + +* ``gpu_vm``: Abstraction of a virtual GPU address space with + meta-data. Typically one per client (DRM file-private), or one per + execution context. +* ``gpu_vma``: Abstraction of a GPU address range within a gpu_vm with + associated meta-data. The backing stora
Re: [PATCH v5 1/4] pwm: rename pwm_apply_state() to pwm_apply_cansleep()
On Fri, Nov 24, 2023 at 02:31:18PM +0100, Thierry Reding wrote: > On Sat, Nov 18, 2023 at 04:16:17PM +, Sean Young wrote: > > In order to introduce a pwm api which can be used from atomic context, > > we will need two functions for applying pwm changes: > > > > int pwm_apply_cansleep(struct pwm *, struct pwm_state *); > > int pwm_apply_atomic(struct pwm *, struct pwm_state *); > > > > This commit just deals with renaming pwm_apply_state(), a following > > commit will introduce the pwm_apply_atomic() function. > > Sorry, I still don't agree with that _cansleep suffix. I think it's the > wrong terminology. Just because something can sleep doesn't mean that it > ever will. "Might sleep" is much more accurate because it says exactly > what might happen and indicates what we're guarding against. Sorry, I forgot about this in the last round. I've renamed it _might_sleep in v6 which I'll post shortly. Sean
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
Hi Ville, On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: > On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > > > > All the drm_connector_init variants take at least a pointer to the > > > > device, connector and hooks implementation. > > > > > > > > However, none of them check their value before dereferencing those > > > > pointers which can lead to a NULL-pointer dereference if the author > > > > isn't careful. > > > > > > Arguably oopsing on the spot is preferrable when this can't be caused by > > > user input. It's always a mistake that should be caught early during > > > development. > > > > > > Not everyone checks the return value of drm_connector_init and friends, > > > so those cases will lead to more mysterious bugs later. And probably > > > oopses as well. > > > > So maybe we can do both then, with something like > > > > if (WARN_ON(!dev)) > >return -EINVAL > > > > if (drm_WARN_ON(dev, !connector || !funcs)) > >return -EINVAL; > > > > I'd still like to check for this, so we can have proper testing, and we > > already check for those pointers in some places (like funcs in > > drm_connector_init), so if we don't cover everything we're inconsistent. > > People will invariably cargo-cult this kind of stuff absolutely > everywhere and then all your functions will have tons of dead > code to check their arguments. And that's a bad thing because... ? Also, are you really saying that checking that your arguments make sense is cargo-cult? We're already doing it in some parts of KMS, so we have to be consistent, and the answer to "most drivers don't check the error" cannot be "let's just give on error checking then". > I'd prefer not to go there usually. > > Should we perhaps start to use the (arguably hideous) > - void f(struct foo *bar) > + void f(struct foo bar[static 1]) > syntax to tell the compiler we don't accept NULL pointers? > > Hmm. Apparently that has the same problem as using any > other kind of array syntax in the prototype. That is, > the compiler demands to know the definition of 'struct foo' > even though we're passing in effectively a pointer. Sigh. Honestly, I don't care as long as it's something we can unit-test to make sure we make it consistent. We can't unit test a complete kernel crash. signature.asc Description: PGP signature
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Geert Uytterhoeven writes:
Hello Geert,
> Hi Maxime,
>
> On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard wrote:
>> On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
>> > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
>> > wrote:
>> > > Geert Uytterhoeven writes:
>> > > > The Imagination Technologies PowerVR Series 6 GPU is currently only
>> > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a dependency
>> > > > on
>> > > > ARCH_K3, to prevent asking the user about this driver when configuring
>> > > > a
>> > > > kernel without Texas Instruments K3 Multicore SoC support.
>> > > >
>> > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR
>> > > > driver")
>> > > > Signed-off-by: Geert Uytterhoeven
>
>> > > In any case, I agree with you that restricting to only K3 makes sense.
>> >
>> > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
>> > eventually ;-)
>>
>> I disagree. This is to handle a generic IP, just like panfrost, lima, or
>> etnaviv, and we certaintly don't want to maintain the Kconfig list of
>> every possible architecture and SoC family it might or might not be
>> found.
>
> While PowerVR is a generic IP, I believe it needs a non-generic
> firmware, which is currently only available for AM62x SoCs.
> Once it becomes truly generic, I'm happy to drop all platform
> dependencies. Until then, there is no point in asking everyone who
> configures an arm64 kernel about this driver, unless they also enabled
> K3 support.
>
That's true but it will require a Kconfig patch every time that there is a
design with a different SoC using this generic IP.
So when should be added? Once there's an upstream DTS that has a GPU device?
Once there's a firmware for it in linux-firmware?
I like the guideline of not having a depends on for generic IP and only have
for IP that can only be used in designs with a SoC from the same vendor.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
[PATCH v6 1/4] pwm: rename pwm_apply_state() to pwm_apply_might_sleep()
In order to introduce a pwm api which can be used from atomic context,
we will need two functions for applying pwm changes:
int pwm_apply_might_sleep(struct pwm *, struct pwm_state *);
int pwm_apply_atomic(struct pwm *, struct pwm_state *);
This commit just deals with renaming pwm_apply_state(), a following
commit will introduce the pwm_apply_atomic() function.
Acked-by: Hans de Goede
Acked-by: Jani Nikula
Acked-by: Lee Jones
Signed-off-by: Sean Young
---
Documentation/driver-api/pwm.rst | 8 +++---
.../gpu/drm/i915/display/intel_backlight.c| 6 ++--
drivers/gpu/drm/solomon/ssd130x.c | 2 +-
drivers/hwmon/pwm-fan.c | 8 +++---
drivers/input/misc/da7280.c | 4 +--
drivers/input/misc/pwm-beeper.c | 4 +--
drivers/input/misc/pwm-vibra.c| 8 +++---
drivers/leds/leds-pwm.c | 2 +-
drivers/leds/rgb/leds-pwm-multicolor.c| 4 +--
drivers/media/rc/pwm-ir-tx.c | 4 +--
drivers/platform/x86/lenovo-yogabook.c| 2 +-
drivers/pwm/core.c| 18 ++--
drivers/pwm/pwm-twl-led.c | 2 +-
drivers/pwm/pwm-vt8500.c | 2 +-
drivers/pwm/sysfs.c | 10 +++
drivers/regulator/pwm-regulator.c | 4 +--
drivers/video/backlight/lm3630a_bl.c | 2 +-
drivers/video/backlight/lp855x_bl.c | 2 +-
drivers/video/backlight/pwm_bl.c | 12
drivers/video/fbdev/ssd1307fb.c | 2 +-
include/linux/pwm.h | 28 +--
21 files changed, 67 insertions(+), 67 deletions(-)
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index bb264490a87a..f1d8197c8c43 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -41,7 +41,7 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also
exist.
After being requested, a PWM has to be configured using::
- int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
+ int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state
*state);
This API controls both the PWM period/duty_cycle config and the
enable/disable state.
@@ -57,13 +57,13 @@ If supported by the driver, the signal can be optimized,
for example to improve
EMI by phase shifting the individual channels of a chip.
The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
-around pwm_apply_state() and should not be used if the user wants to change
+around pwm_apply_might_sleep() and should not be used if the user wants to
change
several parameter at once. For example, if you see pwm_config() and
pwm_{enable,disable}() calls in the same function, this probably means you
-should switch to pwm_apply_state().
+should switch to pwm_apply_might_sleep().
The PWM user API also allows one to query the PWM state that was passed to the
-last invocation of pwm_apply_state() using pwm_get_state(). Note this is
+last invocation of pwm_apply_might_sleep() using pwm_get_state(). Note this is
different to what the driver has actually implemented if the request cannot be
satisfied exactly with the hardware in use. There is currently no way for
consumers to get the actually implemented settings.
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c
b/drivers/gpu/drm/i915/display/intel_backlight.c
index 2e8f17c04522..ff9b9918b0a1 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct
drm_connector_state *conn_state,
struct intel_panel *panel =
&to_intel_connector(conn_state->connector)->panel;
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_might_sleep(panel->backlight.pwm,
&panel->backlight.pwm_state);
}
static void
@@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct
drm_connector_state *old_conn
intel_backlight_set_pwm_level(old_conn_state, level);
panel->backlight.pwm_state.enabled = false;
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_might_sleep(panel->backlight.pwm,
&panel->backlight.pwm_state);
}
void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
@@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct
intel_crtc_state *crtc_state,
pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
panel->backlight.pwm_state.enabled = true;
- pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+ pwm_apply_might_sleep(panel->backlight.pwm,
&panel->backlight.pwm_state);
}
static void __intel_
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Wed, Nov 29, 2023 at 10:11:26AM +0100, Maxime Ripard wrote: > Hi Ville, > > On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: > > On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > > > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > > > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > > > > > All the drm_connector_init variants take at least a pointer to the > > > > > device, connector and hooks implementation. > > > > > > > > > > However, none of them check their value before dereferencing those > > > > > pointers which can lead to a NULL-pointer dereference if the author > > > > > isn't careful. > > > > > > > > Arguably oopsing on the spot is preferrable when this can't be caused by > > > > user input. It's always a mistake that should be caught early during > > > > development. > > > > > > > > Not everyone checks the return value of drm_connector_init and friends, > > > > so those cases will lead to more mysterious bugs later. And probably > > > > oopses as well. > > > > > > So maybe we can do both then, with something like > > > > > > if (WARN_ON(!dev)) > > >return -EINVAL > > > > > > if (drm_WARN_ON(dev, !connector || !funcs)) > > >return -EINVAL; > > > > > > I'd still like to check for this, so we can have proper testing, and we > > > already check for those pointers in some places (like funcs in > > > drm_connector_init), so if we don't cover everything we're inconsistent. > > > > People will invariably cargo-cult this kind of stuff absolutely > > everywhere and then all your functions will have tons of dead > > code to check their arguments. > > And that's a bad thing because... ? > > Also, are you really saying that checking that your arguments make sense > is cargo-cult? > > We're already doing it in some parts of KMS, so we have to be > consistent, and the answer to "most drivers don't check the error" > cannot be "let's just give on error checking then". > > > I'd prefer not to go there usually. > > > > Should we perhaps start to use the (arguably hideous) > > - void f(struct foo *bar) > > + void f(struct foo bar[static 1]) > > syntax to tell the compiler we don't accept NULL pointers? > > > > Hmm. Apparently that has the same problem as using any > > other kind of array syntax in the prototype. That is, > > the compiler demands to know the definition of 'struct foo' > > even though we're passing in effectively a pointer. Sigh. > > Honestly, I don't care as long as it's something we can unit-test to > make sure we make it consistent. We can't unit test a complete kernel > crash. Why do you want to put utterly broken code into a unit test? -- Ville Syrjälä Intel
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
On Wed, Nov 29, 2023 at 09:58:12AM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard wrote:
> > On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> > > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > > wrote:
> > > > Geert Uytterhoeven writes:
> > > > > The Imagination Technologies PowerVR Series 6 GPU is currently only
> > > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a dependency
> > > > > on
> > > > > ARCH_K3, to prevent asking the user about this driver when
> > > > > configuring a
> > > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > > >
> > > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR
> > > > > driver")
> > > > > Signed-off-by: Geert Uytterhoeven
>
> > > > In any case, I agree with you that restricting to only K3 makes sense.
> > >
> > > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> > > eventually ;-)
> >
> > I disagree. This is to handle a generic IP, just like panfrost, lima, or
> > etnaviv, and we certaintly don't want to maintain the Kconfig list of
> > every possible architecture and SoC family it might or might not be
> > found.
>
> While PowerVR is a generic IP, I believe it needs a non-generic
> firmware, which is currently only available for AM62x SoCs.
I'm not sure it's actually true, but let's consider it is. Then what? If
the firmware isn't there and/or the DT bits too, then nothing will
happen. We would have wasted a couple of 100kB on a system that is
taking somewhere in the 100MB-10GB range, and that's pretty much it.
If you have we take that patch in though, we have:
- To keep merging patches as firmwares become available.
- If we update linux-firmware only, then the driver is still not
loading even though it could.
- If we have gotten our firmware through some other mean, then the
driver is still not loading even though it could.
It makes life harder for everyone: maintainers, users, devs, based on
the state of some external project that might or might not be updated in
sync.
> Once it becomes truly generic, I'm happy to drop all platform
> dependencies. Until then, there is no point in asking everyone who
> configures an arm64 kernel about this driver, unless they also enabled
> K3 support.
Whether it's truly generic, whatever that means, is irrelevant here.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH] [drm/meson] meson_plane: Add error handling
Hi,
Thanks for your patch!
On 29/11/2023 10:21, Haoran Liu wrote:
This patch adds robust error handling to the meson_plane_create
function in drivers/gpu/drm/meson/meson_plane.c. The function
previously lacked proper handling for potential failure scenarios
of the drm_universal_plane_init call.
Signed-off-by: Haoran Liu
---
drivers/gpu/drm/meson/meson_plane.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/meson/meson_plane.c
b/drivers/gpu/drm/meson/meson_plane.c
index 815dfe30492b..67b36398f146 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -534,6 +534,7 @@ int meson_plane_create(struct meson_drm *priv)
struct meson_plane *meson_plane;
struct drm_plane *plane;
const uint64_t *format_modifiers = format_modifiers_default;
+ int ret;
meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
GFP_KERNEL);
@@ -548,12 +549,16 @@ int meson_plane_create(struct meson_drm *priv)
else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
format_modifiers = format_modifiers_afbc_g12a;
- drm_universal_plane_init(priv->drm, plane, 0xFF,
+ ret = drm_universal_plane_init(priv->drm, plane, 0xFF,
&meson_plane_funcs,
supported_drm_formats,
ARRAY_SIZE(supported_drm_formats),
format_modifiers,
DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
Could you re-align those lines aswell ?
+ if (ret) {
+ devm_kfree(priv->drm->dev, meson_plane);
+ return ret;
+ }
drm_plane_helper_add(plane, &meson_plane_helper_funcs);
Thanks,
Neil
Re: [PATCH] drm/sched: Fix compilation issues with DRM priority rename
Hi Luben,
thanks for the patch!
On Sat, 2023-11-25 at 14:22 -0500, Luben Tuikov wrote:
> Fix compilation issues with DRM scheduler priority rename MIN to LOW.
>
> Signed-off-by: Luben Tuikov
> Reported-by: kernel test robot
> Closes:
> https://lore.kernel.org/oe-kbuild-all/202311252109.wgbjsskg-...@intel.com/
> Cc: Danilo Krummrich
> Cc: Frank Binns
> Cc: Donald Robson
> Cc: Matt Coster
> Cc: Direct Rendering Infrastructure - Development
>
> Fixes: fe375c74806dbd ("drm/sched: Rename priority MIN to LOW")
> Fixes: 5f03a507b29e44 ("drm/nouveau: implement 1:1 scheduler - entity
> relationship")
> ---
> drivers/gpu/drm/imagination/pvr_queue.c | 2 +-
> drivers/gpu/drm/nouveau/nouveau_sched.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_queue.c
> b/drivers/gpu/drm/imagination/pvr_queue.c
> index d65c3fbedf5ac4..5ed9c98fb599c8 100644
> --- a/drivers/gpu/drm/imagination/pvr_queue.c
> +++ b/drivers/gpu/drm/imagination/pvr_queue.c
> @@ -1292,7 +1292,7 @@ struct pvr_queue *pvr_queue_create(struct pvr_context
> *ctx,
> goto err_release_ufo;
>
> err = drm_sched_entity_init(&queue->entity,
> - DRM_SCHED_PRIORITY_MIN,
> + DRM_SCHED_PRIORITY_KERNEL,
> &sched, 1, &ctx->faulty);
> if (err)
> goto err_sched_fini;
At least pvr_queue.c can be built again,
Tested-by: Alexander Sverdlin
> diff --git a/drivers/gpu/drm/nouveau/nouveau_sched.c
> b/drivers/gpu/drm/nouveau/nouveau_sched.c
> index 3393647bd94423..dd98f6910f9cab 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_sched.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_sched.c
> @@ -18,7 +18,7 @@
> * index to the run-queue array.
> */
> enum nouveau_sched_priority {
> - NOUVEAU_SCHED_PRIORITY_SINGLE = DRM_SCHED_PRIORITY_MIN,
> + NOUVEAU_SCHED_PRIORITY_SINGLE = DRM_SCHED_PRIORITY_KERNEL,
> NOUVEAU_SCHED_PRIORITY_COUNT,
> };
>
> @@ -423,7 +423,7 @@ nouveau_sched_init(struct nouveau_sched *sched, struct
> nouveau_drm *drm,
> if (ret)
> goto fail_wq;
>
> - /* Using DRM_SCHED_PRIORITY_MIN, since that's what we're required to
> use
> + /* Using DRM_SCHED_PRIORITY_KERNEL, since that's what we're required
> to use
> * when we want to have a single run-queue only.
> *
> * It's not documented, but one will find out when trying to use any
> @@ -433,7 +433,7 @@ nouveau_sched_init(struct nouveau_sched *sched, struct
> nouveau_drm *drm,
> * Can't use NOUVEAU_SCHED_PRIORITY_SINGLE either, because it's not
> * matching the enum type used in drm_sched_entity_init().
> */
> - ret = drm_sched_entity_init(entity, DRM_SCHED_PRIORITY_MIN,
> + ret = drm_sched_entity_init(entity, DRM_SCHED_PRIORITY_KERNEL,
> &drm_sched, 1, NULL);
> if (ret)
> goto fail_sched;
--
Alexander Sverdlin
Siemens AG
www.siemens.com
[PATCH] drm/panel: nt36523: fix return value check in nt36523_probe()
From: Yang Yingliang
mipi_dsi_device_register_full() never returns NULL pointer, it
will return ERR_PTR() when it fails, so replace the check with
IS_ERR().
Fixes: 0993234a0045 ("drm/panel: Add driver for Novatek NT36523")
Signed-off-by: Yang Yingliang
---
drivers/gpu/drm/panel/panel-novatek-nt36523.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36523.c
b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
index 9b9a7eb1bc60..a189ce236328 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36523.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
@@ -1254,9 +1254,9 @@ static int nt36523_probe(struct mipi_dsi_device *dsi)
return dev_err_probe(dev, -EPROBE_DEFER, "cannot get
secondary DSI host\n");
pinfo->dsi[1] = mipi_dsi_device_register_full(dsi1_host, info);
- if (!pinfo->dsi[1]) {
+ if (IS_ERR(pinfo->dsi[1])) {
dev_err(dev, "cannot get secondary DSI device\n");
- return -ENODEV;
+ return PTR_ERR(pinfo->dsi[1]);
}
}
--
2.25.1
[PATCH] [drm/meson] meson_plane: Add error handling
This patch adds robust error handling to the meson_plane_create
function in drivers/gpu/drm/meson/meson_plane.c. The function
previously lacked proper handling for potential failure scenarios
of the drm_universal_plane_init call.
Signed-off-by: Haoran Liu
---
drivers/gpu/drm/meson/meson_plane.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/meson/meson_plane.c
b/drivers/gpu/drm/meson/meson_plane.c
index 815dfe30492b..67b36398f146 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -534,6 +534,7 @@ int meson_plane_create(struct meson_drm *priv)
struct meson_plane *meson_plane;
struct drm_plane *plane;
const uint64_t *format_modifiers = format_modifiers_default;
+ int ret;
meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
GFP_KERNEL);
@@ -548,12 +549,16 @@ int meson_plane_create(struct meson_drm *priv)
else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
format_modifiers = format_modifiers_afbc_g12a;
- drm_universal_plane_init(priv->drm, plane, 0xFF,
+ ret = drm_universal_plane_init(priv->drm, plane, 0xFF,
&meson_plane_funcs,
supported_drm_formats,
ARRAY_SIZE(supported_drm_formats),
format_modifiers,
DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
+ if (ret) {
+ devm_kfree(priv->drm->dev, meson_plane);
+ return ret;
+ }
drm_plane_helper_add(plane, &meson_plane_helper_funcs);
--
2.17.1
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Wed, 29 Nov 2023, Maxime Ripard wrote: > Hi Ville, > > On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: >> On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: >> > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: >> > > On Tue, 28 Nov 2023, Maxime Ripard wrote: >> > > > All the drm_connector_init variants take at least a pointer to the >> > > > device, connector and hooks implementation. >> > > > >> > > > However, none of them check their value before dereferencing those >> > > > pointers which can lead to a NULL-pointer dereference if the author >> > > > isn't careful. >> > > >> > > Arguably oopsing on the spot is preferrable when this can't be caused by >> > > user input. It's always a mistake that should be caught early during >> > > development. >> > > >> > > Not everyone checks the return value of drm_connector_init and friends, >> > > so those cases will lead to more mysterious bugs later. And probably >> > > oopses as well. >> > >> > So maybe we can do both then, with something like >> > >> > if (WARN_ON(!dev)) >> >return -EINVAL >> > >> > if (drm_WARN_ON(dev, !connector || !funcs)) >> >return -EINVAL; >> > >> > I'd still like to check for this, so we can have proper testing, and we >> > already check for those pointers in some places (like funcs in >> > drm_connector_init), so if we don't cover everything we're inconsistent. >> >> People will invariably cargo-cult this kind of stuff absolutely >> everywhere and then all your functions will have tons of dead >> code to check their arguments. > > And that's a bad thing because... ? > > Also, are you really saying that checking that your arguments make sense > is cargo-cult? It's a powerful thing to be able to assume a NULL argument is always a fatal programming error on the caller's side, and should oops and get caught immediately. It's an assertion. We're not talking about user input or anything like that here. If you start checking for things that can't happen, and return errors for them, you start gracefully handling things that don't have anything graceful about them. Having such checks in place trains people to think they *may* happen. While it should fail fast and loud at the developer's first smoke test, and get fixed then and there. BR, Jani. > > We're already doing it in some parts of KMS, so we have to be > consistent, and the answer to "most drivers don't check the error" > cannot be "let's just give on error checking then". > >> I'd prefer not to go there usually. >> >> Should we perhaps start to use the (arguably hideous) >> - void f(struct foo *bar) >> + void f(struct foo bar[static 1]) >> syntax to tell the compiler we don't accept NULL pointers? >> >> Hmm. Apparently that has the same problem as using any >> other kind of array syntax in the prototype. That is, >> the compiler demands to know the definition of 'struct foo' >> even though we're passing in effectively a pointer. Sigh. > > Honestly, I don't care as long as it's something we can unit-test to > make sure we make it consistent. We can't unit test a complete kernel > crash. -- Jani Nikula, Intel
Re: [PATCH v3 07/11] drm/mediatek: Support alpha blending in VDOSYS0
Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()
Hello Laurent,
On Wed, Nov 29, 2023 at 02:39:55AM +0200, Laurent Pinchart wrote:
> On Thu, Nov 23, 2023 at 06:54:27PM +0100, Uwe Kleine-König wrote:
> > pm_runtime_resume_and_get() already drops the runtime PM usage counter
> > in the error case. So a call to pm_runtime_put_sync() can be dropped.
> >
> > Signed-off-by: Uwe Kleine-König
>
> I wonder if checkpatch should warn about usage of pm_runtime_get_sync().
It should not warn in general. There are cases where
pm_runtime_get_sync() is the right function to use. See for example
commit aec488051633 ("crypto: stm32 - Properly handle pm_runtime_get
failing").
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH 0/3] drm/bridge: ti-sn65dsi86: Some updates
Hello Laurent,
On Wed, Nov 29, 2023 at 02:45:33AM +0200, Laurent Pinchart wrote:
> On Fri, Nov 24, 2023 at 09:56:55AM +0100, Neil Armstrong wrote:
> > On 23/11/2023 18:54, Uwe Kleine-König wrote:
> > > Hello,
> > >
> > > this is a series I created while starring at the ti-sn65dsi86 driver in
> > > the context of my pwm-lifetime series.
> > >
> > > The first patch should be fine. The last one has a few rough edges, but
> > > maybe you like the direction this is going to? The 2nd patch probably
> > > only makes sense if you also take the third.
> > >
> > > Best regards
> > > Uwe
> > >
> > > Uwe Kleine-König (3):
> > >drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()
> > >drm/bridge: ti-sn65dsi86: Change parameters of
> > > ti_sn65dsi86_{read,write}_u16
> > >drm/bridge: ti-sn65dsi86: Loosen coupling of PWM to ti-sn65dsi86 core
> > >
> > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 146 +++---
> > > 1 file changed, 83 insertions(+), 63 deletions(-)
> > >
> > > base-commit: 4e87148f80d198ba5febcbcc969c6b9471099a09
> >
> > It looks fine to me, even without the goal to move the driver to drivers/pwm
> > I think it's same to move the pwm ddata out of the main pdata ans associate
> > it to the pwm aux device lifetime.
> >
> > I don't anything wrong, and so far it's of for me, let's see if there's
> > comments
> > for other people before applying!
>
> I like 1/3 very much, but as mentioned in a reply to 3/3, I'm not
> convinced by that one at all.
OK, I also wasn't completely sure that patches #2 and #3 are a good
idea. The benefit I see is only better separation of the two drivers
(which is very subjective) and making the main driver struct a bit
smaller, saving some memory if the PWM isn't bound (which (maybe) saves
very little memory in some corner cases only).
> Not only does it make the driver more
> complex for, I believe, very little gain (if any), usage of
> devm_kzalloc() in ti_sn_pwm_probe() is most likely wrong.
It's not more wrong that the same construct in nearly every other pwm
driver. With my big life-time series for pwm[1] that idiom is fine.
Best regards
Uwe
[1]
https://lore.kernel.org/linux-pwm/20231121134901.208535-1-u.kleine-koe...@pengutronix.de
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Javier,
On Wed, Nov 29, 2023 at 10:13 AM Javier Martinez Canillas
wrote:
> Geert Uytterhoeven writes:
> > On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard wrote:
> >> On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> >> > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> >> > wrote:
> >> > > Geert Uytterhoeven writes:
> >> > > > The Imagination Technologies PowerVR Series 6 GPU is currently only
> >> > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a
> >> > > > dependency on
> >> > > > ARCH_K3, to prevent asking the user about this driver when
> >> > > > configuring a
> >> > > > kernel without Texas Instruments K3 Multicore SoC support.
> >> > > >
> >> > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR
> >> > > > driver")
> >> > > > Signed-off-by: Geert Uytterhoeven
> >
> >> > > In any case, I agree with you that restricting to only K3 makes sense.
> >> >
> >> > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> >> > eventually ;-)
> >>
> >> I disagree. This is to handle a generic IP, just like panfrost, lima, or
> >> etnaviv, and we certaintly don't want to maintain the Kconfig list of
> >> every possible architecture and SoC family it might or might not be
> >> found.
> >
> > While PowerVR is a generic IP, I believe it needs a non-generic
> > firmware, which is currently only available for AM62x SoCs.
> > Once it becomes truly generic, I'm happy to drop all platform
> > dependencies. Until then, there is no point in asking everyone who
> > configures an arm64 kernel about this driver, unless they also enabled
> > K3 support.
>
> That's true but it will require a Kconfig patch every time that there is a
> design with a different SoC using this generic IP.
It also requires a DT bindings patch, to add a new compatible value,
plus whatever missing properties for SoC integration (e.g. resets).
And a DTS integration patch.
And patches for various on-SoC resources (e.g. clocks).
And perhaps a DRM driver update.
> So when should be added? Once there's an upstream DTS that has a GPU device?
> Once there's a firmware for it in linux-firmware?
It can be added when handling the above. As all patches should be
tested, the firmware must be available first.
When critical mass is reached, platform dependencies can be dropped.
I do hope that will happen rather sooner than later!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
[PATCH] [drm/sti] sti_compositor: Add error handlingin sti_compositor_bind
Previously, the function sti_compositor_bind did not properly
handle potential failure scenarios of drm_vblank_init, which could
lead to unexpected behavior. This update adds a check for the
return value of drm_vblank_init.
Signed-off-by: Haoran Liu
---
drivers/gpu/drm/sti/sti_compositor.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/sti/sti_compositor.c
b/drivers/gpu/drm/sti/sti_compositor.c
index 33487a1fed8f..beddbd1c48eb 100644
--- a/drivers/gpu/drm/sti/sti_compositor.c
+++ b/drivers/gpu/drm/sti/sti_compositor.c
@@ -69,6 +69,7 @@ static int sti_compositor_bind(struct device *dev,
struct drm_plane *primary = NULL;
struct sti_compositor_subdev_descriptor *desc = compo->data.subdev_desc;
unsigned int array_size = compo->data.nb_subdev;
+ int ret;
dev_priv->compo = compo;
@@ -145,7 +146,11 @@ static int sti_compositor_bind(struct device *dev,
}
}
- drm_vblank_init(drm_dev, crtc_id);
+ ret = drm_vblank_init(drm_dev, crtc_id);
+ if (ret) {
+ DRM_ERROR("Failed to initialize vblank\n");
+ return ret;
+ }
return 0;
}
--
2.17.1
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Maxime,
On Wed, Nov 29, 2023 at 10:23 AM Maxime Ripard wrote:
> On Wed, Nov 29, 2023 at 09:58:12AM +0100, Geert Uytterhoeven wrote:
> > On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard wrote:
> > > On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> > > > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > > > wrote:
> > > > > Geert Uytterhoeven writes:
> > > > > > The Imagination Technologies PowerVR Series 6 GPU is currently only
> > > > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a
> > > > > > dependency on
> > > > > > ARCH_K3, to prevent asking the user about this driver when
> > > > > > configuring a
> > > > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > > > >
> > > > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR
> > > > > > driver")
> > > > > > Signed-off-by: Geert Uytterhoeven
> >
> > > > > In any case, I agree with you that restricting to only K3 makes sense.
> > > >
> > > > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> > > > eventually ;-)
> > >
> > > I disagree. This is to handle a generic IP, just like panfrost, lima, or
> > > etnaviv, and we certaintly don't want to maintain the Kconfig list of
> > > every possible architecture and SoC family it might or might not be
> > > found.
> >
> > While PowerVR is a generic IP, I believe it needs a non-generic
> > firmware, which is currently only available for AM62x SoCs.
>
> I'm not sure it's actually true, but let's consider it is. Then what? If
> the firmware isn't there and/or the DT bits too, then nothing will
> happen. We would have wasted a couple of 100kB on a system that is
> taking somewhere in the 100MB-10GB range, and that's pretty much it.
I am talking about posing the question to the user to enable the driver
or not. Which applies to everyone who configures a kernel.
> If you have we take that patch in though, we have:
>
> - To keep merging patches as firmwares become available.
You need to keep merging patches to update DT bindings, DTS,
SoC-specific drivers, the DRM driver itself, ... too.
> - If we update linux-firmware only, then the driver is still not
> loading even though it could.
>
> - If we have gotten our firmware through some other mean, then the
> driver is still not loading even though it could.
You will still need to update parts of the kernel, too.
As long as none of that has happened, asking about the PowerVR driver
on non-AM62x hardware is futile...
> It makes life harder for everyone: maintainers, users, devs, based on
> the state of some external project that might or might not be updated in
> sync.
>
> > Once it becomes truly generic, I'm happy to drop all platform
> > dependencies. Until then, there is no point in asking everyone who
> > configures an arm64 kernel about this driver, unless they also enabled
> > K3 support.
>
> Whether it's truly generic, whatever that means, is irrelevant here.
It is.
BTW, playing the devil's advocate: why is there a dependency on ARM64?
PowerVR GPUs are also present on (at least) arm32 and Intel?
Oh, dropping that would expose this question to Linus, causing his
wrath to come down on you... ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: Simplify using pm_runtime_resume_and_get()
Hi Uwe,
On Wed, Nov 29, 2023 at 10:51:37AM +0100, Uwe Kleine-König wrote:
> On Wed, Nov 29, 2023 at 02:39:55AM +0200, Laurent Pinchart wrote:
> > On Thu, Nov 23, 2023 at 06:54:27PM +0100, Uwe Kleine-König wrote:
> > > pm_runtime_resume_and_get() already drops the runtime PM usage counter
> > > in the error case. So a call to pm_runtime_put_sync() can be dropped.
> > >
> > > Signed-off-by: Uwe Kleine-König
> >
> > I wonder if checkpatch should warn about usage of pm_runtime_get_sync().
>
> It should not warn in general. There are cases where
> pm_runtime_get_sync() is the right function to use. See for example
Sure, the function most likely has some valid use cases (otherwise it
should just be removed), but I think those are are a minority. I was
just thinking out loud anyway.
> commit aec488051633 ("crypto: stm32 - Properly handle pm_runtime_get
> failing").
I don't know much about that device, but wouldn't the best option be to
avoid resuming the device at remove time ? In any case, that's getting
out of topic for the sn65dsi86 :-)
--
Regards,
Laurent Pinchart
Re: [PATCH 1/2] Revert "drm/bridge: Add 200ms delay to wait FW HPD status stable"
On Mon, 20 Nov 2023 17:10:36 +0800, Xin Ji wrote: > This reverts commit 330140d7319fcc4ec68bd924ea212e476bf12275 > > 200ms delay will cause panel display image later than backlight > turn on, revert this patch. > > Applied, thanks! [1/2] Revert "drm/bridge: Add 200ms delay to wait FW HPD status stable" https://cgit.freedesktop.org/drm/drm-misc/commit/?id=af3145aa142c [2/2] drm/bridge: anx7625: Fix Set HPD irq detect window to 2ms https://cgit.freedesktop.org/drm/drm-misc/commit/?id=e3af7053de3f Rob
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Tue, 28 Nov 2023 15:49:08 +0200 Ville Syrjälä wrote: > Should we perhaps start to use the (arguably hideous) > - void f(struct foo *bar) > + void f(struct foo bar[static 1]) > syntax to tell the compiler we don't accept NULL pointers? > > Hmm. Apparently that has the same problem as using any > other kind of array syntax in the prototype. That is, > the compiler demands to know the definition of 'struct foo' > even though we're passing in effectively a pointer. Sigh. __attribute__((nonnull)) ? Thanks, pq pgpZ7WgZr4A8B.pgp Description: OpenPGP digital signature
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Wed, Nov 29, 2023 at 12:12:59PM +0200, Pekka Paalanen wrote: > On Tue, 28 Nov 2023 15:49:08 +0200 > Ville Syrjälä wrote: > > > Should we perhaps start to use the (arguably hideous) > > - void f(struct foo *bar) > > + void f(struct foo bar[static 1]) > > syntax to tell the compiler we don't accept NULL pointers? > > > > Hmm. Apparently that has the same problem as using any > > other kind of array syntax in the prototype. That is, > > the compiler demands to know the definition of 'struct foo' > > even though we're passing in effectively a pointer. Sigh. > > > __attribute__((nonnull)) ? I guess that would work, though the syntax is horrible when you need to flag specific arguments. -- Ville Syrjälä Intel
Re: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions
On 11/29/23 10:53, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 01:05:14 +0300
> Dmitry Osipenko wrote:
>
>> On 11/28/23 15:37, Boris Brezillon wrote:
>>> On Tue, 28 Nov 2023 12:14:42 +0100
>>> Maxime Ripard wrote:
>>>
Hi,
On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
> On Fri, 24 Nov 2023 11:40:06 +0100
> Maxime Ripard wrote:
>
>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
>>> Add locked and remove unlocked postfixes from drm-shmem function names,
>>> making names consistent with the drm/gem core code.
>>>
>>> Reviewed-by: Boris Brezillon
>>> Suggested-by: Boris Brezillon
>>> Signed-off-by: Dmitry Osipenko
>>
>> This contradicts my earlier ack on a patch but...
>>
>>> ---
>>> drivers/gpu/drm/drm_gem_shmem_helper.c| 64 +--
>>> drivers/gpu/drm/lima/lima_gem.c | 8 +--
>>> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>>> drivers/gpu/drm/panfrost/panfrost_gem.c | 6 +-
>>> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 +-
>>> drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
>>> drivers/gpu/drm/v3d/v3d_bo.c | 4 +-
>>> drivers/gpu/drm/virtio/virtgpu_object.c | 4 +-
>>> include/drm/drm_gem_shmem_helper.h| 36 +--
>>> 9 files changed, 64 insertions(+), 64 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> index 0d61f2b3e213..154585ddae08 100644
>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs
>>> drm_gem_shmem_funcs = {
>>> .pin = drm_gem_shmem_object_pin,
>>> .unpin = drm_gem_shmem_object_unpin,
>>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
>>> - .vmap = drm_gem_shmem_object_vmap,
>>> - .vunmap = drm_gem_shmem_object_vunmap,
>>> + .vmap = drm_gem_shmem_object_vmap_locked,
>>> + .vunmap = drm_gem_shmem_object_vunmap_locked,
>>
>> While I think we should indeed be consistent with the names, I would
>> also expect helpers to get the locking right by default.
>
> Wait, actually I think this patch does what you suggest already. The
> _locked() prefix tells the caller: "you should take care of the locking,
> I expect the lock to be held when this is hook/function is called". So
> helpers without the _locked() prefix take care of the locking (which I
> guess matches your 'helpers get the locking right' expectation), and
> those with the _locked() prefix don't.
What I meant by "getting the locking right" is indeed a bit ambiguous,
sorry. What I'm trying to say I guess is that, in this particular case,
I don't think you can expect the vmap implementation to be called with
or without the locks held. The doc for that function will say that it's
either one or the other, but not both.
So helpers should follow what is needed to provide a default vmap/vunmap
implementation, including what locking is expected from a vmap/vunmap
implementation.
>>>
>>> Hm, yeah, I think that's a matter of taste. When locking is often
>>> deferrable, like it is in DRM, I find it beneficial for funcions and
>>> function pointers to reflect the locking scheme, rather than relying on
>>> people properly reading the doc, especially when this is the only
>>> outlier in the group of drm_gem_object_funcs we already have, and it's
>>> not event documented at the drm_gem_object_funcs level [1] :P.
>>>
If that means that vmap is always called with the locks taken, then
drm_gem_shmem_object_vmap can just assume that it will be called with
the locks taken and there's no need to mention it in the name (and you
can probably sprinkle a couple of lockdep assertion to make sure the
locking is indeed consistent).
>>>
>>> Things get very confusing when you end up having drm_gem_shmem helpers
>>> that are suffixed with _locked() to encode the fact locking is the
>>> caller's responsibility and no suffix for the
>>> callee-takes-care-of-the-locking semantics, while other helpers that are
>>> not suffixed at all actually implement the
>>> caller-should-take-care-of-the-locking semantics.
>>>
>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
>> convert whatever function needs to be converted to the unlock suffix so
>> we get a consistent naming.
>
> That would be an _unlocked() prefix if we do it the other way around. I
> think the main confusion comes from the names of the hooks in
> drm_gem_shmem_f
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
On Wed, Nov 29, 2023 at 11:10:51AM +0100, Geert Uytterhoeven wrote:
> On Wed, Nov 29, 2023 at 10:23 AM Maxime Ripard wrote:
> > On Wed, Nov 29, 2023 at 09:58:12AM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard wrote:
> > > > On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> > > > > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > > > > wrote:
> > > > > > Geert Uytterhoeven writes:
> > > > > > > The Imagination Technologies PowerVR Series 6 GPU is currently
> > > > > > > only
> > > > > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a
> > > > > > > dependency on
> > > > > > > ARCH_K3, to prevent asking the user about this driver when
> > > > > > > configuring a
> > > > > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > > > > >
> > > > > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR
> > > > > > > driver")
> > > > > > > Signed-off-by: Geert Uytterhoeven
> > >
> > > > > > In any case, I agree with you that restricting to only K3 makes
> > > > > > sense.
> > > > >
> > > > > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> > > > > eventually ;-)
> > > >
> > > > I disagree. This is to handle a generic IP, just like panfrost, lima, or
> > > > etnaviv, and we certaintly don't want to maintain the Kconfig list of
> > > > every possible architecture and SoC family it might or might not be
> > > > found.
> > >
> > > While PowerVR is a generic IP, I believe it needs a non-generic
> > > firmware, which is currently only available for AM62x SoCs.
I just asked, it's not true in most cases. There's some exceptions
(GX6250 for example) that could require different firmwares depending on
the SoC it's used in, but it's not the case here.
> > I'm not sure it's actually true, but let's consider it is. Then what? If
> > the firmware isn't there and/or the DT bits too, then nothing will
> > happen. We would have wasted a couple of 100kB on a system that is
> > taking somewhere in the 100MB-10GB range, and that's pretty much it.
>
> I am talking about posing the question to the user to enable the driver
> or not. Which applies to everyone who configures a kernel.
If that user doesn't use a defconfig, doesn't use any variant of
*defconfig make target. Plus, the driver still isn't enabled by default.
> > If you have we take that patch in though, we have:
> >
> > - To keep merging patches as firmwares become available.
>
> You need to keep merging patches to update DT bindings, DTS,
> SoC-specific drivers, the DRM driver itself, ... too.
The DT binding and DRM driver is already there, the SoC specific drivers
are probably already there by the time you reach GPU enablement, and the
DT doesn't have to be upstream.
> > - If we update linux-firmware only, then the driver is still not
> > loading even though it could.
> >
> > - If we have gotten our firmware through some other mean, then the
> > driver is still not loading even though it could.
>
> You will still need to update parts of the kernel, too.
Not really, no. We can use the AM62x as an example. The only thing that
was required to enable the driver (excluding the driver itself of
course) was a single DT patch, without anything you've mentioned so far.
> As long as none of that has happened, asking about the PowerVR driver
> on non-AM62x hardware is futile...
Maybe. Just like asking whether you want to enable the UMS driver on
platforms that don't have a USB controller. Or, closer to us, whether
you want to enable AMDGPU on platforms without a PCIe bus.
We *never* do that.
If only because we can't. We don't have a per-SoC Kconfig option, so
even if we were to merge your patch, we would still ask about the
PowerVR driver on, let's say, the AM69 that isn't an AM62x and is just
as futile according to you. Or for the TDA4VM, or...
The other reason is that it's just impossible to maintain. You wouldn't
expect everyone, once they got their USB support in, to amend the
Kconfig dependencies for every USB driver out there, would you?
> > It makes life harder for everyone: maintainers, users, devs, based on
> > the state of some external project that might or might not be updated in
> > sync.
> >
> > > Once it becomes truly generic, I'm happy to drop all platform
> > > dependencies. Until then, there is no point in asking everyone who
> > > configures an arm64 kernel about this driver, unless they also enabled
> > > K3 support.
> >
> > Whether it's truly generic, whatever that means, is irrelevant here.
>
> It is.
>
> BTW, playing the devil's advocate: why is there a dependency on ARM64?
> PowerVR GPUs are also present on (at least) arm32 and Intel?
I would welcome any patch extending that requirement, or droping that
requirement.
> Oh, dropping that would expose this question to Linus, causing his
> wrath to come down on you... ;-)
Don't threaten me with a good time.
Also, it's already the case for
Re: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions
On Wed, 29 Nov 2023 13:47:21 +0300
Dmitry Osipenko wrote:
> On 11/29/23 10:53, Boris Brezillon wrote:
> > On Wed, 29 Nov 2023 01:05:14 +0300
> > Dmitry Osipenko wrote:
> >
> >> On 11/28/23 15:37, Boris Brezillon wrote:
> >>> On Tue, 28 Nov 2023 12:14:42 +0100
> >>> Maxime Ripard wrote:
> >>>
> Hi,
>
> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
> > On Fri, 24 Nov 2023 11:40:06 +0100
> > Maxime Ripard wrote:
> >
> >> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> >>> Add locked and remove unlocked postfixes from drm-shmem function
> >>> names,
> >>> making names consistent with the drm/gem core code.
> >>>
> >>> Reviewed-by: Boris Brezillon
> >>> Suggested-by: Boris Brezillon
> >>> Signed-off-by: Dmitry Osipenko
> >>
> >> This contradicts my earlier ack on a patch but...
> >>
> >>> ---
> >>> drivers/gpu/drm/drm_gem_shmem_helper.c| 64
> >>> +--
> >>> drivers/gpu/drm/lima/lima_gem.c | 8 +--
> >>> drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> >>> drivers/gpu/drm/panfrost/panfrost_gem.c | 6 +-
> >>> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 +-
> >>> drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
> >>> drivers/gpu/drm/v3d/v3d_bo.c | 4 +-
> >>> drivers/gpu/drm/virtio/virtgpu_object.c | 4 +-
> >>> include/drm/drm_gem_shmem_helper.h| 36 +--
> >>> 9 files changed, 64 insertions(+), 64 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>> index 0d61f2b3e213..154585ddae08 100644
> >>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs
> >>> drm_gem_shmem_funcs = {
> >>> .pin = drm_gem_shmem_object_pin,
> >>> .unpin = drm_gem_shmem_object_unpin,
> >>> .get_sg_table = drm_gem_shmem_object_get_sg_table,
> >>> - .vmap = drm_gem_shmem_object_vmap,
> >>> - .vunmap = drm_gem_shmem_object_vunmap,
> >>> + .vmap = drm_gem_shmem_object_vmap_locked,
> >>> + .vunmap = drm_gem_shmem_object_vunmap_locked,
> >>
> >> While I think we should indeed be consistent with the names, I would
> >> also expect helpers to get the locking right by default.
> >
> > Wait, actually I think this patch does what you suggest already. The
> > _locked() prefix tells the caller: "you should take care of the locking,
> > I expect the lock to be held when this is hook/function is called". So
> > helpers without the _locked() prefix take care of the locking (which I
> > guess matches your 'helpers get the locking right' expectation), and
> > those with the _locked() prefix don't.
>
> What I meant by "getting the locking right" is indeed a bit ambiguous,
> sorry. What I'm trying to say I guess is that, in this particular case,
> I don't think you can expect the vmap implementation to be called with
> or without the locks held. The doc for that function will say that it's
> either one or the other, but not both.
>
> So helpers should follow what is needed to provide a default vmap/vunmap
> implementation, including what locking is expected from a vmap/vunmap
> implementation.
> >>>
> >>> Hm, yeah, I think that's a matter of taste. When locking is often
> >>> deferrable, like it is in DRM, I find it beneficial for funcions and
> >>> function pointers to reflect the locking scheme, rather than relying on
> >>> people properly reading the doc, especially when this is the only
> >>> outlier in the group of drm_gem_object_funcs we already have, and it's
> >>> not event documented at the drm_gem_object_funcs level [1] :P.
> >>>
>
> If that means that vmap is always called with the locks taken, then
> drm_gem_shmem_object_vmap can just assume that it will be called with
> the locks taken and there's no need to mention it in the name (and you
> can probably sprinkle a couple of lockdep assertion to make sure the
> locking is indeed consistent).
> >>>
> >>> Things get very confusing when you end up having drm_gem_shmem helpers
> >>> that are suffixed with _locked() to encode the fact locking is the
> >>> caller's responsibility and no suffix for the
> >>> callee-takes-care-of-the-locking semantics, while other helpers that are
> >>> not suffixed at all actually implement the
> >>> caller-should-take-care-of-the-locking semantics.
> >>>
>
> >> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> >> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
>
Re: [PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support
Hi Sascha:
On 11/29/23 16:52, Sascha Hauer wrote:
On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
Hi Sascha:
thanks for you review.
On 11/27/23 18:13, Sascha Hauer wrote:
On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
From: Andy Yan [1]
/sys/kernel/debug/dri/vop2/summary: dump vop display state
/sys/kernel/debug/dri/vop2/regs: dump whole vop registers
/sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
activated modules
Signed-off-by: Andy Yan [2]
---
(no changes since v1)
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++
1 file changed, 399 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9eecbe1f71f9..4a2342209c15 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
@@ -187,6 +188,7 @@ struct vop2 {
*/
u32 registered_num_wins;
+ struct resource *res;
void __iomem *regs;
struct regmap *map;
@@ -228,6 +230,44 @@ struct vop2 {
#define vop2_output_if_is_lvds(x) (x == ROCKCHIP_VOP2_EP_LVDS0 || x ==
ROCKCHIP_VOP2_EP_LVDS1)
#define vop2_output_if_is_dpi(x) (x == ROCKCHIP_VOP2_EP_RGB0)
+struct vop2_regs_dump {
+ const char *name;
+ u32 base;
+ u32 en_reg;
+ u32 en_val;
+ u32 en_mask;
+};
+
+/*
+ * bus-format types.
+ */
+struct drm_bus_format_enum_list {
+ int type;
+ const char *name;
+};
+
+static const struct drm_bus_format_enum_list drm_bus_format_enum_list[] = {
+ { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
+ { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
+ { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
+ { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
+ { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
+ { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
+ { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
+ { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
+ { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
+ { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
+ { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
+ { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
+ { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
+ { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
+ { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
+ { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
+ { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
+ { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
+};
+static DRM_ENUM_NAME_FN(drm_get_bus_format_name, drm_bus_format_enum_list)
+
static const struct regmap_config vop2_regmap_config;
static struct vop2_video_port *to_vop2_video_port(struct drm_crtc *crtc)
@@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs
vop2_crtc_helper_funcs = {
.atomic_disable = vop2_crtc_atomic_disable,
};
+static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct
seq_file *s)
+{
+ struct drm_connector_list_iter conn_iter;
+ struct drm_connector *connector;
+
+ drm_connector_list_iter_begin(crtc->dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
+ if (crtc->state->connector_mask &
drm_connector_mask(connector))
+ seq_printf(s, "Connector: %s\n", connector->name);
+
+ }
+ drm_connector_list_iter_end(&conn_iter);
+}
+
+static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane *plane)
+{
+ struct vop2_win *win = to_vop2_win(plane);
+ struct drm_plane_state *pstate = plane->state;
+ struct drm_rect *src, *dst;
+ struct drm_framebuffer *fb;
+ struct drm_gem_object *obj;
+ struct rockchip_gem_object *rk_obj;
+ bool xmirror;
+ bool ymirror;
+ bool rotate_270;
+ bool rotate_90;
+ dma_addr_t fb_addr;
+ int i;
+
+ seq_printf(s, "%s: %s\n", win->data->name, pstate->crtc ? "ACTIVE" :
"DISABLED");
+ if (!pstate || !pstate->fb)
+ return 0;
+
+ fb = pstate->fb;
+ src = &pstate->src;
+ dst = &pstate->dst;
+ xmirror = pstate->rotation & DRM_MODE_REFLECT_X ? true : false;
+ ymirror = pstate->rotation & DRM_MODE_REFLECT_Y ? true : false;
+ rotate_270 = pstate->rotation & DRM_MODE_ROTATE_270;
+ rotate_90 = pstate->rotation & DRM_MODE_ROTATE_90;
+
+ seq_printf(s, "\twin_id: %d\n", win->win_id);
+
+ seq_printf(s, "\tformat: %p4cc%s glb_alpha[0x%x]\n",
+ &fb->format->format,
+
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Maxime,
On Wed, Nov 29, 2023 at 11:50 AM Maxime Ripard wrote:
> On Wed, Nov 29, 2023 at 11:10:51AM +0100, Geert Uytterhoeven wrote:
> > On Wed, Nov 29, 2023 at 10:23 AM Maxime Ripard wrote:
> > > On Wed, Nov 29, 2023 at 09:58:12AM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard
> > > > wrote:
> > > > > On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> > > > > > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > > > > > wrote:
> > > > > > > Geert Uytterhoeven writes:
> > > > > > > > The Imagination Technologies PowerVR Series 6 GPU is currently
> > > > > > > > only
> > > > > > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a
> > > > > > > > dependency on
> > > > > > > > ARCH_K3, to prevent asking the user about this driver when
> > > > > > > > configuring a
> > > > > > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > > > > > >
> > > > > > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton PowerVR
> > > > > > > > driver")
> > > > > > > > Signed-off-by: Geert Uytterhoeven
> > > >
> > > > > > > In any case, I agree with you that restricting to only K3 makes
> > > > > > > sense.
> > > > > >
> > > > > > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS || ...,
> > > > > > eventually ;-)
> > > > >
> > > > > I disagree. This is to handle a generic IP, just like panfrost, lima,
> > > > > or
> > > > > etnaviv, and we certaintly don't want to maintain the Kconfig list of
> > > > > every possible architecture and SoC family it might or might not be
> > > > > found.
> > > >
> > > > While PowerVR is a generic IP, I believe it needs a non-generic
> > > > firmware, which is currently only available for AM62x SoCs.
>
> I just asked, it's not true in most cases. There's some exceptions
> (GX6250 for example) that could require different firmwares depending on
> the SoC it's used in, but it's not the case here.
OK, please tell me how to use it on e.g. R-Car Gen3.
> > > I'm not sure it's actually true, but let's consider it is. Then what? If
> > > the firmware isn't there and/or the DT bits too, then nothing will
> > > happen. We would have wasted a couple of 100kB on a system that is
> > > taking somewhere in the 100MB-10GB range, and that's pretty much it.
> >
> > I am talking about posing the question to the user to enable the driver
> > or not. Which applies to everyone who configures a kernel.
>
> If that user doesn't use a defconfig, doesn't use any variant of
> *defconfig make target. Plus, the driver still isn't enabled by default.
>
> > > If you have we take that patch in though, we have:
> > >
> > > - To keep merging patches as firmwares become available.
> >
> > You need to keep merging patches to update DT bindings, DTS,
> > SoC-specific drivers, the DRM driver itself, ... too.
>
> The DT binding and DRM driver is already there, the SoC specific drivers
The DT binding only lists "ti,am62-gpu" with "img,img-axe" as a fallback.
> are probably already there by the time you reach GPU enablement, and the
> DT doesn't have to be upstream.
And getting it upstream requires updating the bindings...
> > > - If we update linux-firmware only, then the driver is still not
> > > loading even though it could.
> > >
> > > - If we have gotten our firmware through some other mean, then the
> > > driver is still not loading even though it could.
> >
> > You will still need to update parts of the kernel, too.
>
> Not really, no. We can use the AM62x as an example. The only thing that
> was required to enable the driver (excluding the driver itself of
> course) was a single DT patch, without anything you've mentioned so far.
Who added:
Documentation/devicetree/bindings/gpu/img,powervr.yaml- - ti,am62-gpu
Documentation/devicetree/bindings/gpu/img,powervr.yaml: - const:
img,img-axe # IMG AXE GPU model/revision is fully discoverable
?
> > As long as none of that has happened, asking about the PowerVR driver
> > on non-AM62x hardware is futile...
>
> Maybe. Just like asking whether you want to enable the UMS driver on
> platforms that don't have a USB controller. Or, closer to us, whether
> you want to enable AMDGPU on platforms without a PCIe bus.
>
> We *never* do that.
Thanks for not checking ;-)
if USB
[...]
source "drivers/usb/storage/Kconfig"
config DRM_AMDGPU
tristate "AMD GPU"
depends on DRM && PCI && MMU
> If only because we can't. We don't have a per-SoC Kconfig option, so
> even if we were to merge your patch, we would still ask about the
> PowerVR driver on, let's say, the AM69 that isn't an AM62x and is just
> as futile according to you. Or for the TDA4VM, or...
That's why we use per-family options. It's the next best thing we have.
> The other reason is that it's just impossible to maintain. You wouldn't
> expect everyone, once they got their USB support in, to amend the
> Kconfig dependencies for every
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Wed, Nov 29, 2023 at 11:38:42AM +0200, Jani Nikula wrote: > On Wed, 29 Nov 2023, Maxime Ripard wrote: > > Hi Ville, > > > > On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: > >> On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > >> > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > >> > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > >> > > > All the drm_connector_init variants take at least a pointer to the > >> > > > device, connector and hooks implementation. > >> > > > > >> > > > However, none of them check their value before dereferencing those > >> > > > pointers which can lead to a NULL-pointer dereference if the author > >> > > > isn't careful. > >> > > > >> > > Arguably oopsing on the spot is preferrable when this can't be caused > >> > > by > >> > > user input. It's always a mistake that should be caught early during > >> > > development. > >> > > > >> > > Not everyone checks the return value of drm_connector_init and friends, > >> > > so those cases will lead to more mysterious bugs later. And probably > >> > > oopses as well. > >> > > >> > So maybe we can do both then, with something like > >> > > >> > if (WARN_ON(!dev)) > >> >return -EINVAL > >> > > >> > if (drm_WARN_ON(dev, !connector || !funcs)) > >> >return -EINVAL; > >> > > >> > I'd still like to check for this, so we can have proper testing, and we > >> > already check for those pointers in some places (like funcs in > >> > drm_connector_init), so if we don't cover everything we're inconsistent. > >> > >> People will invariably cargo-cult this kind of stuff absolutely > >> everywhere and then all your functions will have tons of dead > >> code to check their arguments. > > > > And that's a bad thing because... ? > > > > Also, are you really saying that checking that your arguments make sense > > is cargo-cult? > > It's a powerful thing to be able to assume a NULL argument is always a > fatal programming error on the caller's side, and should oops and get > caught immediately. It's an assertion. Yeah, but we're not really doing that either. We have no explicit assertion anywhere. We take a pointer in, and just hope that it will be dereferenced later on and that the kernel will crash. The pointer to the functions especially is only deferenced very later on. And assertions might be powerful, but being able to notice errors and debug them is too. A panic takes away basically any remote access to debug. If you don't have a console, you're done. > We're not talking about user input or anything like that here. > > If you start checking for things that can't happen, and return errors > for them, you start gracefully handling things that don't have anything > graceful about them. But there's nothing graceful to do here: you just return from your probe function that you couldn't probe and that's it. Just like you do when you can't map your registers, or get your interrupt, or register into any framework (including drm_dev_register that pretty much every driver handles properly if it returns an error, without being graceful about it). > Having such checks in place trains people to think they *may* happen. In most cases, kmalloc can't fail. We seem to have a very different policy towards it. > While it should fail fast and loud at the developer's first smoke test, > and get fixed then and there. Returning an error + a warning also qualifies for "fail fast and loud". But keeps the system alive for someone to notice in any case. Maxime signature.asc Description: PGP signature
RE: [bug report] backlight: mp3309c: Add support for MPS MP3309C
Hi Dan,
Can I add the "Reported-by" tag, with your name, in my 2nd vers of
the commit to fix this bug?
Thanks and regards,
Flavio
> -Original Message-
> From: Flavio Suligoi
> Sent: Tuesday, November 28, 2023 9:24 AM
> To: Dan Carpenter
> Cc: dri-devel@lists.freedesktop.org
> Subject: RE: [bug report] backlight: mp3309c: Add support for MPS
> MP3309C
>
> Hi Dan,
>
> Thanks for the report, I'll fix the bug as soon as possible.
>
> Regards,
> Flavio
>
> > -Original Message-
> > From: Dan Carpenter
> > Sent: Tuesday, November 28, 2023 8:20 AM
> > To: Flavio Suligoi
> > Cc: dri-devel@lists.freedesktop.org
> > Subject: [bug report] backlight: mp3309c: Add support for MPS MP3309C
> >
> > Hello Flavio Suligoi,
> >
> > The patch 2e914516a58c: "backlight: mp3309c: Add support for MPS
> > MP3309C" from Nov 16, 2023 (linux-next), leads to the following
> > Smatch static checker warning:
> >
> > drivers/video/backlight/mp3309c.c:277 pm3309c_parse_dt_node()
> > error: uninitialized symbol 'prop_levels'.
> >
> > drivers/video/backlight/mp3309c.c
> > 202 static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
> > 203 struct mp3309c_platform_data
> > *pdata)
> > 204 {
> > 205 struct device_node *node = chip->dev->of_node;
> > 206 struct property *prop_pwms, *prop_levels;
> > 207 int length = 0;
> > 208 int ret, i;
> > 209 unsigned int num_levels, tmp_value;
> > 210
> > 211 if (!node) {
> > 212 dev_err(chip->dev, "failed to get DT node\n");
> > 213 return -ENODEV;
> > 214 }
> > 215
> > 216 /*
> > 217 * Dimming mode: the MP3309C provides two dimming
> > control mode:
> > 218 *
> > 219 * - PWM mode
> > 220 * - Analog by I2C control mode (default)
> > 221 *
> > 222 * I2C control mode is assumed as default but, if the
> > pwms property is
> > 223 * found in the backlight node, the mode switches to
> PWM
> > mode.
> > 224 */
> > 225 pdata->dimming_mode = DIMMING_ANALOG_I2C;
> > 226 prop_pwms = of_find_property(node, "pwms", &length);
> > 227 if (prop_pwms) {
> > 228 chip->pwmd = devm_pwm_get(chip->dev, NULL);
> > 229 if (IS_ERR(chip->pwmd))
> > 230 return dev_err_probe(chip->dev,
> > PTR_ERR(chip->pwmd),
> > 231 "error getting
> pwm
> > data\n");
> > 232 pdata->dimming_mode = DIMMING_PWM;
> > 233 pwm_apply_args(chip->pwmd);
> > 234 }
> > 235
> > 236 /*
> > 237 * In I2C control mode the dimming levels (0..31) are
> > fixed by the
> > 238 * hardware, while in PWM control mode they can be
> > chosen by the user,
> > 239 * to allow nonlinear mappings.
> > 240 */
> > 241 if (pdata->dimming_mode == DIMMING_ANALOG_I2C) {
> > 242 /*
> > 243 * Analog (by I2C commands) control mode:
> fixed
> > 0..31 brightness
> > 244 * levels
> > 245 */
> > 246 num_levels = ANALOG_I2C_NUM_LEVELS;
> > 247
> > 248 /* Enable GPIO used in I2C dimming mode only
> */
> > 249 chip->enable_gpio = devm_gpiod_get(chip->dev,
> > "enable",
> > 250
> > GPIOD_OUT_HIGH);
> > 251 if (IS_ERR(chip->enable_gpio))
> > 252 return dev_err_probe(chip->dev,
> > 253 PTR_ERR(chip-
> > >enable_gpio),
> > 254 "error getting
> > enable gpio\n");
> >
> > prop_levels not initialized on this path.
> >
> > 255 } else {
> > 256 /*
> > 257 * PWM control mode: check for brightness
> level
> > in DT
> > 258 */
> > 259 prop_levels = of_find_property(node,
> > "brightness-levels",
> > 260&length);
> > 261 if (prop_levels) {
> > 262 /* Read brightness levels from DT */
> > 263 num_levels = length / sizeof(u32);
> > 264 if (num_levels < 2)
> > 265 return -EINVAL;
> > 266 } else {
> > 267 /* Use default brightness levels */
> > 268 num_levels =
> > MP3309C_PWM_DEFAULT_NUM_LEVELS;
> > 269 }
> > 270 }
> > 271
> > 272 /* Fill brightness levels array */
> > 273 pdata->levels = devm_kc
[PATCH] drm/imagination: Fixed clang warnings for initial upstreamed patches.
Reported-by: kernel test robot
Closes:
https://lore.kernel.org/oe-kbuild-all/202311242159.hh8mwiam-...@intel.com/
Closes:
https://lore.kernel.org/oe-kbuild-all/202311241752.3ilyyfca-...@intel.com/
Closes:
https://lore.kernel.org/oe-kbuild-all/202311250632.givex7mu-...@intel.com/
Closes:
https://lore.kernel.org/oe-kbuild-all/202311250226.da2yiskp-...@intel.com/
Signed-off-by: Donald Robson
---
drivers/gpu/drm/imagination/pvr_device.c | 2 +-
drivers/gpu/drm/imagination/pvr_device_info.c | 3 ++-
drivers/gpu/drm/imagination/pvr_fw_meta.c | 1 +
drivers/gpu/drm/imagination/pvr_vm.c | 8 +---
4 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.c
b/drivers/gpu/drm/imagination/pvr_device.c
index 8499becf4fbb..048eba776cf2 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -127,7 +127,7 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
* This is called any time we receive a FW event. It iterates over all
* active queues and calls pvr_queue_process() on them.
*/
-void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
+static void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
{
struct pvr_queue *queue, *tmp_queue;
LIST_HEAD(active_queues);
diff --git a/drivers/gpu/drm/imagination/pvr_device_info.c
b/drivers/gpu/drm/imagination/pvr_device_info.c
index 11e6bef52ecd..d3301cde7d11 100644
--- a/drivers/gpu/drm/imagination/pvr_device_info.c
+++ b/drivers/gpu/drm/imagination/pvr_device_info.c
@@ -227,7 +227,8 @@ int pvr_device_info_set_features(struct pvr_device
*pvr_dev, const u64 *features
/* Verify no unsupported values in the bitmask. */
if (features_size > mapping_max_size) {
drm_warn(from_pvr_device(pvr_dev), "Unsupported features in
firmware image");
- } else if (features_size == mapping_max_size && (mapping_max & 63)) {
+ } else if (features_size == mapping_max_size &&
+ ((mapping_max & 63) != 0)) {
u64 invalid_mask = ~0ull << (mapping_max & 63);
if (features[features_size - 1] & invalid_mask)
diff --git a/drivers/gpu/drm/imagination/pvr_fw_meta.c
b/drivers/gpu/drm/imagination/pvr_fw_meta.c
index 119934c36184..c39beb70c317 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_meta.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_meta.c
@@ -4,6 +4,7 @@
#include "pvr_device.h"
#include "pvr_fw.h"
#include "pvr_fw_info.h"
+#include "pvr_fw_meta.h"
#include "pvr_gem.h"
#include "pvr_rogue_cr_defs.h"
#include "pvr_rogue_meta.h"
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c
b/drivers/gpu/drm/imagination/pvr_vm.c
index 04f7d0cf4188..375a03707f4e 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -108,12 +108,6 @@ struct pvr_vm_gpuva {
struct drm_gpuva base;
};
-static __always_inline
-struct pvr_vm_gpuva *to_pvr_vm_gpuva(struct drm_gpuva *gpuva)
-{
- return container_of(gpuva, struct pvr_vm_gpuva, base);
-}
-
enum pvr_vm_bind_type {
PVR_VM_BIND_TYPE_MAP,
PVR_VM_BIND_TYPE_UNMAP,
@@ -528,7 +522,7 @@ pvr_device_addr_and_size_are_valid(u64 device_addr, u64
size)
(device_addr + size <= PVR_PAGE_TABLE_ADDR_SPACE_SIZE);
}
-void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
+static void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
{
}
--
2.25.1
[PATCH v2] [drm/meson] meson_plane: Add error handling v2(re-aligned)
This patch adds robust error handling to the meson_plane_create
function in drivers/gpu/drm/meson/meson_plane.c. The function
previously lacked proper handling for potential failure scenarios
of the drm_universal_plane_init call.
Signed-off-by: Haoran Liu
---
drivers/gpu/drm/meson/meson_plane.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/meson/meson_plane.c
b/drivers/gpu/drm/meson/meson_plane.c
index 815dfe30492b..b43ac61201f3 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -534,6 +534,7 @@ int meson_plane_create(struct meson_drm *priv)
struct meson_plane *meson_plane;
struct drm_plane *plane;
const uint64_t *format_modifiers = format_modifiers_default;
+ int ret;
meson_plane = devm_kzalloc(priv->drm->dev, sizeof(*meson_plane),
GFP_KERNEL);
@@ -548,12 +549,16 @@ int meson_plane_create(struct meson_drm *priv)
else if (meson_vpu_is_compatible(priv, VPU_COMPATIBLE_G12A))
format_modifiers = format_modifiers_afbc_g12a;
- drm_universal_plane_init(priv->drm, plane, 0xFF,
-&meson_plane_funcs,
-supported_drm_formats,
-ARRAY_SIZE(supported_drm_formats),
-format_modifiers,
-DRM_PLANE_TYPE_PRIMARY, "meson_primary_plane");
+ ret = drm_universal_plane_init(priv->drm, plane, 0xFF,
+ &meson_plane_funcs,
+ supported_drm_formats,
+ ARRAY_SIZE(supported_drm_formats),
+ format_modifiers,
+ DRM_PLANE_TYPE_PRIMARY,
"meson_primary_plane");
+ if (ret) {
+ devm_kfree(priv->drm->dev, meson_plane);
+ return ret;
+ }
drm_plane_helper_add(plane, &meson_plane_helper_funcs);
--
2.17.1
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
On Wed, Nov 29, 2023 at 12:08:17PM +0100, Geert Uytterhoeven wrote:
> Hi Maxime,
>
> On Wed, Nov 29, 2023 at 11:50 AM Maxime Ripard wrote:
> > On Wed, Nov 29, 2023 at 11:10:51AM +0100, Geert Uytterhoeven wrote:
> > > On Wed, Nov 29, 2023 at 10:23 AM Maxime Ripard wrote:
> > > > On Wed, Nov 29, 2023 at 09:58:12AM +0100, Geert Uytterhoeven wrote:
> > > > > On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard
> > > > > wrote:
> > > > > > On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven wrote:
> > > > > > > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > > > > > > wrote:
> > > > > > > > Geert Uytterhoeven writes:
> > > > > > > > > The Imagination Technologies PowerVR Series 6 GPU is
> > > > > > > > > currently only
> > > > > > > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a
> > > > > > > > > dependency on
> > > > > > > > > ARCH_K3, to prevent asking the user about this driver when
> > > > > > > > > configuring a
> > > > > > > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > > > > > > >
> > > > > > > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton
> > > > > > > > > PowerVR driver")
> > > > > > > > > Signed-off-by: Geert Uytterhoeven
> > > > >
> > > > > > > > In any case, I agree with you that restricting to only K3 makes
> > > > > > > > sense.
> > > > > > >
> > > > > > > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS ||
> > > > > > > ...,
> > > > > > > eventually ;-)
> > > > > >
> > > > > > I disagree. This is to handle a generic IP, just like panfrost,
> > > > > > lima, or
> > > > > > etnaviv, and we certaintly don't want to maintain the Kconfig list
> > > > > > of
> > > > > > every possible architecture and SoC family it might or might not be
> > > > > > found.
> > > > >
> > > > > While PowerVR is a generic IP, I believe it needs a non-generic
> > > > > firmware, which is currently only available for AM62x SoCs.
> >
> > I just asked, it's not true in most cases. There's some exceptions
> > (GX6250 for example) that could require different firmwares depending on
> > the SoC it's used in, but it's not the case here.
>
> OK, please tell me how to use it on e.g. R-Car Gen3.
I'm not very familiar with the R-Car family of SoCs.
However, if I'm to trust that page:
https://www.renesas.com/us/en/products/automotive-products/automotive-system-chips-socs
None of them feature the same GPU than the AM62x, so that question is
completely different?
> > > > I'm not sure it's actually true, but let's consider it is. Then what? If
> > > > the firmware isn't there and/or the DT bits too, then nothing will
> > > > happen. We would have wasted a couple of 100kB on a system that is
> > > > taking somewhere in the 100MB-10GB range, and that's pretty much it.
> > >
> > > I am talking about posing the question to the user to enable the driver
> > > or not. Which applies to everyone who configures a kernel.
> >
> > If that user doesn't use a defconfig, doesn't use any variant of
> > *defconfig make target. Plus, the driver still isn't enabled by default.
> >
> > > > If you have we take that patch in though, we have:
> > > >
> > > > - To keep merging patches as firmwares become available.
> > >
> > > You need to keep merging patches to update DT bindings, DTS,
> > > SoC-specific drivers, the DRM driver itself, ... too.
> >
> > The DT binding and DRM driver is already there, the SoC specific drivers
>
> The DT binding only lists "ti,am62-gpu" with "img,img-axe" as a fallback.
Sure. And the driver matches on img,img-axe, so it would probe fine even
with a different first compatible.
> > are probably already there by the time you reach GPU enablement, and the
> > DT doesn't have to be upstream.
>
> And getting it upstream requires updating the bindings...
Right. And you still don't have to put it upstream, so the binding isn't
a requirement either.
> > > > - If we update linux-firmware only, then the driver is still not
> > > > loading even though it could.
> > > >
> > > > - If we have gotten our firmware through some other mean, then the
> > > > driver is still not loading even though it could.
> > >
> > > You will still need to update parts of the kernel, too.
> >
> > Not really, no. We can use the AM62x as an example. The only thing that
> > was required to enable the driver (excluding the driver itself of
> > course) was a single DT patch, without anything you've mentioned so far.
>
> Who added:
>
> Documentation/devicetree/bindings/gpu/img,powervr.yaml- - ti,am62-gpu
> Documentation/devicetree/bindings/gpu/img,powervr.yaml: - const:
> img,img-axe # IMG AXE GPU model/revision is fully discoverable
>
> ?
Which is a formality, part of the upstreaming process, but not required
at all from a technical point of view to make a driver probe.
> > > As long as none of that has happened, asking about the PowerVR driver
> > > on non-AM62x hardware is futile...
> >
> > Maybe. Just like asking whether you w
[PATCH 1/2] drm/imagination: avoid -Wmissing-prototype warnings
From: Arnd Bergmann
This warning option is now enabled by default, causing a few build regressions
in combination with the newly added pvr driver:
drivers/gpu/drm/imagination/pvr_device.c:130:6: error: no previous prototype
for 'pvr_device_process_active_queues' [-Werror=missing-prototypes]
130 | void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
| ^~~~
drivers/gpu/drm/imagination/pvr_fw_meta.c:33:1: error: no previous prototype
for 'pvr_meta_cr_read32' [-Werror=missing-prototypes]
33 | pvr_meta_cr_read32(struct pvr_device *pvr_dev, u32 reg_addr, u32
*reg_value_out)
| ^~
drivers/gpu/drm/imagination/pvr_vm.c:542:6: error: no previous prototype for
'pvr_gpuvm_free' [-Werror=missing-prototypes]
542 | void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
Mark pvr_device_process_active_queues and pvr_gpuvm_free static as they are
only used
in the file they are defined in, and include the correct header for the
pvr_meta_cr_read32
declaration.
Fixes: eaf01ee5ba28 ("drm/imagination: Implement job submission and scheduling")
Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and
META FW support")
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Arnd Bergmann
---
drivers/gpu/drm/imagination/pvr_device.c | 2 +-
drivers/gpu/drm/imagination/pvr_fw_meta.c | 1 +
drivers/gpu/drm/imagination/pvr_vm.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.c
b/drivers/gpu/drm/imagination/pvr_device.c
index 8499becf4fbb..048eba776cf2 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -127,7 +127,7 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
* This is called any time we receive a FW event. It iterates over all
* active queues and calls pvr_queue_process() on them.
*/
-void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
+static void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
{
struct pvr_queue *queue, *tmp_queue;
LIST_HEAD(active_queues);
diff --git a/drivers/gpu/drm/imagination/pvr_fw_meta.c
b/drivers/gpu/drm/imagination/pvr_fw_meta.c
index 119934c36184..c39beb70c317 100644
--- a/drivers/gpu/drm/imagination/pvr_fw_meta.c
+++ b/drivers/gpu/drm/imagination/pvr_fw_meta.c
@@ -4,6 +4,7 @@
#include "pvr_device.h"
#include "pvr_fw.h"
#include "pvr_fw_info.h"
+#include "pvr_fw_meta.h"
#include "pvr_gem.h"
#include "pvr_rogue_cr_defs.h"
#include "pvr_rogue_meta.h"
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c
b/drivers/gpu/drm/imagination/pvr_vm.c
index 2aab53594a77..30ecd7d7052e 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -539,7 +539,7 @@ pvr_device_addr_and_size_are_valid(struct pvr_vm_context
*vm_ctx,
(device_addr + size <= PVR_PAGE_TABLE_ADDR_SPACE_SIZE);
}
-void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
+static void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
{
kfree(to_pvr_vm_context(gpuvm));
}
--
2.39.2
[PATCH 2/2] drm/imagination: avoid -Woverflow warning
From: Arnd Bergmann
The array size calculation in pvr_vm_mips_fini() appears to be incorrect based
on
taking the size of the pointer rather than the size of the array, which
manifests
as a warning about signed integer overflow:
In file included from include/linux/kernel.h:16,
from drivers/gpu/drm/imagination/pvr_rogue_fwif.h:10,
from drivers/gpu/drm/imagination/pvr_ccb.h:7,
from drivers/gpu/drm/imagination/pvr_device.h:7,
from drivers/gpu/drm/imagination/pvr_vm_mips.c:4:
drivers/gpu/drm/imagination/pvr_vm_mips.c: In function 'pvr_vm_mips_fini':
include/linux/array_size.h:11:25: error: overflow in conversion from 'long
unsigned int' to 'int' changes value from '18446744073709551615' to '-1'
[-Werror=overflow]
11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
__must_be_array(arr))
| ^
drivers/gpu/drm/imagination/pvr_vm_mips.c:106:24: note: in expansion of macro
'ARRAY_SIZE'
106 | for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr >=
0; page_nr--) {
|^~
Just use the number of array elements directly here, and in the corresponding
init function for consistency.
Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and
MMU support")
Signed-off-by: Arnd Bergmann
---
drivers/gpu/drm/imagination/pvr_vm_mips.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c
b/drivers/gpu/drm/imagination/pvr_vm_mips.c
index 7268cf6e630b..6c2e4cc4e6db 100644
--- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
+++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
@@ -46,7 +46,7 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
if (!mips_data)
return -ENOMEM;
- for (page_nr = 0; page_nr < ARRAY_SIZE(mips_data->pt_pages); page_nr++)
{
+ for (page_nr = 0; page_nr < PVR_MIPS_PT_PAGE_COUNT; page_nr++) {
mips_data->pt_pages[page_nr] = alloc_page(GFP_KERNEL |
__GFP_ZERO);
if (!mips_data->pt_pages[page_nr]) {
err = -ENOMEM;
@@ -103,7 +103,7 @@ pvr_vm_mips_fini(struct pvr_device *pvr_dev)
int page_nr;
vunmap(mips_data->pt);
- for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr >= 0;
page_nr--) {
+ for (page_nr = PVR_MIPS_PT_PAGE_COUNT - 1; page_nr >= 0; page_nr--) {
dma_unmap_page(from_pvr_device(pvr_dev)->dev,
mips_data->pt_dma_addr[page_nr], PAGE_SIZE,
DMA_TO_DEVICE);
--
2.39.2
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Wed, 29 Nov 2023, Maxime Ripard wrote: > On Wed, Nov 29, 2023 at 11:38:42AM +0200, Jani Nikula wrote: >> On Wed, 29 Nov 2023, Maxime Ripard wrote: >> > Hi Ville, >> > >> > On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: >> >> On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: >> >> > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: >> >> > > On Tue, 28 Nov 2023, Maxime Ripard wrote: >> >> > > > All the drm_connector_init variants take at least a pointer to the >> >> > > > device, connector and hooks implementation. >> >> > > > >> >> > > > However, none of them check their value before dereferencing those >> >> > > > pointers which can lead to a NULL-pointer dereference if the author >> >> > > > isn't careful. >> >> > > >> >> > > Arguably oopsing on the spot is preferrable when this can't be caused >> >> > > by >> >> > > user input. It's always a mistake that should be caught early during >> >> > > development. >> >> > > >> >> > > Not everyone checks the return value of drm_connector_init and >> >> > > friends, >> >> > > so those cases will lead to more mysterious bugs later. And probably >> >> > > oopses as well. >> >> > >> >> > So maybe we can do both then, with something like >> >> > >> >> > if (WARN_ON(!dev)) >> >> >return -EINVAL >> >> > >> >> > if (drm_WARN_ON(dev, !connector || !funcs)) >> >> >return -EINVAL; >> >> > >> >> > I'd still like to check for this, so we can have proper testing, and we >> >> > already check for those pointers in some places (like funcs in >> >> > drm_connector_init), so if we don't cover everything we're inconsistent. >> >> >> >> People will invariably cargo-cult this kind of stuff absolutely >> >> everywhere and then all your functions will have tons of dead >> >> code to check their arguments. >> > >> > And that's a bad thing because... ? >> > >> > Also, are you really saying that checking that your arguments make sense >> > is cargo-cult? >> >> It's a powerful thing to be able to assume a NULL argument is always a >> fatal programming error on the caller's side, and should oops and get >> caught immediately. It's an assertion. > > Yeah, but we're not really doing that either. We have no explicit > assertion anywhere. We take a pointer in, and just hope that it will be > dereferenced later on and that the kernel will crash. The pointer to the > functions especially is only deferenced very later on. > > And assertions might be powerful, but being able to notice errors and > debug them is too. A panic takes away basically any remote access to > debug. If you don't have a console, you're done. > >> We're not talking about user input or anything like that here. >> >> If you start checking for things that can't happen, and return errors >> for them, you start gracefully handling things that don't have anything >> graceful about them. > > But there's nothing graceful to do here: you just return from your probe > function that you couldn't probe and that's it. Just like you do when > you can't map your registers, or get your interrupt, or register into > any framework (including drm_dev_register that pretty much every driver > handles properly if it returns an error, without being graceful about > it). Those are all dynamic things that can fail. Quite different from passing NULL dev, connector, or funcs to drm_connector_init() and friends. I think it's wrong to set the example that everything needs to be checked, everything needs to return an error, every call needs to check for error return, all the time, everywhere. People absolutely will cargo cult that, and that's what Ville is referring to. If you pass NULL dev, connector, or funcs to drm_connector_init() I think you absolutely deserve to get an oops. For dev, you could possibly not have reached the function with NULL dev. (And __drm_connector_init() has dev->mode_config before the check, so you'll get a static analyzer warning about dereference before the check.) If you have NULL connector, you didn't check for allocation failure earlier. If you have NULL funcs, you just passed NULL, because it's generally supposed to be a pointer to a static const struct. >> Having such checks in place trains people to think they *may* happen. > > In most cases, kmalloc can't fail. We seem to have a very different > policy towards it. Again, dynamic in nature and can fail. >> While it should fail fast and loud at the developer's first smoke test, >> and get fixed then and there. > > Returning an error + a warning also qualifies for "fail fast and loud". > But keeps the system alive for someone to notice in any case. But where do you draw the line? If we keep adding these checks to things that actually can't happen, we teach developers we need to check for impossible things. And we teach them not to trust anything. I scroll down the file and reach drm_connector_attach_edid_property(). Should we NULL check connector? Should we change the function to int and return a v
Re: [bug report] backlight: mp3309c: Add support for MPS MP3309C
On Wed, Nov 29, 2023 at 11:12:29AM +, Flavio Suligoi wrote: > Hi Dan, > > Can I add the "Reported-by" tag, with your name, in my 2nd vers of > the commit to fix this bug? Yeah. Thanks! If the bug report is sent to a public mailing list then there is no need to ask. regards, dan carpenter
Re: [PATCH 2/2] drm/imagination: avoid -Woverflow warning
Hello Arnd,
Thanks for the patch. I'm slightly concerned that we've not seen this warning
when
building here. I guess we need to check our CI settings...
Reviewed-by: Donald Robson
On Wed, 2023-11-29 at 12:33 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The array size calculation in pvr_vm_mips_fini() appears to be incorrect
> based on
> taking the size of the pointer rather than the size of the array, which
> manifests
> as a warning about signed integer overflow:
>
> In file included from include/linux/kernel.h:16,
> from drivers/gpu/drm/imagination/pvr_rogue_fwif.h:10,
> from drivers/gpu/drm/imagination/pvr_ccb.h:7,
> from drivers/gpu/drm/imagination/pvr_device.h:7,
> from drivers/gpu/drm/imagination/pvr_vm_mips.c:4:
> drivers/gpu/drm/imagination/pvr_vm_mips.c: In function 'pvr_vm_mips_fini':
> include/linux/array_size.h:11:25: error: overflow in conversion from 'long
> unsigned int' to 'int' changes value from '18446744073709551615' to '-1'
> [-Werror=overflow]
>11 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> | ^
> drivers/gpu/drm/imagination/pvr_vm_mips.c:106:24: note: in expansion of macro
> 'ARRAY_SIZE'
> 106 | for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr
> >= 0; page_nr--) {
> |^~
>
> Just use the number of array elements directly here, and in the corresponding
> init function for consistency.
>
> Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and
> MMU support")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/imagination/pvr_vm_mips.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> index 7268cf6e630b..6c2e4cc4e6db 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> @@ -46,7 +46,7 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
> if (!mips_data)
> return -ENOMEM;
>
> - for (page_nr = 0; page_nr < ARRAY_SIZE(mips_data->pt_pages); page_nr++)
> {
> + for (page_nr = 0; page_nr < PVR_MIPS_PT_PAGE_COUNT; page_nr++) {
> mips_data->pt_pages[page_nr] = alloc_page(GFP_KERNEL |
> __GFP_ZERO);
> if (!mips_data->pt_pages[page_nr]) {
> err = -ENOMEM;
> @@ -103,7 +103,7 @@ pvr_vm_mips_fini(struct pvr_device *pvr_dev)
> int page_nr;
>
> vunmap(mips_data->pt);
> - for (page_nr = ARRAY_SIZE(mips_data->pt_pages) - 1; page_nr >= 0;
> page_nr--) {
> + for (page_nr = PVR_MIPS_PT_PAGE_COUNT - 1; page_nr >= 0; page_nr--) {
> dma_unmap_page(from_pvr_device(pvr_dev)->dev,
> mips_data->pt_dma_addr[page_nr], PAGE_SIZE,
> DMA_TO_DEVICE);
>
[PATCH v8 0/8] Improve test coverage of TTM
Add tests for building blocks of the TTM subsystem, such as ttm_resource, ttm_resource_manager, ttm_tt and ttm_buffer_object. This series covers basic functions such as initialization, allocation and clean-up of each struct. Testing of ttm_buffer_object also includes locking and unlocking the object for validation, with special scenarios such as an interrupted wait or deadlock. Some of the test cases check the bulk move mechanism and how it interacts with pinned buffers. This is to be seen if we want to add dedicated testing for bulk move or not. The resource allocation subtests use ttm_sys_manager for now. Resources that don't use system memory will be indirectly tested via tests for ttm_bo_validate()/ttm_bo_init_validate(), using a mock resource manager. Use kunit_tool script to manually run all the tests: $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/ttm/tests To build a kernel with TTM KUnit tests, first enable CONFIG_KUNIT, and then CONFIG_DRM_TTM_KUNIT_TEST. Many thanks, Karolina v8: - Add Tested-by tags to commits that introduce tests - Improve the comment for ttm_bo_reserve_deadlock() subtest (Andi) - Actually clean up the resource when "error_free_blocks" is hit in ttm_mock_manager_alloc(). Without that change, we hit DEBUG_LOCKS_WARN_ON(lock->magic != lock) warning when cleaning up the resource manager because we try clean up an incomplete, orphaned resource. That's not good, and this could bite us back in the future. v7: - Drop size argument from ttm_place_kunit_init(), it's no longer needed - Delete a TODO comment from ttm_bo_validate_tests.c - First evict BOs before calling ttm_resource_manager_set_used() in ttm_mock_manager_fini() - Stop calling ttm_resource_manager_cleanup() as a part of the mock manager fini sequence. It frees a move fence that is allocated via KUnit allocator, which gets freed again as a part of the test cleanup - Set use_tt to true in mock manager and stop passing in the flag for it - Make ttm_dev_empty_funcs static (drivers/gpu/drm/ttm/tests/ttm_tt_test.c:232:25: sparse: sparse: symbol 'ttm_dev_empty_funcs' was not declared. Should it be static?) - Cast bo->base.resv->fences to a generic pointer before it's checked by KUnit (drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c:98:9: sparse: sparse: incompatible types in comparison expression (different base types)) - Clean up mock managers in ttm_bo_validate_move_fence_not_signaled subtest v6: - Include tests for ttm_bo_init_reserved() and ttm_bo_validate(), with a mock resource manager (patches 6-8; no eviction testing) - Add ttm_test_devices_all_init() helper to also init ttm_device instance - Remove fpfn and lpfn from ttm_place_kunit_init() helper -- these fields are neither used nor tested v5: - Actually use the page_flags parameter in ttm_tt_simple_create() v4: - First unreserve the object before calling ww_acquire_fini() in ttm_bo_reserve_double_resv subtest - Silence lockdep in ttm_bo_reserve_deadlock subtest (open to suggestions how to fix it in a different way) - Use a genuine GEM object in ttm_buffer_object instead of an empty one v3: - Instead of modifying the main TTM Makefile, use EXPORT_SYMBOL_FOR_TESTS_ONLY() macro for symbols that are tested but not widely exported. Thanks to this change, TTM tests can be built as modules, even when non-exported functions are used - Change the description of a patch that fixes ttm_pool_pre_populated() v2: - Remove Makefile for KUnit tests and move the definitions to the TTM's one - Switch on CONFIG_DRM_TTM_KUNIT_TEST=m so the tests and TTM module are built as one. This allows building the tests as a module, even if it uses functions that are not exported - Fix ttm_pool_pre_populated(); a wrong flag was passed to ttm_tt_kunit_init() function Karolina Stolarek (8): drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man drm/ttm/tests: Add tests for ttm_tt drm/ttm/tests: Add tests for ttm_bo functions drm/ttm/tests: Fix argument in ttm_tt_kunit_init() drm/ttm/tests: Use an init function from the helpers lib drm/ttm/tests: Test simple BO creation and validation drm/ttm/tests: Add tests with mock resource managers drm/ttm/tests: Add test cases dependent on fence signaling drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/ttm/tests/.kunitconfig| 1 + drivers/gpu/drm/ttm/tests/Makefile| 5 + drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 622 ++ .../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 795 ++ drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 109 ++- drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 7 + drivers/gpu/drm/ttm/tests/ttm_mock_manager.c | 207 + drivers/gpu/drm/ttm/tests/ttm_mock_manager.h | 31 + drivers/gpu/drm/ttm/tests/ttm_pool_test.c | 3 +- drivers/gpu/drm/ttm/tests/ttm_resource_test.c | 335
[PATCH v8 1/8] drm/ttm/tests: Add tests for ttm_resource and ttm_sys_man
Test initialization of ttm_resource using different memory domains.
Add tests for a system memory manager and functions that can be
tested without a fully-featured resource manager. Update
ttm_bo_kunit_init() to initialize BO's kref and a genuine GEM drm
object. Export ttm_resource_alloc for test purposes only.
Signed-off-by: Karolina Stolarek
Tested-by: Amaranath Somalapuram
Reviewed-by: Christian König
---
drivers/gpu/drm/ttm/tests/Makefile| 1 +
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 22 +-
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 3 +
drivers/gpu/drm/ttm/tests/ttm_resource_test.c | 335 ++
drivers/gpu/drm/ttm/ttm_resource.c| 3 +
5 files changed, 363 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_resource_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index ec87c4fc1ad5..c92fe2052ef6 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -3,4 +3,5 @@
obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_device_test.o \
ttm_pool_test.o \
+ttm_resource_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
index 81661d8827aa..779fbc038f17 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
@@ -29,19 +29,39 @@ struct ttm_buffer_object *ttm_bo_kunit_init(struct kunit
*test,
struct ttm_test_devices *devs,
size_t size)
{
- struct drm_gem_object gem_obj = { .size = size };
+ struct drm_gem_object gem_obj = { };
struct ttm_buffer_object *bo;
+ int err;
bo = kunit_kzalloc(test, sizeof(*bo), GFP_KERNEL);
KUNIT_ASSERT_NOT_NULL(test, bo);
bo->base = gem_obj;
+ err = drm_gem_object_init(devs->drm, &bo->base, size);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
bo->bdev = devs->ttm_dev;
+ kref_init(&bo->kref);
return bo;
}
EXPORT_SYMBOL_GPL(ttm_bo_kunit_init);
+struct ttm_place *ttm_place_kunit_init(struct kunit *test,
+ uint32_t mem_type, uint32_t flags)
+{
+ struct ttm_place *place;
+
+ place = kunit_kzalloc(test, sizeof(*place), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, place);
+
+ place->mem_type = mem_type;
+ place->flags = flags;
+
+ return place;
+}
+EXPORT_SYMBOL_GPL(ttm_place_kunit_init);
+
struct ttm_test_devices *ttm_test_devices_basic(struct kunit *test)
{
struct ttm_test_devices *devs;
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
index e261e3660d0b..2f51c833a536 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,6 +29,8 @@ int ttm_device_kunit_init(struct ttm_test_devices *priv,
struct ttm_buffer_object *ttm_bo_kunit_init(struct kunit *test,
struct ttm_test_devices *devs,
size_t size);
+struct ttm_place *ttm_place_kunit_init(struct kunit *test,
+ uint32_t mem_type, uint32_t flags);
struct ttm_test_devices *ttm_test_devices_basic(struct kunit *test);
struct ttm_test_devices *ttm_test_devices_all(struct kunit *test);
diff --git a/drivers/gpu/drm/ttm/tests/ttm_resource_test.c
b/drivers/gpu/drm/ttm/tests/ttm_resource_test.c
new file mode 100644
index ..029e1f094bb0
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_resource_test.c
@@ -0,0 +1,335 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define RES_SIZE SZ_4K
+#define TTM_PRIV_DUMMY_REG (TTM_NUM_MEM_TYPES - 1)
+
+struct ttm_resource_test_case {
+ const char *description;
+ uint32_t mem_type;
+ uint32_t flags;
+};
+
+struct ttm_resource_test_priv {
+ struct ttm_test_devices *devs;
+ struct ttm_buffer_object *bo;
+ struct ttm_place *place;
+};
+
+static const struct ttm_resource_manager_func ttm_resource_manager_mock_funcs
= { };
+
+static int ttm_resource_test_init(struct kunit *test)
+{
+ struct ttm_resource_test_priv *priv;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, priv);
+
+ priv->devs = ttm_test_devices_all(test);
+ KUNIT_ASSERT_NOT_NULL(test, priv->devs);
+
+ test->priv = priv;
+
+ return 0;
+}
+
+static void ttm_resource_test_fini(struct kunit *test)
+{
+ struct ttm_resource_test_priv *priv = test->priv;
+
+ ttm_test_devices_put(test, priv->devs);
+}
+
+st
[PATCH v8 2/8] drm/ttm/tests: Add tests for ttm_tt
Test initialization, creation and destruction of ttm_tt instances.
Export ttm_tt_destroy and ttm_tt_create symbols for testing purposes.
Signed-off-by: Karolina Stolarek
Reviewed-by: Christian König
Tested-by: Amaranath Somalapuram
---
drivers/gpu/drm/ttm/tests/Makefile| 1 +
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 20 ++
drivers/gpu/drm/ttm/tests/ttm_tt_test.c | 295 ++
drivers/gpu/drm/ttm/ttm_tt.c | 3 +
4 files changed, 319 insertions(+)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_tt_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index c92fe2052ef6..f570530bbb60 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_device_test.o \
ttm_pool_test.o \
ttm_resource_test.o \
+ttm_tt_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
index 779fbc038f17..ba4e5c689164 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
@@ -2,9 +2,29 @@
/*
* Copyright © 2023 Intel Corporation
*/
+#include
+
#include "ttm_kunit_helpers.h"
+static struct ttm_tt *ttm_tt_simple_create(struct ttm_buffer_object *bo,
+ uint32_t page_flags)
+{
+ struct ttm_tt *tt;
+
+ tt = kzalloc(sizeof(*tt), GFP_KERNEL);
+ ttm_tt_init(tt, bo, page_flags, ttm_cached, 0);
+
+ return tt;
+}
+
+static void ttm_tt_simple_destroy(struct ttm_device *bdev, struct ttm_tt *ttm)
+{
+ kfree(ttm);
+}
+
struct ttm_device_funcs ttm_dev_funcs = {
+ .ttm_tt_create = ttm_tt_simple_create,
+ .ttm_tt_destroy = ttm_tt_simple_destroy,
};
EXPORT_SYMBOL_GPL(ttm_dev_funcs);
diff --git a/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
b/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
new file mode 100644
index ..fd4502c18de6
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
@@ -0,0 +1,295 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define BO_SIZESZ_4K
+
+struct ttm_tt_test_case {
+ const char *description;
+ uint32_t size;
+ uint32_t extra_pages_num;
+};
+
+static int ttm_tt_test_init(struct kunit *test)
+{
+ struct ttm_test_devices *priv;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, priv);
+
+ priv = ttm_test_devices_all(test);
+ test->priv = priv;
+
+ return 0;
+}
+
+static const struct ttm_tt_test_case ttm_tt_init_basic_cases[] = {
+ {
+ .description = "Page-aligned size",
+ .size = SZ_4K,
+ },
+ {
+ .description = "Extra pages requested",
+ .size = SZ_4K,
+ .extra_pages_num = 1,
+ },
+};
+
+static void ttm_tt_init_case_desc(const struct ttm_tt_test_case *t,
+ char *desc)
+{
+ strscpy(desc, t->description, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(ttm_tt_init_basic, ttm_tt_init_basic_cases,
+ ttm_tt_init_case_desc);
+
+static void ttm_tt_init_basic(struct kunit *test)
+{
+ const struct ttm_tt_test_case *params = test->param_value;
+ struct ttm_buffer_object *bo;
+ struct ttm_tt *tt;
+ uint32_t page_flags = TTM_TT_FLAG_ZERO_ALLOC;
+ enum ttm_caching caching = ttm_cached;
+ uint32_t extra_pages = params->extra_pages_num;
+ int num_pages = params->size >> PAGE_SHIFT;
+ int err;
+
+ tt = kunit_kzalloc(test, sizeof(*tt), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, tt);
+
+ bo = ttm_bo_kunit_init(test, test->priv, params->size);
+
+ err = ttm_tt_init(tt, bo, page_flags, caching, extra_pages);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ KUNIT_ASSERT_EQ(test, tt->num_pages, num_pages + extra_pages);
+
+ KUNIT_ASSERT_EQ(test, tt->page_flags, page_flags);
+ KUNIT_ASSERT_EQ(test, tt->caching, caching);
+
+ KUNIT_ASSERT_NULL(test, tt->dma_address);
+ KUNIT_ASSERT_NULL(test, tt->swap_storage);
+}
+
+static void ttm_tt_init_misaligned(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ttm_tt *tt;
+ enum ttm_caching caching = ttm_cached;
+ uint32_t size = SZ_8K;
+ int num_pages = (size + SZ_4K) >> PAGE_SHIFT;
+ int err;
+
+ tt = kunit_kzalloc(test, sizeof(*tt), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, tt);
+
+ bo = ttm_bo_kunit_init(test, test->priv, size);
+
+ /* Make the object size misaligned */
+ bo->base.size += 1;
+
+ err = ttm_tt_init(tt, bo, 0, caching, 0);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ KUNIT_ASSERT_EQ(test,
[PATCH v8 6/8] drm/ttm/tests: Test simple BO creation and validation
Add tests for ttm_bo_init_reserved() and ttm_bo_validate() that use
sys manager. Define a simple move function in ttm_device_funcs. Expose
destroy callback of the buffer object to make testing of
ttm_bo_init_reserved() behaviour easier.
Signed-off-by: Karolina Stolarek
Reviewed-by: Christian König
Tested-by: Amaranath Somalapuram
---
drivers/gpu/drm/ttm/tests/Makefile| 1 +
.../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 211 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 14 +-
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 1 +
4 files changed, 226 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index 468535f7eed2..2e5ed63fb414 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -6,4 +6,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_resource_test.o \
ttm_tt_test.o \
ttm_bo_test.o \
+ttm_bo_validate_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
new file mode 100644
index ..1d50e4ba9775
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include
+#include
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define BO_SIZESZ_4K
+
+struct ttm_bo_validate_test_case {
+ const char *description;
+ enum ttm_bo_type bo_type;
+ bool with_ttm;
+};
+
+static struct ttm_placement *ttm_placement_kunit_init(struct kunit *test,
+ struct ttm_place *places,
+ unsigned int num_places,
+ struct ttm_place
*busy_places,
+ unsigned int
num_busy_places)
+{
+ struct ttm_placement *placement;
+
+ placement = kunit_kzalloc(test, sizeof(*placement), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, placement);
+
+ placement->num_placement = num_places;
+ placement->placement = places;
+ placement->num_busy_placement = num_busy_places;
+ placement->busy_placement = busy_places;
+
+ return placement;
+}
+
+static void ttm_bo_validate_case_desc(const struct ttm_bo_validate_test_case
*t,
+ char *desc)
+{
+ strscpy(desc, t->description, KUNIT_PARAM_DESC_SIZE);
+}
+
+static const struct ttm_bo_validate_test_case ttm_bo_type_cases[] = {
+ {
+ .description = "Buffer object for userspace",
+ .bo_type = ttm_bo_type_device,
+ },
+ {
+ .description = "Kernel buffer object",
+ .bo_type = ttm_bo_type_kernel,
+ },
+ {
+ .description = "Shared buffer object",
+ .bo_type = ttm_bo_type_sg,
+ },
+};
+
+KUNIT_ARRAY_PARAM(ttm_bo_types, ttm_bo_type_cases,
+ ttm_bo_validate_case_desc);
+
+static void ttm_bo_init_reserved_sys_man(struct kunit *test)
+{
+ const struct ttm_bo_validate_test_case *params = test->param_value;
+ struct ttm_test_devices *priv = test->priv;
+ struct ttm_buffer_object *bo;
+ struct ttm_place *place;
+ struct ttm_placement *placement;
+ enum ttm_bo_type bo_type = params->bo_type;
+ struct ttm_operation_ctx ctx = { };
+ uint32_t size = ALIGN(BO_SIZE, PAGE_SIZE);
+ int err;
+
+ bo = kunit_kzalloc(test, sizeof(*bo), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, bo);
+
+ place = ttm_place_kunit_init(test, TTM_PL_SYSTEM, 0);
+ placement = ttm_placement_kunit_init(test, place, 1, NULL, 0);
+
+ drm_gem_private_object_init(priv->drm, &bo->base, size);
+
+ err = ttm_bo_init_reserved(priv->ttm_dev, bo, bo_type, placement,
+ PAGE_SIZE, &ctx, NULL, NULL,
+ &dummy_ttm_bo_destroy);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_EXPECT_EQ(test, err, 0);
+ KUNIT_EXPECT_EQ(test, kref_read(&bo->kref), 1);
+ KUNIT_EXPECT_PTR_EQ(test, bo->bdev, priv->ttm_dev);
+ KUNIT_EXPECT_EQ(test, bo->type, bo_type);
+ KUNIT_EXPECT_EQ(test, bo->page_alignment, PAGE_SIZE);
+ KUNIT_EXPECT_PTR_EQ(test, bo->destroy, &dummy_ttm_bo_destroy);
+ KUNIT_EXPECT_EQ(test, bo->pin_count, 0);
+ KUNIT_EXPECT_NULL(test, bo->bulk_move);
+ KUNIT_EXPECT_NOT_NULL(test, bo->ttm);
+ KUNIT_EXPECT_FALSE(test, ttm_tt_is_populated(bo->ttm));
+ KUNIT_EXPECT_NOT_NULL(test, (void *)bo->base.resv->fences);
+ KUNIT_EXPECT_EQ(test, ctx.bytes_moved, size);
+
+ if (bo_type != ttm_bo_type_kernel)
+ KUNIT_EXPE
[PATCH v8 4/8] drm/ttm/tests: Fix argument in ttm_tt_kunit_init()
Remove a leftover definition of page order and pass an empty flag value in ttm_pool_pre_populated(). Signed-off-by: Karolina Stolarek Tested-by: Amaranath Somalapuram Reviewed-by: Dominik Karol Piątkowski Acked-by: Christian König --- drivers/gpu/drm/ttm/tests/ttm_pool_test.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/ttm/tests/ttm_pool_test.c b/drivers/gpu/drm/ttm/tests/ttm_pool_test.c index 2d9cae8cd984..b97f7b6daf5b 100644 --- a/drivers/gpu/drm/ttm/tests/ttm_pool_test.c +++ b/drivers/gpu/drm/ttm/tests/ttm_pool_test.c @@ -78,10 +78,9 @@ static struct ttm_pool *ttm_pool_pre_populated(struct kunit *test, struct ttm_test_devices *devs = priv->devs; struct ttm_pool *pool; struct ttm_tt *tt; - unsigned long order = __fls(size / PAGE_SIZE); int err; - tt = ttm_tt_kunit_init(test, order, caching, size); + tt = ttm_tt_kunit_init(test, 0, caching, size); KUNIT_ASSERT_NOT_NULL(test, tt); pool = kunit_kzalloc(test, sizeof(*pool), GFP_KERNEL); -- 2.34.1
[PATCH v8 8/8] drm/ttm/tests: Add test cases dependent on fence signaling
Add test cases that check how the state of dma fences in BO's
reservation object influence the ttm_bo_validation() flow. Do similar
tests for resource manager's move fence.
Signed-off-by: Karolina Stolarek
Tested-by: Amaranath Somalapuram
---
.../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 308 ++
1 file changed, 308 insertions(+)
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
index 5f6c24979f83..a659fc837186 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
@@ -2,6 +2,8 @@
/*
* Copyright © 2023 Intel Corporation
*/
+#include
+#include
#include
#include
@@ -13,11 +15,14 @@
#define BO_SIZESZ_4K
#define MANAGER_SIZE SZ_1M
+static struct spinlock fence_lock;
+
struct ttm_bo_validate_test_case {
const char *description;
enum ttm_bo_type bo_type;
uint32_t mem_type;
bool with_ttm;
+ bool no_gpu_wait;
};
static struct ttm_placement *ttm_placement_kunit_init(struct kunit *test,
@@ -39,6 +44,43 @@ static struct ttm_placement *ttm_placement_kunit_init(struct
kunit *test,
return placement;
}
+static const char *fence_name(struct dma_fence *f)
+{
+ return "ttm-bo-validate-fence";
+}
+
+static const struct dma_fence_ops fence_ops = {
+ .get_driver_name = fence_name,
+ .get_timeline_name = fence_name,
+};
+
+static struct dma_fence *alloc_mock_fence(struct kunit *test)
+{
+ struct dma_fence *fence;
+
+ fence = kunit_kzalloc(test, sizeof(*fence), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, fence);
+
+ dma_fence_init(fence, &fence_ops, &fence_lock, 0, 0);
+
+ return fence;
+}
+
+static void dma_resv_kunit_active_fence_init(struct kunit *test,
+struct dma_resv *resv,
+enum dma_resv_usage usage)
+{
+ struct dma_fence *fence;
+
+ fence = alloc_mock_fence(test);
+ dma_fence_enable_sw_signaling(fence);
+
+ dma_resv_lock(resv, NULL);
+ dma_resv_reserve_fences(resv, 1);
+ dma_resv_add_fence(resv, fence, usage);
+ dma_resv_unlock(resv);
+}
+
static void ttm_bo_validate_case_desc(const struct ttm_bo_validate_test_case
*t,
char *desc)
{
@@ -460,6 +502,265 @@ static void ttm_bo_validate_multihop(struct kunit *test)
ttm_mock_manager_fini(priv->ttm_dev, final_mem);
}
+static const struct ttm_bo_validate_test_case ttm_bo_no_placement_cases[] = {
+ {
+ .description = "Buffer object in system domain, no page vector",
+ },
+ {
+ .description = "Buffer object in system domain with an existing
page vector",
+ .with_ttm = true,
+ },
+};
+
+KUNIT_ARRAY_PARAM(ttm_bo_no_placement, ttm_bo_no_placement_cases,
+ ttm_bo_validate_case_desc);
+
+static void ttm_bo_validate_no_placement_signaled(struct kunit *test)
+{
+ const struct ttm_bo_validate_test_case *params = test->param_value;
+ struct ttm_test_devices *priv = test->priv;
+ struct ttm_buffer_object *bo;
+ struct ttm_place *place;
+ struct ttm_tt *old_tt;
+ struct ttm_placement *placement;
+ struct ttm_resource_manager *man;
+ uint32_t mem_type = TTM_PL_SYSTEM;
+ enum ttm_bo_type bo_type = ttm_bo_type_device;
+ struct ttm_operation_ctx ctx = { };
+ uint32_t size = ALIGN(BO_SIZE, PAGE_SIZE);
+ uint32_t flags;
+ int err;
+
+ place = ttm_place_kunit_init(test, mem_type, 0);
+ man = ttm_manager_type(priv->ttm_dev, mem_type);
+
+ bo = ttm_bo_kunit_init(test, test->priv, size);
+ bo->type = bo_type;
+
+ if (params->with_ttm) {
+ old_tt = priv->ttm_dev->funcs->ttm_tt_create(bo, 0);
+ ttm_pool_alloc(&priv->ttm_dev->pool, old_tt, &ctx);
+ bo->ttm = old_tt;
+ }
+
+ err = ttm_resource_alloc(bo, place, &bo->resource);
+ KUNIT_EXPECT_EQ(test, err, 0);
+ KUNIT_ASSERT_EQ(test, man->usage, size);
+
+ placement = kunit_kzalloc(test, sizeof(*placement), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, placement);
+
+ ttm_bo_reserve(bo, false, false, NULL);
+ err = ttm_bo_validate(bo, placement, &ctx);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_EXPECT_EQ(test, err, 0);
+ KUNIT_ASSERT_EQ(test, man->usage, 0);
+ KUNIT_ASSERT_NOT_NULL(test, bo->ttm);
+ KUNIT_EXPECT_EQ(test, ctx.bytes_moved, 0);
+
+ if (params->with_ttm) {
+ flags = bo->ttm->page_flags;
+
+ KUNIT_ASSERT_PTR_EQ(test, bo->ttm, old_tt);
+ KUNIT_ASSERT_FALSE(test, flags & TTM_TT_FLAG_PRIV_POPULATED);
+ KUNIT_ASSERT_TRUE(test, flags & TTM_TT_FLAG_ZERO_ALLOC);
+ }
+
+ ttm_bo_put(bo);
+}
+
+static int threaded_dma_res
[PATCH v8 5/8] drm/ttm/tests: Use an init function from the helpers lib
Add a new helper function that also initializes the device. Use it in
ttm_tt test suite and delete the local definition.
Signed-off-by: Karolina Stolarek
---
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 14 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 1 +
drivers/gpu/drm/ttm/tests/ttm_tt_test.c | 15 +--
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
index 7b7c1fa805fc..899a54dbe443 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
@@ -150,6 +150,20 @@ int ttm_test_devices_init(struct kunit *test)
}
EXPORT_SYMBOL_GPL(ttm_test_devices_init);
+int ttm_test_devices_all_init(struct kunit *test)
+{
+ struct ttm_test_devices *priv;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, priv);
+
+ priv = ttm_test_devices_all(test);
+ test->priv = priv;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(ttm_test_devices_all_init);
+
void ttm_test_devices_fini(struct kunit *test)
{
ttm_test_devices_put(test, test->priv);
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
index 2f51c833a536..53bb5834939f 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
@@ -39,6 +39,7 @@ void ttm_test_devices_put(struct kunit *test, struct
ttm_test_devices *devs);
/* Generic init/fini for tests that only need DRM/TTM devices */
int ttm_test_devices_init(struct kunit *test);
+int ttm_test_devices_all_init(struct kunit *test);
void ttm_test_devices_fini(struct kunit *test);
#endif // TTM_KUNIT_HELPERS_H
diff --git a/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
b/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
index fd4502c18de6..a33a426a814d 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_tt_test.c
@@ -15,19 +15,6 @@ struct ttm_tt_test_case {
uint32_t extra_pages_num;
};
-static int ttm_tt_test_init(struct kunit *test)
-{
- struct ttm_test_devices *priv;
-
- priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
- KUNIT_ASSERT_NOT_NULL(test, priv);
-
- priv = ttm_test_devices_all(test);
- test->priv = priv;
-
- return 0;
-}
-
static const struct ttm_tt_test_case ttm_tt_init_basic_cases[] = {
{
.description = "Page-aligned size",
@@ -285,7 +272,7 @@ static struct kunit_case ttm_tt_test_cases[] = {
static struct kunit_suite ttm_tt_test_suite = {
.name = "ttm_tt",
- .init = ttm_tt_test_init,
+ .init = ttm_test_devices_all_init,
.exit = ttm_test_devices_fini,
.test_cases = ttm_tt_test_cases,
};
--
2.34.1
[PATCH v8 7/8] drm/ttm/tests: Add tests with mock resource managers
Add mock resource manager to test ttm_bo_validate() with non-system
placements. Update KConfig entry to enable DRM Buddy allocator, used
by the mock manager. Update move function to do more than just assign
a resource.
Signed-off-by: Karolina Stolarek
Reviewed-by: Christian König
Tested-by: Amaranath Somalapuram
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/ttm/tests/.kunitconfig| 1 +
drivers/gpu/drm/ttm/tests/Makefile| 1 +
.../gpu/drm/ttm/tests/ttm_bo_validate_test.c | 276 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 39 ++-
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 2 +
drivers/gpu/drm/ttm/tests/ttm_mock_manager.c | 207 +
drivers/gpu/drm/ttm/tests/ttm_mock_manager.h | 31 ++
8 files changed, 556 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_mock_manager.c
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_mock_manager.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 740c1c0bd068..3be7afa6df3e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -200,6 +200,7 @@ config DRM_TTM_KUNIT_TEST
default n
depends on DRM && KUNIT && MMU
select DRM_TTM
+select DRM_BUDDY
select DRM_EXPORT_FOR_TESTS if m
select DRM_KUNIT_TEST_HELPERS
default KUNIT_ALL_TESTS
diff --git a/drivers/gpu/drm/ttm/tests/.kunitconfig
b/drivers/gpu/drm/ttm/tests/.kunitconfig
index 75fdce0cd98e..9228ce9b913c 100644
--- a/drivers/gpu/drm/ttm/tests/.kunitconfig
+++ b/drivers/gpu/drm/ttm/tests/.kunitconfig
@@ -2,3 +2,4 @@ CONFIG_KUNIT=y
CONFIG_DRM=y
CONFIG_DRM_KUNIT_TEST_HELPERS=y
CONFIG_DRM_TTM_KUNIT_TEST=y
+CONFIG_DRM_BUDDY=y
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index 2e5ed63fb414..f3149de77541 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -7,4 +7,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_tt_test.o \
ttm_bo_test.o \
ttm_bo_validate_test.o \
+ttm_mock_manager.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
index 1d50e4ba9775..5f6c24979f83 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
@@ -8,12 +8,15 @@
#include
#include "ttm_kunit_helpers.h"
+#include "ttm_mock_manager.h"
#define BO_SIZESZ_4K
+#define MANAGER_SIZE SZ_1M
struct ttm_bo_validate_test_case {
const char *description;
enum ttm_bo_type bo_type;
+ uint32_t mem_type;
bool with_ttm;
};
@@ -106,6 +109,49 @@ static void ttm_bo_init_reserved_sys_man(struct kunit
*test)
ttm_bo_put(bo);
}
+static void ttm_bo_init_reserved_mock_man(struct kunit *test)
+{
+ const struct ttm_bo_validate_test_case *params = test->param_value;
+ struct ttm_test_devices *priv = test->priv;
+ struct ttm_buffer_object *bo;
+ struct ttm_place *place;
+ struct ttm_placement *placement;
+ enum ttm_bo_type bo_type = params->bo_type;
+ struct ttm_operation_ctx ctx = { };
+ uint32_t mem_type = TTM_PL_VRAM;
+ uint32_t size = ALIGN(BO_SIZE, PAGE_SIZE);
+ int err;
+
+ ttm_mock_manager_init(priv->ttm_dev, mem_type, MANAGER_SIZE);
+
+ bo = kunit_kzalloc(test, sizeof(*bo), GFP_KERNEL);
+ KUNIT_ASSERT_NOT_NULL(test, bo);
+
+ place = ttm_place_kunit_init(test, mem_type, 0);
+ placement = ttm_placement_kunit_init(test, place, 1, NULL, 0);
+
+ drm_gem_private_object_init(priv->drm, &bo->base, size);
+
+ err = ttm_bo_init_reserved(priv->ttm_dev, bo, bo_type, placement,
+ PAGE_SIZE, &ctx, NULL, NULL,
+ &dummy_ttm_bo_destroy);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_EXPECT_EQ(test, err, 0);
+ KUNIT_EXPECT_EQ(test, kref_read(&bo->kref), 1);
+ KUNIT_EXPECT_PTR_EQ(test, bo->bdev, priv->ttm_dev);
+ KUNIT_EXPECT_EQ(test, bo->type, bo_type);
+ KUNIT_EXPECT_EQ(test, ctx.bytes_moved, size);
+
+ if (bo_type != ttm_bo_type_kernel)
+ KUNIT_EXPECT_TRUE(test,
+
drm_mm_node_allocated(&bo->base.vma_node.vm_node));
+
+ ttm_resource_free(bo, &bo->resource);
+ ttm_bo_put(bo);
+ ttm_mock_manager_fini(priv->ttm_dev, mem_type);
+}
+
static void ttm_bo_init_reserved_resv(struct kunit *test)
{
struct ttm_test_devices *priv = test->priv;
@@ -140,6 +186,51 @@ static void ttm_bo_init_reserved_resv(struct kunit *test)
ttm_bo_put(bo);
}
+static void ttm_bo_validate_basic(struct kunit *test)
+{
+ const struct ttm_bo_validate_test_case *params = test->param_value;
+ struct ttm_test_devices *priv = test->priv;
+ struct ttm_buffer_object *bo;
[PATCH v8 3/8] drm/ttm/tests: Add tests for ttm_bo functions
Test reservation and release of TTM buffer objects. Add tests to check
pin and unpin operations.
Signed-off-by: Karolina Stolarek
Tested-by: Amaranath Somalapuram
Reviewed-by: Andi Shyti
---
drivers/gpu/drm/ttm/tests/Makefile| 1 +
drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 622 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 6 +
3 files changed, 629 insertions(+)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index f570530bbb60..468535f7eed2 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_pool_test.o \
ttm_resource_test.o \
ttm_tt_test.o \
+ttm_bo_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
new file mode 100644
index ..1f8a4f8adc92
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
@@ -0,0 +1,622 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define BO_SIZESZ_8K
+
+struct ttm_bo_test_case {
+ const char *description;
+ bool interruptible;
+ bool no_wait;
+};
+
+static const struct ttm_bo_test_case ttm_bo_reserved_cases[] = {
+ {
+ .description = "Cannot be interrupted and sleeps",
+ .interruptible = false,
+ .no_wait = false,
+ },
+ {
+ .description = "Cannot be interrupted, locks straight away",
+ .interruptible = false,
+ .no_wait = true,
+ },
+ {
+ .description = "Can be interrupted, sleeps",
+ .interruptible = true,
+ .no_wait = false,
+ },
+};
+
+static void ttm_bo_init_case_desc(const struct ttm_bo_test_case *t,
+ char *desc)
+{
+ strscpy(desc, t->description, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(ttm_bo_reserve, ttm_bo_reserved_cases,
ttm_bo_init_case_desc);
+
+static void ttm_bo_reserve_optimistic_no_ticket(struct kunit *test)
+{
+ const struct ttm_bo_test_case *params = test->param_value;
+ struct ttm_buffer_object *bo;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, params->interruptible, params->no_wait, NULL);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ dma_resv_unlock(bo->base.resv);
+}
+
+static void ttm_bo_reserve_locked_no_sleep(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ /* Let's lock it beforehand */
+ dma_resv_lock(bo->base.resv, NULL);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, NULL);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+}
+
+static void ttm_bo_reserve_no_wait_ticket(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ ww_acquire_init(&ctx, &reservation_ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, &ctx);
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+
+ ww_acquire_fini(&ctx);
+}
+
+static void ttm_bo_reserve_double_resv(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ ww_acquire_init(&ctx, &reservation_ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, &ctx);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, &ctx);
+
+ dma_resv_unlock(bo->base.resv);
+ ww_acquire_fini(&ctx);
+
+ KUNIT_ASSERT_EQ(test, err, -EALREADY);
+}
+
+/*
+ * A test case heavily inspired by ww_test_edeadlk_normal(). It injects
+ * a deadlock by manipulating the sequence number of the context that holds
+ * dma_resv lock of bo2 so the other context is "wounded" and has to back off
+ * (indicated by -EDEADLK). The subtest checks if ttm_bo_reserve() properly
+ * propagates that error.
+ */
+static void ttm_bo_reserve_deadlock(struct kunit *test)
+{
+ struct ttm_buffer_object *bo1, *bo2;
+ struct ww_acquire_ctx ctx1, ctx2;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ bo1 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
Re: [PATCH 2/2] drm/imagination: avoid -Woverflow warning
On Wed, Nov 29, 2023, at 13:01, Donald Robson wrote: > Hello Arnd, > > Thanks for the patch. I'm slightly concerned that we've not seen this > warning when > building here. I guess we need to check our CI settings... > > Reviewed-by: Donald Robson This was previously enabled only when building with either "make W=1" or "make C=1", but not the default build, which explains why it only showed up after the merge into linux-next that has the corresponding scripts/Makefile.extrawarn change. It would still be a good idea to add the extra compiler (W=1) and sparse (C=1) warnings in your CI system and address all other issues that might uncover. Arnd
Re: [PATCH 1/2] drm/imagination: avoid -Wmissing-prototype warnings
Hello Arnd,
Thanks for this. However, I fixed these in a patch a few minutes before you
sent
yours. I'm not sure what normally happens in these circumstances, but as my
patch has
the kernel robot tags and a few additional fixes, I think we should probably
use that
one.
Thanks,
Donald
On Wed, 2023-11-29 at 12:33 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> This warning option is now enabled by default, causing a few build regressions
> in combination with the newly added pvr driver:
>
> drivers/gpu/drm/imagination/pvr_device.c:130:6: error: no previous prototype
> for 'pvr_device_process_active_queues' [-Werror=missing-prototypes]
> 130 | void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
> | ^~~~
> drivers/gpu/drm/imagination/pvr_fw_meta.c:33:1: error: no previous prototype
> for 'pvr_meta_cr_read32' [-Werror=missing-prototypes]
>33 | pvr_meta_cr_read32(struct pvr_device *pvr_dev, u32 reg_addr, u32
> *reg_value_out)
> | ^~
> drivers/gpu/drm/imagination/pvr_vm.c:542:6: error: no previous prototype for
> 'pvr_gpuvm_free' [-Werror=missing-prototypes]
> 542 | void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
>
> Mark pvr_device_process_active_queues and pvr_gpuvm_free static as they are
> only used
> in the file they are defined in, and include the correct header for the
> pvr_meta_cr_read32
> declaration.
>
> Fixes: eaf01ee5ba28 ("drm/imagination: Implement job submission and
> scheduling")
> Fixes: cc1aeedb98ad ("drm/imagination: Implement firmware infrastructure and
> META FW support")
> Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/imagination/pvr_device.c | 2 +-
> drivers/gpu/drm/imagination/pvr_fw_meta.c | 1 +
> drivers/gpu/drm/imagination/pvr_vm.c | 2 +-
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/imagination/pvr_device.c
> b/drivers/gpu/drm/imagination/pvr_device.c
> index 8499becf4fbb..048eba776cf2 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.c
> +++ b/drivers/gpu/drm/imagination/pvr_device.c
> @@ -127,7 +127,7 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
> * This is called any time we receive a FW event. It iterates over all
> * active queues and calls pvr_queue_process() on them.
> */
> -void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
> +static void pvr_device_process_active_queues(struct pvr_device *pvr_dev)
> {
> struct pvr_queue *queue, *tmp_queue;
> LIST_HEAD(active_queues);
> diff --git a/drivers/gpu/drm/imagination/pvr_fw_meta.c
> b/drivers/gpu/drm/imagination/pvr_fw_meta.c
> index 119934c36184..c39beb70c317 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_meta.c
> +++ b/drivers/gpu/drm/imagination/pvr_fw_meta.c
> @@ -4,6 +4,7 @@
> #include "pvr_device.h"
> #include "pvr_fw.h"
> #include "pvr_fw_info.h"
> +#include "pvr_fw_meta.h"
> #include "pvr_gem.h"
> #include "pvr_rogue_cr_defs.h"
> #include "pvr_rogue_meta.h"
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c
> b/drivers/gpu/drm/imagination/pvr_vm.c
> index 2aab53594a77..30ecd7d7052e 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -539,7 +539,7 @@ pvr_device_addr_and_size_are_valid(struct pvr_vm_context
> *vm_ctx,
> (device_addr + size <= PVR_PAGE_TABLE_ADDR_SPACE_SIZE);
> }
>
> -void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
> +static void pvr_gpuvm_free(struct drm_gpuvm *gpuvm)
> {
> kfree(to_pvr_vm_context(gpuvm));
> }
Re: [PATCH] drm/imagination: Fixed clang warnings for initial upstreamed patches.
Hi Donald, On Wed, Nov 29, 2023 at 11:20:14AM +, Donald Robson wrote: > Reported-by: kernel test robot > Closes: > https://lore.kernel.org/oe-kbuild-all/202311242159.hh8mwiam-...@intel.com/ > Closes: > https://lore.kernel.org/oe-kbuild-all/202311241752.3ilyyfca-...@intel.com/ > Closes: > https://lore.kernel.org/oe-kbuild-all/202311250632.givex7mu-...@intel.com/ > Closes: > https://lore.kernel.org/oe-kbuild-all/202311250226.da2yiskp-...@intel.com/ > Signed-off-by: Donald Robson The expectation is that you send one patch per type of warning or bug report Maxime signature.asc Description: PGP signature
Re: [EXTERNAL] Re: [PATCH 2/2] drm/imagination: avoid -Woverflow warning
On Wed, 2023-11-29 at 13:04 +0100, Arnd Bergmann wrote: > *** CAUTION: This email originates from a source not known to Imagination > Technologies. Think before you click a link or open an attachment *** > > On Wed, Nov 29, 2023, at 13:01, Donald Robson wrote: > > Hello Arnd, > > > > Thanks for the patch. I'm slightly concerned that we've not seen this > > warning when > > building here. I guess we need to check our CI settings... > > > > Reviewed-by: Donald Robson > > This was previously enabled only when building with either > "make W=1" or "make C=1", but not the default build, which > explains why it only showed up after the merge into linux-next > that has the corresponding scripts/Makefile.extrawarn change. > > It would still be a good idea to add the extra compiler (W=1) > and sparse (C=1) warnings in your CI system and address all > other issues that might uncover. > > Arnd Thanks Arnd, we will do this.
Re: [PATCH 1/2] drm/imagination: avoid -Wmissing-prototype warnings
On Wed, Nov 29, 2023, at 13:07, Donald Robson wrote: > Hello Arnd, > > Thanks for this. However, I fixed these in a patch a few minutes > before you sent > yours. I'm not sure what normally happens in these circumstances, but > as my patch has > the kernel robot tags and a few additional fixes, I think we should > probably use that > one. Sure, that's fine. If you don't mind rebasing, just add a "Reported-by: Arnd Bergmann " line as well. I tend to create a bug fix for any build regressions I see as part of building randconfig kernels, but it's normal that the mainainers have already fixed the same bug before I send my patch, or that there is a better solution than what I come up with. Arnd
[PATCH v6 00/10] drm: ci: fixes
The patch series contains improvements, enabling new ci jobs which enables testing for Mediatek MT8173, Qualcomm APQ 8016 and VirtIO GPU, fixing issues with the ci jobs and updating the expectation files. v2: - Use fdtoverlay command to merge overlay dtbo with the base dtb instead of modifying the kernel sources - Reworded the commit message for enabling jobs - Added a new patch in the series to use scripts/config to enable/disable configs v3: - New patch in the series to add device tree overlay in arch/arm64/boot/dts/qcom - drm-ci scripts to use device tree overlay from arch/arm64/boot/dts/qcom and compile base device tree with overlay support - New patch in the series to enable CONFIG_REGULATOR_DA9211 in defconfig - Remove CONFIG_RTC_DRV_MT6397=y as it is already enabled in defconfig v4: - Drop 'enable CONFIG_REGULATOR_DA9211 in defconfig' patch as it is sent upstream as a seperate patch - Use apq8016-sbc-usb-host.dtb which allows the USB controllers to work in host mode. This patch depends on https://lore.kernel.org/lkml/20230911161518.650726-1-vignesh.ra...@collabora.com/ v5: - Added a new patch in the series to set IGT_FORCE_DRIVER to 'mediatek' for mt8173 - Added a new patch in the series to make artifacts available for virtio jobs - Added a new patch in the series to add pipeline url to fails and flakes files - Generate fails and flakes file with the updated xfails script - https://www.spinics.net/lists/kernel/msg4959630.html - Drop 'virtio: Update ci variables' patch as the tests which causes the malloc issue are skipped v6: - Updated commit message for enable DA9211 regulator fix - Use GPU_VERSION instead of CI_JOB_NAME to check if it is mt8173 while setting IGT_FORCE_DRIVER - Added a new patch in the series to uprev IGT to fix memory corruption - Added a new patch in the series to update drm ci documentation - Generate fails file with drm-misc-next Vignesh Raman (10): drm: ci: igt_runner: Remove todo drm: ci: Force db410c to host mode drm: ci: arm64.config: Enable DA9211 regulator drm: ci: Enable new jobs drm: ci: Use scripts/config to enable/disable configs drm: ci: mediatek: Set IGT_FORCE_DRIVER for mt8173 drm: ci: virtio: Make artifacts available drm: ci: uprev IGT drm/doc: ci: Add IGT version details for flaky tests drm: ci: Update xfails Documentation/gpu/automated_testing.rst | 7 +-- drivers/gpu/drm/ci/arm64.config | 1 + drivers/gpu/drm/ci/build.sh | 16 +++ drivers/gpu/drm/ci/gitlab-ci.yml | 2 +- drivers/gpu/drm/ci/igt_runner.sh | 5 +- drivers/gpu/drm/ci/test.yml | 13 ++ .../drm/ci/xfails/mediatek-mt8173-fails.txt | 13 -- .../gpu/drm/ci/xfails/msm-apq8016-fails.txt | 5 ++ .../drm/ci/xfails/virtio_gpu-none-fails.txt | 46 +++ 9 files changed, 82 insertions(+), 26 deletions(-) -- 2.40.1
[PATCH v6 01/10] drm: ci: igt_runner: Remove todo
/sys/kernel/debug/dri/*/state exist for every atomic KMS driver. We do not test non-atomic drivers, so remove the todo. Acked-by: Helen Koike Signed-off-by: Vignesh Raman --- v2: - No changes v3: - No changes v4: - No changes v5: - No changes v6: - No changes --- drivers/gpu/drm/ci/igt_runner.sh | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/ci/igt_runner.sh b/drivers/gpu/drm/ci/igt_runner.sh index 2f815ee3a8a3..c6cf963592c5 100755 --- a/drivers/gpu/drm/ci/igt_runner.sh +++ b/drivers/gpu/drm/ci/igt_runner.sh @@ -15,7 +15,6 @@ cat /sys/kernel/debug/device_component/* ' # Dump drm state to confirm that kernel was able to find a connected display: -# TODO this path might not exist for all drivers.. maybe run modetest instead? set +e cat /sys/kernel/debug/dri/*/state set -e -- 2.40.1
[PATCH v6 03/10] drm: ci: arm64.config: Enable DA9211 regulator
Mediatek mt8173 board fails to boot with DA9211 regulator disabled. Enable CONFIG_REGULATOR_DA9211=y in arm64.config to fix mt8173 boot issue. Acked-by: Helen Koike Signed-off-by: Vignesh Raman --- v2: - No changes v3: - Remove CONFIG_RTC_DRV_MT6397=y as it is already enabled in defconfig v4: - No changes v5: - No changes v6: - Updated commit message --- drivers/gpu/drm/ci/arm64.config | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/ci/arm64.config b/drivers/gpu/drm/ci/arm64.config index b4f653417883..8dbce9919a57 100644 --- a/drivers/gpu/drm/ci/arm64.config +++ b/drivers/gpu/drm/ci/arm64.config @@ -186,6 +186,7 @@ CONFIG_HW_RANDOM_MTK=y CONFIG_MTK_DEVAPC=y CONFIG_PWM_MTK_DISP=y CONFIG_MTK_CMDQ=y +CONFIG_REGULATOR_DA9211=y # For nouveau. Note that DRM must be a module so that it's loaded after NFS is up to provide the firmware. CONFIG_ARCH_TEGRA=y -- 2.40.1
[PATCH v6 02/10] drm: ci: Force db410c to host mode
Force db410c to host mode to fix network issue which results in failure
to mount root fs via NFS.
See
https://gitlab.freedesktop.org/gfx-ci/linux/-/commit/cb72a629b8c15c80a54dda510743cefd1c4b65b8
Use apq8016-sbc-usb-host.dtb which allows the USB controllers
to work in host mode.
Acked-by: Helen Koike
Signed-off-by: Vignesh Raman
---
v2:
- Use fdtoverlay command to merge overlay dtbo with the base dtb instead of
modifying the kernel sources
v3:
- drm-ci scripts to use device tree overlay from arch/arm64/boot/dts/qcom and
compile base device tree with overlay support
v4:
- Use apq8016-sbc-usb-host.dtb which allows the USB controllers to work in
host mode.
This patch depends on
https://lore.kernel.org/lkml/20230911161518.650726-1-vignesh.ra...@collabora.com/
v5:
- No changes
v6:
- No changes
---
drivers/gpu/drm/ci/build.sh | 2 +-
drivers/gpu/drm/ci/test.yml | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
index e5c5dcedd108..e2260b4a1c67 100644
--- a/drivers/gpu/drm/ci/build.sh
+++ b/drivers/gpu/drm/ci/build.sh
@@ -19,7 +19,7 @@ if [[ "$KERNEL_ARCH" = "arm64" ]]; then
DEVICE_TREES+="
arch/arm64/boot/dts/amlogic/meson-gxl-s805x-libretech-ac.dtb"
DEVICE_TREES+=" arch/arm64/boot/dts/allwinner/sun50i-h6-pine-h64.dtb"
DEVICE_TREES+=" arch/arm64/boot/dts/amlogic/meson-gxm-khadas-vim2.dtb"
-DEVICE_TREES+=" arch/arm64/boot/dts/qcom/apq8016-sbc.dtb"
+DEVICE_TREES+=" arch/arm64/boot/dts/qcom/apq8016-sbc-usb-host.dtb"
DEVICE_TREES+=" arch/arm64/boot/dts/qcom/apq8096-db820c.dtb"
DEVICE_TREES+="
arch/arm64/boot/dts/amlogic/meson-g12b-a311d-khadas-vim3.dtb"
DEVICE_TREES+=" arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtb"
diff --git a/drivers/gpu/drm/ci/test.yml b/drivers/gpu/drm/ci/test.yml
index f285ed67eb3d..4af10f0ff82d 100644
--- a/drivers/gpu/drm/ci/test.yml
+++ b/drivers/gpu/drm/ci/test.yml
@@ -102,7 +102,7 @@ msm:apq8016:
stage: msm
variables:
DRIVER_NAME: msm
-BM_DTB: https://${PIPELINE_ARTIFACTS_BASE}/arm64/apq8016-sbc.dtb
+BM_DTB: https://${PIPELINE_ARTIFACTS_BASE}/arm64/apq8016-sbc-usb-host.dtb
GPU_VERSION: apq8016
BM_CMDLINE: "ip=dhcp console=ttyMSM0,115200n8 $BM_KERNEL_EXTRA_ARGS
root=/dev/nfs rw nfsrootdebug nfsroot=,tcp,nfsvers=4.2 init=/init
$BM_KERNELARGS"
RUNNER_TAG: google-freedreno-db410c
--
2.40.1
[PATCH v6 04/10] drm: ci: Enable new jobs
Enable the following jobs, as the issues noted in the TODO comments have been resolved. This will ensure that these jobs are now included and executed as part of the CI/CD pipeline. msm:apq8016: TODO: current issue: it is not fiding the NFS root. Fix and remove this rule. mediatek:mt8173: TODO: current issue: device is hanging. Fix and remove this rule. virtio_gpu:none: TODO: current issue: malloc(): corrupted top size. Fix and remove this rule. Acked-by: Helen Koike Signed-off-by: Vignesh Raman --- v2: - Reworded the commit message v3: - No changes v4: - No changes v5: - No changes v6: - No changes --- drivers/gpu/drm/ci/test.yml | 9 - 1 file changed, 9 deletions(-) diff --git a/drivers/gpu/drm/ci/test.yml b/drivers/gpu/drm/ci/test.yml index 4af10f0ff82d..e0fdc55c9b69 100644 --- a/drivers/gpu/drm/ci/test.yml +++ b/drivers/gpu/drm/ci/test.yml @@ -108,9 +108,6 @@ msm:apq8016: RUNNER_TAG: google-freedreno-db410c script: - ./install/bare-metal/fastboot.sh - rules: -# TODO: current issue: it is not fiding the NFS root. Fix and remove this rule. -- when: never msm:apq8096: extends: @@ -280,9 +277,6 @@ mediatek:mt8173: DEVICE_TYPE: mt8173-elm-hana GPU_VERSION: mt8173 RUNNER_TAG: mesa-ci-x86-64-lava-mt8173-elm-hana - rules: -# TODO: current issue: device is hanging. Fix and remove this rule. -- when: never mediatek:mt8183: extends: @@ -340,6 +334,3 @@ virtio_gpu:none: - debian/x86_64_test-gl - testing:x86_64 - igt:x86_64 - rules: -# TODO: current issue: malloc(): corrupted top size. Fix and remove this rule. -- when: never \ No newline at end of file -- 2.40.1
[PATCH v6 06/10] drm: ci: mediatek: Set IGT_FORCE_DRIVER for mt8173
Expected driver for mt8173 is "mediatek" and for mt8183
it is "panfrost". Set IGT_FORCE_DRIVER to 'mediatek' as
the expected driver for mt8173.
Signed-off-by: Vignesh Raman
---
v5:
- Added a new patch in the series to set IGT_FORCE_DRIVER to 'mediatek' for
mt8173
v6:
- Use GPU_VERSION instead of CI_JOB_NAME to check if it is mt8173
---
drivers/gpu/drm/ci/igt_runner.sh | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/ci/igt_runner.sh b/drivers/gpu/drm/ci/igt_runner.sh
index c6cf963592c5..70a0f84021a1 100755
--- a/drivers/gpu/drm/ci/igt_runner.sh
+++ b/drivers/gpu/drm/ci/igt_runner.sh
@@ -30,6 +30,10 @@ case "$DRIVER_NAME" in
;;
esac
+if [ "$GPU_VERSION" = "mt8173" ]; then
+export IGT_FORCE_DRIVER=${DRIVER_NAME}
+fi
+
if [ -e "/install/xfails/$DRIVER_NAME-$GPU_VERSION-skips.txt" ]; then
IGT_SKIPS="--skips /install/xfails/$DRIVER_NAME-$GPU_VERSION-skips.txt"
fi
--
2.40.1
[PATCH v6 05/10] drm: ci: Use scripts/config to enable/disable configs
Instead of modifying files in git to enable/disable
configs, use scripts/config on the .config file which
will be used for building the kernel.
Acked-by: Helen Koike
Suggested-by: Jani Nikula
Signed-off-by: Vignesh Raman
---
v2:
- Added a new patch in the series to use scripts/config to enable/disable
configs
v3:
- No changes
v4:
- No changes
v5:
- No changes
v6:
- No changes
---
drivers/gpu/drm/ci/build.sh | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/ci/build.sh b/drivers/gpu/drm/ci/build.sh
index e2260b4a1c67..97ff43759b91 100644
--- a/drivers/gpu/drm/ci/build.sh
+++ b/drivers/gpu/drm/ci/build.sh
@@ -75,19 +75,19 @@ else
fi
fi
-for opt in $ENABLE_KCONFIGS; do
- echo CONFIG_$opt=y >> drivers/gpu/drm/ci/${KERNEL_ARCH}.config
-done
-for opt in $DISABLE_KCONFIGS; do
- echo CONFIG_$opt=n >> drivers/gpu/drm/ci/${KERNEL_ARCH}.config
-done
-
if [[ -n "${MERGE_FRAGMENT}" ]]; then
./scripts/kconfig/merge_config.sh ${DEFCONFIG}
drivers/gpu/drm/ci/${MERGE_FRAGMENT}
else
make `basename ${DEFCONFIG}`
fi
+for opt in $ENABLE_KCONFIGS; do
+./scripts/config --enable CONFIG_$opt
+done
+for opt in $DISABLE_KCONFIGS; do
+./scripts/config --disable CONFIG_$opt
+done
+
make ${KERNEL_IMAGE_NAME}
mkdir -p /lava-files/
--
2.40.1
[PATCH v6 07/10] drm: ci: virtio: Make artifacts available
There were no artifacts available for virtio job. So make the artifacts available in the pipeline job. Signed-off-by: Vignesh Raman --- v5: - Added a new patch in the series to make artifacts available for virtio jobs v6: - No changes --- drivers/gpu/drm/ci/test.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/ci/test.yml b/drivers/gpu/drm/ci/test.yml index e0fdc55c9b69..2c9a1838e728 100644 --- a/drivers/gpu/drm/ci/test.yml +++ b/drivers/gpu/drm/ci/test.yml @@ -329,6 +329,8 @@ virtio_gpu:none: script: - ln -sf $CI_PROJECT_DIR/install /install - mv install/bzImage /lava-files/bzImage +- mkdir -p $CI_PROJECT_DIR/results +- ln -sf $CI_PROJECT_DIR/results /results - install/crosvm-runner.sh install/igt_runner.sh needs: - debian/x86_64_test-gl -- 2.40.1
[PATCH v6 09/10] drm/doc: ci: Add IGT version details for flaky tests
Document the IGT version in the flaky tests reporting template. Signed-off-by: Vignesh Raman --- v6: - Added a new patch in the series to update drm ci documentation --- Documentation/gpu/automated_testing.rst | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Documentation/gpu/automated_testing.rst b/Documentation/gpu/automated_testing.rst index 240e29d5ba68..2d5a28866afe 100644 --- a/Documentation/gpu/automated_testing.rst +++ b/Documentation/gpu/automated_testing.rst @@ -69,14 +69,15 @@ the result. They will still be run. Each new flake entry must be associated with a link to the email reporting the bug to the author of the affected driver, the board name or Device Tree name of -the board, the first kernel version affected, and an approximation of the -failure rate. +the board, the first kernel version affected, the IGT version used for tests, +and an approximation of the failure rate. They should be provided under the following format:: # Bug Report: $LORE_OR_PATCHWORK_URL # Board Name: broken-board.dtb - # Version: 6.6-rc1 + # Linux Version: 6.6-rc1 + # IGT Version: 1.28-gd2af13d9f # Failure Rate: 100 flaky-test -- 2.40.1
[PATCH v6 10/10] drm: ci: Update xfails
Update msm-apq8016-fails, mediatek-mt8173-fails and virtio_gpu-none-fails to include the tests which fail. Signed-off-by: Vignesh Raman --- v2: - No changes v3: - No changes v4: - No changes v5: - Generate fails and flakes file with the updated xfails script - https://www.spinics.net/lists/kernel/msg4959630.html v6: - Generate fails file with drm-misc-next --- .../drm/ci/xfails/mediatek-mt8173-fails.txt | 13 -- .../gpu/drm/ci/xfails/msm-apq8016-fails.txt | 5 ++ .../drm/ci/xfails/virtio_gpu-none-fails.txt | 46 +++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/ci/xfails/mediatek-mt8173-fails.txt b/drivers/gpu/drm/ci/xfails/mediatek-mt8173-fails.txt index 671916067dba..ef0cb7c3698c 100644 --- a/drivers/gpu/drm/ci/xfails/mediatek-mt8173-fails.txt +++ b/drivers/gpu/drm/ci/xfails/mediatek-mt8173-fails.txt @@ -1,5 +1,4 @@ kms_3d,Fail -kms_addfb_basic@addfb25-bad-modifier,Fail kms_bw@linear-tiling-1-displays-1920x1080p,Fail kms_bw@linear-tiling-1-displays-2560x1440p,Fail kms_bw@linear-tiling-1-displays-3840x2160p,Fail @@ -9,13 +8,19 @@ kms_bw@linear-tiling-2-displays-3840x2160p,Fail kms_bw@linear-tiling-3-displays-1920x1080p,Fail kms_bw@linear-tiling-3-displays-2560x1440p,Fail kms_bw@linear-tiling-3-displays-3840x2160p,Fail +kms_color@invalid-gamma-lut-sizes,Fail kms_color@pipe-A-invalid-gamma-lut-sizes,Fail kms_color@pipe-B-invalid-gamma-lut-sizes,Fail -kms_force_connector_basic@force-connector-state,Fail +kms_cursor_legacy@cursor-vs-flip-atomic,Fail +kms_cursor_legacy@cursor-vs-flip-legacy,Fail +kms_flip@flip-vs-modeset-vs-hang,Fail +kms_flip@flip-vs-panning-vs-hang,Fail +kms_flip@flip-vs-suspend,Fail +kms_flip@flip-vs-suspend-interruptible,Fail kms_force_connector_basic@force-edid,Fail kms_force_connector_basic@force-load-detect,Fail kms_force_connector_basic@prune-stale-modes,Fail -kms_invalid_mode@int-max-clock,Fail +kms_hdmi_inject@inject-4k,Fail kms_plane_scaling@planes-upscale-20x20,Fail kms_plane_scaling@planes-upscale-20x20-downscale-factor-0-25,Fail kms_plane_scaling@planes-upscale-20x20-downscale-factor-0-5,Fail @@ -27,3 +32,5 @@ kms_properties@get_properties-sanity-atomic,Fail kms_properties@plane-properties-atomic,Fail kms_properties@plane-properties-legacy,Fail kms_rmfb@close-fd,Fail +kms_selftest@drm_format,Timeout +kms_selftest@drm_format_helper,Timeout diff --git a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt index 9981682feab2..d39d254c935e 100644 --- a/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt +++ b/drivers/gpu/drm/ci/xfails/msm-apq8016-fails.txt @@ -6,10 +6,15 @@ kms_cursor_legacy@all-pipes-single-bo,Fail kms_cursor_legacy@all-pipes-single-move,Fail kms_cursor_legacy@all-pipes-torture-bo,Fail kms_cursor_legacy@all-pipes-torture-move,Fail +kms_cursor_legacy@forked-bo,Fail +kms_cursor_legacy@forked-move,Fail kms_cursor_legacy@pipe-A-forked-bo,Fail kms_cursor_legacy@pipe-A-forked-move,Fail kms_cursor_legacy@pipe-A-single-bo,Fail kms_cursor_legacy@pipe-A-single-move,Fail kms_cursor_legacy@pipe-A-torture-bo,Fail kms_cursor_legacy@pipe-A-torture-move,Fail +kms_force_connector_basic@force-edid,Fail kms_hdmi_inject@inject-4k,Fail +kms_selftest@drm_format,Timeout +kms_selftest@drm_format_helper,Timeout diff --git a/drivers/gpu/drm/ci/xfails/virtio_gpu-none-fails.txt b/drivers/gpu/drm/ci/xfails/virtio_gpu-none-fails.txt index 9586b2339f6f..007f21e56d89 100644 --- a/drivers/gpu/drm/ci/xfails/virtio_gpu-none-fails.txt +++ b/drivers/gpu/drm/ci/xfails/virtio_gpu-none-fails.txt @@ -10,6 +10,49 @@ kms_bw@linear-tiling-1-displays-3840x2160p,Fail kms_bw@linear-tiling-2-displays-1920x1080p,Fail kms_bw@linear-tiling-2-displays-2560x1440p,Fail kms_bw@linear-tiling-2-displays-3840x2160p,Fail +kms_bw@linear-tiling-3-displays-1920x1080p,Fail +kms_bw@linear-tiling-3-displays-2560x1440p,Fail +kms_bw@linear-tiling-3-displays-3840x2160p,Fail +kms_bw@linear-tiling-4-displays-1920x1080p,Fail +kms_bw@linear-tiling-4-displays-2560x1440p,Fail +kms_bw@linear-tiling-4-displays-3840x2160p,Fail +kms_bw@linear-tiling-5-displays-1920x1080p,Fail +kms_bw@linear-tiling-5-displays-2560x1440p,Fail +kms_bw@linear-tiling-5-displays-3840x2160p,Fail +kms_bw@linear-tiling-6-displays-1920x1080p,Fail +kms_bw@linear-tiling-6-displays-2560x1440p,Fail +kms_bw@linear-tiling-6-displays-3840x2160p,Fail +kms_bw@linear-tiling-7-displays-1920x1080p,Fail +kms_bw@linear-tiling-7-displays-2560x1440p,Fail +kms_bw@linear-tiling-7-displays-3840x2160p,Fail +kms_bw@linear-tiling-8-displays-1920x1080p,Fail +kms_bw@linear-tiling-8-displays-2560x1440p,Fail +kms_bw@linear-tiling-8-displays-3840x2160p,Fail +kms_flip@absolute-wf_vblank,Fail +kms_flip@absolute-wf_vblank-interruptible,Fail +kms_flip@basic-flip-vs-wf_vblank,Fail +kms_flip@blocking-absolute-wf_vblank,Fail +kms_flip@blocking-absolute-wf_vblank-interruptible,Fail +kms_flip@blocking-wf_vblank,Fail +kms_flip@busy-flip,Fail +kms_flip@
[PATCH v6 08/10] drm: ci: uprev IGT
virtio-gpu kernel driver reports 16 for count_crtcs which exceeds IGT_MAX_PIPES set to 8 in igt-gpu-tools. This results in below memory corruption, malloc(): corrupted top size Received signal SIGABRT. Stack trace: #0 [fatal_sig_handler+0x17b] #1 [__sigaction+0x40] #2 [pthread_key_delete+0x14c] #3 [gsignal+0x12] #4 [abort+0xd3] #5 [__fsetlocking+0x290] #6 [timer_settime+0x37a] #7 [__default_morecore+0x1f1b] #8 [__libc_calloc+0x161] #9 [drmModeGetPlaneResources+0x44] #10 [igt_display_require+0x194] #11 [__igt_uniquereal_main1356+0x93c] #12 [main+0x3f] #13 [__libc_init_first+0x8a] #14 [__libc_start_main+0x85] #15 [_start+0x21] This is fixed in igt-gpu-tools by increasing IGT_MAX_PIPES to 16. https://patchwork.freedesktop.org/series/126327/ Uprev IGT to include the patches which fixes this issue. Signed-off-by: Vignesh Raman --- v6: - Added a new patch in the series to uprev IGT to fix memory corruption --- drivers/gpu/drm/ci/gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ci/gitlab-ci.yml b/drivers/gpu/drm/ci/gitlab-ci.yml index aeb9bab1b069..dac92cc2777c 100644 --- a/drivers/gpu/drm/ci/gitlab-ci.yml +++ b/drivers/gpu/drm/ci/gitlab-ci.yml @@ -5,7 +5,7 @@ variables: UPSTREAM_REPO: git://anongit.freedesktop.org/drm/drm TARGET_BRANCH: drm-next - IGT_VERSION: d1db7333d9c5fbbb05e50b0804123950d9dc1c46 + IGT_VERSION: d2af13d9f5be5ce23d996e4afd3e45990f5ab977 DEQP_RUNNER_GIT_URL: https://gitlab.freedesktop.org/anholt/deqp-runner.git DEQP_RUNNER_GIT_TAG: v0.15.0 -- 2.40.1
Re: [EXTERNAL] Re: [PATCH 1/2] drm/imagination: avoid -Wmissing-prototype warnings
On Wed, 2023-11-29 at 13:16 +0100, Arnd Bergmann wrote: > *** CAUTION: This email originates from a source not known to Imagination > Technologies. Think before you click a link or open an attachment *** > > On Wed, Nov 29, 2023, at 13:07, Donald Robson wrote: > > Hello Arnd, > > > > Thanks for this. However, I fixed these in a patch a few minutes > > before you sent > > yours. I'm not sure what normally happens in these circumstances, but > > as my patch has > > the kernel robot tags and a few additional fixes, I think we should > > probably use that > > one. > > Sure, that's fine. If you don't mind rebasing, just add a > "Reported-by: Arnd Bergmann " line as well. > > I tend to create a bug fix for any build regressions I see as > part of building randconfig kernels, but it's normal that the > mainainers have already fixed the same bug before I send my > patch, or that there is a better solution than what I come up > with. > > Arnd Will do! In fact Maxime just told me to resubmit as a series anyway. Donald
Re: [PATCH] drm/imagination: DRM_POWERVR should depend on ARCH_K3
Hi Maxime,
On Wed, Nov 29, 2023 at 12:34 PM Maxime Ripard wrote:
> On Wed, Nov 29, 2023 at 12:08:17PM +0100, Geert Uytterhoeven wrote:
> > On Wed, Nov 29, 2023 at 11:50 AM Maxime Ripard wrote:
> > > On Wed, Nov 29, 2023 at 11:10:51AM +0100, Geert Uytterhoeven wrote:
> > > > On Wed, Nov 29, 2023 at 10:23 AM Maxime Ripard
> > > > wrote:
> > > > > On Wed, Nov 29, 2023 at 09:58:12AM +0100, Geert Uytterhoeven wrote:
> > > > > > On Wed, Nov 29, 2023 at 9:35 AM Maxime Ripard
> > > > > > wrote:
> > > > > > > On Tue, Nov 28, 2023 at 08:16:18PM +0100, Geert Uytterhoeven
> > > > > > > wrote:
> > > > > > > > On Tue, Nov 28, 2023 at 8:03 PM Javier Martinez Canillas
> > > > > > > > wrote:
> > > > > > > > > Geert Uytterhoeven writes:
> > > > > > > > > > The Imagination Technologies PowerVR Series 6 GPU is
> > > > > > > > > > currently only
> > > > > > > > > > supported on Texas Instruments K3 AM62x SoCs. Hence add a
> > > > > > > > > > dependency on
> > > > > > > > > > ARCH_K3, to prevent asking the user about this driver when
> > > > > > > > > > configuring a
> > > > > > > > > > kernel without Texas Instruments K3 Multicore SoC support.
> > > > > > > > > >
> > > > > > > > > > Fixes: 4babef0708656c54 ("drm/imagination: Add skeleton
> > > > > > > > > > PowerVR driver")
> > > > > > > > > > Signed-off-by: Geert Uytterhoeven
> > > > > >
> > > > > > > > > In any case, I agree with you that restricting to only K3
> > > > > > > > > makes sense.
> > > > > > > >
> > > > > > > > I am looking forward to adding || SOC_AM33XX || ARCH_RENESAS ||
> > > > > > > > ...,
> > > > > > > > eventually ;-)
> > > > > > >
> > > > > > > I disagree. This is to handle a generic IP, just like panfrost,
> > > > > > > lima, or
> > > > > > > etnaviv, and we certaintly don't want to maintain the Kconfig
> > > > > > > list of
> > > > > > > every possible architecture and SoC family it might or might not
> > > > > > > be
> > > > > > > found.
> > > > > >
> > > > > > While PowerVR is a generic IP, I believe it needs a non-generic
> > > > > > firmware, which is currently only available for AM62x SoCs.
> > >
> > > I just asked, it's not true in most cases. There's some exceptions
> > > (GX6250 for example) that could require different firmwares depending on
> > > the SoC it's used in, but it's not the case here.
> >
> > OK, please tell me how to use it on e.g. R-Car Gen3.
>
> I'm not very familiar with the R-Car family of SoCs.
>
> However, if I'm to trust that page:
> https://www.renesas.com/us/en/products/automotive-products/automotive-system-chips-socs
>
> None of them feature the same GPU than the AM62x, so that question is
> completely different?
According to the documentation I have, they contain PowerVR Series6XT
or Series7XE GPUs.
DRM_POWERVR claims to support "Imagination Technologies PowerVR
(Series 6 or later) or IMG GPU".
> > > > > I'm not sure it's actually true, but let's consider it is. Then what?
> > > > > If
> > > > > the firmware isn't there and/or the DT bits too, then nothing will
> > > > > happen. We would have wasted a couple of 100kB on a system that is
> > > > > taking somewhere in the 100MB-10GB range, and that's pretty much it.
> > > >
> > > > I am talking about posing the question to the user to enable the driver
> > > > or not. Which applies to everyone who configures a kernel.
> > >
> > > If that user doesn't use a defconfig, doesn't use any variant of
> > > *defconfig make target. Plus, the driver still isn't enabled by default.
> > >
> > > > > If you have we take that patch in though, we have:
> > > > >
> > > > > - To keep merging patches as firmwares become available.
> > > >
> > > > You need to keep merging patches to update DT bindings, DTS,
> > > > SoC-specific drivers, the DRM driver itself, ... too.
> > >
> > > The DT binding and DRM driver is already there, the SoC specific drivers
> >
> > The DT binding only lists "ti,am62-gpu" with "img,img-axe" as a fallback.
>
> Sure. And the driver matches on img,img-axe, so it would probe fine even
> with a different first compatible.
>
> > > are probably already there by the time you reach GPU enablement, and the
> > > DT doesn't have to be upstream.
> >
> > And getting it upstream requires updating the bindings...
>
> Right. And you still don't have to put it upstream, so the binding isn't
> a requirement either.
Oh, how we love out-of-tree...
> > > > > - If we update linux-firmware only, then the driver is still not
> > > > > loading even though it could.
> > > > >
> > > > > - If we have gotten our firmware through some other mean, then the
> > > > > driver is still not loading even though it could.
> > > >
> > > > You will still need to update parts of the kernel, too.
> > >
> > > Not really, no. We can use the AM62x as an example. The only thing that
> > > was required to enable the driver (excluding the driver itself of
> > > course) was a single DT patch, without anything you've mentioned so far.
> >
> > Who added:
> >
> > Documentati
Re: [PATCH v2 0/2] drm/bridge: panel: Check device dependency before managing device link
Hi Linus, On Mon, Nov 27, 2023 at 11:13:31PM +0100, Linus Walleij wrote: > On Mon, Nov 27, 2023 at 5:29 PM Maxime Ripard wrote: > > On Mon, Nov 27, 2023 at 05:03:53PM +0100, Linus Walleij wrote: > > > > > Liu Ying (2): > > > > driver core: Export device_is_dependent() to modules > > > > drm/bridge: panel: Check device dependency before managing device link > > > > > > I just applied patch 1 directly to the drm-misc-fixes so we don't have to > > > revert and then re-apply patches, because that is a bigger evil. (We can't > > > rebase these branches...) > > > > Erm, you did wait for GKH or Rafael's ACK to do that, right? > > No. > > It is a bigger evil to leave the tree broken than to enforce formal process, > and it is pretty self-evident. If any of them get annoyed about it we can > revert the patch, or both. Yeah, I definitely understand why you did it, but I don't think it's something we would encourage in drm-misc. We've discussed it with Sima yesterday, and I think we would even need the extra check in dim to make sure that a committer shouldn't do that without dim complaining. Sima played a bit with it, and it should be doable to get something fairly reliable if you use get_maintainers.pl to retrieve the git tree (through scripts/get_maintainer.pl --no-email --no-l --scm) and figuring out if only drm.git, drm-intel.git or drm-misc.git is involved. If it isn't, then we should check that there's the ack of one of the maintainers. Could you write a patch to do so? Thanks! Maxime signature.asc Description: PGP signature
Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap
Hi, Thanks for writing this down On Thu, Nov 16, 2023 at 03:53:20PM +, Simon Ser wrote: > On Thursday, November 9th, 2023 at 08:45, Simon Ser > wrote: > > > User-space sometimes needs to allocate scanout-capable memory for > > GPU rendering purposes. On a vc4/v3d split render/display SoC, this > > is achieved via DRM dumb buffers: the v3d user-space driver opens > > the primary vc4 node, allocates a DRM dumb buffer there, exports it > > as a DMA-BUF, imports it into the v3d render node, and renders to it. > > > > However, DRM dumb buffers are only meant for CPU rendering, they are > > not intended to be used for GPU rendering. Primary nodes should only > > be used for mode-setting purposes, other programs should not attempt > > to open it. Moreover, opening the primary node is already broken on > > some setups: systemd grants permission to open primary nodes to > > physically logged in users, but this breaks when the user is not > > physically logged in (e.g. headless setup) and when the distribution > > is using a different init (e.g. Alpine Linux uses openrc). > > > > We need an alternate way for v3d to allocate scanout-capable memory. > > Leverage DMA heaps for this purpose: expose a CMA heap to user-space. > > So we've discussed about this patch on IRC [1] [2]. Some random notes: > > - We shouldn't create per-DRM-device heaps in general. Instead, we should try > using centralized heaps like the existing system and cma ones. That way > other > drivers (video, render, etc) can also link to these heaps without depending > on the display driver. > - We can't generically link to heaps in core DRM, however we probably provide > a default for shmem and cma helpers. > - We're missing a bunch of heaps, e.g. sometimes there are multiple cma areas > but only a single cma heap is created right now. > - Some hw needs the memory to be in a specific region for scanout (e.g. lower > 256MB of RAM for Allwinner). We could create one heap per such region (but > is > it fine to have overlapping heaps?). Just for reference, it's not the scanout itself that has that requirement on Allwinner SoCs, it's the HW codec. But if you want to display the decoded frame directly using dma-buf, you'll still need to either allocate a scanout buffer and hope it'll be in the lower 256MB, or allocate a buffer from the codec in the lower 256MB and then hope it's scanout-capable (which it is, so that's we do, but there's no guarantee about it). I think the logicvc is a much better example for this, since it requires framebuffers to be in a specific area, with each plane having a dedicated area. AFAIK that's the most extreme example we have upstream. Maxime signature.asc Description: PGP signature
Re: [PATCH 10/10] ACPI: IORT: Allow COMPILE_TEST of IORT
On Tue, Nov 28, 2023 at 08:48:06PM -0400, Jason Gunthorpe wrote: > The arm-smmu driver can COMPILE_TEST on x86, so expand this to also > enable the IORT code so it can be COMPILE_TEST'd too. > > Signed-off-by: Jason Gunthorpe > --- > drivers/acpi/Kconfig| 2 -- > drivers/acpi/Makefile | 2 +- > drivers/acpi/arm64/Kconfig | 1 + > drivers/acpi/arm64/Makefile | 2 +- > drivers/iommu/Kconfig | 1 + > 5 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > index f819e760ff195a..3b7f77b227d13a 100644 > --- a/drivers/acpi/Kconfig > +++ b/drivers/acpi/Kconfig > @@ -541,9 +541,7 @@ config ACPI_PFRUT > To compile the drivers as modules, choose M here: > the modules will be called pfr_update and pfr_telemetry. > > -if ARM64 > source "drivers/acpi/arm64/Kconfig" > -endif > > config ACPI_PPTT > bool > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index eaa09bf52f1760..4e77ae37b80726 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -127,7 +127,7 @@ obj-y += pmic/ > video-objs += acpi_video.o video_detect.o > obj-y+= dptf/ > > -obj-$(CONFIG_ARM64) += arm64/ > +obj-y+= arm64/ > > obj-$(CONFIG_ACPI_VIOT) += viot.o > > diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig > index b3ed6212244c1e..537d49d8ace69e 100644 > --- a/drivers/acpi/arm64/Kconfig > +++ b/drivers/acpi/arm64/Kconfig > @@ -11,6 +11,7 @@ config ACPI_GTDT > > config ACPI_AGDI > bool "Arm Generic Diagnostic Dump and Reset Device Interface" > + depends on ARM64 > depends on ARM_SDE_INTERFACE > help > Arm Generic Diagnostic Dump and Reset Device Interface (AGDI) is > diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile > index 143debc1ba4a9d..71d0e635599390 100644 > --- a/drivers/acpi/arm64/Makefile > +++ b/drivers/acpi/arm64/Makefile > @@ -4,4 +4,4 @@ obj-$(CONFIG_ACPI_IORT) += iort.o > obj-$(CONFIG_ACPI_GTDT) += gtdt.o > obj-$(CONFIG_ACPI_APMT) += apmt.o > obj-$(CONFIG_ARM_AMBA) += amba.o > -obj-y+= dma.o init.o > +obj-$(CONFIG_ARM64) += dma.o init.o > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 7673bb82945b6c..309378e76a9bc9 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -318,6 +318,7 @@ config ARM_SMMU > select IOMMU_API > select IOMMU_IO_PGTABLE_LPAE > select ARM_DMA_USE_IOMMU if ARM > + select ACPI_IORT if ACPI > help > Support for implementations of the ARM System MMU architecture > versions 1 and 2. > -- I don't think it should be done this way. Is the goal compile testing IORT code ? If so, why are we forcing it through the SMMU (only because it can be compile tested while eg SMMUv3 driver can't ?) menu entry ? This looks a bit artificial (and it is unclear from the Kconfig file why only that driver selects IORT, it looks like eg the SMMUv3 does not have the same dependency - there is also the SMMUv3 perf driver to consider). Maybe we can move IORT code into drivers/acpi and add a silent config option there with a dependency on ARM64 || COMPILE_TEST. Don't know but at least it is clearer. As for the benefits of compile testing IORT code - yes the previous patch is a warning to fix but I am not so sure about the actual benefits. Thanks, Lorenzo
Re: [PATCH v2 11/12] drm/rockchip: vop2: Add debugfs support
On Wed, Nov 29, 2023 at 07:01:37PM +0800, Andy Yan wrote:
> Hi Sascha:
>
>
>
> On 11/29/23 16:52, Sascha Hauer wrote:
> > On Mon, Nov 27, 2023 at 06:56:34PM +0800, Andy Yan wrote:
> > > Hi Sascha:
> > >
> > > thanks for you review.
> > >
> > > On 11/27/23 18:13, Sascha Hauer wrote:
> > >
> > > On Wed, Nov 22, 2023 at 08:56:01PM +0800, Andy Yan wrote:
> > >
> > > From: Andy Yan [1]
> > >
> > > /sys/kernel/debug/dri/vop2/summary: dump vop display state
> > > /sys/kernel/debug/dri/vop2/regs: dump whole vop registers
> > > /sys/kernel/debug/dri/vop2/active_regs: only dump the registers of
> > > activated modules
> > >
> > > Signed-off-by: Andy Yan [2]
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 399 +++
> > >1 file changed, 399 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > index 9eecbe1f71f9..4a2342209c15 100644
> > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > @@ -27,6 +27,7 @@
> > >#include
> > >#include
> > >#include
> > > +#include
> > >#include
> > >#include
> > >
> > > @@ -187,6 +188,7 @@ struct vop2 {
> > >*/
> > > u32 registered_num_wins;
> > >
> > > + struct resource *res;
> > > void __iomem *regs;
> > > struct regmap *map;
> > >
> > > @@ -228,6 +230,44 @@ struct vop2 {
> > >#define vop2_output_if_is_lvds(x) (x == ROCKCHIP_VOP2_EP_LVDS0 ||
> > > x == ROCKCHIP_VOP2_EP_LVDS1)
> > >#define vop2_output_if_is_dpi(x) (x == ROCKCHIP_VOP2_EP_RGB0)
> > >
> > > +struct vop2_regs_dump {
> > > + const char *name;
> > > + u32 base;
> > > + u32 en_reg;
> > > + u32 en_val;
> > > + u32 en_mask;
> > > +};
> > > +
> > > +/*
> > > + * bus-format types.
> > > + */
> > > +struct drm_bus_format_enum_list {
> > > + int type;
> > > + const char *name;
> > > +};
> > > +
> > > +static const struct drm_bus_format_enum_list
> > > drm_bus_format_enum_list[] = {
> > > + { DRM_MODE_CONNECTOR_Unknown, "Unknown" },
> > > + { MEDIA_BUS_FMT_RGB565_1X16, "RGB565_1X16" },
> > > + { MEDIA_BUS_FMT_RGB666_1X18, "RGB666_1X18" },
> > > + { MEDIA_BUS_FMT_RGB666_1X24_CPADHI, "RGB666_1X24_CPADHI" },
> > > + { MEDIA_BUS_FMT_RGB666_1X7X3_SPWG, "RGB666_1X7X3_SPWG" },
> > > + { MEDIA_BUS_FMT_YUV8_1X24, "YUV8_1X24" },
> > > + { MEDIA_BUS_FMT_UYYVYY8_0_5X24, "UYYVYY8_0_5X24" },
> > > + { MEDIA_BUS_FMT_YUV10_1X30, "YUV10_1X30" },
> > > + { MEDIA_BUS_FMT_UYYVYY10_0_5X30, "UYYVYY10_0_5X30" },
> > > + { MEDIA_BUS_FMT_RGB888_3X8, "RGB888_3X8" },
> > > + { MEDIA_BUS_FMT_RGB888_1X24, "RGB888_1X24" },
> > > + { MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, "RGB888_1X7X4_SPWG" },
> > > + { MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, "RGB888_1X7X4_JEIDA" },
> > > + { MEDIA_BUS_FMT_UYVY8_2X8, "UYVY8_2X8" },
> > > + { MEDIA_BUS_FMT_YUYV8_1X16, "YUYV8_1X16" },
> > > + { MEDIA_BUS_FMT_UYVY8_1X16, "UYVY8_1X16" },
> > > + { MEDIA_BUS_FMT_RGB101010_1X30, "RGB101010_1X30" },
> > > + { MEDIA_BUS_FMT_YUYV10_1X20, "YUYV10_1X20" },
> > > +};
> > > +static DRM_ENUM_NAME_FN(drm_get_bus_format_name,
> > > drm_bus_format_enum_list)
> > > +
> > >static const struct regmap_config vop2_regmap_config;
> > >
> > >static struct vop2_video_port *to_vop2_video_port(struct drm_crtc
> > > *crtc)
> > > @@ -2487,6 +2527,363 @@ static const struct drm_crtc_helper_funcs
> > > vop2_crtc_helper_funcs = {
> > > .atomic_disable = vop2_crtc_atomic_disable,
> > >};
> > >
> > > +static void vop2_dump_connector_on_crtc(struct drm_crtc *crtc, struct
> > > seq_file *s)
> > > +{
> > > + struct drm_connector_list_iter conn_iter;
> > > + struct drm_connector *connector;
> > > +
> > > + drm_connector_list_iter_begin(crtc->dev, &conn_iter);
> > > + drm_for_each_connector_iter(connector, &conn_iter) {
> > > + if (crtc->state->connector_mask &
> > > drm_connector_mask(connector))
> > > + seq_printf(s, "Connector: %s\n",
> > > connector->name);
> > > +
> > > + }
> > > + drm_connector_list_iter_end(&conn_iter);
> > > +}
> > > +
> > > +static int vop2_plane_state_dump(struct seq_file *s, struct drm_plane
> > > *plane)
> > > +{
> > > + struct vop2_win *win = to_vop2_win(plane);
> > > + struct drm_plane_state *pstate = plane->state;
> > > + struct drm_rect *src, *dst;
> > > + struct drm_framebuffer *fb;
> > > + struct drm_gem_object *obj;
> > > + struct rockchip_gem_object *rk_obj;
> > > + bool xmirror;
> > > +
Re: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions
On Wed, Nov 29, 2023 at 08:53:30AM +0100, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 01:05:14 +0300
> Dmitry Osipenko wrote:
>
> > On 11/28/23 15:37, Boris Brezillon wrote:
> > > On Tue, 28 Nov 2023 12:14:42 +0100
> > > Maxime Ripard wrote:
> > >
> > >> Hi,
> > >>
> > >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
> > >>> On Fri, 24 Nov 2023 11:40:06 +0100
> > >>> Maxime Ripard wrote:
> > >>>
> > On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> > > Add locked and remove unlocked postfixes from drm-shmem function
> > > names,
> > > making names consistent with the drm/gem core code.
> > >
> > > Reviewed-by: Boris Brezillon
> > > Suggested-by: Boris Brezillon
> > > Signed-off-by: Dmitry Osipenko
> >
> > This contradicts my earlier ack on a patch but...
> >
> > > ---
> > > drivers/gpu/drm/drm_gem_shmem_helper.c| 64
> > > +--
> > > drivers/gpu/drm/lima/lima_gem.c | 8 +--
> > > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> > > drivers/gpu/drm/panfrost/panfrost_gem.c | 6 +-
> > > .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 +-
> > > drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
> > > drivers/gpu/drm/v3d/v3d_bo.c | 4 +-
> > > drivers/gpu/drm/virtio/virtgpu_object.c | 4 +-
> > > include/drm/drm_gem_shmem_helper.h| 36 +--
> > > 9 files changed, 64 insertions(+), 64 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > index 0d61f2b3e213..154585ddae08 100644
> > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs
> > > drm_gem_shmem_funcs = {
> > > .pin = drm_gem_shmem_object_pin,
> > > .unpin = drm_gem_shmem_object_unpin,
> > > .get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > - .vmap = drm_gem_shmem_object_vmap,
> > > - .vunmap = drm_gem_shmem_object_vunmap,
> > > + .vmap = drm_gem_shmem_object_vmap_locked,
> > > + .vunmap = drm_gem_shmem_object_vunmap_locked,
> >
> > While I think we should indeed be consistent with the names, I would
> > also expect helpers to get the locking right by default.
> > >>>
> > >>> Wait, actually I think this patch does what you suggest already. The
> > >>> _locked() prefix tells the caller: "you should take care of the locking,
> > >>> I expect the lock to be held when this is hook/function is called". So
> > >>> helpers without the _locked() prefix take care of the locking (which I
> > >>> guess matches your 'helpers get the locking right' expectation), and
> > >>> those with the _locked() prefix don't.
> > >>
> > >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> > >> sorry. What I'm trying to say I guess is that, in this particular case,
> > >> I don't think you can expect the vmap implementation to be called with
> > >> or without the locks held. The doc for that function will say that it's
> > >> either one or the other, but not both.
> > >>
> > >> So helpers should follow what is needed to provide a default vmap/vunmap
> > >> implementation, including what locking is expected from a vmap/vunmap
> > >> implementation.
> > >
> > > Hm, yeah, I think that's a matter of taste. When locking is often
> > > deferrable, like it is in DRM, I find it beneficial for funcions and
> > > function pointers to reflect the locking scheme, rather than relying on
> > > people properly reading the doc, especially when this is the only
> > > outlier in the group of drm_gem_object_funcs we already have, and it's
> > > not event documented at the drm_gem_object_funcs level [1] :P.
> > >
> > >>
> > >> If that means that vmap is always called with the locks taken, then
> > >> drm_gem_shmem_object_vmap can just assume that it will be called with
> > >> the locks taken and there's no need to mention it in the name (and you
> > >> can probably sprinkle a couple of lockdep assertion to make sure the
> > >> locking is indeed consistent).
> > >
> > > Things get very confusing when you end up having drm_gem_shmem helpers
> > > that are suffixed with _locked() to encode the fact locking is the
> > > caller's responsibility and no suffix for the
> > > callee-takes-care-of-the-locking semantics, while other helpers that are
> > > not suffixed at all actually implement the
> > > caller-should-take-care-of-the-locking semantics.
> > >
> > >>
> > I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > convert whatever function needs to be converted to the unlock suffix
Re: [PATCH v6 06/10] drm: ci: mediatek: Set IGT_FORCE_DRIVER for mt8173
Hi Vignesh, On Wed, 29 Nov 2023 at 12:19, Vignesh Raman wrote: > Expected driver for mt8173 is "mediatek" and for mt8183 > it is "panfrost". Set IGT_FORCE_DRIVER to 'mediatek' as > the expected driver for mt8173. Actually, for mt8183 it's both. And for mt8173 it will probably be mediatek+pvr pretty soon. Each of these SoCs (like most Arm devices) have a separate display controller and GPU, with different drivers for each. They'll run different tests with different xfails. So we should figure out a way to support igt running for both devices on the one system. Cheers, Daniel
Re: [PATCH v4 05/45] drm/connector: Check drm_connector_init pointers arguments
On Wed, Nov 29, 2023 at 01:40:38PM +0200, Jani Nikula wrote: > On Wed, 29 Nov 2023, Maxime Ripard wrote: > > On Wed, Nov 29, 2023 at 11:38:42AM +0200, Jani Nikula wrote: > >> On Wed, 29 Nov 2023, Maxime Ripard wrote: > >> > Hi Ville, > >> > > >> > On Tue, Nov 28, 2023 at 03:49:08PM +0200, Ville Syrjälä wrote: > >> >> On Tue, Nov 28, 2023 at 02:29:40PM +0100, Maxime Ripard wrote: > >> >> > On Tue, Nov 28, 2023 at 02:54:02PM +0200, Jani Nikula wrote: > >> >> > > On Tue, 28 Nov 2023, Maxime Ripard wrote: > >> >> > > > All the drm_connector_init variants take at least a pointer to the > >> >> > > > device, connector and hooks implementation. > >> >> > > > > >> >> > > > However, none of them check their value before dereferencing those > >> >> > > > pointers which can lead to a NULL-pointer dereference if the > >> >> > > > author > >> >> > > > isn't careful. > >> >> > > > >> >> > > Arguably oopsing on the spot is preferrable when this can't be > >> >> > > caused by > >> >> > > user input. It's always a mistake that should be caught early during > >> >> > > development. > >> >> > > > >> >> > > Not everyone checks the return value of drm_connector_init and > >> >> > > friends, > >> >> > > so those cases will lead to more mysterious bugs later. And probably > >> >> > > oopses as well. > >> >> > > >> >> > So maybe we can do both then, with something like > >> >> > > >> >> > if (WARN_ON(!dev)) > >> >> >return -EINVAL > >> >> > > >> >> > if (drm_WARN_ON(dev, !connector || !funcs)) > >> >> >return -EINVAL; > >> >> > > >> >> > I'd still like to check for this, so we can have proper testing, and > >> >> > we > >> >> > already check for those pointers in some places (like funcs in > >> >> > drm_connector_init), so if we don't cover everything we're > >> >> > inconsistent. > >> >> > >> >> People will invariably cargo-cult this kind of stuff absolutely > >> >> everywhere and then all your functions will have tons of dead > >> >> code to check their arguments. > >> > > >> > And that's a bad thing because... ? > >> > > >> > Also, are you really saying that checking that your arguments make sense > >> > is cargo-cult? > >> > >> It's a powerful thing to be able to assume a NULL argument is always a > >> fatal programming error on the caller's side, and should oops and get > >> caught immediately. It's an assertion. > > > > Yeah, but we're not really doing that either. We have no explicit > > assertion anywhere. We take a pointer in, and just hope that it will be > > dereferenced later on and that the kernel will crash. The pointer to the > > functions especially is only deferenced very later on. > > > > And assertions might be powerful, but being able to notice errors and > > debug them is too. A panic takes away basically any remote access to > > debug. If you don't have a console, you're done. > > > >> We're not talking about user input or anything like that here. > >> > >> If you start checking for things that can't happen, and return errors > >> for them, you start gracefully handling things that don't have anything > >> graceful about them. > > > > But there's nothing graceful to do here: you just return from your probe > > function that you couldn't probe and that's it. Just like you do when > > you can't map your registers, or get your interrupt, or register into > > any framework (including drm_dev_register that pretty much every driver > > handles properly if it returns an error, without being graceful about > > it). > > Those are all dynamic things that can fail. > > Quite different from passing NULL dev, connector, or funcs to > drm_connector_init() and friends. > > I think it's wrong to set the example that everything needs to be > checked, everything needs to return an error, every call needs to check > for error return, all the time, everywhere. People absolutely will cargo > cult that, and that's what Ville is referring to. > > If you pass NULL dev, connector, or funcs to drm_connector_init() I > think you absolutely deserve to get an oops. > > For dev, you could possibly not have reached the function with NULL > dev. (And __drm_connector_init() has dev->mode_config before the check, > so you'll get a static analyzer warning about dereference before the > check.) If you have NULL connector, you didn't check for allocation > failure earlier. If you have NULL funcs, you just passed NULL, because > it's generally supposed to be a pointer to a static const struct. > > >> Having such checks in place trains people to think they *may* happen. > > > > In most cases, kmalloc can't fail. We seem to have a very different > > policy towards it. > > Again, dynamic in nature and can fail. > > >> While it should fail fast and loud at the developer's first smoke test, > >> and get fixed then and there. > > > > Returning an error + a warning also qualifies for "fail fast and loud". > > But keeps the system alive for someone to notice in any case. > > But where do you draw the line? This also ap
Re: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions
On Wed, 29 Nov 2023 14:09:47 +0100
Maxime Ripard wrote:
> On Wed, Nov 29, 2023 at 08:53:30AM +0100, Boris Brezillon wrote:
> > On Wed, 29 Nov 2023 01:05:14 +0300
> > Dmitry Osipenko wrote:
> >
> > > On 11/28/23 15:37, Boris Brezillon wrote:
> > > > On Tue, 28 Nov 2023 12:14:42 +0100
> > > > Maxime Ripard wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
> > > >>> On Fri, 24 Nov 2023 11:40:06 +0100
> > > >>> Maxime Ripard wrote:
> > > >>>
> > > On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> > >
> > > > Add locked and remove unlocked postfixes from drm-shmem function
> > > > names,
> > > > making names consistent with the drm/gem core code.
> > > >
> > > > Reviewed-by: Boris Brezillon
> > > > Suggested-by: Boris Brezillon
> > > > Signed-off-by: Dmitry Osipenko
> > > >
> > >
> > > This contradicts my earlier ack on a patch but...
> > >
> > > > ---
> > > > drivers/gpu/drm/drm_gem_shmem_helper.c| 64
> > > > +--
> > > > drivers/gpu/drm/lima/lima_gem.c | 8 +--
> > > > drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> > > > drivers/gpu/drm/panfrost/panfrost_gem.c | 6 +-
> > > > .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 2 +-
> > > > drivers/gpu/drm/panfrost/panfrost_mmu.c | 2 +-
> > > > drivers/gpu/drm/v3d/v3d_bo.c | 4 +-
> > > > drivers/gpu/drm/virtio/virtgpu_object.c | 4 +-
> > > > include/drm/drm_gem_shmem_helper.h| 36 +--
> > > > 9 files changed, 64 insertions(+), 64 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > index 0d61f2b3e213..154585ddae08 100644
> > > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs
> > > > drm_gem_shmem_funcs = {
> > > > .pin = drm_gem_shmem_object_pin,
> > > > .unpin = drm_gem_shmem_object_unpin,
> > > > .get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > > - .vmap = drm_gem_shmem_object_vmap,
> > > > - .vunmap = drm_gem_shmem_object_vunmap,
> > > > + .vmap = drm_gem_shmem_object_vmap_locked,
> > > > + .vunmap = drm_gem_shmem_object_vunmap_locked,
> > >
> > > While I think we should indeed be consistent with the names, I would
> > > also expect helpers to get the locking right by default.
> > > >>>
> > > >>> Wait, actually I think this patch does what you suggest already. The
> > > >>> _locked() prefix tells the caller: "you should take care of the
> > > >>> locking,
> > > >>> I expect the lock to be held when this is hook/function is called". So
> > > >>> helpers without the _locked() prefix take care of the locking (which I
> > > >>> guess matches your 'helpers get the locking right' expectation), and
> > > >>> those with the _locked() prefix don't.
> > > >>
> > > >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> > > >> sorry. What I'm trying to say I guess is that, in this particular case,
> > > >> I don't think you can expect the vmap implementation to be called with
> > > >> or without the locks held. The doc for that function will say that it's
> > > >> either one or the other, but not both.
> > > >>
> > > >> So helpers should follow what is needed to provide a default
> > > >> vmap/vunmap
> > > >> implementation, including what locking is expected from a vmap/vunmap
> > > >> implementation.
> > > >
> > > > Hm, yeah, I think that's a matter of taste. When locking is often
> > > > deferrable, like it is in DRM, I find it beneficial for funcions and
> > > > function pointers to reflect the locking scheme, rather than relying on
> > > > people properly reading the doc, especially when this is the only
> > > > outlier in the group of drm_gem_object_funcs we already have, and it's
> > > > not event documented at the drm_gem_object_funcs level [1] :P.
> > > >
> > > >>
> > > >> If that means that vmap is always called with the locks taken, then
> > > >> drm_gem_shmem_object_vmap can just assume that it will be called with
> > > >> the locks taken and there's no need to mention it in the name (and you
> > > >> can probably sprinkle a couple of lockdep assertion to make sure the
> > > >> locking is indeed consistent).
> > > >
> > > > Things get very confusing when you end up having drm_gem_shmem helpers
> > > > that are suffixed with _locked() to encode the fact locking is the
> > > > caller's responsibility and no suffix for the
> > > > callee-takes-care-of-the-locking semantics, while other helpers that are
> > > > not suffixed at all actually implement the
> > > >
Re: [PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state
Thomas Zimmermann writes:
> Hi
>
> Am 27.11.23 um 17:25 schrieb Alyssa Ross:
>> Thomas Zimmermann writes:
>>
>>> Invoke drm_plane_helper_funcs.end_fb_access before
>>> drm_atomic_helper_commit_hw_done(). The latter function hands over
>>> ownership of the plane state to the following commit, which might
>>> free it. Releasing resources in end_fb_access then operates on undefined
>>> state. This bug has been observed with non-blocking commits when they
>>> are being queued up quickly.
>>>
>>> Here is an example stack trace from the bug report. The plane state has
>>> been free'd already, so the pages for drm_gem_fb_vunmap() are gone.
>>>
>>> Unable to handle kernel paging request at virtual address 00010049
>>> [...]
>>> drm_gem_fb_vunmap+0x18/0x74
>>> drm_gem_end_shadow_fb_access+0x1c/0x2c
>>> drm_atomic_helper_cleanup_planes+0x58/0xd8
>>> drm_atomic_helper_commit_tail+0x90/0xa0
>>> commit_tail+0x15c/0x188
>>> commit_work+0x14/0x20
>>>
>>> For aborted commits, it is still ok to run end_fb_access as part of the
>>> plane's cleanup. Add a test to drm_atomic_helper_cleanup_planes().
>>>
>>> v2:
>>> * fix test in drm_atomic_helper_cleanup_planes()
>>>
>>> Reported-by: Alyssa Ross
>>> Closes: https://lore.kernel.org/dri-devel/87leazm0ya@alyssa.is/
>>> Suggested-by: Daniel Vetter
>>> Fixes: 94d879eaf7fb ("drm/atomic-helper: Add {begin,end}_fb_access to plane
>>> helpers")
>>> Signed-off-by: Thomas Zimmermann
>>> Cc: # v6.2+
>>> ---
>>> drivers/gpu/drm/drm_atomic_helper.c | 17 +
>>> 1 file changed, 17 insertions(+)
>>
>> Got this basically immediately. :(
>
> I've never seen such problems on other systems. Is there anything
> different about the Mac systems? How do you trigger these errors?
My understanding is that all sorts of things are different, but I don't
know too much about the details. There's of course a chance that there
could be some other change in the Asahi Linux kernel that causes this
problem to surface — as I said, I reviewed the diff with mainline and
didn't see anything that looked relevant, but I could well have missed
something. I don't think I can test mainline directly, as it doesn't
yet support enough of the hardware — for slightly older Apple Silicon
Mac models, I think enough is upstream that this would be possible, but
I don't have access to any.
I started off encountering these errors every few days. I noticed them
because they would sometimes result in my system either starting to
freeze for 10 seconds at a time, or until I switched VT. They seem to
correlate with the system being under high CPU load. I was also able to
substantially increase the frequency with which they occurred by adding
logging to the kernel — even just drm.debug=0x10 makes a big difference,
and when I also added a few dump_backtrace() calls when I was trying to
understand the code and diagnose the problem, I would relatively
consistently encounter an Oops within a few minutes of load.
BTW: v3 is looking good so far. I've only been testing it since this
morning, though, so I'll keep trying it out for a bit longer before I
declare the problem to have been solved and send a Tested-by.
signature.asc
Description: PGP signature
Re: Radeon regression in 6.6 kernel
On Tue, Nov 28, 2023 at 11:45 PM Luben Tuikov wrote: > > On 2023-11-28 17:13, Alex Deucher wrote: > > On Mon, Nov 27, 2023 at 6:24 PM Phillip Susi wrote: > >> > >> Alex Deucher writes: > >> > In that case those are the already known problems with the scheduler > changes, aren't they? > >>> > >>> Yes. Those changes went into 6.7 though, not 6.6 AFAIK. Maybe I'm > >>> misunderstanding what the original report was actually testing. If it > >>> was 6.7, then try reverting: > >>> 56e449603f0ac580700621a356d35d5716a62ce5 > >>> b70438004a14f4d0f9890b3297cd66248728546c > >> > >> At some point it was suggested that I file a gitlab issue, but I took > >> this to mean it was already known and being worked on. -rc3 came out > >> today and still has the problem. Is there a known issue I could track? > >> > > > > At this point, unless there are any objections, I think we should just > > revert the two patches > Uhm, no. > > Why "the two" patches? > > This email, part of this thread, > > https://lore.kernel.org/all/87r0kircdo@vps.thesusis.net/ > > clearly states that reverting *only* this commit, > 56e449603f0ac5 drm/sched: Convert the GPU scheduler to variable number of > run-queues > *does not* mitigate the failed suspend. (Furthermore, this commit doesn't > really change > anything operational, other than using an allocated array, instead of a > static one, in DRM, > while the 2nd patch is solely contained within the amdgpu driver code.) > > Leaving us with only this change, > b70438004a14f4 drm/amdgpu: move buffer funcs setting up a level > to be at fault, as the kernel log attached in the linked email above shows. > > The conclusion is that only b70438004a14f4 needs reverting. b70438004a14f4 was a fix for 56e449603f0ac5. Without b70438004a14f4, 56e449603f0ac5 breaks amdgpu. Alex
About resuming in the remove callback
Hello Laurent,
On Wed, Nov 29, 2023 at 12:11:50PM +0200, Laurent Pinchart wrote:
> On Wed, Nov 29, 2023 at 10:51:37AM +0100, Uwe Kleine-König wrote:
> > On Wed, Nov 29, 2023 at 02:39:55AM +0200, Laurent Pinchart wrote:
> > > On Thu, Nov 23, 2023 at 06:54:27PM +0100, Uwe Kleine-König wrote:
> > > > pm_runtime_resume_and_get() already drops the runtime PM usage counter
> > > > in the error case. So a call to pm_runtime_put_sync() can be dropped.
> > > >
> > > > Signed-off-by: Uwe Kleine-König
> > >
> > > I wonder if checkpatch should warn about usage of pm_runtime_get_sync().
> >
> > It should not warn in general. There are cases where
> > pm_runtime_get_sync() is the right function to use. See for example
>
> Sure, the function most likely has some valid use cases (otherwise it
> should just be removed), but I think those are are a minority. I was
> just thinking out loud anyway.
>
> > commit aec488051633 ("crypto: stm32 - Properly handle pm_runtime_get
> > failing").
>
> I don't know much about that device, but wouldn't the best option be to
> avoid resuming the device at remove time ? In any case, that's getting
> out of topic for the sn65dsi86 :-)
Agreed for off-topic, I adapted the Subject to make this more obvious
and added Greg and Rafael to Cc:.
Without waking the device in .remove() it's harder to properly free
resources. OTOH if you properly handle a failure to wake the device, you
cope for most of that difficulty already anyhow. Hmm.
One thing that makes this (IMHO) worse is that __device_release_driver()
calls pm_runtime_put_sync() just before device_remove() (which triggers
calling the driver's remove function). See
https://lore.kernel.org/linux-kernel/20230511073428.10264-1-u.kleine-koe...@pengutronix.de
for an earlier discussion about that topic.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
Re: [PATCH] PCI: qcom: Fix compile error
Hi Jani,
On 28/11/23 18:33, Jani Nikula wrote:
On Tue, 28 Nov 2023, Vignesh Raman wrote:
On 28/11/23 12:21, Manivannan Sadhasivam wrote:
On Tue, Nov 28, 2023 at 11:44:26AM +0530, Vignesh Raman wrote:
Hi Mani,
On 28/11/23 10:44, Manivannan Sadhasivam wrote:
On Tue, Nov 28, 2023 at 09:50:26AM +0530, Vignesh Raman wrote:
Commit a2458d8f618a ("PCI/ASPM: pci_enable_link_state: Add argument
to acquire bus lock") has added an argument to acquire bus lock
in pci_enable_link_state, but qcom_pcie_enable_aspm calls it
without this argument, resulting in below build error.
Where do you see this error? That patch is not even merged. Looks like you are
sending the patch against some downstream tree.
I got this error with drm-tip - git://anongit.freedesktop.org/drm-tip
This commit is merged in drm-intel/topic/core-for-CI -
https://cgit.freedesktop.org/drm-intel/log/?h=topic/core-for-CI
Okay. Since this patch is just for CI, please do not CC linux-pci as it causes
confusion.
Sure, thank you.
Jani, is this fix required for topic/core-for-CI ?
Done. Please double check drm-tip works for you now.
It works in drm-tip. Thank you.
Regards,
Vignesh
[PATCH v1 1/1] drm/i915/display: Don't use "proxy" headers
The driver uses math.h and not util_macros.h. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/i915/display/intel_snps_phy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/display/intel_snps_phy.c b/drivers/gpu/drm/i915/display/intel_snps_phy.c index ce5a73a4cc89..bc61e736f9b3 100644 --- a/drivers/gpu/drm/i915/display/intel_snps_phy.c +++ b/drivers/gpu/drm/i915/display/intel_snps_phy.c @@ -3,7 +3,7 @@ * Copyright © 2019 Intel Corporation */ -#include +#include #include "i915_reg.h" #include "intel_ddi.h" -- 2.43.0.rc1.1.gbec44491f096
Re: [PATCH v2 0/2] drm/bridge: panel: Check device dependency before managing device link
On Wed, Nov 29, 2023 at 1:32 PM Maxime Ripard wrote: [Me] > > It is a bigger evil to leave the tree broken than to enforce formal process, > > and it is pretty self-evident. If any of them get annoyed about it we can > > revert the patch, or both. > > Yeah, I definitely understand why you did it, but I don't think it's > something we would encourage in drm-misc. Hm OK I guess, it can be debated but no point in debating it either. > We've discussed it with Sima yesterday, and I think we would even need > the extra check in dim to make sure that a committer shouldn't do that > without dim complaining. (...) > Sima played a bit with it, and it should be doable to get something > fairly reliable if you use get_maintainers.pl to retrieve the git tree > (through scripts/get_maintainer.pl --no-email --no-l --scm) and figuring > out if only drm.git, drm-intel.git or drm-misc.git is involved. > > If it isn't, then we should check that there's the ack of one of the > maintainers. So check for any code that is hitting namespaces outside drivers/gpu/* Documentation/gpu/* or include/[uapi/]drm/* that it is ACKed by the respective maintainer for that area of the kernel? > Could you write a patch to do so? Patch dim? Well my bash skills are a bit so-so. But I guess I could learn it. But did you say there is already a prototype? Yours, Linus Walleij
[PATCH v3 00/12] RB1/QCM2290 features
This series brings:
- interconnect plumbing
- display setup
for QCM2290/QRB2210 and
- CAN bus controller
- HDMI display
- wifi fw variant name
for QTI RB1
and the necessary bindings changes
Patch 1-2 is for Dmitry/freedreno
Patch 3 for Georgi/icc
Patch 5 for Will/iommu
the rest are for Bjorn/qcom
Signed-off-by: Konrad Dybcio
---
Changes in v3:
- Pick up tags
- Fix commit msg of "iommu/arm-smmu-qcom: Add QCM2290.."
- Add missing sob ("qrb2210-rb1: add wifi variant property")
- Link to v2:
https://lore.kernel.org/r/20231125-topic-rb1_feat-v2-0-979b28f35...@linaro.org
Changes in v2:
- Fix up the bindings example in "qcm2290-mdss: Use the non-deprecated DSI
compat" (krzk)
- Fix up sc7180 & sc7280 DTs as a result of the bindings changes
- Pick up rbs where it makes sense
- Link to v1:
https://lore.kernel.org/r/20231125-topic-rb1_feat-v1-0-11d71b12b...@linaro.org
---
Dmitry Baryshkov (1):
arm64: dts: qcom: qrb2210-rb1: add wifi variant property
Konrad Dybcio (11):
dt-bindings: display: msm: qcm2290-mdss: Use the non-deprecated DSI compat
dt-bindings: display: msm: Add reg bus and rotator interconnects
dt-bindings: interconnect: qcom,msm8998-bwmon: Add QCM2290 bwmon instance
dt-bindings: firmware: qcom,scm: Allow interconnect for everyone
iommu/arm-smmu-qcom: Add QCM2290 MDSS compatible
arm64: dts: qcom: sc7180: Add the missing MDSS icc path
arm64: dts: qcom: sc7280: Add the missing MDSS icc path
arm64: dts: qcom: qcm2290: Add display nodes
arm64: dts: qcom: qcm2290: Hook up interconnects
arm64: dts: qcom: qrb2210-rb1: Set up HDMI
arm64: dts: qcom: qrb2210-rb1: Enable CAN bus controller
.../bindings/display/msm/mdss-common.yaml | 18 +-
.../bindings/display/msm/qcom,qcm2290-mdss.yaml| 21 +-
.../bindings/display/msm/qcom,sc7180-mdss.yaml | 14 +-
.../bindings/display/msm/qcom,sc7280-mdss.yaml | 14 +-
.../bindings/display/msm/qcom,sm6115-mdss.yaml | 10 +
.../bindings/display/msm/qcom,sm6125-mdss.yaml | 8 +-
.../bindings/display/msm/qcom,sm6350-mdss.yaml | 8 +-
.../bindings/display/msm/qcom,sm6375-mdss.yaml | 8 +-
.../bindings/display/msm/qcom,sm8450-mdss.yaml | 13 +-
.../devicetree/bindings/firmware/qcom,scm.yaml | 15 -
.../bindings/interconnect/qcom,msm8998-bwmon.yaml | 1 +
arch/arm64/boot/dts/qcom/qcm2290.dtsi | 462 +
arch/arm64/boot/dts/qcom/qrb2210-rb1.dts | 109 +
arch/arm64/boot/dts/qcom/sc7180.dtsi | 8 +-
arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 +-
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
16 files changed, 671 insertions(+), 48 deletions(-)
---
base-commit: 1f5c003694fab4b1ba6cbdcc417488b975c088d0
change-id: 20231125-topic-rb1_feat-dd510532621b
Best regards,
--
Konrad Dybcio
[PATCH v3 01/12] dt-bindings: display: msm: qcm2290-mdss: Use the non-deprecated DSI compat
The "qcom,dsi-ctrl-6g-qcm2290" has been deprecated in commit 0c0f65c6dd44
("dt-bindings: msm: dsi-controller-main: Add compatible strings for every
current SoC"), but the example hasn't been updated to reflect that.
Fix that.
Fixes: 0c0f65c6dd44 ("dt-bindings: msm: dsi-controller-main: Add compatible
strings for every current SoC")
Reviewed-by: Krzysztof Kozlowski
Signed-off-by: Konrad Dybcio
---
.../devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git
a/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml
b/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml
index 5ad155612b6c..d71a8e09a798 100644
--- a/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml
+++ b/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml
@@ -56,7 +56,9 @@ patternProperties:
properties:
compatible:
-const: qcom,dsi-ctrl-6g-qcm2290
+items:
+ - const: qcom,qcm2290-dsi-ctrl
+ - const: qcom,mdss-dsi-ctrl
"^phy@[0-9a-f]+$":
type: object
@@ -136,7 +138,8 @@ examples:
};
dsi@5e94000 {
-compatible = "qcom,dsi-ctrl-6g-qcm2290";
+compatible = "qcom,qcm2290-dsi-ctrl",
+ "qcom,mdss-dsi-ctrl";
reg = <0x05e94000 0x400>;
reg-names = "dsi_ctrl";
--
2.43.0
[PATCH v3 02/12] dt-bindings: display: msm: Add reg bus and rotator interconnects
Apart from the already handled data bus (MAS_MDP_Pn<->DDR), there are other connection paths: - a path that connects rotator block to the DDR. - a path that needs to be handled to ensure MDSS register access functions properly, namely the "reg bus", a.k.a the CPU-MDSS CFG interconnect. Describe these paths to allow using them in device trees and in the driver. Signed-off-by: Dmitry Baryshkov [Konrad: rework for one vs two MDP paths, update examples] Reviewed-by: Krzysztof Kozlowski Signed-off-by: Konrad Dybcio --- .../devicetree/bindings/display/msm/mdss-common.yaml | 18 ++ .../bindings/display/msm/qcom,qcm2290-mdss.yaml| 14 ++ .../bindings/display/msm/qcom,sc7180-mdss.yaml | 14 ++ .../bindings/display/msm/qcom,sc7280-mdss.yaml | 14 ++ .../bindings/display/msm/qcom,sm6115-mdss.yaml | 10 ++ .../bindings/display/msm/qcom,sm6125-mdss.yaml | 8 ++-- .../bindings/display/msm/qcom,sm6350-mdss.yaml | 8 ++-- .../bindings/display/msm/qcom,sm6375-mdss.yaml | 8 ++-- .../bindings/display/msm/qcom,sm8450-mdss.yaml | 13 - 9 files changed, 80 insertions(+), 27 deletions(-) diff --git a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml index f69196e4cc76..c6305a6e0334 100644 --- a/Documentation/devicetree/bindings/display/msm/mdss-common.yaml +++ b/Documentation/devicetree/bindings/display/msm/mdss-common.yaml @@ -61,17 +61,27 @@ properties: ranges: true + # This is not a perfect description, but it's impossible to discern and match + # the entries like we do with interconnect-names interconnects: minItems: 1 items: - description: Interconnect path from mdp0 (or a single mdp) port to the data bus - description: Interconnect path from mdp1 port to the data bus + - description: Interconnect path from CPU to the reg bus interconnect-names: -minItems: 1 -items: - - const: mdp0-mem - - const: mdp1-mem +oneOf: + - minItems: 1 +items: + - const: mdp0-mem + - const: cpu-cfg + + - minItems: 2 +items: + - const: mdp0-mem + - const: mdp1-mem + - const: cpu-cfg resets: items: diff --git a/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml index d71a8e09a798..f0cdb5422688 100644 --- a/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml +++ b/Documentation/devicetree/bindings/display/msm/qcom,qcm2290-mdss.yaml @@ -36,10 +36,14 @@ properties: maxItems: 2 interconnects: -maxItems: 1 +items: + - description: Interconnect path from mdp0 port to the data bus + - description: Interconnect path from CPU to the reg bus interconnect-names: -maxItems: 1 +items: + - const: mdp0-mem + - const: cpu-cfg patternProperties: "^display-controller@[0-9a-f]+$": @@ -98,8 +102,10 @@ examples: interrupt-controller; #interrupt-cells = <1>; -interconnects = <&mmrt_virt MASTER_MDP0 &bimc SLAVE_EBI1>; -interconnect-names = "mdp0-mem"; +interconnects = <&mmrt_virt MASTER_MDP0 &bimc SLAVE_EBI1>, +<&bimc MASTER_APPSS_PROC &config_noc SLAVE_DISPLAY_CFG>; +interconnect-names = "mdp0-mem", + "cpu-cfg"; iommus = <&apps_smmu 0x420 0x2>, <&apps_smmu 0x421 0x0>; diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-mdss.yaml index 3432a2407caa..7a0555b15ddf 100644 --- a/Documentation/devicetree/bindings/display/msm/qcom,sc7180-mdss.yaml +++ b/Documentation/devicetree/bindings/display/msm/qcom,sc7180-mdss.yaml @@ -36,10 +36,14 @@ properties: maxItems: 1 interconnects: -maxItems: 1 +items: + - description: Interconnect path from mdp0 port to the data bus + - description: Interconnect path from CPU to the reg bus interconnect-names: -maxItems: 1 +items: + - const: mdp0-mem + - const: cpu-cfg patternProperties: "^display-controller@[0-9a-f]+$": @@ -106,8 +110,10 @@ examples: interrupt-controller; #interrupt-cells = <1>; -interconnects = <&mmss_noc MASTER_MDP0 &mc_virt SLAVE_EBI1>; -interconnect-names = "mdp0-mem"; +interconnects = <&mmss_noc MASTER_MDP0 &mc_virt SLAVE_EBI1>, +<&gem_noc MASTER_APPSS_PROC &config_noc SLAVE_DISPLAY_CFG>; +interconnect-names = "mdp0-mem", + "cpu-cfg"; iommus = <&apps_smmu 0x800 0x2>; ranges; diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sc7280-mdss.yaml b/Documentation/devic
[PATCH v3 03/12] dt-bindings: interconnect: qcom,msm8998-bwmon: Add QCM2290 bwmon instance
QCM2290 has a single BWMONv4 intance for CPU. Document it. Reviewed-by: Krzysztof Kozlowski Signed-off-by: Konrad Dybcio --- Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml index 7cb8df757477..a88cea732370 100644 --- a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml +++ b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml @@ -25,6 +25,7 @@ properties: - const: qcom,msm8998-bwmon # BWMON v4 - items: - enum: + - qcom,qcm2290-cpu-bwmon - qcom,sc7180-cpu-bwmon - qcom,sc7280-cpu-bwmon - qcom,sc8280xp-cpu-bwmon -- 2.43.0
[PATCH v3 04/12] dt-bindings: firmware: qcom,scm: Allow interconnect for everyone
Every Qualcomm SoC physically has a "CRYPTO0<->DDR" interconnect lane. Allow this property to be present, no matter the SoC. Reviewed-by: Krzysztof Kozlowski Signed-off-by: Konrad Dybcio --- Documentation/devicetree/bindings/firmware/qcom,scm.yaml | 15 --- 1 file changed, 15 deletions(-) diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml index 0613a37a851a..f3a87a8426d0 100644 --- a/Documentation/devicetree/bindings/firmware/qcom,scm.yaml +++ b/Documentation/devicetree/bindings/firmware/qcom,scm.yaml @@ -178,21 +178,6 @@ allOf: minItems: 3 maxItems: 3 - # Interconnects - - if: - not: -properties: - compatible: -contains: - enum: -- qcom,scm-qdu1000 -- qcom,scm-sc8280xp -- qcom,scm-sm8450 -- qcom,scm-sm8550 -then: - properties: -interconnects: false - # Interrupts - if: not: -- 2.43.0
[PATCH v3 07/12] arm64: dts: qcom: sc7280: Add the missing MDSS icc path
MDSS, aside from the MDP-MEM path, also requires the CPU-DISP_CFG one.
Failing to provide it may result in register accesses failing and that's
never good.
Add the missing path.
Signed-off-by: Konrad Dybcio
---
arch/arm64/boot/dts/qcom/sc7280.dtsi | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi
b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index 04bf85b0399a..41d327b1f1b6 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -3958,8 +3959,12 @@ mdss: display-subsystem@ae0 {
interrupt-controller;
#interrupt-cells = <1>;
- interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt
SLAVE_EBI1 0>;
- interconnect-names = "mdp0-mem";
+ interconnects = <&mmss_noc MASTER_MDP0
QCOM_ICC_TAG_ALWAYS
+&mc_virt SLAVE_EBI1
QCOM_ICC_TAG_ALWAYS>,
+ <&gem_noc MASTER_APPSS_PROC
QCOM_ICC_TAG_ALWAYS
+&cnoc2 SLAVE_DISPLAY_CFG
QCOM_ICC_TAG_ALWAYS>;
+ interconnect-names = "mdp0-mem",
+"cpu-cfg";
iommus = <&apps_smmu 0x900 0x402>;
--
2.43.0
[PATCH v3 08/12] arm64: dts: qcom: qcm2290: Add display nodes
Add the required nodes to support display on QCM2290.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Konrad Dybcio
---
arch/arm64/boot/dts/qcom/qcm2290.dtsi | 214 ++
1 file changed, 214 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcm2290.dtsi
b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
index d46e591e72b5..a3edc4667cc5 100644
--- a/arch/arm64/boot/dts/qcom/qcm2290.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
@@ -5,6 +5,7 @@
* Based on sm6115.dtsi and previous efforts by Shawn Guo & Loic Poulain.
*/
+#include
#include
#include
#include
@@ -1105,6 +1106,219 @@ usb_dwc3: usb@4e0 {
};
};
+ mdss: display-subsystem@5e0 {
+ compatible = "qcom,qcm2290-mdss";
+ reg = <0x0 0x05e0 0x0 0x1000>;
+ reg-names = "mdss";
+ interrupts = ;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ clocks = <&gcc GCC_DISP_AHB_CLK>,
+<&gcc GCC_DISP_HF_AXI_CLK>,
+<&dispcc DISP_CC_MDSS_MDP_CLK>;
+ clock-names = "iface",
+ "bus",
+ "core";
+
+ resets = <&dispcc DISP_CC_MDSS_CORE_BCR>;
+
+ power-domains = <&dispcc MDSS_GDSC>;
+
+ iommus = <&apps_smmu 0x420 0x2>,
+<&apps_smmu 0x421 0x0>;
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ status = "disabled";
+
+ mdp: display-controller@5e01000 {
+ compatible = "qcom,qcm2290-dpu";
+ reg = <0x0 0x05e01000 0x0 0x8f000>,
+ <0x0 0x05eb 0x0 0x2008>;
+ reg-names = "mdp",
+ "vbif";
+
+ interrupt-parent = <&mdss>;
+ interrupts = <0>;
+
+ clocks = <&gcc GCC_DISP_HF_AXI_CLK>,
+<&dispcc DISP_CC_MDSS_AHB_CLK>,
+<&dispcc DISP_CC_MDSS_MDP_CLK>,
+<&dispcc DISP_CC_MDSS_MDP_LUT_CLK>,
+<&dispcc DISP_CC_MDSS_VSYNC_CLK>;
+ clock-names = "bus",
+ "iface",
+ "core",
+ "lut",
+ "vsync";
+
+ operating-points-v2 = <&mdp_opp_table>;
+ power-domains = <&rpmpd QCM2290_VDDCX>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ dpu_intf1_out: endpoint {
+ remote-endpoint =
<&mdss_dsi0_in>;
+ };
+ };
+ };
+
+ mdp_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-1920 {
+ opp-hz = /bits/ 64 <1920>;
+ required-opps =
<&rpmpd_opp_min_svs>;
+ };
+
+ opp-19200 {
+ opp-hz = /bits/ 64 <19200>;
+ required-opps =
<&rpmpd_opp_low_svs>;
+ };
+
+ opp-25600 {
+ opp-hz = /bits/ 64 <25600>;
+ required-opps =
<&rpmpd_opp_svs>;
+ };
+
+ opp-30720 {
+ opp-hz = /bits/ 64 <30720>;
+ required-opps =
<&rpmpd_opp_svs_plus>;
+ };
+
+ opp-38400 {
+ opp-hz = /bits/ 64 <38400>;
+ re
[PATCH v3 10/12] arm64: dts: qcom: qrb2210-rb1: Set up HDMI
Add the required nodes to support display output via the HDMI port.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Konrad Dybcio
---
arch/arm64/boot/dts/qcom/qrb2210-rb1.dts | 86
1 file changed, 86 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
b/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
index 94885b9c21c8..ac6584164058 100644
--- a/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
+++ b/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
@@ -40,6 +40,17 @@ key-volume-up {
};
};
+ hdmi-connector {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_con: endpoint {
+ remote-endpoint = <<9611_out>;
+ };
+ };
+ };
+
leds {
compatible = "gpio-leds";
@@ -158,6 +169,68 @@ vph_pwr: regulator-vph-pwr {
};
};
+&gpi_dma0 {
+ status = "okay";
+};
+
+&i2c2 {
+ clock-frequency = <40>;
+ status = "okay";
+
+ lt9611_codec: hdmi-bridge@2b {
+ compatible = "lontium,lt9611uxc";
+ reg = <0x2b>;
+ interrupts-extended = <&tlmm 46 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = <&tlmm 41 GPIO_ACTIVE_HIGH>;
+
+ vdd-supply = <&vreg_hdmi_out_1p2>;
+ vcc-supply = <<9611_3v3>;
+
+ pinctrl-0 = <<9611_irq_pin <9611_rst_pin>;
+ pinctrl-names = "default";
+ #sound-dai-cells = <1>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lt9611_a: endpoint {
+ remote-endpoint = <&mdss_dsi0_out>;
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+
+ lt9611_out: endpoint {
+ remote-endpoint = <&hdmi_con>;
+ };
+ };
+ };
+ };
+};
+
+&mdss {
+ status = "okay";
+};
+
+&mdss_dsi0 {
+ vdda-supply = <&pm2250_l5>;
+ status = "okay";
+};
+
+&mdss_dsi0_out {
+ remote-endpoint = <<9611_a>;
+ data-lanes = <0 1 2 3>;
+};
+
+&mdss_dsi0_phy {
+ status = "okay";
+};
+
&pm2250_resin {
linux,code = ;
status = "okay";
@@ -377,6 +450,19 @@ &sdhc_2 {
};
&tlmm {
+ lt9611_rst_pin: lt9611-rst-state {
+ pins = "gpio41";
+ function = "gpio";
+ input-disable;
+ output-high;
+ };
+
+ lt9611_irq_pin: lt9611-irq-state {
+ pins = "gpio46";
+ function = "gpio";
+ bias-disable;
+ };
+
sd_det_in_on: sd-det-in-on-state {
pins = "gpio88";
function = "gpio";
--
2.43.0
[PATCH v3 06/12] arm64: dts: qcom: sc7180: Add the missing MDSS icc path
MDSS, aside from the MDP-MEM path, also requires the CPU-DISP_CFG one.
Failing to provide it may result in register accesses failing and that's
never good.
Add the missing path.
Signed-off-by: Konrad Dybcio
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 11f353d416b4..9664e42faeb1 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -3100,8 +3100,12 @@ mdss: display-subsystem@ae0 {
interrupt-controller;
#interrupt-cells = <1>;
- interconnects = <&mmss_noc MASTER_MDP0 0 &mc_virt
SLAVE_EBI1 0>;
- interconnect-names = "mdp0-mem";
+ interconnects = <&mmss_noc MASTER_MDP0
QCOM_ICC_TAG_ALWAYS
+&mc_virt SLAVE_EBI1
QCOM_ICC_TAG_ALWAYS>,
+ <&gem_noc MASTER_APPSS_PROC
QCOM_ICC_TAG_ALWAYS
+&config_noc SLAVE_DISPLAY_CFG
QCOM_ICC_TAG_ALWAYS>;
+ interconnect-names = "mdp0-mem",
+"cpu-cfg";
iommus = <&apps_smmu 0x800 0x2>;
--
2.43.0
[PATCH v3 05/12] iommu/arm-smmu-qcom: Add QCM2290 MDSS compatible
Add the QCM2290 MDSS compatible to clients compatible list, as it also
needs the workarounds.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Konrad Dybcio
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 549ae4dba3a6..aea5e85b20ff 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -245,6 +245,7 @@ static const struct of_device_id
qcom_smmu_client_of_match[] __maybe_unused = {
{ .compatible = "qcom,adreno" },
{ .compatible = "qcom,mdp4" },
{ .compatible = "qcom,mdss" },
+ { .compatible = "qcom,qcm2290-mdss" },
{ .compatible = "qcom,sc7180-mdss" },
{ .compatible = "qcom,sc7180-mss-pil" },
{ .compatible = "qcom,sc7280-mdss" },
--
2.43.0
[PATCH v3 11/12] arm64: dts: qcom: qrb2210-rb1: Enable CAN bus controller
Enable the Microchip mcp2518fd hosted on the SPI5 bus.
Signed-off-by: Konrad Dybcio
---
arch/arm64/boot/dts/qcom/qrb2210-rb1.dts | 22 ++
1 file changed, 22 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
b/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
index ac6584164058..ac597eb3fe9d 100644
--- a/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
+++ b/arch/arm64/boot/dts/qcom/qrb2210-rb1.dts
@@ -23,6 +23,14 @@ chosen {
stdout-path = "serial0:115200n8";
};
+ clocks {
+ clk40M: can-clk {
+ compatible = "fixed-clock";
+ clock-frequency = <4000>;
+ #clock-cells = <0>;
+ };
+ };
+
gpio-keys {
compatible = "gpio-keys";
label = "gpio-keys";
@@ -449,6 +457,20 @@ &sdhc_2 {
status = "okay";
};
+&spi5 {
+ status = "okay";
+
+ can@0 {
+ compatible = "microchip,mcp2518fd";
+ reg = <0>;
+ interrupts-extended = <&tlmm 39 IRQ_TYPE_LEVEL_LOW>;
+ clocks = <&clk40M>;
+ spi-max-frequency = <1000>;
+ vdd-supply = <&vdc_5v>;
+ xceiver-supply = <&vdc_5v>;
+ };
+};
+
&tlmm {
lt9611_rst_pin: lt9611-rst-state {
pins = "gpio41";
--
2.43.0
[PATCH v3 09/12] arm64: dts: qcom: qcm2290: Hook up interconnects
Add interconnect provider nodes and hook up interconnects to consumer
devices, including bwmon.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Konrad Dybcio
---
arch/arm64/boot/dts/qcom/qcm2290.dtsi | 248 ++
1 file changed, 248 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcm2290.dtsi
b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
index a3edc4667cc5..ce04d0acdede 100644
--- a/arch/arm64/boot/dts/qcom/qcm2290.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcm2290.dtsi
@@ -12,6 +12,8 @@
#include
#include
#include
+#include
+#include
#include
/ {
@@ -151,6 +153,8 @@ scm: scm {
clocks = <&rpmcc RPM_SMD_CE1_CLK>;
clock-names = "core";
#reset-cells = <1>;
+ interconnects = <&system_noc MASTER_CRYPTO_CORE0
RPM_ALWAYS_TAG
+&bimc SLAVE_EBI1 RPM_ALWAYS_TAG>;
};
};
@@ -669,6 +673,33 @@ usb_qmpphy: phy@1615000 {
status = "disabled";
};
+ system_noc: interconnect@188 {
+ compatible = "qcom,qcm2290-snoc";
+ reg = <0x0 0x0188 0x0 0x60200>;
+ #interconnect-cells = <2>;
+
+ qup_virt: interconnect-qup {
+ compatible = "qcom,qcm2290-qup-virt";
+ #interconnect-cells = <2>;
+ };
+
+ mmnrt_virt: interconnect-mmnrt {
+ compatible = "qcom,qcm2290-mmnrt-virt";
+ #interconnect-cells = <2>;
+ };
+
+ mmrt_virt: interconnect-mmrt {
+ compatible = "qcom,qcm2290-mmrt-virt";
+ #interconnect-cells = <2>;
+ };
+ };
+
+ config_noc: interconnect@190 {
+ compatible = "qcom,qcm2290-cnoc";
+ reg = <0x0 0x0190 0x0 0x8200>;
+ #interconnect-cells = <2>;
+ };
+
qfprom@1b44000 {
compatible = "qcom,qcm2290-qfprom", "qcom,qfprom";
reg = <0x0 0x01b44000 0x0 0x3000>;
@@ -681,6 +712,60 @@ qusb2_hstx_trim: hstx-trim@25b {
};
};
+ pmu@1b8e300 {
+ compatible = "qcom,qcm2290-cpu-bwmon",
"qcom,sdm845-bwmon";
+ reg = <0x0 0x01b8e300 0x0 0x600>;
+ interrupts = ;
+
+ operating-points-v2 = <&cpu_bwmon_opp_table>;
+ interconnects = <&bimc MASTER_APPSS_PROC RPM_ACTIVE_TAG
+&bimc SLAVE_EBI1 RPM_ACTIVE_TAG>;
+
+ cpu_bwmon_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-0 {
+ opp-peak-kBps = <(200 * 4 * 1000)>;
+ };
+
+ opp-1 {
+ opp-peak-kBps = <(300 * 4 * 1000)>;
+ };
+
+ opp-2 {
+ opp-peak-kBps = <(451 * 4 * 1000)>;
+ };
+
+ opp-3 {
+ opp-peak-kBps = <(547 * 4 * 1000)>;
+ };
+
+ opp-4 {
+ opp-peak-kBps = <(681 * 4 * 1000)>;
+ };
+
+ opp-5 {
+ opp-peak-kBps = <(768 * 4 * 1000)>;
+ };
+
+ opp-6 {
+ opp-peak-kBps = <(1017 * 4 * 1000)>;
+ };
+
+ opp-7 {
+ opp-peak-kBps = <(1353 * 4 * 1000)>;
+ };
+
+ opp-8 {
+ opp-peak-kBps = <(1555 * 4 * 1000)>;
+ };
+
+ opp-9 {
+ opp-peak-kBps = <(1804 * 4 * 1000)>;
+ };
+ };
+ };
+
spmi_bus: spmi@1c4 {
compatible = "qcom,spmi-pmic-arb";
reg = <0x0 0x01c4 0x0 0x1100>,
@@ -721,6 +806,12 @@ rng: rng@4453000 {
clock-names = "core";
};
+ bimc: interconnect@448 {
+ compatible = "qcom,qcm2290-bimc";
+
