[PATCH v2 1/5] drm/rockchip: vop2: Check bpc before switching DCLK source

[Daniel Stone]

Hi Cristian,

On Mon, 25 Aug 2025 at 12:08, Cristian Ciocaltea
<cristian.ciocaltea at collabora.com> wrote:
> When making use of the HDMI PHY PLL as a VOP2 DCLK source, it's output
> rate does normally match the mode clock.  But this is only applicable
> for default color depth of 8 bpc.  For higher depths, the output clock
> is further divided by the hardware according to the formula:
>
>   output rate = PHY PLL rate * 8 / bpc

Observing that this results in phy_pll_rate * 8 / 8 == phy_pll_rate
for 8bpc, the formula does actually hold true everywhere.

> @@ -1737,36 +1737,48 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
>          * Switch to HDMI PHY PLL as DCLK source for display modes up
>          * to 4K at 60Hz, if available, otherwise keep using the system CRU.
>          */
> -       if ((vop2->pll_hdmiphy0 || vop2->pll_hdmiphy1) && clock <= VOP2_MAX_DCLK_RATE) {
> -               drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> -                       struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
> +       if (vop2->pll_hdmiphy0 || vop2->pll_hdmiphy1) {
> +               unsigned long max_dclk;
>
> -                       if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> -                               if (!vop2->pll_hdmiphy0)
> -                                       break;
> +               if (vcstate->output_bpc > 8)
> +                       max_dclk = DIV_ROUND_CLOSEST_ULL(VOP2_MAX_DCLK_RATE * 8,
> +                                                        vcstate->output_bpc);
> +               else
> +                       max_dclk = VOP2_MAX_DCLK_RATE;

... so this could just be do the mul+div unconditionally.

> +               if (clock <= max_dclk) {
> +                       drm_for_each_encoder_mask(encoder, crtc->dev, crtc_state->encoder_mask) {
> +                               struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
>
> -                               ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> -                               if (ret < 0)
> -                                       drm_warn(vop2->drm,
> -                                                "Could not switch to HDMI0 PHY PLL: %d\n", ret);
> -                               break;
> -                       }
> +                               if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> +                                       if (!vop2->pll_hdmiphy0)
> +                                               break;
> +
> +                                       if (!vp->dclk_src)
> +                                               vp->dclk_src = clk_get_parent(vp->dclk);
>
> -                       if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI1) {
> -                               if (!vop2->pll_hdmiphy1)
> +                                       ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy0);
> +                                       if (ret < 0)
> +                                               drm_warn(vop2->drm,
> +                                                        "Could not switch to HDMI0 PHY PLL: %d\n",
> +                                                        ret);
>                                         break;
> +                               }
>
> -                               if (!vp->dclk_src)
> -                                       vp->dclk_src = clk_get_parent(vp->dclk);
> +                               if (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI1) {
> +                                       if (!vop2->pll_hdmiphy1)
> +                                               break;
>
> -                               ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy1);
> -                               if (ret < 0)
> -                                       drm_warn(vop2->drm,
> -                                                "Could not switch to HDMI1 PHY PLL: %d\n", ret);
> -                               break;
> +                                       if (!vp->dclk_src)
> +                                               vp->dclk_src = clk_get_parent(vp->dclk);
> +
> +                                       ret = clk_set_parent(vp->dclk, vop2->pll_hdmiphy1);
> +                                       if (ret < 0)
> +                                               drm_warn(vop2->drm,
> +                                                        "Could not switch to HDMI1 PHY PLL: %d\n",
> +                                                        ret);
> +                                       break;
> +                               }

To be honest, this whole thing is a bit weird, and seems like it could
also be struct clk *new_dclk_parent = (rkencoder->crtc_endpoint_id ==
ROCKCHIP_VOP2_EP_HDMI0) ? vop2->pll_hdmiphy0 : vop2->pll_hdmiphy1? But
it's not your code, I know, and the rest of the clock handling is
pretty messy, so I think this is fine as is.

Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel