Hi Cristian,
On Mon, 25 Aug 2025 at 12:08, Cristian Ciocaltea
<cristian.ciocal...@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@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 <dani...@collabora.com>
Cheers,
Daniel
