* [PATCH 0/3] drm-panel: Don't make failures quite so fatal
@ 2024-03-25 21:56 Douglas Anderson
2024-03-25 21:56 ` [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings Douglas Anderson
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Douglas Anderson @ 2024-03-25 21:56 UTC (permalink / raw
To: dri-devel
Cc: Pin-yen Lin, Prahlad Kilambi, Hsin-Yi Wang, Douglas Anderson,
Daniel Vetter, David Airlie, Jessica Zhang, Maarten Lankhorst,
Maxime Ripard, Neil Armstrong, Sam Ravnborg, Thomas Zimmermann,
linux-kernel
This patch series is born out of the observation that after several
Chromebooks transitioned over to the generic "edp-panel" compatible
string that we received a number of in-the-field reports of the
primary graphics device for the Chromebook not coming up.
The current belief is that these Chromebooks are actually suffering
from a true hardware failure and the panel is either fully
disconnected or it has some type of intermittent connection. While we
can't solve that problem, digging showed that we actually dealt with
this situation better _before_ switching to the generic "edp-panel"
compatible string.
Before switching to "edp-panel", devices using eDP would finish their
probe and would actually not show any failure until you tried to turn
the panel on. That was a _good_ thing. The component model used by
many DRM devices means that if the panel doesn't finish probing that
the rest of the DRM device doesn't probe. In turn, that means that any
other display adapters (like ones that would allow hooking up an
external display) don't probe. The end result was that a device with a
broken panel that could have continued to be a useful computer by
hooking up an external display became e-waste.
I won't say that this series is the most elegant/wonderful thing in
the world. Ideally we could fail the probe of the panel and still use
the external display. That's a pretty serious re-design, though. DRM
devices work like they do with the component model because of some of
their inherent complexities.
Douglas Anderson (3):
drm/panel-edp: Abstract out function to set conservative timings
drm/panel-edp: If we fail to powerup/get EDID, use conservative
timings
drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel
probe
drivers/gpu/drm/panel/panel-edp.c | 60 +++++++++++--------
.../gpu/drm/panel/panel-samsung-atna33xc20.c | 9 ++-
2 files changed, 41 insertions(+), 28 deletions(-)
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings
2024-03-25 21:56 [PATCH 0/3] drm-panel: Don't make failures quite so fatal Douglas Anderson
@ 2024-03-25 21:56 ` Douglas Anderson
2024-03-25 23:51 ` Hsin-Yi Wang
2024-03-25 21:56 ` [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use " Douglas Anderson
2024-03-25 21:56 ` [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe Douglas Anderson
2 siblings, 1 reply; 10+ messages in thread
From: Douglas Anderson @ 2024-03-25 21:56 UTC (permalink / raw
To: dri-devel
Cc: Pin-yen Lin, Prahlad Kilambi, Hsin-Yi Wang, Douglas Anderson,
Daniel Vetter, David Airlie, Jessica Zhang, Maarten Lankhorst,
Maxime Ripard, Neil Armstrong, Sam Ravnborg, Thomas Zimmermann,
linux-kernel
If we're using the generic "edp-panel" compatible string and we fail
to detect an eDP panel then we fall back to conservative timings for
powering up and powering down the panel. Abstract out the function for
setting these timings so it can be used in future patches.
No functional change expected--just code movement.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/gpu/drm/panel/panel-edp.c | 40 +++++++++++++++----------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index c4f851200aa2..8a19fea90ddf 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -760,6 +760,25 @@ static void panel_edp_parse_panel_timing_node(struct device *dev,
static const struct edp_panel_entry *find_edp_panel(u32 panel_id, const struct drm_edid *edid);
+static void panel_edp_set_conservative_timings(struct panel_edp *panel, struct panel_desc *desc)
+{
+ /*
+ * It's highly likely that the panel will work if we use very
+ * conservative timings, so let's do that.
+ *
+ * Nearly all panels have a "unprepare" delay of 500 ms though
+ * there are a few with 1000. Let's stick 2000 in just to be
+ * super conservative.
+ *
+ * An "enable" delay of 80 ms seems the most common, but we'll
+ * throw in 200 ms to be safe.
+ */
+ desc->delay.unprepare = 2000;
+ desc->delay.enable = 200;
+
+ panel->detected_panel = ERR_PTR(-EINVAL);
+}
+
static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
{
struct panel_desc *desc;
@@ -816,26 +835,7 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
dev_warn(dev,
"Unknown panel %s %#06x, using conservative timings\n",
vend, product_id);
-
- /*
- * It's highly likely that the panel will work if we use very
- * conservative timings, so let's do that. We already know that
- * the HPD-related delays must have worked since we got this
- * far, so we really just need the "unprepare" / "enable"
- * delays. We don't need "prepare_to_enable" since that
- * overlaps the "enable" delay anyway.
- *
- * Nearly all panels have a "unprepare" delay of 500 ms though
- * there are a few with 1000. Let's stick 2000 in just to be
- * super conservative.
- *
- * An "enable" delay of 80 ms seems the most common, but we'll
- * throw in 200 ms to be safe.
- */
- desc->delay.unprepare = 2000;
- desc->delay.enable = 200;
-
- panel->detected_panel = ERR_PTR(-EINVAL);
+ panel_edp_set_conservative_timings(panel, desc);
} else {
dev_info(dev, "Detected %s %s (%#06x)\n",
vend, panel->detected_panel->ident.name, product_id);
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use conservative timings
2024-03-25 21:56 [PATCH 0/3] drm-panel: Don't make failures quite so fatal Douglas Anderson
2024-03-25 21:56 ` [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings Douglas Anderson
@ 2024-03-25 21:56 ` Douglas Anderson
2024-03-26 0:05 ` Hsin-Yi Wang
2024-03-25 21:56 ` [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe Douglas Anderson
2 siblings, 1 reply; 10+ messages in thread
From: Douglas Anderson @ 2024-03-25 21:56 UTC (permalink / raw
To: dri-devel
Cc: Pin-yen Lin, Prahlad Kilambi, Hsin-Yi Wang, Douglas Anderson,
Daniel Vetter, David Airlie, Jessica Zhang, Maarten Lankhorst,
Maxime Ripard, Neil Armstrong, Sam Ravnborg, Thomas Zimmermann,
linux-kernel
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>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe
2024-03-25 21:56 [PATCH 0/3] drm-panel: Don't make failures quite so fatal Douglas Anderson
2024-03-25 21:56 ` [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings Douglas Anderson
2024-03-25 21:56 ` [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use " Douglas Anderson
@ 2024-03-25 21:56 ` Douglas Anderson
2024-03-26 0:07 ` Hsin-Yi Wang
2 siblings, 1 reply; 10+ messages in thread
From: Douglas Anderson @ 2024-03-25 21:56 UTC (permalink / raw
To: dri-devel
Cc: Pin-yen Lin, Prahlad Kilambi, Hsin-Yi Wang, Douglas Anderson,
Daniel Vetter, David Airlie, Jessica Zhang, Maarten Lankhorst,
Maxime Ripard, Neil Armstrong, Sam Ravnborg, Thomas Zimmermann,
linux-kernel
If we're using the AUX channel for eDP backlight and it fails to probe
for some reason, let's _not_ fail the panel probe.
At least one case where we could fail to init the backlight is because
of a dead or physically missing panel. As talked about in detail in
the earlier patch in this series, ("drm/panel-edp: If we fail to
powerup/get EDID, use conservative timings"), this can cause the
entire system's display pipeline to fail to come up and that's
non-ideal.
If we fail to init the backlight for some transitory reason, we should
dig in and see if there's a way to fix this (perhaps retries?). Even
in that case, though, having a panel whose backlight is stuck at 100%
(the default, at least in the panel Samsung ATNA33XC20 I tested) is
better than having no panel at all.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
If needed, I could split this into two patches: one for each of the
two panels that use drm_panel_dp_aux_backlight(). Since they both go
through drm-misc, though, it doesn't feel worth it.
drivers/gpu/drm/panel/panel-edp.c | 8 +++++++-
drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 9 +++++++--
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 607cdd6feda9..0bf66d9dd5b8 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -944,8 +944,14 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
err = drm_panel_dp_aux_backlight(&panel->base, panel->aux);
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
+
+ /*
+ * Warn if we get an error, but don't consider it fatal. Having
+ * a panel where we can't control the backlight is better than
+ * no panel.
+ */
if (err)
- goto err_finished_pm_runtime;
+ dev_warn(dev, "failed to register dp aux backlight: %d\n", err);
}
drm_panel_add(&panel->base);
diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
index 9c336c71562b..6828a4f24d14 100644
--- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
+++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
@@ -328,9 +328,14 @@ static int atana33xc20_probe(struct dp_aux_ep_device *aux_ep)
ret = drm_panel_dp_aux_backlight(&panel->base, aux_ep->aux);
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
+
+ /*
+ * Warn if we get an error, but don't consider it fatal. Having
+ * a panel where we can't control the backlight is better than
+ * no panel.
+ */
if (ret)
- return dev_err_probe(dev, ret,
- "failed to register dp aux backlight\n");
+ dev_warn(dev, "failed to register dp aux backlight: %d\n", ret);
drm_panel_add(&panel->base);
--
2.44.0.396.g6e790dbe36-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings
2024-03-25 21:56 ` [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings Douglas Anderson
@ 2024-03-25 23:51 ` Hsin-Yi Wang
2024-04-08 6:54 ` Doug Anderson
0 siblings, 1 reply; 10+ messages in thread
From: Hsin-Yi Wang @ 2024-03-25 23:51 UTC (permalink / raw
To: Douglas Anderson
Cc: dri-devel, Pin-yen Lin, Prahlad Kilambi, Daniel Vetter,
David Airlie, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Neil Armstrong, Sam Ravnborg, Thomas Zimmermann, linux-kernel
On Mon, Mar 25, 2024 at 2:56 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> If we're using the generic "edp-panel" compatible string and we fail
> to detect an eDP panel then we fall back to conservative timings for
> powering up and powering down the panel. Abstract out the function for
> setting these timings so it can be used in future patches.
>
> No functional change expected--just code movement.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>
> drivers/gpu/drm/panel/panel-edp.c | 40 +++++++++++++++----------------
> 1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index c4f851200aa2..8a19fea90ddf 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -760,6 +760,25 @@ static void panel_edp_parse_panel_timing_node(struct device *dev,
>
> static const struct edp_panel_entry *find_edp_panel(u32 panel_id, const struct drm_edid *edid);
>
> +static void panel_edp_set_conservative_timings(struct panel_edp *panel, struct panel_desc *desc)
> +{
> + /*
> + * It's highly likely that the panel will work if we use very
> + * conservative timings, so let's do that.
> + *
> + * Nearly all panels have a "unprepare" delay of 500 ms though
> + * there are a few with 1000. Let's stick 2000 in just to be
> + * super conservative.
> + *
> + * An "enable" delay of 80 ms seems the most common, but we'll
> + * throw in 200 ms to be safe.
> + */
> + desc->delay.unprepare = 2000;
> + desc->delay.enable = 200;
> +
> + panel->detected_panel = ERR_PTR(-EINVAL);
> +}
> +
> static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> {
> struct panel_desc *desc;
> @@ -816,26 +835,7 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
> dev_warn(dev,
> "Unknown panel %s %#06x, using conservative timings\n",
> vend, product_id);
> -
> - /*
> - * It's highly likely that the panel will work if we use very
> - * conservative timings, so let's do that. We already know that
> - * the HPD-related delays must have worked since we got this
> - * far, so we really just need the "unprepare" / "enable"
> - * delays. We don't need "prepare_to_enable" since that
> - * overlaps the "enable" delay anyway.
> - *
> - * Nearly all panels have a "unprepare" delay of 500 ms though
> - * there are a few with 1000. Let's stick 2000 in just to be
> - * super conservative.
> - *
> - * An "enable" delay of 80 ms seems the most common, but we'll
> - * throw in 200 ms to be safe.
> - */
> - desc->delay.unprepare = 2000;
> - desc->delay.enable = 200;
> -
> - panel->detected_panel = ERR_PTR(-EINVAL);
> + panel_edp_set_conservative_timings(panel, desc);
> } else {
> dev_info(dev, "Detected %s %s (%#06x)\n",
> vend, panel->detected_panel->ident.name, product_id);
> --
> 2.44.0.396.g6e790dbe36-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use conservative timings
2024-03-25 21:56 ` [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use " Douglas Anderson
@ 2024-03-26 0:05 ` Hsin-Yi Wang
2024-04-08 6:55 ` Doug Anderson
0 siblings, 1 reply; 10+ messages in thread
From: Hsin-Yi Wang @ 2024-03-26 0:05 UTC (permalink / raw
To: Douglas Anderson
Cc: dri-devel, Pin-yen Lin, Prahlad Kilambi, Daniel Vetter,
David Airlie, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Neil Armstrong, Sam Ravnborg, Thomas Zimmermann, linux-kernel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe
2024-03-25 21:56 ` [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe Douglas Anderson
@ 2024-03-26 0:07 ` Hsin-Yi Wang
2024-04-08 6:55 ` Doug Anderson
0 siblings, 1 reply; 10+ messages in thread
From: Hsin-Yi Wang @ 2024-03-26 0:07 UTC (permalink / raw
To: Douglas Anderson
Cc: dri-devel, Pin-yen Lin, Prahlad Kilambi, Daniel Vetter,
David Airlie, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Neil Armstrong, Sam Ravnborg, Thomas Zimmermann, linux-kernel
On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> If we're using the AUX channel for eDP backlight and it fails to probe
> for some reason, let's _not_ fail the panel probe.
>
> At least one case where we could fail to init the backlight is because
> of a dead or physically missing panel. As talked about in detail in
> the earlier patch in this series, ("drm/panel-edp: If we fail to
> powerup/get EDID, use conservative timings"), this can cause the
> entire system's display pipeline to fail to come up and that's
> non-ideal.
>
> If we fail to init the backlight for some transitory reason, we should
> dig in and see if there's a way to fix this (perhaps retries?). Even
> in that case, though, having a panel whose backlight is stuck at 100%
> (the default, at least in the panel Samsung ATNA33XC20 I tested) is
> better than having no panel at all.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> If needed, I could split this into two patches: one for each of the
> two panels that use drm_panel_dp_aux_backlight(). Since they both go
> through drm-misc, though, it doesn't feel worth it.
>
> drivers/gpu/drm/panel/panel-edp.c | 8 +++++++-
> drivers/gpu/drm/panel/panel-samsung-atna33xc20.c | 9 +++++++--
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 607cdd6feda9..0bf66d9dd5b8 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -944,8 +944,14 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
> err = drm_panel_dp_aux_backlight(&panel->base, panel->aux);
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> +
> + /*
> + * Warn if we get an error, but don't consider it fatal. Having
> + * a panel where we can't control the backlight is better than
> + * no panel.
> + */
> if (err)
> - goto err_finished_pm_runtime;
> + dev_warn(dev, "failed to register dp aux backlight: %d\n", err);
> }
>
> drm_panel_add(&panel->base);
> diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> index 9c336c71562b..6828a4f24d14 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> @@ -328,9 +328,14 @@ static int atana33xc20_probe(struct dp_aux_ep_device *aux_ep)
> ret = drm_panel_dp_aux_backlight(&panel->base, aux_ep->aux);
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> +
> + /*
> + * Warn if we get an error, but don't consider it fatal. Having
> + * a panel where we can't control the backlight is better than
> + * no panel.
> + */
> if (ret)
> - return dev_err_probe(dev, ret,
> - "failed to register dp aux backlight\n");
> + dev_warn(dev, "failed to register dp aux backlight: %d\n", ret);
>
> drm_panel_add(&panel->base);
>
> --
> 2.44.0.396.g6e790dbe36-goog
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings
2024-03-25 23:51 ` Hsin-Yi Wang
@ 2024-04-08 6:54 ` Doug Anderson
0 siblings, 0 replies; 10+ messages in thread
From: Doug Anderson @ 2024-04-08 6:54 UTC (permalink / raw
To: Hsin-Yi Wang
Cc: dri-devel, Pin-yen Lin, Prahlad Kilambi, Daniel Vetter,
David Airlie, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Neil Armstrong, Sam Ravnborg, Thomas Zimmermann, linux-kernel
Hi,
On Mon, Mar 25, 2024 at 4:52 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Mon, Mar 25, 2024 at 2:56 PM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > If we're using the generic "edp-panel" compatible string and we fail
> > to detect an eDP panel then we fall back to conservative timings for
> > powering up and powering down the panel. Abstract out the function for
> > setting these timings so it can be used in future patches.
> >
> > No functional change expected--just code movement.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Hsin-Yi Wang <hsinyi@chromium.org>
Pushed to drm-misc-next:
2cbee8ae55f5 drm/panel-edp: Abstract out function to set conservative timings
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use conservative timings
2024-03-26 0:05 ` Hsin-Yi Wang
@ 2024-04-08 6:55 ` Doug Anderson
0 siblings, 0 replies; 10+ messages in thread
From: Doug Anderson @ 2024-04-08 6:55 UTC (permalink / raw
To: Hsin-Yi Wang
Cc: dri-devel, Pin-yen Lin, Prahlad Kilambi, Daniel Vetter,
David Airlie, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Neil Armstrong, Sam Ravnborg, Thomas Zimmermann, linux-kernel
Hi,
On Mon, Mar 25, 2024 at 5:05 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> 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>
Pushed to drm-misc-next:
ce0ff22388ab drm/panel-edp: If we fail to powerup/get EDID, use
conservative timings
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe
2024-03-26 0:07 ` Hsin-Yi Wang
@ 2024-04-08 6:55 ` Doug Anderson
0 siblings, 0 replies; 10+ messages in thread
From: Doug Anderson @ 2024-04-08 6:55 UTC (permalink / raw
To: Hsin-Yi Wang
Cc: dri-devel, Pin-yen Lin, Prahlad Kilambi, Daniel Vetter,
David Airlie, Jessica Zhang, Maarten Lankhorst, Maxime Ripard,
Neil Armstrong, Sam Ravnborg, Thomas Zimmermann, linux-kernel
Hi,
On Mon, Mar 25, 2024 at 5:07 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > If we're using the AUX channel for eDP backlight and it fails to probe
> > for some reason, let's _not_ fail the panel probe.
> >
> > At least one case where we could fail to init the backlight is because
> > of a dead or physically missing panel. As talked about in detail in
> > the earlier patch in this series, ("drm/panel-edp: If we fail to
> > powerup/get EDID, use conservative timings"), this can cause the
> > entire system's display pipeline to fail to come up and that's
> > non-ideal.
> >
> > If we fail to init the backlight for some transitory reason, we should
> > dig in and see if there's a way to fix this (perhaps retries?). Even
> > in that case, though, having a panel whose backlight is stuck at 100%
> > (the default, at least in the panel Samsung ATNA33XC20 I tested) is
> > better than having no panel at all.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Reviewed-by: Hsin-Yi Wang <hsinyi@chromium.org>
Pushed to drm-misc-next:
b48ccb18e642 drm-panel: If drm_panel_dp_aux_backlight() fails, don't
fail panel probe
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-04-08 6:55 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-25 21:56 [PATCH 0/3] drm-panel: Don't make failures quite so fatal Douglas Anderson
2024-03-25 21:56 ` [PATCH 1/3] drm/panel-edp: Abstract out function to set conservative timings Douglas Anderson
2024-03-25 23:51 ` Hsin-Yi Wang
2024-04-08 6:54 ` Doug Anderson
2024-03-25 21:56 ` [PATCH 2/3] drm/panel-edp: If we fail to powerup/get EDID, use " Douglas Anderson
2024-03-26 0:05 ` Hsin-Yi Wang
2024-04-08 6:55 ` Doug Anderson
2024-03-25 21:56 ` [PATCH 3/3] drm-panel: If drm_panel_dp_aux_backlight() fails, don't fail panel probe Douglas Anderson
2024-03-26 0:07 ` Hsin-Yi Wang
2024-04-08 6:55 ` Doug Anderson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).