Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] drm/amdgpu: Grab/put runtime PM references in
 atomic_commit_tail()
To:     Lyude Paul <lyude@redhat.com>, amd-gfx@lists.freedesktop.org
Cc:     Alex Deucher <alexander.deucher@amd.com>,
        "David (ChunMing) Zhou" <David1.Zhou@amd.com>,
        David Airlie <airlied@linux.ie>,
        Harry Wentland <harry.wentland@amd.com>,
        Andrey Grodzovsky <andrey.grodzovsky@amd.com>,
        Tony Cheng <Tony.Cheng@amd.com>,
        "Leo (Sunpeng) Li" <sunpeng.li@amd.com>,
        Shirish S <shirish.s@amd.com>, dri-devel@lists.freedesktop.org,
        linux-kernel@vger.kernel.org
References: <20180604193509.29153-1-lyude@redhat.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
Message-ID: <1a4e687c-f5f4-4716-0461-2b549f17fdc6@amd.com>
Date:   Tue, 5 Jun 2018 08:16:35 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <20180604193509.29153-1-lyude@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Received-SPF: None (protection.outlook.com: amd.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2018 06:16:46.2897 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 26e8d48e-54d4-4ece-0f41-08d5caabebde
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR12MB1725
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Am 04.06.2018 um 21:35 schrieb Lyude Paul:
> So, unfortunately I recently made the discovery that in the upstream
> kernel, the only reason that amdgpu is not currently suffering from
> issues with runtime PM putting the GPU into suspend while it's driving
> displays is due to the fact that on most prime systems, we have sound
> devices associated with the GPU that hold their own runtime PM ref for
> the GPU.
>
> What this means however, is that in the event that there isn't any kind
> of sound device active (which can easily be reproduced by building a
> kernel with sound drivers disabled), the GPU will fall asleep even when
> there's displays active. This appears to be in part due to the fact that
> amdgpu has not actually ever relied on it's rpm_idle() function to be
> the only thing keeping it running, and normally grabs it's own power
> references whenever there are displays active (as can be seen with the
> original pre-DC codepath in amdgpu_display_crtc_set_config() in
> amdgpu_display.c). This means it's very likely that this bug was
> introduced during the switch over the DC.
>
> So to fix this, we start grabbing runtime PM references every time we
> enable a previously disabled CRTC in atomic_commit_tail(). This appears
> to be the correct solution, as it matches up with what i915 does in
> i915/intel_runtime_pm.c.
>
> The one sideaffect of this is that we ignore the variable that the
> pre-DC code used to use for tracking when it needed runtime PM refs,
> adev->have_disp_power_ref. This is mainly because there's no way for a
> driver to tell whether or not all of it's CRTCs are enabled or disabled
> when we've begun committing an atomic state, as there may be CRTC
> commits happening in parallel that aren't contained within the atomic
> state being committed. So, it's safer to just get/put a reference for
> each CRTC being enabled or disabled in the new atomic state.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>

The final decision is with Harray, but at least the explanation makes 
perfect sense to me.

Acked-by: Christian König <christian.koenig@amd.com>.

Harry do you want to pick that up and push it into our internal branch 
while Alex is on vacation or should I do that?

Regards,
Christian.

> ---
> As a note, I'm not entirely happy with this fix and I wouldn't be
> surprised if I missed something while looking through amdgpu. So, please
> don't hesistate to suggest a better fix :).
>
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 1dd1142246c2..361b81ef6997 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -46,6 +46,7 @@
>   #include <linux/moduleparam.h>
>   #include <linux/version.h>
>   #include <linux/types.h>
> +#include <linux/pm_runtime.h>
>   
>   #include <drm/drmP.h>
>   #include <drm/drm_atomic.h>
> @@ -4211,6 +4212,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   			if (dm_old_crtc_state->stream)
>   				remove_stream(adev, acrtc, dm_old_crtc_state->stream);
>   
> +			pm_runtime_get_noresume(dev->dev);
> +
>   			acrtc->enabled = true;
>   			acrtc->hw_mode = new_crtc_state->mode;
>   			crtc->hwmode = new_crtc_state->mode;
> @@ -4396,6 +4399,16 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>   		drm_atomic_helper_wait_for_flip_done(dev, state);
>   
>   	drm_atomic_helper_cleanup_planes(dev, state);
> +
> +	/* Finally, drop a runtime PM reference for each newly disabled CRTC,
> +	 * so we can put the GPU into runtime suspend if we're not driving any
> +	 * displays anymore
> +	 */
> +	pm_runtime_mark_last_busy(dev->dev);
> +	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> +		if (old_crtc_state->active && !new_crtc_state->active)
> +			pm_runtime_put_autosuspend(dev->dev);
> +	}
>   }
>   
>