[v4,10/10] drm/vs: add simple dsi encoder

Commit Message

Keith Zhao May 21, 2024, 10:58 a.m. UTC

add encoder to match cdns dsi driver

Signed-off-by: keith <[e-mail]>
---
 drivers/gpu/drm/verisilicon/Makefile        |   3 +-
 drivers/gpu/drm/verisilicon/vs_drv.c        |   1 +
 drivers/gpu/drm/verisilicon/vs_drv.h        |   1 +
 drivers/gpu/drm/verisilicon/vs_simple_enc.c | 190 ++++++++++++++++++++
 drivers/gpu/drm/verisilicon/vs_simple_enc.h |  25 +++
 5 files changed, 219 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 May 21, 2024, 3:25 p.m. UTC | #1

On Tue, May 21, 2024 at 06:58:17PM +0800, keith wrote:
> add encoder to match cdns dsi driver
> 
> Signed-off-by: keith <[e-mail]>

Please fix your git configuration to include your full name into the
S-o-B and Author fields.

> ---
>  drivers/gpu/drm/verisilicon/Makefile        |   3 +-
>  drivers/gpu/drm/verisilicon/vs_drv.c        |   1 +
>  drivers/gpu/drm/verisilicon/vs_drv.h        |   1 +
>  drivers/gpu/drm/verisilicon/vs_simple_enc.c | 190 ++++++++++++++++++++
>  drivers/gpu/drm/verisilicon/vs_simple_enc.h |  25 +++
>  5 files changed, 219 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 2d02b4a3a567..c35ba9bd6f81 100644
> --- a/drivers/gpu/drm/verisilicon/Makefile
> +++ b/drivers/gpu/drm/verisilicon/Makefile
> @@ -4,7 +4,8 @@ vs_drm-objs := vs_dc_hw.o \
>  		vs_modeset.o \
>  		vs_plane.o \
>  		vs_crtc.o \
> -		vs_drv.o
> +		vs_drv.o \
> +		vs_simple_enc.o
>  
>  vs_drm-$(CONFIG_DRM_INNO_STARFIVE_HDMI) += inno_hdmi-starfive.o
>  obj-$(CONFIG_DRM_VERISILICON_DC8200) += vs_drm.o
> diff --git a/drivers/gpu/drm/verisilicon/vs_drv.c b/drivers/gpu/drm/verisilicon/vs_drv.c
> index 6f04102b05b3..2748d48f2c7e 100644
> --- a/drivers/gpu/drm/verisilicon/vs_drv.c
> +++ b/drivers/gpu/drm/verisilicon/vs_drv.c
> @@ -612,6 +612,7 @@ static struct platform_driver *drm_sub_drivers[] = {
>  #ifdef CONFIG_DRM_INNO_STARFIVE_HDMI
>  	&starfive_hdmi_driver,
>  #endif
> +	&simple_encoder_driver,
>  };
>  
>  static struct component_match *vs_add_external_components(struct device *dev)
> diff --git a/drivers/gpu/drm/verisilicon/vs_drv.h b/drivers/gpu/drm/verisilicon/vs_drv.h
> index c3c08ed5f8ac..f3f0f170777d 100644
> --- a/drivers/gpu/drm/verisilicon/vs_drv.h
> +++ b/drivers/gpu/drm/verisilicon/vs_drv.h
> @@ -17,6 +17,7 @@
>  #include <drm/drm_managed.h>
>  
>  #include "vs_dc_hw.h"
> +#include "vs_simple_enc.h"
>  
>  /*@pitch_alignment: buffer pitch alignment required by sub-devices.*/
>  struct vs_drm_device {
> 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..d0b1755d77d2
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.c
> @@ -0,0 +1,190 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 VeriSilicon Holdings Co., Ltd.

Now it is 2024, so the copyright should probably cover the range.

> + */
> +#include <linux/component.h>
> +#include <linux/of_device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_of.h>
> +
> +#include "vs_crtc.h"
> +#include "vs_simple_enc.h"
> +
> +static const struct simple_encoder_priv dsi_priv = {
> +	.encoder_type = DRM_MODE_ENCODER_DSI

So, is it 'simple' aka something generic or DSI? In the latter case,
please rename it accordingly.

> +};
> +
> +static inline struct vs_simple_encoder *to_simple_encoder(struct drm_encoder *enc)
> +{
> +	return container_of(enc, struct vs_simple_encoder, encoder);
> +}
> +
> +static int encoder_parse_dt(struct device *dev)
> +{
> +	struct vs_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];

Is the value that you've read platform dependent or use case dependent?
What is the actual value being written? Why are you using syscon for it?

> +
> +	return 0;
> +}
> +
> +static void vs_encoder_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_state *state)
> +{
> +	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> +
> +	regmap_update_bits(simple->dss_regmap, simple->offset, simple->mask, simple->mask);


A purist in me would ask to have separate mask and value to write.

> +}

