From patchwork Sun Aug 14 11:43:23 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Senna Tschudin X-Patchwork-Id: 9279253 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 096A860231 for ; Sun, 14 Aug 2016 11:43:34 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id ED17728A21 for ; Sun, 14 Aug 2016 11:43:33 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id E1CD028A57; Sun, 14 Aug 2016 11:43:33 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, FSL_HELO_FAKE, RCVD_IN_DNSWL_MED, T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 1BA3928A21 for ; Sun, 14 Aug 2016 11:43:32 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 4BC8C6E17D; Sun, 14 Aug 2016 11:43:30 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id CE5186E17D for ; Sun, 14 Aug 2016 11:43:28 +0000 (UTC) Received: by mail-wm0-x241.google.com with SMTP id o80so6143589wme.0 for ; Sun, 14 Aug 2016 04:43:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=lJF96BJyM75MP/XfwgY2MPSpYv08T1DU2+Pss/aQrsE=; b=TgNz7g5V+fZi6DqaWI2WZEAgv5p1XXcQN86HwchBA8VJQaAkIP+o2/dmX2eWsppa1f COmXPoEwVP7ekyWdYtce7Ebv6lmMLTvlJ9+bqTH8tfscA5ulsSoy1WaomyPrP856YQLu p864f8qDYEQ+T3Mnn22Hj+xHxKhekxqtj4TQPrwY0KlB8eEM+sj48RfptxVITiJc0X3E iqr2yj3wJVfeaHDwDXPhQD8ub7me8Cl0mqqP1a5MTsfOuyNDQf9LDESB/fyRZWUoEb+y HUYhuxbGP1r1d1RQdomRnTL1+n9pUyPq5a4d+PzlJ4MlxGB6ihqjo+3CKlu4t/F/gbS7 sMpw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=lJF96BJyM75MP/XfwgY2MPSpYv08T1DU2+Pss/aQrsE=; b=a4nyAK6LnZAkFjEH0o363M4DgplaKtogUqe483erBwvn3x85nVH/HCnpn9SHQ8jWR5 vIS0TS/zmrQSygO088KgHiApwlV5HauKo+kCkOEY92DJi3JsAhQ7zfVfuq43CUFqC+jD A+nhiCez2OBzz5HU2LGjblO3AdZij21l1JbDJQ/keacCcQgD6aSIgiqUm9g+GVvzGzCk fTO2TFrsdAg5XmOhekQUfDG+kuc5EW/hDYdUjmCfjTjTxVelTNy8i68p/kggcjw58q11 skuUOlY+/V/mkKs6SK1ngseZqlmXx0hIEDlK8TmRAigFDoj2RdX5LqjqQ4tgMOwW8tNo 1t1A== X-Gm-Message-State: AEkoouulkbJAa6S6EncKitkPjLh56jQpYJs8cdPkfrP91B6G6JKJ/DD2S055nvdKKHiCjw== X-Received: by 10.28.45.65 with SMTP id t62mr8896615wmt.14.1471175007367; Sun, 14 Aug 2016 04:43:27 -0700 (PDT) Received: from gmail.com (pub082136068007.dh-hfc.datazug.ch. [82.136.68.7]) by smtp.gmail.com with ESMTPSA id x133sm11350921wmf.16.2016.08.14.04.43.25 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 14 Aug 2016 04:43:25 -0700 (PDT) Date: Sun, 14 Aug 2016 13:43:23 +0200 From: Peter Senna Tschudin To: Daniel Vetter Subject: Re: [PATCH v3 03/10] drm/imx: atomic phase 1: Use transitional atomic CRTC and plane helpers Message-ID: <20160814114323.GA3890@gmail.com> References: <1467618039-7457-1-git-send-email-gnuiyl@gmail.com> <1467618039-7457-4-git-send-email-gnuiyl@gmail.com> <20160813101154.GA3864@n2100.armlinux.org.uk> <20160813104531.GA4844@n2100.armlinux.org.uk> <20160813112947.GA6718@n2100.armlinux.org.uk> <20160814094413.GX6232@phenom.ffwll.local> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2 (2016-07-01) Cc: Daniel Vetter , Russell King - ARM Linux , dri-devel X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 > > > > > > > > > > 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. 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;