Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <8887ded7-d1ab-844c-e3a3-f39f6ef6264a@samsung.com>
Date:   Wed, 31 Mar 2021 13:08:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:88.0)
        Gecko/20100101 Thunderbird/88.0
Subject: Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things
 properly for reading the EDID
Content-Language: en-GB
To:     Douglas Anderson <dianders@chromium.org>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@siol.net>,
        Sam Ravnborg <sam@ravnborg.org>
Cc:     robdclark@chromium.org, dri-devel@lists.freedesktop.org,
        David Airlie <airlied@linux.ie>, linux-arm-msm@vger.kernel.org,
        Steev Klimaszewski <steev@kali.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>,
        Robert Foss <robert.foss@linaro.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        linux-kernel@vger.kernel.org
From:   Andrzej Hajda <a.hajda@samsung.com>
In-Reply-To: <20210329195255.v2.11.Ied721dc895156046ac523baa55a71da241cd09c7@changeid>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <20210330025345.3980086-1-dianders@chromium.org>
        <CGME20210330025435eucas1p12b96966451ee0691f6d5d99b64ac2c8b@eucas1p1.samsung.com>
        <20210329195255.v2.11.Ied721dc895156046ac523baa55a71da241cd09c7@changeid>
Precedence: bulk


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@chromium.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;
>