Re: [PATCH 1/6] drm/i915: unconditionally copy mode into crtc at boot time

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Dec 16, 2013 at 04:35:40PM -0800, Jesse Barnes wrote:
> On Thu, 12 Dec 2013 14:44:33 -0800
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > On Thu, 12 Dec 2013 23:39:39 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Thu, Dec 12, 2013 at 01:29:39PM -0800, Jesse Barnes wrote:
> > > > On Thu, 12 Dec 2013 22:21:29 +0100
> > > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > 
> > > > > On Thu, Dec 12, 2013 at 12:41:52PM -0800, Jesse Barnes wrote:
> > > > > > The BIOS code will need this to properly inherit the mode.
> > > > > > 
> > > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index af3717a..1ae3d44 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -11175,7 +11175,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> > > > > >  	 */
> > > > > >  	list_for_each_entry(crtc, &dev->mode_config.crtc_list,
> > > > > >  			    base.head) {
> > > > > > -		if (crtc->active && i915_fastboot) {
> > > > > > +		if (crtc->active) {
> > > > > 
> > > > > That's just enabling half of the fastboot hack, so nacked.
> > > > 
> > > > This part isn't the hack, but is required for fastboot.  Without this,
> > > > we'll end up with a bogus mode when we try to inherit from the BIOS.
> > > > So if you want to nack this I'll have to put the other BIOS bits under
> > > > fastboot as well.
> > > 
> > > crtc->config.pipe_src_w/h not good enough? I've thought that's what you've
> > > used in the last iteration, hence my suprise why we suddenly need to
> > > resurrect this hack here. All the information this hack exposes to
> > > crtc->mode is available in the pipe config, so I really don't think you
> > > neeed it.
> > > 
> > > Count me confused for now, please explain.
> > 
> > Yes, we read out all the data into the pipe config.  But if we don't
> > put that data into the CRTC proper, any subsequent code that uses the
> > CRTC mode will fail and think there's nothing there (resulting in fail
> > at fbcon DPMS time for example).
> 
> The actual warning w/o this change is:
> 
> [   17.088591] [drm:intel_pipe_config_compare] *ERROR* mismatch in pipe_src_w (expected 0, found 4096)
> [   17.088592] ------------[ cut here ]------------
> [   17.088619] WARNING: CPU: 1 PID: 534 at drivers/gpu/drm/i915/intel_display.c:9588 check_crtc_state+0x6bf/0xc90 [i915]()
> [   17.088619] pipe state doesn't match!
> ...
> [   17.088710]  [<ffffffffa02f2caf>] check_crtc_state+0x6bf/0xc90 [i915]
> [   17.088729]  [<ffffffffa03007ab>] intel_modeset_check_state+0x2bb/0x7b0 [i915]
> [   17.088745]  [<ffffffffa0300d35>] intel_set_mode+0x25/0x30 [i915]
> [   17.088762]  [<ffffffffa03015db>] intel_crtc_set_config+0x7ab/0x9a0 [i915]
> [   17.088777]  [<ffffffffa0133a4d>] drm_mode_set_config_internal+0x5d/0xe0 [drm]
> [   17.088783]  [<ffffffffa0184ea1>] drm_fb_helper_set_par+0x71/0xf0 [drm_kms_helper]
> [   17.088787]  [<ffffffff8135b301>] fb_set_var+0x191/0x430
> 
> So the first mode set from the fb layer is half baked, because the core
> DRM structures didn't have the right mode to pass down, and so we fall
> over.
> 
> I can fix this either by always copying the mode info into the core
> structs, or by putting the call to fbdev_init_bios under i915_fastboot
> as well.

Hm, I think I need to take a closer look here again since I really thought
it'd would Just Work. Hiding more behind the fastboot hack isn't really
what I want either, especially since we want to move from checking the
input mode to checking the pipe config for fastbooting. So having a single
piece which relies on that reconstructed mode would be rather ugly.

Can you please attach the full debug log for this?

Also, could this just be a side effect of the more fancy ->initial_config
logic, i.e. what happens if we disable that one?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch