Re: [Freedreno] [PATCH v3] drm/msm/mdp5: stop overriding drvdata

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 27/10/2022 21:30, Abhinav Kumar wrote:
> On 10/24/2022 8:26 AM, Dmitry Baryshkov wrote:
> > The rest of the code expects that master's device drvdata is the
> > struct msm_drm_private instance. Do not override the mdp5's drvdata.
> >
> > Fixes: 6874f48bb8b0 ("drm/msm: make mdp5/dpu devices master components")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > Abhinav, Rob, please pick this for -fixes.
> >
> > This is an updated version of [1]. Fixed the read_mdp_hw_revision()
> > function. PM runtime isn't available at the moment, as priv->kms is not
> > set.
> >
> > [1] https://patchwork.freedesktop.org/patch/490326/?series=105392&rev=1
> >
> > Changes since v2:
> > - Removed the clause checking whether mdp5_enable() has failed (it can
> >    not fail, noted by Abhinav)
> >
> > Changes since v1:
> > - Expanded the patch to also handle the read_mdp_hw_revision() and also
> >    to move pm enablement to the place where the pm_runtime can actually
> >    be used.
> >
> > ---
> >   drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 32 +++++++++++++-----------
> >   1 file changed, 17 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c 
> > b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > index b0d21838a134..b46f983f2b46 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > @@ -203,7 +203,7 @@ static int mdp5_set_split_display(struct msm_kms 
> > *kms,
> >                                 slave_encoder);
> >   }
> > -static void mdp5_destroy(struct platform_device *pdev);
> > +static void mdp5_destroy(struct mdp5_kms *mdp5_kms);
> >   static void mdp5_kms_destroy(struct msm_kms *kms)
> >   {
> > @@ -223,7 +223,7 @@ static void mdp5_kms_destroy(struct msm_kms *kms)
> >       }
> >       mdp_kms_destroy(&mdp5_kms->base);
> > -    mdp5_destroy(mdp5_kms->pdev);
> > +    mdp5_destroy(mdp5_kms);
> >   }
> >   #ifdef CONFIG_DEBUG_FS
> > @@ -519,9 +519,10 @@ static void read_mdp_hw_revision(struct mdp5_kms 
> > *mdp5_kms,
> >       struct device *dev = &mdp5_kms->pdev->dev;
> >       u32 version;
> > -    pm_runtime_get_sync(dev);
> > +    /* Manually enable the MDP5, as pm runtime isn't usable yet. */
> > +    mdp5_enable(mdp5_kms);
> >       version = mdp5_read(mdp5_kms, REG_MDP5_HW_VERSION);
> > -    pm_runtime_put_sync(dev);
> > +    mdp5_disable(mdp5_kms);
>
> Please correct me if wrong here, if we bypass the pm to enable the 
> clocks explicitly here, are we still guaranteed about GDSC to be ON?

The gdsc is tied to the mdss device, not to mdp5. So the gdsc will be 
enabled, because for mdp5 to probe the mdss device also should be powered.

> >       *major = FIELD(version, MDP5_HW_VERSION_MAJOR);
> >       *minor = FIELD(version, MDP5_HW_VERSION_MINOR);
> > @@ -559,6 +560,8 @@ static int mdp5_kms_init(struct drm_device *dev)
> >       int irq, i, ret;
> >       ret = mdp5_init(to_platform_device(dev->dev), dev);
> > +    if (ret)
> > +        return ret;
> >       /* priv->kms would have been populated by the MDP5 driver */
> >       kms = priv->kms;
> > @@ -632,9 +635,8 @@ static int mdp5_kms_init(struct drm_device *dev)
> >       return ret;
> >   }
> > -static void mdp5_destroy(struct platform_device *pdev)
> > +static void mdp5_destroy(struct mdp5_kms *mdp5_kms)
> >   {
> > -    struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
> >       int i;
> >       if (mdp5_kms->ctlm)
> > @@ -648,7 +650,7 @@ static void mdp5_destroy(struct platform_device 
> > *pdev)
> >           kfree(mdp5_kms->intfs[i]);
> >       if (mdp5_kms->rpm_enabled)
> > -        pm_runtime_disable(&pdev->dev);
> > +        pm_runtime_disable(&mdp5_kms->pdev->dev);
> >       drm_atomic_private_obj_fini(&mdp5_kms->glob_state);
> >       drm_modeset_lock_fini(&mdp5_kms->glob_state_lock);
> > @@ -797,8 +799,6 @@ static int mdp5_init(struct platform_device *pdev, 
> > struct drm_device *dev)
> >           goto fail;
> >       }
> > -    platform_set_drvdata(pdev, mdp5_kms);
> > -
> >       spin_lock_init(&mdp5_kms->resource_lock);
> >       mdp5_kms->dev = dev;
> > @@ -839,9 +839,6 @@ static int mdp5_init(struct platform_device *pdev, 
> > struct drm_device *dev)
> >        */
> >       clk_set_rate(mdp5_kms->core_clk, 200000000);
> > -    pm_runtime_enable(&pdev->dev);
> > -    mdp5_kms->rpm_enabled = true;
> > -
> >       read_mdp_hw_revision(mdp5_kms, &major, &minor);
> >       mdp5_kms->cfg = mdp5_cfg_init(mdp5_kms, major, minor);
> > @@ -893,10 +890,13 @@ static int mdp5_init(struct platform_device 
> > *pdev, struct drm_device *dev)
> >       /* set uninit-ed kms */
> >       priv->kms = &mdp5_kms->base.base;
> > +    pm_runtime_enable(&pdev->dev);
> > +    mdp5_kms->rpm_enabled = true;
> > +
> >       return 0;
> >   fail:
> >       if (mdp5_kms)
> > -        mdp5_destroy(pdev);
> > +        mdp5_destroy(mdp5_kms);
> >       return ret;
> >   }
> > @@ -953,7 +953,8 @@ static int mdp5_dev_remove(struct platform_device 
> > *pdev)
> >   static __maybe_unused int mdp5_runtime_suspend(struct device *dev)
> >   {
> >       struct platform_device *pdev = to_platform_device(dev);
> > -    struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
> > +    struct msm_drm_private *priv = platform_get_drvdata(pdev);
> > +    struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> >       DBG("");
> > @@ -963,7 +964,8 @@ static __maybe_unused int 
> > mdp5_runtime_suspend(struct device *dev)
> >   static __maybe_unused int mdp5_runtime_resume(struct device *dev)
> >   {
> >       struct platform_device *pdev = to_platform_device(dev);
> > -    struct mdp5_kms *mdp5_kms = platform_get_drvdata(pdev);
> > +    struct msm_drm_private *priv = platform_get_drvdata(pdev);
> > +    struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(priv->kms));
> >       DBG("");

--
With best wishes
Dmitry