Hi Sascha,
On 26.09.24 15:15, Sascha Hauer wrote:
> This adds support for the Rockchip VOP2 video core found on the Rockchip
> SoCs RK3566, RK3568 and RK3588. The code is based on the Linux driver
> and has been heavily stripped down for barebox. Support for the cluster
> windows has been removed, also support for YUV modes has been dropped.
>
> Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
With below described adjustments:
Tested-by: Ahmad Fatoum <a.fat...@pengutronix.de> # rk3566 HDMI
> +config DRIVER_VIDEO_ROCKCHIP_HDMI
> + bool "Rockchip HDMI driver"
> + select VIDEO_VPL
> + select OFTREE
> + depends on DRIVER_VIDEO_EDID
> + depends on DRIVER_VIDEO_DW_HDMI
Turn these two into select? Makes it easier to enable.
> +static int dw_hdmi_rockchip_probe(struct device *dev)
> +{
> + struct dw_hdmi_plat_data *plat_data;
> + struct rockchip_hdmi *hdmi;
> + int ret;
> +
> + hdmi = xzalloc(sizeof(*hdmi));
> +
> + ret = dev_get_drvdata(dev, (const void **)&plat_data);
> + if (ret)
> + return ret;
We should really phase out dev_get_drvdata. For one, it doesn't
do the same thing as in Linux and those casts are ugly.
Can you use device_get_match_data instead?
> + ret = rockchip_hdmi_parse_dt(hdmi);
> + if (ret) {
> + if (ret != -EPROBE_DEFER)
> + dev_err(hdmi->dev, "Unable to parse OF data\n");
dev_err_probe here and below would be nice, but I assume it's that way in Linux
too?
> +static struct vop2_win *vop2_find_unused_win(struct vop2 *vop2)
> +{
> + int i;
> +
> + for (i = 0; i < vop2->registered_num_wins; i++) {
> + struct vop2_win *win = &vop2->win[i];
I think the logic here is wrong. In a below hunk, we already have the same loop
and now we set i = 0 and restart. That means that the same win can be used
twice.
win has struct fb_info as element though, so in that case, the same framebuffer
is registered twice, which leads to an error message on my rk3566.
You may want to turn this into a:
static struct vop2_win *vop2_next_unused_win(struct vop2 *vop2, int *i)
and then start from i. The i++ in the main loop will take care to advance i,
so no win is reused again.
> + nvp = 0;
> + for (i = 0; i < vop2->registered_num_wins; i++) {
> + struct vop2_win *win = &vop2->win[i];
> +
> + if (vop2->data->soc_id == 3566) {
> + /*
> + * On RK3566 these windows don't have an independent
> + * framebuffer. They share the framebuffer with smart0,
> + * esmart0 and cluster0 respectively.
> + */
> + switch (win->data->phys_id) {
> + case ROCKCHIP_VOP2_SMART1:
> + case ROCKCHIP_VOP2_ESMART1:
> + case ROCKCHIP_VOP2_CLUSTER1:
> + continue;
> + }
I don't get any overlay framebuffers registered on rk3566. You know what might
be the reason?
> + for (i = 0; i < vop2->registered_num_wins; i++) {
> + struct vop2_win *win = &vop2->win[i];
> + struct vop2_video_port *vp = win->vp;
> + int j;
> +
> + if (win->type != DRM_PLANE_TYPE_PRIMARY)
> + continue;
On rk3566, you may end up with vp == NULL here, so you probably
want to add a `|| !vp` into the above if condition.
> +
> + ret = vop2_register_plane(vp, win);
> + if (ret)
> + continue;
> +
> + for (j = 0; j < overlay_per_vp; j++) {
> + win = vop2_find_unused_win(vop2);
> + if (!win)
> + break;
> +
> + win->vp = vp;
> +
> + ret = vop2_register_plane(vp, win);
> + if (ret)
> + return ret;
> + }
> + vp->nlayers = j + 1;
> + }
> +
> + return 0;
> +}
> +
> +struct driver vop2_driver = {
> + .probe = vop2_bind,
> + .name = "rockchip-vop2",
> + .of_compatible = vop2_dt_match,
> +};
> +late_platform_driver(vop2_driver);
Does this have to be a late driver? Can't we let deep probe take
care of the dependency handling?
> diff --git a/drivers/video/rockchip/rockchip_vop_reg.c
> b/drivers/video/rockchip/rockchip_vop_reg.c
This file is unused.
Cheers,
Ahmad
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
