> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> Sent: 2024年5月15日 23:17
> To: Keith Zhao <keith.z...@starfivetech.com>
> Cc: devicet...@vger.kernel.org; dri-devel@lists.freedesktop.org;
> linux-ker...@vger.kernel.org; linux-ri...@lists.infradead.org;
> a...@eecs.berkeley.edu; suijingf...@loongson.cn; tzimmerm...@suse.de;
> paul.walms...@sifive.com; mrip...@kernel.org; Xingyu Wu
> <xingyu...@starfivetech.com>; Jack Zhu <jack....@starfivetech.com>;
> pal...@dabbelt.com; krzysztof.kozlowski...@linaro.org; William Qiu
> <william....@starfivetech.com>; Shengyang Chen
> <shengyang.c...@starfivetech.com>; Changhuang Liang
> <changhuang.li...@starfivetech.com>
> Subject: Re: [v3 6/6] drm/vs: simple encoder
>
> On Wed, May 15, 2024 at 10:07:27AM +0000, Keith Zhao wrote:
> >
> >
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> > > Sent: 2023年12月5日 21:19
> > > To: Keith Zhao <keith.z...@starfivetech.com>
> > > Cc: devicet...@vger.kernel.org; dri-devel@lists.freedesktop.org;
> > > linux-ker...@vger.kernel.org; linux-ri...@lists.infradead.org;
> > > a...@eecs.berkeley.edu; suijingf...@loongson.cn; tzimmerm...@suse.de;
> > > paul.walms...@sifive.com; mrip...@kernel.org; Xingyu Wu
> > > <xingyu...@starfivetech.com>; Jack Zhu <jack....@starfivetech.com>;
> > > pal...@dabbelt.com; krzysztof.kozlowski...@linaro.org; William Qiu
> > > <william....@starfivetech.com>; Shengyang Chen
> > > <shengyang.c...@starfivetech.com>; Changhuang Liang
> > > <changhuang.li...@starfivetech.com>
> > > Subject: Re: [v3 6/6] drm/vs: simple encoder
> > >
> > > On Tue, 5 Dec 2023 at 15:14, Dmitry Baryshkov
> > > <dmitry.barysh...@linaro.org>
> > > wrote:
> > > >
> > > > On Mon, 4 Dec 2023 at 14:33, Keith Zhao
> > > > <keith.z...@starfivetech.com>
> > > wrote:
> > > > >
> > > > > add simple encoder for dsi bridge
> > > >
> > > > This doesn't look like a proper commit message.
> > > >
> > > > >
> > > > > Signed-off-by: Keith Zhao <keith.z...@starfivetech.com>
> > > > > ---
> > > > > drivers/gpu/drm/verisilicon/Makefile | 4 +-
> > > > > drivers/gpu/drm/verisilicon/vs_drv.c | 2 +
> > > > > drivers/gpu/drm/verisilicon/vs_simple_enc.c | 195
> > > > > ++++++++++++++++++++
> > > > > ++++++++++++++++++++ drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > > > > ++++++++++++++++++++ |
> > > > > 23 +++
> > > > > 4 files changed, 223 insertions(+), 1 deletion(-) create mode
> > > > > 100644 drivers/gpu/drm/verisilicon/vs_simple_enc.c
> > > > > create mode 100644 drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > > > >
> > > > > diff --git a/drivers/gpu/drm/verisilicon/Makefile
> > > > > b/drivers/gpu/drm/verisilicon/Makefile
> > > > > index 71fadafcee13..cd5d0a90bcfe 100644
> > > > > --- a/drivers/gpu/drm/verisilicon/Makefile
> > > > > +++ b/drivers/gpu/drm/verisilicon/Makefile
> > > > > @@ -5,6 +5,8 @@ vs_drm-objs := vs_dc_hw.o \
> > > > > vs_crtc.o \
> > > > > vs_drv.o \
> > > > > vs_modeset.o \
> > > > > - vs_plane.o
> > > > > + vs_plane.o \
> > > > > + vs_simple_enc.o
> > > > > +
> > > > > vs_drm-$(CONFIG_DRM_VERISILICON_STARFIVE_HDMI) +=
> > > starfive_hdmi.o
> > > > > obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o diff --git
> > > > > a/drivers/gpu/drm/verisilicon/vs_drv.c
> > > > > b/drivers/gpu/drm/verisilicon/vs_drv.c
> > > > > index d7e5199fe293..946f137ab124 100644
> > > > > --- a/drivers/gpu/drm/verisilicon/vs_drv.c
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_drv.c
> > > > > @@ -23,6 +23,7 @@
> > > > > #include "vs_drv.h"
> > > > > #include "vs_modeset.h"
> > > > > #include "vs_dc.h"
> > > > > +#include "vs_simple_enc.h"
> > > > >
> > > > > #define DRV_NAME "verisilicon"
> > > > > #define DRV_DESC "Verisilicon DRM driver"
> > > > > @@ -217,6 +218,7 @@ static struct platform_driver
> > > > > *drm_sub_drivers[] = { #ifdef
> CONFIG_DRM_VERISILICON_STARFIVE_HDMI
> > > > > &starfive_hdmi_driver,
> > > > > #endif
> > > > > + &simple_encoder_driver,
> > > > > };
> > > > >
> > > > > static struct component_match *vs_drm_match_add(struct device
> > > > > *dev) diff --git a/drivers/gpu/drm/verisilicon/vs_simple_enc.c
> > > > > b/drivers/gpu/drm/verisilicon/vs_simple_enc.c
> > > > > new file mode 100644
> > > > > index 000000000000..c5a8d82bc469
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.c
> > > > > @@ -0,0 +1,195 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * Copyright (C) 2020 VeriSilicon Holdings Co., Ltd.
> > > > > + */
> > > > > +#include <linux/component.h>
> > > > > +#include <linux/of_device.h>
> > > > > +#include <linux/module.h>
> > > > > +
> > > > > +#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h>
> > > > > +#include <drm/drm_crtc_helper.h> #include <drm/drm_of.h>
> > > > > +#include <linux/regmap.h> #include <linux/media-bus-format.h>
> > > > > +#include <linux/mfd/syscon.h>
> > > > > +
> > > > > +#include "vs_crtc.h"
> > > > > +#include "vs_simple_enc.h"
> > > > > +
> > > > > +static const struct simple_encoder_priv dsi_priv = {
> > > >
> > > > Please use proper prefix for all the struct and function names.
> > > > vs_simple_encoder sounds better. Or vs_dsi_encoder.
> > > >
> > > > > + .encoder_type = DRM_MODE_ENCODER_DSI };
> > > > > +
> > > > > +static inline struct simple_encoder *to_simple_encoder(struct
> > > > > +drm_encoder *enc) {
> > > > > + return container_of(enc, struct simple_encoder,
> > > > > +encoder); }
> > > > > +
> > > > > +static int encoder_parse_dt(struct device *dev) {
> > > > > + struct simple_encoder *simple = dev_get_drvdata(dev);
> > > > > + unsigned int args[2];
> > > > > +
> > > > > + simple->dss_regmap =
> > > syscon_regmap_lookup_by_phandle_args(dev->of_node,
> > > > > +
> > > "starfive,syscon",
> > > > > +
> > > 2,
> > > > > + args);
> > > > > +
> > > > > + if (IS_ERR(simple->dss_regmap)) {
> > > > > + return dev_err_probe(dev,
> > > PTR_ERR(simple->dss_regmap),
> > > > > + "getting the regmap
> > > failed\n");
> > > > > + }
> > > > > +
> > > > > + simple->offset = args[0];
> > > > > + simple->mask = args[1];
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +void encoder_atomic_enable(struct drm_encoder *encoder,
> > > > > + struct drm_atomic_state *state) {
> > > > > + struct simple_encoder *simple =
> > > > > +to_simple_encoder(encoder);
> > > > > +
> > > > > + regmap_update_bits(simple->dss_regmap, simple->offset,
> > > simple->mask,
> > > > > + simple->mask); }
> > > > > +
> > > > > +int encoder_atomic_check(struct drm_encoder *encoder,
> > > > > + struct drm_crtc_state *crtc_state,
> > > > > + struct drm_connector_state *conn_state)
> {
> > > > > + struct vs_crtc_state *vs_crtc_state =
> > > to_vs_crtc_state(crtc_state);
> > > > > + struct drm_connector *connector = conn_state->connector;
> > > > > + int ret = 0;
> > > > > +
> > > > > + struct drm_bridge *first_bridge =
> > > drm_bridge_chain_get_first_bridge(encoder);
> > > > > + struct drm_bridge_state *bridge_state =
> > > > > + ERR_PTR(-EINVAL);
> > > > > +
> > > > > + vs_crtc_state->encoder_type = encoder->encoder_type;
> > > > > +
> > > > > + if (first_bridge &&
> first_bridge->funcs->atomic_duplicate_state)
> > > > > + bridge_state =
> > > > > + drm_atomic_get_bridge_state(crtc_state->state, first_bridge);
> > > >
> > > > Please don't poke into others' playground. This should go into
> > > > your DSI bridge's atomic_check() instead.
> > >
> > > Hmm. And you can not use vs_crtc_state from your bridge. Actually
> > > this design makes me wonder, how does your hardware work? Is it
> > > possible to send the DSI commands to the panel?
> > >
> > Hi Dmitry:
> > 1、 This fun is used to check the media bus format whether in support list ,
> > if
> not , this mode will be invalid,
> > I just used the bridge api to get the media bus format. (sync with
> > the input bridge(connector) bus format)
> > 2、 the bridge doesn't care the vs_crtc_state, just do their own
> drm_bridge_funcs ->atomic_check,
> > or it will be impossible to make this dsi encoder to fit dsi bridge
> > driver.
> > 3、 hardware work :
> > encoder_atomic_check get the media bus format and update it to
> vs_crtc_state-> output_fmt,
> > during the crtc_enable , it will write the output_fmt to hardware .
> >
> > 4、 to send dsi command to panel , It is only relevant to dsi controllers
> and panel driver (bridge and panel )，
> > It does not involve the logic associated with encoder and crtc
>
> Do you have a pointer to your DSI bridge driver somewhere (a preliminary
> version is ok, if it's not ready yet for upstream submission). From the
> current
> design it seems that there is no need for a separate 'simple'
> encoder. Instead it might be easier/better to transform your DSI bridge driver
> into an encoder driver, especially as you are prety flexible in the component
> connections.
>
Hi Dmitry:
My SoC DSI bridge driver is cdns dsi and cdns dsi is already in the path:
/linux/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
Encoder_bind will attach the bridge to encoder
/* output port is port1*/
bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
......
return drm_bridge_attach(encoder, bridge, NULL, 0);
There are two types of bridges in the Linux mainline code:
Type 1: only contains bridge driver, for example
/linux/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
/linux/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
Type 2: Contains encoder + bridge:
/linux/drivers/gpu\drm/i2c/tda998x_drv.c
This simple encoder driver is used to match the Type 1 bridge.
Of course, if i adapt to Type 2, i only need to care the encoder possible crtc.
Skip this encoder
Thanks
Keith
> >
> > https://elixir.bootlin.com/linux/v6.6.30/source/drivers/gpu/drm/mxsfb/
> > lcdif_kms.c#L457
> > Do the similar logic
>
> --
> With best wishes
> Dmitry
