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

[Icenowy Zheng <zhengxingda@iscas.ac.cn>]

在 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@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).

Yes this seems to sound right, but doing such rework sounds quite big
and unnecessary after it's applied...

> 
> > +
> > +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