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@xxxxxxxx> wrote: > > On Thu, Aug 8, 2013 at 11:03 PM, Sean Paul <seanpaul@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > > 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