Is it necessary to clear those bits when stopping the stream?


[skipped the rest]

> +
> +
> +struct platform_driver simple_encoder_driver = {
> +	.probe = vs_encoder_probe,
> +	.remove = vs_encoder_remove,
> +	.driver = {
> +		.name = "vs-simple-encoder",
> +		.of_match_table = of_match_ptr(simple_encoder_dt_match),
> +	},
> +};
> +
> +MODULE_DESCRIPTION("Simple Encoder Driver");

VeriSilicon DSI Encoder

> +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..73e356bfeb2c
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
> + */
> +
> +#ifndef __VS_SIMPLE_ENC_H_
> +#define __VS_SIMPLE_ENC_H_
> +
> +#include <drm/drm_encoder.h>
> +
> +struct simple_encoder_priv {
> +	unsigned char encoder_type;
> +};
> +
> +struct vs_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;
> +};

Is there a need for aheader for the encoder? Can you move the
definitions to the source file?

> +
> +extern struct platform_driver simple_encoder_driver;
> +#endif /* __VS_SIMPLE_ENC_H_ */
> -- 
> 2.27.0
>

Maxime Ripard May 22, 2024, 7:29 a.m. UTC | #2

Hi,

On Tue, May 21, 2024 at 06:58:17PM GMT, keith wrote:
> add encoder to match cdns dsi driver
> 
> Signed-off-by: keith <[e-mail]>
> ---
>  drivers/gpu/drm/verisilicon/Makefile        |   3 +-
>  drivers/gpu/drm/verisilicon/vs_drv.c        |   1 +
>  drivers/gpu/drm/verisilicon/vs_drv.h        |   1 +
>  drivers/gpu/drm/verisilicon/vs_simple_enc.c | 190 ++++++++++++++++++++
>  drivers/gpu/drm/verisilicon/vs_simple_enc.h |  25 +++
>  5 files changed, 219 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 2d02b4a3a567..c35ba9bd6f81 100644
> --- a/drivers/gpu/drm/verisilicon/Makefile
> +++ b/drivers/gpu/drm/verisilicon/Makefile
> @@ -4,7 +4,8 @@ vs_drm-objs := vs_dc_hw.o \
>  		vs_modeset.o \
>  		vs_plane.o \
>  		vs_crtc.o \
> -		vs_drv.o
> +		vs_drv.o \
> +		vs_simple_enc.o
>  
>  vs_drm-$(CONFIG_DRM_INNO_STARFIVE_HDMI) += inno_hdmi-starfive.o
>  obj-$(CONFIG_DRM_VERISILICON_DC8200) += vs_drm.o
> diff --git a/drivers/gpu/drm/verisilicon/vs_drv.c b/drivers/gpu/drm/verisilicon/vs_drv.c
> index 6f04102b05b3..2748d48f2c7e 100644
> --- a/drivers/gpu/drm/verisilicon/vs_drv.c
> +++ b/drivers/gpu/drm/verisilicon/vs_drv.c
> @@ -612,6 +612,7 @@ static struct platform_driver *drm_sub_drivers[] = {
>  #ifdef CONFIG_DRM_INNO_STARFIVE_HDMI
>  	&starfive_hdmi_driver,
>  #endif
> +	&simple_encoder_driver,
>  };
>  
>  static struct component_match *vs_add_external_components(struct device *dev)
> diff --git a/drivers/gpu/drm/verisilicon/vs_drv.h b/drivers/gpu/drm/verisilicon/vs_drv.h
> index c3c08ed5f8ac..f3f0f170777d 100644
> --- a/drivers/gpu/drm/verisilicon/vs_drv.h
> +++ b/drivers/gpu/drm/verisilicon/vs_drv.h
> @@ -17,6 +17,7 @@
>  #include <drm/drm_managed.h>
>  
>  #include "vs_dc_hw.h"
> +#include "vs_simple_enc.h"
>  
>  /*@pitch_alignment: buffer pitch alignment required by sub-devices.*/
>  struct vs_drm_device {
> 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..d0b1755d77d2
> --- /dev/null
> +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.c
> @@ -0,0 +1,190 @@
> +// 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 <linux/regmap.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_of.h>
> +
> +#include "vs_crtc.h"
> +#include "vs_simple_enc.h"
> +
> +static const struct simple_encoder_priv dsi_priv = {
> +	.encoder_type = DRM_MODE_ENCODER_DSI
> +};

A simple encoder is a thing in KMS, and it's not what you are doing /
using. Please use a different name.

> +static inline struct vs_simple_encoder *to_simple_encoder(struct drm_encoder *enc)
> +{
> +	return container_of(enc, struct vs_simple_encoder, encoder);
> +}
> +
> +static int encoder_parse_dt(struct device *dev)
> +{
> +	struct vs_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;
> +}
> +
> +static void vs_encoder_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_state *state)
> +{
> +	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> +
> +	regmap_update_bits(simple->dss_regmap, simple->offset, simple->mask, simple->mask);
> +}

