Messages in this thread | | | Date | Wed, 24 May 2023 11:52:14 +0200 | From | Neil Armstrong <> | Subject | Re: [PATCH 2/9] drm/panel: Check for already prepared/enabled in drm_panel |
| |
Hi,
On 23/05/2023 21:27, Douglas Anderson wrote: > In a whole pile of panel drivers, we have code to make the > prepare/unprepare/enable/disable callbacks behave as no-ops if they've > already been called. It's silly to have this code duplicated > everywhere. Add it to the core instead so that we can eventually > delete it from all the drivers. Note: to get some idea of the > duplicated code, try: > git grep 'if.*>prepared' -- drivers/gpu/drm/panel > git grep 'if.*>enabled' -- drivers/gpu/drm/panel > > NOTE: arguably, the right thing to do here is actually to skip this > patch and simply remove all the extra checks from the individual > drivers. Perhaps the checks were needed at some point in time in the > past but maybe they no longer are? Certainly as we continue > transitioning over to "panel_bridge" then we expect there to be much > less variety in how these calls are made. When we're called as part of > the bridge chain, things should be pretty simple. In fact, there was > some discussion in the past about these checks [1], including a > discussion about whether the checks were needed and whether the calls > ought to be refcounted. At the time, I decided not to mess with it > because it felt too risky. > > Looking closer at it now, I'm fairly certain that nothing in the > existing codebase is expecting these calls to be refcounted. The only > real question is whether someone is already doing something to ensure > prepare()/unprepare() match and enabled()/disable() match. I would say > that, even if there is something else ensuring that things match, > there's enough complexity that adding an extra bool and an extra > double-check here is a good idea. Let's add a drm_warn() to let people > know that it's considered a minor error to take advantage of > drm_panel's double-checking but we'll still make things work fine. > > [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++----- > include/drm/drm_panel.h | 14 +++++++++++ > 2 files changed, 57 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index f634371c717a..4e1c4e42575b 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -105,11 +105,22 @@ EXPORT_SYMBOL(drm_panel_remove); > */ > int drm_panel_prepare(struct drm_panel *panel) > { > + int ret; > + > if (!panel) > return -EINVAL; > > - if (panel->funcs && panel->funcs->prepare) > - return panel->funcs->prepare(panel); > + if (panel->prepared) { > + dev_warn(panel->dev, "Skipping prepare of already prepared panel\n"); > + return 0; > + } > + > + if (panel->funcs && panel->funcs->prepare) { > + ret = panel->funcs->prepare(panel); > + if (ret < 0) > + return ret; > + } > + panel->prepared = true; > > return 0; > } > @@ -128,11 +139,22 @@ EXPORT_SYMBOL(drm_panel_prepare); > */ > int drm_panel_unprepare(struct drm_panel *panel) > { > + int ret; > + > if (!panel) > return -EINVAL; > > - if (panel->funcs && panel->funcs->unprepare) > - return panel->funcs->unprepare(panel); > + if (!panel->prepared) { > + dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); > + return 0; > + } > + > + if (panel->funcs && panel->funcs->unprepare) { > + ret = panel->funcs->unprepare(panel); > + if (ret < 0) > + return ret; > + } > + panel->prepared = false; > > return 0; > } > @@ -155,11 +177,17 @@ int drm_panel_enable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + if (panel->enabled) { > + dev_warn(panel->dev, "Skipping enable of already enabled panel\n"); > + return 0; > + } > + > if (panel->funcs && panel->funcs->enable) { > ret = panel->funcs->enable(panel); > if (ret < 0) > return ret; > } > + panel->enabled = true; > > ret = backlight_enable(panel->backlight); > if (ret < 0) > @@ -187,13 +215,22 @@ int drm_panel_disable(struct drm_panel *panel) > if (!panel) > return -EINVAL; > > + if (!panel->enabled) { > + dev_warn(panel->dev, "Skipping disable of already disabled panel\n"); > + return 0; > + } > + > ret = backlight_disable(panel->backlight); > if (ret < 0) > DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n", > ret); > > - if (panel->funcs && panel->funcs->disable) > - return panel->funcs->disable(panel); > + if (panel->funcs && panel->funcs->disable) { > + ret = panel->funcs->disable(panel); > + if (ret < 0) > + return ret; > + } > + panel->enabled = false; > > return 0; > } > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 432fab2347eb..c6cf75909389 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -198,6 +198,20 @@ struct drm_panel { > * the panel is powered up. > */ > bool prepare_prev_first; > + > + /** > + * @prepared: > + * > + * If true then the panel has been prepared. > + */ > + bool prepared; > + > + /** > + * @enabled: > + * > + * If true then the panel has been enabled. > + */ > + bool enabled; > }; > > void drm_panel_init(struct drm_panel *panel, struct device *dev,
LGTM and let's cleanup the panel drivers
Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
| |