[PATCH 25/60] drm: Add helper to create a connector for a chain of bridges

[Daniel Vetter]

On Sun, Jul 07, 2019 at 09:19:02PM +0300, Laurent Pinchart wrote:
> Most bridge drivers create a DRM connector to model the connector at the
> output of the bridge. This model is historical and has worked pretty
> well so far, but causes several issues:
> 
> - It prevents supporting more complex display pipelines where DRM
> connector operations are split over multiple components. For instance a
> pipeline with a bridge connected to the DDC signals to read EDID data,
> and another one connected to the HPD signal to detect connection and
> disconnection, will not be possible to support through this model.
> 
> - It requires every bridge driver to implement similar connector
> handling code, resulting in code duplication.
> 
> - It assumes that a bridge will either be wired to a connector or to
> another bridge, but doesn't support bridges that can be used in both
> positions very well (although there is some ad-hoc support for this in
> the analogix_dp bridge driver).
> 
> In order to solve these issues, ownership of the connector needs to be
> moved to the display controller driver.
> 
> To avoid code duplication in display controller drivers, add a new
> helper to create and manage a DRM connector backed by a chain of
> bridges. All connector operations are delegating to the appropriate
> bridge in the chain.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  drivers/gpu/drm/Makefile               |   3 +-
>  drivers/gpu/drm/drm_bridge_connector.c | 385 +++++++++++++++++++++++++
>  include/drm/drm_bridge_connector.h     |  18 ++
>  3 files changed, 405 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/drm_bridge_connector.c
>  create mode 100644 include/drm/drm_bridge_connector.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f0d2ee35794..1b74653c9db9 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -37,7 +37,8 @@ drm_vram_helper-y := drm_gem_vram_helper.o \
>  		     drm_vram_mm_helper.o
>  obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
>  
> -drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper.o \
> +drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
> +		drm_dsc.o drm_probe_helper.o \
>  		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
>  		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
>  		drm_simple_kms_helper.o drm_modeset_helper.o \
> diff --git a/drivers/gpu/drm/drm_bridge_connector.c b/drivers/gpu/drm/drm_bridge_connector.c
> new file mode 100644
> index 000000000000..09f2d6bfb561
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_bridge_connector.c
> @@ -0,0 +1,385 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drm_atomic_state_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_probe_helper.h>
> +
> +/**
> + * DOC: overview
> + *
> + * The DRM bridge connector helper object provides a DRM connector
> + * implementation that wraps a chain of &struct drm_bridge. The connector
> + * operations are fully implemented based on the operations of the bridges in
> + * the chain, and don't require any intervention from the display controller
> + * driver at runtime.
> + *
> + * To use the helper, display controller drivers create a bridge connector with
> + * a call to drm_bridge_connector_init(). This associates the newly created
> + * connector with the chain of bridges passed to the function and registers it
> + * with the DRM device. At that point the connector becomes fully usable, no
> + * further operation is needed.
> + *
> + * The DRM bridge connector operations are implemented based on the operations
> + * provided by the bridges in the chain. Each connector operation is delegated
> + * to the bridge closest to the connector (at the end of the chain) that
> + * provides the relevant functionality.
> + *
> + * To make use of this helper, all bridges in the chain shall report bridge
> + * operation flags (&drm_bridge->ops) and bridge output type
> + * (&drm_bridge->type), and none of them may create a DRM connector directly.
> + */
> +
> +/**
> + * struct drm_bridge_connector - A connector backed by a chain of bridges
> + */
> +struct drm_bridge_connector {
> +	/**
> +	 * @base: The base DRM connector
> +	 */
> +	struct drm_connector base;
> +	/**
> +	 * @bridge:
> +	 *
> +	 * The first bridge in the chain (connected to the output of the CRTC).
> +	 */
> +	struct drm_bridge *bridge;
> +	/**
> +	 * @bridge_edid:
> +	 *
> +	 * The last bridge in the chain (closest to the connector) that provides
> +	 * EDID read support, if any (see &DRM_BRIDGE_OP_EDID).
> +	 */
> +	struct drm_bridge *bridge_edid;
> +	/**
> +	 * @bridge_hpd:
> +	 *
> +	 * The last bridge in the chain (closest to the connector) that provides
> +	 * hot-plug detection notification, if any (see &DRM_BRIDGE_OP_HPD).
> +	 */
> +	struct drm_bridge *bridge_hpd;
> +	/**
> +	 * @bridge_detect:
> +	 *
> +	 * The last bridge in the chain (closest to the connector) that provides
> +	 * connector detection, if any (see &DRM_BRIDGE_OP_DETECT).
> +	 */
> +	struct drm_bridge *bridge_detect;
> +	/**
> +	 * @bridge_detect:
> +	 *
> +	 * The last bridge in the chain (closest to the connector) that provides
> +	 * connector modes detection, if any (see &DRM_BRIDGE_OP_MODES).
> +	 */
> +	struct drm_bridge *bridge_modes;
> +	/**
> +	 * @hdmi_mode: Valid for HDMI connectors only.
> +	 */
> +	bool hdmi_mode;

This should probably be in drm_display_info somewhere, not here?


Wrt the overall design ... why do we need a new struct? If we assume (at
least for now) that we only allow one encoder for such a bridge chain
(currently still true), then you can always go from the connector to it's
only possibel encoder. And from there to the bridge chain.

Furthermore all the special bridge pointers here can just be found at
runtime by walking the bridge links. And none of these paths are hot
enough to make this a problem.

With that your drm_bridge_connector here would become just a pile of
functions as default implementations for connectors. Making it more
modular and more helper-y and easier to transition gradually.

> +};
> +
> +#define to_drm_bridge_connector(x) \
> +	container_of(x, struct drm_bridge_connector, base)
> +
> +/* -----------------------------------------------------------------------------
> + * Bridge Connector Hot-Plug Handling
> + */
> +
> +static void drm_bridge_connector_hpd_notify(struct drm_connector *connector,
> +					    enum drm_connector_status status)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	if (status != connector_status_disconnected)
> +		return;
> +
> +	/*
> +	 * Notify all bridges in the pipeline of disconnection. This is required
> +	 * to let the HDMI encoders reset their internal state related to
> +	 * connection status, such as the CEC address.
> +	 */
> +	for (bridge = bridge_connector->bridge; bridge; bridge = bridge->next) {
> +		if (bridge->funcs->lost_hotplug)
> +			bridge->funcs->lost_hotplug(bridge);

So looking at this you pretty much implement my idea for hdp handling
already, except you're calling it ->lost_hotplug and not ->notify_hpd.
Plus you require some callback registration. Essentially my design (that I
explained in my reply to your bridge patch) would just make
drm_bridge_connector_hpd_cb() the one and only hpd_cb, and punt all hpd
handling to bridge drivers like you do here.

Ofc that leaves us with "who's calling drm_kms_helper_hotplug_event()",
and that's what the new hdp_notify on the encoder and the global
drm_mode_config_helper_funcs would be for.

> +	}
> +}
> +
> +static void drm_bridge_connector_hpd_cb(void *cb_data,
> +					enum drm_connector_status status)
> +{
> +	struct drm_bridge_connector *drm_bridge_connector = cb_data;
> +	struct drm_connector *connector = &drm_bridge_connector->base;
> +	struct drm_device *dev = connector->dev;
> +	enum drm_connector_status old_status;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +	old_status = connector->status;
> +	connector->status = status;
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	if (old_status == status)
> +		return;
> +
> +	drm_bridge_connector_hpd_notify(connector, status);
> +
> +	drm_kms_helper_hotplug_event(dev);
> +}
> +
> +/**
> + * drm_bridge_connector_enable_hpd - Enable hot-plug detection for the connector
> + * @connector: The DRM bridge connector
> + *
> + * This function enables hot-plug detection for the given bridge connector.
> + * This is typically used by display drivers in their resume handler.
> + */
> +void drm_bridge_connector_enable_hpd(struct drm_connector *connector)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *hpd = bridge_connector->bridge_hpd;
> +
> +	if (hpd)
> +		drm_bridge_hpd_enable(hpd, drm_bridge_connector_hpd_cb,
> +				      bridge_connector);
> +}
> +EXPORT_SYMBOL_GPL(drm_bridge_connector_enable_hpd);
> +
> +/**
> + * drm_bridge_connector_disable_hpd - Disable hot-plug detection for the
> + * connector
> + * @connector: The DRM bridge connector
> + *
> + * This function disables hot-plug detection for the given bridge connector.
> + * This is typically used by display drivers in their suspend handler.
> + */
> +void drm_bridge_connector_disable_hpd(struct drm_connector *connector)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *hpd = bridge_connector->bridge_hpd;
> +
> +	if (hpd)
> +		drm_bridge_hpd_disable(hpd);
> +}
> +EXPORT_SYMBOL_GPL(drm_bridge_connector_disable_hpd);
> +
> +/* -----------------------------------------------------------------------------
> + * Bridge Connector Functions
> + */
> +
> +static enum drm_connector_status
> +drm_bridge_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *detect = bridge_connector->bridge_detect;
> +	enum drm_connector_status status;
> +
> +	if (detect) {
> +		status = detect->funcs->detect(detect);
> +
> +		drm_bridge_connector_hpd_notify(connector, status);
> +	} else {
> +		switch (connector->connector_type) {
> +		case DRM_MODE_CONNECTOR_DPI:
> +		case DRM_MODE_CONNECTOR_LVDS:
> +		case DRM_MODE_CONNECTOR_DSI:
> +			status = connector_status_connected;
> +			break;
> +		default:
> +			status = connector_status_unknown;
> +			break;
> +		}
> +	}
> +
> +	return status;
> +}
> +
> +static void drm_bridge_connector_destroy(struct drm_connector *connector)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +
> +	if (bridge_connector->bridge_hpd) {
> +		struct drm_bridge *hpd = bridge_connector->bridge_hpd;
> +
> +		drm_bridge_hpd_disable(hpd);
> +	}
> +
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +
> +	kfree(bridge_connector);
> +}
> +
> +static const struct drm_connector_funcs drm_bridge_connector_funcs = {
> +	.reset = drm_atomic_helper_connector_reset,
> +	.detect = drm_bridge_connector_detect,

For that smooht helper library feeling I think we should export _detect
and get_modes at least.

Cheers, Daniel

> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.destroy = drm_bridge_connector_destroy,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Bridge Connector Helper Functions
> + */
> +
> +#define MAX_EDID  512
> +
> +static int drm_bridge_connector_get_modes_edid(struct drm_connector *connector,
> +					       struct drm_bridge *bridge)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	enum drm_connector_status status;
> +	struct edid *edid;
> +	int n;
> +
> +	status = drm_bridge_connector_detect(connector, false);
> +	if (status != connector_status_connected)
> +		goto no_edid;
> +
> +	edid = bridge->funcs->get_edid(bridge, connector);
> +	if (!edid || !drm_edid_is_valid(edid)) {
> +		kfree(edid);
> +		goto no_edid;
> +	}
> +
> +	drm_connector_update_edid_property(connector, edid);
> +	n = drm_add_edid_modes(connector, edid);
> +
> +	bridge_connector->hdmi_mode = drm_detect_hdmi_monitor(edid);
> +
> +	kfree(edid);
> +	return n;
> +
> +no_edid:
> +	drm_connector_update_edid_property(connector, NULL);
> +	return 0;
> +}
> +
> +static int drm_bridge_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		to_drm_bridge_connector(connector);
> +	struct drm_bridge *bridge;
> +
> +	/*
> +	 * If display exposes EDID, then we parse that in the normal way to
> +	 * build table of supported modes.
> +	 */
> +	bridge = bridge_connector->bridge_edid;
> +	if (bridge)
> +		return drm_bridge_connector_get_modes_edid(connector, bridge);
> +
> +	/*
> +	 * Otherwise if the display pipeline reports modes (e.g. with a fixed
> +	 * resolution panel or an analog TV output), query it.
> +	 */
> +	bridge = bridge_connector->bridge_modes;
> +	if (bridge)
> +		return bridge->funcs->get_modes(bridge, connector);
> +
> +	/*
> +	 * We can't retrieve modes, which can happen for instance for a DVI or
> +	 * VGA output with the DDC bus unconnected. The KMS core will add the
> +	 * default modes.
> +	 */
> +	return 0;
> +}
> +
> +static const struct drm_connector_helper_funcs drm_bridge_connector_helper_funcs = {
> +	.get_modes = drm_bridge_connector_get_modes,
> +	/* No need for .mode_valid(), the bridges are checked by the core. */
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Bridge Connector Initialisation
> + */
> +
> +/**
> + * drm_bridge_connector_init - Initialise a connector for a chain of bridges
> + * @drm: the DRM device
> + * @bridge: the bridge closest to the CRTC output
> + *
> + * Allocate, initialise and register a &drm_bridge_connector with the @drm
> + * device. The connector is associated with a chain of bridges that starts at
> + * the CRTC output with @bridge. All bridges in the chain shall report bridge
> + * operation flags (&drm_bridge->ops) and bridge output type
> + * (&drm_bridge->type), and none of them may create a DRM connector directly.
> + *
> + * Returns a pointer to the new connector on success, or a negative error
> + * pointer otherwise.
> + */
> +struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> +						struct drm_bridge *bridge)
> +{
> +	struct drm_bridge_connector *bridge_connector;
> +	struct drm_connector *connector;
> +	int connector_type;
> +
> +	bridge_connector = kzalloc(sizeof(*bridge_connector), GFP_KERNEL);
> +	if (!bridge_connector)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge_connector->bridge = bridge;
> +
> +	/*
> +	 * Initialise connector status handling. First locate the furthest
> +	 * bridges in the pipeline that support HPD and output detection. Then
> +	 * initialise the connector polling mode, using HPD if available and
> +	 * falling back to polling if supported. If neither HPD nor output
> +	 * detection are available, we don't support hotplug detection at all.
> +	 */
> +	connector_type = DRM_MODE_CONNECTOR_Unknown;
> +	for ( ; bridge; bridge = bridge->next) {
> +		if (bridge->ops & DRM_BRIDGE_OP_EDID)
> +			bridge_connector->bridge_edid = bridge;
> +		if (bridge->ops & DRM_BRIDGE_OP_HPD)
> +			bridge_connector->bridge_hpd = bridge;
> +		if (bridge->ops & DRM_BRIDGE_OP_DETECT)
> +			bridge_connector->bridge_detect = bridge;
> +		if (bridge->ops & DRM_BRIDGE_OP_MODES)
> +			bridge_connector->bridge_modes = bridge;
> +
> +		if (!bridge->next)
> +			connector_type = bridge->type;
> +	}
> +
> +	if (connector_type == DRM_MODE_CONNECTOR_Unknown) {
> +		kfree(bridge_connector);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/*
> +	 * TODO: Handle interlace_allowed, doublescan_allowed, stereo_allowed
> +	 * and ycbcr_420_allowed.
> +	 */
> +	connector = &bridge_connector->base;
> +	drm_connector_init(drm, connector, &drm_bridge_connector_funcs,
> +			   connector_type);
> +	drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
> +
> +	if (bridge_connector->bridge_hpd)
> +		connector->polled = DRM_CONNECTOR_POLL_HPD;
> +	else if (bridge_connector->bridge_detect)
> +		connector->polled = DRM_CONNECTOR_POLL_CONNECT
> +				  | DRM_CONNECTOR_POLL_DISCONNECT;
> +
> +	return connector;
> +}
> +EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
> diff --git a/include/drm/drm_bridge_connector.h b/include/drm/drm_bridge_connector.h
> new file mode 100644
> index 000000000000..ec33b44954b8
> --- /dev/null
> +++ b/include/drm/drm_bridge_connector.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2019 Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +
> +#ifndef __DRM_BRIDGE_CONNECTOR_H__
> +#define __DRM_BRIDGE_CONNECTOR_H__
> +
> +struct drm_bridge;
> +struct drm_connector;
> +struct drm_device;
> +
> +void drm_bridge_connector_enable_hpd(struct drm_connector *connector);
> +void drm_bridge_connector_disable_hpd(struct drm_connector *connector);
> +struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> +						struct drm_bridge *bridge);
> +
> +#endif /* __DRM_BRIDGE_CONNECTOR_H__ */
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch