[PATCH v3 03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers
Peter Senna Tschudin
peter.senna at gmail.com
Sun Aug 14 11:43:23 UTC 2016
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.
-------------- next part --------------
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;
More information about the dri-devel
mailing list