From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755435AbcAYH3e (ORCPT ); Mon, 25 Jan 2016 02:29:34 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:34690 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753293AbcAYH3b (ORCPT ); Mon, 25 Jan 2016 02:29:31 -0500 Date: Mon, 25 Jan 2016 08:29:38 +0100 From: Daniel Vetter To: Laurent Pinchart Cc: Daniel Vetter , Maxime Ripard , Mike Turquette , Stephen Boyd , David Airlie , Thierry Reding , Philipp Zabel , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-sunxi@googlegroups.com, Chen-Yu Tsai , Hans de Goede , Alexander Kaplan , Boris Brezillon , Wynter Woods , Thomas Petazzoni , Rob Clark Subject: Re: [PATCH v2 13/26] drm/fb_cma_helper: Remove implicit call to disable_unused_functions Message-ID: <20160125072938.GI11240@phenom.ffwll.local> Mail-Followup-To: Laurent Pinchart , Maxime Ripard , Mike Turquette , Stephen Boyd , David Airlie , Thierry Reding , Philipp Zabel , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-sunxi@googlegroups.com, Chen-Yu Tsai , Hans de Goede , Alexander Kaplan , Boris Brezillon , Wynter Woods , Thomas Petazzoni , Rob Clark References: <1452785109-6172-1-git-send-email-maxime.ripard@free-electrons.com> <2014724.QmjXVju9UW@avalon> <20160115101730.GH19130@phenom.ffwll.local> <1484612.l42nGNbAu5@avalon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1484612.l42nGNbAu5@avalon> X-Operating-System: Linux phenom 4.3.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 25, 2016 at 12:19:27AM +0200, Laurent Pinchart wrote: > Hi Daniel, > > On Friday 15 January 2016 11:17:30 Daniel Vetter wrote: > > On Fri, Jan 15, 2016 at 01:13:05AM +0200, Laurent Pinchart wrote: > > > On Thursday 14 January 2016 16:24:56 Maxime Ripard wrote: > > >> The drm_fbdev_cma_init function always calls the > > >> drm_helper_disable_unused_functions. Since it's part of the usual probe > > >> process, all the drivers using that helper will end up having their > > >> encoder and CRTC disable functions called at probe if their device has > > >> not been reported as enabled. > > >> > > >> This could be fixed by reading out from the registers the current state > > >> of the device if it is enabled, but even that will not handle the case > > >> where the device is actually disabled. > > >> > > >> Moreover, the drivers using the atomic modesetting expect that their > > >> enable and disable callback to be called when the device is already > > >> enabled or disabled (respectively). > > >> > > >> We can however fix this issue by moving the call to > > >> drm_helper_disable_unused_functions out of drm_fbdev_cma_init and make > > >> the drivers needing it (all the drivers calling drm_fbdev_cma_init and > > >> not using the atomic modesetting) explicitly call it. > > > > > > I'd rather add it to all drivers that call drm_fbdev_cma_init(). All the > > > atomic ones must have special code to cope with it that could be rendered > > > useless by the removal of the drm_helper_disable_unused_functions() call. > > > That code should be removed too, and it would be easier to check drivers > > > one by one and fixing them individually (outside of this patch series, > > > unless you insist ;-)) when removing the explicit > > > drm_helper_disable_unused_functions() call. > > > > I had the same thought, but figured there's really no good reason ever to > > do this. I suspect most of that disable_unused_function stuff we have all > > over is supreme cargo-cult anyway ;-) > > I'm not sure you got my point. Yes, the drm_helper_disable_unused_functions() > call should be removed from atomic drivers, but that will leave support code > added for the sole reason of avoiding the crash in the drivers. That code will > likely not be noticed and will stay there rotting. If we added > drm_helper_disable_unused_functions() calls to all atomic drivers then driver > authors will hopefully check carefully if there's any support code that can be > removed when removing the function call. It would act as a kind of FIXME > reminder. drm_helper_disable_unused_functions() already prefers the ->disable callbacks over dpms, like atomic helpers. I don't think there's any dead code due to this change. The problem was that driver/hw blew up since it was disabled when the hw was off already. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch