| Message ID | 1646758500-3776-6-git-send-email-quic_vpolimer@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update mdp clk to max supported value to support higher refresh rates | expand |
On Tue, 8 Mar 2022 at 19:55, Vinod Polimera <quic_vpolimer@quicinc.com> wrote: > > use max clock during probe/bind sequence from the opp table. > The clock will be scaled down when framework sends an update. > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index d550f90..d9922b9 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -1221,6 +1221,7 @@ static int dpu_bind(struct device *dev, struct device *master, void *data) > struct dpu_kms *dpu_kms; > struct dss_module_power *mp; > int ret = 0; > + unsigned long max_freq = ULONG_MAX; > > dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL); > if (!dpu_kms) > @@ -1243,6 +1244,8 @@ static int dpu_bind(struct device *dev, struct device *master, void *data) > return ret; > } > > + dev_pm_opp_find_freq_floor(dev, &max_freq); You leak a reference to the opp here. The function returns a value, which should be dev_pm_opp_put(). Moreover judging from the dev_pm_opp_set_rate() code I think you don't have to find an exact frequency, as it will call clk_round_rate()/_find_freq_ceil() anyway. Could you please check that it works? > + dev_pm_opp_set_rate(dev, max_freq); > platform_set_drvdata(pdev, dpu_kms); > > ret = msm_kms_init(&dpu_kms->base, &kms_funcs); > -- > 2.7.4 >
Hi, On Tue, Mar 8, 2022 at 8:55 AM Vinod Polimera <quic_vpolimer@quicinc.com> wrote: > > use max clock during probe/bind sequence from the opp table. > The clock will be scaled down when framework sends an update. > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++ > 1 file changed, 3 insertions(+) In addition to Dmitry's requests, can you also make this patch #1 in the series since the DTS stuff really ought to come _after_ this one. ...and actually, thinking about it further: 1. If we land this fix, we actually don't _need_ the dts patches, right? Sure, the clock rate will get assigned before probe but then we'll change it right away in probe, right? 2. If we land the dts patches _before_ the driver patch then it will be a regression, right? 3. The dts patches and driver patch will probably land through separate trees. The driver patch will go through the MSM DRM tree and the device tree patches through the Qualcomm armsoc tree, right? Assuming that the above is right, we should: 1. Put the driver patch first. 2. Remove the "Fixes" tag on the dts patches. I guess in theory we could have a FIxes tag on this patch? 3. Note in the dts patches commit messages that they depend on the driver patch. 4. Delay the dts patches until the driver change has made it to mainline. Does that sound reasonable?
> -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sent: Tuesday, March 8, 2022 10:40 PM > To: quic_vpolimer <quic_vpolimer@quicinc.com> > Cc: dri-devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; > freedreno@lists.freedesktop.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; robdclark@gmail.com; dianders@chromium.org; > swboyd@chromium.org; quic_kalyant <quic_kalyant@quicinc.com> > Subject: Re: [PATCH v5 5/5] drm/msm/disp/dpu1: set mdp clk to the > maximum frequency in opp table during probe > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > On Tue, 8 Mar 2022 at 19:55, Vinod Polimera <quic_vpolimer@quicinc.com> > wrote: > > > > use max clock during probe/bind sequence from the opp table. > > The clock will be scaled down when framework sends an update. > > > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > index d550f90..d9922b9 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > > @@ -1221,6 +1221,7 @@ static int dpu_bind(struct device *dev, struct > device *master, void *data) > > struct dpu_kms *dpu_kms; > > struct dss_module_power *mp; > > int ret = 0; > > + unsigned long max_freq = ULONG_MAX; > > > > dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), > GFP_KERNEL); > > if (!dpu_kms) > > @@ -1243,6 +1244,8 @@ static int dpu_bind(struct device *dev, struct > device *master, void *data) > > return ret; > > } > > > > + dev_pm_opp_find_freq_floor(dev, &max_freq); > > You leak a reference to the opp here. The function returns a value, > which should be dev_pm_opp_put(). > Moreover judging from the dev_pm_opp_set_rate() code I think you don't > have to find an exact frequency, as it will call > clk_round_rate()/_find_freq_ceil() anyway. > Could you please check that it works? clk_round_rate will get the max frequency in freq_table in clk driver and if that frequency is missing in opp table it will throw error. So, It will be optimal to get max freq in opp table and set it based on that freq. > > > + dev_pm_opp_set_rate(dev, max_freq); > > platform_set_drvdata(pdev, dpu_kms); > > > > ret = msm_kms_init(&dpu_kms->base, &kms_funcs); > > -- > > 2.7.4 > > > > > -- > With best wishes > Dmitry
> -----Original Message----- > From: Doug Anderson <dianders@chromium.org> > Sent: Thursday, March 10, 2022 12:55 AM > To: quic_vpolimer <quic_vpolimer@quicinc.com> > Cc: dri-devel <dri-devel@lists.freedesktop.org>; linux-arm-msm <linux-arm- > msm@vger.kernel.org>; freedreno <freedreno@lists.freedesktop.org>; > open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > <devicetree@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob > Clark <robdclark@gmail.com>; Stephen Boyd <swboyd@chromium.org>; > quic_kalyant <quic_kalyant@quicinc.com> > Subject: Re: [PATCH v5 5/5] drm/msm/disp/dpu1: set mdp clk to the > maximum frequency in opp table during probe > > WARNING: This email originated from outside of Qualcomm. Please be wary > of any links or attachments, and do not enable macros. > > Hi, > > On Tue, Mar 8, 2022 at 8:55 AM Vinod Polimera > <quic_vpolimer@quicinc.com> wrote: > > > > use max clock during probe/bind sequence from the opp table. > > The clock will be scaled down when framework sends an update. > > > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++ > > 1 file changed, 3 insertions(+) > > In addition to Dmitry's requests, can you also make this patch #1 in > the series since the DTS stuff really ought to come _after_ this one. > > ...and actually, thinking about it further: > > 1. If we land this fix, we actually don't _need_ the dts patches, > right? Sure, the clock rate will get assigned before probe but then > we'll change it right away in probe, right? > > 2. If we land the dts patches _before_ the driver patch then it will > be a regression, right? > > 3. The dts patches and driver patch will probably land through > separate trees. The driver patch will go through the MSM DRM tree and > the device tree patches through the Qualcomm armsoc tree, right? > > > Assuming that the above is right, we should: > > 1. Put the driver patch first. > Moved driver patch first. > 2. Remove the "Fixes" tag on the dts patches. I guess in theory we > could have a FIxes tag on this patch? > - Removed fixed tag on dt patches and added on driver patch. > 3. Note in the dts patches commit messages that they depend on the driver > patch. > - Added dependency patch on driver patch for dt patch. > 4. Delay the dts patches until the driver change has made it to mainline. > > Does that sound reasonable?
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index d550f90..d9922b9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -1221,6 +1221,7 @@ static int dpu_bind(struct device *dev, struct device *master, void *data) struct dpu_kms *dpu_kms; struct dss_module_power *mp; int ret = 0; + unsigned long max_freq = ULONG_MAX; dpu_kms = devm_kzalloc(&pdev->dev, sizeof(*dpu_kms), GFP_KERNEL); if (!dpu_kms) @@ -1243,6 +1244,8 @@ static int dpu_bind(struct device *dev, struct device *master, void *data) return ret; } + dev_pm_opp_find_freq_floor(dev, &max_freq); + dev_pm_opp_set_rate(dev, max_freq); platform_set_drvdata(pdev, dpu_kms); ret = msm_kms_init(&dpu_kms->base, &kms_funcs);
use max clock during probe/bind sequence from the opp table. The clock will be scaled down when framework sends an update. Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 3 +++ 1 file changed, 3 insertions(+)