[v3,6/6] drm/vs: simple encoder

Commit Message

Keith Zhao Dec. 4, 2023, 12:33 p.m. UTC

add simple encoder for dsi bridge

Signed-off-by: Keith Zhao <[e-mail]>
---
 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

Comments

Dmitry Baryshkov Dec. 5, 2023, 1:14 p.m. UTC | #1

On Mon, 4 Dec 2023 at 14:33, Keith Zhao <[e-mail]> wrote:
>
> add simple encoder for dsi bridge

This doesn't look like a proper commit message.

>
> Signed-off-by: Keith Zhao <[e-mail]>
> ---
>  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.

> +
> +       if (IS_ERR(bridge_state)) {
> +               if (connector->display_info.num_bus_formats)
> +                       vs_crtc_state->output_fmt = connector->display_info.bus_formats[0];
> +               else
> +                       vs_crtc_state->output_fmt = MEDIA_BUS_FMT_FIXED;
> +       } else {
> +               vs_crtc_state->output_fmt = bridge_state->input_bus_cfg.format;
> +       }
> +
> +       switch (vs_crtc_state->output_fmt) {
> +       case MEDIA_BUS_FMT_FIXED:
> +       case MEDIA_BUS_FMT_RGB565_1X16:
> +       case MEDIA_BUS_FMT_RGB666_1X18:
> +       case MEDIA_BUS_FMT_RGB888_1X24:
> +       case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> +       case MEDIA_BUS_FMT_RGB101010_1X30:
> +       case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> +       case MEDIA_BUS_FMT_UYVY8_1X16:
> +       case MEDIA_BUS_FMT_YUV8_1X24:
> +       case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> +       case MEDIA_BUS_FMT_UYVY10_1X20:
> +       case MEDIA_BUS_FMT_YUV10_1X30:
> +               ret = 0;
> +               break;
> +       default:
> +               ret = -EINVAL;
> +               break;
> +       }
> +
> +       /* If MEDIA_BUS_FMT_FIXED, set it to default value */
> +       if (vs_crtc_state->output_fmt == MEDIA_BUS_FMT_FIXED)
> +               vs_crtc_state->output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
> +
> +       return ret;
> +}
> +
> +static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> +       .atomic_check = encoder_atomic_check,
> +       .atomic_enable = encoder_atomic_enable,
> +};
> +
> +static int encoder_bind(struct device *dev, struct device *master, void *data)
> +{
> +       struct drm_device *drm_dev = data;
> +       struct simple_encoder *simple = dev_get_drvdata(dev);
> +       struct drm_encoder *encoder;
> +       struct drm_bridge *bridge;
> +       int ret;
> +
> +       encoder = &simple->encoder;
> +
> +       ret = drmm_encoder_init(drm_dev, encoder, NULL, simple->priv->encoder_type, NULL);
> +       if (ret)
> +               return ret;
> +
> +       drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +
> +       encoder->possible_crtcs =
> +                       drm_of_find_possible_crtcs(drm_dev, dev->of_node);
> +
> +       /* output port is port1*/
> +       bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> +       if (IS_ERR(bridge))
> +               return 0;
> +
> +       return drm_bridge_attach(encoder, bridge, NULL, 0);
> +}
> +
> +static const struct component_ops encoder_component_ops = {
> +       .bind = encoder_bind,
> +};
> +
> +static const struct of_device_id simple_encoder_dt_match[] = {
> +       { .compatible = "starfive,dsi-encoder", .data = &dsi_priv},
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, simple_encoder_dt_match);
> +
> +static int encoder_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct simple_encoder *simple;
> +       int ret;
> +
> +       simple = devm_kzalloc(dev, sizeof(*simple), GFP_KERNEL);
> +       if (!simple)
> +               return -ENOMEM;
> +
> +       simple->priv = of_device_get_match_data(dev);
> +
> +       simple->dev = dev;
> +
> +       dev_set_drvdata(dev, simple);
> +
> +       ret = encoder_parse_dt(dev);
> +       if (ret)
> +               return ret;
> +
> +       return component_add(dev, &encoder_component_ops);
> +}
> +
> +static int encoder_remove(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +
> +       component_del(dev, &encoder_component_ops);
> +       dev_set_drvdata(dev, NULL);
> +
> +       return 0;
> +}
> +
> +struct platform_driver simple_encoder_driver = {
> +       .probe = encoder_probe,
> +       .remove = encoder_remove,
> +       .driver = {
> +               .name = "vs-simple-encoder",
> +               .of_match_table = of_match_ptr(simple_encoder_dt_match),
> +       },
> +};
> +
> +MODULE_DESCRIPTION("Simple Encoder Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/verisilicon/vs_simple_enc.h b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> new file mode 100644
> index 000000000000..fb33ca9e18d6
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
> + */
> +
> +#ifndef __VS_SIMPLE_ENC_H_
> +#define __VS_SIMPLE_ENC_H_
> +
> +struct simple_encoder_priv {
> +       unsigned char encoder_type;
> +};
> +
> +struct simple_encoder {
> +       struct drm_encoder encoder;
> +       struct device *dev;
> +       const struct simple_encoder_priv *priv;
> +       struct regmap *dss_regmap;
> +       unsigned int offset;
> +       unsigned int mask;
> +};
> +
> +extern struct platform_driver simple_encoder_driver;
> +#endif /* __VS_SIMPLE_ENC_H_ */
> --
> 2.34.1
>

Dmitry Baryshkov Dec. 5, 2023, 1:18 p.m. UTC | #2

On Tue, 5 Dec 2023 at 15:14, Dmitry Baryshkov
<[e-mail]> wrote:
>
> On Mon, 4 Dec 2023 at 14:33, Keith Zhao <[e-mail]> wrote:
> >
> > add simple encoder for dsi bridge
>
> This doesn't look like a proper commit message.
>
> >
> > Signed-off-by: Keith Zhao <[e-mail]>
> > ---
> >  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?

>
> > +
> > +       if (IS_ERR(bridge_state)) {
> > +               if (connector->display_info.num_bus_formats)
> > +                       vs_crtc_state->output_fmt = connector->display_info.bus_formats[0];
> > +               else
> > +                       vs_crtc_state->output_fmt = MEDIA_BUS_FMT_FIXED;
> > +       } else {
> > +               vs_crtc_state->output_fmt = bridge_state->input_bus_cfg.format;
> > +       }
> > +
> > +       switch (vs_crtc_state->output_fmt) {
> > +       case MEDIA_BUS_FMT_FIXED:
> > +       case MEDIA_BUS_FMT_RGB565_1X16:
> > +       case MEDIA_BUS_FMT_RGB666_1X18:
> > +       case MEDIA_BUS_FMT_RGB888_1X24:
> > +       case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> > +       case MEDIA_BUS_FMT_RGB101010_1X30:
> > +       case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> > +       case MEDIA_BUS_FMT_UYVY8_1X16:
> > +       case MEDIA_BUS_FMT_YUV8_1X24:
> > +       case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> > +       case MEDIA_BUS_FMT_UYVY10_1X20:
> > +       case MEDIA_BUS_FMT_YUV10_1X30:
> > +               ret = 0;
> > +               break;
> > +       default:
> > +               ret = -EINVAL;
> > +               break;
> > +       }
> > +
> > +       /* If MEDIA_BUS_FMT_FIXED, set it to default value */
> > +       if (vs_crtc_state->output_fmt == MEDIA_BUS_FMT_FIXED)
> > +               vs_crtc_state->output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
> > +
> > +       return ret;
> > +}
> > +
> > +static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> > +       .atomic_check = encoder_atomic_check,
> > +       .atomic_enable = encoder_atomic_enable,
> > +};
> > +
> > +static int encoder_bind(struct device *dev, struct device *master, void *data)
> > +{
> > +       struct drm_device *drm_dev = data;
> > +       struct simple_encoder *simple = dev_get_drvdata(dev);
> > +       struct drm_encoder *encoder;
> > +       struct drm_bridge *bridge;
> > +       int ret;
> > +
> > +       encoder = &simple->encoder;
> > +
> > +       ret = drmm_encoder_init(drm_dev, encoder, NULL, simple->priv->encoder_type, NULL);
> > +       if (ret)
> > +               return ret;
> > +
> > +       drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> > +
> > +       encoder->possible_crtcs =
> > +                       drm_of_find_possible_crtcs(drm_dev, dev->of_node);
> > +
> > +       /* output port is port1*/
> > +       bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > +       if (IS_ERR(bridge))
> > +               return 0;
> > +
> > +       return drm_bridge_attach(encoder, bridge, NULL, 0);
> > +}
> > +
> > +static const struct component_ops encoder_component_ops = {
> > +       .bind = encoder_bind,
> > +};
> > +
> > +static const struct of_device_id simple_encoder_dt_match[] = {
> > +       { .compatible = "starfive,dsi-encoder", .data = &dsi_priv},
> > +       {},
> > +};
> > +MODULE_DEVICE_TABLE(of, simple_encoder_dt_match);
> > +
> > +static int encoder_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct simple_encoder *simple;
> > +       int ret;
> > +
> > +       simple = devm_kzalloc(dev, sizeof(*simple), GFP_KERNEL);
> > +       if (!simple)
> > +               return -ENOMEM;
> > +
> > +       simple->priv = of_device_get_match_data(dev);
> > +
> > +       simple->dev = dev;
> > +
> > +       dev_set_drvdata(dev, simple);
> > +
> > +       ret = encoder_parse_dt(dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return component_add(dev, &encoder_component_ops);
> > +}
> > +
> > +static int encoder_remove(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +
> > +       component_del(dev, &encoder_component_ops);
> > +       dev_set_drvdata(dev, NULL);
> > +
> > +       return 0;
> > +}
> > +
> > +struct platform_driver simple_encoder_driver = {
> > +       .probe = encoder_probe,
> > +       .remove = encoder_remove,
> > +       .driver = {
> > +               .name = "vs-simple-encoder",
> > +               .of_match_table = of_match_ptr(simple_encoder_dt_match),
> > +       },
> > +};
> > +
> > +MODULE_DESCRIPTION("Simple Encoder Driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/gpu/drm/verisilicon/vs_simple_enc.h b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > new file mode 100644
> > index 000000000000..fb33ca9e18d6
> > --- /dev/null
> > +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
> > + */
> > +
> > +#ifndef __VS_SIMPLE_ENC_H_
> > +#define __VS_SIMPLE_ENC_H_
> > +
> > +struct simple_encoder_priv {
> > +       unsigned char encoder_type;
> > +};
> > +
> > +struct simple_encoder {
> > +       struct drm_encoder encoder;
> > +       struct device *dev;
> > +       const struct simple_encoder_priv *priv;
> > +       struct regmap *dss_regmap;
> > +       unsigned int offset;
> > +       unsigned int mask;
> > +};
> > +
> > +extern struct platform_driver simple_encoder_driver;
> > +#endif /* __VS_SIMPLE_ENC_H_ */
> > --
> > 2.34.1
> >
>
>
> --
> With best wishes
> Dmitry

Keith Zhao May 15, 2024, 10:07 a.m. UTC | #3

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: 2023年12月5日 21:19
> To: Keith Zhao <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Xingyu Wu
> <[e-mail]>; Jack Zhu <[e-mail]>;
> [e-mail]; [e-mail]; William Qiu
> <[e-mail]>; Shengyang Chen
> <[e-mail]>; Changhuang Liang
> <[e-mail]>
> Subject: Re: [v3 6/6] drm/vs: simple encoder
> 
> On Tue, 5 Dec 2023 at 15:14, Dmitry Baryshkov <[e-mail]>
> wrote:
> >
> > On Mon, 4 Dec 2023 at 14:33, Keith Zhao <[e-mail]>
> wrote:
> > >
> > > add simple encoder for dsi bridge
> >
> > This doesn't look like a proper commit message.
> >
> > >
> > > Signed-off-by: Keith Zhao <[e-mail]>
> > > ---
> > >  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

https://elixir.bootlin.com/linux/v6.6.30/source/drivers/gpu/drm/mxsfb/lcdif_kms.c#L457
Do the similar logic

Best wish
> >
> > > +
> > > +       if (IS_ERR(bridge_state)) {
> > > +               if (connector->display_info.num_bus_formats)
> > > +                       vs_crtc_state->output_fmt =
> connector->display_info.bus_formats[0];
> > > +               else
> > > +                       vs_crtc_state->output_fmt =
> MEDIA_BUS_FMT_FIXED;
> > > +       } else {
> > > +               vs_crtc_state->output_fmt =
> bridge_state->input_bus_cfg.format;
> > > +       }
> > > +
> > > +       switch (vs_crtc_state->output_fmt) {
> > > +       case MEDIA_BUS_FMT_FIXED:
> > > +       case MEDIA_BUS_FMT_RGB565_1X16:
> > > +       case MEDIA_BUS_FMT_RGB666_1X18:
> > > +       case MEDIA_BUS_FMT_RGB888_1X24:
> > > +       case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
> > > +       case MEDIA_BUS_FMT_RGB101010_1X30:
> > > +       case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
> > > +       case MEDIA_BUS_FMT_UYVY8_1X16:
> > > +       case MEDIA_BUS_FMT_YUV8_1X24:
> > > +       case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
> > > +       case MEDIA_BUS_FMT_UYVY10_1X20:
> > > +       case MEDIA_BUS_FMT_YUV10_1X30:
> > > +               ret = 0;
> > > +               break;
> > > +       default:
> > > +               ret = -EINVAL;
> > > +               break;
> > > +       }
> > > +
> > > +       /* If MEDIA_BUS_FMT_FIXED, set it to default value */
> > > +       if (vs_crtc_state->output_fmt == MEDIA_BUS_FMT_FIXED)
> > > +               vs_crtc_state->output_fmt =
> > > + MEDIA_BUS_FMT_RGB888_1X24;
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> > > +       .atomic_check = encoder_atomic_check,
> > > +       .atomic_enable = encoder_atomic_enable, };
> > > +
> > > +static int encoder_bind(struct device *dev, struct device *master,
> > > +void *data) {
> > > +       struct drm_device *drm_dev = data;
> > > +       struct simple_encoder *simple = dev_get_drvdata(dev);
> > > +       struct drm_encoder *encoder;
> > > +       struct drm_bridge *bridge;
> > > +       int ret;
> > > +
> > > +       encoder = &simple->encoder;
> > > +
> > > +       ret = drmm_encoder_init(drm_dev, encoder, NULL,
> simple->priv->encoder_type, NULL);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> > > +
> > > +       encoder->possible_crtcs =
> > > +                       drm_of_find_possible_crtcs(drm_dev,
> > > + dev->of_node);
> > > +
> > > +       /* output port is port1*/
> > > +       bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
> > > +       if (IS_ERR(bridge))
> > > +               return 0;
> > > +
> > > +       return drm_bridge_attach(encoder, bridge, NULL, 0); }
> > > +
> > > +static const struct component_ops encoder_component_ops = {
> > > +       .bind = encoder_bind,
> > > +};
> > > +
> > > +static const struct of_device_id simple_encoder_dt_match[] = {
> > > +       { .compatible = "starfive,dsi-encoder", .data = &dsi_priv},
> > > +       {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, simple_encoder_dt_match);
> > > +
> > > +static int encoder_probe(struct platform_device *pdev) {
> > > +       struct device *dev = &pdev->dev;
> > > +       struct simple_encoder *simple;
> > > +       int ret;
> > > +
> > > +       simple = devm_kzalloc(dev, sizeof(*simple), GFP_KERNEL);
> > > +       if (!simple)
> > > +               return -ENOMEM;
> > > +
> > > +       simple->priv = of_device_get_match_data(dev);
> > > +
> > > +       simple->dev = dev;
> > > +
> > > +       dev_set_drvdata(dev, simple);
> > > +
> > > +       ret = encoder_parse_dt(dev);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       return component_add(dev, &encoder_component_ops); }
> > > +
> > > +static int encoder_remove(struct platform_device *pdev) {
> > > +       struct device *dev = &pdev->dev;
> > > +
> > > +       component_del(dev, &encoder_component_ops);
> > > +       dev_set_drvdata(dev, NULL);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +struct platform_driver simple_encoder_driver = {
> > > +       .probe = encoder_probe,
> > > +       .remove = encoder_remove,
> > > +       .driver = {
> > > +               .name = "vs-simple-encoder",
> > > +               .of_match_table =
> of_match_ptr(simple_encoder_dt_match),
> > > +       },
> > > +};
> > > +
> > > +MODULE_DESCRIPTION("Simple Encoder Driver");
> MODULE_LICENSE("GPL");
> > > diff --git a/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > > b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > > new file mode 100644
> > > index 000000000000..fb33ca9e18d6
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > > @@ -0,0 +1,23 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
> > > + */
> > > +
> > > +#ifndef __VS_SIMPLE_ENC_H_
> > > +#define __VS_SIMPLE_ENC_H_
> > > +
> > > +struct simple_encoder_priv {
> > > +       unsigned char encoder_type;
> > > +};
> > > +
> > > +struct simple_encoder {
> > > +       struct drm_encoder encoder;
> > > +       struct device *dev;
> > > +       const struct simple_encoder_priv *priv;
> > > +       struct regmap *dss_regmap;
> > > +       unsigned int offset;
> > > +       unsigned int mask;
> > > +};
> > > +
> > > +extern struct platform_driver simple_encoder_driver; #endif /*
> > > +__VS_SIMPLE_ENC_H_ */
> > > --
> > > 2.34.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
> 
> 
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov May 15, 2024, 3:17 p.m. UTC | #4

On Wed, May 15, 2024 at 10:07:27AM +0000, Keith Zhao wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: 2023年12月5日 21:19
> > To: Keith Zhao <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Xingyu Wu
> > <[e-mail]>; Jack Zhu <[e-mail]>;
> > [e-mail]; [e-mail]; William Qiu
> > <[e-mail]>; Shengyang Chen
> > <[e-mail]>; Changhuang Liang
> > <[e-mail]>
> > Subject: Re: [v3 6/6] drm/vs: simple encoder
> > 
> > On Tue, 5 Dec 2023 at 15:14, Dmitry Baryshkov <[e-mail]>
> > wrote:
> > >
> > > On Mon, 4 Dec 2023 at 14:33, Keith Zhao <[e-mail]>
> > wrote:
> > > >
> > > > add simple encoder for dsi bridge
> > >
> > > This doesn't look like a proper commit message.
> > >
> > > >
> > > > Signed-off-by: Keith Zhao <[e-mail]>
> > > > ---
> > > >  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

Keith Zhao May 16, 2024, 2:57 a.m. UTC | #5

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: 2024年5月15日 23:17
> To: Keith Zhao <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Xingyu Wu
> <[e-mail]>; Jack Zhu <[e-mail]>;
> [e-mail]; [e-mail]; William Qiu
> <[e-mail]>; Shengyang Chen
> <[e-mail]>; Changhuang Liang
> <[e-mail]>
> 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 <[e-mail]>
> > > Sent: 2023年12月5日 21:19
> > > To: Keith Zhao <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; Xingyu Wu
> > > <[e-mail]>; Jack Zhu <[e-mail]>;
> > > [e-mail]; [e-mail]; William Qiu
> > > <[e-mail]>; Shengyang Chen
> > > <[e-mail]>; Changhuang Liang
> > > <[e-mail]>
> > > Subject: Re: [v3 6/6] drm/vs: simple encoder
> > >
> > > On Tue, 5 Dec 2023 at 15:14, Dmitry Baryshkov
> > > <[e-mail]>
> > > wrote:
> > > >
> > > > On Mon, 4 Dec 2023 at 14:33, Keith Zhao
> > > > <[e-mail]>
> > > wrote:
> > > > >
> > > > > add simple encoder for dsi bridge
> > > >
> > > > This doesn't look like a proper commit message.
> > > >
> > > > >
> > > > > Signed-off-by: Keith Zhao <[e-mail]>
> > > > > ---
> > > > >  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

Patch

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 = {
+	.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);
+
+	if (IS_ERR(bridge_state)) {
+		if (connector->display_info.num_bus_formats)
+			vs_crtc_state->output_fmt = connector->display_info.bus_formats[0];
+		else
+			vs_crtc_state->output_fmt = MEDIA_BUS_FMT_FIXED;
+	} else {
+		vs_crtc_state->output_fmt = bridge_state->input_bus_cfg.format;
+	}
+
+	switch (vs_crtc_state->output_fmt) {
+	case MEDIA_BUS_FMT_FIXED:
+	case MEDIA_BUS_FMT_RGB565_1X16:
+	case MEDIA_BUS_FMT_RGB666_1X18:
+	case MEDIA_BUS_FMT_RGB888_1X24:
+	case MEDIA_BUS_FMT_RGB666_1X24_CPADHI:
+	case MEDIA_BUS_FMT_RGB101010_1X30:
+	case MEDIA_BUS_FMT_UYYVYY8_0_5X24:
+	case MEDIA_BUS_FMT_UYVY8_1X16:
+	case MEDIA_BUS_FMT_YUV8_1X24:
+	case MEDIA_BUS_FMT_UYYVYY10_0_5X30:
+	case MEDIA_BUS_FMT_UYVY10_1X20:
+	case MEDIA_BUS_FMT_YUV10_1X30:
+		ret = 0;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	/* If MEDIA_BUS_FMT_FIXED, set it to default value */
+	if (vs_crtc_state->output_fmt == MEDIA_BUS_FMT_FIXED)
+		vs_crtc_state->output_fmt = MEDIA_BUS_FMT_RGB888_1X24;
+
+	return ret;
+}
+
+static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
+	.atomic_check = encoder_atomic_check,
+	.atomic_enable = encoder_atomic_enable,
+};
+
+static int encoder_bind(struct device *dev, struct device *master, void *data)
+{
+	struct drm_device *drm_dev = data;
+	struct simple_encoder *simple = dev_get_drvdata(dev);
+	struct drm_encoder *encoder;
+	struct drm_bridge *bridge;
+	int ret;
+
+	encoder = &simple->encoder;
+
+	ret = drmm_encoder_init(drm_dev, encoder, NULL, simple->priv->encoder_type, NULL);
+	if (ret)
+		return ret;
+
+	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
+
+	encoder->possible_crtcs =
+			drm_of_find_possible_crtcs(drm_dev, dev->of_node);
+
+	/* output port is port1*/
+	bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0);
+	if (IS_ERR(bridge))
+		return 0;
+
+	return drm_bridge_attach(encoder, bridge, NULL, 0);
+}
+
+static const struct component_ops encoder_component_ops = {
+	.bind = encoder_bind,
+};
+
+static const struct of_device_id simple_encoder_dt_match[] = {
+	{ .compatible = "starfive,dsi-encoder", .data = &dsi_priv},
+	{},
+};
+MODULE_DEVICE_TABLE(of, simple_encoder_dt_match);
+
+static int encoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct simple_encoder *simple;
+	int ret;
+
+	simple = devm_kzalloc(dev, sizeof(*simple), GFP_KERNEL);
+	if (!simple)
+		return -ENOMEM;
+
+	simple->priv = of_device_get_match_data(dev);
+
+	simple->dev = dev;
+
+	dev_set_drvdata(dev, simple);
+
+	ret = encoder_parse_dt(dev);
+	if (ret)
+		return ret;
+
+	return component_add(dev, &encoder_component_ops);
+}
+
+static int encoder_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	component_del(dev, &encoder_component_ops);
+	dev_set_drvdata(dev, NULL);
+
+	return 0;
+}
+
+struct platform_driver simple_encoder_driver = {
+	.probe = encoder_probe,
+	.remove = encoder_remove,
+	.driver = {
+		.name = "vs-simple-encoder",
+		.of_match_table = of_match_ptr(simple_encoder_dt_match),
+	},
+};
+
+MODULE_DESCRIPTION("Simple Encoder Driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/verisilicon/vs_simple_enc.h b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
new file mode 100644
index 000000000000..fb33ca9e18d6
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
@@ -0,0 +1,23 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
+ */
+
+#ifndef __VS_SIMPLE_ENC_H_
+#define __VS_SIMPLE_ENC_H_
+
+struct simple_encoder_priv {
+	unsigned char encoder_type;
+};
+
+struct simple_encoder {
+	struct drm_encoder encoder;
+	struct device *dev;
+	const struct simple_encoder_priv *priv;
+	struct regmap *dss_regmap;
+	unsigned int offset;
+	unsigned int mask;
+};
+
+extern struct platform_driver simple_encoder_driver;
+#endif /* __VS_SIMPLE_ENC_H_ */