From patchwork Thu Jul 27 17:16:29 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Anderson X-Patchwork-Id: 13330425 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EBF33C001E0 for ; Thu, 27 Jul 2023 17:18:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232601AbjG0RSr (ORCPT ); Thu, 27 Jul 2023 13:18:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50974 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231602AbjG0RSh (ORCPT ); Thu, 27 Jul 2023 13:18:37 -0400 Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7185D128 for ; Thu, 27 Jul 2023 10:18:35 -0700 (PDT) Received: by mail-pf1-x42e.google.com with SMTP id d2e1a72fcca58-686f38692b3so869294b3a.2 for ; Thu, 27 Jul 2023 10:18:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1690478315; x=1691083115; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=KQZ8Cw06bentZdl+4zG5W3NqtbC6pKPyy9qdL8J5He4=; b=Wtom9MHIMsOHg/Z5VkFflz+bbEfQkzNKAwvEaF2r5kP67rXgwP4lABgDusT6irOTWp i+kJfgTKgHTJio8COvmtKItEZCkajdrmWCUQnGqX8I0XsIE2KGhYFYiXLYUuu3PivQ+e nssRUDSVXyRXCS49j6EwevNZvfsHtD/m1nATk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690478315; x=1691083115; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=KQZ8Cw06bentZdl+4zG5W3NqtbC6pKPyy9qdL8J5He4=; b=G3NCYIo6onyuF8zyZOf1Ro4ZHWQrrKXmpx2KrVlI+756V9E6lCKzj0tbpULdrM+HF/ GBgle3/YnrJBIKSIF+q85nX1fYGQPii18Hpz0De4ZFyITHSSvK3qlPamtX+thic5DLpq WV6eUVtG55z5bcY6VEz1lD37eclSBGw5sj4K6L+WtmrzQQlK7vYrhl8OpPxMghIIhJq7 fZO4+hI+VMRc+mX9hPS2L2HWYS4LNxFDZEcu+PiMpAKO5EYJ0or4xpaYGooJWVzjLpIb qXxDz6Pb4QsNIycm9X+FcnW4Lvi+5F9RsuFBf3aJedjvcpL3Lu4QU0sWK3aPTQ87KIAZ vQxw== X-Gm-Message-State: ABy/qLbmXGl7Q2H8NQQ+LfhZt6hQkDwVWMcKVqnJXqSEQ0Bcioch66GO kJeMJGa5ZOVxLqrC7lFzUuqIzA== X-Google-Smtp-Source: APBJJlGmZC9borubx3T6e0kzMzxMxJUj+Df+XCqWavB4O4v7bS4lCkgxWXjA17w0bSDBXf5fn5KDug== X-Received: by 2002:a05:6a20:6a04:b0:133:21c3:115e with SMTP id p4-20020a056a206a0400b0013321c3115emr7569088pzk.48.1690478314913; Thu, 27 Jul 2023 10:18:34 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:2339:954b:b98f:611a]) by smtp.gmail.com with ESMTPSA id 17-20020aa79111000000b0064f76992905sm1702524pfh.202.2023.07.27.10.18.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 27 Jul 2023 10:18:34 -0700 (PDT) From: Douglas Anderson To: Jiri Kosina , Benjamin Tissoires , Bjorn Andersson , Konrad Dybcio , Rob Herring , Frank Rowand , Krzysztof Kozlowski , Conor Dooley , Neil Armstrong , Sam Ravnborg , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann Cc: linux-arm-msm@vger.kernel.org, yangcong5@huaqin.corp-partner.google.com, devicetree@vger.kernel.org, Daniel Vetter , hsinyi@google.com, Chris Morgan , linux-input@vger.kernel.org, cros-qcom-dts-watchers@chromium.org, Dmitry Torokhov , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Douglas Anderson Subject: [PATCH v4 02/11] drm/panel: Check for already prepared/enabled in drm_panel Date: Thu, 27 Jul 2023 10:16:29 -0700 Message-ID: <20230727101636.v4.2.I59b417d4c29151cc2eff053369ec4822b606f375@changeid> X-Mailer: git-send-email 2.41.0.487.g6d72f3e995-goog In-Reply-To: <20230727171750.633410-1-dianders@chromium.org> References: <20230727171750.633410-1-dianders@chromium.org> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.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. We'll also add an entry to the official DRM todo list to remove the now pointless check from the panels after this patch lands and, eventually, fixup anyone who is triggering the new warning. [1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid Acked-by: Neil Armstrong Reviewed-by: Maxime Ripard Signed-off-by: Douglas Anderson --- This has Neil's Ack and Maxime's review and I could commit it to drm-misc-next, but for now I'm holding off to land with the rest of this series since the second drm patch in my series depends on this one. Once this lands somewhere, I'll take the action item to post patches to delete the now pointless checks in the individual panels. Changes in v4: - Document further cleanup in the official DRM todo list. Documentation/gpu/todo.rst | 24 ++++++++++++++++++ drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++----- include/drm/drm_panel.h | 14 +++++++++++ 3 files changed, 81 insertions(+), 6 deletions(-) diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 68bdafa0284f..e3b272c97758 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -452,6 +452,30 @@ Contact: Thomas Zimmermann Level: Starter +Clean up checks for already prepared/enabled in panels +------------------------------------------------------ + +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. 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 + +In the patch ("drm/panel: Check for already prepared/enabled in drm_panel") +we've moved this check to the core. Now we can most definitely remove the +check from the individual panels and save a pile of code. + +In adition to removing the check from the individual panels, it is believed +that even the core shouldn't need this check and that should be considered +an error if other code ever relies on this check. The check in the core +currently prints a warning whenever something is relying on this check with +dev_warn(). After a little while, we likely want to promote this to a +WARN(1) to help encourage folks not to rely on this behavior. + +Contact: Douglas Anderson + +Level: Starter/Intermediate + Core refactorings ================= 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,