| Message ID | 20230523122802.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together | expand |
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>
On Tue, May 23, 2023 at 12:27:56PM -0700, 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 For some reason I haven't dug into in greater detail on my RK3326 and RK3568 boards I hit an issue on suspend/shutdown whereby the unprepare is called multiple times. I suspect it's the dw-mipi-dsi-rockchip.c driver and the dw-mipi-dsi.c drivers both calling the unprepare, but I haven't been able to debug it completely yet. > 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 Regulator unbalanced disables are a bane of my existence. For the panel-newvision-nv3051d.c driver I upstreamed a few releases ago I was told to not include the is_enabled logic and as a result I get a warning on suspend or shutdown when it disables the panel. Unprepare gets called twice and that results in an unbalanced regulator disable. > 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, > -- > 2.40.1.698.g37aff9b760-goog > Thank you for looking into this more. It's one of the last QOL bugs for some devices I'm working on, even if the end result is no big deal (the other QOL bug involves a WARN on probing a rotated panel).
Hi, On Wed, May 24, 2023 at 2:52 AM Neil Armstrong <neil.armstrong@linaro.org> wrote: > > 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> Thanks! For now I'll hold off on landing to see where this series ends up. If the series ends up looking good we'll have to coordinate landing the various bits between the drm and the hid trees and the second drm patch in my series depends on this one> If my series implodes I'll land this one on its own with your Ack. In any case, once this lands somewhere I'll take an AI to cleanup the panels. -Doug
Hi, On Wed, May 24, 2023 at 9:19 AM Chris Morgan <macroalpha82@gmail.com> wrote: > > Thank you for looking into this more. It's one of the last QOL bugs > for some devices I'm working on, even if the end result is no big > deal (the other QOL bug involves a WARN on probing a rotated panel). Glad it'll be helpful! For the WARN on probing a rotated panel I thought there was some progress on that front. Commit e3ea1806e4ad ("drm/bridge: panel: Set orientation on panel_bridge connector") or commit 15b9ca1641f0 ("drm: Config orientation property if panel provides it") didn't help you?
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,
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(-)