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

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 25/60] drm: Add helper to create a connector for a chain of bridges
Date: Wed, 14 Aug 2019 17:01:31 +0200	[thread overview]
Message-ID: <20190814150131.GG7444@phenom.ffwll.local> (raw)
In-Reply-To: <20190808194843.GB6055@pendragon.ideasonboard.com>

Hi Laurent,

On Thu, Aug 08, 2019 at 10:48:43PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Thu, Jul 18, 2019 at 07:01:03PM +0200, Daniel Vetter wrote:
> > 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@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@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?
> 
> Yes, and it's unused in this patch, I've just noticed that. Field
> dropped.
> 
> > 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.
> 
> The main purpose of this structure is indeed to cache the bridge
> pointers, which could be recalculated at runtime. I agree there's no
> real hot path, but caching them still feels nice :-)

We've treated anything in atomic as not a hot-path, preferring clean code
over fast. Except if someone can proof otherwise, which very few ever
bother to even try :-)

> How do you go from the connector to its encoder though ? The
> drm_connector encoder field is valid for non-atomic drivers only, and
> the encoder_ids field is marked as not to be accessed directly. Should I
> use drm_connector_for_each_possible_encoder() and pick the first encoder
> ?

pick_single_encoder_for_connector. Was once exported even ... I think for
bridge we can just hard-code the assumption that there's only one
connector for a bridge chain.

For more fancy topologies this ofc all breaks down, but maybe we can
postpone solving that problem ...

> 
> > > +};
> > > +
> > > +#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.
> 
> Renamed already in my private tree, will be in v2 :-)
> 
> > 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.
> 
> Let's discuss that in the replies to the other patch, as the discussion
> is already longer there. I'm not opposed to your proposal, but I've
> asked a few questions to clarify it.
> 
> > > +	}
> > > +}
> > > +
> > > +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.
> 
> Already, even without a user ? Or should we wait until someone needs
> them ?

I figured mostly to have an excuse for the kerneldoc ...

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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel