[PATCH 0/2] drm/rockchip: vop2: two fixes from working on DSI enablement

[PATCH 0/2] drm/rockchip: vop2: two fixes from working on DSI enablement

[Heiko Stuebner]

While working on enabling DSI support on rk3588 I stumbled across the issue
of the display staying black while both the vop2 and dsi drivers were
claiming to be running just fine.

After a bit of checking I found that I got DSI output on VP3 when HDMI
on VP0 was at least associated in the DTS, but not without.

Andy's patch [0] helped and is definitly necessary, but did not fix the
problem completely. The missing thing was that VP3 is rk3588-specific
(rk3568 only has 3 video-ports, not 4 like rk3588) and the layers of
VP3 never got configured.

Patch2 fixes this.


[0] https://lore.kernel.org/dri-devel/20240422101905.32703-2-andyshrk@163.com/

Heiko Stuebner (2):
  drm/rockchip: vop2: fix rk3588 dp+dsi maxclk verification
  drm/rockchip: vop2: configure layers for vp3 on rk3588

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 16 ++++++++++++++--
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/2] drm/rockchip: vop2: fix rk3588 dp+dsi maxclk verification

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

The clock is in Hz while the value checked against is in kHz, so
actual frequencies will never be able to be below to max value.
Fix this by specifying the max-value in Hz too.

Fixes: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 9bee1fd88e6a2..523880a4e8e74 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -1719,7 +1719,7 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
 		else
 			dclk_out_rate = v_pixclk >> 2;
 
