Re: [PATCH v3 03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers

[Daniel Vetter <daniel@ffwll.ch>]

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel