| 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: <eyfgdk6d2oj2tpw7vndxcn3jw5ib3h4sahghbe23xn4qvj3rr6@jwzuzjpihnlx>
Date: Sat, 16 Aug 2025 21:10:09 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Icenowy Zheng <uwu@...nowy.me>
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
On Sun, Aug 17, 2025 at 01:22:07AM +0800, Icenowy Zheng wrote:
> 在 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:
I think you can pass NULL for funcs only if you use drmm_* functions to
handle encoder. This way it gets cleaned before
drm_mode_config_cleanup() gets called.
> [ 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
--
With best wishes
Dmitry
Powered by blists - more mailing lists