Re: [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use conservative timings - Hsin-Yi Wang

From: Hsin-Yi Wang <hsinyi@chromium.org>
To: Douglas Anderson <dianders@chromium.org>
Cc: dri-devel@lists.freedesktop.org,
	Pin-yen Lin <treapking@chromium.org>,
	 Prahlad Kilambi <prahladk@google.com>,
	Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>,
	 Jessica Zhang <quic_jesszhan@quicinc.com>,
	 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	 Neil Armstrong <neil.armstrong@linaro.org>,
	Sam Ravnborg <sam@ravnborg.org>,
	 Thomas Zimmermann <tzimmermann@suse.de>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use conservative timings
Date: Mon, 25 Mar 2024 17:05:13 -0700	[thread overview]
Message-ID: <CAJMQK-j0aCtH8KU5UiWuJbbgRLTmM4d6rkxXff1VBPsdXX2WHQ@mail.gmail.com> (raw)
In-Reply-To: <20240325145626.2.Ia7a55a9657b0b6aa4644fd497a0bc595a771258c@changeid>

On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> If at boot we fail to power up the eDP panel (most often happens if
> the eDP panel never asserts HPD to us) or if we are unable to read the
> EDID at bootup to figure out the panel's ID then let's use the
> conservative eDP panel powerup/powerdown timings but _not_ fail the
> probe.
>
> It might seem strange to _not_ fail the probe in this case since we
> were unable to powerup the panel and confirm it's there. However,
> there is a reason to do this. Specifically, if we fail to probe the
> panel then it really throws the whole display pipeline for loop. Most
> DRM subsystems are written so that they wait until all components
> (including the panel) have probed before they set everything up. When
> the panel doesn't come up then this never happens. As a side effect of
> not setting everything up then other display adapters don't get
> initialized. As a practical example, I can see that if I open up a
> sc7180-trogdor based Chromebook that's using the generic "edp-panel"
> and unplug the eDP panel that it causes the _external_ DP monitor not
> to function. This is obviously bad because it means that a device with
> a dead eDP panel becomes e-waste when it could instead still be given
> useful life with an external display.
>
> NOTES:
> - When we fail to probe like this, boot is a bit slow because we try
>   several times to power the panel up. This doesn't feel horrible
>   because it'll eventually work and the retries are known to help
>   bring some panels up.
> - In the case where we hit the condition of failing to power up, the
>   display will likely _never_ have a chance to work again until
>   reboot. Once the panel-edp pm_runtime resume function fails it
>   doesn't ever seem to retry. This is probably for the best given that
>   we don't have any real timing/modes. eDP isn't expected to be
>   "hotplugged" so this makes some sense.
> - It turns out that this makes panel-edp behave more similarly for
>   users of the generic "edp-panel" compatible string and the old fixed
>   panel compatible string. With the old fixed panel compatible string
>   we don't talk to the panel during probe so we'd actually behave much
>   the same way that we'll now behave for the generic "edp-panel".
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Hsin-Yi Wang <hsinyi@chromium.org>

> ---
>
>  drivers/gpu/drm/panel/panel-edp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 8a19fea90ddf..607cdd6feda9 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -808,7 +808,10 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
>         /* Power the panel on so we can read the EDID */
>         ret = pm_runtime_get_sync(dev);
>         if (ret < 0) {
> -               dev_err(dev, "Couldn't power on panel to read EDID: %d\n", ret);
> +               dev_err(dev,
> +                       "Couldn't power on panel to ID it; using conservative timings: %d\n",
> +                       ret);
> +               panel_edp_set_conservative_timings(panel, desc);
>                 goto exit;
>         }
>
> @@ -816,8 +819,8 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
>         if (base_block) {
>                 panel_id = drm_edid_get_panel_id(base_block);
>         } else {
> -               dev_err(dev, "Couldn't identify panel via EDID\n");
> -               ret = -EIO;
> +               dev_err(dev, "Couldn't read EDID for ID; using conservative timings\n");
> +               panel_edp_set_conservative_timings(panel, desc);
>                 goto exit;
>         }
>         drm_edid_decode_panel_id(panel_id, vend, &product_id);
> @@ -844,12 +847,11 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
>                 desc->delay = *panel->detected_panel->delay;
>         }
>
> -       ret = 0;
>  exit:
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_put_autosuspend(dev);
>
> -       return ret;
> +       return 0;
>  }
>
>  static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
> --
> 2.44.0.396.g6e790dbe36-goog
>