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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Mon, Mar 25, 2024 at 2:56 PM Douglas Anderson <dianders@chromiumorg> 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 <[email protected]>
Reviewed-by: Hsin-Yi Wang <[email protected]>
> ---
>
> 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
>
On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson <dianders@chromiumorg> 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 <[email protected]>
Reviewed-by: Hsin-Yi Wang <[email protected]>
> ---
> 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
>
Hi,
On Mon, Mar 25, 2024 at 4:52 PM Hsin-Yi Wang <[email protected]> wrote:
>
> On Mon, Mar 25, 2024 at 2:56 PM Douglas Anderson <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Hsin-Yi Wang <[email protected]>
Pushed to drm-misc-next:
2cbee8ae55f5 drm/panel-edp: Abstract out function to set conservative timings
Hi,
On Mon, Mar 25, 2024 at 5:07 PM Hsin-Yi Wang <[email protected]> wrote:
>
> On Mon, Mar 25, 2024 at 2:57 PM Douglas Anderson <[email protected]> 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 <[email protected]>
>
> Reviewed-by: Hsin-Yi Wang <[email protected]>
Pushed to drm-misc-next:
b48ccb18e642 drm-panel: If drm_panel_dp_aux_backlight() fails, don't
fail panel probe