Bug 66516

Wrong bug number? It links to a libre-office bug ...

How many other 845g bugs have I filed this week... bug 66462.

b5ea2d5681522f1b8ef886b5ac039903bf1d39fe is the first bad commit
commit b5ea2d5681522f1b8ef886b5ac039903bf1d39fe
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Jun 27 17:52:15 2013 +0200

    drm/i915: fix hpd interrupt register locking
    
    Our interrupt handler (in hardirq context) could race with the timer
    (in softirq context), hence we need to hold the spinlock around the
    call to ->hdp_irq_setup in intel_hpd_irq_handler, too.
    
    But as an optimization (and more so to clarify things) we don't need
    to do the irqsave/restore dance in the hardirq context.
    
    Note also that on ilk+ the race isn't just against the hotplug
    reenable timer, but also against the fifo underrun reporting. That one
    also modifies the SDEIMR register (again protected by the same
    dev_priv->irq_lock).
    
    To lock things down again sprinkle a assert_spin_locked. But exclude
    the functions touching SDEIMR for now, I want to extract them all into
    a new helper function (like we do already for pipestate, display
    interrupts and all the various gt interrupts).
    
    v2: Add the missing 't' Egbert spotted in a comment.
    
    v3: Actually fix the right misspelled comment (Paulo).
    
    Cc: Egbert Eich <eich@suse.de>
    Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

(Now verifying the bisect.)

Anticlimactically, no.

66e3d5c09940d08d94b03e65b420fadaa7484318 is the first bad commit
commit 66e3d5c09940d08d94b03e65b420fadaa7484318
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Sun Jun 16 21:24:16 2013 +0200

    drm/i915: move i9xx dpll enabling into crtc enable function
    
    Now that we have the proper pipe config to track this, we don't need
    to write any registers any more.
    
    Note that for platforms without DPLL_MD (pre-gen4) which store the
    pixel mutliplier in the DPLL register I've decided to keep the
    seemingly "redundant" write: The comment right below saying "do this
    trice for luck" doesn't instill confidence ...
    
    v2: Drop a few now unnecessary local variables and switch the enable
    function to take a struct intel_crtc * to simply arguments.
    
    v3: Rebase on top of the newly-colored BUG_ON.
    
    v4: Amend commit message to alliviate Imre's comment about the
    redudant DPLL write for the pixel mutliplier.
    
    Cc: Imre Deak <imre.deak@intel.com>
    Cc: Jani Nikula <jani.nikula@linux.intel.com>
    Reviewed-by: Imre Deak <imre.deak@intel.com>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Definitely this time.

good: DPLL_A: 0xd0820000 (enabled, dvo, default clock, DAC/serial mode, p1 = 4, p2 = 4)

bad: DPLL_A: 0x90820000 (enabled, non-dvo, default clock, DAC/serial mode, p1 = 4, p2 = 4)

Working hack:

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1a8a01b..e77acf1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3717,7 +3717,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 
-	if (!intel_crtc->active)
+	if (!intel_crtc->active && 0)
 		return;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -4597,6 +4597,7 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
 	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS)) {
 		dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	} else {
+		dpll |= DPLL_DVO_HIGH_SPEED;
 		if (clock->p1 == 2)
 			dpll |= PLL_P1_DIVIDE_BY_TWO;
 		else
@@ -9431,7 +9432,7 @@ static struct intel_quirk intel_quirks[] = {
 	{ 0x2782, 0x17aa, 0x201a, quirk_pipea_force },
 
 	/* 830/845 need to leave pipe A & dpll A up */
-	{ 0x2562, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
+	//{ 0x2562, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 	{ 0x3577, PCI_ANY_ID, PCI_ANY_ID, quirk_pipea_force },
 
 	/* Lenovo U160 cannot use SSC on LVDS */

And now no output on resume.

Created attachment 82095 [details] [review]
i8xx dvo pll fixes

quick hack to check whether I'm right ... If you can test so, I'd be very interested in low-res dvo modes (i.e. below the dotclock limit where we'd currently use a P2=4 divisor).

No noticeable improvement. What I suspect is that we are simply not restoring the backlight - this is one of the machines where we do not know the maximum backlight value so disable our interface. And in this case, we have no other interfaces.

Doing a full save/restore across suspend isn't enough. (I was hoping that preserving the backlight register even if we can't understand them would be sufficient.) Not even poking 0xff into the PCI backlight control word.

So not backlight, and not a difference in final registers... Even a missing register or incorrect sequence.

Remaining hack required to lit up the LVDS:

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_d
index 2fedc3e..bab3a52 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10212,6 +10212,11 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc
                        encoder->base.crtc = NULL;
                }
        }
+
+       if (!crtc->active) {
+               crtc->active = true;
+               intel_crtc_disable(crtc);
+       }
 }
 
 static void intel_sanitize_encoder(struct intel_encoder *encoder)

(no word on resume issues yet)

On Sat, Jul 6, 2013 at 7:24 PM,  <bugzilla-daemon@freedesktop.org> wrote:
> https://bugs.freedesktop.org/show_bug.cgi?id=66516
>
> --- Comment #13 from Chris Wilson <chris@chris-wilson.co.uk> ---
> Remaining hack required to lit up the LVDS:
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_d
> index 2fedc3e..bab3a52 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10212,6 +10212,11 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc
>                         encoder->base.crtc = NULL;
>                 }
>         }
> +
> +       if (!crtc->active) {
> +               crtc->active = true;
> +               intel_crtc_disable(crtc);
> +       }
>  }
>
>  static void intel_sanitize_encoder(struct intel_encoder *encoder)

Hm, since the pipe A quirk in effect this just means that we leave pll
and pipe running, but kill everything else. Which probably means we've
broken a subtile ordering constraint somewhere recently ... no idea
what yet.
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

845g is currently happy - until resume...