From: Douglas Anderson <dianders@chromium.org> To: Jiri Kosina <jikos@kernel.org>, Benjamin Tissoires <benjamin.tissoires@redhat.com>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Neil Armstrong <neil.armstrong@linaro.org>, Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de> Cc: devicetree@vger.kernel.org, cros-qcom-dts-watchers@chromium.org, linux-arm-msm@vger.kernel.org, yangcong5@huaqin.corp-partner.google.com, Dmitry Torokhov <dmitry.torokhov@gmail.com>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Douglas Anderson <dianders@chromium.org>, linux-input@vger.kernel.org, hsinyi@google.com Subject: [PATCH 2/9] drm/panel: Check for already prepared/enabled in drm_panel Date: Tue, 23 May 2023 12:27:56 -0700 [thread overview] Message-ID: <20230523122802.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid> (raw) In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org> 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, -- 2.40.1.698.g37aff9b760-goog
WARNING: multiple messages have this Message-ID (diff)
From: Douglas Anderson <dianders@chromium.org> To: Jiri Kosina <jikos@kernel.org>, Benjamin Tissoires <benjamin.tissoires@redhat.com>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Neil Armstrong <neil.armstrong@linaro.org>, Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org, linux-input@vger.kernel.org, Dmitry Torokhov <dmitry.torokhov@gmail.com>, hsinyi@google.com, devicetree@vger.kernel.org, yangcong5@huaqin.corp-partner.google.com, linux-kernel@vger.kernel.org, Daniel Vetter <daniel@ffwll.ch>, linux-arm-msm@vger.kernel.org, cros-qcom-dts-watchers@chromium.org, Douglas Anderson <dianders@chromium.org> Subject: [PATCH 2/9] drm/panel: Check for already prepared/enabled in drm_panel Date: Tue, 23 May 2023 12:27:56 -0700 [thread overview] Message-ID: <20230523122802.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid> (raw) In-Reply-To: <20230523193017.4109557-1-dianders@chromium.org> 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, -- 2.40.1.698.g37aff9b760-goog
next prev parent reply other threads:[~2023-05-23 19:31 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-23 19:27 [PATCH 0/9] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Douglas Anderson 2023-05-23 19:27 ` Douglas Anderson 2023-05-23 19:27 ` [PATCH 1/9] dt-bindings: HID: i2c-hid: Add "panel" property to i2c-hid backed panels Douglas Anderson 2023-05-23 19:27 ` Douglas Anderson 2023-05-30 15:34 ` Krzysztof Kozlowski 2023-05-30 15:34 ` Krzysztof Kozlowski 2023-05-30 16:52 ` Doug Anderson 2023-05-30 16:52 ` Doug Anderson 2023-05-23 19:27 ` Douglas Anderson [this message] 2023-05-23 19:27 ` [PATCH 2/9] drm/panel: Check for already prepared/enabled in drm_panel Douglas Anderson 2023-05-24 9:52 ` Neil Armstrong 2023-05-24 9:52 ` Neil Armstrong 2023-05-24 16:57 ` Doug Anderson 2023-05-24 16:57 ` Doug Anderson 2023-05-24 16:19 ` Chris Morgan 2023-05-24 16:19 ` Chris Morgan 2023-05-24 17:04 ` Doug Anderson 2023-05-24 17:04 ` Doug Anderson 2023-05-23 19:27 ` [PATCH 3/9] drm/panel: Add a way for other devices to follow panel state Douglas Anderson 2023-05-23 19:27 ` Douglas Anderson 2023-05-23 19:27 ` [PATCH 4/9] HID: i2c-hid: Switch to SYSTEM_SLEEP_PM_OPS() Douglas Anderson 2023-05-23 19:27 ` Douglas Anderson 2023-05-23 19:27 ` [PATCH 5/9] HID: i2c-hid: Rearrange probe() to power things up later Douglas Anderson 2023-05-23 19:27 ` Douglas Anderson 2023-05-23 23:27 ` kernel test robot 2023-05-23 23:27 ` kernel test robot 2023-05-24 13:01 ` kernel test robot 2023-05-24 13:01 ` kernel test robot 2023-05-23 19:28 ` [PATCH 6/9] HID: i2c-hid: Make suspend and resume into helper functions Douglas Anderson 2023-05-23 19:28 ` Douglas Anderson 2023-05-23 19:28 ` [PATCH 7/9] HID: i2c-hid: Support being a panel follower Douglas Anderson 2023-05-23 19:28 ` Douglas Anderson 2023-05-24 0:28 ` kernel test robot 2023-05-24 0:28 ` kernel test robot 2023-05-24 15:37 ` kernel test robot 2023-05-24 15:37 ` kernel test robot 2023-05-24 17:29 ` Doug Anderson 2023-05-24 17:29 ` Doug Anderson 2023-05-23 19:28 ` [PATCH 8/9] HID: i2c-hid: Do panel follower work on the system_wq Douglas Anderson 2023-05-23 19:28 ` Douglas Anderson 2023-05-24 1:36 ` kernel test robot 2023-05-24 1:36 ` kernel test robot 2023-05-24 18:56 ` kernel test robot 2023-05-24 18:56 ` kernel test robot 2023-05-23 19:28 ` [PATCH 9/9] arm64: dts: qcom: sc7180: Link trogdor touchscreens to the panels Douglas Anderson 2023-05-23 19:28 ` Douglas Anderson 2023-05-23 21:39 ` [PATCH 0/9] drm/panel and i2c-hid: Allow panels and touchscreens to power sequence together Dmitry Torokhov 2023-05-23 21:39 ` Dmitry Torokhov 2023-05-23 23:52 ` Doug Anderson 2023-05-23 23:52 ` Doug Anderson 2023-05-26 19:29 ` Konrad Dybcio 2023-05-26 19:29 ` Konrad Dybcio
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20230523122802.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid \ --to=dianders@chromium.org \ --cc=andersson@kernel.org \ --cc=benjamin.tissoires@redhat.com \ --cc=conor+dt@kernel.org \ --cc=cros-qcom-dts-watchers@chromium.org \ --cc=devicetree@vger.kernel.org \ --cc=dmitry.torokhov@gmail.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=hsinyi@google.com \ --cc=jikos@kernel.org \ --cc=konrad.dybcio@linaro.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-input@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mripard@kernel.org \ --cc=neil.armstrong@linaro.org \ --cc=robh+dt@kernel.org \ --cc=sam@ravnborg.org \ --cc=tzimmermann@suse.de \ --cc=yangcong5@huaqin.corp-partner.google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.