2022-03-09 00:55:21

by Dmitry Baryshkov

[permalink] [raw]
Subject: Re: [PATCH v5 5/5] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table during probe

On Tue, 8 Mar 2022 at 19:55, Vinod Polimera <[email protected]> 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 <[email protected]>
> ---
> 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
>


--
With best wishes
Dmitry


2022-03-15 05:37:07

by Vinod Polimera

[permalink] [raw]
Subject: RE: [PATCH v5 5/5] drm/msm/disp/dpu1: set mdp clk to the maximum frequency in opp table during probe



> -----Original Message-----
> From: Dmitry Baryshkov <[email protected]>
> Sent: Tuesday, March 8, 2022 10:40 PM
> To: quic_vpolimer <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; quic_kalyant <[email protected]>
> 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 <[email protected]>
> 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 <[email protected]>
> > ---
> > 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