From mboxrd@z Thu Jan 1 00:00:00 1970 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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 30F04C433F5 for ; Wed, 17 Nov 2021 18:24:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0D48661BC1 for ; Wed, 17 Nov 2021 18:24:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239961AbhKQS1u (ORCPT ); Wed, 17 Nov 2021 13:27:50 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55168 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231303AbhKQS1t (ORCPT ); Wed, 17 Nov 2021 13:27:49 -0500 Received: from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com [IPv6:2607:f8b0:4864:20::22b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1FF47C061570 for ; Wed, 17 Nov 2021 10:24:50 -0800 (PST) Received: by mail-oi1-x22b.google.com with SMTP id m6so8332245oim.2 for ; Wed, 17 Nov 2021 10:24:50 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=lQ6/wfGxlq62JVYF2E32Rlz6vPrZMUyQROpg1BnR6yc=; b=iJ4RM1d7x7IyjFcBbZxyPp3mTGbPzfcdiC4ayejMerwTaE6kvxxQNbdNI3IW1UANOm L2bIw3wpFdnpwXCZvzOso5Y2Tw2uj0aczx4iy4lh3dpCRZYjxEJ9kg7rh1JGFJg2ajvc 5fTgGT4YGyt4SaCS/fIbIeHouKL9x3aQ0F26E= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=lQ6/wfGxlq62JVYF2E32Rlz6vPrZMUyQROpg1BnR6yc=; b=BYLYzs9z0dupkmiKZFxVzsyUl6nSnLyniHlEU5+EQ34D8mUNwCuywKlyKbiNXvN+Oz ynngWiLV93yQwMp+q4G3rx/VsMnLxgQnEi8jvwfIRO9mCHqQ+e6NDKffBggAXmIcTFdh puaGzsjy2hk5EbJ3WP+gOLF0CVLL7H0vun3TnroJ4USiWmBqlSj4OeJ58QegqsnCe1Qq 1u34BhkpFAWvrS5H0q1Hvlh63ZypspJ3cVU2BDQEF1jNu89K+wsdOvCBE6uvC1R9lPhw D7Obdci2+dDhzyc0mZmanxqTikyjnV09lZEV03mcVHnKRudiACbzEyPzoFpVw7IflP8T tJWg== X-Gm-Message-State: AOAM5318bPnqaVJhpAFRY9K3b8u+zOKRqdwRCSc20yTBAwOK2IG+wV0f 4bnocptQoYMwyScLV6Rgdt60dw38rOyYKw== X-Google-Smtp-Source: ABdhPJymA4Ev9jw/aMElTqQWz45jitPDvt+sSgBDu9jFgVXfTq+W2HbtjUefDw4Omb4MOjnrgwtySQ== X-Received: by 2002:aca:ac8a:: with SMTP id v132mr1785229oie.44.1637173488679; Wed, 17 Nov 2021 10:24:48 -0800 (PST) Received: from mail-ot1-f45.google.com (mail-ot1-f45.google.com. [209.85.210.45]) by smtp.gmail.com with ESMTPSA id x16sm117730ott.8.2021.11.17.10.24.46 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 17 Nov 2021 10:24:47 -0800 (PST) Received: by mail-ot1-f45.google.com with SMTP id w6-20020a9d77c6000000b0055e804fa524so6258105otl.3 for ; Wed, 17 Nov 2021 10:24:46 -0800 (PST) X-Received: by 2002:a9d:4b19:: with SMTP id q25mr14987933otf.186.1637173485758; Wed, 17 Nov 2021 10:24:45 -0800 (PST) MIME-Version: 1.0 References: <20211103234018.4009771-1-briannorris@chromium.org> <20211103164002.2.Ie6c485320b35b89fd49e15a73f0a68e3bb49eef9@changeid> In-Reply-To: From: Brian Norris Date: Wed, 17 Nov 2021 10:24:34 -0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events To: Doug Anderson Cc: Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Andrzej Hajda , Dmitry Torokhov , linux-kernel@vger.kernel.org, linux-input@vger.kernel.org, David Airlie , linux-rockchip@lists.infradead.org, "Kristian H . Kristensen" , Rob Clark , Rob Clark , Daniel Vetter Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Doug, On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson wrote: > On Wed, Nov 3, 2021 at 4:40 PM Brian Norris wrote: ... > > Leverage a new drm_input_helper library to get easy access to > > likely-relevant input event callbacks. > > So IMO this is a really useful thing and I'm in support of it landing. > It's not much code and it clearly gives a big benefit. However, I > would request a CONFIG option to control this so that if someone > really finds some use case where it isn't needed or if they find a > good way to do this in userspace without latency problems then they > can turn it off. Does that sound reasonable? Sure, I think so. This feature is unfortunately on the borderline of "policy" (which we normally avoid baking into the kernel), so having some control over it is probably a good idea -- e.g., module parameter, CONFIG_*, or both. I suppose that would make sense to be a "self_refresh"-level control, and not a "drm_input_helper"-level control? Because different applications (PSR, GPU boost, etc.) may have different characteristics and reasons for leveraging this or not. > > Inspired-by: Kristian H. Kristensen > > Signed-off-by: Brian Norris > > --- > > This was in part picked up from: > > > > https://lore.kernel.org/all/20180405095000.9756-25-enric.balletbo@collabora.com/ > > [PATCH v6 24/30] drm/rockchip: Disable PSR on input events > > > > with significant rewrites/reworks: > > > > - moved to common drm_input_helper and drm_self_refresh_helper > > implementation > > - track state only through crtc->state->self_refresh_active > > > > Note that I'm relatively unfamiliar with DRM locking expectations, but I > > believe access to drm_crtc->state (which helps us track redundant > > transitions) is OK under the locking provided by > > drm_atomic_get_crtc_state(). > > Yeah, I'm no expert here either. I gave a review a shot anyway since > it's been all quiet, but adult supervision is probably required... Thanks ;) > I can believe that you are safe from corrupting things, but I think > you still have locking problems, don't you? What about this: > > 1. PSR is _not_ active but we're 1 microsecond away from entering PSR > > 2. Input event comes through. > > 3. Start executing drm_self_refresh_transition(false). > > 4. PSR timer expires and starts executing drm_self_refresh_transition(true). > > 5. Input event "wins the race" but sees that PSR is already disabled => noop > > 6. PSR timer gets the lock now. Starts PSR transition. > > Wouldn't it be better to cancel / reschedule any PSR entry as soon as > you see the input event? I did think about that option (calling mod_timer to delay the next PSR entry), but thought it was a bit excessive, at least in terms of calling it a "race" -- the race between steps #5 and #6 are essentially equivalent to the natural (unsolvable) race between #1 and #2 (we can't really read the future about input events). But rereading your explanation and thinking again, I see that you're pointing out less of a "race" in the traditional sense, and more of a missing part of this feature: I think what you're really saying is that input events should not only exit PSR, but they should delay PSR (re)entry for some time. With my current patch, input events only enforce any delay time window if we were already in PSR. I'll try to factor that into the next version. Thanks! Brian