-		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000);
+		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000000);
 		if (!dclk_rate) {
 			drm_err(vop2->drm, "DP dclk_out_rate out of range, dclk_out_rate: %ld KHZ\n",
 				dclk_out_rate);
@@ -1736,7 +1736,7 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
 		 * dclk_rate = N * dclk_core_rate N = (1,2,4 ),
 		 * we get a little factor here
 		 */
-		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000);
+		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000000);
 		if (!dclk_rate) {
 			drm_err(vop2->drm, "MIPI dclk out of range, dclk_out_rate: %ld KHZ\n",
 				dclk_out_rate);
-- 
2.39.2

[PATCH 2/2] drm/rockchip: vop2: configure layers for vp3 on rk3588

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

The rk3588 VOP2 has 4 video-ports, yet the driver currently only
configures the first 3, as used on the rk3568.

Add another block to configure the vp3 as well, if applicable.

Fixes: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 12 ++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 523880a4e8e74..1a327a9ed7ee4 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2303,6 +2303,7 @@ static void vop2_setup_alpha(struct vop2_video_port *vp)
 static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 {
 	struct vop2 *vop2 = vp->vop2;
+	const struct vop2_data *vop2_data = vop2->data;
 	struct drm_plane *plane;
 	u32 layer_sel = 0;
 	u32 port_sel;
@@ -2344,6 +2345,17 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
 	else
 		port_sel |= FIELD_PREP(RK3568_OVL_PORT_SET__PORT2_MUX, 8);
 
+	/* configure vp3 */
+	if (vop2_data->soc_id == 3588) {
+		struct vop2_video_port *vp3 = &vop2->vps[3];
+
+		if (vp3->nlayers)
+			port_sel |= FIELD_PREP(RK3588_OVL_PORT_SET__PORT3_MUX,
+				(vp3->nlayers + vp2->nlayers + vp1->nlayers + vp0->nlayers - 1));
+		else
+			port_sel |= FIELD_PREP(RK3588_OVL_PORT_SET__PORT3_MUX, 8);
+	}
+
 	layer_sel = vop2_readl(vop2, RK3568_OVL_LAYER_SEL);
 
 	ofs = 0;
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
index 615a16196aff6..f46fb795414e1 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
@@ -489,6 +489,7 @@ enum dst_factor_mode {
 #define RK3588_OVL_PORT_SEL__CLUSTER2			GENMASK(21, 20)
 #define RK3568_OVL_PORT_SEL__CLUSTER1			GENMASK(19, 18)
 #define RK3568_OVL_PORT_SEL__CLUSTER0			GENMASK(17, 16)
+#define RK3588_OVL_PORT_SET__PORT3_MUX			GENMASK(15, 12)
 #define RK3568_OVL_PORT_SET__PORT2_MUX			GENMASK(11, 8)
 #define RK3568_OVL_PORT_SET__PORT1_MUX			GENMASK(7, 4)
 #define RK3568_OVL_PORT_SET__PORT0_MUX			GENMASK(3, 0)
-- 
2.39.2

Re: [PATCH 1/2] drm/rockchip: vop2: fix rk3588 dp+dsi maxclk verification

[Quentin Schulz]

Hi Heiko,

On 4/25/24 9:55 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The clock is in Hz while the value checked against is in kHz, so
> actual frequencies will never be able to be below to max value.
> Fix this by specifying the max-value in Hz too.
> 
> Fixes: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 9bee1fd88e6a2..523880a4e8e74 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1719,7 +1719,7 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>   		else
>   			dclk_out_rate = v_pixclk >> 2;
>   
> -		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000);
> +		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000000);
>   		if (!dclk_rate) {
>   			drm_err(vop2->drm, "DP dclk_out_rate out of range, dclk_out_rate: %ld KHZ\n",

It seems the error message is incorrect as well and should be saying Hz 
instead of KHz. (note also the lowercase z).

>   				dclk_out_rate);
> @@ -1736,7 +1736,7 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>   		 * dclk_rate = N * dclk_core_rate N = (1,2,4 ),
>   		 * we get a little factor here
>   		 */
> -		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000);
> +		dclk_rate = rk3588_calc_dclk(dclk_out_rate, 600000000);
>   		if (!dclk_rate) {
>   			drm_err(vop2->drm, "MIPI dclk out of range, dclk_out_rate: %ld KHZ\n",

Ditto.

Otherwise,

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 2/2] drm/rockchip: vop2: configure layers for vp3 on rk3588

[Quentin Schulz]

Hi Heiko,

On 4/25/24 9:55 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The rk3588 VOP2 has 4 video-ports, yet the driver currently only
> configures the first 3, as used on the rk3568.
> 

I'm wondering whether we should update the drawing at the top of the 
driver then?

> Add another block to configure the vp3 as well, if applicable.
> 
> Fixes: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 12 ++++++++++++
>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  1 +
>   2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 523880a4e8e74..1a327a9ed7ee4 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2303,6 +2303,7 @@ static void vop2_setup_alpha(struct vop2_video_port *vp)
>   static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>   {
>   	struct vop2 *vop2 = vp->vop2;
> +	const struct vop2_data *vop2_data = vop2->data;
>   	struct drm_plane *plane;
>   	u32 layer_sel = 0;
>   	u32 port_sel;
> @@ -2344,6 +2345,17 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
>   	else
>   		port_sel |= FIELD_PREP(RK3568_OVL_PORT_SET__PORT2_MUX, 8);
>   
> +	/* configure vp3 */
> +	if (vop2_data->soc_id == 3588) {

I think it'd be smarter to check against vop2->data->nr_vps >= 4; so 
that we don't need to maintain a list of SoCs that support a specific 
number of video ports.

> +		struct vop2_video_port *vp3 = &vop2->vps[3];

This is always possible because vps is statically allocated for 4 items, 
c.f. struct vop2_video_port vps[ROCKCHIP_MAX_CRTC]; so we don't 
necessarily need it in this specific location and can group it with the 
others. Cosmetic suggestion though.

Otherwise, the change itself makes sense to me, so:

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

Thanks!
Quentin

Re: [PATCH 2/2] drm/rockchip: vop2: configure layers for vp3 on rk3588

[Heiko Stübner]

Am Freitag, 3. Mai 2024, 14:57:03 CEST schrieb Quentin Schulz:
> Hi Heiko,
> 
> On 4/25/24 9:55 PM, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > The rk3588 VOP2 has 4 video-ports, yet the driver currently only
> > configures the first 3, as used on the rk3568.
> > 
> 
> I'm wondering whether we should update the drawing at the top of the 
> driver then?
> 
> > Add another block to configure the vp3 as well, if applicable.
> > 
> > Fixes: 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> > ---
> >   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 12 ++++++++++++
> >   drivers/gpu/drm/rockchip/rockchip_drm_vop2.h |  1 +
> >   2 files changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > index 523880a4e8e74..1a327a9ed7ee4 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> > @@ -2303,6 +2303,7 @@ static void vop2_setup_alpha(struct vop2_video_port *vp)
> >   static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
> >   {
> >   	struct vop2 *vop2 = vp->vop2;
> > +	const struct vop2_data *vop2_data = vop2->data;
> >   	struct drm_plane *plane;
> >   	u32 layer_sel = 0;
> >   	u32 port_sel;
> > @@ -2344,6 +2345,17 @@ static void vop2_setup_layer_mixer(struct vop2_video_port *vp)
> >   	else
> >   		port_sel |= FIELD_PREP(RK3568_OVL_PORT_SET__PORT2_MUX, 8);
> >   
> > +	/* configure vp3 */
> > +	if (vop2_data->soc_id == 3588) {
> 
> I think it'd be smarter to check against vop2->data->nr_vps >= 4; so 
> that we don't need to maintain a list of SoCs that support a specific 
> number of video ports.

probably ;-)

> 
> > +		struct vop2_video_port *vp3 = &vop2->vps[3];
> 
> This is always possible because vps is statically allocated for 4 items, 
> c.f. struct vop2_video_port vps[ROCKCHIP_MAX_CRTC]; so we don't 
> necessarily need it in this specific location and can group it with the 
> others. Cosmetic suggestion though.
> 
> Otherwise, the change itself makes sense to me, so:
> 
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

though comments from Andy from Rockchip in another thread suggest that
this is not necessary at all, as the "last" vp somehow has a hardware lock
to take the remaining layers or so.

And while tracking down dsi issues I had a "binary" state of
"gray display" without this patch and working DSI with it, in the last days
I haven't been able to reproduce this anymore.

So I guess I'll fix up the first patch according to your comment and keep
this change here for later.


Heiko