Re: [PATCH 2/3] drm/vc4: hdmi: Move the HSM clock enable to runtime_pm

[Dave Stevenson]

Hi Maxime

On Tue, 25 May 2021 at 10:11, Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> In order to access the HDMI controller, we need to make sure the HSM
> clock is enabled. If we were to access it with the clock disabled, the
> CPU would completely hang, resulting in an hard crash.
>
> Since we have different code path that would require it, let's move that
> clock enable / disable to runtime_pm that will take care of the
> reference counting for us.

This does change the order of clk_set_rate vs clk_prepare_enable, so
the clock is already running during
vc4_hdmi_encoder_pre_crtc_configure when the pixel rate is known.
However the crtc and HDMI blocks won't be actively passing pixels at
that point, so I don't see an issue with changing the rate. The clock
manager block will sort out the rate change happily.

Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>

> Fixes: 4f6e3d66ac52 ("drm/vc4: Add runtime PM support to the HDMI encoder driver")
> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 41 +++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index f9de8632a28b..867009a471e1 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -632,7 +632,6 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct drm_encoder *encoder,
>                    HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
>
>         clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
> -       clk_disable_unprepare(vc4_hdmi->hsm_clock);
>         clk_disable_unprepare(vc4_hdmi->pixel_clock);
>
>         ret = pm_runtime_put(&vc4_hdmi->pdev->dev);
> @@ -943,13 +942,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>                 return;
>         }
>
> -       ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
> -       if (ret) {
> -               DRM_ERROR("Failed to turn on HSM clock: %d\n", ret);
> -               clk_disable_unprepare(vc4_hdmi->pixel_clock);
> -               return;
> -       }
> -
>         vc4_hdmi_cec_update_clk_div(vc4_hdmi);
>
>         if (pixel_rate > 297000000)
> @@ -962,7 +954,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>         ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock, bvb_rate);
>         if (ret) {
>                 DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
> -               clk_disable_unprepare(vc4_hdmi->hsm_clock);
>                 clk_disable_unprepare(vc4_hdmi->pixel_clock);
>                 return;
>         }
> @@ -970,7 +961,6 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
>         ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
>         if (ret) {
>                 DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
> -               clk_disable_unprepare(vc4_hdmi->hsm_clock);
>                 clk_disable_unprepare(vc4_hdmi->pixel_clock);
>                 return;
>         }
> @@ -2097,6 +2087,29 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM
> +static int vc4_hdmi_runtime_suspend(struct device *dev)
> +{
> +       struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +
> +       clk_disable_unprepare(vc4_hdmi->hsm_clock);
> +
> +       return 0;
> +}
> +
> +static int vc4_hdmi_runtime_resume(struct device *dev)
> +{
> +       struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
> +       int ret;
> +
> +       ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
> +       if (ret)
> +               return ret;
> +
> +       return 0;
> +}
> +#endif
> +
>  static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
>  {
>         const struct vc4_hdmi_variant *variant = of_device_get_match_data(dev);
> @@ -2353,11 +2366,19 @@ static const struct of_device_id vc4_hdmi_dt_match[] = {
>         {}
>  };
>
> +
> +static const struct dev_pm_ops vc4_hdmi_pm_ops = {
> +       SET_RUNTIME_PM_OPS(vc4_hdmi_runtime_suspend,
> +                          vc4_hdmi_runtime_resume,
> +                          NULL)
> +};
> +
>  struct platform_driver vc4_hdmi_driver = {
>         .probe = vc4_hdmi_dev_probe,
>         .remove = vc4_hdmi_dev_remove,
>         .driver = {
>                 .name = "vc4_hdmi",
>                 .of_match_table = vc4_hdmi_dt_match,
> +               .pm = &vc4_hdmi_pm_ops,
>         },
>  };
> --
> 2.31.1
>