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
