| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aebba18e-8da7-fba0-a012-5e69bca424f4@linaro.org>
Date: Fri, 27 Jan 2023 19:34:06 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Doug Anderson <dianders@...omium.org>
Cc: dri-devel@...ts.freedesktop.org, Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Daniel Vetter <daniel@...ll.ch>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Dave Stevenson <dave.stevenson@...pberrypi.com>,
Robert Foss <robert.foss@...aro.org>,
Sean Paul <sean@...rly.run>, Jonas Karlman <jonas@...boo.se>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
David Airlie <airlied@...il.com>,
linux-arm-msm@...r.kernel.org, Stephen Boyd <swboyd@...omium.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Vinod Koul <vkoul@...nel.org>, freedreno@...ts.freedesktop.org,
linux-kernel@...r.kernel.org, ye xingchen <ye.xingchen@....com.cn>
Subject: Re: [RFT PATCH 2/2] drm/msm/dsi: Stop unconditionally powering up DSI
hosts at modeset
On 27/01/2023 18:32, Doug Anderson wrote:
> Hi,
>
> On Thu, Jan 26, 2023 at 9:49 PM Dmitry Baryshkov
> <dmitry.baryshkov@...aro.org> wrote:
>>
>> Hi,
>>
>> On Sat, 14 Jan 2023 at 01:56, Douglas Anderson <dianders@...omium.org> wrote:
>>>
>>> In commit 7d8e9a90509f ("drm/msm/dsi: move DSI host powerup to modeset
>>> time"), we moved powering up DSI hosts to modeset time. This wasn't
>>> because it was an elegant design, but there were no better options.
>>>
>>> That commit actually ended up breaking ps8640, and thus was born
>>> commit ec7981e6c614 ("drm/msm/dsi: don't powerup at modeset time for
>>> parade-ps8640") as a temporary hack to un-break ps8640 by moving it to
>>> the old way of doing things. It turns out that ps8640 _really_ doesn't
>>> like its pre_enable() function to be called after
>>> dsi_mgr_bridge_power_on(). Specifically (from experimentation, not
>>> because I have any inside knowledge), it looks like the assertion of
>>> "RST#" in the ps8640 runtime resume handler seems like it's not
>>> allowed to happen after dsi_mgr_bridge_power_on()
>>>
>>> Recently, Dave Stevenson's series landed allowing bridges some control
>>> over pre_enable ordering. The meaty commit for our purposes is commit
>>> 4fb912e5e190 ("drm/bridge: Introduce pre_enable_prev_first to alter
>>> bridge init order"). As documented by that series, if a bridge doesn't
>>> set "pre_enable_prev_first" then we should use the old ordering.
>>>
>>> Now that we have the commit ("drm/bridge: tc358762: Set
>>> pre_enable_prev_first") we can go back to the old ordering, which also
>>> allows us to remove the ps8640 special case.
>>>
>>> One last note is that even without reverting commit 7d8e9a90509f
>>> ("drm/msm/dsi: move DSI host powerup to modeset time"), if you _just_
>>> revert the ps8640 special case and try it out then it doesn't seem to
>>> fail anymore. I spent time bisecting / debugging this and it turns out
>>> to be mostly luck, so we still want this patch to make sure it's
>>> solid. Specifically the reason it sorta works these days is because
>>> we implemented wait_hpd_asserted() in ps8640 now, plus the magic of
>>> "pm_runtime" autosuspend. The fact that we have wait_hpd_asserted()
>>> implemented means that we actually power the bridge chip up just a wee
>>> bit earlier and then the bridge happens to stay on because of
>>> autosuspend and thus ends up powered before dsi_mgr_bridge_power_on().
>>
>> I had a small comment on your patch, but then I was distracted and
>> forgot to send it. See below.
>>
>>>
>>> Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>
>>> Cc: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>> Cc: Abhinav Kumar <quic_abhinavk@...cinc.com>
>>> Signed-off-by: Douglas Anderson <dianders@...omium.org>
>>> ---
>>>
>>> drivers/gpu/drm/msm/dsi/dsi_manager.c | 68 +++++----------------------
>>> 1 file changed, 11 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_manager.c b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> index 3a1417397283..5e6b8d423b96 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_manager.c
>>> @@ -34,32 +34,6 @@ static struct msm_dsi_manager msm_dsim_glb;
>>> #define IS_SYNC_NEEDED() (msm_dsim_glb.is_sync_needed)
>>> #define IS_MASTER_DSI_LINK(id) (msm_dsim_glb.master_dsi_link_id == id)
>>>
>>> -#ifdef CONFIG_OF
>>> -static bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> - struct drm_bridge *next_bridge = drm_bridge_get_next_bridge(bridge);
>>> -
>>> - /*
>>> - * If the next bridge in the chain is the Parade ps8640 bridge chip
>>> - * then don't power on early since it seems to violate the expectations
>>> - * of the firmware that the bridge chip is running.
>>> - *
>>> - * NOTE: this is expected to be a temporary special case. It's expected
>>> - * that we'll eventually have a framework that allows the next level
>>> - * bridge to indicate whether it needs us to power on before it or
>>> - * after it. When that framework is in place then we'll use it and
>>> - * remove this special case.
>>> - */
>>> - return !(next_bridge && next_bridge->of_node &&
>>> - of_device_is_compatible(next_bridge->of_node, "parade,ps8640"));
>>> -}
>>> -#else
>>> -static inline bool dsi_mgr_power_on_early(struct drm_bridge *bridge)
>>> -{
>>> - return true;
>>> -}
>>> -#endif
>>> -
>>> static inline struct msm_dsi *dsi_mgr_get_dsi(int id)
>>> {
>>> return msm_dsim_glb.dsi[id];
>>> @@ -254,7 +228,7 @@ static void msm_dsi_manager_set_split_display(u8 id)
>>> }
>>> }
>>>
>>> -static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>> +static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>>
>> Can you please keep the dsi_mgr_bridge_power_on() as is and just
>> remove the now-legacy dsi_mgr_power_on_early().
>
> By this, I assume you mean keep the function separate but still remove
> the call to it from "modeset" and unconditionally call it from
> dsi_mgr_bridge_pre_enable(), right?
Yes, of course.
>
>
>>> {
>>> int id = dsi_mgr_bridge_get_id(bridge);
>>> struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>> @@ -300,36 +274,10 @@ static void dsi_mgr_bridge_power_on(struct drm_bridge *bridge)
>>> if (is_bonded_dsi && msm_dsi1)
>>> msm_dsi_host_enable_irq(msm_dsi1->host);
>>>
>>> - return;
>>> -
>>> -host1_on_fail:
>>> - msm_dsi_host_power_off(host);
>>> -host_on_fail:
>>> - dsi_mgr_phy_disable(id);
>>> -phy_en_fail:
>>> - return;
>>> -}
>>> -
>>> -static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>>> -{
>>> - int id = dsi_mgr_bridge_get_id(bridge);
>>> - struct msm_dsi *msm_dsi = dsi_mgr_get_dsi(id);
>>> - struct msm_dsi *msm_dsi1 = dsi_mgr_get_dsi(DSI_1);
>>> - struct mipi_dsi_host *host = msm_dsi->host;
>>> - bool is_bonded_dsi = IS_BONDED_DSI();
>>> - int ret;
>>> -
>>> - DBG("id=%d", id);
>>> - if (!msm_dsi_device_connected(msm_dsi))
>>> - return;
>>> -
>>> /* Do nothing with the host if it is slave-DSI in case of bonded DSI */
>>> if (is_bonded_dsi && !IS_MASTER_DSI_LINK(id))
>>> return;
>>>
>>> - if (!dsi_mgr_power_on_early(bridge))
>>> - dsi_mgr_bridge_power_on(bridge);
>>> -
>>> ret = msm_dsi_host_enable(host);
>>> if (ret) {
>>> pr_err("%s: enable host %d failed, %d\n", __func__, id, ret);
>>> @@ -349,7 +297,16 @@ static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge)
>>> host1_en_fail:
>>> msm_dsi_host_disable(host);
>>> host_en_fail:
>>> -
>>> + msm_dsi_host_disable_irq(host);
>>> + if (is_bonded_dsi && msm_dsi1) {
>>> + msm_dsi_host_disable_irq(msm_dsi1->host);
>>> + msm_dsi_host_power_off(msm_dsi1->host);
>>> + }
>>> +host1_on_fail:
>>> + msm_dsi_host_power_off(host);
>>> +host_on_fail:
>>> + dsi_mgr_phy_disable(id);
>>> +phy_en_fail:
>>> return;
>>> }
>>>
>>> @@ -438,9 +395,6 @@ static void dsi_mgr_bridge_mode_set(struct drm_bridge *bridge,
>>> msm_dsi_host_set_display_mode(host, adjusted_mode);
>>> if (is_bonded_dsi && other_dsi)
>>> msm_dsi_host_set_display_mode(other_dsi->host, adjusted_mode);
>>> -
>>> - if (dsi_mgr_power_on_early(bridge))
>>> - dsi_mgr_bridge_power_on(bridge);
>>> }
>>>
>>> static enum drm_mode_status dsi_mgr_bridge_mode_valid(struct drm_bridge *bridge,
>>> --
>>> 2.39.0.314.g84b9a713c41-goog
>>>
>>
>>
>> --
>> With best wishes
>> Dmitry
--
With best wishes
Dmitry
Powered by blists - more mailing lists