RE: [PATCH 6.1.y-cip 15/20] drm: renesas: Add RZ/G2L DU Support

From: <nobuhiro1.iwamatsu@toshiba.co.jp>
To: <biju.das.jz@bp.renesas.com>, <cip-dev@lists.cip-project.org>,
	<pavel@denx.de>
Cc: <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: RE: [PATCH 6.1.y-cip 15/20] drm: renesas: Add RZ/G2L DU Support
Date: Wed, 6 Sep 2023 00:23:47 +0000	[thread overview]
Message-ID: <TYWPR01MB942095E6CB6A4BE1B9A5A87C92EFA@TYWPR01MB9420.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20230905160737.167877-16-biju.das.jz@bp.renesas.com>

Hi,

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: Wednesday, September 6, 2023 1:08 AM
> To: cip-dev@lists.cip-project.org; iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□
> ＤＩＴ○ＯＳＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; Pavel Machek
> <pavel@denx.de>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>; Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: [PATCH 6.1.y-cip 15/20] drm: renesas: Add RZ/G2L DU Support
> 
> The LCD controller is composed of Frame Compression Processor (FCPVD),
> Video Signal Processor (VSPD), and Display Unit (DU).
> 
> It has DPI/DSI interfaces and supports a maximum resolution of 1080p
> along with 2 RPFs to support the blending of two picture layers and
> raster operations (ROPs).
> 
> The DU module is connected to VSPD. Add RZ/G2L DU support for RZ/G2L
> alike SoCs.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

You forgot to add the original commit ID and Signed-off.
And I could not find this patch in upstream tree. Probably any patch after this one has the same problem.

> ---
>  drivers/gpu/drm/Kconfig                  |   2 +
>  drivers/gpu/drm/Makefile                 |   1 +
>  drivers/gpu/drm/rz-du/Kconfig            |  12 +
>  drivers/gpu/drm/rz-du/Makefile           |   8 +
>  drivers/gpu/drm/rz-du/rzg2l_du_crtc.c    | 599
> +++++++++++++++++++
>  drivers/gpu/drm/rz-du/rzg2l_du_crtc.h    |  92 +++
>  drivers/gpu/drm/rz-du/rzg2l_du_drv.c     | 185 ++++++
>  drivers/gpu/drm/rz-du/rzg2l_du_drv.h     |  84 +++
>  drivers/gpu/drm/rz-du/rzg2l_du_encoder.c | 109 ++++
>  drivers/gpu/drm/rz-du/rzg2l_du_encoder.h |  32 +
>  drivers/gpu/drm/rz-du/rzg2l_du_kms.c     | 711
> +++++++++++++++++++++++
>  drivers/gpu/drm/rz-du/rzg2l_du_kms.h     |  43 ++
>  drivers/gpu/drm/rz-du/rzg2l_du_regs.h    |  67 +++
>  drivers/gpu/drm/rz-du/rzg2l_du_vsp.c     | 469 +++++++++++++++
>  drivers/gpu/drm/rz-du/rzg2l_du_vsp.h     |  97 ++++
>  15 files changed, 2511 insertions(+)
>  create mode 100644 drivers/gpu/drm/rz-du/Kconfig
>  create mode 100644 drivers/gpu/drm/rz-du/Makefile
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_crtc.c
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_crtc.h
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_drv.c
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_drv.h
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_encoder.c
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_encoder.h
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_kms.c
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_kms.h
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_regs.h
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_vsp.c
>  create mode 100644 drivers/gpu/drm/rz-du/rzg2l_du_vsp.h
> 

<snip>

> --
> + * Initialization
> + */
> +
> +int rzg2l_du_crtc_create(struct rzg2l_du_device *rcdu)
> +{
> +	struct rzg2l_du_crtc *rcrtc = &rcdu->crtcs[0];
> +	struct drm_crtc *crtc = &rcrtc->crtc;
> +	struct drm_plane *primary;
> +	int ret;
> +
> +	rcrtc->rstc = devm_reset_control_get_shared(rcdu->dev, NULL);
> +	if (IS_ERR(rcrtc->rstc)) {
> +		dev_err(rcdu->dev, "can't get cpg reset\n");
> +		return PTR_ERR(rcrtc->rstc);
> +	}
> +
> +	rcrtc->rzg2l_clocks.aclk = devm_clk_get(rcdu->dev, "aclk");
> +	if (IS_ERR(rcrtc->rzg2l_clocks.aclk)) {
> +		dev_err(rcdu->dev, "no axi clock for DU\n");
> +		return PTR_ERR(rcrtc->rzg2l_clocks.aclk);
> +	}
> +
> +	rcrtc->rzg2l_clocks.pclk = devm_clk_get(rcdu->dev, "pclk");
> +	if (IS_ERR(rcrtc->rzg2l_clocks.pclk)) {
> +		dev_err(rcdu->dev, "no peripheral clock for DU\n");
> +		return PTR_ERR(rcrtc->rzg2l_clocks.pclk);
> +	}
> +
> +	rcrtc->rzg2l_clocks.dclk = devm_clk_get(rcdu->dev, "vclk");
> +	if (IS_ERR(rcrtc->rzg2l_clocks.dclk)) {
> +		dev_err(rcdu->dev, "no video clock for DU\n");
> +		return PTR_ERR(rcrtc->rzg2l_clocks.dclk);
> +	}
> +
> +	init_waitqueue_head(&rcrtc->flip_wait);
> +	rcrtc->dev = rcdu;
> +
> +	primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
> +
> +	ret = drm_crtc_init_with_planes(&rcdu->ddev, crtc, primary, NULL,
> +					&crtc_funcs_rz, NULL);
> +	if (ret < 0)
> +		return ret;
> +
> +	drm_crtc_helper_add(crtc, &crtc_helper_funcs);
> +
> +	rzg2l_du_crtc_crc_init(rcrtc);

Will this work correctly if rzg2l_du_crtc_crc_init fails?
It looks like the error in rzg2l_du_crtc_crc_init cannot be caught.

> +
> +	return 0;
> +}

Best regards,
  Nobuhiro