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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 25 Mar 2024 17:05:13 -0700
From: Hsin-Yi Wang <hsinyi@...omium.org>
To: Douglas Anderson <dianders@...omium.org>
Cc: dri-devel@...ts.freedesktop.org, Pin-yen Lin <treapking@...omium.org>, 
	Prahlad Kilambi <prahladk@...gle.com>, Daniel Vetter <daniel@...ll.ch>, David Airlie <airlied@...il.com>, 
	Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Sam Ravnborg <sam@...nborg.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use
 conservative timings

On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson <dianders@...omiumorg> 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@...omium.org>

Reviewed-by: Hsin-Yi Wang <hsinyi@...omium.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
>