| Message ID | 1375995827-22481-1-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@chromium.org> wrote: > This patch adds the notion of a drm_bridge. A bridge is a chained > device which hangs off an encoder. The drm driver using the bridge > should provide the association between encoder and bridge. Once a > bridge is associated with an encoder, it will participate in mode > set, dpms, and optionally connection detection. > > Since a connector may not be able to determine the connection state > downstream of the bridge, bridges may implement detect() which will take > precedence over connector detect() if the bridge is already attached > to an encoder and that encoder is already attached to the connector. > In practical terms, this requires the drm driver to make these > associations at init time, which is fine for SoC applications where > this link is static. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> A few comments below. I think overall any such infrastructure simply needs to demonstrate viability on a few drivers, that makes it much simpler to check whether the interfaces make sense. Generally I'd make more sense for me if the bridge is just an implementation detail of a given encoder and if the bridge would not be exposed to the crtc helpers. With that abstraction chaining would be more natural imo and we'd also have a much higher chance that bridge drivers work accross different drm drivers: Atm you can run some encoder stuff in the crtc callbacks and the other way round (and pretty much every driver for a bit more complicated hw does that), and every driver can differ in those tricks a bit. If we bake the bridge callbacks into the crtc helpers I expect that for bridges with tricky ordering constraints this will become a giant mess. So I'd prefer much if this would work like drm i2c slave encoders. Cheers, Daniel > --- > drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++ > drivers/gpu/drm/drm_crtc_helper.c | 112 ++++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/drm_sysfs.c | 8 ++- > include/drm/drm_crtc.h | 48 ++++++++++++++++ > include/drm/drm_crtc_helper.h | 34 ++++++++++++ > 5 files changed, 241 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index fc83bb9..0311e2b 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev) > } > EXPORT_SYMBOL(drm_connector_unplug_all); > > +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, > + const struct drm_bridge_funcs *funcs) > +{ > + int ret; > + > + drm_modeset_lock_all(dev); > + > + ret = drm_mode_object_get(dev, &bridge->base, DRM_MODE_OBJECT_BRIDGE); > + if (ret) > + goto out; > + > + bridge->dev = dev; > + bridge->funcs = funcs; > + > + list_add_tail(&bridge->head, &dev->mode_config.bridge_list); > + dev->mode_config.num_bridge++; > + > + out: > + drm_modeset_unlock_all(dev); > + return ret; > +} > +EXPORT_SYMBOL(drm_bridge_init); > + > +void drm_bridge_cleanup(struct drm_bridge *bridge) > +{ > + struct drm_device *dev = bridge->dev; > + > + drm_modeset_lock_all(dev); > + drm_mode_object_put(dev, &bridge->base); > + list_del(&bridge->head); > + dev->mode_config.num_bridge--; > + drm_modeset_unlock_all(dev); > +} > +EXPORT_SYMBOL(drm_bridge_cleanup); > + > int drm_encoder_init(struct drm_device *dev, > struct drm_encoder *encoder, > const struct drm_encoder_funcs *funcs, > @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr > total_objects += dev->mode_config.num_crtc; > total_objects += dev->mode_config.num_connector; > total_objects += dev->mode_config.num_encoder; > + total_objects += dev->mode_config.num_bridge; > > group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL); > if (!group->id_list) > @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr > group->num_crtcs = 0; > group->num_connectors = 0; > group->num_encoders = 0; > + group->num_bridges = 0; > return 0; > } > > @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, > struct drm_crtc *crtc; > struct drm_encoder *encoder; > struct drm_connector *connector; > + struct drm_bridge *bridge; > int ret; > > if ((ret = drm_mode_group_init(dev, group))) > @@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, > group->id_list[group->num_crtcs + group->num_encoders + > group->num_connectors++] = connector->base.id; > > + list_for_each_entry(bridge, &dev->mode_config.bridge_list, head) > + group->id_list[group->num_crtcs + group->num_encoders + > + group->num_connectors + group->num_bridges++] = > + bridge->base.id; > + > return 0; > } > EXPORT_SYMBOL(drm_mode_group_init_legacy_group); > @@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev) > INIT_LIST_HEAD(&dev->mode_config.fb_list); > INIT_LIST_HEAD(&dev->mode_config.crtc_list); > INIT_LIST_HEAD(&dev->mode_config.connector_list); > + INIT_LIST_HEAD(&dev->mode_config.bridge_list); > INIT_LIST_HEAD(&dev->mode_config.encoder_list); > INIT_LIST_HEAD(&dev->mode_config.property_list); > INIT_LIST_HEAD(&dev->mode_config.property_blob_list); > @@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) > struct drm_connector *connector, *ot; > struct drm_crtc *crtc, *ct; > struct drm_encoder *encoder, *enct; > + struct drm_bridge *bridge, *brt; > struct drm_framebuffer *fb, *fbt; > struct drm_property *property, *pt; > struct drm_property_blob *blob, *bt; > @@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev) > encoder->funcs->destroy(encoder); > } > > + list_for_each_entry_safe(bridge, brt, > + &dev->mode_config.bridge_list, head) { > + bridge->funcs->destroy(bridge); > + } > + > list_for_each_entry_safe(connector, ot, > &dev->mode_config.connector_list, head) { > connector->funcs->destroy(connector); > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 6a64749..30139fe 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); > static bool drm_kms_helper_poll = true; > module_param_named(poll, drm_kms_helper_poll, bool, 0600); > > +static enum drm_connector_status detect_connection( > + struct drm_connector *connector, bool force) > +{ > + struct drm_encoder *encoder = connector->encoder; > + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; That won't work since the connector->encoder link is only set up at runtime when a mode is set. Furthermore it's possible for connectors to use different encoders. So if a connector can't figure out whether anything is connected itself but needs to poke the bridge that needs to be forward in a different fashion. One way would be to allow embedding both the drm_bridge an the drm encoder into an over structure and the let a bridge specific callback do the casting. > + > + /* > + * If there's a bridge attached to the encoder (attached to > + * the connector) and it can detect an active connection, > + * re-route the detect call to the bridge. > + */ > + if (bridge && bridge->funcs->detect) > + return bridge->funcs->detect(bridge, force); > + > + return connector->funcs->detect(connector, force); > +} > + > static void drm_mode_validate_flag(struct drm_connector *connector, > int flags) > { > @@ -137,7 +154,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > if (connector->funcs->force) > connector->funcs->force(connector); > } else { > - connector->status = connector->funcs->detect(connector, true); > + connector->status = detect_connection(connector, true); > } > > /* Re-enable polling in case the global poll config changed. */ > @@ -256,11 +273,22 @@ static void > drm_encoder_disable(struct drm_encoder *encoder) > { > struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; > + struct drm_bridge_helper_funcs *bridge_funcs; > + > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->disable(encoder->bridge); > + } > > if (encoder_funcs->disable) > (*encoder_funcs->disable)(encoder); > else > (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); > + > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->post_disable(encoder->bridge); > + } > } > > /** > @@ -392,6 +420,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; > struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > struct drm_encoder_helper_funcs *encoder_funcs; > + struct drm_bridge_helper_funcs *bridge_funcs; > int saved_x, saved_y; > struct drm_encoder *encoder; > bool ret = true; > @@ -424,6 +453,17 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > > if (encoder->crtc != crtc) > continue; > + > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + ret = bridge_funcs->mode_fixup(encoder->bridge, mode, > + adjusted_mode); > + if (!ret) { > + DRM_DEBUG_KMS("Bridge fixup failed\n"); > + goto done; > + } > + } I strongly believe that this isn't good enough for many cases and that often the bridge and encoder want to more closely discuss how to set up the link between them (e.g. all the nonsense dsi panels might want). > + > encoder_funcs = encoder->helper_private; > if (!(ret = encoder_funcs->mode_fixup(encoder, mode, > adjusted_mode))) { > @@ -443,9 +483,20 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > > if (encoder->crtc != crtc) > continue; > + > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->disable(encoder->bridge); > + } > + > encoder_funcs = encoder->helper_private; > /* Disable the encoders as the first thing we do. */ > encoder_funcs->prepare(encoder); > + > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->post_disable(encoder->bridge); > + } > } > > drm_crtc_prepare_encoders(dev); > @@ -469,6 +520,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > mode->base.id, mode->name); > encoder_funcs = encoder->helper_private; > encoder_funcs->mode_set(encoder, mode, adjusted_mode); > + > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->mode_set(encoder->bridge, mode, > + adjusted_mode); > + } > } > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > @@ -479,9 +536,18 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > if (encoder->crtc != crtc) > continue; > > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->pre_enable(encoder->bridge); > + } > + > encoder_funcs = encoder->helper_private; > encoder_funcs->commit(encoder); > > + if (encoder->bridge) { > + bridge_funcs = encoder->bridge->helper_private; > + bridge_funcs->enable(encoder->bridge); > + } > } > > /* Store real post-adjustment hardware mode. */ > @@ -856,8 +922,9 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) > void drm_helper_connector_dpms(struct drm_connector *connector, int mode) > { > struct drm_encoder *encoder = connector->encoder; > + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > struct drm_crtc *crtc = encoder ? encoder->crtc : NULL; > - int old_dpms; > + int old_dpms, encoder_dpms = DRM_MODE_DPMS_OFF; > > if (mode == connector->dpms) > return; > @@ -865,6 +932,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) > old_dpms = connector->dpms; > connector->dpms = mode; > > + if (encoder) > + encoder_dpms = drm_helper_choose_encoder_dpms(encoder); > + > /* from off to on, do crtc then encoder */ > if (mode < old_dpms) { > if (crtc) { > @@ -876,18 +946,28 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) > if (encoder) { > struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; > if (encoder_funcs->dpms) > - (*encoder_funcs->dpms) (encoder, > - drm_helper_choose_encoder_dpms(encoder)); > + (*encoder_funcs->dpms) (encoder, encoder_dpms); > + } > + if (bridge) { > + struct drm_bridge_helper_funcs *bridge_funcs; > + > + bridge_funcs = bridge->helper_private; > + bridge_funcs->dpms(bridge, encoder_dpms); Here we only have one dpms callback, no post_disable/pre_enable. Presuming you have a bridge that really needs the former this will fall over when doing dpms transitions. > } > } > > /* from on to off, do encoder then crtc */ > if (mode > old_dpms) { > + if (bridge) { > + struct drm_bridge_helper_funcs *bridge_funcs; > + > + bridge_funcs = bridge->helper_private; > + bridge_funcs->dpms(bridge, encoder_dpms); > + } > if (encoder) { > struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; > if (encoder_funcs->dpms) > - (*encoder_funcs->dpms) (encoder, > - drm_helper_choose_encoder_dpms(encoder)); > + (*encoder_funcs->dpms) (encoder, encoder_dpms); > } > if (crtc) { > struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > @@ -924,9 +1004,11 @@ int drm_helper_resume_force_mode(struct drm_device *dev) > { > struct drm_crtc *crtc; > struct drm_encoder *encoder; > + struct drm_bridge *bridge; > + struct drm_bridge_helper_funcs *bridge_funcs; > struct drm_encoder_helper_funcs *encoder_funcs; > struct drm_crtc_helper_funcs *crtc_funcs; > - int ret; > + int ret, encoder_dpms; > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > > @@ -946,10 +1028,20 @@ int drm_helper_resume_force_mode(struct drm_device *dev) > if(encoder->crtc != crtc) > continue; > > + encoder_dpms = drm_helper_choose_encoder_dpms( > + encoder); > + > + bridge = encoder->bridge; > + if (bridge) { > + bridge_funcs = bridge->helper_private; > + bridge_funcs->dpms(bridge, > + encoder_dpms); > + } > + > encoder_funcs = encoder->helper_private; > if (encoder_funcs->dpms) > (*encoder_funcs->dpms) (encoder, > - drm_helper_choose_encoder_dpms(encoder)); > + encoder_dpms); > } > > crtc_funcs = crtc->helper_private; > @@ -1006,7 +1098,7 @@ static void output_poll_execute(struct work_struct *work) > !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) > continue; > > - connector->status = connector->funcs->detect(connector, false); > + connector->status = detect_connection(connector, false); > if (old_status != connector->status) { > const char *old, *new; > > @@ -1092,7 +1184,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) > > old_status = connector->status; > > - connector->status = connector->funcs->detect(connector, false); > + connector->status = detect_connection(connector, false); > DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", > connector->base.id, > drm_get_connector_name(connector), > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > index 2290b3b..a912d90 100644 > --- a/drivers/gpu/drm/drm_sysfs.c > +++ b/drivers/gpu/drm/drm_sysfs.c > @@ -184,6 +184,8 @@ static ssize_t status_show(struct device *device, > char *buf) > { > struct drm_connector *connector = to_drm_connector(device); > + struct drm_encoder *encoder = connector->encoder; > + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > enum drm_connector_status status; > int ret; > > @@ -191,7 +193,11 @@ static ssize_t status_show(struct device *device, > if (ret) > return ret; > > - status = connector->funcs->detect(connector, true); > + if (bridge && bridge->funcs->detect) > + status = bridge->funcs->detect(bridge, true); > + else > + status = connector->funcs->detect(connector, true); > + > mutex_unlock(&connector->dev->mode_config.mutex); > > return snprintf(buf, PAGE_SIZE, "%s\n", > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index fa12a2f..f020957 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -49,6 +49,7 @@ struct drm_clip_rect; > #define DRM_MODE_OBJECT_FB 0xfbfbfbfb > #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb > #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee > +#define DRM_MODE_OBJECT_BRIDGE 0xbdbdbdbd > > struct drm_mode_object { > uint32_t id; > @@ -305,6 +306,7 @@ struct drm_connector; > struct drm_encoder; > struct drm_pending_vblank_event; > struct drm_plane; > +struct drm_bridge; > > /** > * drm_crtc_funcs - control CRTCs for a given device > @@ -507,6 +509,7 @@ struct drm_encoder_funcs { > * @possible_crtcs: bitmask of potential CRTC bindings > * @possible_clones: bitmask of potential sibling encoders for cloning > * @crtc: currently bound CRTC > + * @bridge: bridge associated to the encoder > * @funcs: control functions > * @helper_private: mid-layer private data > * > @@ -523,6 +526,7 @@ struct drm_encoder { > uint32_t possible_clones; > > struct drm_crtc *crtc; > + struct drm_bridge *bridge; > const struct drm_encoder_funcs *funcs; > void *helper_private; > }; > @@ -683,6 +687,41 @@ struct drm_plane { > }; > > /** > + * drm_bridge_funcs - drm_bridge control functions > + * @detect: Checks if something is connected to the bridge. If this is > + * implemented, it should take precedence over connector->detect() > + * since the connector may not be able to deduce whether something is > + * connected downstream of the bridge. > + * @destroy: make object go away > + */ > +struct drm_bridge_funcs { > + enum drm_connector_status (*detect)(struct drm_bridge *bridge, > + bool force); > + void (*destroy)(struct drm_bridge *bridge); > +}; > + > +/** > + * drm_bridge - central DRM bridge control structure > + * @dev: DRM device this bridge belongs to > + * @head: list management > + * @base: base mode object > + * @connector_type: the type of connector this bridge can associate with > + * @funcs: control functions > + * @helper_private: mid-layer private data > + */ > +struct drm_bridge { > + struct drm_device *dev; > + struct list_head head; > + > + struct drm_mode_object base; > + > + int connector_type; > + > + const struct drm_bridge_funcs *funcs; > + void *helper_private; > +}; > + > +/** > * drm_mode_set - new values for a CRTC config change > * @head: list management > * @fb: framebuffer to use for new config > @@ -742,6 +781,7 @@ struct drm_mode_group { > uint32_t num_crtcs; > uint32_t num_encoders; > uint32_t num_connectors; > + uint32_t num_bridges; > > /* list of object IDs for this group */ > uint32_t *id_list; > @@ -756,6 +796,8 @@ struct drm_mode_group { > * @fb_list: list of framebuffers available > * @num_connector: number of connectors on this device > * @connector_list: list of connector objects > + * @num_bridge: number of bridges on this device > + * @bridge_list: list of bridge objects > * @num_encoder: number of encoders on this device > * @encoder_list: list of encoder objects > * @num_crtc: number of CRTCs on this device > @@ -793,6 +835,8 @@ struct drm_mode_config { > > int num_connector; > struct list_head connector_list; > + int num_bridge; > + struct list_head bridge_list; > int num_encoder; > struct list_head encoder_list; > int num_plane; > @@ -878,6 +922,10 @@ extern void drm_connector_cleanup(struct drm_connector *connector); > /* helper to unplug all connectors from sysfs for device */ > extern void drm_connector_unplug_all(struct drm_device *dev); > > +extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, > + const struct drm_bridge_funcs *funcs); > +extern void drm_bridge_cleanup(struct drm_bridge *bridge); > + > extern int drm_encoder_init(struct drm_device *dev, > struct drm_encoder *encoder, > const struct drm_encoder_funcs *funcs, > diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h > index f43d556..89a11f9 100644 > --- a/include/drm/drm_crtc_helper.h > +++ b/include/drm/drm_crtc_helper.h > @@ -125,6 +125,34 @@ struct drm_connector_helper_funcs { > struct drm_encoder *(*best_encoder)(struct drm_connector *connector); > }; > > +/** > + * drm_bridge_helper_funcs - helper operations for bridges (all required) > + * @dpms: Control power levels on the bridge. If the mode passed in is > + * unsupported, the provider must use the next lowest power level. > + * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge > + * @disable: Called right before encoder prepare, disables the bridge > + * @post_disable: Called right after encoder prepare, for lockstepped disable > + * @mode_set: Set this mode to the bridge > + * @pre_enable: Called right before encoder commit, for lockstepped commit > + * @enable: Called right after encoder commit, enables the bridge > + * > + * The helper operations are called by the mid-layer CRTC helper, however they > + * can also be called directly by drivers that don't use the helper functions. > + */ > +struct drm_bridge_helper_funcs { > + void (*dpms)(struct drm_bridge *bridge, int mode); > + bool (*mode_fixup)(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode); > + void (*disable)(struct drm_bridge *bridge); > + void (*post_disable)(struct drm_bridge *bridge); > + void (*mode_set)(struct drm_bridge *bridge, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode); > + void (*pre_enable)(struct drm_bridge *bridge); > + void (*enable)(struct drm_bridge *bridge); > +}; > + > extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); > extern void drm_helper_disable_unused_functions(struct drm_device *dev); > extern int drm_crtc_helper_set_config(struct drm_mode_set *set); > @@ -160,6 +188,12 @@ static inline void drm_connector_helper_add(struct drm_connector *connector, > connector->helper_private = (void *)funcs; > } > > +static inline void drm_bridge_helper_add(struct drm_bridge *bridge, > + const struct drm_bridge_helper_funcs *funcs) > +{ > + bridge->helper_private = (void *)funcs; > +} > + > extern int drm_helper_resume_force_mode(struct drm_device *dev); > extern void drm_kms_helper_poll_init(struct drm_device *dev); > extern void drm_kms_helper_poll_fini(struct drm_device *dev); > -- > 1.8.3 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@chromium.org> wrote: >> This patch adds the notion of a drm_bridge. A bridge is a chained >> device which hangs off an encoder. The drm driver using the bridge >> should provide the association between encoder and bridge. Once a >> bridge is associated with an encoder, it will participate in mode >> set, dpms, and optionally connection detection. >> >> Since a connector may not be able to determine the connection state >> downstream of the bridge, bridges may implement detect() which will take >> precedence over connector detect() if the bridge is already attached >> to an encoder and that encoder is already attached to the connector. >> In practical terms, this requires the drm driver to make these >> associations at init time, which is fine for SoC applications where >> this link is static. >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > A few comments below. I think overall any such infrastructure simply > needs to demonstrate viability on a few drivers, that makes it much > simpler to check whether the interfaces make sense. Just a quick note.. I have a branch of msm where I've been using a slightly earlier version of this (and in next day or so will be rebased on this version). and I guess Sean is using this on a driver or two. So that's at least two drivers ;-) BR, -R > Generally I'd make > more sense for me if the bridge is just an implementation detail of a > given encoder and if the bridge would not be exposed to the crtc > helpers. With that abstraction chaining would be more natural imo and > we'd also have a much higher chance that bridge drivers work accross > different drm drivers: Atm you can run some encoder stuff in the crtc > callbacks and the other way round (and pretty much every driver for a > bit more complicated hw does that), and every driver can differ in > those tricks a bit. If we bake the bridge callbacks into the crtc > helpers I expect that for bridges with tricky ordering constraints > this will become a giant mess. So I'd prefer much if this would work > like drm i2c slave encoders. > > Cheers, Daniel > >> --- >> drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++ >> drivers/gpu/drm/drm_crtc_helper.c | 112 ++++++++++++++++++++++++++++++++++---- >> drivers/gpu/drm/drm_sysfs.c | 8 ++- >> include/drm/drm_crtc.h | 48 ++++++++++++++++ >> include/drm/drm_crtc_helper.h | 34 ++++++++++++ >> 5 files changed, 241 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index fc83bb9..0311e2b 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev) >> } >> EXPORT_SYMBOL(drm_connector_unplug_all); >> >> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> + const struct drm_bridge_funcs *funcs) >> +{ >> + int ret; >> + >> + drm_modeset_lock_all(dev); >> + >> + ret = drm_mode_object_get(dev, &bridge->base, DRM_MODE_OBJECT_BRIDGE); >> + if (ret) >> + goto out; >> + >> + bridge->dev = dev; >> + bridge->funcs = funcs; >> + >> + list_add_tail(&bridge->head, &dev->mode_config.bridge_list); >> + dev->mode_config.num_bridge++; >> + >> + out: >> + drm_modeset_unlock_all(dev); >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_bridge_init); >> + >> +void drm_bridge_cleanup(struct drm_bridge *bridge) >> +{ >> + struct drm_device *dev = bridge->dev; >> + >> + drm_modeset_lock_all(dev); >> + drm_mode_object_put(dev, &bridge->base); >> + list_del(&bridge->head); >> + dev->mode_config.num_bridge--; >> + drm_modeset_unlock_all(dev); >> +} >> +EXPORT_SYMBOL(drm_bridge_cleanup); >> + >> int drm_encoder_init(struct drm_device *dev, >> struct drm_encoder *encoder, >> const struct drm_encoder_funcs *funcs, >> @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr >> total_objects += dev->mode_config.num_crtc; >> total_objects += dev->mode_config.num_connector; >> total_objects += dev->mode_config.num_encoder; >> + total_objects += dev->mode_config.num_bridge; >> >> group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL); >> if (!group->id_list) >> @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr >> group->num_crtcs = 0; >> group->num_connectors = 0; >> group->num_encoders = 0; >> + group->num_bridges = 0; >> return 0; >> } >> >> @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, >> struct drm_crtc *crtc; >> struct drm_encoder *encoder; >> struct drm_connector *connector; >> + struct drm_bridge *bridge; >> int ret; >> >> if ((ret = drm_mode_group_init(dev, group))) >> @@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, >> group->id_list[group->num_crtcs + group->num_encoders + >> group->num_connectors++] = connector->base.id; >> >> + list_for_each_entry(bridge, &dev->mode_config.bridge_list, head) >> + group->id_list[group->num_crtcs + group->num_encoders + >> + group->num_connectors + group->num_bridges++] = >> + bridge->base.id; >> + >> return 0; >> } >> EXPORT_SYMBOL(drm_mode_group_init_legacy_group); >> @@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev) >> INIT_LIST_HEAD(&dev->mode_config.fb_list); >> INIT_LIST_HEAD(&dev->mode_config.crtc_list); >> INIT_LIST_HEAD(&dev->mode_config.connector_list); >> + INIT_LIST_HEAD(&dev->mode_config.bridge_list); >> INIT_LIST_HEAD(&dev->mode_config.encoder_list); >> INIT_LIST_HEAD(&dev->mode_config.property_list); >> INIT_LIST_HEAD(&dev->mode_config.property_blob_list); >> @@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> struct drm_connector *connector, *ot; >> struct drm_crtc *crtc, *ct; >> struct drm_encoder *encoder, *enct; >> + struct drm_bridge *bridge, *brt; >> struct drm_framebuffer *fb, *fbt; >> struct drm_property *property, *pt; >> struct drm_property_blob *blob, *bt; >> @@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> encoder->funcs->destroy(encoder); >> } >> >> + list_for_each_entry_safe(bridge, brt, >> + &dev->mode_config.bridge_list, head) { >> + bridge->funcs->destroy(bridge); >> + } >> + >> list_for_each_entry_safe(connector, ot, >> &dev->mode_config.connector_list, head) { >> connector->funcs->destroy(connector); >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c >> index 6a64749..30139fe 100644 >> --- a/drivers/gpu/drm/drm_crtc_helper.c >> +++ b/drivers/gpu/drm/drm_crtc_helper.c >> @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); >> static bool drm_kms_helper_poll = true; >> module_param_named(poll, drm_kms_helper_poll, bool, 0600); >> >> +static enum drm_connector_status detect_connection( >> + struct drm_connector *connector, bool force) >> +{ >> + struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > > That won't work since the connector->encoder link is only set up at > runtime when a mode is set. Furthermore it's possible for connectors > to use different encoders. > > So if a connector can't figure out whether anything is connected > itself but needs to poke the bridge that needs to be forward in a > different fashion. One way would be to allow embedding both the > drm_bridge an the drm encoder into an over structure and the let a > bridge specific callback do the casting. > >> + >> + /* >> + * If there's a bridge attached to the encoder (attached to >> + * the connector) and it can detect an active connection, >> + * re-route the detect call to the bridge. >> + */ >> + if (bridge && bridge->funcs->detect) >> + return bridge->funcs->detect(bridge, force); >> + >> + return connector->funcs->detect(connector, force); >> +} >> + >> static void drm_mode_validate_flag(struct drm_connector *connector, >> int flags) >> { >> @@ -137,7 +154,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, >> if (connector->funcs->force) >> connector->funcs->force(connector); >> } else { >> - connector->status = connector->funcs->detect(connector, true); >> + connector->status = detect_connection(connector, true); >> } >> >> /* Re-enable polling in case the global poll config changed. */ >> @@ -256,11 +273,22 @@ static void >> drm_encoder_disable(struct drm_encoder *encoder) >> { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->disable(encoder->bridge); >> + } >> >> if (encoder_funcs->disable) >> (*encoder_funcs->disable)(encoder); >> else >> (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->post_disable(encoder->bridge); >> + } >> } >> >> /** >> @@ -392,6 +420,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; >> struct drm_encoder_helper_funcs *encoder_funcs; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> int saved_x, saved_y; >> struct drm_encoder *encoder; >> bool ret = true; >> @@ -424,6 +453,17 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> >> if (encoder->crtc != crtc) >> continue; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + ret = bridge_funcs->mode_fixup(encoder->bridge, mode, >> + adjusted_mode); >> + if (!ret) { >> + DRM_DEBUG_KMS("Bridge fixup failed\n"); >> + goto done; >> + } >> + } > > I strongly believe that this isn't good enough for many cases and that > often the bridge and encoder want to more closely discuss how to set > up the link between them (e.g. all the nonsense dsi panels might > want). > >> + >> encoder_funcs = encoder->helper_private; >> if (!(ret = encoder_funcs->mode_fixup(encoder, mode, >> adjusted_mode))) { >> @@ -443,9 +483,20 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> >> if (encoder->crtc != crtc) >> continue; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->disable(encoder->bridge); >> + } >> + >> encoder_funcs = encoder->helper_private; >> /* Disable the encoders as the first thing we do. */ >> encoder_funcs->prepare(encoder); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->post_disable(encoder->bridge); >> + } >> } >> >> drm_crtc_prepare_encoders(dev); >> @@ -469,6 +520,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> mode->base.id, mode->name); >> encoder_funcs = encoder->helper_private; >> encoder_funcs->mode_set(encoder, mode, adjusted_mode); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->mode_set(encoder->bridge, mode, >> + adjusted_mode); >> + } >> } >> >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >> @@ -479,9 +536,18 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> if (encoder->crtc != crtc) >> continue; >> >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->pre_enable(encoder->bridge); >> + } >> + >> encoder_funcs = encoder->helper_private; >> encoder_funcs->commit(encoder); >> >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->enable(encoder->bridge); >> + } >> } >> >> /* Store real post-adjustment hardware mode. */ >> @@ -856,8 +922,9 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) >> void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> { >> struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; >> struct drm_crtc *crtc = encoder ? encoder->crtc : NULL; >> - int old_dpms; >> + int old_dpms, encoder_dpms = DRM_MODE_DPMS_OFF; >> >> if (mode == connector->dpms) >> return; >> @@ -865,6 +932,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> old_dpms = connector->dpms; >> connector->dpms = mode; >> >> + if (encoder) >> + encoder_dpms = drm_helper_choose_encoder_dpms(encoder); >> + >> /* from off to on, do crtc then encoder */ >> if (mode < old_dpms) { >> if (crtc) { >> @@ -876,18 +946,28 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> if (encoder) { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> - (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); >> + } >> + if (bridge) { >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, encoder_dpms); > > Here we only have one dpms callback, no post_disable/pre_enable. > Presuming you have a bridge that really needs the former this will > fall over when doing dpms transitions. > >> } >> } >> >> /* from on to off, do encoder then crtc */ >> if (mode > old_dpms) { >> + if (bridge) { >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, encoder_dpms); >> + } >> if (encoder) { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> - (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); >> } >> if (crtc) { >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; >> @@ -924,9 +1004,11 @@ int drm_helper_resume_force_mode(struct drm_device *dev) >> { >> struct drm_crtc *crtc; >> struct drm_encoder *encoder; >> + struct drm_bridge *bridge; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> struct drm_encoder_helper_funcs *encoder_funcs; >> struct drm_crtc_helper_funcs *crtc_funcs; >> - int ret; >> + int ret, encoder_dpms; >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { >> >> @@ -946,10 +1028,20 @@ int drm_helper_resume_force_mode(struct drm_device *dev) >> if(encoder->crtc != crtc) >> continue; >> >> + encoder_dpms = drm_helper_choose_encoder_dpms( >> + encoder); >> + >> + bridge = encoder->bridge; >> + if (bridge) { >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, >> + encoder_dpms); >> + } >> + >> encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + encoder_dpms); >> } >> >> crtc_funcs = crtc->helper_private; >> @@ -1006,7 +1098,7 @@ static void output_poll_execute(struct work_struct *work) >> !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) >> continue; >> >> - connector->status = connector->funcs->detect(connector, false); >> + connector->status = detect_connection(connector, false); >> if (old_status != connector->status) { >> const char *old, *new; >> >> @@ -1092,7 +1184,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) >> >> old_status = connector->status; >> >> - connector->status = connector->funcs->detect(connector, false); >> + connector->status = detect_connection(connector, false); >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", >> connector->base.id, >> drm_get_connector_name(connector), >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index 2290b3b..a912d90 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -184,6 +184,8 @@ static ssize_t status_show(struct device *device, >> char *buf) >> { >> struct drm_connector *connector = to_drm_connector(device); >> + struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; >> enum drm_connector_status status; >> int ret; >> >> @@ -191,7 +193,11 @@ static ssize_t status_show(struct device *device, >> if (ret) >> return ret; >> >> - status = connector->funcs->detect(connector, true); >> + if (bridge && bridge->funcs->detect) >> + status = bridge->funcs->detect(bridge, true); >> + else >> + status = connector->funcs->detect(connector, true); >> + >> mutex_unlock(&connector->dev->mode_config.mutex); >> >> return snprintf(buf, PAGE_SIZE, "%s\n", >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index fa12a2f..f020957 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -49,6 +49,7 @@ struct drm_clip_rect; >> #define DRM_MODE_OBJECT_FB 0xfbfbfbfb >> #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb >> #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee >> +#define DRM_MODE_OBJECT_BRIDGE 0xbdbdbdbd >> >> struct drm_mode_object { >> uint32_t id; >> @@ -305,6 +306,7 @@ struct drm_connector; >> struct drm_encoder; >> struct drm_pending_vblank_event; >> struct drm_plane; >> +struct drm_bridge; >> >> /** >> * drm_crtc_funcs - control CRTCs for a given device >> @@ -507,6 +509,7 @@ struct drm_encoder_funcs { >> * @possible_crtcs: bitmask of potential CRTC bindings >> * @possible_clones: bitmask of potential sibling encoders for cloning >> * @crtc: currently bound CRTC >> + * @bridge: bridge associated to the encoder >> * @funcs: control functions >> * @helper_private: mid-layer private data >> * >> @@ -523,6 +526,7 @@ struct drm_encoder { >> uint32_t possible_clones; >> >> struct drm_crtc *crtc; >> + struct drm_bridge *bridge; >> const struct drm_encoder_funcs *funcs; >> void *helper_private; >> }; >> @@ -683,6 +687,41 @@ struct drm_plane { >> }; >> >> /** >> + * drm_bridge_funcs - drm_bridge control functions >> + * @detect: Checks if something is connected to the bridge. If this is >> + * implemented, it should take precedence over connector->detect() >> + * since the connector may not be able to deduce whether something is >> + * connected downstream of the bridge. >> + * @destroy: make object go away >> + */ >> +struct drm_bridge_funcs { >> + enum drm_connector_status (*detect)(struct drm_bridge *bridge, >> + bool force); >> + void (*destroy)(struct drm_bridge *bridge); >> +}; >> + >> +/** >> + * drm_bridge - central DRM bridge control structure >> + * @dev: DRM device this bridge belongs to >> + * @head: list management >> + * @base: base mode object >> + * @connector_type: the type of connector this bridge can associate with >> + * @funcs: control functions >> + * @helper_private: mid-layer private data >> + */ >> +struct drm_bridge { >> + struct drm_device *dev; >> + struct list_head head; >> + >> + struct drm_mode_object base; >> + >> + int connector_type; >> + >> + const struct drm_bridge_funcs *funcs; >> + void *helper_private; >> +}; >> + >> +/** >> * drm_mode_set - new values for a CRTC config change >> * @head: list management >> * @fb: framebuffer to use for new config >> @@ -742,6 +781,7 @@ struct drm_mode_group { >> uint32_t num_crtcs; >> uint32_t num_encoders; >> uint32_t num_connectors; >> + uint32_t num_bridges; >> >> /* list of object IDs for this group */ >> uint32_t *id_list; >> @@ -756,6 +796,8 @@ struct drm_mode_group { >> * @fb_list: list of framebuffers available >> * @num_connector: number of connectors on this device >> * @connector_list: list of connector objects >> + * @num_bridge: number of bridges on this device >> + * @bridge_list: list of bridge objects >> * @num_encoder: number of encoders on this device >> * @encoder_list: list of encoder objects >> * @num_crtc: number of CRTCs on this device >> @@ -793,6 +835,8 @@ struct drm_mode_config { >> >> int num_connector; >> struct list_head connector_list; >> + int num_bridge; >> + struct list_head bridge_list; >> int num_encoder; >> struct list_head encoder_list; >> int num_plane; >> @@ -878,6 +922,10 @@ extern void drm_connector_cleanup(struct drm_connector *connector); >> /* helper to unplug all connectors from sysfs for device */ >> extern void drm_connector_unplug_all(struct drm_device *dev); >> >> +extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> + const struct drm_bridge_funcs *funcs); >> +extern void drm_bridge_cleanup(struct drm_bridge *bridge); >> + >> extern int drm_encoder_init(struct drm_device *dev, >> struct drm_encoder *encoder, >> const struct drm_encoder_funcs *funcs, >> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h >> index f43d556..89a11f9 100644 >> --- a/include/drm/drm_crtc_helper.h >> +++ b/include/drm/drm_crtc_helper.h >> @@ -125,6 +125,34 @@ struct drm_connector_helper_funcs { >> struct drm_encoder *(*best_encoder)(struct drm_connector *connector); >> }; >> >> +/** >> + * drm_bridge_helper_funcs - helper operations for bridges (all required) >> + * @dpms: Control power levels on the bridge. If the mode passed in is >> + * unsupported, the provider must use the next lowest power level. >> + * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge >> + * @disable: Called right before encoder prepare, disables the bridge >> + * @post_disable: Called right after encoder prepare, for lockstepped disable >> + * @mode_set: Set this mode to the bridge >> + * @pre_enable: Called right before encoder commit, for lockstepped commit >> + * @enable: Called right after encoder commit, enables the bridge >> + * >> + * The helper operations are called by the mid-layer CRTC helper, however they >> + * can also be called directly by drivers that don't use the helper functions. >> + */ >> +struct drm_bridge_helper_funcs { >> + void (*dpms)(struct drm_bridge *bridge, int mode); >> + bool (*mode_fixup)(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode); >> + void (*disable)(struct drm_bridge *bridge); >> + void (*post_disable)(struct drm_bridge *bridge); >> + void (*mode_set)(struct drm_bridge *bridge, >> + struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode); >> + void (*pre_enable)(struct drm_bridge *bridge); >> + void (*enable)(struct drm_bridge *bridge); >> +}; >> + >> extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); >> extern void drm_helper_disable_unused_functions(struct drm_device *dev); >> extern int drm_crtc_helper_set_config(struct drm_mode_set *set); >> @@ -160,6 +188,12 @@ static inline void drm_connector_helper_add(struct drm_connector *connector, >> connector->helper_private = (void *)funcs; >> } >> >> +static inline void drm_bridge_helper_add(struct drm_bridge *bridge, >> + const struct drm_bridge_helper_funcs *funcs) >> +{ >> + bridge->helper_private = (void *)funcs; >> +} >> + >> extern int drm_helper_resume_force_mode(struct drm_device *dev); >> extern void drm_kms_helper_poll_init(struct drm_device *dev); >> extern void drm_kms_helper_poll_fini(struct drm_device *dev); >> -- >> 1.8.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Aug 8, 2013 at 5:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@chromium.org> wrote: >> This patch adds the notion of a drm_bridge. A bridge is a chained >> device which hangs off an encoder. The drm driver using the bridge >> should provide the association between encoder and bridge. Once a >> bridge is associated with an encoder, it will participate in mode >> set, dpms, and optionally connection detection. >> >> Since a connector may not be able to determine the connection state >> downstream of the bridge, bridges may implement detect() which will take >> precedence over connector detect() if the bridge is already attached >> to an encoder and that encoder is already attached to the connector. >> In practical terms, this requires the drm driver to make these >> associations at init time, which is fine for SoC applications where >> this link is static. >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > A few comments below. I think overall any such infrastructure simply > needs to demonstrate viability on a few drivers, that makes it much > simpler to check whether the interfaces make sense. We have two bridges using it here, and we're working on adding a third. Rob want to add one too. > Generally I'd make > more sense for me if the bridge is just an implementation detail of a > given encoder and if the bridge would not be exposed to the crtc > helpers. With that abstraction chaining would be more natural imo and > we'd also have a much higher chance that bridge drivers work accross > different drm drivers: Atm you can run some encoder stuff in the crtc > callbacks and the other way round (and pretty much every driver for a > bit more complicated hw does that), and every driver can differ in > those tricks a bit. If we bake the bridge callbacks into the crtc > helpers I expect that for bridges with tricky ordering constraints > this will become a giant mess. So I'd prefer much if this would work > like drm i2c slave encoders. Interestingly, this is how we started. We first put these bridges drivers in the i2c/ dir and called them directly from exynos. Then things grew, and became very invasive in the host driver. So we added a new interface which allowed us to implement this properly in all drivers in a way which also extends to more drivers. Stéphane > > Cheers, Daniel > >> --- >> drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++ >> drivers/gpu/drm/drm_crtc_helper.c | 112 ++++++++++++++++++++++++++++++++++---- >> drivers/gpu/drm/drm_sysfs.c | 8 ++- >> include/drm/drm_crtc.h | 48 ++++++++++++++++ >> include/drm/drm_crtc_helper.h | 34 ++++++++++++ >> 5 files changed, 241 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index fc83bb9..0311e2b 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev) >> } >> EXPORT_SYMBOL(drm_connector_unplug_all); >> >> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> + const struct drm_bridge_funcs *funcs) >> +{ >> + int ret; >> + >> + drm_modeset_lock_all(dev); >> + >> + ret = drm_mode_object_get(dev, &bridge->base, DRM_MODE_OBJECT_BRIDGE); >> + if (ret) >> + goto out; >> + >> + bridge->dev = dev; >> + bridge->funcs = funcs; >> + >> + list_add_tail(&bridge->head, &dev->mode_config.bridge_list); >> + dev->mode_config.num_bridge++; >> + >> + out: >> + drm_modeset_unlock_all(dev); >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_bridge_init); >> + >> +void drm_bridge_cleanup(struct drm_bridge *bridge) >> +{ >> + struct drm_device *dev = bridge->dev; >> + >> + drm_modeset_lock_all(dev); >> + drm_mode_object_put(dev, &bridge->base); >> + list_del(&bridge->head); >> + dev->mode_config.num_bridge--; >> + drm_modeset_unlock_all(dev); >> +} >> +EXPORT_SYMBOL(drm_bridge_cleanup); >> + >> int drm_encoder_init(struct drm_device *dev, >> struct drm_encoder *encoder, >> const struct drm_encoder_funcs *funcs, >> @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr >> total_objects += dev->mode_config.num_crtc; >> total_objects += dev->mode_config.num_connector; >> total_objects += dev->mode_config.num_encoder; >> + total_objects += dev->mode_config.num_bridge; >> >> group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL); >> if (!group->id_list) >> @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr >> group->num_crtcs = 0; >> group->num_connectors = 0; >> group->num_encoders = 0; >> + group->num_bridges = 0; >> return 0; >> } >> >> @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, >> struct drm_crtc *crtc; >> struct drm_encoder *encoder; >> struct drm_connector *connector; >> + struct drm_bridge *bridge; >> int ret; >> >> if ((ret = drm_mode_group_init(dev, group))) >> @@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, >> group->id_list[group->num_crtcs + group->num_encoders + >> group->num_connectors++] = connector->base.id; >> >> + list_for_each_entry(bridge, &dev->mode_config.bridge_list, head) >> + group->id_list[group->num_crtcs + group->num_encoders + >> + group->num_connectors + group->num_bridges++] = >> + bridge->base.id; >> + >> return 0; >> } >> EXPORT_SYMBOL(drm_mode_group_init_legacy_group); >> @@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev) >> INIT_LIST_HEAD(&dev->mode_config.fb_list); >> INIT_LIST_HEAD(&dev->mode_config.crtc_list); >> INIT_LIST_HEAD(&dev->mode_config.connector_list); >> + INIT_LIST_HEAD(&dev->mode_config.bridge_list); >> INIT_LIST_HEAD(&dev->mode_config.encoder_list); >> INIT_LIST_HEAD(&dev->mode_config.property_list); >> INIT_LIST_HEAD(&dev->mode_config.property_blob_list); >> @@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> struct drm_connector *connector, *ot; >> struct drm_crtc *crtc, *ct; >> struct drm_encoder *encoder, *enct; >> + struct drm_bridge *bridge, *brt; >> struct drm_framebuffer *fb, *fbt; >> struct drm_property *property, *pt; >> struct drm_property_blob *blob, *bt; >> @@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> encoder->funcs->destroy(encoder); >> } >> >> + list_for_each_entry_safe(bridge, brt, >> + &dev->mode_config.bridge_list, head) { >> + bridge->funcs->destroy(bridge); >> + } >> + >> list_for_each_entry_safe(connector, ot, >> &dev->mode_config.connector_list, head) { >> connector->funcs->destroy(connector); >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c >> index 6a64749..30139fe 100644 >> --- a/drivers/gpu/drm/drm_crtc_helper.c >> +++ b/drivers/gpu/drm/drm_crtc_helper.c >> @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); >> static bool drm_kms_helper_poll = true; >> module_param_named(poll, drm_kms_helper_poll, bool, 0600); >> >> +static enum drm_connector_status detect_connection( >> + struct drm_connector *connector, bool force) >> +{ >> + struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > > That won't work since the connector->encoder link is only set up at > runtime when a mode is set. Furthermore it's possible for connectors > to use different encoders. > > So if a connector can't figure out whether anything is connected > itself but needs to poke the bridge that needs to be forward in a > different fashion. One way would be to allow embedding both the > drm_bridge an the drm encoder into an over structure and the let a > bridge specific callback do the casting. > >> + >> + /* >> + * If there's a bridge attached to the encoder (attached to >> + * the connector) and it can detect an active connection, >> + * re-route the detect call to the bridge. >> + */ >> + if (bridge && bridge->funcs->detect) >> + return bridge->funcs->detect(bridge, force); >> + >> + return connector->funcs->detect(connector, force); >> +} >> + >> static void drm_mode_validate_flag(struct drm_connector *connector, >> int flags) >> { >> @@ -137,7 +154,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, >> if (connector->funcs->force) >> connector->funcs->force(connector); >> } else { >> - connector->status = connector->funcs->detect(connector, true); >> + connector->status = detect_connection(connector, true); >> } >> >> /* Re-enable polling in case the global poll config changed. */ >> @@ -256,11 +273,22 @@ static void >> drm_encoder_disable(struct drm_encoder *encoder) >> { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->disable(encoder->bridge); >> + } >> >> if (encoder_funcs->disable) >> (*encoder_funcs->disable)(encoder); >> else >> (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->post_disable(encoder->bridge); >> + } >> } >> >> /** >> @@ -392,6 +420,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; >> struct drm_encoder_helper_funcs *encoder_funcs; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> int saved_x, saved_y; >> struct drm_encoder *encoder; >> bool ret = true; >> @@ -424,6 +453,17 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> >> if (encoder->crtc != crtc) >> continue; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + ret = bridge_funcs->mode_fixup(encoder->bridge, mode, >> + adjusted_mode); >> + if (!ret) { >> + DRM_DEBUG_KMS("Bridge fixup failed\n"); >> + goto done; >> + } >> + } > > I strongly believe that this isn't good enough for many cases and that > often the bridge and encoder want to more closely discuss how to set > up the link between them (e.g. all the nonsense dsi panels might > want). > >> + >> encoder_funcs = encoder->helper_private; >> if (!(ret = encoder_funcs->mode_fixup(encoder, mode, >> adjusted_mode))) { >> @@ -443,9 +483,20 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> >> if (encoder->crtc != crtc) >> continue; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->disable(encoder->bridge); >> + } >> + >> encoder_funcs = encoder->helper_private; >> /* Disable the encoders as the first thing we do. */ >> encoder_funcs->prepare(encoder); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->post_disable(encoder->bridge); >> + } >> } >> >> drm_crtc_prepare_encoders(dev); >> @@ -469,6 +520,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> mode->base.id, mode->name); >> encoder_funcs = encoder->helper_private; >> encoder_funcs->mode_set(encoder, mode, adjusted_mode); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->mode_set(encoder->bridge, mode, >> + adjusted_mode); >> + } >> } >> >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >> @@ -479,9 +536,18 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> if (encoder->crtc != crtc) >> continue; >> >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->pre_enable(encoder->bridge); >> + } >> + >> encoder_funcs = encoder->helper_private; >> encoder_funcs->commit(encoder); >> >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->enable(encoder->bridge); >> + } >> } >> >> /* Store real post-adjustment hardware mode. */ >> @@ -856,8 +922,9 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) >> void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> { >> struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; >> struct drm_crtc *crtc = encoder ? encoder->crtc : NULL; >> - int old_dpms; >> + int old_dpms, encoder_dpms = DRM_MODE_DPMS_OFF; >> >> if (mode == connector->dpms) >> return; >> @@ -865,6 +932,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> old_dpms = connector->dpms; >> connector->dpms = mode; >> >> + if (encoder) >> + encoder_dpms = drm_helper_choose_encoder_dpms(encoder); >> + >> /* from off to on, do crtc then encoder */ >> if (mode < old_dpms) { >> if (crtc) { >> @@ -876,18 +946,28 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> if (encoder) { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> - (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); >> + } >> + if (bridge) { >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, encoder_dpms); > > Here we only have one dpms callback, no post_disable/pre_enable. > Presuming you have a bridge that really needs the former this will > fall over when doing dpms transitions. > >> } >> } >> >> /* from on to off, do encoder then crtc */ >> if (mode > old_dpms) { >> + if (bridge) { >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, encoder_dpms); >> + } >> if (encoder) { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> - (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); >> } >> if (crtc) { >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; >> @@ -924,9 +1004,11 @@ int drm_helper_resume_force_mode(struct drm_device *dev) >> { >> struct drm_crtc *crtc; >> struct drm_encoder *encoder; >> + struct drm_bridge *bridge; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> struct drm_encoder_helper_funcs *encoder_funcs; >> struct drm_crtc_helper_funcs *crtc_funcs; >> - int ret; >> + int ret, encoder_dpms; >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { >> >> @@ -946,10 +1028,20 @@ int drm_helper_resume_force_mode(struct drm_device *dev) >> if(encoder->crtc != crtc) >> continue; >> >> + encoder_dpms = drm_helper_choose_encoder_dpms( >> + encoder); >> + >> + bridge = encoder->bridge; >> + if (bridge) { >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, >> + encoder_dpms); >> + } >> + >> encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + encoder_dpms); >> } >> >> crtc_funcs = crtc->helper_private; >> @@ -1006,7 +1098,7 @@ static void output_poll_execute(struct work_struct *work) >> !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) >> continue; >> >> - connector->status = connector->funcs->detect(connector, false); >> + connector->status = detect_connection(connector, false); >> if (old_status != connector->status) { >> const char *old, *new; >> >> @@ -1092,7 +1184,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) >> >> old_status = connector->status; >> >> - connector->status = connector->funcs->detect(connector, false); >> + connector->status = detect_connection(connector, false); >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", >> connector->base.id, >> drm_get_connector_name(connector), >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index 2290b3b..a912d90 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -184,6 +184,8 @@ static ssize_t status_show(struct device *device, >> char *buf) >> { >> struct drm_connector *connector = to_drm_connector(device); >> + struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; >> enum drm_connector_status status; >> int ret; >> >> @@ -191,7 +193,11 @@ static ssize_t status_show(struct device *device, >> if (ret) >> return ret; >> >> - status = connector->funcs->detect(connector, true); >> + if (bridge && bridge->funcs->detect) >> + status = bridge->funcs->detect(bridge, true); >> + else >> + status = connector->funcs->detect(connector, true); >> + >> mutex_unlock(&connector->dev->mode_config.mutex); >> >> return snprintf(buf, PAGE_SIZE, "%s\n", >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index fa12a2f..f020957 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -49,6 +49,7 @@ struct drm_clip_rect; >> #define DRM_MODE_OBJECT_FB 0xfbfbfbfb >> #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb >> #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee >> +#define DRM_MODE_OBJECT_BRIDGE 0xbdbdbdbd >> >> struct drm_mode_object { >> uint32_t id; >> @@ -305,6 +306,7 @@ struct drm_connector; >> struct drm_encoder; >> struct drm_pending_vblank_event; >> struct drm_plane; >> +struct drm_bridge; >> >> /** >> * drm_crtc_funcs - control CRTCs for a given device >> @@ -507,6 +509,7 @@ struct drm_encoder_funcs { >> * @possible_crtcs: bitmask of potential CRTC bindings >> * @possible_clones: bitmask of potential sibling encoders for cloning >> * @crtc: currently bound CRTC >> + * @bridge: bridge associated to the encoder >> * @funcs: control functions >> * @helper_private: mid-layer private data >> * >> @@ -523,6 +526,7 @@ struct drm_encoder { >> uint32_t possible_clones; >> >> struct drm_crtc *crtc; >> + struct drm_bridge *bridge; >> const struct drm_encoder_funcs *funcs; >> void *helper_private; >> }; >> @@ -683,6 +687,41 @@ struct drm_plane { >> }; >> >> /** >> + * drm_bridge_funcs - drm_bridge control functions >> + * @detect: Checks if something is connected to the bridge. If this is >> + * implemented, it should take precedence over connector->detect() >> + * since the connector may not be able to deduce whether something is >> + * connected downstream of the bridge. >> + * @destroy: make object go away >> + */ >> +struct drm_bridge_funcs { >> + enum drm_connector_status (*detect)(struct drm_bridge *bridge, >> + bool force); >> + void (*destroy)(struct drm_bridge *bridge); >> +}; >> + >> +/** >> + * drm_bridge - central DRM bridge control structure >> + * @dev: DRM device this bridge belongs to >> + * @head: list management >> + * @base: base mode object >> + * @connector_type: the type of connector this bridge can associate with >> + * @funcs: control functions >> + * @helper_private: mid-layer private data >> + */ >> +struct drm_bridge { >> + struct drm_device *dev; >> + struct list_head head; >> + >> + struct drm_mode_object base; >> + >> + int connector_type; >> + >> + const struct drm_bridge_funcs *funcs; >> + void *helper_private; >> +}; >> + >> +/** >> * drm_mode_set - new values for a CRTC config change >> * @head: list management >> * @fb: framebuffer to use for new config >> @@ -742,6 +781,7 @@ struct drm_mode_group { >> uint32_t num_crtcs; >> uint32_t num_encoders; >> uint32_t num_connectors; >> + uint32_t num_bridges; >> >> /* list of object IDs for this group */ >> uint32_t *id_list; >> @@ -756,6 +796,8 @@ struct drm_mode_group { >> * @fb_list: list of framebuffers available >> * @num_connector: number of connectors on this device >> * @connector_list: list of connector objects >> + * @num_bridge: number of bridges on this device >> + * @bridge_list: list of bridge objects >> * @num_encoder: number of encoders on this device >> * @encoder_list: list of encoder objects >> * @num_crtc: number of CRTCs on this device >> @@ -793,6 +835,8 @@ struct drm_mode_config { >> >> int num_connector; >> struct list_head connector_list; >> + int num_bridge; >> + struct list_head bridge_list; >> int num_encoder; >> struct list_head encoder_list; >> int num_plane; >> @@ -878,6 +922,10 @@ extern void drm_connector_cleanup(struct drm_connector *connector); >> /* helper to unplug all connectors from sysfs for device */ >> extern void drm_connector_unplug_all(struct drm_device *dev); >> >> +extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> + const struct drm_bridge_funcs *funcs); >> +extern void drm_bridge_cleanup(struct drm_bridge *bridge); >> + >> extern int drm_encoder_init(struct drm_device *dev, >> struct drm_encoder *encoder, >> const struct drm_encoder_funcs *funcs, >> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h >> index f43d556..89a11f9 100644 >> --- a/include/drm/drm_crtc_helper.h >> +++ b/include/drm/drm_crtc_helper.h >> @@ -125,6 +125,34 @@ struct drm_connector_helper_funcs { >> struct drm_encoder *(*best_encoder)(struct drm_connector *connector); >> }; >> >> +/** >> + * drm_bridge_helper_funcs - helper operations for bridges (all required) >> + * @dpms: Control power levels on the bridge. If the mode passed in is >> + * unsupported, the provider must use the next lowest power level. >> + * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge >> + * @disable: Called right before encoder prepare, disables the bridge >> + * @post_disable: Called right after encoder prepare, for lockstepped disable >> + * @mode_set: Set this mode to the bridge >> + * @pre_enable: Called right before encoder commit, for lockstepped commit >> + * @enable: Called right after encoder commit, enables the bridge >> + * >> + * The helper operations are called by the mid-layer CRTC helper, however they >> + * can also be called directly by drivers that don't use the helper functions. >> + */ >> +struct drm_bridge_helper_funcs { >> + void (*dpms)(struct drm_bridge *bridge, int mode); >> + bool (*mode_fixup)(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode); >> + void (*disable)(struct drm_bridge *bridge); >> + void (*post_disable)(struct drm_bridge *bridge); >> + void (*mode_set)(struct drm_bridge *bridge, >> + struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode); >> + void (*pre_enable)(struct drm_bridge *bridge); >> + void (*enable)(struct drm_bridge *bridge); >> +}; >> + >> extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); >> extern void drm_helper_disable_unused_functions(struct drm_device *dev); >> extern int drm_crtc_helper_set_config(struct drm_mode_set *set); >> @@ -160,6 +188,12 @@ static inline void drm_connector_helper_add(struct drm_connector *connector, >> connector->helper_private = (void *)funcs; >> } >> >> +static inline void drm_bridge_helper_add(struct drm_bridge *bridge, >> + const struct drm_bridge_helper_funcs *funcs) >> +{ >> + bridge->helper_private = (void *)funcs; >> +} >> + >> extern int drm_helper_resume_force_mode(struct drm_device *dev); >> extern void drm_kms_helper_poll_init(struct drm_device *dev); >> extern void drm_kms_helper_poll_fini(struct drm_device *dev); >> -- >> 1.8.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@chromium.org> wrote: >> This patch adds the notion of a drm_bridge. A bridge is a chained >> device which hangs off an encoder. The drm driver using the bridge >> should provide the association between encoder and bridge. Once a >> bridge is associated with an encoder, it will participate in mode >> set, dpms, and optionally connection detection. >> >> Since a connector may not be able to determine the connection state >> downstream of the bridge, bridges may implement detect() which will take >> precedence over connector detect() if the bridge is already attached >> to an encoder and that encoder is already attached to the connector. >> In practical terms, this requires the drm driver to make these >> associations at init time, which is fine for SoC applications where >> this link is static. >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > A few comments below. I think overall any such infrastructure simply > needs to demonstrate viability on a few drivers, that makes it much > simpler to check whether the interfaces make sense. Thanks for your feedback, Daniel. As mentioned by Stephane and Rob, there are a few drivers already using this. In the ChromeOS tree, we have 2 simple DP->LVDS bridges which quite frankly aren't terribly interesting atm, and we'll be converting an HDMI->myDP bridge in a couple of weeks (time permitting, of course). > Generally I'd make > more sense for me if the bridge is just an implementation detail of a > given encoder and if the bridge would not be exposed to the crtc > helpers. After we discussed this on IRC, I converted our exynos driver to do this. You can check out the patch here (https://gerrit.chromium.org/gerrit/#/c/64680). It felt like a lot of boilerplate code that would be duplicated in every drm driver. Given the number of hooks we have, it's a pretty fine granularity such that there aren't *that* many creative ways to order things (famous last words). If a driver finds the need to differ from the helper order, they probably aren't going to be using the helper anyways. > With that abstraction chaining would be more natural imo and > we'd also have a much higher chance that bridge drivers work accross > different drm drivers: Atm you can run some encoder stuff in the crtc > callbacks and the other way round (and pretty much every driver for a > bit more complicated hw does that), and every driver can differ in > those tricks a bit. If we bake the bridge callbacks into the crtc > helpers I expect that for bridges with tricky ordering constraints > this will become a giant mess. So I'd prefer much if this would work > like drm i2c slave encoders. > In the three bridge chips I've dealt with, they all try their best to be transparent to the encoder. They all fail at this. However their quirks are usually not dependent on the encoder implementation, but rather the general order of operations (ie: when to assert hotplug, video signal on, etc.). Furthermore, drm_bridge will allow us to use bridges across drm drivers. For example, the HDMI->myDP driver I mentioned earlier is used on a number of platforms, not just exynos. It would be a waste to bind it to exynos when a more general implementation can be achieved. > Cheers, Daniel > >> --- >> drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++ >> drivers/gpu/drm/drm_crtc_helper.c | 112 ++++++++++++++++++++++++++++++++++---- >> drivers/gpu/drm/drm_sysfs.c | 8 ++- >> include/drm/drm_crtc.h | 48 ++++++++++++++++ >> include/drm/drm_crtc_helper.h | 34 ++++++++++++ >> 5 files changed, 241 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index fc83bb9..0311e2b 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev) >> } >> EXPORT_SYMBOL(drm_connector_unplug_all); >> >> +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> + const struct drm_bridge_funcs *funcs) >> +{ >> + int ret; >> + >> + drm_modeset_lock_all(dev); >> + >> + ret = drm_mode_object_get(dev, &bridge->base, DRM_MODE_OBJECT_BRIDGE); >> + if (ret) >> + goto out; >> + >> + bridge->dev = dev; >> + bridge->funcs = funcs; >> + >> + list_add_tail(&bridge->head, &dev->mode_config.bridge_list); >> + dev->mode_config.num_bridge++; >> + >> + out: >> + drm_modeset_unlock_all(dev); >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_bridge_init); >> + >> +void drm_bridge_cleanup(struct drm_bridge *bridge) >> +{ >> + struct drm_device *dev = bridge->dev; >> + >> + drm_modeset_lock_all(dev); >> + drm_mode_object_put(dev, &bridge->base); >> + list_del(&bridge->head); >> + dev->mode_config.num_bridge--; >> + drm_modeset_unlock_all(dev); >> +} >> +EXPORT_SYMBOL(drm_bridge_cleanup); >> + >> int drm_encoder_init(struct drm_device *dev, >> struct drm_encoder *encoder, >> const struct drm_encoder_funcs *funcs, >> @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr >> total_objects += dev->mode_config.num_crtc; >> total_objects += dev->mode_config.num_connector; >> total_objects += dev->mode_config.num_encoder; >> + total_objects += dev->mode_config.num_bridge; >> >> group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL); >> if (!group->id_list) >> @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr >> group->num_crtcs = 0; >> group->num_connectors = 0; >> group->num_encoders = 0; >> + group->num_bridges = 0; >> return 0; >> } >> >> @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, >> struct drm_crtc *crtc; >> struct drm_encoder *encoder; >> struct drm_connector *connector; >> + struct drm_bridge *bridge; >> int ret; >> >> if ((ret = drm_mode_group_init(dev, group))) >> @@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, >> group->id_list[group->num_crtcs + group->num_encoders + >> group->num_connectors++] = connector->base.id; >> >> + list_for_each_entry(bridge, &dev->mode_config.bridge_list, head) >> + group->id_list[group->num_crtcs + group->num_encoders + >> + group->num_connectors + group->num_bridges++] = >> + bridge->base.id; >> + >> return 0; >> } >> EXPORT_SYMBOL(drm_mode_group_init_legacy_group); >> @@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev) >> INIT_LIST_HEAD(&dev->mode_config.fb_list); >> INIT_LIST_HEAD(&dev->mode_config.crtc_list); >> INIT_LIST_HEAD(&dev->mode_config.connector_list); >> + INIT_LIST_HEAD(&dev->mode_config.bridge_list); >> INIT_LIST_HEAD(&dev->mode_config.encoder_list); >> INIT_LIST_HEAD(&dev->mode_config.property_list); >> INIT_LIST_HEAD(&dev->mode_config.property_blob_list); >> @@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> struct drm_connector *connector, *ot; >> struct drm_crtc *crtc, *ct; >> struct drm_encoder *encoder, *enct; >> + struct drm_bridge *bridge, *brt; >> struct drm_framebuffer *fb, *fbt; >> struct drm_property *property, *pt; >> struct drm_property_blob *blob, *bt; >> @@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> encoder->funcs->destroy(encoder); >> } >> >> + list_for_each_entry_safe(bridge, brt, >> + &dev->mode_config.bridge_list, head) { >> + bridge->funcs->destroy(bridge); >> + } >> + >> list_for_each_entry_safe(connector, ot, >> &dev->mode_config.connector_list, head) { >> connector->funcs->destroy(connector); >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c >> index 6a64749..30139fe 100644 >> --- a/drivers/gpu/drm/drm_crtc_helper.c >> +++ b/drivers/gpu/drm/drm_crtc_helper.c >> @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); >> static bool drm_kms_helper_poll = true; >> module_param_named(poll, drm_kms_helper_poll, bool, 0600); >> >> +static enum drm_connector_status detect_connection( >> + struct drm_connector *connector, bool force) >> +{ >> + struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > > That won't work since the connector->encoder link is only set up at > runtime when a mode is set. Furthermore it's possible for connectors > to use different encoders. > > So if a connector can't figure out whether anything is connected > itself but needs to poke the bridge that needs to be forward in a > different fashion. One way would be to allow embedding both the > drm_bridge an the drm encoder into an over structure and the let a > bridge specific callback do the casting. > Yep, you and I discussed this on IRC the other day. You said this was fine b/c it would be used with SoCs and they would have a static mapping. I originally had the bridge hanging off the connector, which would allow this type of re-routing regardless of whether a mode set had occurred. >> + >> + /* >> + * If there's a bridge attached to the encoder (attached to >> + * the connector) and it can detect an active connection, >> + * re-route the detect call to the bridge. >> + */ >> + if (bridge && bridge->funcs->detect) >> + return bridge->funcs->detect(bridge, force); >> + >> + return connector->funcs->detect(connector, force); >> +} >> + >> static void drm_mode_validate_flag(struct drm_connector *connector, >> int flags) >> { >> @@ -137,7 +154,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, >> if (connector->funcs->force) >> connector->funcs->force(connector); >> } else { >> - connector->status = connector->funcs->detect(connector, true); >> + connector->status = detect_connection(connector, true); >> } >> >> /* Re-enable polling in case the global poll config changed. */ >> @@ -256,11 +273,22 @@ static void >> drm_encoder_disable(struct drm_encoder *encoder) >> { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->disable(encoder->bridge); >> + } >> >> if (encoder_funcs->disable) >> (*encoder_funcs->disable)(encoder); >> else >> (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->post_disable(encoder->bridge); >> + } >> } >> >> /** >> @@ -392,6 +420,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; >> struct drm_encoder_helper_funcs *encoder_funcs; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> int saved_x, saved_y; >> struct drm_encoder *encoder; >> bool ret = true; >> @@ -424,6 +453,17 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> >> if (encoder->crtc != crtc) >> continue; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + ret = bridge_funcs->mode_fixup(encoder->bridge, mode, >> + adjusted_mode); >> + if (!ret) { >> + DRM_DEBUG_KMS("Bridge fixup failed\n"); >> + goto done; >> + } >> + } > > I strongly believe that this isn't good enough for many cases and that > often the bridge and encoder want to more closely discuss how to set > up the link between them (e.g. all the nonsense dsi panels might > want). > At least in the cases I've seen, that's not the case. I don't want to overcomplicate things without a concrete justification. So far everything else in this patch will be used by real hardware. That said, it'd be pretty easy to tweak this if needed. >> + >> encoder_funcs = encoder->helper_private; >> if (!(ret = encoder_funcs->mode_fixup(encoder, mode, >> adjusted_mode))) { >> @@ -443,9 +483,20 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> >> if (encoder->crtc != crtc) >> continue; >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->disable(encoder->bridge); >> + } >> + >> encoder_funcs = encoder->helper_private; >> /* Disable the encoders as the first thing we do. */ >> encoder_funcs->prepare(encoder); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->post_disable(encoder->bridge); >> + } >> } >> >> drm_crtc_prepare_encoders(dev); >> @@ -469,6 +520,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> mode->base.id, mode->name); >> encoder_funcs = encoder->helper_private; >> encoder_funcs->mode_set(encoder, mode, adjusted_mode); >> + >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->mode_set(encoder->bridge, mode, >> + adjusted_mode); >> + } >> } >> >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >> @@ -479,9 +536,18 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, >> if (encoder->crtc != crtc) >> continue; >> >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->pre_enable(encoder->bridge); >> + } >> + >> encoder_funcs = encoder->helper_private; >> encoder_funcs->commit(encoder); >> >> + if (encoder->bridge) { >> + bridge_funcs = encoder->bridge->helper_private; >> + bridge_funcs->enable(encoder->bridge); >> + } >> } >> >> /* Store real post-adjustment hardware mode. */ >> @@ -856,8 +922,9 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) >> void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> { >> struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; >> struct drm_crtc *crtc = encoder ? encoder->crtc : NULL; >> - int old_dpms; >> + int old_dpms, encoder_dpms = DRM_MODE_DPMS_OFF; >> >> if (mode == connector->dpms) >> return; >> @@ -865,6 +932,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> old_dpms = connector->dpms; >> connector->dpms = mode; >> >> + if (encoder) >> + encoder_dpms = drm_helper_choose_encoder_dpms(encoder); >> + >> /* from off to on, do crtc then encoder */ >> if (mode < old_dpms) { >> if (crtc) { >> @@ -876,18 +946,28 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) >> if (encoder) { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> - (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); >> + } >> + if (bridge) { >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, encoder_dpms); > > Here we only have one dpms callback, no post_disable/pre_enable. > Presuming you have a bridge that really needs the former this will > fall over when doing dpms transitions. > So should we add pre_enable/enable/disable/post_disable around encoder dpms and then call bridge dpms? Or maybe we should just do away with dpms callback entirely and only use enable/disable? Another option would be pre_dpms/post_dpms. >> } >> } >> >> /* from on to off, do encoder then crtc */ >> if (mode > old_dpms) { >> + if (bridge) { >> + struct drm_bridge_helper_funcs *bridge_funcs; >> + >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, encoder_dpms); >> + } >> if (encoder) { >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> - (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); >> } >> if (crtc) { >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; >> @@ -924,9 +1004,11 @@ int drm_helper_resume_force_mode(struct drm_device *dev) >> { >> struct drm_crtc *crtc; >> struct drm_encoder *encoder; >> + struct drm_bridge *bridge; >> + struct drm_bridge_helper_funcs *bridge_funcs; >> struct drm_encoder_helper_funcs *encoder_funcs; >> struct drm_crtc_helper_funcs *crtc_funcs; >> - int ret; >> + int ret, encoder_dpms; >> >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { >> >> @@ -946,10 +1028,20 @@ int drm_helper_resume_force_mode(struct drm_device *dev) >> if(encoder->crtc != crtc) >> continue; >> >> + encoder_dpms = drm_helper_choose_encoder_dpms( >> + encoder); >> + >> + bridge = encoder->bridge; >> + if (bridge) { >> + bridge_funcs = bridge->helper_private; >> + bridge_funcs->dpms(bridge, >> + encoder_dpms); >> + } >> + >> encoder_funcs = encoder->helper_private; >> if (encoder_funcs->dpms) >> (*encoder_funcs->dpms) (encoder, >> - drm_helper_choose_encoder_dpms(encoder)); >> + encoder_dpms); >> } >> >> crtc_funcs = crtc->helper_private; >> @@ -1006,7 +1098,7 @@ static void output_poll_execute(struct work_struct *work) >> !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) >> continue; >> >> - connector->status = connector->funcs->detect(connector, false); >> + connector->status = detect_connection(connector, false); >> if (old_status != connector->status) { >> const char *old, *new; >> >> @@ -1092,7 +1184,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) >> >> old_status = connector->status; >> >> - connector->status = connector->funcs->detect(connector, false); >> + connector->status = detect_connection(connector, false); >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", >> connector->base.id, >> drm_get_connector_name(connector), >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c >> index 2290b3b..a912d90 100644 >> --- a/drivers/gpu/drm/drm_sysfs.c >> +++ b/drivers/gpu/drm/drm_sysfs.c >> @@ -184,6 +184,8 @@ static ssize_t status_show(struct device *device, >> char *buf) >> { >> struct drm_connector *connector = to_drm_connector(device); >> + struct drm_encoder *encoder = connector->encoder; >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; >> enum drm_connector_status status; >> int ret; >> >> @@ -191,7 +193,11 @@ static ssize_t status_show(struct device *device, >> if (ret) >> return ret; >> >> - status = connector->funcs->detect(connector, true); >> + if (bridge && bridge->funcs->detect) >> + status = bridge->funcs->detect(bridge, true); >> + else >> + status = connector->funcs->detect(connector, true); >> + >> mutex_unlock(&connector->dev->mode_config.mutex); >> >> return snprintf(buf, PAGE_SIZE, "%s\n", >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index fa12a2f..f020957 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -49,6 +49,7 @@ struct drm_clip_rect; >> #define DRM_MODE_OBJECT_FB 0xfbfbfbfb >> #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb >> #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee >> +#define DRM_MODE_OBJECT_BRIDGE 0xbdbdbdbd >> >> struct drm_mode_object { >> uint32_t id; >> @@ -305,6 +306,7 @@ struct drm_connector; >> struct drm_encoder; >> struct drm_pending_vblank_event; >> struct drm_plane; >> +struct drm_bridge; >> >> /** >> * drm_crtc_funcs - control CRTCs for a given device >> @@ -507,6 +509,7 @@ struct drm_encoder_funcs { >> * @possible_crtcs: bitmask of potential CRTC bindings >> * @possible_clones: bitmask of potential sibling encoders for cloning >> * @crtc: currently bound CRTC >> + * @bridge: bridge associated to the encoder >> * @funcs: control functions >> * @helper_private: mid-layer private data >> * >> @@ -523,6 +526,7 @@ struct drm_encoder { >> uint32_t possible_clones; >> >> struct drm_crtc *crtc; >> + struct drm_bridge *bridge; >> const struct drm_encoder_funcs *funcs; >> void *helper_private; >> }; >> @@ -683,6 +687,41 @@ struct drm_plane { >> }; >> >> /** >> + * drm_bridge_funcs - drm_bridge control functions >> + * @detect: Checks if something is connected to the bridge. If this is >> + * implemented, it should take precedence over connector->detect() >> + * since the connector may not be able to deduce whether something is >> + * connected downstream of the bridge. >> + * @destroy: make object go away >> + */ >> +struct drm_bridge_funcs { >> + enum drm_connector_status (*detect)(struct drm_bridge *bridge, >> + bool force); >> + void (*destroy)(struct drm_bridge *bridge); >> +}; >> + >> +/** >> + * drm_bridge - central DRM bridge control structure >> + * @dev: DRM device this bridge belongs to >> + * @head: list management >> + * @base: base mode object >> + * @connector_type: the type of connector this bridge can associate with >> + * @funcs: control functions >> + * @helper_private: mid-layer private data >> + */ >> +struct drm_bridge { >> + struct drm_device *dev; >> + struct list_head head; >> + >> + struct drm_mode_object base; >> + >> + int connector_type; >> + >> + const struct drm_bridge_funcs *funcs; >> + void *helper_private; >> +}; >> + >> +/** >> * drm_mode_set - new values for a CRTC config change >> * @head: list management >> * @fb: framebuffer to use for new config >> @@ -742,6 +781,7 @@ struct drm_mode_group { >> uint32_t num_crtcs; >> uint32_t num_encoders; >> uint32_t num_connectors; >> + uint32_t num_bridges; >> >> /* list of object IDs for this group */ >> uint32_t *id_list; >> @@ -756,6 +796,8 @@ struct drm_mode_group { >> * @fb_list: list of framebuffers available >> * @num_connector: number of connectors on this device >> * @connector_list: list of connector objects >> + * @num_bridge: number of bridges on this device >> + * @bridge_list: list of bridge objects >> * @num_encoder: number of encoders on this device >> * @encoder_list: list of encoder objects >> * @num_crtc: number of CRTCs on this device >> @@ -793,6 +835,8 @@ struct drm_mode_config { >> >> int num_connector; >> struct list_head connector_list; >> + int num_bridge; >> + struct list_head bridge_list; >> int num_encoder; >> struct list_head encoder_list; >> int num_plane; >> @@ -878,6 +922,10 @@ extern void drm_connector_cleanup(struct drm_connector *connector); >> /* helper to unplug all connectors from sysfs for device */ >> extern void drm_connector_unplug_all(struct drm_device *dev); >> >> +extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, >> + const struct drm_bridge_funcs *funcs); >> +extern void drm_bridge_cleanup(struct drm_bridge *bridge); >> + >> extern int drm_encoder_init(struct drm_device *dev, >> struct drm_encoder *encoder, >> const struct drm_encoder_funcs *funcs, >> diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h >> index f43d556..89a11f9 100644 >> --- a/include/drm/drm_crtc_helper.h >> +++ b/include/drm/drm_crtc_helper.h >> @@ -125,6 +125,34 @@ struct drm_connector_helper_funcs { >> struct drm_encoder *(*best_encoder)(struct drm_connector *connector); >> }; >> >> +/** >> + * drm_bridge_helper_funcs - helper operations for bridges (all required) >> + * @dpms: Control power levels on the bridge. If the mode passed in is >> + * unsupported, the provider must use the next lowest power level. >> + * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge >> + * @disable: Called right before encoder prepare, disables the bridge >> + * @post_disable: Called right after encoder prepare, for lockstepped disable >> + * @mode_set: Set this mode to the bridge >> + * @pre_enable: Called right before encoder commit, for lockstepped commit >> + * @enable: Called right after encoder commit, enables the bridge >> + * >> + * The helper operations are called by the mid-layer CRTC helper, however they >> + * can also be called directly by drivers that don't use the helper functions. >> + */ >> +struct drm_bridge_helper_funcs { >> + void (*dpms)(struct drm_bridge *bridge, int mode); >> + bool (*mode_fixup)(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode); >> + void (*disable)(struct drm_bridge *bridge); >> + void (*post_disable)(struct drm_bridge *bridge); >> + void (*mode_set)(struct drm_bridge *bridge, >> + struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode); >> + void (*pre_enable)(struct drm_bridge *bridge); >> + void (*enable)(struct drm_bridge *bridge); >> +}; >> + >> extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); >> extern void drm_helper_disable_unused_functions(struct drm_device *dev); >> extern int drm_crtc_helper_set_config(struct drm_mode_set *set); >> @@ -160,6 +188,12 @@ static inline void drm_connector_helper_add(struct drm_connector *connector, >> connector->helper_private = (void *)funcs; >> } >> >> +static inline void drm_bridge_helper_add(struct drm_bridge *bridge, >> + const struct drm_bridge_helper_funcs *funcs) >> +{ >> + bridge->helper_private = (void *)funcs; >> +} >> + >> extern int drm_helper_resume_force_mode(struct drm_device *dev); >> extern void drm_kms_helper_poll_init(struct drm_device *dev); >> extern void drm_kms_helper_poll_fini(struct drm_device *dev); >> -- >> 1.8.3 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Aug 9, 2013 at 9:19 AM, Sean Paul <seanpaul@chromium.org> wrote: > On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@chromium.org> wrote: >>> This patch adds the notion of a drm_bridge. A bridge is a chained >>> device which hangs off an encoder. The drm driver using the bridge >>> should provide the association between encoder and bridge. Once a >>> bridge is associated with an encoder, it will participate in mode >>> set, dpms, and optionally connection detection. >>> >>> Since a connector may not be able to determine the connection state >>> downstream of the bridge, bridges may implement detect() which will take >>> precedence over connector detect() if the bridge is already attached >>> to an encoder and that encoder is already attached to the connector. >>> In practical terms, this requires the drm driver to make these >>> associations at init time, which is fine for SoC applications where >>> this link is static. >>> >>> Signed-off-by: Sean Paul <seanpaul@chromium.org> >> >> A few comments below. I think overall any such infrastructure simply >> needs to demonstrate viability on a few drivers, that makes it much >> simpler to check whether the interfaces make sense. > > Thanks for your feedback, Daniel. > > As mentioned by Stephane and Rob, there are a few drivers already > using this. In the ChromeOS tree, we have 2 simple DP->LVDS bridges > which quite frankly aren't terribly interesting atm, and we'll be > converting an HDMI->myDP bridge in a couple of weeks (time permitting, > of course). radeon also uses DP->LVDS and DP->VGA bridges on a number of systems. It currently uses a driver specific hack, but it could probably be converted to use the bridge infrastructure. Alex
On Fri, Aug 09, 2013 at 09:19:22AM -0400, Sean Paul wrote: > On Thu, Aug 8, 2013 at 8:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@chromium.org> wrote: > >> This patch adds the notion of a drm_bridge. A bridge is a chained > >> device which hangs off an encoder. The drm driver using the bridge > >> should provide the association between encoder and bridge. Once a > >> bridge is associated with an encoder, it will participate in mode > >> set, dpms, and optionally connection detection. > >> > >> Since a connector may not be able to determine the connection state > >> downstream of the bridge, bridges may implement detect() which will take > >> precedence over connector detect() if the bridge is already attached > >> to an encoder and that encoder is already attached to the connector. > >> In practical terms, this requires the drm driver to make these > >> associations at init time, which is fine for SoC applications where > >> this link is static. > >> > >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > > > A few comments below. I think overall any such infrastructure simply > > needs to demonstrate viability on a few drivers, that makes it much > > simpler to check whether the interfaces make sense. > > Thanks for your feedback, Daniel. > > As mentioned by Stephane and Rob, there are a few drivers already > using this. In the ChromeOS tree, we have 2 simple DP->LVDS bridges > which quite frankly aren't terribly interesting atm, and we'll be > converting an HDMI->myDP bridge in a couple of weeks (time permitting, > of course). > > > > Generally I'd make > > more sense for me if the bridge is just an implementation detail of a > > given encoder and if the bridge would not be exposed to the crtc > > helpers. > > After we discussed this on IRC, I converted our exynos driver to do > this. You can check out the patch here > (https://gerrit.chromium.org/gerrit/#/c/64680). Hm, looking at this (I didn't read the full thing) it smells a bit like exynos registers the connector first eg as hdmi. And then it sets up the hdmi->whatever bridge later on. Imo (and presuming my 5 minutes strolling around guess is right) that's the wrong approach since the connector is the userspace interface object and should represent the actual output port. So if you have a pile of conversion bridges that's irrelevant and the user doesn't really care that the first step is a hmdi connetion if it eventually ends up on a lvds panel (or something like that). Instead the connector should be registered by the last bridge, with the correct type. > It felt like a lot of boilerplate code that would be duplicated in > every drm driver. Given the number of hooks we have, it's a pretty > fine granularity such that there aren't *that* many creative ways to > order things (famous last words). If a driver finds the need to differ > from the helper order, they probably aren't going to be using the > helper anyways. I think you haven't seen some of the fun stuff we've done in i915 before we've given up and scrapped it all ;-) But the current interface look sensible enough that we can fix things by moving it back into drivers again (and just leaving the the generic encoder->bridge pointer NULL). > > With that abstraction chaining would be more natural imo and > > we'd also have a much higher chance that bridge drivers work accross > > different drm drivers: Atm you can run some encoder stuff in the crtc > > callbacks and the other way round (and pretty much every driver for a > > bit more complicated hw does that), and every driver can differ in > > those tricks a bit. If we bake the bridge callbacks into the crtc > > helpers I expect that for bridges with tricky ordering constraints > > this will become a giant mess. So I'd prefer much if this would work > > like drm i2c slave encoders. > > > > In the three bridge chips I've dealt with, they all try their best to > be transparent to the encoder. They all fail at this. However their > quirks are usually not dependent on the encoder implementation, but > rather the general order of operations (ie: when to assert hotplug, > video signal on, etc.). > > Furthermore, drm_bridge will allow us to use bridges across drm > drivers. For example, the HDMI->myDP driver I mentioned earlier is > used on a number of platforms, not just exynos. It would be a waste to > bind it to exynos when a more general implementation can be achieved. I guess it's best to just look a bit at actual implementations, without them my comments are likely rather useless. > >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > >> index 6a64749..30139fe 100644 > >> --- a/drivers/gpu/drm/drm_crtc_helper.c > >> +++ b/drivers/gpu/drm/drm_crtc_helper.c > >> @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); > >> static bool drm_kms_helper_poll = true; > >> module_param_named(poll, drm_kms_helper_poll, bool, 0600); > >> > >> +static enum drm_connector_status detect_connection( > >> + struct drm_connector *connector, bool force) > >> +{ > >> + struct drm_encoder *encoder = connector->encoder; > >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > > > > That won't work since the connector->encoder link is only set up at > > runtime when a mode is set. Furthermore it's possible for connectors > > to use different encoders. > > > > So if a connector can't figure out whether anything is connected > > itself but needs to poke the bridge that needs to be forward in a > > different fashion. One way would be to allow embedding both the > > drm_bridge an the drm encoder into an over structure and the let a > > bridge specific callback do the casting. > > > > Yep, you and I discussed this on IRC the other day. You said this was > fine b/c it would be used with SoCs and they would have a static > mapping. I originally had the bridge hanging off the connector, which > would allow this type of re-routing regardless of whether a mode set > had occurred. Yeah, I've just had a slightly different idea in mind like struct my_strange_bridge { struct drm_bridge base; struct drm_connector connector; struct drm_encoder *encoder; /* connector this bridge is hanging off */ }; And then we could let the bridge driver set up everything (and the bridge driver could then also upcast). So the bridge driver would both set up the bridge and the connector. Then the bridge driver could freely upcast the connector pointer in the ->detect callback to whatever it actually needs. > >> @@ -392,6 +420,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > >> struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; > >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > >> struct drm_encoder_helper_funcs *encoder_funcs; > >> + struct drm_bridge_helper_funcs *bridge_funcs; > >> int saved_x, saved_y; > >> struct drm_encoder *encoder; > >> bool ret = true; > >> @@ -424,6 +453,17 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > >> > >> if (encoder->crtc != crtc) > >> continue; > >> + > >> + if (encoder->bridge) { > >> + bridge_funcs = encoder->bridge->helper_private; > >> + ret = bridge_funcs->mode_fixup(encoder->bridge, mode, > >> + adjusted_mode); > >> + if (!ret) { > >> + DRM_DEBUG_KMS("Bridge fixup failed\n"); > >> + goto done; > >> + } > >> + } > > > > I strongly believe that this isn't good enough for many cases and that > > often the bridge and encoder want to more closely discuss how to set > > up the link between them (e.g. all the nonsense dsi panels might > > want). > > > > At least in the cases I've seen, that's not the case. I don't want to > overcomplicate things without a concrete justification. So far > everything else in this patch will be used by real hardware. That > said, it'd be pretty easy to tweak this if needed. Yeah, I guess we can always create a drm_bridge_config struct which contains the requested and adjusted mode and anything else the bridge and connector need to agree on. But since that's an easy refactoring we can postpone until there's a need. > >> @@ -865,6 +932,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) > >> old_dpms = connector->dpms; > >> connector->dpms = mode; > >> > >> + if (encoder) > >> + encoder_dpms = drm_helper_choose_encoder_dpms(encoder); > >> + > >> /* from off to on, do crtc then encoder */ > >> if (mode < old_dpms) { > >> if (crtc) { > >> @@ -876,18 +946,28 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) > >> if (encoder) { > >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; > >> if (encoder_funcs->dpms) > >> - (*encoder_funcs->dpms) (encoder, > >> - drm_helper_choose_encoder_dpms(encoder)); > >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); > >> + } > >> + if (bridge) { > >> + struct drm_bridge_helper_funcs *bridge_funcs; > >> + > >> + bridge_funcs = bridge->helper_private; > >> + bridge_funcs->dpms(bridge, encoder_dpms); > > > > Here we only have one dpms callback, no post_disable/pre_enable. > > Presuming you have a bridge that really needs the former this will > > fall over when doing dpms transitions. > > > > So should we add pre_enable/enable/disable/post_disable around encoder > dpms and then call bridge dpms? Or maybe we should just do away with > dpms callback entirely and only use enable/disable? Another option > would be pre_dpms/post_dpms. At least on modern intel platforms there's no support at all any more for intermediate dpms states. And I think we're not the only ones ;-) Personally I'd vote to ditch the special ->dpms callback and implement the dpms changes with the enable/disable hooks around the encoder->dpms callback. That requires though that we clamp any userspace dpms request != DPMS_ON to DPMS_OFF. One more I've noticed below. > > > >> } > >> } > >> > >> /* from on to off, do encoder then crtc */ > >> if (mode > old_dpms) { > >> + if (bridge) { > >> + struct drm_bridge_helper_funcs *bridge_funcs; > >> + > >> + bridge_funcs = bridge->helper_private; > >> + bridge_funcs->dpms(bridge, encoder_dpms); > >> + } > >> if (encoder) { > >> struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; > >> if (encoder_funcs->dpms) > >> - (*encoder_funcs->dpms) (encoder, > >> - drm_helper_choose_encoder_dpms(encoder)); > >> + (*encoder_funcs->dpms) (encoder, encoder_dpms); > >> } > >> if (crtc) { > >> struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; > >> @@ -924,9 +1004,11 @@ int drm_helper_resume_force_mode(struct drm_device *dev) > >> { > >> struct drm_crtc *crtc; > >> struct drm_encoder *encoder; > >> + struct drm_bridge *bridge; > >> + struct drm_bridge_helper_funcs *bridge_funcs; > >> struct drm_encoder_helper_funcs *encoder_funcs; > >> struct drm_crtc_helper_funcs *crtc_funcs; > >> - int ret; > >> + int ret, encoder_dpms; > >> > >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { > >> > >> @@ -946,10 +1028,20 @@ int drm_helper_resume_force_mode(struct drm_device *dev) > >> if(encoder->crtc != crtc) > >> continue; > >> > >> + encoder_dpms = drm_helper_choose_encoder_dpms( > >> + encoder); > >> + > >> + bridge = encoder->bridge; > >> + if (bridge) { > >> + bridge_funcs = bridge->helper_private; > >> + bridge_funcs->dpms(bridge, > >> + encoder_dpms); > >> + } > >> + > >> encoder_funcs = encoder->helper_private; > >> if (encoder_funcs->dpms) > >> (*encoder_funcs->dpms) (encoder, > >> - drm_helper_choose_encoder_dpms(encoder)); > >> + encoder_dpms); > >> } > >> > >> crtc_funcs = crtc->helper_private; > >> @@ -1006,7 +1098,7 @@ static void output_poll_execute(struct work_struct *work) > >> !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) > >> continue; > >> > >> - connector->status = connector->funcs->detect(connector, false); > >> + connector->status = detect_connection(connector, false); > >> if (old_status != connector->status) { > >> const char *old, *new; > >> > >> @@ -1092,7 +1184,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) > >> > >> old_status = connector->status; > >> > >> - connector->status = connector->funcs->detect(connector, false); > >> + connector->status = detect_connection(connector, false); > >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", > >> connector->base.id, > >> drm_get_connector_name(connector), > >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c > >> index 2290b3b..a912d90 100644 > >> --- a/drivers/gpu/drm/drm_sysfs.c > >> +++ b/drivers/gpu/drm/drm_sysfs.c > >> @@ -184,6 +184,8 @@ static ssize_t status_show(struct device *device, > >> char *buf) > >> { > >> struct drm_connector *connector = to_drm_connector(device); > >> + struct drm_encoder *encoder = connector->encoder; > >> + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; > >> enum drm_connector_status status; > >> int ret; > >> > >> @@ -191,7 +193,11 @@ static ssize_t status_show(struct device *device, > >> if (ret) > >> return ret; > >> > >> - status = connector->funcs->detect(connector, true); > >> + if (bridge && bridge->funcs->detect) > >> + status = bridge->funcs->detect(bridge, true); > >> + else > >> + status = connector->funcs->detect(connector, true); > >> + > >> mutex_unlock(&connector->dev->mode_config.mutex); > >> > >> return snprintf(buf, PAGE_SIZE, "%s\n", > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > >> index fa12a2f..f020957 100644 > >> --- a/include/drm/drm_crtc.h > >> +++ b/include/drm/drm_crtc.h > >> @@ -49,6 +49,7 @@ struct drm_clip_rect; > >> #define DRM_MODE_OBJECT_FB 0xfbfbfbfb > >> #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb > >> #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee > >> +#define DRM_MODE_OBJECT_BRIDGE 0xbdbdbdbd > >> > >> struct drm_mode_object { > >> uint32_t id; > >> @@ -305,6 +306,7 @@ struct drm_connector; > >> struct drm_encoder; > >> struct drm_pending_vblank_event; > >> struct drm_plane; > >> +struct drm_bridge; > >> > >> /** > >> * drm_crtc_funcs - control CRTCs for a given device > >> @@ -507,6 +509,7 @@ struct drm_encoder_funcs { > >> * @possible_crtcs: bitmask of potential CRTC bindings > >> * @possible_clones: bitmask of potential sibling encoders for cloning > >> * @crtc: currently bound CRTC > >> + * @bridge: bridge associated to the encoder > >> * @funcs: control functions > >> * @helper_private: mid-layer private data > >> * > >> @@ -523,6 +526,7 @@ struct drm_encoder { > >> uint32_t possible_clones; > >> > >> struct drm_crtc *crtc; > >> + struct drm_bridge *bridge; > >> const struct drm_encoder_funcs *funcs; > >> void *helper_private; > >> }; > >> @@ -683,6 +687,41 @@ struct drm_plane { > >> }; > >> > >> /** > >> + * drm_bridge_funcs - drm_bridge control functions > >> + * @detect: Checks if something is connected to the bridge. If this is > >> + * implemented, it should take precedence over connector->detect() > >> + * since the connector may not be able to deduce whether something is > >> + * connected downstream of the bridge. > >> + * @destroy: make object go away > >> + */ > >> +struct drm_bridge_funcs { > >> + enum drm_connector_status (*detect)(struct drm_bridge *bridge, > >> + bool force); > >> + void (*destroy)(struct drm_bridge *bridge); > >> +}; What's the reasoning behind the split here (and hiding part of the helper interface behind the helper_private pointer)? Note that for connector/encoder/crtc this split exists since the direct vtable is to implement the kms userspace interface (e.g. what drm/i915 still implements) whereas all the _helper_funcs vtables are only used by the helper code in drm_crtc_helper.c. The idea is that drivers could use this pointer for their own modeset infrastructuer (we don't do that in i915 since void *isn't especially typesafe and wasting a pointer doesn't matter). But for the bridge stuff which is all opt-in internal interfaces there's imo not justification to hide half of the vfuncs somewhere. Cheers, Daniel
Hi all, Ok I've tossed around another idea with Sean on irc, but it turned out to be too complicated for irc. So here it's in more detail. So the goal is to share bridge drivers/transcoders/whatever between different drivers without going all-in with something like cdf but allowing easy transitions. Which means all the different drm drivers should be allowed to have their own special-sauce interfaces on top of whatever drm_bridge currently provides, and encoder/bdrige drivers should still be able to share as much as possible. So for an example let's look at the sil164 chip for which we have one driver in drm/i2c/sil164_drv.c which implements the drm i2c slave encoder interface and 2 second driver in drm/i915/dvo_sil164.c which implements the i915 dvo interface. So we have two interface structures: - struct drm_encoder_slave which subclasses currently a drm_encoder. - struct intel_dvo_device which doesn't subclass anything The first mismatch is that the encoder slaves are real drm encoders, so we'd need to add a drm_bridge first somewhere. But I guess that can be hacked up (and it's kinda irrelevant for new drivers like msm). struct intel_dvo_device is conceptually like a drm_bridge, the only change would be to move most of the interface vfuncs from intel_dvo_dev_ops to the drm_bdrige. Now furthermore we have a bunch of bridge driver specific stuff like the i2c adapter, saved register state and other similar stuff. For the i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo case it's splattered over intel_dvo_device and a (very tiny) struct sil164_priv. But we could unify that easily by moving e.g. the i2c_adapter from the dvo interface to the driver private. So the first important thing to note is that we won't be able to unify these different things completely because it'd be a big pile of refactoring work (so we need to be able to live with in-between states anyway). And there's always special fun like the ->get_hw_state callback i915 drivers have. So I think we need to allow such flexibility, even when ignoring how to get there. My idea to be able to pull shared drivers of in this scenario is to add a drm_bridge->driver_private void* pointer. Then we could embedde a drm_bridge into both both drm_encoder_slave and into intel_dvo_device. The unified sil164 driver would then have two init functions: One sets up a drm encoder slave, the other a dvo i915 device. Both get required setup data in their own special means (and we could easily add a new dt based setup functon, too). But thanks to the ->driver_private pointer shared code between these two interfaces doesn't need to care one bit whether it implements the dvo or the encoder slave interface. It just follows the ->driver_private pointer to get at all the relevant data it needs. Otoh this still allows special sauce functions like get_hw_state to accept the more specific interface struct and do fancy things on top. Of course that specialized stuff can't be shared, but all the common parts can. Not having a ->driver_private pointer would require that drm_bridge drivers would need to embed the drm_bridge into their own data structures. Which means that they can't support two different drm_bridge interface extensions and so would (at least partially) defeat the code sharing goals. Note that if we use drm_bridge as the baseline for implementing dsi slaves (I think this would make sense) such issues with drm_bridge interface extensions will be even more fun. Cheers, Daniel
On Tue, Aug 13, 2013 at 3:16 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > Hi all, > > Ok I've tossed around another idea with Sean on irc, but it turned out > to be too complicated for irc. So here it's in more detail. So the > goal is to share bridge drivers/transcoders/whatever between different > drivers without going all-in with something like cdf but allowing easy > transitions. Which means all the different drm drivers should be > allowed to have their own special-sauce interfaces on top of whatever > drm_bridge currently provides, and encoder/bdrige drivers should still > be able to share as much as possible. > [For the benefit of those not on irc atm] This sounds good to me. I also agree with everything in your previous email, with the exception of those pesky helper functions and my stubborn insistence to keep them in as long as possible. I'm working on a v2 patch set that will include one of our DP->LVDS bridge drivers as a proof-of-concept. Sean > So for an example let's look at the sil164 chip for which we have one > driver in drm/i2c/sil164_drv.c which implements the drm i2c slave > encoder interface and 2 second driver in drm/i915/dvo_sil164.c which > implements the i915 dvo interface. So we have two interface > structures: > - struct drm_encoder_slave which subclasses currently a drm_encoder. > - struct intel_dvo_device which doesn't subclass anything > > The first mismatch is that the encoder slaves are real drm encoders, > so we'd need to add a drm_bridge first somewhere. But I guess that can > be hacked up (and it's kinda irrelevant for new drivers like msm). > struct intel_dvo_device is conceptually like a drm_bridge, the only > change would be to move most of the interface vfuncs from > intel_dvo_dev_ops to the drm_bdrige. > > Now furthermore we have a bunch of bridge driver specific stuff like > the i2c adapter, saved register state and other similar stuff. For the > i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo > case it's splattered over intel_dvo_device and a (very tiny) struct > sil164_priv. But we could unify that easily by moving e.g. the > i2c_adapter from the dvo interface to the driver private. > > So the first important thing to note is that we won't be able to unify > these different things completely because it'd be a big pile of > refactoring work (so we need to be able to live with in-between states > anyway). And there's always special fun like the ->get_hw_state > callback i915 drivers have. So I think we need to allow such > flexibility, even when ignoring how to get there. > > My idea to be able to pull shared drivers of in this scenario is to > add a drm_bridge->driver_private void* pointer. Then we could embedde > a drm_bridge into both both drm_encoder_slave and into > intel_dvo_device. > > The unified sil164 driver would then have two init functions: One sets > up a drm encoder slave, the other a dvo i915 device. Both get required > setup data in their own special means (and we could easily add a new > dt based setup functon, too). But thanks to the ->driver_private > pointer shared code between these two interfaces doesn't need to care > one bit whether it implements the dvo or the encoder slave interface. > It just follows the ->driver_private pointer to get at all the > relevant data it needs. > > Otoh this still allows special sauce functions like get_hw_state to > accept the more specific interface struct and do fancy things on top. > Of course that specialized stuff can't be shared, but all the common > parts can. > > Not having a ->driver_private pointer would require that drm_bridge > drivers would need to embed the drm_bridge into their own data > structures. Which means that they can't support two different > drm_bridge interface extensions and so would (at least partially) > defeat the code sharing goals. > > Note that if we use drm_bridge as the baseline for implementing dsi > slaves (I think this would make sense) such issues with drm_bridge > interface extensions will be even more fun. > > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Tue, Aug 13, 2013 at 3:55 PM, Sean Paul <seanpaul@chromium.org> wrote: > On Tue, Aug 13, 2013 at 3:16 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> Hi all, >> >> Ok I've tossed around another idea with Sean on irc, but it turned out >> to be too complicated for irc. So here it's in more detail. So the >> goal is to share bridge drivers/transcoders/whatever between different >> drivers without going all-in with something like cdf but allowing easy >> transitions. Which means all the different drm drivers should be >> allowed to have their own special-sauce interfaces on top of whatever >> drm_bridge currently provides, and encoder/bdrige drivers should still >> be able to share as much as possible. >> > > [For the benefit of those not on irc atm] This sounds good to me. I > also agree with everything in your previous email, with the exception > of those pesky helper functions and my stubborn insistence to keep > them in as long as possible. btw, I think I still like the idea of not calling enable/disable from crtc helpers (ie. let the encoder call 'em), since this way we have more flexibility with the sequencing without an explosion of pre/post variants.. mode_fixup/mode_set, etc should stay called from the crtc helpers, since the sequencing here is not really so critical. For i915 or anyone not using the helpers, they just add calls to these from wherever is appropriate in their own internal helpers. BR, -R > I'm working on a v2 patch set that will include one of our DP->LVDS > bridge drivers as a proof-of-concept. > > Sean > > > >> So for an example let's look at the sil164 chip for which we have one >> driver in drm/i2c/sil164_drv.c which implements the drm i2c slave >> encoder interface and 2 second driver in drm/i915/dvo_sil164.c which >> implements the i915 dvo interface. So we have two interface >> structures: >> - struct drm_encoder_slave which subclasses currently a drm_encoder. >> - struct intel_dvo_device which doesn't subclass anything >> >> The first mismatch is that the encoder slaves are real drm encoders, >> so we'd need to add a drm_bridge first somewhere. But I guess that can >> be hacked up (and it's kinda irrelevant for new drivers like msm). >> struct intel_dvo_device is conceptually like a drm_bridge, the only >> change would be to move most of the interface vfuncs from >> intel_dvo_dev_ops to the drm_bdrige. >> >> Now furthermore we have a bunch of bridge driver specific stuff like >> the i2c adapter, saved register state and other similar stuff. For the >> i2c slave encoder we keep that in struct sil164_priv, for the i915 dvo >> case it's splattered over intel_dvo_device and a (very tiny) struct >> sil164_priv. But we could unify that easily by moving e.g. the >> i2c_adapter from the dvo interface to the driver private. >> >> So the first important thing to note is that we won't be able to unify >> these different things completely because it'd be a big pile of >> refactoring work (so we need to be able to live with in-between states >> anyway). And there's always special fun like the ->get_hw_state >> callback i915 drivers have. So I think we need to allow such >> flexibility, even when ignoring how to get there. >> >> My idea to be able to pull shared drivers of in this scenario is to >> add a drm_bridge->driver_private void* pointer. Then we could embedde >> a drm_bridge into both both drm_encoder_slave and into >> intel_dvo_device. >> >> The unified sil164 driver would then have two init functions: One sets >> up a drm encoder slave, the other a dvo i915 device. Both get required >> setup data in their own special means (and we could easily add a new >> dt based setup functon, too). But thanks to the ->driver_private >> pointer shared code between these two interfaces doesn't need to care >> one bit whether it implements the dvo or the encoder slave interface. >> It just follows the ->driver_private pointer to get at all the >> relevant data it needs. >> >> Otoh this still allows special sauce functions like get_hw_state to >> accept the more specific interface struct and do fancy things on top. >> Of course that specialized stuff can't be shared, but all the common >> parts can. >> >> Not having a ->driver_private pointer would require that drm_bridge >> drivers would need to embed the drm_bridge into their own data >> structures. Which means that they can't support two different >> drm_bridge interface extensions and so would (at least partially) >> defeat the code sharing goals. >> >> Note that if we use drm_bridge as the baseline for implementing dsi >> slaves (I think this would make sense) such issues with drm_bridge >> interface extensions will be even more fun. >> >> Cheers, Daniel >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index fc83bb9..0311e2b 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -781,6 +781,41 @@ void drm_connector_unplug_all(struct drm_device *dev) } EXPORT_SYMBOL(drm_connector_unplug_all); +int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, + const struct drm_bridge_funcs *funcs) +{ + int ret; + + drm_modeset_lock_all(dev); + + ret = drm_mode_object_get(dev, &bridge->base, DRM_MODE_OBJECT_BRIDGE); + if (ret) + goto out; + + bridge->dev = dev; + bridge->funcs = funcs; + + list_add_tail(&bridge->head, &dev->mode_config.bridge_list); + dev->mode_config.num_bridge++; + + out: + drm_modeset_unlock_all(dev); + return ret; +} +EXPORT_SYMBOL(drm_bridge_init); + +void drm_bridge_cleanup(struct drm_bridge *bridge) +{ + struct drm_device *dev = bridge->dev; + + drm_modeset_lock_all(dev); + drm_mode_object_put(dev, &bridge->base); + list_del(&bridge->head); + dev->mode_config.num_bridge--; + drm_modeset_unlock_all(dev); +} +EXPORT_SYMBOL(drm_bridge_cleanup); + int drm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, const struct drm_encoder_funcs *funcs, @@ -1190,6 +1225,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr total_objects += dev->mode_config.num_crtc; total_objects += dev->mode_config.num_connector; total_objects += dev->mode_config.num_encoder; + total_objects += dev->mode_config.num_bridge; group->id_list = kzalloc(total_objects * sizeof(uint32_t), GFP_KERNEL); if (!group->id_list) @@ -1198,6 +1234,7 @@ static int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *gr group->num_crtcs = 0; group->num_connectors = 0; group->num_encoders = 0; + group->num_bridges = 0; return 0; } @@ -1207,6 +1244,7 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_crtc *crtc; struct drm_encoder *encoder; struct drm_connector *connector; + struct drm_bridge *bridge; int ret; if ((ret = drm_mode_group_init(dev, group))) @@ -1223,6 +1261,11 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev, group->id_list[group->num_crtcs + group->num_encoders + group->num_connectors++] = connector->base.id; + list_for_each_entry(bridge, &dev->mode_config.bridge_list, head) + group->id_list[group->num_crtcs + group->num_encoders + + group->num_connectors + group->num_bridges++] = + bridge->base.id; + return 0; } EXPORT_SYMBOL(drm_mode_group_init_legacy_group); @@ -3905,6 +3948,7 @@ void drm_mode_config_init(struct drm_device *dev) INIT_LIST_HEAD(&dev->mode_config.fb_list); INIT_LIST_HEAD(&dev->mode_config.crtc_list); INIT_LIST_HEAD(&dev->mode_config.connector_list); + INIT_LIST_HEAD(&dev->mode_config.bridge_list); INIT_LIST_HEAD(&dev->mode_config.encoder_list); INIT_LIST_HEAD(&dev->mode_config.property_list); INIT_LIST_HEAD(&dev->mode_config.property_blob_list); @@ -3941,6 +3985,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) struct drm_connector *connector, *ot; struct drm_crtc *crtc, *ct; struct drm_encoder *encoder, *enct; + struct drm_bridge *bridge, *brt; struct drm_framebuffer *fb, *fbt; struct drm_property *property, *pt; struct drm_property_blob *blob, *bt; @@ -3951,6 +3996,11 @@ void drm_mode_config_cleanup(struct drm_device *dev) encoder->funcs->destroy(encoder); } + list_for_each_entry_safe(bridge, brt, + &dev->mode_config.bridge_list, head) { + bridge->funcs->destroy(bridge); + } + list_for_each_entry_safe(connector, ot, &dev->mode_config.connector_list, head) { connector->funcs->destroy(connector); diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 6a64749..30139fe 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -71,6 +71,23 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head); static bool drm_kms_helper_poll = true; module_param_named(poll, drm_kms_helper_poll, bool, 0600); +static enum drm_connector_status detect_connection( + struct drm_connector *connector, bool force) +{ + struct drm_encoder *encoder = connector->encoder; + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; + + /* + * If there's a bridge attached to the encoder (attached to + * the connector) and it can detect an active connection, + * re-route the detect call to the bridge. + */ + if (bridge && bridge->funcs->detect) + return bridge->funcs->detect(bridge, force); + + return connector->funcs->detect(connector, force); +} + static void drm_mode_validate_flag(struct drm_connector *connector, int flags) { @@ -137,7 +154,7 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, if (connector->funcs->force) connector->funcs->force(connector); } else { - connector->status = connector->funcs->detect(connector, true); + connector->status = detect_connection(connector, true); } /* Re-enable polling in case the global poll config changed. */ @@ -256,11 +273,22 @@ static void drm_encoder_disable(struct drm_encoder *encoder) { struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; + struct drm_bridge_helper_funcs *bridge_funcs; + + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->disable(encoder->bridge); + } if (encoder_funcs->disable) (*encoder_funcs->disable)(encoder); else (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); + + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->post_disable(encoder->bridge); + } } /** @@ -392,6 +420,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, struct drm_display_mode *adjusted_mode, saved_mode, saved_hwmode; struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; struct drm_encoder_helper_funcs *encoder_funcs; + struct drm_bridge_helper_funcs *bridge_funcs; int saved_x, saved_y; struct drm_encoder *encoder; bool ret = true; @@ -424,6 +453,17 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder->crtc != crtc) continue; + + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + ret = bridge_funcs->mode_fixup(encoder->bridge, mode, + adjusted_mode); + if (!ret) { + DRM_DEBUG_KMS("Bridge fixup failed\n"); + goto done; + } + } + encoder_funcs = encoder->helper_private; if (!(ret = encoder_funcs->mode_fixup(encoder, mode, adjusted_mode))) { @@ -443,9 +483,20 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder->crtc != crtc) continue; + + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->disable(encoder->bridge); + } + encoder_funcs = encoder->helper_private; /* Disable the encoders as the first thing we do. */ encoder_funcs->prepare(encoder); + + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->post_disable(encoder->bridge); + } } drm_crtc_prepare_encoders(dev); @@ -469,6 +520,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, mode->base.id, mode->name); encoder_funcs = encoder->helper_private; encoder_funcs->mode_set(encoder, mode, adjusted_mode); + + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->mode_set(encoder->bridge, mode, + adjusted_mode); + } } /* Now enable the clocks, plane, pipe, and connectors that we set up. */ @@ -479,9 +536,18 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder->crtc != crtc) continue; + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->pre_enable(encoder->bridge); + } + encoder_funcs = encoder->helper_private; encoder_funcs->commit(encoder); + if (encoder->bridge) { + bridge_funcs = encoder->bridge->helper_private; + bridge_funcs->enable(encoder->bridge); + } } /* Store real post-adjustment hardware mode. */ @@ -856,8 +922,9 @@ static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) void drm_helper_connector_dpms(struct drm_connector *connector, int mode) { struct drm_encoder *encoder = connector->encoder; + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; struct drm_crtc *crtc = encoder ? encoder->crtc : NULL; - int old_dpms; + int old_dpms, encoder_dpms = DRM_MODE_DPMS_OFF; if (mode == connector->dpms) return; @@ -865,6 +932,9 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) old_dpms = connector->dpms; connector->dpms = mode; + if (encoder) + encoder_dpms = drm_helper_choose_encoder_dpms(encoder); + /* from off to on, do crtc then encoder */ if (mode < old_dpms) { if (crtc) { @@ -876,18 +946,28 @@ void drm_helper_connector_dpms(struct drm_connector *connector, int mode) if (encoder) { struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; if (encoder_funcs->dpms) - (*encoder_funcs->dpms) (encoder, - drm_helper_choose_encoder_dpms(encoder)); + (*encoder_funcs->dpms) (encoder, encoder_dpms); + } + if (bridge) { + struct drm_bridge_helper_funcs *bridge_funcs; + + bridge_funcs = bridge->helper_private; + bridge_funcs->dpms(bridge, encoder_dpms); } } /* from on to off, do encoder then crtc */ if (mode > old_dpms) { + if (bridge) { + struct drm_bridge_helper_funcs *bridge_funcs; + + bridge_funcs = bridge->helper_private; + bridge_funcs->dpms(bridge, encoder_dpms); + } if (encoder) { struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; if (encoder_funcs->dpms) - (*encoder_funcs->dpms) (encoder, - drm_helper_choose_encoder_dpms(encoder)); + (*encoder_funcs->dpms) (encoder, encoder_dpms); } if (crtc) { struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; @@ -924,9 +1004,11 @@ int drm_helper_resume_force_mode(struct drm_device *dev) { struct drm_crtc *crtc; struct drm_encoder *encoder; + struct drm_bridge *bridge; + struct drm_bridge_helper_funcs *bridge_funcs; struct drm_encoder_helper_funcs *encoder_funcs; struct drm_crtc_helper_funcs *crtc_funcs; - int ret; + int ret, encoder_dpms; list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { @@ -946,10 +1028,20 @@ int drm_helper_resume_force_mode(struct drm_device *dev) if(encoder->crtc != crtc) continue; + encoder_dpms = drm_helper_choose_encoder_dpms( + encoder); + + bridge = encoder->bridge; + if (bridge) { + bridge_funcs = bridge->helper_private; + bridge_funcs->dpms(bridge, + encoder_dpms); + } + encoder_funcs = encoder->helper_private; if (encoder_funcs->dpms) (*encoder_funcs->dpms) (encoder, - drm_helper_choose_encoder_dpms(encoder)); + encoder_dpms); } crtc_funcs = crtc->helper_private; @@ -1006,7 +1098,7 @@ static void output_poll_execute(struct work_struct *work) !(connector->polled & DRM_CONNECTOR_POLL_DISCONNECT)) continue; - connector->status = connector->funcs->detect(connector, false); + connector->status = detect_connection(connector, false); if (old_status != connector->status) { const char *old, *new; @@ -1092,7 +1184,7 @@ void drm_helper_hpd_irq_event(struct drm_device *dev) old_status = connector->status; - connector->status = connector->funcs->detect(connector, false); + connector->status = detect_connection(connector, false); DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %s to %s\n", connector->base.id, drm_get_connector_name(connector), diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 2290b3b..a912d90 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -184,6 +184,8 @@ static ssize_t status_show(struct device *device, char *buf) { struct drm_connector *connector = to_drm_connector(device); + struct drm_encoder *encoder = connector->encoder; + struct drm_bridge *bridge = encoder ? encoder->bridge : NULL; enum drm_connector_status status; int ret; @@ -191,7 +193,11 @@ static ssize_t status_show(struct device *device, if (ret) return ret; - status = connector->funcs->detect(connector, true); + if (bridge && bridge->funcs->detect) + status = bridge->funcs->detect(bridge, true); + else + status = connector->funcs->detect(connector, true); + mutex_unlock(&connector->dev->mode_config.mutex); return snprintf(buf, PAGE_SIZE, "%s\n", diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fa12a2f..f020957 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -49,6 +49,7 @@ struct drm_clip_rect; #define DRM_MODE_OBJECT_FB 0xfbfbfbfb #define DRM_MODE_OBJECT_BLOB 0xbbbbbbbb #define DRM_MODE_OBJECT_PLANE 0xeeeeeeee +#define DRM_MODE_OBJECT_BRIDGE 0xbdbdbdbd struct drm_mode_object { uint32_t id; @@ -305,6 +306,7 @@ struct drm_connector; struct drm_encoder; struct drm_pending_vblank_event; struct drm_plane; +struct drm_bridge; /** * drm_crtc_funcs - control CRTCs for a given device @@ -507,6 +509,7 @@ struct drm_encoder_funcs { * @possible_crtcs: bitmask of potential CRTC bindings * @possible_clones: bitmask of potential sibling encoders for cloning * @crtc: currently bound CRTC + * @bridge: bridge associated to the encoder * @funcs: control functions * @helper_private: mid-layer private data * @@ -523,6 +526,7 @@ struct drm_encoder { uint32_t possible_clones; struct drm_crtc *crtc; + struct drm_bridge *bridge; const struct drm_encoder_funcs *funcs; void *helper_private; }; @@ -683,6 +687,41 @@ struct drm_plane { }; /** + * drm_bridge_funcs - drm_bridge control functions + * @detect: Checks if something is connected to the bridge. If this is + * implemented, it should take precedence over connector->detect() + * since the connector may not be able to deduce whether something is + * connected downstream of the bridge. + * @destroy: make object go away + */ +struct drm_bridge_funcs { + enum drm_connector_status (*detect)(struct drm_bridge *bridge, + bool force); + void (*destroy)(struct drm_bridge *bridge); +}; + +/** + * drm_bridge - central DRM bridge control structure + * @dev: DRM device this bridge belongs to + * @head: list management + * @base: base mode object + * @connector_type: the type of connector this bridge can associate with + * @funcs: control functions + * @helper_private: mid-layer private data + */ +struct drm_bridge { + struct drm_device *dev; + struct list_head head; + + struct drm_mode_object base; + + int connector_type; + + const struct drm_bridge_funcs *funcs; + void *helper_private; +}; + +/** * drm_mode_set - new values for a CRTC config change * @head: list management * @fb: framebuffer to use for new config @@ -742,6 +781,7 @@ struct drm_mode_group { uint32_t num_crtcs; uint32_t num_encoders; uint32_t num_connectors; + uint32_t num_bridges; /* list of object IDs for this group */ uint32_t *id_list; @@ -756,6 +796,8 @@ struct drm_mode_group { * @fb_list: list of framebuffers available * @num_connector: number of connectors on this device * @connector_list: list of connector objects + * @num_bridge: number of bridges on this device + * @bridge_list: list of bridge objects * @num_encoder: number of encoders on this device * @encoder_list: list of encoder objects * @num_crtc: number of CRTCs on this device @@ -793,6 +835,8 @@ struct drm_mode_config { int num_connector; struct list_head connector_list; + int num_bridge; + struct list_head bridge_list; int num_encoder; struct list_head encoder_list; int num_plane; @@ -878,6 +922,10 @@ extern void drm_connector_cleanup(struct drm_connector *connector); /* helper to unplug all connectors from sysfs for device */ extern void drm_connector_unplug_all(struct drm_device *dev); +extern int drm_bridge_init(struct drm_device *dev, struct drm_bridge *bridge, + const struct drm_bridge_funcs *funcs); +extern void drm_bridge_cleanup(struct drm_bridge *bridge); + extern int drm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, const struct drm_encoder_funcs *funcs, diff --git a/include/drm/drm_crtc_helper.h b/include/drm/drm_crtc_helper.h index f43d556..89a11f9 100644 --- a/include/drm/drm_crtc_helper.h +++ b/include/drm/drm_crtc_helper.h @@ -125,6 +125,34 @@ struct drm_connector_helper_funcs { struct drm_encoder *(*best_encoder)(struct drm_connector *connector); }; +/** + * drm_bridge_helper_funcs - helper operations for bridges (all required) + * @dpms: Control power levels on the bridge. If the mode passed in is + * unsupported, the provider must use the next lowest power level. + * @mode_fixup: Try to fixup (or reject entirely) proposed mode for this bridge + * @disable: Called right before encoder prepare, disables the bridge + * @post_disable: Called right after encoder prepare, for lockstepped disable + * @mode_set: Set this mode to the bridge + * @pre_enable: Called right before encoder commit, for lockstepped commit + * @enable: Called right after encoder commit, enables the bridge + * + * The helper operations are called by the mid-layer CRTC helper, however they + * can also be called directly by drivers that don't use the helper functions. + */ +struct drm_bridge_helper_funcs { + void (*dpms)(struct drm_bridge *bridge, int mode); + bool (*mode_fixup)(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); + void (*disable)(struct drm_bridge *bridge); + void (*post_disable)(struct drm_bridge *bridge); + void (*mode_set)(struct drm_bridge *bridge, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); + void (*pre_enable)(struct drm_bridge *bridge); + void (*enable)(struct drm_bridge *bridge); +}; + extern int drm_helper_probe_single_connector_modes(struct drm_connector *connector, uint32_t maxX, uint32_t maxY); extern void drm_helper_disable_unused_functions(struct drm_device *dev); extern int drm_crtc_helper_set_config(struct drm_mode_set *set); @@ -160,6 +188,12 @@ static inline void drm_connector_helper_add(struct drm_connector *connector, connector->helper_private = (void *)funcs; } +static inline void drm_bridge_helper_add(struct drm_bridge *bridge, + const struct drm_bridge_helper_funcs *funcs) +{ + bridge->helper_private = (void *)funcs; +} + extern int drm_helper_resume_force_mode(struct drm_device *dev); extern void drm_kms_helper_poll_init(struct drm_device *dev); extern void drm_kms_helper_poll_fini(struct drm_device *dev);
This patch adds the notion of a drm_bridge. A bridge is a chained device which hangs off an encoder. The drm driver using the bridge should provide the association between encoder and bridge. Once a bridge is associated with an encoder, it will participate in mode set, dpms, and optionally connection detection. Since a connector may not be able to determine the connection state downstream of the bridge, bridges may implement detect() which will take precedence over connector detect() if the bridge is already attached to an encoder and that encoder is already attached to the connector. In practical terms, this requires the drm driver to make these associations at init time, which is fine for SoC applications where this link is static. Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++ drivers/gpu/drm/drm_crtc_helper.c | 112 ++++++++++++++++++++++++++++++++++---- drivers/gpu/drm/drm_sysfs.c | 8 ++- include/drm/drm_crtc.h | 48 ++++++++++++++++ include/drm/drm_crtc_helper.h | 34 ++++++++++++ 5 files changed, 241 insertions(+), 11 deletions(-)