by Dmitry Baryshkov

[permalink] [raw]

Subject: Re: [v3 6/6] drm/vs: simple encoder

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

--
With best wishes
Dmitry

2023-12-06 08:57:03

by Maxime Ripard

[permalink] [raw]

Subject: Re: [v3 4/6] drm/vs: Add KMS crtc&plane

On Mon, Dec 04, 2023 at 08:33:13PM +0800, Keith Zhao wrote:
> +static const struct vs_plane_info dc_hw_planes_rev0[PLANE_NUM] = {
> + {
> + .name = "Primary",
> + .id = PRIMARY_PLANE_0,
> + .type = DRM_PLANE_TYPE_PRIMARY,
> + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> + .formats = primary_overlay_format0,
> + .num_modifiers = ARRAY_SIZE(format_modifier0),
> + .modifiers = format_modifier0,
> + .min_width = 0,
> + .min_height = 0,
> + .max_width = 4096,
> + .max_height = 4096,
> + .rotation = DRM_MODE_ROTATE_0 |
> + DRM_MODE_ROTATE_90 |
> + DRM_MODE_ROTATE_180 |
> + DRM_MODE_ROTATE_270 |
> + DRM_MODE_REFLECT_X |
> + DRM_MODE_REFLECT_Y,
> + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE),
> + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> + BIT(DRM_COLOR_YCBCR_BT2020),
> + .degamma_size = DEGAMMA_SIZE,
> + .min_scale = FRAC_16_16(1, 3),
> + .max_scale = FRAC_16_16(10, 1),
> + .zpos = 0,
> + .watermark = true,
> + .color_mgmt = true,
> + .roi = true,
> + },
> + {
> + .name = "Overlay",
> + .id = OVERLAY_PLANE_0,
> + .type = DRM_PLANE_TYPE_OVERLAY,
> + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> + .formats = primary_overlay_format0,
> + .num_modifiers = ARRAY_SIZE(format_modifier0),
> + .modifiers = format_modifier0,
> + .min_width = 0,
> + .min_height = 0,
> + .max_width = 4096,
> + .max_height = 4096,
> + .rotation = DRM_MODE_ROTATE_0 |
> + DRM_MODE_ROTATE_90 |
> + DRM_MODE_ROTATE_180 |
> + DRM_MODE_ROTATE_270 |
> + DRM_MODE_REFLECT_X |
> + DRM_MODE_REFLECT_Y,
> + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE),
> + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> + BIT(DRM_COLOR_YCBCR_BT2020),
> + .degamma_size = DEGAMMA_SIZE,
> + .min_scale = FRAC_16_16(1, 3),
> + .max_scale = FRAC_16_16(10, 1),
> + .zpos = 1,
> + .watermark = true,
> + .color_mgmt = true,
> + .roi = true,
> + },
> + {
> + .name = "Overlay_1",
> + .id = OVERLAY_PLANE_1,
> + .type = DRM_PLANE_TYPE_OVERLAY,
> + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> + .formats = primary_overlay_format0,
> + .num_modifiers = ARRAY_SIZE(secondary_format_modifiers),
> + .modifiers = secondary_format_modifiers,
> + .min_width = 0,
> + .min_height = 0,
> + .max_width = 4096,
> + .max_height = 4096,
> + .rotation = 0,
> + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE),
> + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> + BIT(DRM_COLOR_YCBCR_BT2020),
> + .degamma_size = DEGAMMA_SIZE,
> + .min_scale = DRM_PLANE_NO_SCALING,
> + .max_scale = DRM_PLANE_NO_SCALING,
> + .zpos = 2,
> + .watermark = true,
> + .color_mgmt = true,
> + .roi = true,
> + },
> + {
> + .name = "Primary_1",
> + .id = PRIMARY_PLANE_1,
> + .type = DRM_PLANE_TYPE_PRIMARY,
> + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> + .formats = primary_overlay_format0,
> + .num_modifiers = ARRAY_SIZE(format_modifier0),
> + .modifiers = format_modifier0,
> + .min_width = 0,
> + .min_height = 0,
> + .max_width = 4096,
> + .max_height = 4096,
> + .rotation = DRM_MODE_ROTATE_0 |
> + DRM_MODE_ROTATE_90 |
> + DRM_MODE_ROTATE_180 |
> + DRM_MODE_ROTATE_270 |
> + DRM_MODE_REFLECT_X |
> + DRM_MODE_REFLECT_Y,
> + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE),
> + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> + BIT(DRM_COLOR_YCBCR_BT2020),
> + .degamma_size = DEGAMMA_SIZE,
> + .min_scale = FRAC_16_16(1, 3),
> + .max_scale = FRAC_16_16(10, 1),
> + .zpos = 3,
> + .watermark = true,
> + .color_mgmt = true,
> + .roi = true,
> + },
> + {
> + .name = "Overlay_2",
> + .id = OVERLAY_PLANE_2,
> + .type = DRM_PLANE_TYPE_OVERLAY,
> + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> + .formats = primary_overlay_format0,
> + .num_modifiers = ARRAY_SIZE(format_modifier0),
> + .modifiers = format_modifier0,
> + .min_width = 0,
> + .min_height = 0,
> + .max_width = 4096,
> + .max_height = 4096,
> + .rotation = DRM_MODE_ROTATE_0 |
> + DRM_MODE_ROTATE_90 |
> + DRM_MODE_ROTATE_180 |
> + DRM_MODE_ROTATE_270 |
> + DRM_MODE_REFLECT_X |
> + DRM_MODE_REFLECT_Y,
> + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE),
> + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> + BIT(DRM_COLOR_YCBCR_BT2020),
> + .degamma_size = DEGAMMA_SIZE,
> + .min_scale = FRAC_16_16(1, 3),
> + .max_scale = FRAC_16_16(10, 1),
> + .zpos = 4,
> + .watermark = true,
> + .color_mgmt = true,
> + .roi = true,
> + },
> + {
> + .name = "Overlay_3",
> + .id = OVERLAY_PLANE_3,
> + .type = DRM_PLANE_TYPE_OVERLAY,
> + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> + .formats = primary_overlay_format0,
> + .num_modifiers = ARRAY_SIZE(secondary_format_modifiers),
> + .modifiers = secondary_format_modifiers,
> + .min_width = 0,
> + .min_height = 0,
> + .max_width = 4096,
> + .max_height = 4096,
> + .rotation = 0,
> + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> + BIT(DRM_MODE_BLEND_PREMULTI) |
> + BIT(DRM_MODE_BLEND_COVERAGE),
> + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> + BIT(DRM_COLOR_YCBCR_BT2020),
> + .degamma_size = DEGAMMA_SIZE,
> + .min_scale = DRM_PLANE_NO_SCALING,
> + .max_scale = DRM_PLANE_NO_SCALING,
> + .zpos = 5,
> + .watermark = true,
> + .color_mgmt = true,
> + .roi = true,
> + },
> + {
> + .name = "Cursor",
> + .id = CURSOR_PLANE_0,
> + .type = DRM_PLANE_TYPE_CURSOR,
> + .num_formats = ARRAY_SIZE(cursor_formats),
> + .formats = cursor_formats,
> + .num_modifiers = 0,
> + .modifiers = NULL,
> + .min_width = 32,
> + .min_height = 32,
> + .max_width = 64,
> + .max_height = 64,
> + .rotation = 0,
> + .degamma_size = 0,
> + .min_scale = DRM_PLANE_NO_SCALING,
> + .max_scale = DRM_PLANE_NO_SCALING,
> + .zpos = 255,
> + .watermark = false,
> + .color_mgmt = false,
> + .roi = false,
> + },
> + {
> + .name = "Cursor_1",
> + .id = CURSOR_PLANE_1,
> + .type = DRM_PLANE_TYPE_CURSOR,
> + .num_formats = ARRAY_SIZE(cursor_formats),
> + .formats = cursor_formats,
> + .num_modifiers = 0,
> + .modifiers = NULL,
> + .min_width = 32,
> + .min_height = 32,
> + .max_width = 64,
> + .max_height = 64,
> + .rotation = 0,
> + .degamma_size = 0,
> + .min_scale = DRM_PLANE_NO_SCALING,
> + .max_scale = DRM_PLANE_NO_SCALING,
> + .zpos = 255,
> + .watermark = false,
> + .color_mgmt = false,
> + .roi = false,
> + },
> +};
> +
> +static const struct vs_dc_info dc8200_info = {
> + .name = "DC8200",
> + .panel_num = 2,
> + .plane_num = 8,
> + .planes = dc_hw_planes_rev0,
> + .layer_num = 6,
> + .max_bpc = 10,
> + .color_formats = DRM_COLOR_FORMAT_RGB444 |
> + DRM_COLOR_FORMAT_YCBCR444 |
> + DRM_COLOR_FORMAT_YCBCR422 |
> + DRM_COLOR_FORMAT_YCBCR420,
> + .gamma_size = GAMMA_EX_SIZE,
> + .gamma_bits = 12,
> + .pitch_alignment = 128,
> + .pipe_sync = false,
> + .background = true,
> + .panel_sync = true,
> + .cap_dec = true,
> +};

