diff mbox series

drm/panel-edp: use put_sync in unprepare

Message ID 20231220221418.2610185-1-hsinyi@chromium.org
State New
Headers
Series drm/panel-edp: use put_sync in unprepare |

Commit Message

Hsin-Yi Wang Dec. 20, 2023, 10:13 p.m. UTC 
  Some edp panel requires T10 (Delay from end of valid video data transmitted
by the Source device to power-off) less than 500ms. Using autosuspend with
delay set as 1000 violates this requirement.

Use put_sync_suspend in unprepare to meet the spec. For other cases (such
as getting EDID), it still uses autosuspend.

Suggested-by: Douglas Anderson <dianders@chromium.org>
Fixes: 5f04e7ce392d ("drm/panel-edp: Split eDP panels out of panel-simple")
Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
 drivers/gpu/drm/panel/panel-edp.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Doug Anderson Dec. 20, 2023, 10:43 p.m. UTC | #1 
Hi,

On Wed, Dec 20, 2023 at 2:14 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some edp panel requires T10 (Delay from end of valid video data transmitted
> by the Source device to power-off) less than 500ms. Using autosuspend with
> delay set as 1000 violates this requirement.
>
> Use put_sync_suspend in unprepare to meet the spec. For other cases (such
> as getting EDID), it still uses autosuspend.
>
> Suggested-by: Douglas Anderson <dianders@chromium.org>
> Fixes: 5f04e7ce392d ("drm/panel-edp: Split eDP panels out of panel-simple")

Probably instead:

Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid
excessive unprepare / prepare")

...you could send a new version or I could just fix it up when I apply it.


> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
>  drivers/gpu/drm/panel/panel-edp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Yeah, it's really unfortunate but I think we have to do this. It will
add big delays any time we need to turn the panel off and quickly back
on again, but I don't think we can reliably meet T10 without it. Even
turning down the autosuspend delay won't really help since someone
could do something like read the EDID while the delay was happening to
reset the delay. At least we can still use "autosuspend" to avoid
powering off between reading the EDID and powering up the panel since
the EDID grabs runtime_pm itself and still uses autosuspend.

I don't remember this particular problem before and nobody has yelled
about it in the past. ...and the requirement seems crazy, but it
certainly is in the spec sheets so we should be good citizens and
honor it. On the plus side, this means that we will always fully power
cycle the panel whenever we turn video off and that means that if any
other panels out there have weird issues like "samsung-atna33xc20"
this will also fix them since this is the same fix I had to do in that
driver.

In any case:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I'm about to go on vacation, so I won't apply this until January.
Other drm-misc maintainers are free to apply sooner than that if
they're comfortable with it.
Doug Anderson Jan. 8, 2024, 9:34 p.m. UTC | #2 
Hi,

On Wed, Dec 20, 2023 at 2:43 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Dec 20, 2023 at 2:14 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
> >
> > Some edp panel requires T10 (Delay from end of valid video data transmitted
> > by the Source device to power-off) less than 500ms. Using autosuspend with
> > delay set as 1000 violates this requirement.
> >
> > Use put_sync_suspend in unprepare to meet the spec. For other cases (such
> > as getting EDID), it still uses autosuspend.
> >
> > Suggested-by: Douglas Anderson <dianders@chromium.org>
> > Fixes: 5f04e7ce392d ("drm/panel-edp: Split eDP panels out of panel-simple")
>
> Probably instead:
>
> Fixes: 3235b0f20a0a ("drm/panel: panel-simple: Use runtime pm to avoid
> excessive unprepare / prepare")
>
> ...you could send a new version or I could just fix it up when I apply it.
>
>
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> >  drivers/gpu/drm/panel/panel-edp.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
>
> Yeah, it's really unfortunate but I think we have to do this. It will
> add big delays any time we need to turn the panel off and quickly back
> on again, but I don't think we can reliably meet T10 without it. Even
> turning down the autosuspend delay won't really help since someone
> could do something like read the EDID while the delay was happening to
> reset the delay. At least we can still use "autosuspend" to avoid
> powering off between reading the EDID and powering up the panel since
> the EDID grabs runtime_pm itself and still uses autosuspend.
>
> I don't remember this particular problem before and nobody has yelled
> about it in the past. ...and the requirement seems crazy, but it
> certainly is in the spec sheets so we should be good citizens and
> honor it. On the plus side, this means that we will always fully power
> cycle the panel whenever we turn video off and that means that if any
> other panels out there have weird issues like "samsung-atna33xc20"
> this will also fix them since this is the same fix I had to do in that
> driver.
>
> In any case:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> I'm about to go on vacation, so I won't apply this until January.
> Other drm-misc maintainers are free to apply sooner than that if
> they're comfortable with it.

Things were silent and I'm back from vacation, so I've gone ahead and
pushed this to drm-misc-next.

Technically I could have pushed it to drm-misc-fixes, but from my
understanding of the issue it was not causing any actual problems
other than making someone upset who was staring at oscilloscope traces
and comparing them to a spec sheet. Given that this changes timings,
I'd rather have the extra bake time of going through drm-misc-next.

49ddab089611 drm/panel-edp: use put_sync in unprepare


-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index cd05c76868e3..7d556b1bfa82 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -429,8 +429,7 @@  static int panel_edp_unprepare(struct drm_panel *panel)
 	if (!p->prepared)
 		return 0;
 
-	pm_runtime_mark_last_busy(panel->dev);
-	ret = pm_runtime_put_autosuspend(panel->dev);
+	ret = pm_runtime_put_sync_suspend(panel->dev);
 	if (ret < 0)
 		return ret;
 	p->prepared = false;