Bug 75552

Summary: Need one kernel option to enable all connected displays
Product: DRI Reporter: Guang Yang <guang.a.yang>
Component: DRM/IntelAssignee: Daniel Vetter <daniel>
Status: CLOSED INVALID QA Contact: Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity: enhancement    
Priority: lowest CC: intel-gfx-bugs, lei.a.liu, qingshuai.tian, ville.syrjala
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
fallback to old initial config if not all outputs are enabled none

Description Guang Yang 2014-02-27 01:22:03 UTC
commit eb1bfe807cb7b62a326fa20df5e3118a32c6f923
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Wed Feb 12 12:26:25 2014 -0800

    drm/i915: allow re-use BIOS connector config for initial fbdev config v3

    The BIOS or boot loader will generally create an initial display
    configuration for us that includes some set of active pipes and
    displays.  This routine tries to figure out which pipes and connectors
    are active and stuffs them into the crtcs and modes array given to us by
    the drm_fb_helper code.

    The overall sequence is:
      intel_fbdev_init - from driver load
        intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
        drm_fb_helper_init - build fb helper structs
        drm_fb_helper_single_add_all_connectors - more fb helper structs
      intel_fbdev_initial_config - apply the config
        drm_fb_helper_initial_config - call ->probe then register_framebuffer()
            drm_setup_crtcs - build crtc config for fbdev
              intel_fb_initial_config - find active connectors etc
            drm_fb_helper_single_fb_probe - set up fbdev
              intelfb_create - re-use or alloc fb, build out fbdev structs

    v2: use BIOS connector config unconditionally if possible (Daniel)
        check for crtc cloning and reject (Daniel)
        fix up comments (Daniel)
    v3: use command line args and preferred modes first (Ville)

    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
    Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
    [danvet: Re-add the WARN_ON for a missing encoder crtc - the state
    sanitizer should take care of this. And spell-ocd the comments.]
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The kernel driver behavior changed with Jesse's patch above , the i915 driver will try to keep the BIOS config instead of lighting up every port.
So need Jesse add one kernel option to enable all connected displays as before.
Comment 1 Chris Wilson 2014-02-27 07:00:55 UTC
Why?
Comment 2 Guang Yang 2014-02-28 07:15:50 UTC
(In reply to comment #1)
> Why?
This new changing will cause inconvenient for QA's work  :). After talking with Jesse at meeting, he agree to add one new option for kernel for QA. Just report this bug to track this task.
Comment 3 Chris Wilson 2014-02-28 11:08:58 UTC
i915.fastboot=-1?
Comment 4 Daniel Vetter 2014-03-03 08:31:35 UTC
(In reply to comment #3)
> i915.fastboot=-1?

It's a different issue, disabling fastboot just kills the modeset hacks, not the initial config takeover. I guess we need to be a notch more clever and also light up any displays not lit up by the bios (but otherwise keep the crtc/output/modes as set). Assigning to Jesse to polish this more.
Comment 5 Daniel Vetter 2014-03-03 08:32:09 UTC
i.e. expect me to shot down a patch just adding the option ;-)
Comment 6 Daniel Vetter 2014-03-03 14:25:34 UTC
Consolidating these bugs a bit

*** This bug has been marked as a duplicate of bug 75077 ***
Comment 7 Jesse Barnes 2014-03-03 17:12:31 UTC
> i.e. expect me to shot down a patch just adding the option ;-)

All QA is asking for is a way to go back to the old behavior, which
should be pretty trivial.

What is it you're threatening to shoot down so I don't bother posting
it?
Comment 8 Daniel Vetter 2014-03-03 17:19:24 UTC
On Mon, Mar 3, 2014 at 6:12 PM,  <bugzilla-daemon@freedesktop.org> wrote:
> --- Comment #7 from Jesse Barnes <jbarnes@virtuousgeek.org> ---
>> i.e. expect me to shot down a patch just adding the option ;-)
>
> All QA is asking for is a way to go back to the old behavior, which
> should be pretty trivial.
>
> What is it you're threatening to shoot down so I don't bother posting
> it?

I think we should actually enable any outputs the bios didn't, while
still keeping the bios' config wrt pipe selections and precise mode.
At least I think that's the least suprising thing to do, and for
almost all fastboot use-cases we don't care one bit about multihead.
Comment 9 Jesse Barnes 2014-03-04 19:16:15 UTC
This one isn't a dupe, it's a feature request to allow going back to something like the old behavior, lighting up as many displays as possible.
Comment 10 Daniel Vetter 2014-03-04 19:16:30 UTC
*** Bug 75077 has been marked as a duplicate of this bug. ***
Comment 11 Jesse Barnes 2014-03-04 19:17:29 UTC
Note to QA: you still can't use this behavior as a reliable test of whether a given port is working.  That's because we can't support all possible 3 output configs on all platforms.  Sometimes displays won't be enabled, but that doesn't mean there's a bug.
Comment 12 Daniel Vetter 2014-03-04 19:17:39 UTC
Created attachment 95113 [details] [review]
fallback to old initial config if not all outputs are enabled

Please test.
Comment 13 Daniel Vetter 2014-03-04 19:18:51 UTC
Ville, can you please also give this patch a spin to check whether it does what you expect?
Comment 14 Ville Syrjala 2014-03-04 20:06:15 UTC
(In reply to comment #13)
> Ville, can you please also give this patch a spin to check whether it does
> what you expect?

Needs a small adjustment and then it works on my IVB machine at least.

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index afccc6e..4f1a977 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -318,8 +318,6 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
                        continue;
                }
 
-               num_connectors_enabled++;
-
                encoder = connector->encoder;
                if (!encoder || WARN_ON(!encoder->crtc)) {
                        DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
@@ -328,6 +326,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
                        continue;
                }
 
+               num_connectors_enabled++;
+
                new_crtc = intel_fb_helper_crtc(fb_helper, encoder->crtc);
Comment 15 Daniel Vetter 2014-03-04 20:09:28 UTC
Guang, please apply Ville's fixup on top of my patch when testing - my patch alone is broken.
Comment 16 Chris Wilson 2014-03-04 20:17:37 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > i915.fastboot=-1?
> 
> It's a different issue, disabling fastboot just kills the modeset hacks, not
> the initial config takeover. I guess we need to be a notch more clever and
> also light up any displays not lit up by the bios (but otherwise keep the
> crtc/output/modes as set). Assigning to Jesse to polish this more.

What I meant is that this QA mode is the antithesis of fastboot - and I want to avoid the mistake of them declaring fastboot a failure because of their own requirements.
Comment 17 Guang Yang 2014-03-05 11:06:42 UTC
I found that with Daniel's patch(03494932b88b57cb4a177f754bb0c87bf342d1b8) on dinq which including the patch on comment 12 and 14 can enable all connectd outputs as our driver did before, did this patch impact the fastboot hardly as Chris worried?
Comment 18 Daniel Vetter 2014-03-27 22:58:16 UTC
I think we've fixed the original issues that prevented hotplug on the fbcon from working correctly, so I think there's no need any more for additional hacks.
Comment 19 Guang Yang 2014-03-28 01:44:24 UTC
Yeah, agree with Daniel, close this one.
Comment 20 Jari Tahvanainen 2016-10-07 10:13:05 UTC
Closing verified+invalid.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.