[v4,12/14] drm/mediatek: Improve compatibility of display driver

  - Add new functions to enable/disable components, and
  reuse them when clock enable/disable
- Check if the component is defined before using it since
  some modules are MT8188 only (ex. PADDING)
- Control components according to its type rather than ID
- Use a for-loop to add/remove components in an arrays,
  so we can only maintain this array to make sure every
  component will be initialized properly

Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
---
 .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   | 186 +++++++++---------
 1 file changed, 96 insertions(+), 90 deletions(-)
  

Il 21/06/23 05:19, Hsiao Chien Sung ha scritto:
> - Add new functions to enable/disable components, and
>    reuse them when clock enable/disable
> - Check if the component is defined before using it since
>    some modules are MT8188 only (ex. PADDING)
> - Control components according to its type rather than ID
> - Use a for-loop to add/remove components in an arrays,
>    so we can only maintain this array to make sure every
>    component will be initialized properly
> 
> Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com>
> ---
>   .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c   | 186 +++++++++---------
>   1 file changed, 96 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> index c0a38f5217ee..69ae531294ff 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -51,6 +51,7 @@ enum mtk_ovl_adaptor_comp_id {
>   
>   struct ovl_adaptor_comp_match {
>   	enum mtk_ovl_adaptor_comp_type type;
> +	enum mtk_ddp_comp_id comp_id;
>   	int alias_id;
>   };
>   
> @@ -67,21 +68,78 @@ static const char * const private_comp_stem[OVL_ADAPTOR_TYPE_NUM] = {
>   };
>   
>   static const struct ovl_adaptor_comp_match comp_matches[OVL_ADAPTOR_ID_MAX] = {
> -	[OVL_ADAPTOR_MDP_RDMA0]	= { OVL_ADAPTOR_TYPE_RDMA, 0 },
> -	[OVL_ADAPTOR_MDP_RDMA1]	= { OVL_ADAPTOR_TYPE_RDMA, 1 },
> -	[OVL_ADAPTOR_MDP_RDMA2]	= { OVL_ADAPTOR_TYPE_RDMA, 2 },
> -	[OVL_ADAPTOR_MDP_RDMA3]	= { OVL_ADAPTOR_TYPE_RDMA, 3 },
> -	[OVL_ADAPTOR_MDP_RDMA4]	= { OVL_ADAPTOR_TYPE_RDMA, 4 },
> -	[OVL_ADAPTOR_MDP_RDMA5]	= { OVL_ADAPTOR_TYPE_RDMA, 5 },
> -	[OVL_ADAPTOR_MDP_RDMA6]	= { OVL_ADAPTOR_TYPE_RDMA, 6 },
> -	[OVL_ADAPTOR_MDP_RDMA7]	= { OVL_ADAPTOR_TYPE_RDMA, 7 },
> -	[OVL_ADAPTOR_MERGE0]	= { OVL_ADAPTOR_TYPE_MERGE, 1 },
> -	[OVL_ADAPTOR_MERGE1]	= { OVL_ADAPTOR_TYPE_MERGE, 2 },
> -	[OVL_ADAPTOR_MERGE2]	= { OVL_ADAPTOR_TYPE_MERGE, 3 },
> -	[OVL_ADAPTOR_MERGE3]	= { OVL_ADAPTOR_TYPE_MERGE, 4 },
> -	[OVL_ADAPTOR_ETHDR0]	= { OVL_ADAPTOR_TYPE_ETHDR, 0 },
> +	[OVL_ADAPTOR_ETHDR0] = { OVL_ADAPTOR_TYPE_ETHDR, DDP_COMPONENT_ETHDR_MIXER, 0 },
> +	[OVL_ADAPTOR_MDP_RDMA0] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA0, 0 },
> +	[OVL_ADAPTOR_MDP_RDMA1] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA1, 1 },
> +	[OVL_ADAPTOR_MDP_RDMA2] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA2, 2 },
> +	[OVL_ADAPTOR_MDP_RDMA3] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA3, 3 },
> +	[OVL_ADAPTOR_MDP_RDMA4] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA4, 4 },
> +	[OVL_ADAPTOR_MDP_RDMA5] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA5, 5 },
> +	[OVL_ADAPTOR_MDP_RDMA6] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA6, 6 },
> +	[OVL_ADAPTOR_MDP_RDMA7] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA7, 7 },
> +	[OVL_ADAPTOR_MERGE0] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE1, 1 },
> +	[OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE2, 2 },
> +	[OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE3, 3 },
> +	[OVL_ADAPTOR_MERGE3] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE4, 4 },
>   };
>   
> +static int mtk_ovl_adaptor_enable(struct device *dev, enum mtk_ovl_adaptor_comp_type type)
> +{
> +	int ret = 0;

int ret;

> +
> +	if (!dev)

if (!dev)
	return -ENODEV;

> +		goto end;
> +
> +	switch (type) {
> +	case OVL_ADAPTOR_TYPE_ETHDR:
> +		ret = mtk_ethdr_clk_enable(dev);
> +		break;
> +	case OVL_ADAPTOR_TYPE_MERGE:
> +		ret = mtk_merge_clk_enable(dev);

We already have a .clk_enable() callback in struct mtk_ddp_comp_funcs: to
greatly enhance your nice cleanup, you could use that instead, which basically
eliminates the need of having any if branch and/or switch.

> +		break;
> +	case OVL_ADAPTOR_TYPE_RDMA:
> +		// only LARB users need to do this

Please, C-style comments only.

> +		ret = pm_runtime_get_sync(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to enable power domain, error(%d)\n", ret);
> +			goto end;
> +		}
> +		ret = mtk_mdp_rdma_clk_enable(dev);
> +		if (ret)
> +			pm_runtime_put(dev);
> +		break;
> +	default:
> +		dev_err(dev, "Unknown type: %d\n", type);

Are we supposed to return 0 for unknown type?!

> +	}
> +
> +	if (ret)
> +		dev_err(dev, "Failed to enable clock: error(%d)\n", ret);

if (ret) {
	dev_err(...)
	return ret;
}

return 0;

> +
> +end:
> +	return ret;
> +}
> +
> +static void mtk_ovl_adaptor_disable(struct device *dev, enum mtk_ovl_adaptor_comp_type type)
> +{
> +	if (!dev)
> +		return;
> +
> +	switch (type) {
> +	case OVL_ADAPTOR_TYPE_ETHDR:
> +		mtk_ethdr_clk_disable(dev);
> +		break;
> +	case OVL_ADAPTOR_TYPE_MERGE:
> +		mtk_merge_clk_disable(dev);
> +		break;
> +	case OVL_ADAPTOR_TYPE_RDMA:
> +		mtk_mdp_rdma_clk_disable(dev);
> +		pm_runtime_put(dev);
> +		break;
> +	default:
> +		dev_err(dev, "Unknown type: %d\n", type);
> +	}
> +}
> +
>   void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx,
>   				  struct mtk_plane_state *state,
>   				  struct cmdq_pkt *cmdq_pkt)
> @@ -186,72 +244,30 @@ void mtk_ovl_adaptor_stop(struct device *dev)
>   int mtk_ovl_adaptor_clk_enable(struct device *dev)
>   {
>   	struct mtk_disp_ovl_adaptor *ovl_adaptor = dev_get_drvdata(dev);
> -	struct device *comp;
> -	int ret;
> +	int ret = 0;

Keep `int ret;`

>   	int i;
>   
> -	for (i = 0; i < OVL_ADAPTOR_MERGE0; i++) {
> -		comp = ovl_adaptor->ovl_adaptor_comp[i];
> -		ret = pm_runtime_get_sync(comp);
> -		if (ret < 0) {
> -			dev_err(dev, "Failed to enable power domain %d, err %d\n", i, ret);
> -			goto pwr_err;
> -		}
> -	}
> -
>   	for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> -		comp = ovl_adaptor->ovl_adaptor_comp[i];
> -
> -		if (i < OVL_ADAPTOR_MERGE0)
> -			ret = mtk_mdp_rdma_clk_enable(comp);
> -		else if (i < OVL_ADAPTOR_ETHDR0)
> -			ret = mtk_merge_clk_enable(comp);
> -		else
> -			ret = mtk_ethdr_clk_enable(comp);
> +		ret = mtk_ovl_adaptor_enable(ovl_adaptor->ovl_adaptor_comp[i],
> +					     comp_matches[i].type);
>   		if (ret) {
> -			dev_err(dev, "Failed to enable clock %d, err %d\n", i, ret);
> -			goto clk_err;
> +			while (--i >= 0)
> +				mtk_ovl_adaptor_disable(ovl_adaptor->ovl_adaptor_comp[i],
> +							comp_matches[i].type);
> +			break;

Instead of a break here, just return ret?

>   		}
>   	}
> -
> -	return ret;
> -
> -clk_err:
> -	while (--i >= 0) {
> -		comp = ovl_adaptor->ovl_adaptor_comp[i];
> -		if (i < OVL_ADAPTOR_MERGE0)
> -			mtk_mdp_rdma_clk_disable(comp);
> -		else if (i < OVL_ADAPTOR_ETHDR0)
> -			mtk_merge_clk_disable(comp);
> -		else
> -			mtk_ethdr_clk_disable(comp);
> -	}
> -	i = OVL_ADAPTOR_MERGE0;
> -
> -pwr_err:
> -	while (--i >= 0)
> -		pm_runtime_put(ovl_adaptor->ovl_adaptor_comp[i]);
> -
>   	return ret;

