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

[Icenowy Zheng <uwu@icenowy.me>]

在 2025-09-23星期二的 03:53 +0300，Dmitry Baryshkov写道：
> On Sun, Sep 21, 2025 at 04:34:41PM +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@icenowy.me>
> > ---
> > Changes in v2:
> > - Changed some Control flows according to previous reviews.
> > - Added missing of_node_put when checking of endpoints for output
> > type.
> > - Switched all userspace-visible modeset objects to be managed by
> > drmm
> >   instead of devm.
> > - Utilize devm_drm_bridge_alloc() in internal bridge.
> > - Prevented the usage of simple encoder helpers by passing a NULL
> > funcs pointer.
> > - Let devm enable clocks when getting them.
> > - Removed explicit `.cache_type = REGCACHE_NONE` in regmap config.
> > - Fixed a debug print using a variable before initialization.
> > - Fixed a wrong index when using bulk to handle resets.
> > - Added missing configuration for DPI format (currently fixed
> > RGB888).
> > 
> >  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  |  54 +++
> >  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           | 205 +++++++++++
> >  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    | 157 +++++++++
> >  .../drm/verisilicon/vs_primary_plane_regs.h   |  53 +++
> >  21 files changed, 1789 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
> > 
> > +
> > +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;
> 
> I still think that it's better to have per-interface type bridge
> funcs
> rather than checking for the interface type inside the function.
> 
> > +
> > +       vbridge->output_bus_fmt = bridge_state-
> > >output_bus_cfg.format;
> > +
> > +       return 0;
> > +}
> > +
> > +
> > +       bridge = devm_drm_bridge_alloc(drm_dev->dev, struct
> > vs_bridge, base,
> > +                                      &vs_bridge_funcs);
> > +       if (!bridge)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       bridge->crtc = crtc;
> > +       bridge->intf = intf;
> > +       bridge->next = next;
> > +
> > +       if (intf == VSDC_OUTPUT_INTERFACE_DPI)
> > +               enctype = DRM_MODE_ENCODER_DPI;
> > +       else
> > +               enctype = DRM_MODE_ENCODER_NONE;
> 
> Nit: DRM_MODE_ENCODER_TMDS ?

The DC it self never encodes TMDS, and although most SoC connect the DP
interface to HDMI TX controllers, it's theortically to use other
bridges here (e.g. DP TX controllers).

> 
> > +
> > +       bridge->enc = drmm_plain_encoder_alloc(drm_dev, NULL,
> > enctype, NULL);
> > +       if (IS_ERR(bridge->enc)) {
> > +               dev_err(drm_dev->dev,
> > +                       "Cannot initialize encoder for output
> > %u\n", output);
> > +               ret = PTR_ERR(bridge->enc);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       bridge->enc->possible_crtcs = drm_crtc_mask(&crtc->base);
> > +
> > +       ret = drm_bridge_attach(bridge->enc, &bridge->base, NULL,
> > +                               DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> > +       if (ret) {
> > +               dev_err(drm_dev->dev,
> > +                       "Cannot attach bridge for output %u\n",
> > output);
> > +               return ERR_PTR(ret);
> > +       }
> > +
> > +       bridge->conn = drm_bridge_connector_init(drm_dev, bridge-
> > >enc);
> > +       if (IS_ERR(bridge->conn)) {
> > +               dev_err(drm_dev->dev,
> > +                       "Cannot create connector for output %u\n",
> > output);
> > +               ret = PTR_ERR(bridge->conn);
> > +               return ERR_PTR(ret);
> > +       }
> > +       drm_connector_attach_encoder(bridge->conn, bridge->enc);
> > +
> > +       return bridge;
> > +}
> 
> Other than that LGTM.
>