From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm: call connector->dpms(OFF) when disabling Date: Fri, 28 Sep 2012 15:11:44 +0300 Message-ID: <20120928121144.GP19732@intel.com> References: <1348817238-30228-1-git-send-email-rob.clark@linaro.org> <20120928095605.GJ2098@bremse> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id B41DE9F00A for ; Fri, 28 Sep 2012 05:11:48 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: rogerq@ti.com, patches@linaro.org, dri-devel@lists.freedesktop.org, Rob Clark List-Id: dri-devel@lists.freedesktop.org On Fri, Sep 28, 2012 at 01:55:19PM +0200, Daniel Vetter wrote: > On Fri, Sep 28, 2012 at 12:54 PM, Rob Clark wrote: > > Maybe it just makes sense to always do connector->dpms(OFF) before > > unhooking the chain, rather than directly calling dpms on the > > encoder/crtc? > = > Well, that makes the entire (optional) ->disable stuff a bit more > awkward. The thing imo really is that the crtc helper assume that the > connectors do not hold any hw state, whereas you're omap driver > violates that assumption. > = > For the intel driver we've fixed this by doing any sink handling (e.g. > for dp) in the encoder->dpms functions. So I think the right way for > omap is to do the same (or conclude that the crtc helpers are a bad > fit and ditch them). Having connectors that are special, but only in > some of the cases imo makes squat sense for a generic helper library. > = > >> Which is imo a clear sign that the crtc helper is a misfit for your hw= and > >> you should stop using it ;-) And adding special-case handling like this > >> into common code, especially if it weakens the semantic concepts in the > >> helper layer is a recipe for a maintainance disaster a few years down = the > >> road. Hence > > > > well, I think by the time we start adding atomic-modeset and > > common/dsi panel framework, there might be need for a new set of > > helpers.. but I'm not sure that the hw is quite strange enough to need > > an omap specific set of helpers. Maybe I start w/ something in > > omapdrm but then refactor into common functions once a few drivers are > > converted to atomic modeset and panel framework. > = > I see a few ways forward with the crtc helpers and atomic modeset: > - bake the assumption into the code that drivers using the crtc > helpers don't have any shared global resources and drop all these > checks. This would boil down to writing a new set_config to handle > global modeset changes (instead of changes to just one crtc). > Obviously the current possible_encoders/possible_crtcs would still be > checked. > - like above, but add a driver-global ->check callback before starting > the modeset sequence, but with the new configuration already applied > to the kms structures. That's essentially what my intel_atomic.c code does. Somewhat ironically, since your modeset rework, that code is now more suited for other drivers and I need to go and rewrite it for i915. -- = Ville Syrj=E4l=E4 Intel OTC