[PATCH v3 03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers
Russell King - ARM Linux
linux at armlinux.org.uk
Sat Aug 13 11:29:47 UTC 2016
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.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
More information about the dri-devel
mailing list