Re: [PATCH v7 3/8] drm: verisilicon: add a driver for Verisilicon display controllers

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello Icenowy Zheng,

On Thu Jan 29, 2026 at 3:39 AM CET, Icenowy Zheng wrote:
> From: Icenowy Zheng <uwu@icenowy.me>
>
> This is a from-scratch driver targeting Verisilicon DC-series display
> controllers, which feature self-identification functionality like their
> GC-series GPUs.
>
> Only DC8200 is being supported now, and only the main framebuffer is set
> up (as the DRM primary plane). Support for more DC models and more
> features is my further targets.
>
> As the display controller is delivered to SoC vendors as a whole part,
> this driver does not use component framework and extra bridges inside a
> SoC is expected to be implemented as dedicated bridges (this driver
> properly supports bridge chaining).
>
> Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
> Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
> Tested-by: Han Gao <gaohan@iscas.ac.cn>
> Tested-by: Michal Wilczynski <m.wilczynski@samsung.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

I have reviewed the bridge part of this patch and have a few remarks, see
below.

[...]

> +++ b/drivers/gpu/drm/verisilicon/vs_bridge.c
> @@ -0,0 +1,371 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 Icenowy Zheng <uwu@icenowy.me>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +
> +#include <uapi/linux/media-bus-format.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_simple_kms_helper.h>
> +
> +#include "vs_bridge.h"
> +#include "vs_bridge_regs.h"
> +#include "vs_crtc.h"
> +#include "vs_dc.h"
> +
> +static int vs_bridge_attach(struct drm_bridge *bridge,
> +			    struct drm_encoder *encoder,
> +			    enum drm_bridge_attach_flags flags)
> +{
> +	struct vs_bridge *vbridge = drm_bridge_to_vs_bridge(bridge);
> +
> +	return drm_bridge_attach(encoder, vbridge->next_bridge,
> +				 bridge, flags);
> +}
> +
> +struct vsdc_dp_format {
> +	u32 linux_fmt;
> +	bool is_yuv;
> +	u32 vsdc_fmt;
> +};

Moving the bool after the two 'u32's would be better for packing and
spatial locality (especially in case more fields are added in the future).

> +
> +static struct vsdc_dp_format vsdc_dp_supported_fmts[] = {
> +	/* default to RGB888 */
> +	{ MEDIA_BUS_FMT_FIXED, false, VSDC_DISP_DP_CONFIG_FMT_RGB888 },
> +	{ MEDIA_BUS_FMT_RGB888_1X24, false, VSDC_DISP_DP_CONFIG_FMT_RGB888 },
> +	{ MEDIA_BUS_FMT_RGB565_1X16, false, VSDC_DISP_DP_CONFIG_FMT_RGB565 },
> +	{ MEDIA_BUS_FMT_RGB666_1X18, false, VSDC_DISP_DP_CONFIG_FMT_RGB666 },
> +	{ MEDIA_BUS_FMT_RGB101010_1X30,
> +	  false, VSDC_DISP_DP_CONFIG_FMT_RGB101010 },

You can put up to 100 chars per line and avoid the newline here to make
this table more readable. Same below.

> +	{ MEDIA_BUS_FMT_UYVY8_1X16, true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYVY8 },
> +	{ MEDIA_BUS_FMT_UYVY10_1X20, true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYVY10 },
> +	{ MEDIA_BUS_FMT_YUV8_1X24, true, VSDC_DISP_DP_CONFIG_YUV_FMT_YUV8 },
> +	{ MEDIA_BUS_FMT_YUV10_1X30, true, VSDC_DISP_DP_CONFIG_YUV_FMT_YUV10 },
> +	{ MEDIA_BUS_FMT_UYYVYY8_0_5X24,
> +	  true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYYVYY8 },
> +	{ MEDIA_BUS_FMT_UYYVYY10_0_5X30,
> +	  true, VSDC_DISP_DP_CONFIG_YUV_FMT_UYYVYY10 },
> +};
> +

[...]

> +struct vs_bridge *vs_bridge_init(struct drm_device *drm_dev,
> +				 struct vs_crtc *crtc)
> +{
> +	unsigned int output = crtc->id;
> +	struct vs_bridge *bridge;

In common practice a variable named 'bridge' is used to point to a 'struct
drm_bridge', so it feels weird when it is used for another type. Can you
rename to 'vbridge' or 'vsbridge' or similar, to clarify it's the
"Verisilicon bridge"?

This is after all what you did in vs_bridge_attach() above, where the
ambiguity of the 'bridge' name used for a driver-specific struct is
evident.

> +	struct drm_bridge *next;
> +	enum vs_bridge_output_interface intf;
> +	const struct drm_bridge_funcs *bridge_funcs;
> +	int ret, enctype;
> +
> +	intf = vs_bridge_detect_output_interface(drm_dev->dev->of_node,
> +						 output);
> +	if (intf == -ENODEV) {
> +		drm_dbg(drm_dev, "Skipping output %u\n", output);
> +		return NULL;
> +	}
> +
> +	next = devm_drm_of_get_bridge(drm_dev->dev, drm_dev->dev->of_node,
> +				      output, intf);
> +	if (IS_ERR(next)) {
> +		ret = PTR_ERR(next);
> +		if (ret != -EPROBE_DEFER)
> +			drm_err(drm_dev,
> +				"Cannot get downstream bridge of output %u\n",
> +				output);

100 chars per line are allowed, so this could fit on a single line being
nicer to read. This applies to a lot places in this driver, of logging
calls in particular. I understand this would be annoying to change on an
already reviewed patch and at v7 so up to you, but it would be good to keep
it in mind for the future.

> +		return ERR_PTR(ret);
> +	}
> +
> +	if (intf == VSDC_OUTPUT_INTERFACE_DPI)
> +		bridge_funcs = &vs_dpi_bridge_funcs;
> +	else
> +		bridge_funcs = &vs_dp_bridge_funcs;
> +
> +	bridge = devm_drm_bridge_alloc(drm_dev->dev, struct vs_bridge, base,
> +				       bridge_funcs);

The 'struct drm_bridge' field embedded in a driver-specific struct is
conventionally called 'bridge', so renaming it from 'base' to 'bridge'
would make it more consistent with other drivers. That would go in sync
with the coding convention I mentioned above: 'bridge' for struct
drm_bridge, <XYZ>bridge or just <XYZZ> for a custom driver struct embedding
a bridge.

> +	if (IS_ERR(bridge))
> +		return ERR_PTR(PTR_ERR(bridge));
> +
> +	bridge->crtc = crtc;
> +	bridge->intf = intf;
> +	bridge->next_bridge = next;

There is now a next_bridge field in struct drm_bridge, which handles the
bridge lifetime in a safer way and more simply [0], so you could use it:

       bridge->base.next_bridge = next;

Or, after the renames I suggested above:

       vbridge->bridge.next_bridge = next;

[0] https://elixir.bootlin.com/linux/v7.0-rc2/source/include/drm/drm_bridge.h#L1269-L1278

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com