Messages in this thread |  | | Date | Mon, 09 Mar 2026 13:47:18 +0100 | | Subject | Re: [PATCH v7 3/8] drm: verisilicon: add a driver for Verisilicon display controllers | | From | "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).
> + > +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
|  |