That should be handled through cdns_dsi_platform_ops.enable.

> +static int vs_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;
> +
> +	vs_crtc_state->encoder_type = encoder->encoder_type;
> +	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;
> +
> +	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;
> +}

And that should be handled by the core cdns-dsi driver.

Maxime

> +static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> +	.atomic_check = vs_encoder_atomic_check,
> +	.atomic_enable = vs_encoder_atomic_enable,
> +};
> +
> +static int vs_encoder_bind(struct device *dev, struct device *master, void *data)
> +{
> +	struct drm_device *drm_dev = data;
> +	struct vs_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)) {
> +		if (PTR_ERR(bridge) == -ENODEV) {
> +			bridge = NULL;
> +			return 0;
> +		}
> +
> +		return PTR_ERR(bridge);
> +	}
> +
> +	return drm_bridge_attach(encoder, bridge, NULL, 0);
> +}
> +
> +static const struct component_ops encoder_component_ops = {
> +	.bind = vs_encoder_bind,
> +};
> +
> +static int vs_encoder_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct vs_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 vs_encoder_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	component_del(dev, &encoder_component_ops);
> +	dev_set_drvdata(dev, NULL);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id simple_encoder_dt_match[] = {
> +	{ .compatible = "starfive,dsi-encoder", .data = &dsi_priv},
> +	{},
> +};

You also don't need a specific compatible here, just reuse cdns,dsi.

Maxime

Keith Zhao June 23, 2024, 7:17 a.m. UTC | #3

Hi Dmitry:

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: 2024年5月21日 23:25
> To: Keith Zhao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Xingyu Wu
> <[e-mail]>; [e-mail]; Jack Zhu
> <[e-mail]>; Shengyang Chen
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v4 10/10] drm/vs: add simple dsi encoder
> 
> On Tue, May 21, 2024 at 06:58:17PM +0800, keith wrote:
> > add encoder to match cdns dsi driver
> >
> > Signed-off-by: keith <[e-mail]>
> 
> Please fix your git configuration to include your full name into the S-o-B and
> Author fields.
Ok ,will fix it
> 
> > ---
> >  drivers/gpu/drm/verisilicon/Makefile        |   3 +-
> >  drivers/gpu/drm/verisilicon/vs_drv.c        |   1 +
> >  drivers/gpu/drm/verisilicon/vs_drv.h        |   1 +
> >  drivers/gpu/drm/verisilicon/vs_simple_enc.c | 190
> > ++++++++++++++++++++  drivers/gpu/drm/verisilicon/vs_simple_enc.h |
> > 25 +++
> >  5 files changed, 219 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 2d02b4a3a567..c35ba9bd6f81 100644
> > --- a/drivers/gpu/drm/verisilicon/Makefile
> > +++ b/drivers/gpu/drm/verisilicon/Makefile
> > @@ -4,7 +4,8 @@ vs_drm-objs := vs_dc_hw.o \
> >  		vs_modeset.o \
> >  		vs_plane.o \
> >  		vs_crtc.o \
> > -		vs_drv.o
> > +		vs_drv.o \
> > +		vs_simple_enc.o
> >
> >  vs_drm-$(CONFIG_DRM_INNO_STARFIVE_HDMI) += inno_hdmi-starfive.o
> >  obj-$(CONFIG_DRM_VERISILICON_DC8200) += vs_drm.o diff --git
> > a/drivers/gpu/drm/verisilicon/vs_drv.c
> > b/drivers/gpu/drm/verisilicon/vs_drv.c
> > index 6f04102b05b3..2748d48f2c7e 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_drv.c
> > +++ b/drivers/gpu/drm/verisilicon/vs_drv.c
> > @@ -612,6 +612,7 @@ static struct platform_driver *drm_sub_drivers[] =
> > {  #ifdef CONFIG_DRM_INNO_STARFIVE_HDMI
> >  	&starfive_hdmi_driver,
> >  #endif
> > +	&simple_encoder_driver,
> >  };
> >
> >  static struct component_match *vs_add_external_components(struct
> > device *dev) diff --git a/drivers/gpu/drm/verisilicon/vs_drv.h
> > b/drivers/gpu/drm/verisilicon/vs_drv.h
> > index c3c08ed5f8ac..f3f0f170777d 100644
> > --- a/drivers/gpu/drm/verisilicon/vs_drv.h
> > +++ b/drivers/gpu/drm/verisilicon/vs_drv.h
> > @@ -17,6 +17,7 @@
> >  #include <drm/drm_managed.h>
> >
> >  #include "vs_dc_hw.h"
> > +#include "vs_simple_enc.h"
> >
> >  /*@pitch_alignment: buffer pitch alignment required by sub-devices.*/
> > struct vs_drm_device { 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..d0b1755d77d2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.c
> > @@ -0,0 +1,190 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 VeriSilicon Holdings Co., Ltd.
> 
> Now it is 2024, so the copyright should probably cover the range.
Ok ,will fix it
> 
> > + */
> > +#include <linux/component.h>
> > +#include <linux/of_device.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/media-bus-format.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_of.h>
> > +
> > +#include "vs_crtc.h"
> > +#include "vs_simple_enc.h"
> > +
> > +static const struct simple_encoder_priv dsi_priv = {
> > +	.encoder_type = DRM_MODE_ENCODER_DSI
> 
> So, is it 'simple' aka something generic or DSI? In the latter case, please rename
> it accordingly.
Ok will done 
> 
> > +};
> > +
> > +static inline struct vs_simple_encoder *to_simple_encoder(struct
> > +drm_encoder *enc) {
> > +	return container_of(enc, struct vs_simple_encoder, encoder); }
> > +
> > +static int encoder_parse_dt(struct device *dev) {
> > +	struct vs_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];
> 
> Is the value that you've read platform dependent or use case dependent?
> What is the actual value being written? Why are you using syscon for it?

The syscon is used to select crtcs binded with encoder,
If this encoder binds to crtc0 , set the syscon reg bit0 = 1
If this encoder binds to crtc1 , set the syscon reg bit1 = 1 (0x2)
Maybe I can do this by the possible_crtc instead of using args from dts


> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void vs_encoder_atomic_enable(struct drm_encoder *encoder,
> > +struct drm_atomic_state *state) {
> > +	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> > +
> > +	regmap_update_bits(simple->dss_regmap, simple->offset, simple->mask,
> > +simple->mask);
> 
> 
> A purist in me would ask to have separate mask and value to write.
Understand , will avoid this action 
> 
> > +}
> 
> Is it necessary to clear those bits when stopping the stream?
No need to do this , if clear those bits , the encoder will point to a unknown crtc
> 
> 
> [skipped the rest]
> 
> > +
> > +
> > +struct platform_driver simple_encoder_driver = {
> > +	.probe = vs_encoder_probe,
> > +	.remove = vs_encoder_remove,
> > +	.driver = {
> > +		.name = "vs-simple-encoder",
> > +		.of_match_table = of_match_ptr(simple_encoder_dt_match),
> > +	},
> > +};
> > +
> > +MODULE_DESCRIPTION("Simple Encoder Driver");
> 
> VeriSilicon DSI Encoder
Ok  will done
> 
> > +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..73e356bfeb2c
> > --- /dev/null
> > +++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
> > @@ -0,0 +1,25 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
> > + */
> > +
> > +#ifndef __VS_SIMPLE_ENC_H_
> > +#define __VS_SIMPLE_ENC_H_
> > +
> > +#include <drm/drm_encoder.h>
> > +
> > +struct simple_encoder_priv {
> > +	unsigned char encoder_type;
> > +};
> > +
> > +struct vs_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;
> > +};
> 
> Is there a need for aheader for the encoder? Can you move the definitions to
> the source file?
Ok will done 
> 
> > +
> > +extern struct platform_driver simple_encoder_driver; #endif /*
> > +__VS_SIMPLE_ENC_H_ */
> > --
> > 2.27.0
> >
> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov June 23, 2024, 9:11 p.m. UTC | #4

On Sun, Jun 23, 2024 at 07:17:09AM GMT, Keith Zhao wrote:
> Hi Dmitry:
> 
> > On Tue, May 21, 2024 at 06:58:17PM +0800, keith wrote:

> > > +								  "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];
> > 
> > Is the value that you've read platform dependent or use case dependent?
> > What is the actual value being written? Why are you using syscon for it?
> 
> The syscon is used to select crtcs binded with encoder,
> If this encoder binds to crtc0 , set the syscon reg bit0 = 1
> If this encoder binds to crtc1 , set the syscon reg bit1 = 1 (0x2)
> Maybe I can do this by the possible_crtc instead of using args from dts

If this is a constant between your platforms, it should not be a part of
DT.

> 
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void vs_encoder_atomic_enable(struct drm_encoder *encoder,
> > > +struct drm_atomic_state *state) {
> > > +	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> > > +
> > > +	regmap_update_bits(simple->dss_regmap, simple->offset, simple->mask,
> > > +simple->mask);
> > 
> > 
> > A purist in me would ask to have separate mask and value to write.
> Understand , will avoid this action 
> > 
> > > +}
> > 
> > Is it necessary to clear those bits when stopping the stream?
> No need to do this , if clear those bits , the encoder will point to a unknown crtc

what are the consequences? Is it desirable or not?

Keith Zhao June 25, 2024, 8:33 a.m. UTC | #5

> -----Original Message-----
> From: Dmitry Baryshkov <[e-mail]>
> Sent: 2024年6月24日 5:11
> To: Keith Zhao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; Xingyu Wu
> <[e-mail]>; [e-mail]; Jack Zhu
> <[e-mail]>; Shengyang Chen
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v4 10/10] drm/vs: add simple dsi encoder
> 
> On Sun, Jun 23, 2024 at 07:17:09AM GMT, Keith Zhao wrote:
> > Hi Dmitry:
> >
> > > On Tue, May 21, 2024 at 06:58:17PM +0800, keith wrote:
> 
> > > > +								  "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];
> > >
> > > Is the value that you've read platform dependent or use case dependent?
> > > What is the actual value being written? Why are you using syscon for it?
> >
> > The syscon is used to select crtcs binded with encoder, If this
> > encoder binds to crtc0 , set the syscon reg bit0 = 1 If this encoder
> > binds to crtc1 , set the syscon reg bit1 = 1 (0x2) Maybe I can do this
> > by the possible_crtc instead of using args from dts
> 
> If this is a constant between your platforms, it should not be a part of DT.
> 
> >
> >
> > >
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void vs_encoder_atomic_enable(struct drm_encoder *encoder,
> > > > +struct drm_atomic_state *state) {
> > > > +	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> > > > +
> > > > +	regmap_update_bits(simple->dss_regmap, simple->offset,
> > > > +simple->mask,
> > > > +simple->mask);
> > >
> > >
> > > A purist in me would ask to have separate mask and value to write.
> > Understand , will avoid this action
> > >
> > > > +}
> > >
> > > Is it necessary to clear those bits when stopping the stream?
> > No need to do this , if clear those bits , the encoder will point to a
> > unknown crtc
> 
> what are the consequences? Is it desirable or not?
There are two crtcs.
Each display terminal encoder can combine any crtc, depending on the value of possible crtc.
When the bit is 0, it means that the encoder matches crtc0.
When the bit is 1, it means that the encoder matches crtc1.
The possible crtc of this encoder is 2 , the reg bit is 1.    
When the video stream is stopped, if the bit is cleared, the result is that the encoder hardware points to crtc0, 
and the encoder points to crtc1 based on the drm framework(because the possible crtc no change).

> 
> --
> With best wishes
> Dmitry

Dmitry Baryshkov June 25, 2024, 10:59 a.m. UTC | #6

On Tue, Jun 25, 2024 at 08:33:48AM GMT, Keith Zhao wrote:
> 
> 
> > -----Original Message-----
> > From: Dmitry Baryshkov <[e-mail]>
> > Sent: 2024年6月24日 5:11
> > To: Keith Zhao <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; Xingyu Wu
> > <[e-mail]>; [e-mail]; Jack Zhu
> > <[e-mail]>; Shengyang Chen
> > <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]
> > Subject: Re: [PATCH v4 10/10] drm/vs: add simple dsi encoder
> > 
> > On Sun, Jun 23, 2024 at 07:17:09AM GMT, Keith Zhao wrote:
> > > Hi Dmitry:
> > >
> > > > On Tue, May 21, 2024 at 06:58:17PM +0800, keith wrote:
> > 
> > > > > +								  "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];
> > > >
> > > > Is the value that you've read platform dependent or use case dependent?
> > > > What is the actual value being written? Why are you using syscon for it?
> > >
> > > The syscon is used to select crtcs binded with encoder, If this
> > > encoder binds to crtc0 , set the syscon reg bit0 = 1 If this encoder
> > > binds to crtc1 , set the syscon reg bit1 = 1 (0x2) Maybe I can do this
> > > by the possible_crtc instead of using args from dts
> > 
> > If this is a constant between your platforms, it should not be a part of DT.
> > 
> > >
> > >
> > > >
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static void vs_encoder_atomic_enable(struct drm_encoder *encoder,
> > > > > +struct drm_atomic_state *state) {
> > > > > +	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
> > > > > +
> > > > > +	regmap_update_bits(simple->dss_regmap, simple->offset,
> > > > > +simple->mask,
> > > > > +simple->mask);
> > > >
> > > >
> > > > A purist in me would ask to have separate mask and value to write.
> > > Understand , will avoid this action
> > > >
> > > > > +}
> > > >
> > > > Is it necessary to clear those bits when stopping the stream?
> > > No need to do this , if clear those bits , the encoder will point to a
> > > unknown crtc
> > 
> > what are the consequences? Is it desirable or not?
> There are two crtcs.
> Each display terminal encoder can combine any crtc, depending on the value of possible crtc.
> When the bit is 0, it means that the encoder matches crtc0.
> When the bit is 1, it means that the encoder matches crtc1.
> The possible crtc of this encoder is 2 , the reg bit is 1.    
> When the video stream is stopped, if the bit is cleared, the result is that the encoder hardware points to crtc0, 
> and the encoder points to crtc1 based on the drm framework(because the possible crtc no change).

I'm not sure if I understood you correctly. If it doesn't disable or
disconnect the encoder, I'd skip that in the .disable path.

Patch

diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile
index 2d02b4a3a567..c35ba9bd6f81 100644
--- a/drivers/gpu/drm/verisilicon/Makefile
+++ b/drivers/gpu/drm/verisilicon/Makefile
@@ -4,7 +4,8 @@  vs_drm-objs := vs_dc_hw.o \
 		vs_modeset.o \
 		vs_plane.o \
 		vs_crtc.o \
