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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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)