| Message ID | 20240425195506.2935955-3-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/rockchip: vop2: two fixes from working on DSI enablement | expand |
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
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
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)