Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID

[Andrzej Hajda]

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@xxxxxxxxxxxx>
> ---
> 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;
>