| Message ID | 20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Fix EDID reading on ti-sn65dsi86; solve some chicken-and-egg problems | expand |
On Fri 16 Apr 17:39 CDT 2021, Douglas Anderson wrote: > Historically simple-panel had code to make sure that if prepare() was > called on an already-prepared panel that it was a no-op. Similarly if > unprepare() was called on an already-unprepared panel it was also a > no-op. Essentially it means that these functions always "forced" the > value to be whatever the caller wanted it to be. You can see that the > forcing behavior dates back at least as far as 2014 by looking at > commit 613a633e7a56 ("drm/panel: simple: Add proper definition for > prepare and unprepare"). > > Apparently the code supporting the historical behavior may not be > needed [1] and prepare() / unprepare() are supposed to be > balanced. Let's try removing it and see if anyone breaks! If they do > then we can have a debate about whether we should change that code or > revert this patch. :-) If nobody breaks then we've nicely saved a few > lines of code and some complexity. > > [1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/panel/panel-simple.c | 14 -------------- > 1 file changed, 14 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c > index 5a2953c4ca44..a2c3008af7e5 100644 > --- a/drivers/gpu/drm/panel/panel-simple.c > +++ b/drivers/gpu/drm/panel/panel-simple.c > @@ -176,8 +176,6 @@ struct panel_simple { > bool enabled; > bool no_hpd; > > - bool prepared; > - > ktime_t prepared_time; > ktime_t unprepared_time; > > @@ -355,18 +353,12 @@ static int panel_simple_suspend(struct device *dev) > > static int panel_simple_unprepare(struct drm_panel *panel) > { > - struct panel_simple *p = to_panel_simple(panel); > int ret; > > - /* Unpreparing when already unprepared is a no-op */ > - if (!p->prepared) > - return 0; > - > pm_runtime_mark_last_busy(panel->dev); > ret = pm_runtime_put_autosuspend(panel->dev); > if (ret < 0) > return ret; > - p->prepared = false; > > return 0; > } > @@ -475,18 +467,12 @@ static int panel_simple_prepare(struct drm_panel *panel) > struct panel_simple *p = to_panel_simple(panel); > int ret; > > - /* Preparing when already prepared is a no-op */ > - if (p->prepared) > - return 0; > - > ret = pm_runtime_get_sync(panel->dev); > if (ret < 0) { > pm_runtime_put_autosuspend(panel->dev); > return ret; > } > > - p->prepared = true; > - > return 0; > } > > -- > 2.31.1.368.gbe11c130af-goog >
Hi, On Fri, Apr 16, 2021 at 3:41 PM Douglas Anderson <dianders@chromium.org> wrote: > > Historically simple-panel had code to make sure that if prepare() was > called on an already-prepared panel that it was a no-op. Similarly if > unprepare() was called on an already-unprepared panel it was also a > no-op. Essentially it means that these functions always "forced" the > value to be whatever the caller wanted it to be. You can see that the > forcing behavior dates back at least as far as 2014 by looking at > commit 613a633e7a56 ("drm/panel: simple: Add proper definition for > prepare and unprepare"). > > Apparently the code supporting the historical behavior may not be > needed [1] and prepare() / unprepare() are supposed to be > balanced. Let's try removing it and see if anyone breaks! If they do > then we can have a debate about whether we should change that code or > revert this patch. :-) If nobody breaks then we've nicely saved a few > lines of code and some complexity. > > [1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > (no changes since v1) > > drivers/gpu/drm/panel/panel-simple.c | 14 -------------- > 1 file changed, 14 deletions(-) So it turns out that when looking at panel_simple_remove() I already found a case that's not necessarily refcounting. There I see drm_panel_unprepare() just simply hardcoded, but (as I understand it) there's no reason to believe that the panel has been prepared at remove() time. Yes, I could fix panel_simple_remove() but instead, for now, I think I'm going to drop this patch from the series. If someone wants to pick it up then I certainly won't object, but for now keeping the way things are seems the best way to keep from getting shouted at. -Doug
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 5a2953c4ca44..a2c3008af7e5 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -176,8 +176,6 @@ struct panel_simple { bool enabled; bool no_hpd; - bool prepared; - ktime_t prepared_time; ktime_t unprepared_time; @@ -355,18 +353,12 @@ static int panel_simple_suspend(struct device *dev) static int panel_simple_unprepare(struct drm_panel *panel) { - struct panel_simple *p = to_panel_simple(panel); int ret; - /* Unpreparing when already unprepared is a no-op */ - if (!p->prepared) - return 0; - pm_runtime_mark_last_busy(panel->dev); ret = pm_runtime_put_autosuspend(panel->dev); if (ret < 0) return ret; - p->prepared = false; return 0; } @@ -475,18 +467,12 @@ static int panel_simple_prepare(struct drm_panel *panel) struct panel_simple *p = to_panel_simple(panel); int ret; - /* Preparing when already prepared is a no-op */ - if (p->prepared) - return 0; - ret = pm_runtime_get_sync(panel->dev); if (ret < 0) { pm_runtime_put_autosuspend(panel->dev); return ret; } - p->prepared = true; - return 0; }
Historically simple-panel had code to make sure that if prepare() was called on an already-prepared panel that it was a no-op. Similarly if unprepare() was called on an already-unprepared panel it was also a no-op. Essentially it means that these functions always "forced" the value to be whatever the caller wanted it to be. You can see that the forcing behavior dates back at least as far as 2014 by looking at commit 613a633e7a56 ("drm/panel: simple: Add proper definition for prepare and unprepare"). Apparently the code supporting the historical behavior may not be needed [1] and prepare() / unprepare() are supposed to be balanced. Let's try removing it and see if anyone breaks! If they do then we can have a debate about whether we should change that code or revert this patch. :-) If nobody breaks then we've nicely saved a few lines of code and some complexity. [1] https://lore.kernel.org/r/YHePsQgqOau1V5lD@pendragon.ideasonboard.com Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- (no changes since v1) drivers/gpu/drm/panel/panel-simple.c | 14 -------------- 1 file changed, 14 deletions(-)