I really think that entire thing is to workaround a suboptimal device
tree binding. You should have two CRTCs in the device tree, you'll probe
twice, and you won't get to do that whole dance.

> +struct dc_hw_plane_reg {
> + u32 y_address;
> + u32 u_address;
> + u32 v_address;
> + u32 y_stride;
> + u32 u_stride;
> + u32 v_stride;
> + u32 size;
> + u32 top_left;
> + u32 bottom_right;
> + u32 scale_factor_x;
> + u32 scale_factor_y;
> + u32 h_filter_coef_index;
> + u32 h_filter_coef_data;
> + u32 v_filter_coef_index;
> + u32 v_filter_coef_data;
> + u32 init_offset;
> + u32 color_key;
> + u32 color_key_high;
> + u32 clear_value;
> + u32 color_table_index;
> + u32 color_table_data;
> + u32 scale_config;
> + u32 water_mark;
> + u32 degamma_index;
> + u32 degamma_data;
> + u32 degamma_ex_data;
> + u32 src_global_color;
> + u32 dst_global_color;
> + u32 blend_config;
> + u32 roi_origin;
> + u32 roi_size;
> + u32 yuv_to_rgb_coef0;
> + u32 yuv_to_rgb_coef1;
> + u32 yuv_to_rgb_coef2;
> + u32 yuv_to_rgb_coef3;
> + u32 yuv_to_rgb_coef4;
> + u32 yuv_to_rgb_coefd0;
> + u32 yuv_to_rgb_coefd1;
> + u32 yuv_to_rgb_coefd2;
> + u32 y_clamp_bound;
> + u32 uv_clamp_bound;
> + u32 rgb_to_rgb_coef0;
> + u32 rgb_to_rgb_coef1;
> + u32 rgb_to_rgb_coef2;
> + u32 rgb_to_rgb_coef3;
> + u32 rgb_to_rgb_coef4;
> +};

That's your plane state.

> +struct dc_hw_fb {
> + u32 y_address;
> + u32 u_address;
> + u32 v_address;
> + u32 clear_value;
> + u32 water_mark;
> + u16 y_stride;
> + u16 u_stride;
> + u16 v_stride;
> + u16 width;
> + u16 height;
> + u8 format;
> + u8 tile_mode;
> + u8 rotation;
> + u8 yuv_color_space;
> + u8 swizzle;
> + u8 uv_swizzle;
> + u8 zpos;
> + u8 display_id;
> + bool clear_enable;
> + bool dec_enable;
> + bool enable;
> + bool dirty;
> +};

Your framebuffer

> +struct dc_hw_scale {
> + u32 scale_factor_x;
> + u32 scale_factor_y;
> + bool enable;
> + bool dirty;
> +};
> +
> +struct dc_hw_position {
> + u16 start_x;
> + u16 start_y;
> + u16 end_x;
> + u16 end_y;
> + bool dirty;
> +};
> +
> +struct dc_hw_blend {
> + u8 alpha;
> + u8 blend_mode;
> + bool dirty;
> +};
> +
> +struct dc_hw_colorkey {
> + u32 colorkey;
> + u32 colorkey_high;
> + u8 transparency;
> + bool dirty;
> +};

Your CRTC state.

> +struct dc_hw_roi {
> + u16 x;
> + u16 y;
> + u16 width;
> + u16 height;
> + bool enable;
> + bool dirty;
> +};
> +
> +struct dc_hw_cursor {
> + u32 address;
> + u16 x;
> + u16 y;
> + u16 hot_x;
> + u16 hot_y;
> + u8 size;
> + u8 display_id;
> + bool enable;
> + bool dirty;
> +};
> +
> +struct dc_hw_display {
> + u32 bus_format;
> + u16 h_active;
> + u16 h_total;
> + u16 h_sync_start;
> + u16 h_sync_end;
> + u16 v_active;
> + u16 v_total;
> + u16 v_sync_start;
> + u16 v_sync_end;
> + u8 id;
> + bool h_sync_polarity;
> + bool v_sync_polarity;
> + bool enable;
> +};

Your display mode.

Really, you have the huge majority of those informations already
available, there's no need to duplicate it. And chances are you'll
create bugs in the process.
Maxime

Attachments:

(No filename) (10.71 kB)
signature.asc (235.00 B)
Download all attachments

2023-12-06 09:05:37

Hi Keith：

在 2023-12-08 11:00:31，"Keith Zhao" <[email protected]> 写道：
>
>
>On 2023/12/8 8:37, Andy Yan wrote:
>> Hi Keth：
>>
>>
>>
>>
>>
>>
>> 在 2023-12-07 18:48:13，"Keith Zhao" <[email protected]> 写道：
>>>
>>>
>>>On 2023/12/7 17:02, Andy Yan wrote:
>>>>
>>>>
>>>>
>>>>
>>>> Hi Keith：
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> At 2023-12-06 22:11:33, "Keith Zhao" <[email protected]> wrote:
>>>>>
>>>>>
>>>>>On 2023/12/6 20:56, Maxime Ripard wrote:
>>>>>> On Wed, Dec 06, 2023 at 08:02:55PM +0800, Keith Zhao wrote:
>>>>>>> >> +static const struct of_device_id starfive_hdmi_dt_ids[] = {
>>>>>>> >> + { .compatible = "starfive,jh7110-inno-hdmi",},
>>>>>>> >
>>>>>>> > So it's inno hdmi, just like Rockchip then?
>>>>>>> >
>>>>>>> > This should be a common driver.
>>>>>>>
>>>>>>> Rockchip has a inno hdmi IP. and Starfive has a inno hdmi IP.
>>>>>>> but the harewawre difference of them is big , it is not easy to use the common driver
>>>>>>> maybe i need the inno hdmi version here to make a distinction
>>>>>>
>>>>>> I just had a look at the rockchip header file: all the registers but the
>>>>>> STARFIVE_* ones are identical.
>>>>>>
>>>>>> There's no need to have two identical drivers then, please use the
>>>>>> rockchip driver instead.
>>>>>>
>>>>>> Maxime
>>>>>
>>>>>ok, have a simple test , edid can get . i will continue
>>>>
>>>> Maybe you can take drivers/gpu/drm/bridge/synopsys/dw-hdmi as a reference， this
>>>> is also a hdmi ip used by rockchip/meson/sunxi/jz/imx。
>>>> We finally make it share one driver。
>>>>>
>>>hi Andy:
>>>
>>>dw_hdmi seems a good choice , it can handle inno hdmi hardware by define its dw_hdmi_plat_data.
>>>does it means i can write own driver files such as(dw_hdmi-starfive.c) based on dw_hdmi instead of add plat_data in inno_hdmi.c
>>>
>>
>> I think the process maybe like this：
>>
>> 1. split the inno_hdmi.c under rockchip to inno_hdmi.c(the common part), inno_hdmi-rockchip.c(the soc specific part)
>> 2. move the common part inno_hdmi.c to drivers/gpu/drm/bridge/innosilicon/
>> 3. add startfive specific part, inno_hdmi-startfive.c
>>
>> bellow git log from kernel three show how we convert dw_hdmi to a common driver:
>>
>>
>>
>> 12b9f204e804 drm: bridge/dw_hdmi: add rockchip rk3288 support
>> 74af9e4d03b8 dt-bindings: Add documentation for rockchip dw hdmi
>> d346c14eeea9 drm: bridge/dw_hdmi: add function dw_hdmi_phy_enable_spare
>> a4d3b8b050d5 drm: bridge/dw_hdmi: clear i2cmphy_stat0 reg in hdmi_phy_wait_i2c_done
>> 632d035bace2 drm: bridge/dw_hdmi: add mode_valid support
>> 0cd9d1428322 drm: bridge/dw_hdmi: add support for multi-byte register width access
>> cd152393967e dt-bindings: add document for dw_hdmi
>> b21f4b658df8 drm: imx: imx-hdmi: move imx-hdmi to bridge/dw_hdmi
>> aaa757a092c2 drm: imx: imx-hdmi: split phy configuration to platform driver
>> 3d1b35a3d9f3 drm: imx: imx-hdmi: convert imx-hdmi to drm_bridge mode
>> c2c3848851a7 drm: imx: imx-hdmi: return defer if can't get ddc i2c adapter
>> b587833933de drm: imx: imx-hdmi: make checkpatch happy
>>
>hi Andy:
>I got you means,
>as I don't have a rockchip board on hand , to split the inno_hdmi.c can not be tested.
>
>how adout this idea:
>1、split the starfive_hdmi.c under verisilicion to inno_hdmi.c(the common part), inno_hdmi-starfive.c(the soc specific part)
>2. move the common part inno_hdmi.c to drivers/gpu/drm/bridge/innosilicon/
>3. In the future, inno hdmi.c under rockchip will reuse the public driver.

I am not sure if drm maintainers are happy with this。

To be honest， I also don't have a i.mx board when I start convert dw_hdmi to a common driver,
some respectable people from the community help test and give me many valuable advice， this
is the power of open source。

I found a rk3036 based kylin board this week，but it can't boot yet，I will go on try if
I can boot it this weekend。 I can do the test on rockchip side， if i can make this board work。

>
>>
>>>Thanks for pointing this out!!!
>>>
>>>>>
>>>>>_______________________________________________
>>>>>linux-riscv mailing list
>>>>>[email protected]
>>>>>http://lists.infradead.org/mailman/listinfo/linux-riscv
>>>
>>>_______________________________________________
>>>linux-riscv mailing list
>>>[email protected]
>>>http://lists.infradead.org/mailman/listinfo/linux-riscv

2023-12-08 09:14:48

On Wed, Jan 31, 2024 at 09:33:06AM +0000, Keith Zhao wrote:
>
>
> > -----邮件原件-----
> > 发件人: Maxime Ripard <[email protected]>
> > 发送时间: 2023年12月6日 16:56
> > 收件人: Keith Zhao <[email protected]>
> > 抄送: [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > William Qiu <[email protected]>; Xingyu Wu
> > <[email protected]>; [email protected];
> > [email protected]; [email protected]; [email protected];
> > Shengyang Chen <[email protected]>; Jack Zhu
> > <[email protected]>; Changhuang Liang
> > <[email protected]>; [email protected];
> > [email protected]
> > 主题: Re: [v3 4/6] drm/vs: Add KMS crtc&plane
> >
> > On Mon, Dec 04, 2023 at 08:33:13PM +0800, Keith Zhao wrote:
> > > +static const struct vs_plane_info dc_hw_planes_rev0[PLANE_NUM] = {
> > > + {
> > > + .name = "Primary",
> > > + .id = PRIMARY_PLANE_0,
> > > + .type = DRM_PLANE_TYPE_PRIMARY,
> > > + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> > > + .formats = primary_overlay_format0,
> > > + .num_modifiers = ARRAY_SIZE(format_modifier0),
> > > + .modifiers = format_modifier0,
> > > + .min_width = 0,
> > > + .min_height = 0,
> > > + .max_width = 4096,
> > > + .max_height = 4096,
> > > + .rotation = DRM_MODE_ROTATE_0 |
> > > + DRM_MODE_ROTATE_90 |
> > > + DRM_MODE_ROTATE_180 |
> > > + DRM_MODE_ROTATE_270 |
> > > + DRM_MODE_REFLECT_X |
> > > + DRM_MODE_REFLECT_Y,
> > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > + BIT(DRM_MODE_BLEND_PREMULTI) |
> > > + BIT(DRM_MODE_BLEND_COVERAGE),
> > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> > > + BIT(DRM_COLOR_YCBCR_BT2020),
> > > + .degamma_size = DEGAMMA_SIZE,
> > > + .min_scale = FRAC_16_16(1, 3),
> > > + .max_scale = FRAC_16_16(10, 1),
> > > + .zpos = 0,
> > > + .watermark = true,
> > > + .color_mgmt = true,
> > > + .roi = true,
> > > + },
> > > + {
> > > + .name = "Overlay",
> > > + .id = OVERLAY_PLANE_0,
> > > + .type = DRM_PLANE_TYPE_OVERLAY,
> > > + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> > > + .formats = primary_overlay_format0,
> > > + .num_modifiers = ARRAY_SIZE(format_modifier0),
> > > + .modifiers = format_modifier0,
> > > + .min_width = 0,
> > > + .min_height = 0,
> > > + .max_width = 4096,
> > > + .max_height = 4096,
> > > + .rotation = DRM_MODE_ROTATE_0 |
> > > + DRM_MODE_ROTATE_90 |
> > > + DRM_MODE_ROTATE_180 |
> > > + DRM_MODE_ROTATE_270 |
> > > + DRM_MODE_REFLECT_X |
> > > + DRM_MODE_REFLECT_Y,
> > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > + BIT(DRM_MODE_BLEND_PREMULTI) |
> > > + BIT(DRM_MODE_BLEND_COVERAGE),
> > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> > > + BIT(DRM_COLOR_YCBCR_BT2020),
> > > + .degamma_size = DEGAMMA_SIZE,
> > > + .min_scale = FRAC_16_16(1, 3),
> > > + .max_scale = FRAC_16_16(10, 1),
> > > + .zpos = 1,
> > > + .watermark = true,
> > > + .color_mgmt = true,
> > > + .roi = true,
> > > + },
> > > + {
> > > + .name = "Overlay_1",
> > > + .id = OVERLAY_PLANE_1,
> > > + .type = DRM_PLANE_TYPE_OVERLAY,
> > > + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> > > + .formats = primary_overlay_format0,
> > > + .num_modifiers = ARRAY_SIZE(secondary_format_modifiers),
> > > + .modifiers = secondary_format_modifiers,
> > > + .min_width = 0,
> > > + .min_height = 0,
> > > + .max_width = 4096,
> > > + .max_height = 4096,
> > > + .rotation = 0,
> > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > + BIT(DRM_MODE_BLEND_PREMULTI) |
> > > + BIT(DRM_MODE_BLEND_COVERAGE),
> > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> > > + BIT(DRM_COLOR_YCBCR_BT2020),
> > > + .degamma_size = DEGAMMA_SIZE,
> > > + .min_scale = DRM_PLANE_NO_SCALING,
> > > + .max_scale = DRM_PLANE_NO_SCALING,
> > > + .zpos = 2,
> > > + .watermark = true,
> > > + .color_mgmt = true,
> > > + .roi = true,
> > > + },
> > > + {
> > > + .name = "Primary_1",
> > > + .id = PRIMARY_PLANE_1,
> > > + .type = DRM_PLANE_TYPE_PRIMARY,
> > > + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> > > + .formats = primary_overlay_format0,
> > > + .num_modifiers = ARRAY_SIZE(format_modifier0),
> > > + .modifiers = format_modifier0,
> > > + .min_width = 0,
> > > + .min_height = 0,
> > > + .max_width = 4096,
> > > + .max_height = 4096,
> > > + .rotation = DRM_MODE_ROTATE_0 |
> > > + DRM_MODE_ROTATE_90 |
> > > + DRM_MODE_ROTATE_180 |
> > > + DRM_MODE_ROTATE_270 |
> > > + DRM_MODE_REFLECT_X |
> > > + DRM_MODE_REFLECT_Y,
> > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > + BIT(DRM_MODE_BLEND_PREMULTI) |
> > > + BIT(DRM_MODE_BLEND_COVERAGE),
> > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> > > + BIT(DRM_COLOR_YCBCR_BT2020),
> > > + .degamma_size = DEGAMMA_SIZE,
> > > + .min_scale = FRAC_16_16(1, 3),
> > > + .max_scale = FRAC_16_16(10, 1),
> > > + .zpos = 3,
> > > + .watermark = true,
> > > + .color_mgmt = true,
> > > + .roi = true,
> > > + },
> > > + {
> > > + .name = "Overlay_2",
> > > + .id = OVERLAY_PLANE_2,
> > > + .type = DRM_PLANE_TYPE_OVERLAY,
> > > + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> > > + .formats = primary_overlay_format0,
> > > + .num_modifiers = ARRAY_SIZE(format_modifier0),
> > > + .modifiers = format_modifier0,
> > > + .min_width = 0,
> > > + .min_height = 0,
> > > + .max_width = 4096,
> > > + .max_height = 4096,
> > > + .rotation = DRM_MODE_ROTATE_0 |
> > > + DRM_MODE_ROTATE_90 |
> > > + DRM_MODE_ROTATE_180 |
> > > + DRM_MODE_ROTATE_270 |
> > > + DRM_MODE_REFLECT_X |
> > > + DRM_MODE_REFLECT_Y,
> > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > + BIT(DRM_MODE_BLEND_PREMULTI) |
> > > + BIT(DRM_MODE_BLEND_COVERAGE),
> > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> > > + BIT(DRM_COLOR_YCBCR_BT2020),
> > > + .degamma_size = DEGAMMA_SIZE,
> > > + .min_scale = FRAC_16_16(1, 3),
> > > + .max_scale = FRAC_16_16(10, 1),
> > > + .zpos = 4,
> > > + .watermark = true,
> > > + .color_mgmt = true,
> > > + .roi = true,
> > > + },
> > > + {
> > > + .name = "Overlay_3",
> > > + .id = OVERLAY_PLANE_3,
> > > + .type = DRM_PLANE_TYPE_OVERLAY,
> > > + .num_formats = ARRAY_SIZE(primary_overlay_format0),
> > > + .formats = primary_overlay_format0,
> > > + .num_modifiers = ARRAY_SIZE(secondary_format_modifiers),
> > > + .modifiers = secondary_format_modifiers,
> > > + .min_width = 0,
> > > + .min_height = 0,
> > > + .max_width = 4096,
> > > + .max_height = 4096,
> > > + .rotation = 0,
> > > + .blend_mode = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> > > + BIT(DRM_MODE_BLEND_PREMULTI) |
> > > + BIT(DRM_MODE_BLEND_COVERAGE),
> > > + .color_encoding = BIT(DRM_COLOR_YCBCR_BT709) |
> > > + BIT(DRM_COLOR_YCBCR_BT2020),
> > > + .degamma_size = DEGAMMA_SIZE,
> > > + .min_scale = DRM_PLANE_NO_SCALING,
> > > + .max_scale = DRM_PLANE_NO_SCALING,
> > > + .zpos = 5,
> > > + .watermark = true,
> > > + .color_mgmt = true,
> > > + .roi = true,
> > > + },
> > > + {
> > > + .name = "Cursor",
> > > + .id = CURSOR_PLANE_0,
> > > + .type = DRM_PLANE_TYPE_CURSOR,
> > > + .num_formats = ARRAY_SIZE(cursor_formats),
> > > + .formats = cursor_formats,
> > > + .num_modifiers = 0,
> > > + .modifiers = NULL,
> > > + .min_width = 32,
> > > + .min_height = 32,
> > > + .max_width = 64,
> > > + .max_height = 64,
> > > + .rotation = 0,
> > > + .degamma_size = 0,
> > > + .min_scale = DRM_PLANE_NO_SCALING,
> > > + .max_scale = DRM_PLANE_NO_SCALING,
> > > + .zpos = 255,
> > > + .watermark = false,
> > > + .color_mgmt = false,
> > > + .roi = false,
> > > + },
> > > + {
> > > + .name = "Cursor_1",
> > > + .id = CURSOR_PLANE_1,
> > > + .type = DRM_PLANE_TYPE_CURSOR,
> > > + .num_formats = ARRAY_SIZE(cursor_formats),
> > > + .formats = cursor_formats,
> > > + .num_modifiers = 0,
> > > + .modifiers = NULL,
> > > + .min_width = 32,
> > > + .min_height = 32,
> > > + .max_width = 64,
> > > + .max_height = 64,
> > > + .rotation = 0,
> > > + .degamma_size = 0,
> > > + .min_scale = DRM_PLANE_NO_SCALING,
> > > + .max_scale = DRM_PLANE_NO_SCALING,
> > > + .zpos = 255,
> > > + .watermark = false,
> > > + .color_mgmt = false,
> > > + .roi = false,
> > > + },
> > > +};
> > > +
> > > +static const struct vs_dc_info dc8200_info = {
> > > + .name = "DC8200",
> > > + .panel_num = 2,
> > > + .plane_num = 8,
> > > + .planes = dc_hw_planes_rev0,
> > > + .layer_num = 6,
> > > + .max_bpc = 10,
> > > + .color_formats = DRM_COLOR_FORMAT_RGB444 |
> > > + DRM_COLOR_FORMAT_YCBCR444 |
> > > + DRM_COLOR_FORMAT_YCBCR422 |
> > > + DRM_COLOR_FORMAT_YCBCR420,
> > > + .gamma_size = GAMMA_EX_SIZE,
> > > + .gamma_bits = 12,
> > > + .pitch_alignment = 128,
> > > + .pipe_sync = false,
> > > + .background = true,
> > > + .panel_sync = true,
> > > + .cap_dec = true,
> > > +};
> >
> > I really think that entire thing is to workaround a suboptimal device tree binding.
> > You should have two CRTCs in the device tree, you'll probe twice, and you won't
> > get to do that whole dance.
> >

> I tried to modify it according to this idea Found it too difficult In
> terms of hardware, the two crtc designs are too close to separate, and
> they are even designed into the same reg with different bits
> representing crtc0 and crtc1. It seems not easy to described the 2
> ctrc hardware by 2 device nodes

What are these bits doing?

> The idea is to avoid a whole dance I don't know if I understand
> correctly about whole dance. Is it means I create 2 ctrc and 8 plane
> in the dc_bind?

Yeah, you should strive to make it two separate devices.

Maxime

Attachments:

(No filename) (10.28 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-31 16:42:46

by Maxime Ripard

[permalink] [raw]

Subject: Re: 回复: [v3 4/6] drm/vs: Add KMS crtc&plane

On Wed, Jan 31, 2024 at 08:57:21AM +0000, Keith Zhao wrote:
> > > +static const struct vs_dc_info dc8200_info = {
> > > + .name = "DC8200",
> > > + .panel_num = 2,
> > > + .plane_num = 8,
> > > + .planes = dc_hw_planes_rev0,
> > > + .layer_num = 6,
> > > + .max_bpc = 10,
> > > + .color_formats = DRM_COLOR_FORMAT_RGB444 |
> > > + DRM_COLOR_FORMAT_YCBCR444 |
> > > + DRM_COLOR_FORMAT_YCBCR422 |
> > > + DRM_COLOR_FORMAT_YCBCR420,
> > > + .gamma_size = GAMMA_EX_SIZE,
> > > + .gamma_bits = 12,
> > > + .pitch_alignment = 128,
> > > + .pipe_sync = false,
> > > + .background = true,
> > > + .panel_sync = true,
> > > + .cap_dec = true,
> > > +};
> >
> > I really think that entire thing is to workaround a suboptimal device tree binding.
> > You should have two CRTCs in the device tree, you'll probe twice, and you won't
> > get to do that whole dance.
> >
> Hi Maxime:
> I tried to modify it according to this idea Found it too difficult In terms of hardware,
> the two crtc designs are too close to separate, and they are even designed into the same reg with different bits representing crtc0 and crtc1.
> It seems not easy to described the 2 ctrc hardware by 2 device nodes
>
> The idea is to avoid a whole dance
> I don't know if I understand correctly about whole dance.
> Is it means I create 2 ctrc and 8 plane in the dc_bind?

It looks like you just sent the same mail?

Maxime

Attachments:

(No filename) (1.42 kB)
signature.asc (235.00 B)
Download all attachments