Re: [PATCH v28 6/7] drm/mediatek: add drm ovl_adaptor sub driver for MT8195

Re: [PATCH v28 6/7] drm/mediatek: add drm ovl_adaptor sub driver for MT8195

[Nícolas F. R. A. Prado]

On Mon, Nov 07, 2022 at 03:24:12PM +0800, Nancy.Lin wrote:
> Add drm ovl_adaptor sub driver. Bring up ovl_adaptor sub driver if
> the component exists in the path.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
[..]
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 30dcb65d8a5a..ce5617ad04cb 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
[..]
> @@ -897,22 +906,18 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
>  		crtc_i++;
>  
>  	for (i = 0; i < path_len; i++) {
> -		enum mtk_ddp_comp_id comp_id = path[i];
> +		unsigned int comp_id = path[i];
>  		struct device_node *node;
> -		struct mtk_ddp_comp *comp;
>  
>  		node = priv->comp_node[comp_id];
> -		comp = &priv->ddp_comp[comp_id];
> -
> -		if (!node) {
> -			dev_info(dev,
> -				 "Not creating crtc %d because component %d is disabled or missing\n",
> -				 crtc_i, comp_id);
> -			return 0;
> -		}
>  
> -		if (!comp->dev) {
> -			dev_err(dev, "Component %pOF not initialized\n", node);
> +		/* Not all drm components have a DTS device node, such as ovl_adaptor,
> +		 * which is the drm bring up sub driver
> +		 */
> +		if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR) {
> +			dev_err(dev,
> +				"Not creating crtc %d because component %d is disabled, missing or not initialized\n",
> +				crtc_i, comp_id);
>  			return -ENODEV;

Why do you change the behavior here? If !node, the return should be 0, because
there are two separate data streams, for internal and external display, and they
are optional. It should be possible to for example have the nodes for external
display disabled in DT and still have the driver probe and have a working
internal display. But with this change you're breaking that. Also, this message
should stay as dev_info and not mention "not initialized", so basically it
should stay the same as before the change.

Thanks,
Nícolas

Re: [PATCH v28 6/7] drm/mediatek: add drm ovl_adaptor sub driver for MT8195

[Nancy Lin (林欣螢)]

Dear Nicolas,

Thanks for the review.

On Fri, 2022-11-25 at 17:24 -0500, Nícolas F. R. A. Prado wrote:
> On Mon, Nov 07, 2022 at 03:24:12PM +0800, Nancy.Lin wrote:
> > Add drm ovl_adaptor sub driver. Bring up ovl_adaptor sub driver if
> > the component exists in the path.
> > 
> > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > Tested-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> > Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > ---
> 
> [..]
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 30dcb65d8a5a..ce5617ad04cb 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> 
> [..]
> > @@ -897,22 +906,18 @@ int mtk_drm_crtc_create(struct drm_device
> > *drm_dev,
> >  		crtc_i++;
> >  
> >  	for (i = 0; i < path_len; i++) {
> > -		enum mtk_ddp_comp_id comp_id = path[i];
> > +		unsigned int comp_id = path[i];
> >  		struct device_node *node;
> > -		struct mtk_ddp_comp *comp;
> >  
> >  		node = priv->comp_node[comp_id];
> > -		comp = &priv->ddp_comp[comp_id];
> > -
> > -		if (!node) {
> > -			dev_info(dev,
> > -				 "Not creating crtc %d because
> > component %d is disabled or missing\n",
> > -				 crtc_i, comp_id);
> > -			return 0;
> > -		}
> >  
> > -		if (!comp->dev) {
> > -			dev_err(dev, "Component %pOF not
> > initialized\n", node);
> > +		/* Not all drm components have a DTS device node, such
> > as ovl_adaptor,
> > +		 * which is the drm bring up sub driver
> > +		 */
> > +		if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR)
> > {
> > +			dev_err(dev,
> > +				"Not creating crtc %d because component
> > %d is disabled, missing or not initialized\n",
> > +				crtc_i, comp_id);
> >  			return -ENODEV;
> 
> Why do you change the behavior here? If !node, the return should be
> 0, because
> there are two separate data streams, for internal and external
> display, and they
> are optional. It should be possible to for example have the nodes for
> external
> display disabled in DT and still have the driver probe and have a
> working
> internal display. But with this change you're breaking that. Also,
> this message
> should stay as dev_info and not mention "not initialized", so
> basically it
> should stay the same as before the change.
> 
> Thanks,
> Nícolas

Yes, You are right. This is my mistake. I should not change this
behavior. I will fix it and modify as following for the ovl_adaptor sub
driver component, which don't have dts device node.

@@ -905,7 +914,10 @@ int mtk_drm_crtc_create(struct drm_device
*drm_dev,
                node = priv->comp_node[comp_id];
                comp = &priv->ddp_comp[comp_id];

-               if (!node) {
+               /* Not all drm components have a DTS device node, such
as ovl_adaptor,
+                * which is the drm bring up sub driver
+                */
+               if (!node && comp_id != DDP_COMPONENT_DRM_OVL_ADAPTOR)
{
                        dev_info(dev,
                                 "Not creating crtc %d because
component %d is disabled or missing\n",
                                 crtc_i, comp_id);
@@ -938,7 +950,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
        }

Regards,
Nancy

Re: [PATCH v28 1/7] dt-bindings: mediatek: add ethdr definition for mt8195

[Chun-Kuang Hu]

Hi, Nancy:

I've lost the v29 mail, so I reply in this mail for v29.
I got some message by make dt_binding_check.

/home/chunkuang/git/linux/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.example.dtb:
hdr-engine@1c114000: mediatek,gce-client-reg:0: [4294967295, 7, 16384,
4096, 4294967295, 7, 20480, 4096, 4294967295, 7, 28672, 4096,
4294967295, 7, 36864, 4096, 4294967295, 7, 40960, 4096, 4294967295, 7,
45056, 4096, 4294967295, 7, 49152, 4096] is too long
From schema: /home/chunkuang/git/linux/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
/home/chunkuang/git/linux/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.example.dtb:
hdr-engine@1c114000: mediatek,gce-client-reg: [[4294967295, 7, 16384,
4096, 4294967295, 7, 20480, 4096, 4294967295, 7, 28672, 4096,
4294967295, 7, 36864, 4096, 4294967295, 7, 40960, 4096, 4294967295, 7,
45056, 4096, 4294967295, 7, 49152, 4096]] is too short
From schema: /home/chunkuang/git/linux/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml

Regards,
Chun-Kuang.

Nancy.Lin <nancy.lin@mediatek.com> 於 2022年11月7日 週一 下午3:24寫道：

>
> Add vdosys1 ETHDR definition.
>
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../display/mediatek/mediatek,ethdr.yaml      | 188 ++++++++++++++++++
>  1 file changed, 188 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
> new file mode 100644
> index 000000000000..3b11e47a8834
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
> @@ -0,0 +1,188 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,ethdr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek Ethdr Device
> +
> +maintainers:
> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +
> +description:
> +  ETHDR (ET High Dynamic Range) is a MediaTek internal HDR engine and is
> +  designed for HDR video and graphics conversion in the external display path.
> +  It handles multiple HDR input types and performs tone mapping, color
> +  space/color format conversion, and then combine different layers,
> +  output the required HDR or SDR signal to the subsequent display path.
> +  This engine is composed of two video frontends, two graphic frontends,
> +  one video backend and a mixer. ETHDR has two DMA function blocks, DS and ADL.
> +  These two function blocks read the pre-programmed registers from DRAM and
> +  set them to HW in the v-blanking period.
> +
> +properties:
> +  compatible:
> +    const: mediatek,mt8195-disp-ethdr
> +
> +  reg:
> +    maxItems: 7
> +
> +  reg-names:
> +    items:
> +      - const: mixer
> +      - const: vdo_fe0
> +      - const: vdo_fe1
> +      - const: gfx_fe0
> +      - const: gfx_fe1
> +      - const: vdo_be
> +      - const: adl_ds
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  iommus:
> +    minItems: 1
> +    maxItems: 2
> +
> +  clocks:
> +    items:
> +      - description: mixer clock
> +      - description: video frontend 0 clock
> +      - description: video frontend 1 clock
> +      - description: graphic frontend 0 clock
> +      - description: graphic frontend 1 clock
> +      - description: video backend clock
> +      - description: autodownload and menuload clock
> +      - description: video frontend 0 async clock
> +      - description: video frontend 1 async clock
> +      - description: graphic frontend 0 async clock
> +      - description: graphic frontend 1 async clock
> +      - description: video backend async clock
> +      - description: ethdr top clock
> +
> +  clock-names:
> +    items:
> +      - const: mixer
> +      - const: vdo_fe0
> +      - const: vdo_fe1
> +      - const: gfx_fe0
> +      - const: gfx_fe1
> +      - const: vdo_be
> +      - const: adl_ds
> +      - const: vdo_fe0_async
> +      - const: vdo_fe1_async
> +      - const: gfx_fe0_async
> +      - const: gfx_fe1_async
> +      - const: vdo_be_async
> +      - const: ethdr_top
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: video frontend 0 async reset
> +      - description: video frontend 1 async reset
> +      - description: graphic frontend 0 async reset
> +      - description: graphic frontend 1 async reset
> +      - description: video backend async reset
> +
> +  reset-names:
> +    items:
> +      - const: vdo_fe0_async
> +      - const: vdo_fe1_async
> +      - const: gfx_fe0_async
> +      - const: gfx_fe1_async
> +      - const: vdo_be_async
> +
> +  mediatek,gce-client-reg:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: The register of display function block to be set by gce.
> +      There are 4 arguments in this property, gce node, subsys id, offset and
> +      register size. The subsys id is defined in the gce header of each chips
> +      include/dt-bindings/gce/<chip>-gce.h, mapping to the register of display
> +      function block.
> +    items:
> +      items:
> +        - description: phandle of GCE
> +        - description: GCE subsys id
> +        - description: register offset
> +        - description: register size
> +    minItems: 7
> +    maxItems: 7
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - power-domains
> +  - resets
> +  - mediatek,gce-client-reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/mt8195-clk.h>
> +    #include <dt-bindings/gce/mt8195-gce.h>
> +    #include <dt-bindings/memory/mt8195-memory-port.h>
> +    #include <dt-bindings/power/mt8195-power.h>
> +    #include <dt-bindings/reset/mt8195-resets.h>
> +
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        hdr-engine@1c114000 {
> +                compatible = "mediatek,mt8195-disp-ethdr";
> +                reg = <0 0x1c114000 0 0x1000>,
> +                      <0 0x1c115000 0 0x1000>,
> +                      <0 0x1c117000 0 0x1000>,
> +                      <0 0x1c119000 0 0x1000>,
> +                      <0 0x1c11a000 0 0x1000>,
> +                      <0 0x1c11b000 0 0x1000>,
> +                      <0 0x1c11c000 0 0x1000>;
> +                reg-names = "mixer", "vdo_fe0", "vdo_fe1", "gfx_fe0", "gfx_fe1",
> +                            "vdo_be", "adl_ds";
> +                mediatek,gce-client-reg = <&gce0 SUBSYS_1c11XXXX 0x4000 0x1000>,
> +                                          <&gce0 SUBSYS_1c11XXXX 0x5000 0x1000>,
> +                                          <&gce0 SUBSYS_1c11XXXX 0x7000 0x1000>,
> +                                          <&gce0 SUBSYS_1c11XXXX 0x9000 0x1000>,
> +                                          <&gce0 SUBSYS_1c11XXXX 0xa000 0x1000>,
> +                                          <&gce0 SUBSYS_1c11XXXX 0xb000 0x1000>,
> +                                          <&gce0 SUBSYS_1c11XXXX 0xc000 0x1000>;
> +                clocks = <&vdosys1 CLK_VDO1_DISP_MIXER>,
> +                         <&vdosys1 CLK_VDO1_HDR_VDO_FE0>,
> +                         <&vdosys1 CLK_VDO1_HDR_VDO_FE1>,
> +                         <&vdosys1 CLK_VDO1_HDR_GFX_FE0>,
> +                         <&vdosys1 CLK_VDO1_HDR_GFX_FE1>,
> +                         <&vdosys1 CLK_VDO1_HDR_VDO_BE>,
> +                         <&vdosys1 CLK_VDO1_26M_SLOW>,
> +                         <&vdosys1 CLK_VDO1_HDR_VDO_FE0_DL_ASYNC>,
> +                         <&vdosys1 CLK_VDO1_HDR_VDO_FE1_DL_ASYNC>,
> +                         <&vdosys1 CLK_VDO1_HDR_GFX_FE0_DL_ASYNC>,
> +                         <&vdosys1 CLK_VDO1_HDR_GFX_FE1_DL_ASYNC>,
> +                         <&vdosys1 CLK_VDO1_HDR_VDO_BE_DL_ASYNC>,
> +                         <&topckgen CLK_TOP_ETHDR>;
> +                clock-names = "mixer", "vdo_fe0", "vdo_fe1", "gfx_fe0", "gfx_fe1",
> +                              "vdo_be", "adl_ds", "vdo_fe0_async", "vdo_fe1_async",
> +                              "gfx_fe0_async", "gfx_fe1_async","vdo_be_async",
> +                              "ethdr_top";
> +                power-domains = <&spm MT8195_POWER_DOMAIN_VDOSYS1>;
> +                iommus = <&iommu_vpp M4U_PORT_L3_HDR_DS>,
> +                         <&iommu_vpp M4U_PORT_L3_HDR_ADL>;
> +                interrupts = <GIC_SPI 517 IRQ_TYPE_LEVEL_HIGH 0>; /* disp mixer */
> +                resets = <&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_VDO_FE0_DL_ASYNC>,
> +                         <&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_VDO_FE1_DL_ASYNC>,
> +                         <&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_GFX_FE0_DL_ASYNC>,
> +                         <&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_GFX_FE1_DL_ASYNC>,
> +                         <&vdosys1 MT8195_VDOSYS1_SW1_RST_B_HDR_VDO_BE_DL_ASYNC>;
> +                reset-names = "vdo_fe0_async", "vdo_fe1_async", "gfx_fe0_async",
> +                              "gfx_fe1_async", "vdo_be_async";
> +        };
> +    };
> +...
> --
> 2.18.0
>