All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Douglas Anderson <dianders@chromium.org>,
	dri-devel@lists.freedesktop.org
Cc: 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>,
	freedreno@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v3 4/4] drm/bridge: parade-ps8640: Handle DP AUX more properly
Date: Wed, 1 Jun 2022 23:46:54 +0300	[thread overview]
Message-ID: <f415aa0c-edf8-94de-bde8-faf151f5f530@linaro.org> (raw)
In-Reply-To: <20220510122726.v3.4.Ia6324ebc848cd40b4dbd3ad3289a7ffb5c197779@changeid>

On 10/05/2022 22:29, Douglas Anderson wrote:
> 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>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> 
> Changes in v3:
> - Adapt to v3 changes in aux bus.
> - Use devm_drm_bridge_add() to simplify.
> 
> Changes in v2:
> - Rewrote atop new method introduced by patch #1.
> 
>   drivers/gpu/drm/bridge/parade-ps8640.c | 74 ++++++++++++++++----------
>   1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index e2467e58b5b7..ff4227f6d800 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -537,7 +537,7 @@ 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;
> @@ -576,13 +576,40 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg
>   	dsi->format = MIPI_DSI_FMT_RGB888;
>   	dsi->lanes = NUM_MIPI_LANES;
>   
> -	return 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 it called this directly (AKA the
> +	 * panel isn't under the "aux-bus" node). 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);
> +
> +	ret = devm_drm_bridge_add(dev, &ps_bridge->bridge);
> +	if (ret)
> +		return ret;
> +
> +	return devm_mipi_dsi_attach(dev, ps_bridge->dsi);
>   }
>   
>   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;
> @@ -623,6 +650,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);
> @@ -665,35 +700,19 @@ static int ps8640_probe(struct i2c_client *client)
>   	if (ret)
>   		return ret;
>   
> -	devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
> +	ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
>   
> -	/* 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;
> +	/*
> +	 * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
> +	 * usa to call ps8640_bridge_link_panel() directly. NOTE: in this case
> +	 * the function is allowed to -EPROBE_DEFER.
> +	 */
> +	if (ret == -ENODEV)
> +		return ps8640_bridge_link_panel(&ps_bridge->aux);
>   
> -err_bridge_remove:
> -	drm_bridge_remove(&ps_bridge->bridge);
>   	return ret;
>   }
>   
> -static int ps8640_remove(struct i2c_client *client)
> -{
> -	struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> -
> -	drm_bridge_remove(&ps_bridge->bridge);
> -
> -	return 0;
> -}
> -
>   static const struct of_device_id ps8640_match[] = {
>   	{ .compatible = "parade,ps8640" },
>   	{ }
> @@ -702,7 +721,6 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
>   
>   static struct i2c_driver ps8640_driver = {
>   	.probe_new = ps8640_probe,
> -	.remove = ps8640_remove,
>   	.driver = {
>   		.name = "ps8640",
>   		.of_match_table = ps8640_match,


-- 
With best wishes
Dmitry

WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Douglas Anderson <dianders@chromium.org>,
	dri-devel@lists.freedesktop.org
Cc: Hsin-Yi Wang <hsinyi@chromium.org>,
	Abhinav Kumar <quic_abhinavk@quicinc.com>,
	Philip Chen <philipchen@chromium.org>,
	Sankeerth Billakanti <quic_sbillaka@quicinc.com>,
	Robert Foss <robert.foss@linaro.org>,
	freedreno@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
	Stephen Boyd <swboyd@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: Re: [PATCH v3 4/4] drm/bridge: parade-ps8640: Handle DP AUX more properly
Date: Wed, 1 Jun 2022 23:46:54 +0300	[thread overview]
Message-ID: <f415aa0c-edf8-94de-bde8-faf151f5f530@linaro.org> (raw)
In-Reply-To: <20220510122726.v3.4.Ia6324ebc848cd40b4dbd3ad3289a7ffb5c197779@changeid>

On 10/05/2022 22:29, Douglas Anderson wrote:
> 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>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
> 
> Changes in v3:
> - Adapt to v3 changes in aux bus.
> - Use devm_drm_bridge_add() to simplify.
> 
> Changes in v2:
> - Rewrote atop new method introduced by patch #1.
> 
>   drivers/gpu/drm/bridge/parade-ps8640.c | 74 ++++++++++++++++----------
>   1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index e2467e58b5b7..ff4227f6d800 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -537,7 +537,7 @@ 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;
> @@ -576,13 +576,40 @@ static int ps8640_bridge_host_attach(struct device *dev, struct ps8640 *ps_bridg
>   	dsi->format = MIPI_DSI_FMT_RGB888;
>   	dsi->lanes = NUM_MIPI_LANES;
>   
> -	return 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 it called this directly (AKA the
> +	 * panel isn't under the "aux-bus" node). 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);
> +
> +	ret = devm_drm_bridge_add(dev, &ps_bridge->bridge);
> +	if (ret)
> +		return ret;
> +
> +	return devm_mipi_dsi_attach(dev, ps_bridge->dsi);
>   }
>   
>   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;
> @@ -623,6 +650,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);
> @@ -665,35 +700,19 @@ static int ps8640_probe(struct i2c_client *client)
>   	if (ret)
>   		return ret;
>   
> -	devm_of_dp_aux_populate_ep_devices(&ps_bridge->aux);
> +	ret = devm_of_dp_aux_populate_bus(&ps_bridge->aux, ps8640_bridge_link_panel);
>   
> -	/* 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;
> +	/*
> +	 * If devm_of_dp_aux_populate_bus() returns -ENODEV then it's up to
> +	 * usa to call ps8640_bridge_link_panel() directly. NOTE: in this case
> +	 * the function is allowed to -EPROBE_DEFER.
> +	 */
> +	if (ret == -ENODEV)
> +		return ps8640_bridge_link_panel(&ps_bridge->aux);
>   
> -err_bridge_remove:
> -	drm_bridge_remove(&ps_bridge->bridge);
>   	return ret;
>   }
>   
> -static int ps8640_remove(struct i2c_client *client)
> -{
> -	struct ps8640 *ps_bridge = i2c_get_clientdata(client);
> -
> -	drm_bridge_remove(&ps_bridge->bridge);
> -
> -	return 0;
> -}
> -
>   static const struct of_device_id ps8640_match[] = {
>   	{ .compatible = "parade,ps8640" },
>   	{ }
> @@ -702,7 +721,6 @@ MODULE_DEVICE_TABLE(of, ps8640_match);
>   
>   static struct i2c_driver ps8640_driver = {
>   	.probe_new = ps8640_probe,
> -	.remove = ps8640_remove,
>   	.driver = {
>   		.name = "ps8640",
>   		.of_match_table = ps8640_match,


-- 
With best wishes
Dmitry

  reply	other threads:[~2022-06-01 20:46 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-10 19:29 [PATCH v3 0/4] drm/dp: Make DP AUX bus usage easier; use it on ps8640 Douglas Anderson
2022-05-10 19:29 ` Douglas Anderson
2022-05-10 19:29 ` [PATCH v3 1/4] drm/dp: Export symbol / kerneldoc fixes for DP AUX bus Douglas Anderson
2022-05-10 19:29   ` Douglas Anderson
2022-05-11  0:20   ` Dmitry Baryshkov
2022-05-11  0:20     ` Dmitry Baryshkov
2022-05-20 20:27   ` Doug Anderson
2022-05-20 20:27     ` Doug Anderson
2022-06-01 20:29   ` Dmitry Baryshkov
2022-06-01 20:29     ` Dmitry Baryshkov
2022-05-10 19:29 ` [PATCH v3 2/4] drm/dp: Add callbacks to make using DP AUX bus properly easier Douglas Anderson
2022-05-10 19:29   ` Douglas Anderson
2022-06-01 20:35   ` Dmitry Baryshkov
2022-06-01 20:35     ` Dmitry Baryshkov
2022-05-10 19:29 ` [PATCH v3 3/4] drm/bridge: Add devm_drm_bridge_add() Douglas Anderson
2022-05-10 19:29   ` Douglas Anderson
2022-05-11  0:22   ` Dmitry Baryshkov
2022-05-11  0:22     ` Dmitry Baryshkov
2022-05-20 20:28     ` Doug Anderson
2022-05-20 20:28       ` Doug Anderson
2022-05-21  9:17   ` Maxime Ripard
2022-05-21  9:17     ` Maxime Ripard
2022-05-23 17:00     ` Doug Anderson
2022-05-23 17:00       ` Doug Anderson
2022-05-31 21:06       ` Doug Anderson
2022-05-31 21:06         ` Doug Anderson
2022-06-03  8:21         ` Maxime Ripard
2022-06-03  8:21           ` Maxime Ripard
2022-06-03 10:19           ` Dmitry Baryshkov
2022-06-03 10:19             ` Dmitry Baryshkov
2022-06-03 13:52             ` Doug Anderson
2022-06-03 13:52               ` Doug Anderson
2022-06-03 14:16               ` Maxime Ripard
2022-06-03 14:16                 ` Maxime Ripard
2022-06-03 14:14             ` Maxime Ripard
2022-06-03 14:14               ` Maxime Ripard
2022-06-03 14:56               ` Doug Anderson
2022-06-03 14:56                 ` Doug Anderson
2022-06-09 13:30                 ` Maxime Ripard
2022-06-09 13:30                   ` Maxime Ripard
2022-06-08 15:50     ` Daniel Vetter
2022-06-08 15:50       ` Daniel Vetter
2022-05-10 19:29 ` [PATCH v3 4/4] drm/bridge: parade-ps8640: Handle DP AUX more properly Douglas Anderson
2022-05-10 19:29   ` Douglas Anderson
2022-06-01 20:46   ` Dmitry Baryshkov [this message]
2022-06-01 20:46     ` Dmitry Baryshkov
2022-05-31 21:03 ` [PATCH v3 0/4] drm/dp: Make DP AUX bus usage easier; use it on ps8640 Doug Anderson
2022-05-31 21:03   ` Doug Anderson
2022-06-02 22:18 ` Doug Anderson
2022-06-02 22:18   ` Doug 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=f415aa0c-edf8-94de-bde8-faf151f5f530@linaro.org \
    --to=dmitry.baryshkov@linaro.org \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=airlied@linux.ie \
    --cc=andrzej.hajda@intel.com \
    --cc=dianders@chromium.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: link
Be 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.