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.atanas...@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_*/
