[PATCH v2 3/5] drm/rockchip: vop2: Improve display modes handling on RK3588 HDMI0

[Maxime Ripard]

On Wed, Dec 11, 2024 at 07:01:15PM +0100, Heiko Stübner wrote:
> Am Mittwoch, 11. Dezember 2024, 18:47:44 CET schrieb Maxime Ripard:
> > On Wed, Dec 11, 2024 at 06:23:03PM +0100, Heiko Stübner wrote:
> > > Am Mittwoch, 11. Dezember 2024, 18:07:57 CET schrieb Maxime Ripard:
> > > > On Wed, Dec 11, 2024 at 12:15:07PM +0200, Cristian Ciocaltea wrote:
> > > > > The RK3588 specific implementation is currently quite limited in terms
> > > > > of handling the full range of display modes supported by the connected
> > > > > screens, e.g. 2560x1440 at 75Hz, 2048x1152 at 60Hz, 1024x768 at 60Hz are just a
> > > > > few of them.
> > > > > 
> > > > > Additionally, it doesn't cope well with non-integer refresh rates like
> > > > > 59.94, 29.97, 23.98, etc.
> > > > > 
> > > > > Make use of HDMI0 PHY PLL as a more accurate DCLK source to handle
> > > > > all display modes up to 4K at 60Hz.
> > > > > 
> > > > > Tested-by: FUKAUMI Naoki <naoki at radxa.com>
> > > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea at collabora.com>
> > > > > ---
> > > > >  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 34 ++++++++++++++++++++++++++++
> > > > >  1 file changed, 34 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > > index 8b2f53ffefdbf1cc8737b3a86e630a03a7fd9348..393fe6aa170aaee9663c4a6d98c1cd6a5ef79392 100644
> > > > > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > > > > @@ -158,6 +158,7 @@ struct vop2_video_port {
> > > > >  	struct drm_crtc crtc;
> > > > >  	struct vop2 *vop2;
> > > > >  	struct clk *dclk;
> > > > > +	struct clk *dclk_src;
> > > > >  	unsigned int id;
> > > > >  	const struct vop2_video_port_data *data;
> > > > >  
> > > > > @@ -212,6 +213,7 @@ struct vop2 {
> > > > >  	struct clk *hclk;
> > > > >  	struct clk *aclk;
> > > > >  	struct clk *pclk;
> > > > > +	struct clk *pll_hdmiphy0;
> > > > >  
> > > > >  	/* optional internal rgb encoder */
> > > > >  	struct rockchip_rgb *rgb;
> > > > > @@ -220,6 +222,8 @@ struct vop2 {
> > > > >  	struct vop2_win win[];
> > > > >  };
> > > > >  
> > > > > +#define VOP2_MAX_DCLK_RATE		600000 /* kHz */
> > > > > +
> > > > >  #define vop2_output_if_is_hdmi(x)	((x) == ROCKCHIP_VOP2_EP_HDMI0 || \
> > > > >  					 (x) == ROCKCHIP_VOP2_EP_HDMI1)
> > > > >  
> > > > > @@ -1033,6 +1037,9 @@ static void vop2_crtc_atomic_disable(struct drm_crtc *crtc,
> > > > >  
> > > > >  	vop2_crtc_disable_irq(vp, VP_INT_DSP_HOLD_VALID);
> > > > >  
> > > > > +	if (vp->dclk_src)
> > > > > +		clk_set_parent(vp->dclk, vp->dclk_src);
> > > > > +
> > > > >  	clk_disable_unprepare(vp->dclk);
> > > > >  
> > > > >  	vop2->enable_count--;
> > > > > @@ -2049,6 +2056,27 @@ static void vop2_crtc_atomic_enable(struct drm_crtc *crtc,
> > > > >  
> > > > >  	vop2_vp_write(vp, RK3568_VP_MIPI_CTRL, 0);
> > > > >  
> > > > > +	/*
> > > > > +	 * 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 && mode->crtc_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 (rkencoder->crtc_endpoint_id == ROCKCHIP_VOP2_EP_HDMI0) {
> > > > > +				if (!vp->dclk_src)
> > > > > +					vp->dclk_src = clk_get_parent(vp->dclk);
> > > > > +
> > > > > +				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;
> > > > > +			}
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > 
> > > > It seems pretty fragile to do it at atomic_enable time, even more so
> > > > since you don't lock the parent either.
> > > > 
> > > > Any reason not to do it in the DRM or clock driver probe, and make sure
> > > > you never change the parent somehow?
> > > 
> > > On rk3588 we have 3 dclk_s and 2 hdmi controllers. Each video-port can
> > > use the clock generated from either the hdmi0phy or hdmi1phy, depending
> > > on which hdmi-controller it uses.
> > > 
> > > So you actually need to know which vpX will output to which hdmiY to then
> > > reparent that dclk to the hdmiphy output.
> > 
> > The Rockchip nomenclature isn't super obvious to me, sorry. Is there a
> > datasheet for this somewhere? Also, does this vpX -> HDMI-Y mapping need
> > to be dynamic?
> 
> VPs are CRTCs in drm-language and each of them can drive a differing
> number of output encoders. Those video-ports also have differing output
> characteristics in terms of supported resolution and other properties.
> 
> The rk3588 TRM has leaked in a number of places, and if you find a
> TRM-part2, there is a section labeled "Display Output Interface Description"
> that has a nice graphic for that.
> 
> Or in short:
> - CRTC(VP)0 supports 8K resolution and can drive DP0+1, HDMI0+1, eDP0+1
>   [if I'm reading things correctly, 8K together with CRTC1 somehow)
> - CRTC(VP)1 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP0+1
> - CRTC(VP)2 supports 4K resolution and can drive DP0+1, HDMI0+1, eDP01, DSI0+1
> - CRTC(VP)3 supports 2K resolution and can drive DSI0+1 and some BT1120,BT656
> 
> so for the 3 higher resolution CRTCs there are essentially 6 or 8 output options
> depending on the board design

That's much clearer, thanks. I'm not entirely sure how that links to the
need for the PLL to change its parent depending on the ouput. Do you
need to always have all the outputs on the same PLL?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20241217/45cb2ca2/attachment.sig>