[Intel-gfx] [PATCH] drm/i915: fastboot support for HSW and BDW

[Jesse Barnes]

On Mon, 13 Jan 2014 17:55:27 -0200
Paulo Zanoni <przanoni at gmail.com> wrote:

> 2014/1/8 Jesse Barnes <jbarnes at virtuousgeek.org>:
> > No idea if this is correct or not, all the WRPLL programming is new to
> > me.  Paulo, can you take a look?  At least it doesn't complain on my BDW
> > here...
> 
> As a side note: can't we add some debugfs interface that, when read,
> would trigger a HW state readout, and then would compare it against
> what we have? It would be the perfect thing to validate patches like
> the one you wrote... And it would make QA easier (just write a test
> case that sets modes and reads the debugfs interface! Or just modify
> kms_setmode/testdisplay to do it everywhere).
> 
> Some comments:
> 
> - I guess the commit message could be improved :)
> - We probably need to do something about the VGA output, which uses
> the old intel_crt encoder instead of intel_digital_port. If this patch
> doesn't regress anything, we could do it on a separate patch.

I'm not using intel_digital_port here though, or is that what
ddi_get_encoder_port needs?  If so then yeah I guess I'll need to add
specific VGA support somehow.

> - Don't we also need to patch intel_pipe_config_compare() so it checks
> the now-used adjusted_mode.crtc_clock?

Yeah, it's there, just needs to come out from under its !HAS_DDI rock.

> - Based on that, don't we also need HW readout support for port_clock?

Ah yeah that needs to be set too.

> - You should probably compare your patch against "[PATCH 2/2]
> drm/i915: add fast boot support for Haswell" sent by Furquan Shaikh in
> August, and also check the reviews we gave to it.

Just checked (I had totally forgotten that), I think his version is a
little nicer, but I didn't see an update.

Furquan, were you planning to re-submit for inclusion?

> - I tried booting it on my HSW machine that only has an eDP output,
> and the driver doesn't load:
> 
> I get:
> 
> [   62.635700] WARNING: CPU: 0 PID: 1120 at
> drivers/gpu/drm/i915/intel_ddi.c:431
> intel_ddi_get_crtc_encoder+0x95/0xa0 [i915]()
> [   62.635703] 0 encoders on crtc for pipe A

Hm what would cause that?  Why would we not have an encoder assigned?

> 
> Then later I see:
> 
> [   62.636200] kernel BUG at drivers/gpu/drm/i915/intel_ddi.c:433!
> [   62.636249] invalid opcode: 0000 [#1] SMP
> 
> Which is the "BUG_ON(ret == NULL);" line
> 
> Please notice that I'm not using the i915_fastboot command line argument.
> > -
> > -               ironlake_get_fdi_m_n_config(crtc, pipe_config);
> >         }
> >
> > +       ironlake_get_fdi_m_n_config(crtc, pipe_config);
> 
> This is a little confusing, probably wrong, because FDI is only used
> when has_pch_encoder is true, but you're calling this code on all
> cases.
> 
> Function ironlake_get_fdi_m_n_config() calls
> intel_cpu_transcoder_get_m_n() for eDP/DP/HDMI.
> However, we have encoder->get_config which calls
> intel_ddi_get_config() which calls intel_dp_get_m_n(), which will call
> intel_cpu_transcoder_get_m_n() again on the same cases. So we call the
> same thing twice.

Confusing I agree, but I think we set the CPU m/n values for
eDP/DP/HDMI on HSW+?  If so, we need these values to figure out the
actual mode clock.  I think the redundant call is ok, but I'll check
other platforms.

> I would imagine you probably don't need to move this line above, but I
> may be wrong. If something is wrong, you probably need to fix some
> other code that's assuming that "gen5+ DP m/n registers are on the
> PCH" or something like that.

We mainly need to get the values early on to calculate the pixel clock,
so maybe the later call can be dropped.

-- 
Jesse Barnes, Intel Open Source Technology Center