| Message ID | 1649938766-6768-2-git-send-email-quic_sbillaka@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v7,1/4] drm/msm/dp: Add eDP support via aux_bus | expand |
Quoting Dmitry Baryshkov (2022-04-14 12:16:14) > > I think it's too verbose and a bit incorrect. > This is a bit saner: > /* > * These ops do not make sense for eDP, since they are provided > * by the panel-bridge corresponding to the attached eDP panel. > */ > > My question was whether we really need to disable them for eDP since for > eDP the detect and and get_modes will be overridden anyway. And to go further, I'd expect that a bridge should expose the functionality that it supports, regardless of what is connected down the chain. Otherwise we won't be able to mix and match bridges because the code is brittle, making assumptions about what is connected.
Hi, On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Dmitry Baryshkov (2022-04-14 12:16:14) > > > > I think it's too verbose and a bit incorrect. Not sure which part you're asserting is incorrect, but shorter is OK w/ me too. > > This is a bit saner: > > /* > > * These ops do not make sense for eDP, since they are provided > > * by the panel-bridge corresponding to the attached eDP panel. > > */ > > > > My question was whether we really need to disable them for eDP since for > > eDP the detect and and get_modes will be overridden anyway. Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? It's definitely worth confirming but from my reading of the code it _probably_ wouldn't hurt. One thing someone would want to confirm would be what would happen if we move this code and the panel code to implement DRM_BRIDGE_OP_EDID properly. It looks as if both actually ought to be implementing that instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a future day. Could we get into trouble if one moved before the other? Then the panel would no longer override the eDP controller and the eDP controller would try to read from a possibly unpowered panel? So I guess in the end my preference would be that we know that driving the EDID read from the controller isn't a great idea for eDP (since we have no way to ensure that the panel is powered) so why risk it and set the bit saying we can do it? For hotplug/detect I'm even less confident that setting the bits would be harmless. I haven't sat down and traced everything, but from what I can see the panel _doesn't_ set these bits, does it? I believe that the rule is that when every bridge in the chain _doesn't_ implement detect/hotplug that the panel is always present. The moment someone says "hey, I can detect" then it suddenly becomes _not_ always present. Yes, I guess we could have the panel implement "detect" and return true, but I'm not convinced that's actually better... > And to go further, I'd expect that a bridge should expose the > functionality that it supports, regardless of what is connected down the > chain. Otherwise we won't be able to mix and match bridges because the > code is brittle, making assumptions about what is connected.
On 14/04/2022 23:09, Doug Anderson wrote: > Hi, > > On Thu, Apr 14, 2022 at 12:40 PM Stephen Boyd <swboyd@chromium.org> wrote: >> >> Quoting Dmitry Baryshkov (2022-04-14 12:16:14) >>> >>> I think it's too verbose and a bit incorrect. > > Not sure which part you're asserting is incorrect, but shorter is OK w/ me too. I was referring to the "If we don't implement the ops..." part. For some reason I thought that panel implements detect() callback (and thus the DRM will not care because the next bridge takes precedence). However I was mistaken, please excuse me. Your description was correct and I was wrong. The panel bridge doesn't implement callback. Most probably I mixed it with the display_connector bridge. So... your description is more correct. > > >>> This is a bit saner: >>> /* >>> * These ops do not make sense for eDP, since they are provided >>> * by the panel-bridge corresponding to the attached eDP panel. >>> */ >>> >>> My question was whether we really need to disable them for eDP since for >>> eDP the detect and and get_modes will be overridden anyway. > > Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? > It's definitely worth confirming but from my reading of the code it > _probably_ wouldn't hurt. > > One thing someone would want to confirm would be what would happen if > we move this code and the panel code to implement DRM_BRIDGE_OP_EDID > properly. It looks as if both actually ought to be implementing that > instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a > future day. Could we get into trouble if one moved before the other? > Then the panel would no longer override the eDP controller and the eDP > controller would try to read from a possibly unpowered panel? That would depend on the way the get_edid would be implemented in DP driver. Currently the edid is cached via the dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain. With this patchset, the dp_hpd_plug_handle() -> dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be called too late for the get_modes/get_edid (from dp_bridge's enable() op). There is another issue. drm_panel has only get_modes() callback, so panel_bridge can not implement get_edid() unless we extend the panel interface (which might be a good idea). > > So I guess in the end my preference would be that we know that driving > the EDID read from the controller isn't a great idea for eDP (since we > have no way to ensure that the panel is powered) so why risk it and > set the bit saying we can do it? Yep. > For hotplug/detect I'm even less confident that setting the bits would > be harmless. I haven't sat down and traced everything, but from what I > can see the panel _doesn't_ set these bits, does it? I believe that > the rule is that when every bridge in the chain _doesn't_ implement > detect/hotplug that the panel is always present. The moment someone > says "hey, I can detect" then it suddenly becomes _not_ always > present. Yes, I guess we could have the panel implement "detect" and > return true, but I'm not convinced that's actually better... I think it makes sense to implement OP_DETECT in panel bridge (that always returns connector_status_connected) at least to override the possible detect ops in previous bridges. >> And to go further, I'd expect that a bridge should expose the >> functionality that it supports, regardless of what is connected down the >> chain. Otherwise we won't be able to mix and match bridges because the >> code is brittle, making assumptions about what is connected. > > From my point of view the bridge simply doesn't support any of the > three things when we're in eDP mode. Yes, it's the same driver. Yes, > eDP and DP share nearly the same signalling on the wire. Yes, it's > easily possible to make a single controller that supports DP and eDP. > ...but the rules around detection and power sequencing are simply > different for the two cases. I just hope that during refactoring this can be expressed in a more natural way. > The controller simply _cannot_ detect > whether the panel is connected in the eDP case and it _must_ assume > that the panel is always connected. Yes, it has an HPD pin. No, that > HPD pin doesn't tell when the panel is present. The panel is always > present. The panel is always present. Yep, I remember regarding the HPD pin. And I still think that panel-edp (and panel bridge) should provide an overriding OP_DETECT. > So, IMO, it is _incorrect_ to say we can support HPD and DETECT if we > know we're in eDP mode. I see your point. Let's do it this way. Maybe (hopefully) it will become more logical during refactoring. Or maybe I'll just tune myself into the DP/eDP logic :D
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index d7a19d6..43c59cb 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -10,6 +10,7 @@ #include <linux/component.h> #include <linux/of_irq.h> #include <linux/delay.h> +#include <drm/dp/drm_dp_aux_bus.h> #include "msm_drv.h" #include "msm_kms.h" @@ -259,14 +260,12 @@ static int dp_display_bind(struct device *dev, struct device *master, dp->dp_display.drm_dev = drm; priv->dp[dp->id] = &dp->dp_display; - rc = dp->parser->parse(dp->parser, dp->dp_display.connector_type); + rc = dp->parser->parse(dp->parser); if (rc) { DRM_ERROR("device tree parsing failed\n"); goto end; } - dp->dp_display.next_bridge = dp->parser->next_bridge; - dp->aux->drm_dev = drm; rc = dp_aux_register(dp->aux); if (rc) { @@ -1319,6 +1318,8 @@ static int dp_display_probe(struct platform_device *pdev) dp->pdev = pdev; dp->name = "drm_dp"; dp->dp_display.connector_type = desc->connector_type; + dp->dp_display.is_edp = + (dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP); rc = dp_init_sub_modules(dp); if (rc) { @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display) dp_hpd_event_setup(dp); - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); + if (!dp_display->is_edp) + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); } void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) @@ -1530,6 +1532,60 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) } } +static int dp_display_get_next_bridge(struct msm_dp *dp) +{ + int rc; + struct dp_display_private *dp_priv; + struct device_node *aux_bus; + struct device *dev; + + dp_priv = container_of(dp, struct dp_display_private, dp_display); + dev = &dp_priv->pdev->dev; + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); + + if (aux_bus && dp->is_edp) { + dp_display_host_init(dp_priv); + dp_catalog_ctrl_hpd_config(dp_priv->catalog); + dp_display_host_phy_init(dp_priv); + enable_irq(dp_priv->irq); + + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux); + of_node_put(aux_bus); + if (rc) { + disable_irq(dp_priv->irq); + dp_display_host_phy_exit(dp_priv); + dp_display_host_deinit(dp_priv); + return rc; + } + } else if (dp->is_edp) { + DRM_ERROR("eDP aux_bus not found\n"); + return -ENODEV; + } + + /* + * External bridges are mandatory for eDP interfaces: one has to + * provide at least an eDP panel (which gets wrapped into panel-bridge). + * + * For DisplayPort interfaces external bridges are optional, so + * silently ignore an error if one is not present (-ENODEV). + */ + rc = dp_parser_find_next_bridge(dp_priv->parser); + if (rc == -ENODEV) { + if (dp->is_edp) { + DRM_ERROR("eDP: next bridge is not present\n"); + return rc; + } + } else if (rc) { + if (rc != -EPROBE_DEFER) + DRM_ERROR("DP: error parsing next bridge: %d\n", rc); + return rc; + } + + dp->next_bridge = dp_priv->parser->next_bridge; + + return 0; +} + int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder) { @@ -1553,6 +1609,10 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, dp_display->encoder = encoder; + ret = dp_display_get_next_bridge(dp_display); + if (ret) + return ret; + dp_display->bridge = dp_bridge_init(dp_display, dev, encoder); if (IS_ERR(dp_display->bridge)) { ret = PTR_ERR(dp_display->bridge); diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h index 49a1d89..1377cc3 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.h +++ b/drivers/gpu/drm/msm/dp/dp_display.h @@ -21,6 +21,7 @@ struct msm_dp { bool audio_enabled; bool power_on; unsigned int connector_type; + bool is_edp; hdmi_codec_plugged_cb plugged_cb; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 7ce1aca..5f0293f 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -114,10 +114,12 @@ struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device * bridge->funcs = &dp_bridge_ops; bridge->type = dp_display->connector_type; - bridge->ops = - DRM_BRIDGE_OP_DETECT | - DRM_BRIDGE_OP_HPD | - DRM_BRIDGE_OP_MODES; + if (!dp_display->is_edp) { + bridge->ops = + DRM_BRIDGE_OP_DETECT | + DRM_BRIDGE_OP_HPD | + DRM_BRIDGE_OP_MODES; + } rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (rc) { diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c index 1056b8d..4bdbf91 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.c +++ b/drivers/gpu/drm/msm/dp/dp_parser.c @@ -265,7 +265,7 @@ static int dp_parser_clock(struct dp_parser *parser) return 0; } -static int dp_parser_find_next_bridge(struct dp_parser *parser) +int dp_parser_find_next_bridge(struct dp_parser *parser) { struct device *dev = &parser->pdev->dev; struct drm_bridge *bridge; @@ -279,7 +279,7 @@ static int dp_parser_find_next_bridge(struct dp_parser *parser) return 0; } -static int dp_parser_parse(struct dp_parser *parser, int connector_type) +static int dp_parser_parse(struct dp_parser *parser) { int rc = 0; @@ -300,25 +300,6 @@ static int dp_parser_parse(struct dp_parser *parser, int connector_type) if (rc) return rc; - /* - * External bridges are mandatory for eDP interfaces: one has to - * provide at least an eDP panel (which gets wrapped into panel-bridge). - * - * For DisplayPort interfaces external bridges are optional, so - * silently ignore an error if one is not present (-ENODEV). - */ - rc = dp_parser_find_next_bridge(parser); - if (rc == -ENODEV) { - if (connector_type == DRM_MODE_CONNECTOR_eDP) { - DRM_ERROR("eDP: next bridge is not present\n"); - return rc; - } - } else if (rc) { - if (rc != -EPROBE_DEFER) - DRM_ERROR("DP: error parsing next bridge: %d\n", rc); - return rc; - } - /* Map the corresponding regulator information according to * version. Currently, since we only have one supported platform, * mapping the regulator directly. diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h index d371bae..950416c 100644 --- a/drivers/gpu/drm/msm/dp/dp_parser.h +++ b/drivers/gpu/drm/msm/dp/dp_parser.h @@ -125,7 +125,7 @@ struct dp_parser { u32 max_dp_lanes; struct drm_bridge *next_bridge; - int (*parse)(struct dp_parser *parser, int connector_type); + int (*parse)(struct dp_parser *parser); }; /** @@ -141,4 +141,15 @@ struct dp_parser { */ struct dp_parser *dp_parser_get(struct platform_device *pdev); +/** + * dp_parser_find_next_bridge() - find an additional bridge to DP + * + * @parser: dp_parser data from client + * return: 0 if able to get the bridge else return an error code + * + * This function is used to find any additional bridge attached to + * the DP controller. The eDP interface requires a panel bridge. + */ +int dp_parser_find_next_bridge(struct dp_parser *parser); + #endif
This patch adds support for generic eDP sink through aux_bus. The eDP/DP controller driver should support aux transactions originating from the panel-edp driver and hence should be initialized and ready. The panel bridge supporting the panel should be ready before the bridge connector is initialized. The generic panel probe needs the controller resources to be enabled to support the aux transactions originating from the panel probe. Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> --- Changes in v7: - aux_bus is mandatory for eDP - connector type check modified to just check for eDP Changes in v6: - Remove initialization - Fix aux_bus node leak - Split the patches drivers/gpu/drm/msm/dp/dp_display.c | 68 ++++++++++++++++++++++++++++++++++--- drivers/gpu/drm/msm/dp/dp_display.h | 1 + drivers/gpu/drm/msm/dp/dp_drm.c | 10 +++--- drivers/gpu/drm/msm/dp/dp_parser.c | 23 ++----------- drivers/gpu/drm/msm/dp/dp_parser.h | 13 ++++++- 5 files changed, 85 insertions(+), 30 deletions(-)