| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0a931bef23d8feb758f5a752ee84a4abaddb8fa4.camel@icenowy.me>
Date: Sun, 17 Aug 2025 01:22:07 +0800
From: Icenowy Zheng <uwu@...nowy.me>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard
<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Herring
<robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Drew Fustini <fustini@...nel.org>, Guo Ren
<guoren@...nel.org>, Fu Wei <wefu@...hat.com>, Philipp Zabel
<p.zabel@...gutronix.de>, Heiko Stuebner <heiko@...ech.de>, Andrzej Hajda
<andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Laurent Pinchart
<Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>, Michal Wilczynski
<m.wilczynski@...sung.com>, Han Gao <rabenda.cn@...il.com>, Yao Zi
<ziyao@...root.org>, dri-devel@...ts.freedesktop.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org
Subject: Re: [RFC PATCH 3/8] drm: verisilicon: add a driver for Verisilicon
display controllers
在 2025-08-17星期日的 00:48 +0800，Icenowy Zheng写道：
> 在 2025-08-16星期六的 19:18 +0300，Dmitry Baryshkov写道：
> > On Fri, Aug 15, 2025 at 12:40:43AM +0800, Icenowy Zheng wrote:
> > > 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@...nowy.me>
> > > ---
> > > drivers/gpu/drm/Kconfig | 2 +
> > > drivers/gpu/drm/Makefile | 1 +
> > > drivers/gpu/drm/verisilicon/Kconfig | 15 +
> > > drivers/gpu/drm/verisilicon/Makefile | 5 +
> > > drivers/gpu/drm/verisilicon/vs_bridge.c | 330
> > > ++++++++++++++++++
> > > drivers/gpu/drm/verisilicon/vs_bridge.h | 40 +++
> > > drivers/gpu/drm/verisilicon/vs_bridge_regs.h | 47 +++
> > > drivers/gpu/drm/verisilicon/vs_crtc.c | 217 ++++++++++++
> > > drivers/gpu/drm/verisilicon/vs_crtc.h | 29 ++
> > > drivers/gpu/drm/verisilicon/vs_crtc_regs.h | 60 ++++
> > > drivers/gpu/drm/verisilicon/vs_dc.c | 233
> > > +++++++++++++
> > > drivers/gpu/drm/verisilicon/vs_dc.h | 39 +++
> > > drivers/gpu/drm/verisilicon/vs_dc_top_regs.h | 27 ++
> > > drivers/gpu/drm/verisilicon/vs_drm.c | 177 ++++++++++
> > > drivers/gpu/drm/verisilicon/vs_drm.h | 29 ++
> > > drivers/gpu/drm/verisilicon/vs_hwdb.c | 150 ++++++++
> > > drivers/gpu/drm/verisilicon/vs_hwdb.h | 29 ++
> > > drivers/gpu/drm/verisilicon/vs_plane.c | 102 ++++++
> > > drivers/gpu/drm/verisilicon/vs_plane.h | 68 ++++
> > > .../gpu/drm/verisilicon/vs_primary_plane.c | 166 +++++++++
> > > .../drm/verisilicon/vs_primary_plane_regs.h | 53 +++
> > > 21 files changed, 1819 insertions(+)
> > > create mode 100644 drivers/gpu/drm/verisilicon/Kconfig
> > > create mode 100644 drivers/gpu/drm/verisilicon/Makefile
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_bridge.c
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_bridge.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_bridge_regs.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc_regs.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_top_regs.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_drm.c
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_drm.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_hwdb.c
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_hwdb.h
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c
> > > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h
> > > create mode 100644
> > > drivers/gpu/drm/verisilicon/vs_primary_plane.c
> > > create mode 100644
> > > drivers/gpu/drm/verisilicon/vs_primary_plane_regs.h
> > >
> > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > > index f7ea8e895c0c0..33601485ecdba 100644
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -396,6 +396,8 @@ source "drivers/gpu/drm/sprd/Kconfig"
> > >
> > > source "drivers/gpu/drm/imagination/Kconfig"
> > >
> > > +source "drivers/gpu/drm/verisilicon/Kconfig"
> > > +
> > > config DRM_HYPERV
> > > tristate "DRM Support for Hyper-V synthetic video device"
> > > depends on DRM && PCI && HYPERV
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > > index 4dafbdc8f86ac..32ed4cf9df1bd 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -231,6 +231,7 @@ obj-y += solomon/
> > > obj-$(CONFIG_DRM_SPRD) += sprd/
> > > obj-$(CONFIG_DRM_LOONGSON) += loongson/
> > > obj-$(CONFIG_DRM_POWERVR) += imagination/
> > > +obj-$(CONFIG_DRM_VERISILICON_DC) += verisilicon/
> > >
> > > # Ensure drm headers are self-contained and pass kernel-doc
> > > hdrtest-files := \
> > > diff --git a/drivers/gpu/drm/verisilicon/Kconfig
> > > b/drivers/gpu/drm/verisilicon/Kconfig
> > > new file mode 100644
> > > index 0000000000000..0235577c72824
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/verisilicon/Kconfig
> > > @@ -0,0 +1,15 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +config DRM_VERISILICON_DC
> > > + tristate "DRM Support for Verisilicon DC-series display
> > > controllers"
> > > + depends on DRM && COMMON_CLK
> > > + depends on RISCV || COMPILER_TEST
> > > + select DRM_CLIENT_SELECTION
> > > + select DRM_GEM_DMA_HELPER
> > > + select DRM_KMS_HELPER
> > > + select DRM_BRIDGE_CONNECTOR
> > > + select REGMAP_MMIO
> > > + select VIDEOMODE_HELPERS
> > > + help
> > > + Choose this option if you have a SoC with Verisilicon
> > > DC-
> > > series
> > > + display controllers. If M is selected, the module will
> > > be
> > > called
> > > + verisilicon-dc.
> > > diff --git a/drivers/gpu/drm/verisilicon/Makefile
> > > b/drivers/gpu/drm/verisilicon/Makefile
> > > new file mode 100644
> > > index 0000000000000..fd8d805fbcde1
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/verisilicon/Makefile
> > > @@ -0,0 +1,5 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +
> > > +verisilicon-dc-objs := vs_bridge.o vs_crtc.o vs_dc.o vs_drm.o
> > > vs_hwdb.o vs_plane.o vs_primary_plane.o
> > > +
> > > +obj-$(CONFIG_DRM_VERISILICON_DC) += verisilicon-dc.o
> > > diff --git a/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > b/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > new file mode 100644
> > > index 0000000000000..c8caf31fac7d6
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/verisilicon/vs_bridge.c
> > > @@ -0,0 +1,330 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Copyright (C) 2025 Icenowy Zheng <uwu@...nowy.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, flags);
> > > +}
> > > +
> > > +struct vsdc_dp_format {
> > > + u32 linux_fmt;
> > > + bool is_yuv;
> > > + u32 vsdc_fmt;
> > > +};
> > > +
> > > +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_RGB888_1X24, false,
> > > VSDC_DISP_DP_CONFIG_FMT_RGB888 },
> > > + { MEDIA_BUS_FMT_RGB101010_1X30,
> > > + false, VSDC_DISP_DP_CONFIG_FMT_RGB101010 },
> > > + { 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 },
> > > +};
> > > +
> > > +static u32 *vs_bridge_atomic_get_output_bus_fmts(struct
> > > drm_bridge
> > > *bridge,
> > > + struct drm_bridge_state
> > > *bridge_state,
> > > + struct drm_crtc_state
> > > *crtc_state,
> > > + struct
> > > drm_connector_state
> > > *conn_state,
> > > + unsigned int
> > > *num_output_fmts)
> > > +{
> > > + struct vs_bridge *vbridge =
> > > drm_bridge_to_vs_bridge(bridge);
> > > + struct drm_connector *conn = conn_state->connector;
> > > + u32 *output_fmts;
> > > + unsigned int i;
> > > +
> > > + if (vbridge->intf == VSDC_OUTPUT_INTERFACE_DPI)
> >
> > This kind of checks looks like there should be a drm_encoder
> > handled
> > by
> > the same driver. Or maybe it's better to have two sets of funcs
> > structures, one for the DPI, one for DP.
>
> Well these functions used to be for an encoder, however I found that
> encoders cannot take part in format negotiation, and at least some
> source says encoder is deprecated in this situation and a first
> bridge
> in the bridge chain is better here.
>
> A simple encoder is created by this part of driver, but all its works
> are moved to this bridge, similar to what other drivers with bridge
> chaining support do.
>
> >
> > > + *num_output_fmts = 1;
> > > + else
> > > + *num_output_fmts =
> > > ARRAY_SIZE(vsdc_dp_supported_fmts);
> > > +
> > > + output_fmts = kcalloc(*num_output_fmts,
> > > sizeof(*output_fmts),
> > > + GFP_KERNEL);
> > > + if (!output_fmts)
> > > + return NULL;
> > > +
> > > + if (vbridge->intf == VSDC_OUTPUT_INTERFACE_DPI) {
> > > + if (conn->display_info.num_bus_formats &&
> > > + conn->display_info.bus_formats)
> > > + output_fmts[0] = conn-
> > > > display_info.bus_formats[0];
> > > + else
> > > + output_fmts[0] = MEDIA_BUS_FMT_FIXED;
> > > + } else {
> > > + for (i = 0; i < *num_output_fmts; i++)
> > > + output_fmts[i] =
> > > vsdc_dp_supported_fmts[i].linux_fmt;
> >
> > memcpy(a, b, min(ARRAY_SIZE(), num_output_fmts)) ?
>
> vsdc_dp_supported_fmts is a map of linux_fmt to hardware-specific
> parameter, so memcpy won't work here.
>
> >
> > > + }
> > > +
> > > + return output_fmts;
> > > +}
> > > +
> > > +static bool vs_bridge_out_dp_fmt_supported(u32 out_fmt)
> > > +{
> > > + unsigned int i;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(vsdc_dp_supported_fmts); i++)
> > > + if (vsdc_dp_supported_fmts[i].linux_fmt ==
> > > out_fmt)
> >
> > return true;
> >
> > > + break;
> > > +
> > > + return !(i == ARRAY_SIZE(vsdc_dp_supported_fmts));
> >
> > return false;
> >
> > > +}
> > > +
> > > +static u32 *vs_bridge_atomic_get_input_bus_fmts(struct
> > > drm_bridge
> > > *bridge,
> > > + struct drm_bridge_state
> > > *bridge_state,
> > > + struct drm_crtc_state
> > > *crtc_state,
> > > + struct
> > > drm_connector_state
> > > *conn_state,
> > > + u32 output_fmt,
> > > + unsigned int
> > > *num_input_fmts)
> > > +{
> > > + struct vs_bridge *vbridge =
> > > drm_bridge_to_vs_bridge(bridge);
> > > +
> > > + if (vbridge->intf == VSDC_OUTPUT_INTERFACE_DP &&
> > > + !vs_bridge_out_dp_fmt_supported(output_fmt)) {
> > > + *num_input_fmts = 0;
> > > + return NULL;
> > > + }
> > > +
> > > + return drm_atomic_helper_bridge_propagate_bus_fmt(bridge,
> > > bridge_state,
> > > +
> > > crtc_state,
> > > +
> > > conn_state,
> > > +
> > > output_fmt,
> > > +
> > > num_input_fmts);
> > > +}
> > > +
> > > +static int vs_bridge_atomic_check(struct drm_bridge *bridge,
> > > + struct drm_bridge_state
> > > *bridge_state,
> > > + struct drm_crtc_state
> > > *crtc_state,
> > > + struct drm_connector_state
> > > *conn_state)
> > > +{
> > > + struct vs_bridge *vbridge =
> > > drm_bridge_to_vs_bridge(bridge);
> > > +
> > > + if (vbridge->intf == VSDC_OUTPUT_INTERFACE_DP &&
> > > + !vs_bridge_out_dp_fmt_supported(bridge_state-
> > > > output_bus_cfg.format))
> > > + return -EINVAL;
> > > +
> > > + vbridge->output_bus_fmt = bridge_state-
> > > > output_bus_cfg.format;
> >
> > You are saving a state value into a non-state variable. There is no
> > guarantee that this atomic_check() will be followed by the actual
> > commit. So, either you have to use a struct that extends
> > drm_bridge_state here or store the output_bus_fmt during
> > atomic_enable().
>
> In fact I don't want to save it -- the kernel is quirky here and this
> value does not get passed into atomic_enable. I mimicked what other
> drivers do. See ingenic_drm_bridge_atomic_check() in ingenic/ingenic-
> drm-drv.c and meson_encoder_hdmi_atomic_check() in
> meson/meson_encoder_hdmi.c .
>
> >
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void vs_bridge_atomic_enable(struct drm_bridge *bridge,
> > > + struct drm_atomic_state
> > > *state)
> > > +{
> > > + struct vs_bridge *vbridge =
> > > drm_bridge_to_vs_bridge(bridge);
> > > + struct drm_bridge_state *br_state =
> > > drm_atomic_get_bridge_state(state,
> > > +
> > >
> > > bridge);
> > > + struct vs_crtc *crtc = vbridge->crtc;
> > > + struct vs_dc *dc = crtc->dc;
> > > + unsigned int output = crtc->id;
> > > + u32 dp_fmt;
> > > + unsigned int i;
> > > +
> > > + DRM_DEBUG_DRIVER("Enabling output %u\n", output);
> > > +
> > > + switch (vbridge->intf) {
> > > + case VSDC_OUTPUT_INTERFACE_DPI:
> > > + regmap_clear_bits(dc->regs,
> > > VSDC_DISP_DP_CONFIG(output),
> > > + VSDC_DISP_DP_CONFIG_DP_EN);
> > > + break;
> > > + case VSDC_OUTPUT_INTERFACE_DP:
> > > + for (i = 0; i <
> > > ARRAY_SIZE(vsdc_dp_supported_fmts);
> > > i++) {
> > > + if (vsdc_dp_supported_fmts[i].linux_fmt
> > > ==
> > > + vbridge->output_bus_fmt)
> > > + break;
> > > + }
> > > + WARN_ON_ONCE(i ==
> > > ARRAY_SIZE(vsdc_dp_supported_fmts));
> > > + dp_fmt = vsdc_dp_supported_fmts[i].vsdc_fmt;
> >
> > This might trigger all static checkers in the universe. It's not
> > really
> > possible, since you've checked it in the atomic_check(), but...
>
> Sigh I don't know how to properly describe it...
>
> I can only say something really bad happens if the previous
> WARN_ON_ONCE is triggered.
>
> >
> > > + dp_fmt |= VSDC_DISP_DP_CONFIG_DP_EN;
> > > + regmap_write(dc->regs,
> > > VSDC_DISP_DP_CONFIG(output),
> > > dp_fmt);
> > > + regmap_assign_bits(dc->regs,
> > > +
> > > VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_YUV,
> > > +
> > > vsdc_dp_supported_fmts[i].is_yuv);
> > > + break;
> > > + }
> > > +
> > > + regmap_clear_bits(dc->regs,
> > > VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_DAT_POL);
> > > + regmap_assign_bits(dc->regs,
> > > VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_DE_POL,
> > > + br_state->output_bus_cfg.flags &
> > > + DRM_BUS_FLAG_DE_LOW);
> > > + regmap_assign_bits(dc->regs,
> > > VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_CLK_POL,
> > > + br_state->output_bus_cfg.flags &
> > > + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE);
> > > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_DE_EN |
> > > + VSDC_DISP_PANEL_CONFIG_DAT_EN |
> > > + VSDC_DISP_PANEL_CONFIG_CLK_EN);
> > > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_RUNNING);
> > > + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > + VSDC_DISP_PANEL_START_MULTI_DISP_SYNC);
> > > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > + VSDC_DISP_PANEL_START_RUNNING(output));
> > > +
> > > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc-
> > > > id),
> > > + VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
> > > +}
> > > +
> > > +static void vs_bridge_atomic_disable(struct drm_bridge *bridge,
> > > + struct drm_atomic_state
> > > *state)
> > > +{
> > > + struct vs_bridge *vbridge =
> > > drm_bridge_to_vs_bridge(bridge);
> > > + struct vs_crtc *crtc = vbridge->crtc;
> > > + struct vs_dc *dc = crtc->dc;
> > > + unsigned int output = crtc->id;
> > > +
> > > + DRM_DEBUG_DRIVER("Disabling output %u\n", output);
> > > +
> > > + regmap_clear_bits(dc->regs, VSDC_DISP_PANEL_START,
> > > + VSDC_DISP_PANEL_START_MULTI_DISP_SYNC |
> > > + VSDC_DISP_PANEL_START_RUNNING(output));
> > > + regmap_clear_bits(dc->regs,
> > > VSDC_DISP_PANEL_CONFIG(output),
> > > + VSDC_DISP_PANEL_CONFIG_RUNNING);
> > > +
> > > + regmap_set_bits(dc->regs, VSDC_DISP_PANEL_CONFIG_EX(crtc-
> > > > id),
> > > + VSDC_DISP_PANEL_CONFIG_EX_COMMIT);
> > > +}
> > > +
> > > +static const struct drm_bridge_funcs vs_bridge_funcs = {
> > > + .attach = vs_bridge_attach,
> > > + .atomic_enable = vs_bridge_atomic_enable,
> > > + .atomic_disable = vs_bridge_atomic_disable,
> > > + .atomic_check = vs_bridge_atomic_check,
> > > + .atomic_get_input_bus_fmts =
> > > vs_bridge_atomic_get_input_bus_fmts,
> > > + .atomic_get_output_bus_fmts =
> > > vs_bridge_atomic_get_output_bus_fmts,
> > > + .atomic_duplicate_state =
> > > drm_atomic_helper_bridge_duplicate_state,
> > > + .atomic_destroy_state =
> > > drm_atomic_helper_bridge_destroy_state,
> > > + .atomic_reset = drm_atomic_helper_bridge_reset,
> > > +};
> > > +
> > > +static int vs_bridge_detect_output_interface(struct device_node
> > > *of_node,
> > > + unsigned int output)
> > > +{
> > > + int ret;
> > > + struct device_node *remote;
> > > +
> > > + remote = of_graph_get_remote_node(of_node, output,
> > > +
> > > VSDC_OUTPUT_INTERFACE_DPI);
> >
> > This deserves a comment in the source file.
> >
> > > + if (remote) {
> > > + ret = VSDC_OUTPUT_INTERFACE_DPI;
> >
> > return here, drop else{}
>
> Well a of_node_put() is missing before the final return, and Yao Zi
> noted me of it.
>
> >
> > > + } else {
> > > + remote = of_graph_get_remote_node(of_node,
> > > output,
> > > +
> > > VSDC_OUTPUT_INTERFACE_DP);
> > > + if (remote)
> > > + ret = VSDC_OUTPUT_INTERFACE_DP;
> >
> > return
> >
> > > + else
> > > + ret = -ENODEV;
> > > + }
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +struct vs_bridge *vs_bridge_init(struct drm_device *drm_dev,
> > > + struct vs_crtc *crtc)
> > > +{
> > > + unsigned int output = crtc->id;
> > > + struct vs_bridge *bridge;
> > > + struct drm_bridge *next;
> > > + enum vs_bridge_output_interface intf;
> > > + int ret;
> > > +
> > > + intf = vs_bridge_detect_output_interface(drm_dev->dev-
> > > > of_node,
> > > + output);
> > > + if (intf == -ENODEV) {
> > > + dev_info(drm_dev->dev, "Skipping output %u\n",
> > > output);
> > > + return NULL;
> > > + }
> > > +
> > > + bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge),
> > > GFP_KERNEL);
> >
> > devm_drm_bridge_alloc()
> >
> > > + if (!bridge)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + bridge->crtc = crtc;
> > > + bridge->intf = intf;
> > > + bridge->base.funcs = &vs_bridge_funcs;
> > > +
> > > + next = devm_drm_of_get_bridge(drm_dev->dev, drm_dev->dev-
> > > > of_node,
> > > + output, intf);
> > > + if (IS_ERR(next)) {
> > > + ret = PTR_ERR(next);
> > > + goto err_free_bridge;
> > > + }
> > > +
> > > + bridge->next = next;
> > > +
> > > + ret = drm_simple_encoder_init(drm_dev, &bridge->enc,
> >
> > Oh, so there is an encoder... Please drop drm_simple_encoder, it's
> > deprecated, and try moving all the ifs to the encoder funcs.
>
> Ah? Is it really deprecated? I can find no source of this
> deprecation.
>
> In addition, I think many drivers here are using a bridge as a
> "better
> encoder" because of the restriction of current encoder
> implementation,
> and I am doing the same thing. Either encoder functionality should be
> improved to on par with bridge, or such dummy encoders with a bridge
> should exist, and some helper for creating them should exist. It
> might
> be not drm_simple_encoder_init (because I can understand the
> deprecation of other parts of the simple-kms routines, although I see
> no formal documentation mentioning it's deprecated, maybe I missed
> some
> newspaper?), but it should exist.
I see some practice of passing NULL to drmm_plain_encoder_alloc() from
the adp driver, however looks like this isn't always safe and on my
test of this change on top of verisilicon driver (on top of v6.17-rc1)
I got mysterious oops if the DC driver happens to be probed before the
HDMI controller driver:
```
[ 28.372698] Unable to handle kernel access to user memory without
uaccess routines at virtual address 0000000000000010
[ 28.383596] Current (udev-worker) pgtable: 4K pagesize, 39-bit VAs,
pgdp=0x0000000108532000
[ 28.392180] [0000000000000010] pgd=0000000000000000,
p4d=0000000000000000, pud=0000000000000000
[ 28.401108] Oops [#1]
[ 28.403405] Modules linked in: th1520_dw_hdmi verisilicon_dc(+)
configfs dm_mod
[ 28.410773] CPU: 1 UID: 0 PID: 527 Comm: (udev-worker) Not tainted
6.17.0-rc1+ #92 NONE
[ 28.418890] Hardware name: Sipeed Lichee Pi 4A (DT)
[ 28.423784] epc : drm_mode_config_cleanup+0xc6/0x224
[ 28.428800] ra : drm_mode_config_cleanup+0xa4/0x224
[ 28.433796] epc : ffffffff807d7016 ra : ffffffff807d6ff4 sp :
ffffffc6004f3790
[ 28.441038] gp : ffffffff81c52ae8 tp : ffffffd700873ac0 t0 :
0000000000000040
[ 28.448279] t1 : 0000000000000000 t2 : 0000000000001c91 s0 :
ffffffc6004f3810
[ 28.455514] s1 : ffffffd70847f000 a0 : ffffffd70847e840 a1 :
ffffffd70847f2f8
[ 28.462770] a2 : ffffffff81c94178 a3 : 0000000000000002 a4 :
0000000000000000
[ 28.470027] a5 : 0000000000000000 a6 : 0000000000000436 a7 :
ffffffd70d3eb350
[ 28.477263] s2 : fffffffffffffff8 s3 : ffffffd70847f2d0 s4 :
ffffffd70847f2b8
[ 28.484499] s5 : dead000000000100 s6 : ffffffd70847f018 s7 :
0000000000000000
[ 28.491733] s8 : ffffffc6004f3d40 s9 : ffffffff01d04198 s10:
ffffffc6004f3c80
[ 28.498968] s11: ffffffff01d042c0 t3 : 0000000000000002 t4 :
0000000000000402
[ 28.506200] t5 : ffffffd70847f1f8 t6 : ffffffd70847f200
[ 28.511523] status: 0000000200000120 badaddr: 0000000000000010
cause: 000000000000000d
[ 28.519454] [<ffffffff807d7016>] drm_mode_config_cleanup+0xc6/0x224
[ 28.525754] [<ffffffff807d75f8>]
drm_mode_config_init_release+0xc/0x14
[ 28.532312] [<ffffffff807d5b8e>] drm_managed_release+0x7a/0x100
[ 28.538257] [<ffffffff807c6b9a>] devm_drm_dev_init_release+0x62/0x78
[ 28.544641] [<ffffffff8084877a>] devm_action_release+0xe/0x18
[ 28.550411] [<ffffffff80848b1a>] release_nodes+0x3e/0x94
[ 28.555752] [<ffffffff80849cc6>] devres_release_all+0x72/0xb4
[ 28.561529] [<ffffffff80843d74>] device_unbind_cleanup+0x10/0x58
[ 28.567554] [<ffffffff80844510>] really_probe+0x184/0x30c
[ 28.572973] [<ffffffff808446fc>] __driver_probe_device+0x64/0x10c
[ 28.579086] [<ffffffff80844868>] driver_probe_device+0x2c/0xb4
[ 28.584937] [<ffffffff80844a5a>] __driver_attach+0x9a/0x1a4
[ 28.590530] [<ffffffff80842386>] bus_for_each_dev+0x62/0xb0
[ 28.596130] [<ffffffff80843ea6>] driver_attach+0x1a/0x24
[ 28.601461] [<ffffffff80843662>] bus_add_driver+0xf6/0x200
[ 28.606972] [<ffffffff80845858>] driver_register+0x40/0xdc
[ 28.612478] [<ffffffff80846bac>]
__platform_driver_register+0x1c/0x24
[ 28.618946] [<ffffffff01d0b020>]
vs_dc_platform_driver_init+0x20/0x1000 [verisilicon_dc]
[ 28.627075] [<ffffffff800158c6>] do_one_initcall+0x56/0x1cc
[ 28.632675] [<ffffffff800c8116>] do_init_module+0x52/0x1dc
[ 28.638187] [<ffffffff800c9bd2>] load_module+0x16e2/0x1afc
[ 28.643696] [<ffffffff800ca1c6>] init_module_from_file+0x76/0xb0
[ 28.649726] [<ffffffff800ca436>]
__riscv_sys_finit_module+0x1fe/0x3cc
[ 28.656195] [<ffffffff80cfee1e>] do_trap_ecall_u+0x296/0x370
[ 28.661878] [<ffffffff80d09a56>] handle_exception+0x146/0x152
[ 28.667656] Code: 8993 2d04 b903 0007 8513 ff87 1961 8e63 00f9 7d5c
(6b9c) 9782
[ 28.675233] ---[ end trace 0000000000000000 ]---
```
Powered by blists - more mailing lists