From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/2] drm/i915: ignore bios output config if not all outputs are on Date: Wed, 5 Mar 2014 19:34:45 +0100 Message-ID: <20140305183445.GY17001@phenom.ffwll.local> References: <1393963722-5617-1-git-send-email-daniel.vetter@ffwll.ch> <1393963722-5617-2-git-send-email-daniel.vetter@ffwll.ch> <20140304123301.0876a258@jbarnes-desktop> <20140304210812.GO17001@phenom.ffwll.local> <20140305082708.4317fd42@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f50.google.com (mail-ee0-f50.google.com [74.125.83.50]) by gabe.freedesktop.org (Postfix) with ESMTP id 3DAD3FA5A7 for ; Wed, 5 Mar 2014 10:34:53 -0800 (PST) Received: by mail-ee0-f50.google.com with SMTP id c13so641182eek.37 for ; Wed, 05 Mar 2014 10:34:52 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140305082708.4317fd42@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jesse Barnes Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Mar 05, 2014 at 08:27:08AM -0800, Jesse Barnes wrote: > On Tue, 4 Mar 2014 22:08:12 +0100 > Daniel Vetter wrote: > = > > On Tue, Mar 04, 2014 at 12:33:01PM -0800, Jesse Barnes wrote: > > > On Tue, 4 Mar 2014 21:08:42 +0100 > > > Daniel Vetter wrote: > > > = > > > > Both Ville and QA rather immediately complained that with the new > > > > initial_config logic from Jesse not all outputs get enabled. Since = the > > > > fbdev emulation pretty much tries to always enable as many outputs = as > > > > possible (it even has hotplug handling and all that) fall back if m= ore > > > > outputs could have been enabled. > > > > = > > > > v2: Fix up my confusion about what enabled means - it's passed from > > > > the fbdev helper, we need to check for a non-zero connector->encoder > > > > link. Spotted by Ville. > > > > = > > > > Cc: Jesse Barnes > > > > Cc: Ville Syrj=E4l=E4 > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=3D75552 > > > > Tested-by: Ville Syrj=E4l=E4 > > > > Signed-off-by: Daniel Vetter > > > > --- > > > > drivers/gpu/drm/i915/intel_fbdev.c | 17 +++++++++++++++++ > > > > 1 file changed, 17 insertions(+) > > > > = > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i= 915/intel_fbdev.c > > > > index df00e6b01f0d..c1a20c3babde 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > > @@ -290,6 +290,8 @@ static bool intel_fb_initial_config(struct drm_= fb_helper *fb_helper, > > > > int i, j; > > > > bool *save_enabled; > > > > bool fallback =3D true; > > > > + int num_connectors_enabled =3D 0; > > > > + int num_connectors_detected =3D 0; > > > > = > > > > /* > > > > * If the user specified any force options, just bail here > > > > @@ -324,6 +326,10 @@ static bool intel_fb_initial_config(struct drm= _fb_helper *fb_helper, > > > > = > > > > fb_conn =3D fb_helper->connector_info[i]; > > > > connector =3D fb_conn->connector; > > > > + > > > > + if (connector->status =3D=3D connector_status_connected) > > > > + num_connectors_detected++; > > > > + > > > > if (!enabled[i]) { > > > > DRM_DEBUG_KMS("connector %d not enabled, skipping\n", > > > > connector->base.id); > > > > @@ -338,6 +344,8 @@ static bool intel_fb_initial_config(struct drm_= fb_helper *fb_helper, > > > > continue; > > > > } > > > > = > > > > + num_connectors_enabled++; > > > > + > > > > new_crtc =3D intel_fb_helper_crtc(fb_helper, encoder->crtc); > > > > = > > > > /* > > > > @@ -393,6 +401,15 @@ static bool intel_fb_initial_config(struct drm= _fb_helper *fb_helper, > > > > fallback =3D false; > > > > } > > > > = > > > > + /* > > > > + * If the BIOS didn't enable everything it could, fall back to ha= ve the > > > > + * same user experiencing of lighting up as much as possible like= the > > > > + * fbdev helper library. > > > > + */ > > > > + if (num_connectors_enabled !=3D num_connectors_detected && > > > > + num_connectors_enabled < INTEL_INFO(dev)->num_pipes) > > > > + fallback =3D true; > > > = > > > I think we need a debug message in here so people can figure out why > > > their fastboot failed with this patch included. E.g. "some connected > > > outputs weren't enabled, falling back to old behavior". > > > = > > > Also note that this will probably always happen in certain configs, a= nd > > > the fallback behavior won't be any better since we may not be able to > > > light up everything that's attached. > > = > > Excellent suggestion, I've gone ahead and added debug output for all ca= ses > > where we fall back. > > > = > > > With those caveats: > > > Reviewed-by: Jesse Barnes > = > Thinking about this some more last night, I think it would be better to > count the pipes and the connectors, and bail out if we have detected > connectors available but not enabled and some free pipes. That would > prevent unnecessary fastboot breakage I think. I do take pipes into account and only bail out if we'd have a free one. I don't see what more we could do (beside trying to keep the crtcs for the already enabled connectors)? -Daniel -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch