by Shawn Sung

[permalink] [raw]

Subject: Re: [PATCH v3 07/11] drm/mediatek: Support alpha blending in VDOSYS0

On Thu, 2023-11-23 at 09:22 +0000, CK Hu (胡俊光) wrote:
> Hi, Hsiao-chien:
>

----
snip
----

> > @@ -420,14 +496,92 @@ void mtk_ovl_layer_config(struct device *dev,
> > unsigned int idx,
> >
> > overlay_pitch.pitch = pitch;
> >
> > - if (!pending->enable) {
> > + if (!pending->enable || !pending->width || !pending->height) {
> > mtk_ovl_layer_off(dev, idx, cmdq_pkt);
> > return;
> > }
> >
> > - con = ovl_fmt_convert(ovl, fmt);
> > - if (state->base.fb && state->base.fb->format->has_alpha)
> > - con |= OVL_CON_AEN | OVL_CON_ALPHA;
> > + con = ovl_fmt_convert(ovl, fmt, blend_mode);
> > + if (state->base.fb) {
> > + con |= OVL_CON_AEN;
>
> It seems all SoC ovl hardware support alpha blending, why in mmsys
> just
> some support?

Because alpha blending was not implemented before, and we only verified
this patch on MT8195 and MT8188. Will review this part again. Thanks.

> > + con |= state->base.alpha & 0xff;
> > + }
> > +
> > + if (blend_mode == DRM_MODE_BLEND_PIXEL_NONE ||
> > + (state->base.fb && !state->base.fb->format->has_alpha))
> > + ignore_pixel_alpha = OVL_CONST_BLEND;
> > +
> > + /* need to do Y2R and R2R to reduce 10bit data to 8bit for CRC
> > calculation */
>
> Reducing 10 bit to 8 bit would lose color detail. Just for CRC to
> lose
> this detail? Why not just use 10 bit to calculate CRC?
>

This is a hardware limitation, OVL will only retrieve 8 bits per
channel for CRC calculation. Although we do limit this function to
8195/8188 only, the color detail will still lose on these platforms if
we must support CRC.

Regards,
Shawn

2023-11-30 01:20:18

by CK Hu (胡俊光)

[permalink] [raw]

Subject: Re: [PATCH v3 10/11] drm/mediatek: Support CRC in VDOSYS0

Hi, Hsiao-chien:

Change the title to Support CRC in ovl.

On Tue, 2023-10-17 at 14:47 +0800, Hsiao Chien Sung wrote:
> We choose OVL as CRC generator from other hardware
> components that are also capable of calculating CRCs,
> since its frame done event triggers vblanks, it can be
> used as a signal to know when is safe to retrieve CRC of
> the frame.
>
> Please note that position of the hardware component
> that is chosen as CRC generator in the display path is
> significant. For example, while OVL is the first module
> in VDOSYS0, its CRC won't be affected by the modules
> after it, which means effects applied by PQ, Gamma,
> Dither or any other components after OVL won't be
> calculated in CRC generation.
>
> Signed-off-by: Hsiao Chien Sung <[email protected]>
> ---
> drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 111
> ++++++++++++++++++--
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 3 +
> 3 files changed, 109 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index c44f5b31bab5..08cc2d2fef9f 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -102,6 +102,9 @@ void mtk_ovl_enable_vblank(struct device *dev);
> void mtk_ovl_disable_vblank(struct device *dev);
> const u32 *mtk_ovl_get_formats(struct device *dev);
> size_t mtk_ovl_get_num_formats(struct device *dev);
> +size_t mtk_ovl_crc_cnt(struct device *dev);
> +u32 *mtk_ovl_crc_entry(struct device *dev);
> +void mtk_ovl_crc_read(struct device *dev);
>
> void mtk_ovl_adaptor_add_comp(struct device *dev, struct mtk_mutex
> *mutex);
> void mtk_ovl_adaptor_remove_comp(struct device *dev, struct
> mtk_mutex *mutex);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 75de1350e337..ecc38932fd44 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -25,6 +25,13 @@
> #define OVL_FME_CPL_INT BIT(1)
> #define DISP_REG_OVL_INTSTA 0x0008
> #define DISP_REG_OVL_EN 0x000c
> +#define OVL_EN BIT(0)
> +#define OVL_OP_8BIT_MODE BIT(4)
> +#define OVL_HG_FOVL_CK_ON BIT(8)
> +#define OVL_HF_FOVL_CK_ON BIT(10)
> +#define DISP_REG_OVL_TRIG 0x0010
> +#define OVL_CRC_EN BIT(8)
> +#define OVL_CRC_CLR BIT(9)
> #define DISP_REG_OVL_RST 0x0014
> #define DISP_REG_OVL_ROI_SIZE 0x0020
> #define DISP_REG_OVL_DATAPATH_CON 0x0024
> @@ -44,6 +51,8 @@
> #define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
> #define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
> #define DISP_REG_OVL_ADDR_MT2701 0x0040
> +#define DISP_REG_OVL_CRC 0x0270
> +#define OVL_CRC_OUT_MASK GENMASK(30, 0)
> #define DISP_REG_OVL_CLRFMT_EXT 0x02D0
> #define DISP_REG_OVL_CLRFMT_EXT1 0x02D8
> #define OVL_CLRFMT_EXT1_CSC_EN(n) (1 << (((n) *
> 4) + 1))
> @@ -151,6 +160,24 @@ static const u32 mt8195_formats[] = {
> DRM_FORMAT_YUYV,
> };
>
> +static const u32 mt8195_ovl_crc_ofs[] = {
> + DISP_REG_OVL_CRC,
> +};
> +
> +/**
> + * struct mtk_disp_ovl_data - ovl driver data
> + * @addr: offset of the first layer (layer-0)
> + * @gmc_bits: gmc (gating memory clock) bit masks for adjusting
> positivity for ovl
> + * @layer_nr: layer numbers that ovl supports
> + * @fmt_rgb565_is_0: whether or not rgb565 is represented as 0
> + * @smi_id_en: determine if smi needs to be enabled
> + * @supports_afbc: determine if ovl supports afbc
> + * @formats: format table that ovl supports
> + * @num_formats: number of formats that ovl supports
> + * @supports_clrfmt_ext: whether the ovl supports clear format (for
> alpha blend)

This is not related to crc, so move to another patch.

> + * @crc_ofs: crc offset table
> + * @crc_cnt: count of crc registers (could be more than one bank)
> + */
> struct mtk_disp_ovl_data {
> unsigned int addr;
> unsigned int gmc_bits;
> @@ -161,12 +188,20 @@ struct mtk_disp_ovl_data {
> const u32 *formats;
> size_t num_formats;
> bool supports_clrfmt_ext;
> + const u32 *crc_ofs;
> + size_t crc_cnt;
> };
>
> -/*
> +/**
> * struct mtk_disp_ovl - DISP_OVL driver structure
> * @crtc: associated crtc to report vblank events to
> + * @clk: clock of the ovl
> + * @regs: base address of the ovl register that can be accessed by
> cpu
> + * @cmdq_reg: register related info for cmdq (subsys, offset
> ...etc.)
> * @data: platform data
> + * @vblank_cb: callback function when vblank irq happened
> + * @vblank_cb_data: data to the callback function
> + * @crc: crc related information
> */
> struct mtk_disp_ovl {
> struct drm_crtc *crtc;
> @@ -176,8 +211,30 @@ struct mtk_disp_ovl {
> const struct mtk_disp_ovl_data *data;
> void (*vblank_cb)(void *data);
> void *vblank_cb_data;
> + struct mtk_drm_crc crc;
> };
>
> +size_t mtk_ovl_crc_cnt(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + return ovl->crc.cnt;
> +}
> +
> +u32 *mtk_ovl_crc_entry(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + return ovl->crc.va;
> +}
> +
> +void mtk_ovl_crc_read(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + mtk_drm_crc_read(&ovl->crc, ovl->regs);
> +}
> +
> static irqreturn_t mtk_disp_ovl_irq_handler(int irq, void *dev_id)
> {
> struct mtk_disp_ovl *priv = dev_id;
> @@ -216,7 +273,7 @@ void mtk_ovl_enable_vblank(struct device *dev)
> struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>
> writel(0x0, ovl->regs + DISP_REG_OVL_INTSTA);
> - writel_relaxed(OVL_FME_CPL_INT, ovl->regs +
> DISP_REG_OVL_INTEN);
> + writel(OVL_FME_CPL_INT, ovl->regs + DISP_REG_OVL_INTEN);

This is changing writel_relaxed() to writel(), not crc. So move to
another patch.

> }
>
> void mtk_ovl_disable_vblank(struct device *dev)
> @@ -269,14 +326,30 @@ void mtk_ovl_start(struct device *dev)
> */
> reg |= OVL_OUTPUT_CLAMP;
>
> - writel_relaxed(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);
> - writel_relaxed(0x1, ovl->regs + DISP_REG_OVL_EN);
> + writel(reg, ovl->regs + DISP_REG_OVL_DATAPATH_CON);

This is changing writel_relaxed() to writel(), not crc. So move to
another patch.

Regards,
CK

> +
> + reg = OVL_EN;
> +
> + if (ovl->data->crc_cnt) {
> + /* enable crc */
> + writel(OVL_CRC_EN, ovl->regs + DISP_REG_OVL_TRIG);
> + /* enable crc related clocks */
> + reg |= OVL_OP_8BIT_MODE | OVL_HG_FOVL_CK_ON |
> OVL_HF_FOVL_CK_ON;
> + }
> + writel(reg, ovl->regs + DISP_REG_OVL_EN);
> +
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> + mtk_drm_crc_cmdq_start(&ovl->crc);
> +#endif
> }
>
> void mtk_ovl_stop(struct device *dev)
> {
> struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
>
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> + mtk_drm_crc_cmdq_stop(&ovl->crc);
> +#endif
> writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_EN);
> if (ovl->data->smi_id_en) {
> unsigned int reg;
> @@ -689,15 +762,31 @@ static int mtk_disp_ovl_probe(struct
> platform_device *pdev)
> dev_err(dev, "failed to ioremap ovl\n");
> return PTR_ERR(priv->regs);
> }
> +
> + priv->data = of_device_get_match_data(dev);
> + platform_set_drvdata(pdev, priv);
> +
> + if (priv->data->crc_cnt) {
> + mtk_drm_crc_init(&priv->crc,
> + priv->data->crc_ofs, priv->data-
> >crc_cnt,
> + DISP_REG_OVL_TRIG, OVL_CRC_CLR);
> + }
> +
> #if IS_REACHABLE(CONFIG_MTK_CMDQ)
> ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
> if (ret)
> dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
> -#endif
> -
> - priv->data = of_device_get_match_data(dev);
> - platform_set_drvdata(pdev, priv);
>
> + if (priv->data->crc_cnt) {
> + if (of_property_read_u32_index(dev->of_node,
> + "mediatek,gce-events",
> 0,
> + &priv->crc.cmdq_event))
> {
> + dev_warn(dev, "failed to get gce-events for
> crc\n");
> + }
> + priv->crc.cmdq_reg = &priv->cmdq_reg;
> + mtk_drm_crc_cmdq_create(dev, &priv->crc);
> + }
> +#endif
> ret = devm_request_irq(dev, irq, mtk_disp_ovl_irq_handler,
> IRQF_TRIGGER_NONE, dev_name(dev), priv);
> if (ret < 0) {
> @@ -718,6 +807,10 @@ static int mtk_disp_ovl_probe(struct
> platform_device *pdev)
>
> static void mtk_disp_ovl_remove(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + mtk_drm_crc_destroy(&ovl->crc);
> component_del(&pdev->dev, &mtk_disp_ovl_component_ops);
> pm_runtime_disable(&pdev->dev);
> }
> @@ -788,6 +881,8 @@ static const struct mtk_disp_ovl_data
> mt8195_ovl_driver_data = {
> .formats = mt8195_formats,
> .num_formats = ARRAY_SIZE(mt8195_formats),
> .supports_clrfmt_ext = true,
> + .crc_ofs = mt8195_ovl_crc_ofs,
> + .crc_cnt = ARRAY_SIZE(mt8195_ovl_crc_ofs),
> };
>
> static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 9940909c7435..1118efcde98a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -349,6 +349,9 @@ static const struct mtk_ddp_comp_funcs ddp_ovl =
> {
> .clk_enable = mtk_ovl_clk_enable,
> .clk_disable = mtk_ovl_clk_disable,
> .config = mtk_ovl_config,
> + .crc_cnt = mtk_ovl_crc_cnt,
> + .crc_entry = mtk_ovl_crc_entry,
> + .crc_read = mtk_ovl_crc_read,
> .start = mtk_ovl_start,
> .stop = mtk_ovl_stop,
> .register_vblank_cb = mtk_ovl_register_vblank_cb,