| Message ID | 20240325145626.2.Ia7a55a9657b0b6aa4644fd497a0bc595a771258c@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm-panel: Don't make failures quite so fatal | expand |
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 >
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
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,
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(-)