| Message ID | 20160814114323.GA3890@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Aug 14, 2016 at 7:43 PM, Peter Senna Tschudin <peter.senna@gmail.com> wrote: > On Sun, Aug 14, 2016 at 12:46:27PM +0200, Daniel Vetter wrote: >> On Sun, Aug 14, 2016 at 11:44:14AM +0200, Daniel Vetter wrote: >> > On Sat, Aug 13, 2016 at 12:29:47PM +0100, Russell King - ARM Linux wrote: >> > > On Sat, Aug 13, 2016 at 11:45:31AM +0100, Russell King - ARM Linux wrote: >> > > > On Sat, Aug 13, 2016 at 11:11:54AM +0100, Russell King - ARM Linux wrote: >> > > > > On Mon, Jul 04, 2016 at 03:40:32PM +0800, Liu Ying wrote: >> > > > > > Use the drm_plane_helper_update/disable() and drm_helper_crtc_mode_set() >> > > > > > transitional atomic helpers. The crtc->mode_set_nofb callback is added >> > > > > > so that the primary plane is no longer tied to the CRTC. Check/update >> > > > > > logics are separated to make sure crtc->mode_set_nofb and plane->atomic_update >> > > > > > are always successful. Also, some necessary logics are tweaked for a smooth >> > > > > > transition. >> > > > > > >> > > > > > Signed-off-by: Liu Ying <gnuiyl@gmail.com> >> > > > > >> > > > > This patch causes a regression with my xorg ddx driver: >> > > > > >> > > > > [ 29.850] (EE) armada(0): [drm] failed to set mode on crtc 24: Invalid argument >> > > > > >> > > > > I'm not sure why (yet). >> > > > >> > > > Hi, >> > > > >> > > > Enabling DRM debugging doesn't seem to provide much in the way of clues: >> > > > >> > > > [drm:drm_ioctl] pid=1015, dev=0xe200, auth=1, DRM_IOCTL_MODE_SETCRTC >> > > > [drm:drm_mode_setcrtc] [CRTC:24:crtc-0] >> > > > [drm:drm_mode_setcrtc] [CONNECTOR:34:HDMI-A-1] >> > > > [drm:drm_crtc_helper_set_config] >> > > > [drm:drm_crtc_helper_set_config] [CRTC:24:crtc-0] [FB:48] #connectors=1 (x y) (0 0) >> > > > [drm:drm_crtc_helper_set_config] [CONNECTOR:34:HDMI-A-1] to [CRTC:24:crtc-0] >> > > > [drm:drm_crtc_helper_set_config] attempting to set mode from userspace >> > > > [drm:drm_mode_debug_printmodeline] Modeline 49:"1920x1080" 0 148500 1920 2448 2492 2640 1080 1084 1089 1125 0x0 0x5 >> > > > [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0] >> > > > [drm:drm_vblank_off] crtc 0, vblank enabled 0, inmodeset 0 >> > > > [drm:drm_update_vblank_count] updating vblank count on crtc 0: current=2, diff=0, hw=0 hw_last=0 >> > > > [drm:drm_mode_object_reference] OBJ ID: 51 (1) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 51 (2) >> > > > [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81ee00 >> > > > [drm:drm_mode_object_reference] OBJ ID: 47 (3) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4) >> > > > [drm:drm_mode_object_reference] OBJ ID: 48 (2) >> > > > [drm:drm_atomic_set_fb_for_plane] Set [FB:48] for plane state ed632d00 >> > > > [drm:drm_mode_object_unreference] OBJ ID: 48 (3) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 51 (1) >> > > > [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0] >> > > > [drm:drm_crtc_helper_set_mode] [CRTC:24:crtc-0] >> > > > [drm:drm_mode_object_reference] OBJ ID: 52 (1) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 52 (2) >> > > > [drm:drm_atomic_set_mode_for_crtc] Set [MODE:1920x1080] for CRTC state ed81f600 >> > > > [drm:drm_mode_object_reference] OBJ ID: 47 (3) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4) >> > > > [drm:drm_mode_object_reference] OBJ ID: 47 (3) >> > > > [drm:drm_atomic_set_fb_for_plane] Set [FB:47] for plane state ed632d00 >> > > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 52 (1) >> > > > [drm:drm_crtc_helper_set_mode] [ENCODER:33:TMDS-33] set [MODE:46:1920x1080] >> > > > [drm:drm_vblank_on] crtc 0, vblank enabled 0, inmodeset 1 >> > > > [drm:drm_calc_timestamping_constants] crtc 24: hwmode: htotal 2640, vtotal 1125, vdisplay 1080 >> > > > [drm:drm_calc_timestamping_constants] crtc 24: clock 148500 kHz framedur 20000000 linedur 17777 >> > > > [drm:drm_mode_object_reference] OBJ ID: 47 (3) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 47 (4) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 48 (2) >> > > > [drm:drm_mode_object_unreference] OBJ ID: 34 (4) >> > > > [drm:drm_ioctl] ret = -22 >> > > > >> > > > With a bit more debugging, this is what's failing: >> > > > >> > > > ipu_plane_atomic_check:442: fail >> > > > [drm:drm_crtc_helper_set_config] *ERROR* failed to set mode on [CRTC:24:crtc-0] >> > > > >> > > > if (old_fb && (state->src_w != old_state->src_w || >> > > > state->src_h != old_state->src_h || >> > > > fb->pixel_format != old_fb->pixel_format)) { >> > > > printk("%s:%d: fail\n", __func__, __LINE__); >> > > > return -EINVAL; >> > > > } >> > > > >> > > > This test is stupid as it stands - it means we can _never_ change the >> > > > format or size of _any_ plane, something that the old code allowed: >> > > > >> > > > - if (ipu_plane->enabled) { >> > > > - if (src_w != ipu_plane->w || src_h != ipu_plane->h || >> > > > - fb->pixel_format != ipu_plane->base.fb->pixel_format) >> > > > - return -EINVAL; >> > > > >> > > > This used to work because we'd call ipu_crtc_prepare()->ipu_fb_disable() >> > > > before the mode set, which would clear ipu_plane->enabled, causing this >> > > > test to be skipped. However, in the new code, we merely test for the >> > > > presence of the previous framebuffer, which is really not the same thing >> > > > at all. >> > > > >> > > > The old functionality needs to be restored because significantly >> > > > changing the displayed mode is something which must be supported with >> > > > HDMI. >> > > >> > > Bypassing the above check also shows that: >> > > >> > > if (old_fb && fb->pitches[0] != old_fb->pitches[0]) { >> > > printk("%s:%d: fail\n", __func__, __LINE__); >> > > return -EINVAL; >> > > } >> > > >> > > also fails. Disabling this as well results in loss of sync on the >> > > HDMI display, although the mode set now succeeds. >> > > >> > > The more I think about this, the more I come to one of two possible >> > > conclusions: >> > > >> > > 1. These checks were not appropriate with the old code as we were >> > > always disabling the display channel prior to making changes. >> > > >> > > We need atomic mode set to work in exactly the same way as the >> > > previous code: as a series of disable-modify-enable set of steps, >> > > so that we don't need these restrictive and regression causing >> > > checks. >> > > >> > > or >> > > >> > > 2. imx-drm has these particular restrictions which make it inappropriate >> > > to be converted to atomic mode set, as we always need to go through >> > > a disable-modify-enable series of steps - which means the atomic >> > > mode set changes for imx-drm need to be reverted back to this patch. >> > > >> > > I'm pretty sure (2) doesn't really apply, because I see no reason why >> > > we can't disable the display channel while we reconfigure it. That, >> > > to me, seems to be an entirely reasonable thing to do. >> > >> > Already ddiscussed this at lenght on irc with Peter Senna. If you have >> > plane update restrictions where you need to cycle the crtc completely, >> > the right thing to do is to set crtc_state->mode_changed instead of return >> > -EINVAL. The helper/core will then convert that into an EINVAL if >> > userspace doesn't set ALLOW_MODESET. And since the helper function to map >> > set_config to atomic does this, it should all just work. >> > >> > Note: There's about 3 such conditions in imx' atomic_check function which >> > needs this change. >> >> You also need to update the overall atomic_check to re-run the modeset >> checks after the plane checks again (since plane checks can update >> mode_changed). Peter Senna has the full diff, but apparently it doesn't >> work either. So there's probably more bugs somewhere. > > Patch attached. This patch change the output from black screen to a > buggy image that actually gets updated if I move the mouse, which means > that it doesn't fix the issue. Tested on next-20160812. > > Not enabling the frame buffer emulation(Kconfig or commenting out the > call to drm_fbdev_cma_init()) solves the issue. > I've send a patch[1] to fix the issue by following the approach suggested by Daniel Vetter. Comments or Tested-by are welcome. [1] http://www.spinics.net/lists/dri-devel/msg115488.html Regards, Liu Ying
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 9f7dafc..7de7126 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -171,10 +171,31 @@ static void imx_drm_output_poll_changed(struct drm_device *drm) drm_fbdev_cma_hotplug_event(imxdrm->fbhelper); } +static int imx_atomic_check(struct drm_device *dev, + struct drm_atomic_state *state) +{ + int ret = 0; + + ret = drm_atomic_helper_check_modeset(dev, state); + if (ret) + return ret; + + ret = drm_atomic_helper_check_planes(dev, state); + if (ret) + return ret; + + /* planes can set ->needs_modeset, once more to reach a stable state */ + ret = drm_atomic_helper_check_modeset(dev, state); + if (ret) + return ret; + + return ret; +} + static const struct drm_mode_config_funcs imx_drm_mode_config_funcs = { .fb_create = drm_fb_cma_create, .output_poll_changed = imx_drm_output_poll_changed, - .atomic_check = drm_atomic_helper_check, + .atomic_check = imx_atomic_check, .atomic_commit = drm_atomic_helper_commit, }; diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c index 4ad67d0..b98581a 100644 --- a/drivers/gpu/drm/imx/ipuv3-plane.c +++ b/drivers/gpu/drm/imx/ipuv3-plane.c @@ -322,10 +322,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, * since we cannot touch active IDMAC channels, we do not support * resizing the enabled plane or changing its format */ - if (old_fb && (state->src_w != old_state->src_w || - state->src_h != old_state->src_h || - fb->pixel_format != old_fb->pixel_format)) - return -EINVAL; + if (state->src_w != old_state->src_w || + state->src_h != old_state->src_h || + (old_fb && fb->pixel_format != old_fb->pixel_format)) + crtc_state->mode_changed = true; eba = drm_plane_state_to_eba(state); @@ -336,7 +336,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; if (old_fb && fb->pitches[0] != old_fb->pitches[0]) - return -EINVAL; + crtc_state->mode_changed = true; switch (fb->pixel_format) { case DRM_FORMAT_YUV420: @@ -372,7 +372,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane, return -EINVAL; if (old_fb && old_fb->pitches[1] != fb->pitches[1]) - return -EINVAL; + crtc_state->mode_changed = true; } return 0;