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/[email protected]/
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
From: Heiko Stuebner <[email protected]>
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 <[email protected]>
---
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
From: Heiko Stuebner <[email protected]>
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 <[email protected]>
---
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
Hi Heiko,
On 4/25/24 9:55 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks!
Quentin
Hi Heiko,
On 4/25/24 9:55 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
Thanks!
Quentin
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 <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
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