From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1033557AbdAFKKi (ORCPT ); Fri, 6 Jan 2017 05:10:38 -0500 Received: from mail-wm0-f68.google.com ([74.125.82.68]:36254 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753806AbdAFKKA (ORCPT ); Fri, 6 Jan 2017 05:10:00 -0500 Date: Fri, 6 Jan 2017 11:09:56 +0100 From: Daniel Vetter To: Peter Ujfalusi Cc: daniel.vetter@intel.com, jani.nikula@linux.intel.com, seanpaul@chromium.org, airlied@linux.ie, gleb@fastmail.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, freedesktop-bugs@PaulSD.com Subject: Re: [PATCH] drm: Schedule the output_poll_work with 1s delay if we have delayed event Message-ID: <20170106100956.2zy2gx7ld3kqjwcf@phenom.ffwll.local> Mail-Followup-To: Peter Ujfalusi , daniel.vetter@intel.com, jani.nikula@linux.intel.com, seanpaul@chromium.org, airlied@linux.ie, gleb@fastmail.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, freedesktop-bugs@PaulSD.com References: <20170104120053.31882-1-peter.ujfalusi@ti.com> <20170105084324.n33j6jrprnhbg736@phenom.ffwll.local> <1d7bbe7d-428e-5ad6-4bea-70bad4e62f3f@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1d7bbe7d-428e-5ad6-4bea-70bad4e62f3f@ti.com> X-Operating-System: Linux phenom 4.8.0-1-amd64 User-Agent: NeoMutt/20161126 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jan 05, 2017 at 12:40:29PM +0200, Peter Ujfalusi wrote: > On 01/05/2017 10:43 AM, Daniel Vetter wrote: > > On Wed, Jan 04, 2017 at 02:00:53PM +0200, Peter Ujfalusi wrote: > >> Instead of scheduling the work to handle the initial delayed event, use 1s > >> delay. > >> > >> When the delayed event is handled w/o delay - in a similar matter when the > >> poll had been initialized before drm_helper_probe_single_connector_modes() > >> is called - it triggers a race in Optimus setups. > >> > >> Fixes: 339fd36238dd ("drm: drm_probe_helper: Fix output_poll_work scheduling") > >> Cc: stable@vger.kernel.org # v4.9 > >> Signed-off-by: Peter Ujfalusi > >> --- > >> Hi, > >> > >> related bug report: https://bugs.freedesktop.org/show_bug.cgi?id=98690 > >> > >> Regards, > >> Peter > >> > >> drivers/gpu/drm/drm_probe_helper.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > >> index 98ed110e28ed..f30c14b0a72f 100644 > >> --- a/drivers/gpu/drm/drm_probe_helper.c > >> +++ b/drivers/gpu/drm/drm_probe_helper.c > >> @@ -146,8 +146,9 @@ void drm_kms_helper_poll_enable_locked(struct drm_device *dev) > >> drm_connector_list_iter_put(&conn_iter); > >> > >> if (dev->mode_config.delayed_event) { > >> + /* Use short (1s) delay to handle the initial delayed event */ > >> poll = true; > >> - delay = 0; > >> + delay = HZ; > > > > This smells a bit like duct-tape papering over some other race. > > Yes, I agree. We could revert 339fd36238dd also to put back the duct-tape. Since the revert and this patch are the same kind of duct-tape (massive delay) I prefer something like this patch ... > > > Lack of > > locking or something else. Most likely some drivers enable polling a bit > > too early in their load sequence. > > All is pointing to Optimus. Intel and nouveau alone works, but we have > failure on Optimus laptops. > I have tried to narrow it down with another patch attached to the > bugzilla: https://bugs.freedesktop.org/attachment.cgi?id=128742 > > I expected it to fix the problem. But it did not. So I'm puzzled. > > >And if we can't figure this out, then we > > need some really big FIXME: here that this papers over driver races. > > Yeah, now we know that there is something wrong in some driver(s). We > can hide it with delaying the poll_work. It would be great if > nouveau/optimus guys would be able to take a look at this. ... but with all the above information (optimus only, what blows up) condensed into both the commit message and a big WARNING comment in the code. Then I think this is reasonable to merge (but still a decent wtf), since it addresses a rather serious regression. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch