[PATCH v3 03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers
Ying Liu
gnuiyl at gmail.com
Mon Aug 15 06:21:51 UTC 2016
On Sun, Aug 14, 2016 at 7:43 PM, Peter Senna Tschudin
<peter.senna at 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 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.
>>
>> 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
More information about the dri-devel
mailing list