| 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: <8887ded7-d1ab-844c-e3a3-f39f6ef6264a@samsung.com>
Date: Wed, 31 Mar 2021 13:08:38 +0200
From: Andrzej Hajda <a.hajda@...sung.com>
To: Douglas Anderson <dianders@...omium.org>,
Neil Armstrong <narmstrong@...libre.com>,
Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...l.net>,
Sam Ravnborg <sam@...nborg.org>
Cc: robdclark@...omium.org, dri-devel@...ts.freedesktop.org,
David Airlie <airlied@...ux.ie>, linux-arm-msm@...r.kernel.org,
Steev Klimaszewski <steev@...i.org>,
Stephen Boyd <swboyd@...omium.org>,
Stanislav Lisovskiy <stanislav.lisovskiy@...el.com>,
Robert Foss <robert.foss@...aro.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things
properly for reading the EDID
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> eDP panels won't provide their EDID unless they're powered on. Let's
> chain a power-on before we read the EDID. This roughly matches what
> was done in 'parade-ps8640.c'.
>
> NOTE: The old code attempted to call pm_runtime_get_sync() before
> reading the EDID. While that was enough to power the bridge chip on,
> it wasn't enough to talk to the panel for two reasons:
> 1. Since we never ran the bridge chip's pre-enable then we never set
> the bit to ignore HPD. This meant the bridge chip didn't even _try_
> to go out on the bus and communicate with the panel.
> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
> if the panel wasn't on.
>
> One thing that's a bit odd here is taking advantage of the EDID that
> the core might have cached for us. See the patch ("drm/edid: Use the
> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
> by:
> - Instantly failing aux transfers if we're not powered.
> - If the first read of the EDID fails we try again after powering.
>
> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> Depending on what people think of the other patches in this series,
> some of this could change.
> - If everyone loves the "runtime PM" in the panel driver then we
> could, in theory, put the pre-enable chaining straight in the "aux
> transfer" function.
> - If everyone hates the EDID cache moving to the core then we can
> avoid some of the awkward flow of things and keep the EDID cache in
> the sn65dsi86 driver.
I wonder if this shouldn't be solved in the core - ie caller of
get_modes callback should be responsible for powering up the pipeline,
otherwise we need to repeat this stuff in every bridge/panel driver.
Any thoughts?
Regards
Andrzej
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 +++++++++++++++++++++++++--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c0398daaa4a6..673c9f1c2d8e 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -128,6 +128,7 @@
> * @dp_lanes: Count of dp_lanes we're using.
> * @ln_assign: Value to program to the LN_ASSIGN register.
> * @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
> + * @pre_enabled: If true then pre_enable() has run.
> *
> * @gchip: If we expose our GPIOs, this is used.
> * @gchip_output: A cache of whether we've set GPIOs to output. This
> @@ -155,6 +156,7 @@ struct ti_sn_bridge {
> int dp_lanes;
> u8 ln_assign;
> u8 ln_polrs;
> + bool pre_enabled;
>
> #if defined(CONFIG_OF_GPIO)
> struct gpio_chip gchip;
> @@ -268,11 +270,33 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> struct edid *edid;
> + bool was_enabled;
> int num = 0;
>
> - pm_runtime_get_sync(pdata->dev);
> + /*
> + * Try to get the EDID first without anything special. There are
> + * three things that could happen with this call.
> + * a) It might just return from its cache.
> + * b) It might try to initiate an AUX transfer which might work.
> + * c) It might try to initiate an AUX transfer which might fail because
> + * we're not powered up.
> + *
> + * If we get a failure we'll assume case c) and try again. NOTE: we
> + * don't want to power up every time because that's slow and we don't
> + * have visibility into whether the data has already been cached.
> + */
> edid = drm_get_edid(connector, &pdata->aux.ddc);
> - pm_runtime_put(pdata->dev);
> + if (!edid) {
> + was_enabled = pdata->pre_enabled;
> +
> + if (!was_enabled)
> + drm_bridge_chain_pre_enable(&pdata->bridge);
> +
> + edid = drm_get_edid(connector, &pdata->aux.ddc);
> +
> + if (!was_enabled)
> + drm_bridge_chain_post_disable(&pdata->bridge);
> + }
>
> if (edid) {
> if (drm_edid_is_valid(edid))
> @@ -852,12 +876,16 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> HPD_DISABLE);
>
> drm_panel_prepare(pdata->panel);
> +
> + pdata->pre_enabled = true;
> }
>
> static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>
> + pdata->pre_enabled = false;
> +
> drm_panel_unprepare(pdata->panel);
>
> clk_disable_unprepare(pdata->refclk);
> @@ -891,6 +919,13 @@ static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
> int ret;
> u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
>
> + /*
> + * Things just won't work if the panel isn't powered. Return failure
> + * right away.
> + */
> + if (!pdata->pre_enabled)
> + return -EIO;
> +
> if (len > SN_AUX_MAX_PAYLOAD_BYTES)
> return -EINVAL;
>
Powered by blists - more mailing lists