Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752327AbdHBN5e (ORCPT ); Wed, 2 Aug 2017 09:57:34 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:54192 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751940AbdHBN5d (ORCPT ); Wed, 2 Aug 2017 09:57:33 -0400 Date: Wed, 2 Aug 2017 14:57:30 +0100 From: Liviu Dudau To: Laurent Pinchart Cc: Daniel Vetter , Maxime Ripard , 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" Subject: Re: [PATCH v2 1/4] drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users Message-ID: <20170802135730.GR970@e110455-lin.cambridge.arm.com> References: <3036323.BRqfrJhm0Z@avalon> <20170802133206.GQ970@e110455-lin.cambridge.arm.com> <1828962.5J4WLQuEBS@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1828962.5J4WLQuEBS@avalon> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7488 Lines: 169 On Wed, Aug 02, 2017 at 04:49:16PM +0300, Laurent Pinchart wrote: > Hi Liviu, > > On Wednesday 02 Aug 2017 14:32:06 Liviu Dudau wrote: > > On Wed, Aug 02, 2017 at 04:27:27PM +0300, Laurent Pinchart wrote: > > > On Wednesday 02 Aug 2017 13:46:48 Liviu Dudau wrote: > > >> On Wed, Aug 02, 2017 at 01:27:23PM +0200, Daniel Vetter wrote: > > >>> On Wed, Aug 2, 2017 at 1:20 PM, Liviu Dudau wrote: > > >>>> On Thu, Jul 20, 2017 at 03:01:16PM +0200, Maxime Ripard wrote: > > >>>>> +/** > > >>>>> + * drm_atomic_helper_commit_tail_rpm - commit atomic update to > > >>>>> hardware > > >>>>> + * @old_state: new modeset state to be committed > > >>>>> + * > > >>>>> + * This is an alternative implementation for the > > >>>>> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for > > >>>>> drivers > > >>>>> + * that support runtime_pm or need the CRTC to be enabled to > > >>>>> perform a > > >>>>> + * commit. Otherwise, one should use the default implementation > > >>>>> + * drm_atomic_helper_commit_tail(). > > >>>>> + */ > > >>>>> +void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state > > >>>>> *old_state) > > >>>>> +{ > > >>>>> + struct drm_device *dev = old_state->dev; > > >>>>> + > > >>>>> + drm_atomic_helper_commit_modeset_disables(dev, old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_commit_modeset_enables(dev, old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_commit_planes(dev, old_state, > > >>>>> + DRM_PLANE_COMMIT_ACTIVE_ONLY); > > >>>>> + > > >>>>> + drm_atomic_helper_commit_hw_done(old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_wait_for_vblanks(dev, old_state); > > >>>>> + > > >>>>> + drm_atomic_helper_cleanup_planes(dev, old_state); > > >>>>> +} > > >>>>> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm); > > >>>>> + > > >>>> > > >>>> Given that this function is supposed to be used by runtime PM aware > > >>>> drivers and that the hook is called from the DRM core, should there > > >>>> not be some pm_runtime_{get,put} calls wrapping the body of this > > >>>> function? > > >> > > >> Hi Daniel, > > >> > > >>> No, because the drm atomic helpers have no idea which device is > > >>> backing which part of the drm device. Some drivers might on have one > > >>> device for the entire driver, some one device for just the display > > >>> part (which might or might not match drm_device->dev). And many arm > > >>> drivers have a device for each block separately (and you need to call > > >>> rpm_get/put on each). And some something in-between (e.g. core device > > >>> + external encoders). > > >> > > >> Hmm, I understand your point about this function not having to care > > >> about pm_runtime_{get,put}, but I do not agree with your view that if it > > >> wanted to care about it, it wouldn't be able to do the right thing > > >> because it doesn't have the right device. After all, this function is > > >> about handling the updates that this atomic commit is concerned about, > > >> so having the old_state->dev drm_device pointer and its associated device > > >> should be enough. Am I missing something? > > > > > > In embedded system it's quite common for display hardware to be made of > > > multiple IP cores. Depending on the SoC you could have to manage runtime > > > PM at the CRTC level for instance. The DRM core doesn't know about that, > > > and sees a single device only. > > > > > > I've expressed my doubts previously about the need for a RPM version of > > > drm_atomic_helper_commit_tail(), as the resulting order of CRTC > > > enable/disable and plane update operations can lead to corrupt frames > > > when enabling the CRTC. I had a commit tail implementation in the rcar-du > > > driver that was very similar to drm_atomic_helper_commit_tail_rpm(), and > > > had to move back to the standard order to fix the corrupt frame issue. > > > The result isn't as clean as I would like, as power handling is split > > > between the CRTC enable/disable and the atomic begin/flush in a way that > > > is not straightforward. > > > > > > It now occurred to me that a simpler implementation could be possible. > > > I'll have to experiment with it first, but the idea is as follows. > > > > > > 1 Call pm_runtime_get_sync() at the beginning of .commit_tail() and > > > pm_runtime_put() at the end. > > > > > > 2. Use the standard CRTC disable, plane update, CRTC enable order in > > > .commit_tail(). > > > > > > 3. Call pm_runtime_get() in the CRTC .enable() handler and > > > pm_runtime_put() in the CRTC .disable() handler; > > > > Well, that is what mali-dp driver currently does, but according to Daniel > > (and I can see his POV) that is wrong. > > Is it ? I might not have understood his arguments the same way (or possibly > failed to even see them). Are you referring to this comments in this mail > thread, or to something else ? I'm talking about his reply above. My understanding: you can't do pm_runtime_{get,set} in the atomic_commit_tail hook because: 1. you don't know if you have the correct drm_device->dev (or all the relevant devices) to call pm_runtime_get() on. 2. If pm_runtime_get() affects in any way the atomic commit behaviour by being at the top of the commit_tail_rpm() function then you are: a) papering over bugs in one's driver b) going to turn off things at the end of commit_tail_rpm() function when you call pm_runtime_put() so your changes are going to be lost. > > > I'm playing with removing all of that to see if there are any side effects > > in Mali DP like the ones you mentioned for RCAR. > > Note that the first frame will usually not be noticed as it often takes a few > frames for the display to turn on. Yes, and I have a TV connected to the output that takes ages to sync. But I can usually run some quick rmmmod+insmod tests and the TV would be too slow to turn off the output, so I can see if there are any artifacts. Best regards, Liviu > > > > This should guarantee that the device won't be suspended between CRTC > > > disable and CRTC enable during a mode set, without requiring any special > > > collaboration between CRTC enable/disable and atomic begin/flush to > > > handle runtime PM. If drivers implement this, there should be no need for > > > drm_atomic_helper_commit_tail_rpm(). > > > > > > Maxime, Daniel, what do you think about this ? > > > > > >>> I don't think there's anything the helpers can to to help support > > >>> this. > > >>> > > >>> Also, just wrapping functions with rpm_get/put only papers over bugs > > >>> in your driver - either you enable something, then the rpm_get needs > > >>> to be done anyway (since the hw will be shut down otherwise), or you > > >>> disable something, same reasons. The only thing a rpm_get/put does is > > >>> paper over the bugs where you try to access the hw when it's off. As > > >>> soon as this function finishes, the hw is shut down again, drops all > > >> register values on the floor, so either that access wasn't needed, or > > >>> your driver has a bug (because it assumes the register values survive > > >>> when they don't). > > >>> > > >>> So imo all around a bad idea, at least from my experience of doing rpm > > >>> enabling on i915 hw. > > >> > > >> Understood. Thanks! > > -- > Regards, > > Laurent Pinchart > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