Re: [PATCH 12/15] drm/mediatek: Support CRC in VDOSYS1

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 23/08/23 17:13, Hsiao Chien Sung ha scritto:
> We choose Mixer as CRC generator in VDOSYS1 since
> its frame done event will trigger vblanks, we can know
> when is safe to retrieve CRC of the frame.
> 
> In VDOSYS1, there's no image procession after Mixer,
> unlike OVL in VDOSYS0, Mixer's CRC will include all the
> effects that are applied to the frame.
> 
> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_disp_drv.h       |   1 +
>   .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   |  10 ++
>   drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c   |   1 +
>   drivers/gpu/drm/mediatek/mtk_ethdr.c          | 120 ++++++++++++++++++
>   drivers/gpu/drm/mediatek/mtk_ethdr.h          |   4 +
>   5 files changed, 136 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index d2753360ae1e..014086d4d7ca 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -127,6 +127,7 @@ unsigned int mtk_ovl_adaptor_layer_nr(struct device *dev);
>   struct device *mtk_ovl_adaptor_dma_dev_get(struct device *dev);
>   const u32 *mtk_ovl_adaptor_get_formats(struct device *dev);
>   size_t mtk_ovl_adaptor_get_num_formats(struct device *dev);
> +u32 mtk_ovl_adaptor_crc_cnt(struct device *dev);
>   
>   void mtk_rdma_bypass_shadow(struct device *dev);
>   int mtk_rdma_clk_enable(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> index c0a38f5217ee..64f98b26f4ce 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -159,6 +159,13 @@ void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx,
>   	mtk_ethdr_layer_config(ethdr, idx, state, cmdq_pkt);
>   }
>   
> +u32 mtk_ovl_adaptor_crc_cnt(struct device *dev)
> +{
> +	struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev);
> +
> +	return mtk_ethdr_crc_cnt(ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_ETHDR0]);
> +}
> +
>   void mtk_ovl_adaptor_config(struct device *dev, unsigned int w,
>   			    unsigned int h, unsigned int vrefresh,
>   			    unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
> @@ -274,6 +281,9 @@ void mtk_ovl_adaptor_register_vblank_cb(struct device *dev, void (*vblank_cb)(vo
>   
>   	mtk_ethdr_register_vblank_cb(ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_ETHDR0],
>   				     vblank_cb, vblank_cb_data);
> +
> +	mtk_ethdr_register_crtc(ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_ETHDR0],
> +				(struct drm_crtc *)vblank_cb_data);
>   }
>   
>   void mtk_ovl_adaptor_unregister_vblank_cb(struct device *dev)
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 1b747a34a06b..143136491607 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -398,6 +398,7 @@ static const struct mtk_ddp_comp_funcs ddp_ovl_adaptor = {
>   	.clk_enable = mtk_ovl_adaptor_clk_enable,
>   	.clk_disable = mtk_ovl_adaptor_clk_disable,
>   	.config = mtk_ovl_adaptor_config,
> +	.crc_cnt = mtk_ovl_adaptor_crc_cnt,
>   	.start = mtk_ovl_adaptor_start,
>   	.stop = mtk_ovl_adaptor_stop,
>   	.layer_nr = mtk_ovl_adaptor_layer_nr,
> diff --git a/drivers/gpu/drm/mediatek/mtk_ethdr.c b/drivers/gpu/drm/mediatek/mtk_ethdr.c
> index 3058c122a4c3..9e341d86d9f9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_ethdr.c
> +++ b/drivers/gpu/drm/mediatek/mtk_ethdr.c
> @@ -24,6 +24,9 @@
>   #define MIX_FME_CPL_INTEN			BIT(1)
>   #define MIX_INTSTA			0x8
>   #define MIX_EN				0xc
> +#define MIX_TRIG			0x10
> +#define MIX_TRIG_CRC_EN				BIT(8)
> +#define MIX_TRIG_CRC_RST			BIT(9)
>   #define MIX_RST				0x14
>   #define MIX_ROI_SIZE			0x18
>   #define MIX_DATAPATH_CON		0x1c
> @@ -39,6 +42,11 @@
>   #define PREMULTI_SOURCE				(3 << 12)
>   #define MIX_L_SRC_SIZE(n)		(0x30 + 0x18 * (n))
>   #define MIX_L_SRC_OFFSET(n)		(0x34 + 0x18 * (n))
> +
> +/* CRC register offsets for odd and even lines */
> +#define MIX_CRC_O			0x110
> +#define MIX_CRC_E			0x114
> +
>   #define MIX_FUNC_DCM0			0x120
>   #define MIX_FUNC_DCM1			0x124
>   #define MIX_FUNC_DCM_ENABLE			0xffffffff
> @@ -70,6 +78,9 @@ struct mtk_ethdr_comp {
>   	struct device		*dev;
>   	void __iomem		*regs;
>   	struct cmdq_client_reg	cmdq_base;
> +	struct cmdq_client *cmdq_client;
> +	struct cmdq_pkt *cmdq_pkt;
> +	u32 cmdq_event;
>   };
>   
>   struct mtk_ethdr {
> @@ -80,6 +91,9 @@ struct mtk_ethdr {
>   	void			*vblank_cb_data;
>   	int			irq;
>   	struct reset_control	*reset_ctl;
> +	struct drm_crtc		*crtc;
> +	const u32		*crcs;
> +	size_t			crc_cnt;
>   };
>   
>   static const char * const ethdr_clk_str[] = {
> @@ -98,6 +112,95 @@ static const char * const ethdr_clk_str[] = {
>   	"vdo_be_async",
>   };
>   
> +static const u32 ethdr_crcs[] = {
> +	MIX_CRC_O,
> +	MIX_CRC_E,
> +};
> +
> +u32 mtk_ethdr_crc_cnt(struct device *dev)
> +{
> +	struct mtk_ethdr *priv = dev_get_drvdata(dev);
> +
> +	return (u32)priv->crc_cnt;
> +}
> +
> +void mtk_ethdr_register_crtc(struct device *dev, struct drm_crtc *crtc)
> +{
> +	struct mtk_ethdr *priv = dev_get_drvdata(dev);
> +
> +	priv->crtc = crtc;
> +}
> +
> +static void mtk_ethdr_crc_loop_start(struct device *dev)
> +{
> +	int i;
> +	struct mtk_ethdr *priv;
> +	struct mtk_ethdr_comp *mixer;
> +	struct mtk_drm_crtc *mtk_crtc;

You can initialize those variables at declaration time, since there's no
check in between initializations.

> +
> +	priv = dev_get_drvdata(dev);
> +	mixer = &priv->ethdr_comp[ETHDR_MIXER];
> +	mtk_crtc = container_of(priv->crtc, struct mtk_drm_crtc, base);
> +
> +	if (!mixer->cmdq_event || mixer->cmdq_client)
> +		return;
> +
> +	mixer->cmdq_client = cmdq_mbox_create(dev, 0);
> +	if (IS_ERR(mixer->cmdq_client)) {
> +		pr_err("failed to create mailbox client\n");

Is there any way to *guarantee* that this won't fail?

> +		return;
> +	}
> +	mixer->cmdq_pkt = cmdq_pkt_create(mixer->cmdq_client, PAGE_SIZE);
> +	if (!mixer->cmdq_pkt) {
> +		pr_err("failed to create cmdq packet\n");
> +		return;
> +	}
> +

Regards,
Angelo