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 \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