Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/msm: misordered msm_dcp_register() and msm_dcp_unregister() calls - [High] drm/msm: missing msm_separate_gpu_kms_components() check and DCP count - [Low] drm/msm: link failure in msm_dcp_register() when DRM_MSM=y and VIRTIO=m --
commit 8c20c0a0e35ec53b4a670a45026a398c4476bb45 Author: Jayant Shekhar <[email protected]> drm/msm: Register the DCP backend This commit registers and unregisters the DCP platform driver from the MSM DRM module init and exit paths, and provides no-op stubs in msm_drv.h when CONFIG_DRM_MSM_DCP is disabled. > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index 5816c64d7ce7d..d413c361c4426 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -1208,6 +1208,7 @@ static int __init msm_drm_register(void) > DBG("init"); > msm_mdp_register(); > msm_dpu_register(); > + msm_dcp_register(); > msm_dsi_register(); > msm_hdmi_register(); > msm_dp_register(); > @@ -1229,6 +1230,7 @@ static void __exit msm_drm_unregister(void) > msm_dsi_unregister(); > msm_mdp_unregister(); > msm_dpu_unregister(); > + msm_dcp_unregister(); [Severity: High] Are msm_dcp_register() and msm_dcp_unregister() ordered in the right place relative to adreno_register() and adreno_unregister()? In msm_drm_register() and msm_drm_unregister(), MDP5 (msm_mdp_register()) and DPU (msm_dpu_register()) are registered first and unregistered last because their platform devices are children of the top-level msm_mdss device. They are populated when msm_mdss_register() runs at the end of msm_drm_register() and depopulated via mdss_remove()->of_platform_depopulate() when msm_mdss_unregister() runs at the start of msm_drm_unregister(), which tears down the component master before adreno_unregister() is called. By contrast, DCP (qcom,dcp-virtio) is a standalone top-level platform device rather than a child of msm_mdss: drivers/gpu/drm/msm/dcp/dcp_drv.c:dcp_component_register() { ... /* A GPU is optional for the DCP display-only DRM device. */ msm_add_gpu_components(dev, &match); ret = component_master_add_with_match(dev, &msm_drm_ops, match); ... } drivers/gpu/drm/msm/dcp/dcp_drv.c:dcp_component_unregister() { struct msm_drm_private *drv = dev_get_drvdata(dev); component_master_del(dev, &msm_drm_ops); dcp_dpu_kms_cleanup_bridges(drv->kms); } Calling msm_dcp_register() before adreno_register() registers the DCP component master before the Adreno component driver is registered. On module unload, calling msm_dcp_unregister() at the end of msm_drm_unregister() runs adreno_unregister() while the DCP component master (dcp_subsystem_dev) is still active and bound, rather than first deleting the component master via msm_dcp_unregister()->dcp_virtio_unregister()->dcp_virtio_remove()-> dcp_component_unregister()->component_master_del(). Should msm_dcp_register() be called at the end of msm_drm_register() and msm_dcp_unregister() at the start of msm_drm_unregister(), alongside msm_mdp4_register()/msm_mdss_register() and msm_mdss_unregister()/msm_mdp4_unregister()? > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h > index aa5dfd82ac6f7..befaa9f2cb5c5 100644 > --- a/drivers/gpu/drm/msm/msm_drv.h > +++ b/drivers/gpu/drm/msm/msm_drv.h > @@ -417,6 +417,14 @@ static inline void msm_dpu_register(void) {} > static inline void msm_dpu_unregister(void) {} > #endif > > +#ifdef CONFIG_DRM_MSM_DCP > +void msm_dcp_register(void); > +void msm_dcp_unregister(void); [Severity: Low] Will enabling CONFIG_DRM_MSM_DCP cause a vmlinux link failure when CONFIG_DRM_MSM=y and CONFIG_VIRTIO=m? In drivers/gpu/drm/msm/Kconfig, DRM_MSM_DCP is a bool symbol that depends on tristate DRM_MSM and tristate VIRTIO: config DRM_MSM_DCP bool "Enable DCP firmware-delegated display support in MSM DRM driver" depends on DRM_MSM depends on VIRTIO When CONFIG_DRM_MSM=y and CONFIG_VIRTIO=m, CONFIG_DRM_MSM_DCP can be enabled as y, building msm_drv.o and the DCP VirtIO backend objects into vmlinux while virtio.ko and virtio_ring.ko are built as loadable modules, leaving msm_drm_register()->msm_dcp_register()->dcp_virtio_register() with unresolved references to VirtIO symbols such as register_virtio_driver(), register_virtio_device(), and vring_create_virtqueue(). > +#else > +static inline void msm_dcp_register(void) {} > +static inline void msm_dcp_unregister(void) {} > +#endif [Severity: High] How does DCP registration interact with msm_separate_gpu_kms_components() and msm_separate_gpu_kms_auto()? When msm_dcp_register()->dcp_virtio_register()->dcp_virtio_probe()-> dcp_component_register() runs, it adds the GPU to the component match list unconditionally: drivers/gpu/drm/msm/dcp/dcp_drv.c:dcp_component_register() { ... /* A GPU is optional for the DCP display-only DRM device. */ msm_add_gpu_components(dev, &match); ret = component_master_add_with_match(dev, &msm_drm_ops, match); ... } Unlike msm_drv_probe() in drivers/gpu/drm/msm/msm_drv.c, this does not check !msm_separate_gpu_kms_components(): drivers/gpu/drm/msm/msm_drv.c:msm_drv_probe() { ... if (!msm_separate_gpu_kms_components()) { ret = msm_add_gpu_components(master_dev, &match); if (ret) return ret; } ... } When msm_separate_gpu_kms_components() is true and an Adreno GPU is present in DT, adreno_probe() calls msm_gpu_probe() instead of component_add(). Because dcp_component_register() still adds the GPU node to its match list, component_master_add_with_match() will wait indefinitely for a GPU component that is never registered, preventing the DCP DRM device from ever binding. In addition, unlike CONFIG_DRM_MSM_MDP4 and CONFIG_DRM_MSM_MDSS in drivers/gpu/drm/msm/msm_drv.h, no msm_count_dcp() helper is provided or included in msm_separate_gpu_kms_auto(): drivers/gpu/drm/msm/msm_drv.c:msm_separate_gpu_kms_auto() { int gpus = msm_count_gpus(); int display_subsystems = msm_count_mdss() + msm_count_mdp4(); if (gpus <= 0 || display_subsystems <= 0) return false; /* Single card if there is exactly one GPU and one display subsystem */ return gpus > 1 || display_subsystems > 1; } Without counting DCP nodes in display_subsystems, won't msm_separate_gpu_kms_auto() miscalculate the number of display subsystems on platforms with DCP? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=22
