VT switch broken with docking station DP

VT switch broken with docking station DP

[Takashi Iwai]

Hi,

recently I noticed that VT console doesn't work any longer when I dock
a Dell E7270 laptop with a DP monitor.  The bug detail is like this:

At first, I boot the laptop without dock.  I can switch between X and
VT via ctrl-alt-F1, so far.  Then I dock it to a docking station
connected with a DP monitor.  Now, when I switch to VT, it behaves as
if frozen, the X graphics screen remains.  But actually it's only
graphics and the keyboard input is processed in VT.  I can go back to
X via alt-F7 again.  The situation remains until I undock and I kill X
once.

After looking more deeply at drm debug log, I found out that it's
caused by the drm atomic check.  Essentially, it's because eDP has the
lower resolution (1366x768) than DP (1920x1080).  Since booting with
eDP, the frame buffer size is 1366x768.  Then it hits the following
check in drm_atomic_plane_check():

	fb_width = state->fb->width << 16;
	fb_height = state->fb->height << 16;

	/* Make sure source coordinates are inside the fb. */
	if (state->src_w > fb_width ||
	    state->src_x > fb_width - state->src_w ||
	    state->src_h > fb_height ||
	    state->src_y > fb_height - state->src_h) {
		DRM_DEBUG_ATOMIC("Invalid source coordinates "
				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
		return -ENOSPC;
	}

Actually after commenting out "return -ENOSPC", VT switch works fine.

But the code above made me wonder what's the requirement here.  IIRC,
the VT always worked on a display with a higher resolution even if the
frame buffer is smaller.  Only a part of display was used, but it was
OK, far better than the frozen graphics :)

Can we simply drop this check, or may we add a flag to skip it for VT
switching?  Or any better idea?


thanks,

Takashi

Re: VT switch broken with docking station DP

[Ville Syrjälä]

On Thu, Jan 05, 2017 at 04:37:27PM +0100, Takashi Iwai wrote:
> Hi,
> 
> recently I noticed that VT console doesn't work any longer when I dock
> a Dell E7270 laptop with a DP monitor.  The bug detail is like this:
> 
> At first, I boot the laptop without dock.  I can switch between X and
> VT via ctrl-alt-F1, so far.  Then I dock it to a docking station
> connected with a DP monitor.  Now, when I switch to VT, it behaves as
> if frozen, the X graphics screen remains.  But actually it's only
> graphics and the keyboard input is processed in VT.  I can go back to
> X via alt-F7 again.  The situation remains until I undock and I kill X
> once.
> 
> After looking more deeply at drm debug log, I found out that it's
> caused by the drm atomic check.  Essentially, it's because eDP has the
> lower resolution (1366x768) than DP (1920x1080).  Since booting with
> eDP, the frame buffer size is 1366x768.  Then it hits the following
> check in drm_atomic_plane_check():
> 
> 	fb_width = state->fb->width << 16;
> 	fb_height = state->fb->height << 16;
> 
> 	/* Make sure source coordinates are inside the fb. */
> 	if (state->src_w > fb_width ||
> 	    state->src_x > fb_width - state->src_w ||
> 	    state->src_h > fb_height ||
> 	    state->src_y > fb_height - state->src_h) {
> 		DRM_DEBUG_ATOMIC("Invalid source coordinates "
> 				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> 				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> 				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> 				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> 				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
> 		return -ENOSPC;
> 	}
> 
> Actually after commenting out "return -ENOSPC", VT switch works fine.
> 
> But the code above made me wonder what's the requirement here.  IIRC,
> the VT always worked on a display with a higher resolution even if the
> frame buffer is smaller.  Only a part of display was used, but it was
> OK, far better than the frozen graphics :)
> 
> Can we simply drop this check, or may we add a flag to skip it for VT
> switching?  Or any better idea?

Find out why it didn't allocate a big enough framebuffer to begin with,
or alternatively why it tried to specify source coordinates exceeding
the fb dimensions.

There is clearly a bug somewhere, just not here.

-- 
Ville Syrjälä
Intel OTC

Re: VT switch broken with docking station DP

[Takashi Iwai]

On Thu, 05 Jan 2017 16:59:31 +0100,
Ville Syrjälä wrote:
> 
> On Thu, Jan 05, 2017 at 04:37:27PM +0100, Takashi Iwai wrote:
> > Hi,
> > 
> > recently I noticed that VT console doesn't work any longer when I dock
> > a Dell E7270 laptop with a DP monitor.  The bug detail is like this:
> > 
> > At first, I boot the laptop without dock.  I can switch between X and
> > VT via ctrl-alt-F1, so far.  Then I dock it to a docking station
> > connected with a DP monitor.  Now, when I switch to VT, it behaves as
> > if frozen, the X graphics screen remains.  But actually it's only
> > graphics and the keyboard input is processed in VT.  I can go back to
> > X via alt-F7 again.  The situation remains until I undock and I kill X
> > once.
> > 
> > After looking more deeply at drm debug log, I found out that it's
> > caused by the drm atomic check.  Essentially, it's because eDP has the
> > lower resolution (1366x768) than DP (1920x1080).  Since booting with
> > eDP, the frame buffer size is 1366x768.  Then it hits the following
> > check in drm_atomic_plane_check():
> > 
> > 	fb_width = state->fb->width << 16;
> > 	fb_height = state->fb->height << 16;
> > 
> > 	/* Make sure source coordinates are inside the fb. */
> > 	if (state->src_w > fb_width ||
> > 	    state->src_x > fb_width - state->src_w ||
> > 	    state->src_h > fb_height ||
> > 	    state->src_y > fb_height - state->src_h) {
> > 		DRM_DEBUG_ATOMIC("Invalid source coordinates "
> > 				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> > 				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> > 				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> > 				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> > 				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
> > 		return -ENOSPC;
> > 	}
> > 
> > Actually after commenting out "return -ENOSPC", VT switch works fine.
> > 
> > But the code above made me wonder what's the requirement here.  IIRC,
> > the VT always worked on a display with a higher resolution even if the
> > frame buffer is smaller.  Only a part of display was used, but it was
> > OK, far better than the frozen graphics :)
> > 
> > Can we simply drop this check, or may we add a flag to skip it for VT
> > switching?  Or any better idea?
> 
> Find out 2why it didn't allocate a big enough framebuffer to begin with,
> or alternatively why it tried to specify source coordinates exceeding
> the fb dimensions.
> 
> There is clearly a bug somewhere, just not here.

Hrm, so that's my misunderstanding.  The problem is that it tries to
assign to 1368x768 resolution for DP while the fb is 1366x768...
Sounds familiar?


Takashi

Re: VT switch broken with docking station DP

[Ville Syrjälä]

On Thu, Jan 05, 2017 at 05:19:58PM +0100, Takashi Iwai wrote:
> On Thu, 05 Jan 2017 16:59:31 +0100,
> Ville Syrjälä wrote:
> > 
> > On Thu, Jan 05, 2017 at 04:37:27PM +0100, Takashi Iwai wrote:
> > > Hi,
> > > 
> > > recently I noticed that VT console doesn't work any longer when I dock
> > > a Dell E7270 laptop with a DP monitor.  The bug detail is like this:
> > > 
> > > At first, I boot the laptop without dock.  I can switch between X and
> > > VT via ctrl-alt-F1, so far.  Then I dock it to a docking station
> > > connected with a DP monitor.  Now, when I switch to VT, it behaves as
> > > if frozen, the X graphics screen remains.  But actually it's only
> > > graphics and the keyboard input is processed in VT.  I can go back to
> > > X via alt-F7 again.  The situation remains until I undock and I kill X
> > > once.
> > > 
> > > After looking more deeply at drm debug log, I found out that it's
> > > caused by the drm atomic check.  Essentially, it's because eDP has the
> > > lower resolution (1366x768) than DP (1920x1080).  Since booting with
> > > eDP, the frame buffer size is 1366x768.  Then it hits the following
> > > check in drm_atomic_plane_check():
> > > 
> > > 	fb_width = state->fb->width << 16;
> > > 	fb_height = state->fb->height << 16;
> > > 
> > > 	/* Make sure source coordinates are inside the fb. */
> > > 	if (state->src_w > fb_width ||
> > > 	    state->src_x > fb_width - state->src_w ||
> > > 	    state->src_h > fb_height ||
> > > 	    state->src_y > fb_height - state->src_h) {
> > > 		DRM_DEBUG_ATOMIC("Invalid source coordinates "
> > > 				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> > > 				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> > > 				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> > > 				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> > > 				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
> > > 		return -ENOSPC;
> > > 	}
> > > 
> > > Actually after commenting out "return -ENOSPC", VT switch works fine.
> > > 
> > > But the code above made me wonder what's the requirement here.  IIRC,
> > > the VT always worked on a display with a higher resolution even if the
> > > frame buffer is smaller.  Only a part of display was used, but it was
> > > OK, far better than the frozen graphics :)
> > > 
> > > Can we simply drop this check, or may we add a flag to skip it for VT
> > > switching?  Or any better idea?
> > 
> > Find out 2why it didn't allocate a big enough framebuffer to begin with,
> > or alternatively why it tried to specify source coordinates exceeding
> > the fb dimensions.
> > 
> > There is clearly a bug somewhere, just not here.
> 
> Hrm, so that's my misunderstanding.  The problem is that it tries to
> assign to 1368x768 resolution for DP while the fb is 1366x768...
> Sounds familiar?

Not really. Sounds like there's a bug in the fb helper somewhere that it
tries to add new connectors to the mix without checking that the fb is
big enough for whatever modes are supported on said connectors.

As far as the 1366 vs. 1368 thing goes, we have quirks in the EDID
parser to convert 1368 to 1366 with the assumption that 1366 is what
they really meant. The reason for the quirk being that EDID can't
actually say 1366 since it's not a multiple of 8. It might be
interesting to know where that 1368 mode came from and why the quirk
didn't convert it to 1366. But even fixing that part (assuming there's
something to fix) doesn't really excuse the fact that the fb helper
picked a resolution for the connector that doesn't fit in the fb.

-- 
Ville Syrjälä
Intel OTC

Re: VT switch broken with docking station DP

[Takashi Iwai]

