From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Kalyan Thota <kalyan_t@codeaurora.org> Cc: Rob Clark <robdclark@gmail.com>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Krishna Manikandan <mkrishn@codeaurora.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Daniel Hung-yu Wu <hywu@google.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Douglas Anderson <dianders@chromium.org>, Matthias Kaehlcke <mka@google.com>, Michelle Dean <midean@google.com>, Steev Klimaszewski <steev@kali.org>, freedreno <freedreno@lists.freedesktop.org>, y@qualcomm.com Subject: Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume Date: Thu, 1 Apr 2021 16:31:14 +0300 [thread overview] Message-ID: <CAA8EJpouKeor8J2QG3nUtLqyNTwd1J44BXDjk4fHCYhykeJ7Hw@mail.gmail.com> (raw) In-Reply-To: <96eb927abe1a22711709900cec7f8d11@codeaurora.org> On Thu, 1 Apr 2021 at 16:19, <kalyan_t@codeaurora.org> wrote: > > On 2021-04-01 07:37, Dmitry Baryshkov wrote: > > On 01/04/2021 01:47, Rob Clark wrote: > >> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov > >> <dmitry.baryshkov@linaro.org> wrote: > >>> > >>> On 31/03/2021 14:27, Kalyan Thota wrote: > >>>> WARN_ON was introduced by the below commit to catch runtime resumes > >>>> that are getting triggered before icc path was set. > >>>> > >>>> "drm/msm/disp/dpu1: icc path needs to be set before dpu runtime > >>>> resume" > >>>> > >>>> For the targets where the bw scaling is not enabled, this WARN_ON is > >>>> a false alarm. Fix the WARN condition appropriately. > >>> > >>> Should we change all DPU targets to use bw scaling to the mdp from > >>> the > >>> mdss nodes? The limitation to sc7180 looks artificial. > >> > >> yes, we should, this keeps biting us on 845 > > > > Done, > > https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/ > > Hi Dmitry, > > https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/ > > you need to add clk_inefficiency_factor, bw_inefficiency_factor in the > catalogue for the new > targets where bw scaling is being enabled. please reuse sc7180 values. Done in patch 1 in that series. > > secondly, the AXI clock needs to be moved from mdss to mdp device like > as in sc7180 dt if its not done already. Is this enough: sm8250 has <&gcc GCC_DISP_HF_AXI_CLK> both in mdss and mdp nodes sdm845 has <&gcc GCC_DISP_AXI_CLK> in mdss node and <&dispcc DISP_CC_MDSS_AXI_CLK> in the mdp node. > > lastly, if you are planning to remove the static votes from dpu_mdss, do > you also want to move the > interconnect paths from mdss device to mdp device in the dt ? I have no strong opinion on this. So far I did not change dt to be compatible with the current device trees. > > > Thanks, > Kalyan > > > > >> > >>>> > >>>> Reported-by: Steev Klimaszewski <steev@kali.org> > >> > >> Please add Fixes: tag as well > Adding Fixes tag above my sign-off, should i push another version or can > it be picked from here ? > > Fixes: Id252b9c2887 ("drm/msm/disp/dpu1: icc path needs to be set before > dpu runtime resume") > >> > >>>> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org> > >>>> --- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 +++++--- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 9 +++++++++ > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++----- > >>>> 3 files changed, 20 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 cab387f..0071a4d 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> @@ -294,6 +294,9 @@ static int > >>>> dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms) > >>>> struct icc_path *path1; > >>>> struct drm_device *dev = dpu_kms->dev; > >>>> > >>>> + if (!dpu_supports_bw_scaling(dev)) > >>>> + return 0; > >>>> + > >>>> path0 = of_icc_get(dev->dev, "mdp0-mem"); > >>>> path1 = of_icc_get(dev->dev, "mdp1-mem"); > >>>> > >>>> @@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > >>>> DPU_DEBUG("REG_DMA is not defined"); > >>>> } > >>>> > >>>> - if (of_device_is_compatible(dev->dev->of_node, > >>>> "qcom,sc7180-mdss")) > >>>> - dpu_kms_parse_data_bus_icc_path(dpu_kms); > >>>> + dpu_kms_parse_data_bus_icc_path(dpu_kms); > >>>> > >>>> pm_runtime_get_sync(&dpu_kms->pdev->dev); > >>>> > >>>> @@ -1198,7 +1200,7 @@ static int __maybe_unused > >>>> dpu_runtime_resume(struct device *dev) > >>>> > >>>> ddev = dpu_kms->dev; > >>>> > >>>> - WARN_ON(!(dpu_kms->num_paths)); > >>>> + WARN_ON((dpu_supports_bw_scaling(ddev) && > >>>> !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)); > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> index d6717d6..f7bcc0a 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> @@ -154,6 +154,15 @@ struct vsync_info { > >>>> > >>>> #define to_dpu_global_state(x) container_of(x, struct > >>>> dpu_global_state, base) > >>>> > >>>> +/** > >>>> + * dpu_supports_bw_scaling: returns true for drivers that support > >>>> bw scaling. > >>>> + * @dev: Pointer to drm_device structure > >>>> + */ > >>>> +static inline int dpu_supports_bw_scaling(struct drm_device *dev) > >>>> +{ > >>>> + return of_device_is_compatible(dev->dev->of_node, > >>>> "qcom,sc7180-mdss"); > >>>> +} > >>>> + > >>>> /* Global private object state for tracking resources that are > >>>> shared across > >>>> * multiple kms objects (planes/crtcs/etc). > >>>> */ > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> index cd40788..8cd712c 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> @@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct > >>>> drm_device *dev, > >>>> struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem"); > >>>> struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem"); > >>>> > >>>> + if (dpu_supports_bw_scaling(dev)) > >>>> + return 0; > >>>> + > >>>> if (IS_ERR_OR_NULL(path0)) > >>>> return PTR_ERR_OR_ZERO(path0); > >>>> > >>>> @@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev) > >>>> > >>>> DRM_DEBUG("mapped mdss address space @%pK\n", > >>>> dpu_mdss->mmio); > >>>> > >>>> - if (!of_device_is_compatible(dev->dev->of_node, > >>>> "qcom,sc7180-mdss")) { > >>>> - ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss); > >>>> - if (ret) > >>>> - return ret; > >>>> - } > >>>> + ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss); > >>>> + if (ret) > >>>> + return ret; > >>>> > >>>> mp = &dpu_mdss->mp; > >>>> ret = msm_dss_parse_clock(pdev, mp); > >>>> > >>> > >>> > >>> -- > >>> With best wishes > >>> Dmitry -- With best wishes Dmitry
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Kalyan Thota <kalyan_t@codeaurora.org> Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Krishna Manikandan <mkrishn@codeaurora.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Daniel Hung-yu Wu <hywu@google.com>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Douglas Anderson <dianders@chromium.org>, Matthias Kaehlcke <mka@google.com>, Michelle Dean <midean@google.com>, Steev Klimaszewski <steev@kali.org>, freedreno <freedreno@lists.freedesktop.org>, y@qualcomm.com Subject: Re: [Freedreno] [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume Date: Thu, 1 Apr 2021 16:31:14 +0300 [thread overview] Message-ID: <CAA8EJpouKeor8J2QG3nUtLqyNTwd1J44BXDjk4fHCYhykeJ7Hw@mail.gmail.com> (raw) In-Reply-To: <96eb927abe1a22711709900cec7f8d11@codeaurora.org> On Thu, 1 Apr 2021 at 16:19, <kalyan_t@codeaurora.org> wrote: > > On 2021-04-01 07:37, Dmitry Baryshkov wrote: > > On 01/04/2021 01:47, Rob Clark wrote: > >> On Wed, Mar 31, 2021 at 9:03 AM Dmitry Baryshkov > >> <dmitry.baryshkov@linaro.org> wrote: > >>> > >>> On 31/03/2021 14:27, Kalyan Thota wrote: > >>>> WARN_ON was introduced by the below commit to catch runtime resumes > >>>> that are getting triggered before icc path was set. > >>>> > >>>> "drm/msm/disp/dpu1: icc path needs to be set before dpu runtime > >>>> resume" > >>>> > >>>> For the targets where the bw scaling is not enabled, this WARN_ON is > >>>> a false alarm. Fix the WARN condition appropriately. > >>> > >>> Should we change all DPU targets to use bw scaling to the mdp from > >>> the > >>> mdss nodes? The limitation to sc7180 looks artificial. > >> > >> yes, we should, this keeps biting us on 845 > > > > Done, > > https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/ > > Hi Dmitry, > > https://lore.kernel.org/linux-arm-msm/20210401020533.3956787-2-dmitry.baryshkov@linaro.org/ > > you need to add clk_inefficiency_factor, bw_inefficiency_factor in the > catalogue for the new > targets where bw scaling is being enabled. please reuse sc7180 values. Done in patch 1 in that series. > > secondly, the AXI clock needs to be moved from mdss to mdp device like > as in sc7180 dt if its not done already. Is this enough: sm8250 has <&gcc GCC_DISP_HF_AXI_CLK> both in mdss and mdp nodes sdm845 has <&gcc GCC_DISP_AXI_CLK> in mdss node and <&dispcc DISP_CC_MDSS_AXI_CLK> in the mdp node. > > lastly, if you are planning to remove the static votes from dpu_mdss, do > you also want to move the > interconnect paths from mdss device to mdp device in the dt ? I have no strong opinion on this. So far I did not change dt to be compatible with the current device trees. > > > Thanks, > Kalyan > > > > >> > >>>> > >>>> Reported-by: Steev Klimaszewski <steev@kali.org> > >> > >> Please add Fixes: tag as well > Adding Fixes tag above my sign-off, should i push another version or can > it be picked from here ? > > Fixes: Id252b9c2887 ("drm/msm/disp/dpu1: icc path needs to be set before > dpu runtime resume") > >> > >>>> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org> > >>>> --- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 8 +++++--- > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 9 +++++++++ > >>>> drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 11 ++++++----- > >>>> 3 files changed, 20 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 cab387f..0071a4d 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > >>>> @@ -294,6 +294,9 @@ static int > >>>> dpu_kms_parse_data_bus_icc_path(struct dpu_kms *dpu_kms) > >>>> struct icc_path *path1; > >>>> struct drm_device *dev = dpu_kms->dev; > >>>> > >>>> + if (!dpu_supports_bw_scaling(dev)) > >>>> + return 0; > >>>> + > >>>> path0 = of_icc_get(dev->dev, "mdp0-mem"); > >>>> path1 = of_icc_get(dev->dev, "mdp1-mem"); > >>>> > >>>> @@ -934,8 +937,7 @@ static int dpu_kms_hw_init(struct msm_kms *kms) > >>>> DPU_DEBUG("REG_DMA is not defined"); > >>>> } > >>>> > >>>> - if (of_device_is_compatible(dev->dev->of_node, > >>>> "qcom,sc7180-mdss")) > >>>> - dpu_kms_parse_data_bus_icc_path(dpu_kms); > >>>> + dpu_kms_parse_data_bus_icc_path(dpu_kms); > >>>> > >>>> pm_runtime_get_sync(&dpu_kms->pdev->dev); > >>>> > >>>> @@ -1198,7 +1200,7 @@ static int __maybe_unused > >>>> dpu_runtime_resume(struct device *dev) > >>>> > >>>> ddev = dpu_kms->dev; > >>>> > >>>> - WARN_ON(!(dpu_kms->num_paths)); > >>>> + WARN_ON((dpu_supports_bw_scaling(ddev) && > >>>> !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)); > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> index d6717d6..f7bcc0a 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > >>>> @@ -154,6 +154,15 @@ struct vsync_info { > >>>> > >>>> #define to_dpu_global_state(x) container_of(x, struct > >>>> dpu_global_state, base) > >>>> > >>>> +/** > >>>> + * dpu_supports_bw_scaling: returns true for drivers that support > >>>> bw scaling. > >>>> + * @dev: Pointer to drm_device structure > >>>> + */ > >>>> +static inline int dpu_supports_bw_scaling(struct drm_device *dev) > >>>> +{ > >>>> + return of_device_is_compatible(dev->dev->of_node, > >>>> "qcom,sc7180-mdss"); > >>>> +} > >>>> + > >>>> /* Global private object state for tracking resources that are > >>>> shared across > >>>> * multiple kms objects (planes/crtcs/etc). > >>>> */ > >>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> index cd40788..8cd712c 100644 > >>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c > >>>> @@ -41,6 +41,9 @@ static int dpu_mdss_parse_data_bus_icc_path(struct > >>>> drm_device *dev, > >>>> struct icc_path *path0 = of_icc_get(dev->dev, "mdp0-mem"); > >>>> struct icc_path *path1 = of_icc_get(dev->dev, "mdp1-mem"); > >>>> > >>>> + if (dpu_supports_bw_scaling(dev)) > >>>> + return 0; > >>>> + > >>>> if (IS_ERR_OR_NULL(path0)) > >>>> return PTR_ERR_OR_ZERO(path0); > >>>> > >>>> @@ -276,11 +279,9 @@ int dpu_mdss_init(struct drm_device *dev) > >>>> > >>>> DRM_DEBUG("mapped mdss address space @%pK\n", > >>>> dpu_mdss->mmio); > >>>> > >>>> - if (!of_device_is_compatible(dev->dev->of_node, > >>>> "qcom,sc7180-mdss")) { > >>>> - ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss); > >>>> - if (ret) > >>>> - return ret; > >>>> - } > >>>> + ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss); > >>>> + if (ret) > >>>> + return ret; > >>>> > >>>> mp = &dpu_mdss->mp; > >>>> ret = msm_dss_parse_clock(pdev, mp); > >>>> > >>> > >>> > >>> -- > >>> With best wishes > >>> Dmitry -- With best wishes Dmitry _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-04-01 17:42 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-31 11:27 [v1] drm/msm/disp/dpu1: fix warn stack reported during dpu resume Kalyan Thota 2021-03-31 11:27 ` Kalyan Thota 2021-03-31 15:55 ` Doug Anderson 2021-03-31 15:55 ` Doug Anderson 2021-03-31 16:03 ` Dmitry Baryshkov 2021-03-31 16:03 ` Dmitry Baryshkov 2021-03-31 22:47 ` Rob Clark 2021-03-31 22:47 ` Rob Clark 2021-04-01 2:07 ` Dmitry Baryshkov 2021-04-01 2:07 ` Dmitry Baryshkov 2021-04-01 13:18 ` [Freedreno] " kalyan_t 2021-04-01 13:18 ` kalyan_t 2021-04-01 13:31 ` Dmitry Baryshkov [this message] 2021-04-01 13:31 ` Dmitry Baryshkov 2021-04-02 11:42 ` kalyan_t 2021-04-02 11:42 ` kalyan_t
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAA8EJpouKeor8J2QG3nUtLqyNTwd1J44BXDjk4fHCYhykeJ7Hw@mail.gmail.com \ --to=dmitry.baryshkov@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=dianders@chromium.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=hywu@google.com \ --cc=kalyan_t@codeaurora.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=midean@google.com \ --cc=mka@google.com \ --cc=mkrishn@codeaurora.org \ --cc=robdclark@gmail.com \ --cc=steev@kali.org \ --cc=y@qualcomm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.