[v3 6/6] drm/vs: simple encoder
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Wed May 15 08:17:08 PDT 2024
On Wed, May 15, 2024 at 10:07:27AM +0000, Keith Zhao wrote:
>
>
> > -----Original Message-----
> > From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > Sent: 2023年12月5日 21:19
> > To: Keith Zhao <keith.zhao at starfivetech.com>
> > Cc: devicetree at vger.kernel.org; dri-devel at lists.freedesktop.org;
> > linux-kernel at vger.kernel.org; linux-riscv at lists.infradead.org;
> > aou at eecs.berkeley.edu; suijingfeng at loongson.cn; tzimmermann at suse.de;
> > paul.walmsley at sifive.com; mripard at kernel.org; Xingyu Wu
> > <xingyu.wu at starfivetech.com>; Jack Zhu <jack.zhu at starfivetech.com>;
> > palmer at dabbelt.com; krzysztof.kozlowski+dt at linaro.org; William Qiu
> > <william.qiu at starfivetech.com>; Shengyang Chen
> > <shengyang.chen at starfivetech.com>; Changhuang Liang
> > <changhuang.liang at starfivetech.com>
> > Subject: Re: [v3 6/6] drm/vs: simple encoder
> >
> > On Tue, 5 Dec 2023 at 15:14, Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> > wrote:
> > >
> > > On Mon, 4 Dec 2023 at 14:33, Keith Zhao <keith.zhao at starfivetech.com>
> > wrote:
> > > >
> > > > add simple encoder for dsi bridge
> > >
> > > This doesn't look like a proper commit message.
> > >
> > > >
> > > > Signed-off-by: Keith Zhao <keith.zhao at 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.
>
> 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
More information about the linux-riscv
mailing list