From 993a2adc6e2e94a0a7b5bfc054eda90ac95f62c3 Mon Sep 17 00:00:00 2001 From: Kuogee Hsieh Date: Tue, 17 May 2022 09:21:34 -0700 Subject: drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() dp_catalog_ctrl_reset() will software reset DP controller. But it will not reset programmable registers to default value. DP driver still have to clear mask bits to interrupt status registers to disable interrupts after software reset of controller. At current implementation, dp_ctrl_reset_irq_ctrl() will software reset dp controller but did not call dp_catalog_ctrl_enable_irq(false) to clear hpd related interrupt mask bits to disable hpd related interrupts due to it mistakenly think hpd related interrupt mask bits will be cleared by software reset of dp controller automatically. This mistake may cause system to crash during suspending procedure due to unexpected irq fired and trigger event thread to access dp controller registers with controller clocks are disabled. This patch fixes system crash during suspending problem by removing "enable" flag condition checking at dp_ctrl_reset_irq_ctrl() so that hpd related interrupt mask bits are cleared to prevent unexpected from happening. Changes in v2: -- add more details commit text Changes in v3: -- add synchrons_irq() -- add atomic_t suspended Changes in v4: -- correct Fixes's commit ID -- remove synchrons_irq() Changes in v5: -- revise commit text Changes in v6: -- add event_lock to protect "suspended" Changes in v7: -- delete "suspended" flag Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until plugin interrupt received") Signed-off-by: Kuogee Hsieh Reviewed-by: Stephen Boyd Patchwork: https://patchwork.freedesktop.org/patch/486591/ Link: https://lore.kernel.org/r/1652804494-19650-1-git-send-email-quic_khsieh@quicinc.com Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/dp/dp_ctrl.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c index af7a80c63610..f3e333eff32d 100644 --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable) dp_catalog_ctrl_reset(ctrl->catalog); - if (enable) - dp_catalog_ctrl_enable_irq(ctrl->catalog, enable); + /* + * all dp controller programmable registers will not + * be reset to default value after DP_SW_RESET + * therefore interrupt mask bits have to be updated + * to enable/disable interrupts + */ + dp_catalog_ctrl_enable_irq(ctrl->catalog, enable); } void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl) -- cgit v1.2.3 From 6daf7e4aa91e05b3e138805613686fb5c46d8196 Mon Sep 17 00:00:00 2001 From: Vinod Koul Date: Wed, 25 May 2022 13:09:12 +0530 Subject: drm/msm/disp/dpu1: remove superfluous init Commit 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder") added dsc_common_mode variable which was set to zero but then again programmed, so drop the superfluous init. Fixes: 58dca9810749 ("drm/msm/disp/dpu1: Add support for DSC in encoder") Reported-by: kernel test robot Reviewed-by: Dmitry Baryshkov Reviewed-by: Abhinav Kumar Signed-off-by: Vinod Koul Patchwork: https://patchwork.freedesktop.org/patch/487208/ Link: https://lore.kernel.org/r/20220525073912.2706505-1-vkoul@kernel.org Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 52516eb20cb8..8da01d862b83 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1814,7 +1814,6 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, } } - dsc_common_mode = 0; pic_width = dsc->drm->pic_width; dsc_common_mode = DSC_MODE_MULTIPLEX | DSC_MODE_SPLIT_PANEL; -- cgit v1.2.3 From fb0af2daaa04d69b9e66613b8721d23d41f387f6 Mon Sep 17 00:00:00 2001 From: Jiapeng Chong Date: Tue, 24 May 2022 16:14:13 +0800 Subject: drm/msm/dpu: Remove unused code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eliminate the follow clang warning: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:544:33: warning: variable ‘mode’ set but not used [-Wunused-but-set-variable]. Reported-by: Abaci Robot Signed-off-by: Jiapeng Chong Fixes: 3177589c6e93("drm/msm/dpu: encoder: drop unused mode_fixup callback") Reviewed-by: Abhinav Kumar Patchwork: https://patchwork.freedesktop.org/patch/487136/ Link: https://lore.kernel.org/r/20220524081413.37895-1-jiapeng.chong@linux.alibaba.com Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 8da01d862b83..3a462e327e0e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -541,7 +541,6 @@ static int dpu_encoder_virt_atomic_check( struct dpu_encoder_virt *dpu_enc; struct msm_drm_private *priv; struct dpu_kms *dpu_kms; - const struct drm_display_mode *mode; struct drm_display_mode *adj_mode; struct msm_display_topology topology; struct dpu_global_state *global_state; @@ -559,7 +558,6 @@ static int dpu_encoder_virt_atomic_check( priv = drm_enc->dev->dev_private; dpu_kms = to_dpu_kms(priv->kms); - mode = &crtc_state->mode; adj_mode = &crtc_state->adjusted_mode; global_state = dpu_kms_get_global_state(crtc_state->state); if (IS_ERR(global_state)) -- cgit v1.2.3 From 8caad14e722477807e9324fe0e85dd10151fb134 Mon Sep 17 00:00:00 2001 From: Haowen Bai Date: Mon, 30 May 2022 10:19:55 +0800 Subject: drm/msm/dpu: Fix pointer dereferenced before checking The phys_enc->wb_idx is dereferencing before null checking, so move it after checking. Signed-off-by: Haowen Bai Reviewed-by: Dmitry Baryshkov Fixes: d7d0e73f7de33 ("drm/msm/dpu: introduce the dpu_encoder_phys_* for Reviewed-by: Abhinav Kumar Patchwork: https://patchwork.freedesktop.org/patch/487606/ Link: https://lore.kernel.org/r/1653877196-23114-1-git-send-email-baihaowen@meizu.com Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c index 4829d1ce0cf8..59da348ff339 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c @@ -574,11 +574,11 @@ static void dpu_encoder_phys_wb_disable(struct dpu_encoder_phys *phys_enc) */ static void dpu_encoder_phys_wb_destroy(struct dpu_encoder_phys *phys_enc) { - DPU_DEBUG("[wb:%d]\n", phys_enc->wb_idx - WB_0); - if (!phys_enc) return; + DPU_DEBUG("[wb:%d]\n", phys_enc->wb_idx - WB_0); + kfree(phys_enc); } -- cgit v1.2.3 From b9364eed9232f3d2a846f68c2307eb25c93cc2d0 Mon Sep 17 00:00:00 2001 From: Douglas Anderson Date: Tue, 31 May 2022 16:01:26 -0700 Subject: drm/msm/dpu: Move min BW request and full BW disable back to mdss In commit a670ff578f1f ("drm/msm/dpu: always use mdp device to scale bandwidth") we fully moved interconnect stuff to the DPU driver. This had no change for sc7180 but _did_ have an impact for other SoCs. It made them match the sc7180 scheme. Unfortunately, the sc7180 scheme seems like it was a bit broken. Specifically the interconnect needs to be on for more than just the DPU driver's AXI bus. In the very least it also needs to be on for the DSI driver's AXI bus. This can be seen fairly easily by doing this on a ChromeOS sc7180-trogdor class device: set_power_policy --ac_screen_dim_delay=5 --ac_screen_off_delay=10 sleep 10 cd /sys/bus/platform/devices/ae94000.dsi/power echo on > control When you do that, you'll get a warning splat in the logs about "gcc_disp_hf_axi_clk status stuck at 'off'". One could argue that perhaps what I have done above is "illegal" and that it can't happen naturally in the system because in normal system usage the DPU is pretty much always on when DSI is on. That being said: * In official ChromeOS builds (admittedly a 5.4 kernel with backports) we have seen that splat at bootup. * Even though we don't use "autosuspend" for these components, we don't use the "put_sync" variants. Thus plausibly the DSI could stay "runtime enabled" past when the DPU is enabled. Techncially we shouldn't do that if the DPU's suspend ends up yanking our clock. Let's change things such that the "bare minimum" request for the interconnect happens in the mdss driver again. That means that all of the children can assume that the interconnect is on at the minimum bandwidth. We'll then let the DPU request the higher amount that it wants. It should be noted that this isn't as hacky of a solution as it might initially appear. Specifically: * Since MDSS and DPU individually get their own references to the interconnect then the framework will actually handle aggregating them. The two drivers are _not_ clobbering each other. * When the Qualcomm interconnect driver aggregates it takes the max of all the peaks. Thus having MDSS request a peak, as we're doing here, won't actually change the total interconnect bandwidth (it won't be added to the request for the DPU). This perhaps explains why the "average" requested in MDSS was historically 0 since that one _would_ be added in. NOTE also that in the downstream ChromeOS 5.4 and 5.15 kernels, we're also seeing some RPMH hangs that are addressed by this fix. These hangs are showing up in the field and on _some_ devices with enough stress testing of suspend/resume. Specifically right at suspend time with a stack crawl that looks like this (from chromeos-5.15 tree): rpmh_write_batch+0x19c/0x240 qcom_icc_bcm_voter_commit+0x210/0x420 qcom_icc_set+0x28/0x38 apply_constraints+0x70/0xa4 icc_set_bw+0x150/0x24c dpu_runtime_resume+0x50/0x1c4 pm_generic_runtime_resume+0x30/0x44 __genpd_runtime_resume+0x68/0x7c genpd_runtime_resume+0x12c/0x20c __rpm_callback+0x98/0x138 rpm_callback+0x30/0x88 rpm_resume+0x370/0x4a0 __pm_runtime_resume+0x80/0xb0 dpu_kms_enable_commit+0x24/0x30 msm_atomic_commit_tail+0x12c/0x630 commit_tail+0xac/0x150 drm_atomic_helper_commit+0x114/0x11c drm_atomic_commit+0x68/0x78 drm_atomic_helper_disable_all+0x158/0x1c8 drm_atomic_helper_suspend+0xc0/0x1c0 drm_mode_config_helper_suspend+0x2c/0x60 msm_pm_prepare+0x2c/0x40 pm_generic_prepare+0x30/0x44 genpd_prepare+0x80/0xd0 device_prepare+0x78/0x17c dpm_prepare+0xb0/0x384 dpm_suspend_start+0x34/0xc0 We don't completely understand all the mechanisms in play, but the hang seemed to come and go with random factors. It's not terribly surprising that the hang is gone after this patch since the line of code that was failing is no longer present in the kernel. Fixes: a670ff578f1f ("drm/msm/dpu: always use mdp device to scale bandwidth") Fixes: c33b7c0389e1 ("drm/msm/dpu: add support for clk and bw scaling for display") Signed-off-by: Douglas Anderson Reviewed-by: Abhinav Kumar Tested-by: Jessica Zhang # RB3 (sdm845) and Reviewed-by: Stephen Boyd Reviewed-by: Dmitry Baryshkov Patchwork: https://patchwork.freedesktop.org/patch/487884/ Link: https://lore.kernel.org/r/20220531160059.v2.1.Ie7f6d4bf8cce28131da31a43354727e417cae98d@changeid Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 ----- drivers/gpu/drm/msm/msm_mdss.c | 57 +++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index bce47647d891..e23e2552e802 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -49,8 +49,6 @@ #define DPU_DEBUGFS_DIR "msm_dpu" #define DPU_DEBUGFS_HWMASKNAME "hw_log_mask" -#define MIN_IB_BW 400000000ULL /* Min ib vote 400MB */ - static int dpu_kms_hw_init(struct msm_kms *kms); static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms); @@ -1305,15 +1303,9 @@ static int __maybe_unused dpu_runtime_resume(struct device *dev) struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms); struct drm_encoder *encoder; struct drm_device *ddev; - int i; ddev = dpu_kms->dev; - WARN_ON(!(dpu_kms->num_paths)); - /* Min vote of BW is required before turning on AXI clk */ - for (i = 0; i < dpu_kms->num_paths; i++) - icc_set_bw(dpu_kms->path[i], 0, Bps_to_icc(MIN_IB_BW)); - rc = clk_bulk_prepare_enable(dpu_kms->num_clocks, dpu_kms->clocks); if (rc) { DPU_ERROR("clock enable failed rc:%d\n", rc); diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c index 0454a571adf7..e13c5c12b775 100644 --- a/drivers/gpu/drm/msm/msm_mdss.c +++ b/drivers/gpu/drm/msm/msm_mdss.c @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -25,6 +26,8 @@ #define UBWC_CTRL_2 0x150 #define UBWC_PREDICTION_MODE 0x154 +#define MIN_IB_BW 400000000UL /* Min ib vote 400MB */ + struct msm_mdss { struct device *dev; @@ -36,8 +39,47 @@ struct msm_mdss { unsigned long enabled_mask; struct irq_domain *domain; } irq_controller; + struct icc_path *path[2]; + u32 num_paths; }; +static int msm_mdss_parse_data_bus_icc_path(struct device *dev, + struct msm_mdss *msm_mdss) +{ + struct icc_path *path0 = of_icc_get(dev, "mdp0-mem"); + struct icc_path *path1 = of_icc_get(dev, "mdp1-mem"); + + if (IS_ERR_OR_NULL(path0)) + return PTR_ERR_OR_ZERO(path0); + + msm_mdss->path[0] = path0; + msm_mdss->num_paths = 1; + + if (!IS_ERR_OR_NULL(path1)) { + msm_mdss->path[1] = path1; + msm_mdss->num_paths++; + } + + return 0; +} + +static void msm_mdss_put_icc_path(void *data) +{ + struct msm_mdss *msm_mdss = data; + int i; + + for (i = 0; i < msm_mdss->num_paths; i++) + icc_put(msm_mdss->path[i]); +} + +static void msm_mdss_icc_request_bw(struct msm_mdss *msm_mdss, unsigned long bw) +{ + int i; + + for (i = 0; i < msm_mdss->num_paths; i++) + icc_set_bw(msm_mdss->path[i], 0, Bps_to_icc(bw)); +} + static void msm_mdss_irq(struct irq_desc *desc) { struct msm_mdss *msm_mdss = irq_desc_get_handler_data(desc); @@ -136,6 +178,13 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss) { int ret; + /* + * Several components have AXI clocks that can only be turned on if + * the interconnect is enabled (non-zero bandwidth). Let's make sure + * that the interconnects are at least at a minimum amount. + */ + msm_mdss_icc_request_bw(msm_mdss, MIN_IB_BW); + ret = clk_bulk_prepare_enable(msm_mdss->num_clocks, msm_mdss->clocks); if (ret) { dev_err(msm_mdss->dev, "clock enable failed, ret:%d\n", ret); @@ -178,6 +227,7 @@ static int msm_mdss_enable(struct msm_mdss *msm_mdss) static int msm_mdss_disable(struct msm_mdss *msm_mdss) { clk_bulk_disable_unprepare(msm_mdss->num_clocks, msm_mdss->clocks); + msm_mdss_icc_request_bw(msm_mdss, 0); return 0; } @@ -271,6 +321,13 @@ static struct msm_mdss *msm_mdss_init(struct platform_device *pdev, bool is_mdp5 dev_dbg(&pdev->dev, "mapped mdss address space @%pK\n", msm_mdss->mmio); + ret = msm_mdss_parse_data_bus_icc_path(&pdev->dev, msm_mdss); + if (ret) + return ERR_PTR(ret); + ret = devm_add_action_or_reset(&pdev->dev, msm_mdss_put_icc_path, msm_mdss); + if (ret) + return ERR_PTR(ret); + if (is_mdp5) ret = mdp5_mdss_parse_clock(pdev, &msm_mdss->clocks); else -- cgit v1.2.3