[PATCH v3 03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers
Daniel Vetter
daniel at ffwll.ch
Sun Aug 14 09:44:14 UTC 2016
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 at 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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the dri-devel
mailing list