Re: [RFC,3/3] drm/komeda: Allow non-component drm_bridge only endpoints

["james qian wang (Arm Technology China)" <james.qian.wang@arm.com>]

On Fri, Oct 04, 2019 at 02:34:42PM +0000, Mihail Atanassov wrote:
> To support transmitters other than the tda998x, we need to allow
> non-component framework bridges to be attached to a dummy drm_encoder in
> our driver.
> 
> For the existing supported encoder (tda998x), keep the behaviour as-is,
> since there's no way to unbind if a drm_bridge module goes away under
> our feet.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> ---
>  .../gpu/drm/arm/display/komeda/komeda_dev.h   |   5 +
>  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  58 ++++++--
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   | 133 +++++++++++++++++-
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |   5 +
>  4 files changed, 187 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index e392b8ffc372..64d2902e2e0c 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -176,6 +176,11 @@ struct komeda_dev {
>  
>  	/** @irq: irq number */
>  	int irq;
> +	/** @has_components:
> +	 *
> +	 * if true, use the component framework to bind/unbind driver
> +	 */
> +	bool has_components;
>  
>  	/** @lock: used to protect dpmode */
>  	struct mutex lock;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 9ed25ffe0e22..34cfc6a4c3e4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -10,6 +10,8 @@
>  #include <linux/component.h>
>  #include <linux/pm_runtime.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_encoder.h>
>  #include "komeda_dev.h"
>  #include "komeda_kms.h"
>  
> @@ -69,18 +71,35 @@ static int compare_of(struct device *dev, void *data)
>  	return dev->of_node == data;
>  }
>  
> -static void komeda_add_slave(struct device *master,
> -			     struct component_match **match,
> -			     struct device_node *np,
> -			     u32 port, u32 endpoint)
> +static int komeda_add_slave(struct device *master,
> +			    struct komeda_drv *mdrv,
> +			    struct component_match **match,
> +			    struct device_node *np,
> +			    u32 port, u32 endpoint)
>  {
>  	struct device_node *remote;
> +	struct drm_bridge *bridge;
> +	int ret = 0;
>  
>  	remote = of_graph_get_remote_node(np, port, endpoint);
> -	if (remote) {
> +	if (!remote)
> +		return 0;
> +
> +	if (komeda_remote_device_is_component(np, port, endpoint)) {
> +		mdrv->mdev.has_components = true;
>  		drm_of_component_match_add(master, match, compare_of, remote);
> -		of_node_put(remote);
> +		goto exit;
> +	}
> +
> +	bridge = of_drm_find_bridge(remote);
> +	if (!bridge) {
> +		ret = -EPROBE_DEFER;
> +		goto exit;
>  	}
> +
> +exit:
> +	of_node_put(remote);
> +	return ret;
>  }
>  
>  static int komeda_platform_probe(struct platform_device *pdev)
> @@ -89,6 +108,7 @@ static int komeda_platform_probe(struct platform_device *pdev)
>  	struct component_match *match = NULL;
>  	struct device_node *child;
>  	struct komeda_drv *mdrv;
> +	int ret;
>  
>  	if (!dev->of_node)
>  		return -ENODEV;
> @@ -101,14 +121,27 @@ static int komeda_platform_probe(struct platform_device *pdev)
>  		if (of_node_cmp(child->name, "pipeline") != 0)
>  			continue;
>  
> -		/* add connector */
> -		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 0);
> -		komeda_add_slave(dev, &match, child, KOMEDA_OF_PORT_OUTPUT, 1);
> +		/* attach any remote devices if present */
> +		ret = komeda_add_slave(dev, mdrv, &match, child,
> +				       KOMEDA_OF_PORT_OUTPUT, 0);
> +		if (ret)
> +			goto free_mdrv;
> +		ret = komeda_add_slave(dev, mdrv, &match, child,
> +				       KOMEDA_OF_PORT_OUTPUT, 1);
> +		if (ret)
> +			goto free_mdrv;
>  	}
>  
>  	dev_set_drvdata(dev, mdrv);
>  
> -	return component_master_add_with_match(dev, &komeda_master_ops, match);
> +	return mdrv->mdev.has_components
> +		? component_master_add_with_match(
> +			dev, &komeda_master_ops, match)
> +		: komeda_bind(dev);
> +
> +free_mdrv:
> +	devm_kfree(dev, mdrv);
> +	return ret;
>  }
>  
>  static int komeda_platform_remove(struct platform_device *pdev)
> @@ -116,7 +149,10 @@ static int komeda_platform_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct komeda_drv *mdrv = dev_get_drvdata(dev);
>  
> -	component_master_del(dev, &komeda_master_ops);
> +	if (mdrv->mdev.has_components)
> +		component_master_del(dev, &komeda_master_ops);
> +	else
> +		komeda_unbind(dev);
>  
>  	dev_set_drvdata(dev, NULL);
>  	devm_kfree(dev, mdrv);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> index 03dcf1d7688f..6eb52d1b20a0 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
> @@ -6,6 +6,7 @@
>   */
>  #include <linux/component.h>
>  #include <linux/interrupt.h>
> +#include <linux/of_graph.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -14,6 +15,8 @@
>  #include <drm/drm_gem_cma_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_irq.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -267,6 +270,130 @@ static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
>  	config->helper_private = &komeda_mode_config_helpers;
>  }
>  
> +static void komeda_encoder_destroy(struct drm_encoder *encoder)
> +{
> +	drm_encoder_cleanup(encoder);
> +}
> +
> +static const struct drm_encoder_funcs komeda_dummy_enc_funcs = {
> +	.destroy = komeda_encoder_destroy,
> +};
> +
> +bool komeda_remote_device_is_component(struct device_node *local,
> +				       u32 port, u32 endpoint)
> +{
> +	struct device_node *remote;
> +	char const * const component_devices[] = {
> +		"nxp,tda998x",

I really don't think put this dummy_encoder into komeda is a good
idea.

And I suggest to seperate this dummy_encoder to a individual module
which will build the drm_ridge to a standard drm encoder and component
based module, which will be enable by DT, totally transparent for komeda.

BTW:
I really don't like such logic: distingush the SYSTEM configuration
by check the connected device name, it's hard to maintain and code
sharing, and that's why NOW we have the device-tree.

Thanks
James

> +	};
> +	int i;
> +	bool ret = false;
> +
> +	remote = of_graph_get_remote_node(local, port, endpoint);
> +	if (!remote)
> +		return false;
> +
> +	/* Coprocessor endpoints are always component based. */
> +	if (port != KOMEDA_OF_PORT_OUTPUT) {
> +		ret = true;
> +		goto exit;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(component_devices); ++i) {
> +		if (of_device_is_compatible(remote, component_devices[i])) {
> +			ret = true;
> +			goto exit;
> +		}
> +	}
> +
> +exit:
> +	of_node_put(remote);
> +	return ret;
> +}
> +
> +static int komeda_encoder_attach_bridge(struct komeda_dev *mdev,
> +					struct komeda_kms_dev *kms,
> +					struct drm_encoder *encoder,
> +					struct device_node *np)
> +{
> +	struct device_node *remote;
> +	struct drm_bridge *bridge;
> +	u32 crtcs = 0;
> +	int ret = 0;
> +
> +	if (komeda_remote_device_is_component(np, KOMEDA_OF_PORT_OUTPUT, 0))
> +		return 0;
> +
> +	remote = of_graph_get_remote_node(np, KOMEDA_OF_PORT_OUTPUT, 0);
> +	if (!remote)
> +		return 0;
> +
> +	bridge = of_drm_find_bridge(remote);
> +	if (!bridge) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}
> +
> +	crtcs = drm_of_find_possible_crtcs(&kms->base, remote);
> +
> +	encoder->possible_crtcs = crtcs ? crtcs : 1;
> +
> +	ret = drm_encoder_init(&kms->base, encoder,
> +			       &komeda_dummy_enc_funcs, DRM_MODE_ENCODER_TMDS,
> +			       NULL);
> +	if (ret)
> +		goto exit;
> +
> +	ret = drm_bridge_attach(encoder, bridge, NULL);
> +	if (ret)
> +		goto exit;
> +
> +exit:
> +	of_node_put(remote);
> +	return ret;
> +}
> +
> +static int komeda_encoder_attach_bridges(struct komeda_kms_dev *kms,
> +					 struct komeda_dev *mdev)
> +{
> +	int i, err;
> +
> +	for (i = 0; i < kms->n_crtcs; i++) {
> +		err = komeda_encoder_attach_bridge(
> +			mdev, kms,
> +			&kms->encoders[i], mdev->pipelines[i]->of_node);
> +		if (err)
> +			return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static int komeda_kms_attach_external(struct komeda_kms_dev *kms,
> +				      struct komeda_dev *mdev)
> +{
> +	int err;
> +
> +	if (mdev->has_components) {
> +		err = component_bind_all(mdev->dev, kms);
> +		if (err)
> +			return err;
> +	}
> +
> +	err = komeda_encoder_attach_bridges(kms, mdev);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void komeda_kms_detach_external(struct komeda_dev *mdev,
> +				       struct drm_device *drm)
> +{
> +	if (mdev->has_components)
> +		component_unbind_all(mdev->dev, drm);
> +}
> +
>  int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
>  {
>  	struct drm_device *drm;
> @@ -301,7 +428,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
>  	if (err)
>  		goto cleanup_mode_config;
>  
> -	err = component_bind_all(mdev->dev, kms);
> +	err = komeda_kms_attach_external(kms, mdev);
>  	if (err)
>  		goto cleanup_mode_config;
>  
> @@ -332,7 +459,7 @@ int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev)
>  	drm->irq_enabled = false;
>  	mdev->funcs->disable_irq(mdev);
>  free_component_binding:
> -	component_unbind_all(mdev->dev, drm);
> +	komeda_kms_detach_external(mdev, drm);
>  cleanup_mode_config:
>  	drm_mode_config_cleanup(drm);
>  	komeda_kms_cleanup_private_objs(kms);
> @@ -351,7 +478,7 @@ void komeda_kms_fini(struct komeda_kms_dev *kms)
>  	drm_atomic_helper_shutdown(drm);
>  	drm->irq_enabled = false;
>  	mdev->funcs->disable_irq(mdev);
> -	component_unbind_all(mdev->dev, drm);
> +	komeda_kms_detach_external(mdev, drm);
>  	drm_mode_config_cleanup(drm);
>  	komeda_kms_cleanup_private_objs(kms);
>  	drm->dev_private = NULL;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> index e81ceb298034..c2856e19d092 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
> @@ -12,6 +12,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_device.h>
> +#include <drm/drm_encoder.h>
>  #include <drm/drm_writeback.h>
>  #include <drm/drm_print.h>
>  
> @@ -123,6 +124,7 @@ struct komeda_kms_dev {
>  	int n_crtcs;
>  	/** @crtcs: crtcs list */
>  	struct komeda_crtc crtcs[KOMEDA_MAX_PIPELINES];
> +	struct drm_encoder encoders[KOMEDA_MAX_PIPELINES];
>  };
>  
>  #define to_kplane(p)	container_of(p, struct komeda_plane, base)
> @@ -184,4 +186,7 @@ void komeda_crtc_handle_event(struct komeda_crtc   *kcrtc,
>  int komeda_kms_init(struct komeda_kms_dev *kms, struct komeda_dev *mdev);
>  void komeda_kms_fini(struct komeda_kms_dev *kms);
>  
> +bool komeda_remote_device_is_component(struct device_node *local,
> +				       u32 port, u32 endpoint);
> +
>  #endif /*_KOMEDA_KMS_H_*/