From: Douglas Anderson <dianders@chromium.org> To: dri-devel@lists.freedesktop.org Cc: Philip Chen <philipchen@chromium.org>, Sankeerth Billakanti <quic_sbillaka@quicinc.com>, Stephen Boyd <swboyd@chromium.org>, freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, Hsin-Yi Wang <hsinyi@chromium.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, Robert Foss <robert.foss@linaro.org>, Abhinav Kumar <quic_abhinavk@quicinc.com>, Douglas Anderson <dianders@chromium.org>, Andrzej Hajda <andrzej.hajda@intel.com>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>, Jernej Skrabec <jernej.skrabec@gmail.com>, Jonas Karlman <jonas@kwiboo.se>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Neil Armstrong <narmstrong@baylibre.com>, linux-kernel@vger.kernel.org Subject: [PATCH v2 2/2] drm/bridge: parade-ps8640: Handle DP AUX more properly Date: Tue, 3 May 2022 15:40:29 -0700 [thread overview] Message-ID: <20220503153850.v2.2.Ia6324ebc848cd40b4dbd3ad3289a7ffb5c197779@changeid> (raw) In-Reply-To: <20220503224029.3195306-1-dianders@chromium.org> While it works, for the most part, to assume that the panel has finished probing when devm_of_dp_aux_populate_ep_devices() returns, it's a bit fragile. This is talked about at length in commit a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev"). When reviewing the ps8640 code, I managed to convince myself that it was OK not to worry about it there and that maybe it wasn't really _that_ fragile. However, it turns out that it really is. Simply hardcoding panel_edp_probe() to return -EPROBE_DEFER was enough to put the boot process into an infinite loop. I believe this manages to trip the same issues that we used to trip with the main MSM code where something about our actions trigger Linux to re-probe previously deferred devices right away and each time we try again we re-trigger Linux to re-probe. Let's fix this using the callback introduced in the patch ("drm/dp: Callbacks to make it easier for drivers to use DP AUX bus properly"). When using the new callback, we have to be a little careful. The probe_done() callback is no longer (always) called in the context of our probe routine. That means we can't rely on being able to return -EPROBE_DEFER from it. We re-jigger the order of things a bit to account for that. With this change, the device still boots (though obviously the panel doesn't come up) if I force panel-edp to always return -EPROBE_DEFER. If I fake it and make the panel probe exactly once it also works. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Rewrote atop new method introduced by patch #1. drivers/gpu/drm/bridge/parade-ps8640.c | 77 +++++++++++++++++--------- 1 file changed, 52 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index edb939b14c04..68131ca91eac 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -104,6 +104,7 @@ struct ps8640 { struct gpio_desc *gpio_powerdown; struct device_link *link; bool pre_enabled; + bool bridge_added; }; static const struct regmap_config ps8640_regmap_config[] = { @@ -537,12 +538,11 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .pre_enable = ps8640_pre_enable, }; -static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge) +static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps_bridge) { struct device_node *in_ep, *dsi_node; struct mipi_dsi_device *dsi; struct mipi_dsi_host *host; - int ret; const struct mipi_dsi_device_info info = { .type = "ps8640", .channel = 0, .node = NULL, @@ -577,17 +577,44 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg dsi->format = MIPI_DSI_FMT_RGB888; dsi->lanes = NUM_MIPI_LANES; - ret = devm_mipi_dsi_attach(dev, dsi); + return 0; +} + +static int ps8640_bridge_link_panel(struct drm_dp_aux *aux) +{ + struct ps8640 *ps_bridge = aux_to_ps8640(aux); + struct device *dev = aux->dev; + struct device_node *np = dev->of_node; + int ret; + + /* + * NOTE about returning -EPROBE_DEFER from this function: if we + * return an error (most relevant to -EPROBE_DEFER) it will only + * be passed out to ps8640_probe() if we don't have our panel + * under the "aux-bus". That should be fine because if the panel is + * under "aux-bus" it's guaranteed to have probed by the time this + * function has been called. + */ + + /* port@1 is ps8640 output port */ + ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0); + if (IS_ERR(ps_bridge->panel_bridge)) + return PTR_ERR(ps_bridge->panel_bridge); + + drm_bridge_add(&ps_bridge->bridge); + + ret = devm_mipi_dsi_attach(dev, ps_bridge->dsi); if (ret) - return ret; + drm_bridge_remove(&ps_bridge->bridge); + else + ps_bridge->bridge_added = true; - return 0; + return ret; } static int ps8640_probe(struct i2c_client *client) { struct device *dev = &client->dev; - struct device_node *np = dev->of_node; struct ps8640 *ps_bridge; int ret; u32 i; @@ -628,6 +655,14 @@ static int ps8640_probe(struct i2c_client *client) if (!ps8640_of_panel_on_aux_bus(&client->dev)) ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID; + /* + * Get MIPI DSI resources early. These can return -EPROBE_DEFER so + * we want to get them out of the way sooner. + */ + ret = ps8640_bridge_get_dsi_resources(&client->dev, ps_bridge); + if (ret) + return ret; + ps_bridge->page[PAGE0_DP_CNTL] = client; ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config); @@ -670,31 +705,23 @@ static int ps8640_probe(struct i2c_client *client) if (ret) return ret; - devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux); - - /* port@1 is ps8640 output port */ - ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0); - if (IS_ERR(ps_bridge->panel_bridge)) - return PTR_ERR(ps_bridge->panel_bridge); - - drm_bridge_add(&ps_bridge->bridge); - - ret = ps8640_bridge_host_attach(dev, ps_bridge); - if (ret) - goto err_bridge_remove; - - return 0; - -err_bridge_remove: - drm_bridge_remove(&ps_bridge->bridge); - return ret; + /* + * Kick off the probe for our panel if it's on the dp-aux bus. + * If the panel is on the aux bus ps8640_bridge_link_panel() will + * get called when it finishes probing. If the panel is an old-style + * platform device ps8640_bridge_link_panel() will be called directly + * and its return value will be the return value of our function. + */ + return devm_of_dp_aux_populate_ep_device(&ps_bridge->aux, + ps8640_bridge_link_panel); } static int ps8640_remove(struct i2c_client *client) { struct ps8640 *ps_bridge = i2c_get_clientdata(client); - drm_bridge_remove(&ps_bridge->bridge); + if (ps_bridge->bridge_added) + drm_bridge_remove(&ps_bridge->bridge); return 0; } -- 2.36.0.464.gb9c8b46e94-goog
WARNING: multiple messages have this Message-ID (diff)
From: Douglas Anderson <dianders@chromium.org> To: dri-devel@lists.freedesktop.org Cc: Douglas Anderson <dianders@chromium.org>, Sankeerth Billakanti <quic_sbillaka@quicinc.com>, Philip Chen <philipchen@chromium.org>, Jonas Karlman <jonas@kwiboo.se>, David Airlie <airlied@linux.ie>, linux-arm-msm@vger.kernel.org, Neil Armstrong <narmstrong@baylibre.com>, Abhinav Kumar <quic_abhinavk@quicinc.com>, Robert Foss <robert.foss@linaro.org>, Stephen Boyd <swboyd@chromium.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Andrzej Hajda <andrzej.hajda@intel.com>, Hsin-Yi Wang <hsinyi@chromium.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org, Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Subject: [PATCH v2 2/2] drm/bridge: parade-ps8640: Handle DP AUX more properly Date: Tue, 3 May 2022 15:40:29 -0700 [thread overview] Message-ID: <20220503153850.v2.2.Ia6324ebc848cd40b4dbd3ad3289a7ffb5c197779@changeid> (raw) In-Reply-To: <20220503224029.3195306-1-dianders@chromium.org> While it works, for the most part, to assume that the panel has finished probing when devm_of_dp_aux_populate_ep_devices() returns, it's a bit fragile. This is talked about at length in commit a1e3667a9835 ("drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev"). When reviewing the ps8640 code, I managed to convince myself that it was OK not to worry about it there and that maybe it wasn't really _that_ fragile. However, it turns out that it really is. Simply hardcoding panel_edp_probe() to return -EPROBE_DEFER was enough to put the boot process into an infinite loop. I believe this manages to trip the same issues that we used to trip with the main MSM code where something about our actions trigger Linux to re-probe previously deferred devices right away and each time we try again we re-trigger Linux to re-probe. Let's fix this using the callback introduced in the patch ("drm/dp: Callbacks to make it easier for drivers to use DP AUX bus properly"). When using the new callback, we have to be a little careful. The probe_done() callback is no longer (always) called in the context of our probe routine. That means we can't rely on being able to return -EPROBE_DEFER from it. We re-jigger the order of things a bit to account for that. With this change, the device still boots (though obviously the panel doesn't come up) if I force panel-edp to always return -EPROBE_DEFER. If I fake it and make the panel probe exactly once it also works. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Rewrote atop new method introduced by patch #1. drivers/gpu/drm/bridge/parade-ps8640.c | 77 +++++++++++++++++--------- 1 file changed, 52 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index edb939b14c04..68131ca91eac 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -104,6 +104,7 @@ struct ps8640 { struct gpio_desc *gpio_powerdown; struct device_link *link; bool pre_enabled; + bool bridge_added; }; static const struct regmap_config ps8640_regmap_config[] = { @@ -537,12 +538,11 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .pre_enable = ps8640_pre_enable, }; -static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridge) +static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps_bridge) { struct device_node *in_ep, *dsi_node; struct mipi_dsi_device *dsi; struct mipi_dsi_host *host; - int ret; const struct mipi_dsi_device_info info = { .type = "ps8640", .channel = 0, .node = NULL, @@ -577,17 +577,44 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg dsi->format = MIPI_DSI_FMT_RGB888; dsi->lanes = NUM_MIPI_LANES; - ret = devm_mipi_dsi_attach(dev, dsi); + return 0; +} + +static int ps8640_bridge_link_panel(struct drm_dp_aux *aux) +{ + struct ps8640 *ps_bridge = aux_to_ps8640(aux); + struct device *dev = aux->dev; + struct device_node *np = dev->of_node; + int ret; + + /* + * NOTE about returning -EPROBE_DEFER from this function: if we + * return an error (most relevant to -EPROBE_DEFER) it will only + * be passed out to ps8640_probe() if we don't have our panel + * under the "aux-bus". That should be fine because if the panel is + * under "aux-bus" it's guaranteed to have probed by the time this + * function has been called. + */ + + /* port@1 is ps8640 output port */ + ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0); + if (IS_ERR(ps_bridge->panel_bridge)) + return PTR_ERR(ps_bridge->panel_bridge); + + drm_bridge_add(&ps_bridge->bridge); + + ret = devm_mipi_dsi_attach(dev, ps_bridge->dsi); if (ret) - return ret; + drm_bridge_remove(&ps_bridge->bridge); + else + ps_bridge->bridge_added = true; - return 0; + return ret; } static int ps8640_probe(struct i2c_client *client) { struct device *dev = &client->dev; - struct device_node *np = dev->of_node; struct ps8640 *ps_bridge; int ret; u32 i; @@ -628,6 +655,14 @@ static int ps8640_probe(struct i2c_client *client) if (!ps8640_of_panel_on_aux_bus(&client->dev)) ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID; + /* + * Get MIPI DSI resources early. These can return -EPROBE_DEFER so + * we want to get them out of the way sooner. + */ + ret = ps8640_bridge_get_dsi_resources(&client->dev, ps_bridge); + if (ret) + return ret; + ps_bridge->page[PAGE0_DP_CNTL] = client; ps_bridge->regmap[PAGE0_DP_CNTL] = devm_regmap_init_i2c(client, ps8640_regmap_config); @@ -670,31 +705,23 @@ static int ps8640_probe(struct i2c_client *client) if (ret) return ret; - devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux); - - /* port@1 is ps8640 output port */ - ps_bridge->panel_bridge = devm_drm_of_get_bridge(dev, np, 1, 0); - if (IS_ERR(ps_bridge->panel_bridge)) - return PTR_ERR(ps_bridge->panel_bridge); - - drm_bridge_add(&ps_bridge->bridge); - - ret = ps8640_bridge_host_attach(dev, ps_bridge); - if (ret) - goto err_bridge_remove; - - return 0; - -err_bridge_remove: - drm_bridge_remove(&ps_bridge->bridge); - return ret; + /* + * Kick off the probe for our panel if it's on the dp-aux bus. + * If the panel is on the aux bus ps8640_bridge_link_panel() will + * get called when it finishes probing. If the panel is an old-style + * platform device ps8640_bridge_link_panel() will be called directly + * and its return value will be the return value of our function. + */ + return devm_of_dp_aux_populate_ep_device(&ps_bridge->aux, + ps8640_bridge_link_panel); } static int ps8640_remove(struct i2c_client *client) { struct ps8640 *ps_bridge = i2c_get_clientdata(client); - drm_bridge_remove(&ps_bridge->bridge); + if (ps_bridge->bridge_added) + drm_bridge_remove(&ps_bridge->bridge); return 0; } -- 2.36.0.464.gb9c8b46e94-goog
next prev parent reply other threads:[~2022-05-03 22:40 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-03 22:40 [PATCH v2 0/2] drm/dp: Make DP AUX bus usage easier; use it on ps8640 Douglas Anderson 2022-05-03 22:40 ` Douglas Anderson 2022-05-03 22:40 ` [PATCH v2 1/2] drm/dp: Add callbacks to make using DP AUX bus properly easier Douglas Anderson 2022-05-03 22:40 ` Douglas Anderson 2022-05-04 10:41 ` Dmitry Baryshkov 2022-05-04 10:41 ` Dmitry Baryshkov 2022-05-04 14:53 ` Doug Anderson 2022-05-04 14:53 ` Doug Anderson 2022-05-03 22:40 ` Douglas Anderson [this message] 2022-05-03 22:40 ` [PATCH v2 2/2] drm/bridge: parade-ps8640: Handle DP AUX more properly Douglas Anderson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220503153850.v2.2.Ia6324ebc848cd40b4dbd3ad3289a7ffb5c197779@changeid \ --to=dianders@chromium.org \ --cc=Laurent.pinchart@ideasonboard.com \ --cc=airlied@linux.ie \ --cc=andrzej.hajda@intel.com \ --cc=daniel@ffwll.ch \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=hsinyi@chromium.org \ --cc=jernej.skrabec@gmail.com \ --cc=jonas@kwiboo.se \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=narmstrong@baylibre.com \ --cc=philipchen@chromium.org \ --cc=quic_abhinavk@quicinc.com \ --cc=quic_sbillaka@quicinc.com \ --cc=robert.foss@linaro.org \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.