...and return 0 here.

>   }
>   

Regards,
Angelo
  

On Wed, 2023-06-21 at 10:15 +0200, AngeloGioacchino Del Regno wrote:
> >     
> > >   
> > > +static int mtk_ovl_adaptor_enable(struct device *dev, enum
> > mtk_ovl_adaptor_comp_type type)
> > > +{
> > > +int ret = 0;
> >
> > int ret;
> >
> > > +
> > > +if (!dev)
> >
> > if (!dev)
> > return -ENODEV;
> >

We intentionally ignored null dev and didn't return error here, since
MT8195 and MT8188 shares the same component list, there could be
components that are not probed and therefore is null here (For
example, the new hardware Padding in MT8188 only).
 
> > > +goto end;
> > > +
> > > +switch (type) {
> > > +case OVL_ADAPTOR_TYPE_ETHDR:
> > > +ret = mtk_ethdr_clk_enable(dev);
> > > +break;
> > > +case OVL_ADAPTOR_TYPE_MERGE:
> > > +ret = mtk_merge_clk_enable(dev);
> >
> > We already have a .clk_enable() callback in struct
> > mtk_ddp_comp_funcs: to
> > greatly enhance your nice cleanup, you could use that instead,
> which
> > basically
> > eliminates the need of having any if branch and/or switch.
> >

Thanks for the advice, submitted a new version using
mtk_ddp_comp_funcs.
 
> > > +break;
> > > +case OVL_ADAPTOR_TYPE_RDMA:
> > > +// only LARB users need to do this
> >
> > Please, C-style comments only.
> >
> > > +ret = pm_runtime_get_sync(dev);
> > > +if (ret < 0) {
> > > +dev_err(dev, "Failed to enable power domain, error(%d)\n", ret);
> > > +goto end;
> > > +}
> > > +ret = mtk_mdp_rdma_clk_enable(dev);
> > > +if (ret)
> > > +pm_runtime_put(dev);
> > > +break;
> > > +default:
> > > +dev_err(dev, "Unknown type: %d\n", type);
> >
> > Are we supposed to return 0 for unknown type?!
> >

Yes, we ignored the unknown type intentionally, but this part has been
removed in the new patch since 'switch' are all removed after re-using
mtk_ddp_comp_funcs.
 
> > >   for (i = 0; i < OVL_ADAPTOR_ID_MAX; i++) {
> > > -comp = ovl_adaptor->ovl_adaptor_comp[i];
> > > -
> > > -if (i < OVL_ADAPTOR_MERGE0)
> > > -ret = mtk_mdp_rdma_clk_enable(comp);
> > > -else if (i < OVL_ADAPTOR_ETHDR0)
> > > -ret = mtk_merge_clk_enable(comp);
> > > -else
> > > -ret = mtk_ethdr_clk_enable(comp);
> > > +ret = mtk_ovl_adaptor_enable(ovl_adaptor->ovl_adaptor_comp[i],
> > > +     comp_matches[i].type);
> > >   if (ret) {
> > > -dev_err(dev, "Failed to enable clock %d, err %d\n", i, ret);
> > > -goto clk_err;
> > > +while (--i >= 0)
> > > +mtk_ovl_adaptor_disable(ovl_adaptor->ovl_adaptor_comp[i],
> > > +comp_matches[i].type);
> > > +break;
> >
> > Instead of a break here, just return ret?

Got it, has submitted a new version that returns the error immediately.
 
The reason we break here instead of returning error, is trying to make
one function only has one return. For example, always return at the end
of a function, so if someday we add a printk() at somewhere in that
function for debugging, it won't be skipped by the 'return' before it.
 
Not sure if this convention is not recommended when writing kernel
codes? Thanks.
 
Regards,
Hsiao Chien Sung