-		vs_drv.o
+		vs_drv.o \
+		vs_simple_enc.o
 
 vs_drm-$(CONFIG_DRM_INNO_STARFIVE_HDMI) += inno_hdmi-starfive.o
 obj-$(CONFIG_DRM_VERISILICON_DC8200) += vs_drm.o
diff --git a/drivers/gpu/drm/verisilicon/vs_drv.c b/drivers/gpu/drm/verisilicon/vs_drv.c
index 6f04102b05b3..2748d48f2c7e 100644
--- a/drivers/gpu/drm/verisilicon/vs_drv.c
+++ b/drivers/gpu/drm/verisilicon/vs_drv.c
@@ -612,6 +612,7 @@  static struct platform_driver *drm_sub_drivers[] = {
 #ifdef CONFIG_DRM_INNO_STARFIVE_HDMI
 	&starfive_hdmi_driver,
 #endif
+	&simple_encoder_driver,
 };
 
 static struct component_match *vs_add_external_components(struct device *dev)
diff --git a/drivers/gpu/drm/verisilicon/vs_drv.h b/drivers/gpu/drm/verisilicon/vs_drv.h
index c3c08ed5f8ac..f3f0f170777d 100644
--- a/drivers/gpu/drm/verisilicon/vs_drv.h
+++ b/drivers/gpu/drm/verisilicon/vs_drv.h
@@ -17,6 +17,7 @@ 
 #include <drm/drm_managed.h>
 
 #include "vs_dc_hw.h"
+#include "vs_simple_enc.h"
 
 /*@pitch_alignment: buffer pitch alignment required by sub-devices.*/
 struct vs_drm_device {
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..d0b1755d77d2
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.c
@@ -0,0 +1,190 @@ 
+// 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 <linux/regmap.h>
+#include <linux/media-bus-format.h>
+#include <linux/mfd/syscon.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_of.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 vs_simple_encoder *to_simple_encoder(struct drm_encoder *enc)
+{
+	return container_of(enc, struct vs_simple_encoder, encoder);
+}
+
+static int encoder_parse_dt(struct device *dev)
+{
+	struct vs_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;
+}
+
+static void vs_encoder_atomic_enable(struct drm_encoder *encoder, struct drm_atomic_state *state)
+{
+	struct vs_simple_encoder *simple = to_simple_encoder(encoder);
+
+	regmap_update_bits(simple->dss_regmap, simple->offset, simple->mask, simple->mask);
+}
+
+static int vs_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;
+
+	vs_crtc_state->encoder_type = encoder->encoder_type;
+	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;
+
+	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 = vs_encoder_atomic_check,
+	.atomic_enable = vs_encoder_atomic_enable,
+};
+
+static int vs_encoder_bind(struct device *dev, struct device *master, void *data)
+{
+	struct drm_device *drm_dev = data;
+	struct vs_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)) {
+		if (PTR_ERR(bridge) == -ENODEV) {
+			bridge = NULL;
+			return 0;
+		}
+
+		return PTR_ERR(bridge);
+	}
+
+	return drm_bridge_attach(encoder, bridge, NULL, 0);
+}
+
+static const struct component_ops encoder_component_ops = {
+	.bind = vs_encoder_bind,
+};
+
+static int vs_encoder_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct vs_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 vs_encoder_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	component_del(dev, &encoder_component_ops);
+	dev_set_drvdata(dev, NULL);
+
+	return 0;
+}
+
+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);
+
+struct platform_driver simple_encoder_driver = {
+	.probe = vs_encoder_probe,
+	.remove = vs_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..73e356bfeb2c
--- /dev/null
+++ b/drivers/gpu/drm/verisilicon/vs_simple_enc.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 VeriSilicon Holdings Co., Ltd.
+ */
+
+#ifndef __VS_SIMPLE_ENC_H_
+#define __VS_SIMPLE_ENC_H_
+
+#include <drm/drm_encoder.h>
+
+struct simple_encoder_priv {
+	unsigned char encoder_type;
+};
+
+struct vs_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_ */