Re: [PATCH 13/13] video: Rockchip: Add VOP2 driver

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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.hauer@pengutronix.de>

With below described adjustments:

Tested-by: Ahmad Fatoum <a.fatoum@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 |