On Thu, 05 Jan 2017 17:47:06 +0100,
Ville Syrjälä wrote:
> 
> On Thu, Jan 05, 2017 at 05:19:58PM +0100, Takashi Iwai wrote:
> > On Thu, 05 Jan 2017 16:59:31 +0100,
> > Ville Syrjälä wrote:
> > > 
> > > On Thu, Jan 05, 2017 at 04:37:27PM +0100, Takashi Iwai wrote:
> > > > Hi,
> > > > 
> > > > recently I noticed that VT console doesn't work any longer when I dock
> > > > a Dell E7270 laptop with a DP monitor.  The bug detail is like this:
> > > > 
> > > > At first, I boot the laptop without dock.  I can switch between X and
> > > > VT via ctrl-alt-F1, so far.  Then I dock it to a docking station
> > > > connected with a DP monitor.  Now, when I switch to VT, it behaves as
> > > > if frozen, the X graphics screen remains.  But actually it's only
> > > > graphics and the keyboard input is processed in VT.  I can go back to
> > > > X via alt-F7 again.  The situation remains until I undock and I kill X
> > > > once.
> > > > 
> > > > After looking more deeply at drm debug log, I found out that it's
> > > > caused by the drm atomic check.  Essentially, it's because eDP has the
> > > > lower resolution (1366x768) than DP (1920x1080).  Since booting with
> > > > eDP, the frame buffer size is 1366x768.  Then it hits the following
> > > > check in drm_atomic_plane_check():
> > > > 
> > > > 	fb_width = state->fb->width << 16;
> > > > 	fb_height = state->fb->height << 16;
> > > > 
> > > > 	/* Make sure source coordinates are inside the fb. */
> > > > 	if (state->src_w > fb_width ||
> > > > 	    state->src_x > fb_width - state->src_w ||
> > > > 	    state->src_h > fb_height ||
> > > > 	    state->src_y > fb_height - state->src_h) {
> > > > 		DRM_DEBUG_ATOMIC("Invalid source coordinates "
> > > > 				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> > > > 				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> > > > 				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> > > > 				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> > > > 				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10);
> > > > 		return -ENOSPC;
> > > > 	}
> > > > 
> > > > Actually after commenting out "return -ENOSPC", VT switch works fine.
> > > > 
> > > > But the code above made me wonder what's the requirement here.  IIRC,
> > > > the VT always worked on a display with a higher resolution even if the
> > > > frame buffer is smaller.  Only a part of display was used, but it was
> > > > OK, far better than the frozen graphics :)
> > > > 
> > > > Can we simply drop this check, or may we add a flag to skip it for VT
> > > > switching?  Or any better idea?
> > > 
> > > Find out 2why it didn't allocate a big enough framebuffer to begin with,
> > > or alternatively why it tried to specify source coordinates exceeding
> > > the fb dimensions.
> > > 
> > > There is clearly a bug somewhere, just not here.
> > 
> > Hrm, so that's my misunderstanding.  The problem is that it tries to
> > assign to 1368x768 resolution for DP while the fb is 1366x768...
> > Sounds familiar?
> 
> Not really. Sounds like there's a bug in the fb helper somewhere that it
> tries to add new connectors to the mix without checking that the fb is
> big enough for whatever modes are supported on said connectors.
> 
> As far as the 1366 vs. 1368 thing goes, we have quirks in the EDID
> parser to convert 1368 to 1366 with the assumption that 1366 is what
> they really meant. The reason for the quirk being that EDID can't
> actually say 1366 since it's not a multiple of 8.

Yeah, I know, that's why I found it funny and familiar.  The current
1366/1368 fixup was a hack I wrote many years ago :)

But it's covering only CVT and GTF range parsers.  The EDID parser got
extended since then, the other codes (e.g. for DP-MST like this case)
need the similar hacks.

> It might be
> interesting to know where that 1368 mode came from and why the quirk
> didn't convert it to 1366. But even fixing that part (assuming there's
> something to fix) doesn't really excuse the fact that the fb helper
> picked a resolution for the connector that doesn't fit in the fb.

I think I'm slowly understanding the scene behind it.

- In drm_fb_helper_hotplug_event(), it sets up crtcs again via 
  drm_setup_crtcs(); but at this time, it's calling with fb->width and
  height, i.e. 1366x768.

- drm_setup_crtcs() looks for the modes via drm_target_perferred().

- drm_target_preferred() loops over connectors, and it looks for the
  mode via drm_pick_cmdline_mode() for each connector.

- drm_pick_cmdline_mode() loops for each mode to the connector to look
  for the mode with the exact size (1366x768).  Since DP connector
  didn't get 1366x768 mode but only 1368x768, this loop doesn't hit.

- drm_pick_cmdline_mode() falls back to create a new mode via
  drm_mode_create_from_cmdline_mode() based on 1366x768.  This results
  again in 1368x768 because of rounding although width 1366 is given.

- This new mode is taken as if it were fitting with 1366x768, then
  it's invalidated later at the atomic check time.

So, if my guess is correct, it's neither about the wrong connector nor
about the wrong fb size.  Instead, it comes from the following two
problems: the 1366 fixup isn't applied in some EDID parser codes, and
again the 1366 fixup isn't applied at creating a new cmdline mode.

Unfortunately, the machine is in my office and I'm off from tomorrow,
so the further debugging will be in the next week...


thanks,

Takashi