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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxx>
> > + */
> > +
> > +#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 :-)

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
?

> > +};
> > +
> > +#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 ?

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux