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 X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46F5FC433ED for ; Tue, 11 May 2021 17:20:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 113BF611F0 for ; Tue, 11 May 2021 17:20:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231645AbhEKRVY (ORCPT ); Tue, 11 May 2021 13:21:24 -0400 Received: from mail-oi1-f178.google.com ([209.85.167.178]:46746 "EHLO mail-oi1-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230315AbhEKRVW (ORCPT ); Tue, 11 May 2021 13:21:22 -0400 Received: by mail-oi1-f178.google.com with SMTP id x15so5937207oic.13 for ; Tue, 11 May 2021 10:20:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Ml2PDWbdLX72C7HXwtPmUq3pca/wWJyrCfkk5O/A24g=; b=iMi0i7UPZ8fML1cpEcm4XdrzX1vPTbKRiVtcZwsB3c9SA8pItDZKvhNO2cFO4IT+WR Ko4jVcwl0lbAa1t0NMNCMYpyCph9qRah149kFbioaPyey0DXgHNXNNhU4cGCpHgq3NrK 9+AZdT02rpvgBr2JByZl8iGmHBGICR2L3eFzOJ9BQLtnY9xXG4tzV/+c2Cag5fFOvPsd tnKK8aX+UGfj26n7HrO52iBykuxkD22+Hu6XgK0/F1Sxm8s5Q/7+c3pY40fAsgOz0564 K9FTJvmaj3Uj+aD/fadLshKYkIwXLreVcNvGWYrWboxoJ3gYNSVUD9gkKMa81vIv6Dkp wbwQ== X-Gm-Message-State: AOAM530/M0bh2iKPX5sbwoPmbcNt3KESSO0iuAOskhldCXCBYCBK7FOB N/sFfE1Pl7eNq7KB3ltRRPtr6gjs5UH0DwIeMaM= X-Google-Smtp-Source: ABdhPJw6Y/Tk17pjnnPB+8o5Dt+IAryRiDMHlqYjWIXo6Drc3oOeAWSvVeX9hn7ewn9X75cHrcNVlh5iG4w+TQUqzr4= X-Received: by 2002:a05:6808:90d:: with SMTP id w13mr8344188oih.71.1620753615904; Tue, 11 May 2021 10:20:15 -0700 (PDT) MIME-Version: 1.0 References: <20210508074118.1621729-1-swboyd@chromium.org> In-Reply-To: From: "Rafael J. Wysocki" Date: Tue, 11 May 2021 19:20:04 +0200 Message-ID: Subject: Re: [PATCH] component: Move host device to end of device lists on binding To: Stephen Boyd Cc: "Rafael J. Wysocki" , Daniel Vetter , Greg Kroah-Hartman , Linux Kernel Mailing List , Russell King , Rob Clark , dri-devel Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 11, 2021 at 7:00 PM Stephen Boyd wrote: > > Quoting Rafael J. Wysocki (2021-05-11 03:52:06) > > On Mon, May 10, 2021 at 9:08 PM Stephen Boyd wrote: > > > > [cut] > > > > > > > > > > > > > > I will try it, but then I wonder about things like system wide > > > > > suspend/resume too. The drm encoder chain would need to reimplement the > > > > > logic for system wide suspend/resume so that any PM ops attached to the > > > > > msm device run in the correct order. Right now the bridge PM ops will > > > > > run, the i2c bus PM ops will run, and then the msm PM ops will run. > > > > > After this change, the msm PM ops will run, the bridge PM ops will run, > > > > > and then the i2c bus PM ops will run. It feels like that could be a > > > > > problem if we're suspending the DSI encoder while the bridge is still > > > > > active. > > > > > > > > Yup suspend/resume has the exact same problem as shutdown. > > > > > > I think suspend/resume has the exact opposite problem. At least I think > > > the correct order is to suspend the bridge, then the encoder, i.e. DSI, > > > like is happening today. It looks like drm_atomic_helper_shutdown() > > > operates from the top down when we want bottom up? I admit I have no > > > idea what is supposed to happen here. > > > > Why would the system-wide suspend ordering be different from the > > shutdown ordering? > > I don't really know. I'm mostly noting that today the order of suspend > is to suspend the bridge device first and then the aggregate device. If > the suspend of the aggregate device is traversing the devices like > drm_atomic_helper_shutdown() then it would operate on the bridge device > after it has been suspended, like is happening during shutdown. But it > looks like that isn't happening. At least for the msm driver we're > suspending the aggregate device after the bridge, and there are some > weird usages of prepare and complete in there (see msm_pm_prepare() and > msm_pm_complete) which makes me think that it's all working around this > component code. Well, it looks like the "prepare" phase is used sort-of against the rules (because "prepare" is not supposed to make changes to the hardware configuration or at least that is not its role) in order to work around an ordering issue that is present in shutdown which doesn't have a "prepare" phase. > The prepare phase is going to suspend the display pipeline, and then the > bridge device will run its suspend hooks, and then the aggregate driver > will run its suspend hooks. If we had a proper device for the aggregate > device instead of the bind/unbind component hooks we could clean this > up. I'm not sufficiently familiar with the component code to add anything constructive here, but generally speaking it looks like the "natural" dpm_list ordering does not match the order in which the devices in question should be suspended (or shut down for that matter), so indeed it is necessary to reorder dpm_list this way or another. Please also note that it generally may not be sufficient to reorder dpm_list if the devices are suspended and resumed asynchronously during system-wide transitions, because in that case the callbacks of different devices are only started in the dpm_list order, but they may be completed in a different order (and of course they may run in parallel with each other). Shutdown is simpler, because it runs the callback synchronously for all devices IIRC. 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 X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=BAYES_00,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5DEC8C433B4 for ; Tue, 11 May 2021 17:20:19 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 11A1E61376 for ; Tue, 11 May 2021 17:20:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 11A1E61376 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 56A6B6EA93; Tue, 11 May 2021 17:20:18 +0000 (UTC) Received: from mail-oi1-f180.google.com (mail-oi1-f180.google.com [209.85.167.180]) by gabe.freedesktop.org (Postfix) with ESMTPS id 95D946EA93 for ; Tue, 11 May 2021 17:20:16 +0000 (UTC) Received: by mail-oi1-f180.google.com with SMTP id k25so19723832oic.4 for ; Tue, 11 May 2021 10:20:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Ml2PDWbdLX72C7HXwtPmUq3pca/wWJyrCfkk5O/A24g=; b=szV6dALqUSn5WbTzcOqE7dAeV8CRSxkJaxR3Lxq/IGWSVYSqqAU809ZCP8m562rFtR tHsP3bPbnIpi185eaNJglJ+4K6mIAts54qdHI4fyVlGzUDdYq+AFSK5WMJoE3vr3JLuh +ROv4URoPHn211PHCWyaAiybVFWrf4CUooJApoBbDnQNB1bkQNEfYMySuUYD90axbMlp VLbqV74FLc/h/cE7gB4ksIsAeJ+lx0mUhYFte/iyAIN9s99UYfAEkKqKQFPWb8deTdFC KPVTPLYB8PEkohomR7CMvChY+2Uyn/knnjg8HQMnm1sHmIwJDJHeu1EisCf5SJMtciEo FyTw== X-Gm-Message-State: AOAM532LVZprMfBMzZ8J+Ab/jkjlHu2A954hYSqwq/d/l7iX+RBDQuqz bnkdT20lk2DnPAEQm4XYxcB90NdEzuDJcD74uXY= X-Google-Smtp-Source: ABdhPJw6Y/Tk17pjnnPB+8o5Dt+IAryRiDMHlqYjWIXo6Drc3oOeAWSvVeX9hn7ewn9X75cHrcNVlh5iG4w+TQUqzr4= X-Received: by 2002:a05:6808:90d:: with SMTP id w13mr8344188oih.71.1620753615904; Tue, 11 May 2021 10:20:15 -0700 (PDT) MIME-Version: 1.0 References: <20210508074118.1621729-1-swboyd@chromium.org> In-Reply-To: From: "Rafael J. Wysocki" Date: Tue, 11 May 2021 19:20:04 +0200 Message-ID: Subject: Re: [PATCH] component: Move host device to end of device lists on binding To: Stephen Boyd Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Rafael J. Wysocki" , Greg Kroah-Hartman , Linux Kernel Mailing List , dri-devel , Russell King Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Tue, May 11, 2021 at 7:00 PM Stephen Boyd wrote: > > Quoting Rafael J. Wysocki (2021-05-11 03:52:06) > > On Mon, May 10, 2021 at 9:08 PM Stephen Boyd wrote: > > > > [cut] > > > > > > > > > > > > > > I will try it, but then I wonder about things like system wide > > > > > suspend/resume too. The drm encoder chain would need to reimplement the > > > > > logic for system wide suspend/resume so that any PM ops attached to the > > > > > msm device run in the correct order. Right now the bridge PM ops will > > > > > run, the i2c bus PM ops will run, and then the msm PM ops will run. > > > > > After this change, the msm PM ops will run, the bridge PM ops will run, > > > > > and then the i2c bus PM ops will run. It feels like that could be a > > > > > problem if we're suspending the DSI encoder while the bridge is still > > > > > active. > > > > > > > > Yup suspend/resume has the exact same problem as shutdown. > > > > > > I think suspend/resume has the exact opposite problem. At least I think > > > the correct order is to suspend the bridge, then the encoder, i.e. DSI, > > > like is happening today. It looks like drm_atomic_helper_shutdown() > > > operates from the top down when we want bottom up? I admit I have no > > > idea what is supposed to happen here. > > > > Why would the system-wide suspend ordering be different from the > > shutdown ordering? > > I don't really know. I'm mostly noting that today the order of suspend > is to suspend the bridge device first and then the aggregate device. If > the suspend of the aggregate device is traversing the devices like > drm_atomic_helper_shutdown() then it would operate on the bridge device > after it has been suspended, like is happening during shutdown. But it > looks like that isn't happening. At least for the msm driver we're > suspending the aggregate device after the bridge, and there are some > weird usages of prepare and complete in there (see msm_pm_prepare() and > msm_pm_complete) which makes me think that it's all working around this > component code. Well, it looks like the "prepare" phase is used sort-of against the rules (because "prepare" is not supposed to make changes to the hardware configuration or at least that is not its role) in order to work around an ordering issue that is present in shutdown which doesn't have a "prepare" phase. > The prepare phase is going to suspend the display pipeline, and then the > bridge device will run its suspend hooks, and then the aggregate driver > will run its suspend hooks. If we had a proper device for the aggregate > device instead of the bind/unbind component hooks we could clean this > up. I'm not sufficiently familiar with the component code to add anything constructive here, but generally speaking it looks like the "natural" dpm_list ordering does not match the order in which the devices in question should be suspended (or shut down for that matter), so indeed it is necessary to reorder dpm_list this way or another. Please also note that it generally may not be sufficient to reorder dpm_list if the devices are suspended and resumed asynchronously during system-wide transitions, because in that case the callbacks of different devices are only started in the dpm_list order, but they may be completed in a different order (and of course they may run in parallel with each other). Shutdown is simpler, because it runs the callback synchronously for all devices IIRC.