Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753612Ab1BIKUu (ORCPT ); Wed, 9 Feb 2011 05:20:50 -0500 Received: from smarthost1.greenhost.nl ([195.190.28.78]:53646 "EHLO smarthost1.greenhost.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751723Ab1BIKUs (ORCPT ); Wed, 9 Feb 2011 05:20:48 -0500 Message-ID: In-Reply-To: References: <849307$bf0dak@azsmga001.ch.intel. Date: Wed, 9 Feb 2011 11:20:46 +0100 (CET) Subject: Re: Commit 500f7147cf5bafd139056d521536b10c2bc2e154 breaks _resume_ From: "Indan Zupancic" To: "Chris Wilson" Cc: "Jeff Chua" , "Takashi Iwai" , "Linus Torvalds" , "Rafael J. Wysocki" , "Len Brown" , "LKML" User-Agent: SquirrelMail/1.4.17 MIME-Version: 1.0 Content-Type: text/plain;charset=UTF-8 Content-Transfer-Encoding: 8bit X-Priority: 3 (Normal) Importance: Normal X-Spam-Score: 0.0 X-Scan-Signature: 729ef2e9e2cd27dd49f9ca04774c95e6 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2449 Lines: 57 On Wed, February 9, 2011 10:32, Chris Wilson wrote: > On Wed, 9 Feb 2011 03:56:36 +0100 (CET), "Indan Zupancic" wrote: >> All in all it seems quite wrong, no matter if it happens to work, because >> it depends on the calling order done by the drm layer. If *_crtc_enable() >> is called instead it won't do anything because of that active = true thing. >> This seems to be happening in your case. > > The order is very well defined. > > modesetting (upon resume we set the previous mode): > for each enabled crtc: > crtc_helper->prepare -> intel_crtc_disable() > crtc->mode_set -> intel_crtc_mode_set() > crtc_helper->commit -> intel_crtc_enable() > for each !enabled crtc: > crtc->disable -> intel_crtc_disable() Prepare for what? That could mean anything. Is it only used to prepare changing a mode, or suspend, or what? Same for commit. Should it have been named modes_set_prepare and mode_set_commit? It's clear if you've worked a lot on the code, but if you're just debugging it's had to tell when anything will be called, the function pointer chasing isn't fun. And that doesn't guarantee a driver function won't be called in a different way in the future anyway. I'd go as far as saying that if that's always the order, then just get rid of prepare and commit and do what's needed in the mode_set function. If prepare and commit don't have any other purpose or reason for existence. And back to the original thing, where does that reset() callback fit in? It's absent from the very well defined order. And judging by its name, it should be okay to call at any moment, but it seems that isn't the case. But the real wrong thing is fiddling with "active" outside of those state changing functions to force a certain behaviour later on if a specific function is called just after this (fingers crossed). My advise is to just remove that active = true line and fix any breakage that results in some other way. It acts as an out-of-band message to *crtc_disable() to re-disable already disabled crtcs, which doesn't always work (either now or in the future). If the state wasn't already messed up, it is after doing the active = true thing. Greetings, Indan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/