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

From: Thomas Zimmermann

Date: Tue Mar 10 2026 - 04:27:52 EST

Hi

Am 09.03.26 um 17:35 schrieb Icenowy Zheng:
在 2026-03-09一的 13:47 +0100，Luca Ceresoli写道：
Hello Icenowy Zheng,

On Thu Jan 29, 2026 at 3:39 AM CET, Icenowy Zheng wrote:
From: Icenowy Zheng <uwu@xxxxxxxxxx>

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@xxxxxxxxxx>
Signed-off-by: Icenowy Zheng <zhengxingda@xxxxxxxxxxx>
Tested-by: Han Gao <gaohan@xxxxxxxxxxx>
Tested-by: Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>
Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
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@xxxxxxxxxx>
+ */
+
+#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).
Yes this seems to sound right, but doing such rework sounds quite big
and unnecessary after it's applied...

Why? You are merely moving fields around, right? Just send a patch then.

Best regards
Thomas


+
+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.
Ah I prefer to keep 80 CPL when I can, and the `coding-style.rst`
document still suggests 80.

+ { 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 sounds right.

BTW where is such kind of common practice documented?

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.
Ah, all subclasses in this driver call the base class `base`, and I
still wonder how such convention is documented.

+ 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:
Glad to hear such a field exists now. Will more code about next_bridge
lifetime management being shared?

Thanks,
Icenowy

       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

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)