Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751383AbdGNFh7 (ORCPT ); Fri, 14 Jul 2017 01:37:59 -0400 Received: from mail-it0-f65.google.com ([209.85.214.65]:34301 "EHLO mail-it0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750763AbdGNFh4 (ORCPT ); Fri, 14 Jul 2017 01:37:56 -0400 MIME-Version: 1.0 X-Originating-IP: [2a02:168:5640:0:960b:2678:e223:c1c6] In-Reply-To: <1823154.LT2zYSFJ4j@avalon> References: <1823154.LT2zYSFJ4j@avalon> From: Daniel Vetter Date: Fri, 14 Jul 2017 07:37:54 +0200 X-Google-Sender-Auth: WUwJ-uXmuB2vbP219qnlvSj-ar0 Message-ID: Subject: Re: [PATCH 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users To: Laurent Pinchart Cc: Maxime Ripard , "open list:DRM DRIVERS FOR RENESAS" , linux-samsung-soc , dri-devel , Seung-Woo Kim , Chen-Yu Tsai , Linux Kernel Mailing List , "open list:ARM/Rockchip SoC..." , Kyungmin Park , Kukjin Kim , Krzysztof Kozlowski , Daniel Vetter , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2286 Lines: 45 On Fri, Jul 14, 2017 at 1:43 AM, Laurent Pinchart wrote: >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC >> start to CRTC resume") changed the order of the plane commit and CRTC >> enable operations to accommodate the runtime PM requirements. However, >> this introduced corruption in the first displayed frame, as the CRTC is >> now enabled without any plane configured. On Gen2 hardware the first >> frame will be black and likely unnoticed, but on Gen3 hardware we end up >> starting the display before the VSP compositor, which is more >> noticeable. >> >> To fix this, revert the order of the commit operations back, and handle >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable() >> helper operation handlers. > > I believe that the "runtime PM" order is problematic in most drivers. The > problem usually goes unnoticed as most monitors will not even display the > first frame, and I assume many devices will just output it black, but it's an > issue nonetheless. > > Note that my driver hasn't lost the "runtime PM" requirements, so I had to > support them with the "standard" order. The best way I've found was to runtime > resume in the one of .atomic_begin() and .enable() that is run first. Not very > neat, as similar code would be needed in most drivers. I wonder whether it > wouldn't be useful to add resume/suspend helper callbacks for the CRTC. I discussed this with Laurent and explained that "first black frame" is exactly what i915 does. I'd say given special customer requests we don't yet have to bother with this in general ... Wrt adding more hooks for rpm: I honestly don't like that, because then someone else wants to add a hook for clocks, or for the sideband and then we have a horror show of hooks where every driver uses just a very small subset. The point of atomic helpers is to make them modular, if one part doesn't fit, just redo in your driver. Goal should be that shared parts are good for about 90% of drivers/use-cases (maybe even less, there's sooooo many special cases). tldr; I still think the _runtime_pm variant is the recommended way to do this. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch