Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 10 Oct 2018 21:32:24 +0300
From:   Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To:     Lyude Paul <lyude@redhat.com>
Cc:     intel-gfx@lists.freedesktop.org,
        Daniel Vetter <daniel.vetter@ffwll.ch>, stable@vger.kernel.org,
        Gustavo Padovan <gustavo@padovan.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/atomic_helper: Allow DPMS On<->Off changes for
 unregistered connectors
Message-ID: <20181010183224.GS9144@intel.com>
References: <20181009204424.21462-1-lyude@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20181009204424.21462-1-lyude@redhat.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Oct 09, 2018 at 04:44:24PM -0400, Lyude Paul wrote:
> It appears when testing my previous fix for some of the legacy
> modesetting issues with MST, I misattributed some kernel splats that
> started appearing on my machine after a rebase as being from upstream.
> But it appears they actually came from my patch series:
> 
> [    2.980512] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] Updating routing for [CONNECTOR:65:eDP-1]
> [    2.980516] [drm:drm_atomic_helper_check_modeset [drm_kms_helper]] [CONNECTOR:65:eDP-1] is not registered
> [    2.980516] ------------[ cut here ]------------
> [    2.980519] Could not determine valid watermarks for inherited state
> [    2.980553] WARNING: CPU: 3 PID: 551 at drivers/gpu/drm/i915/intel_display.c:14983 intel_modeset_init+0x14d7/0x19f0 [i915]
> [    2.980556] Modules linked in: i915(O+) i2c_algo_bit drm_kms_helper(O) syscopyarea sysfillrect sysimgblt fb_sys_fops drm(O) intel_rapl x86_pkg_temp_thermal iTCO_wdt wmi_bmof coretemp crc32_pclmul psmouse i2c_i801 mei_me mei i2c_core lpc_ich mfd_core tpm_tis tpm_tis_core wmi tpm thinkpad_acpi pcc_cpufreq video ehci_pci crc32c_intel serio_raw ehci_hcd xhci_pci xhci_hcd
> [    2.980577] CPU: 3 PID: 551 Comm: systemd-udevd Tainted: G           O      4.19.0-rc7Lyude-Test+ #1
> [    2.980579] Hardware name: LENOVO 20BWS1KY00/20BWS1KY00, BIOS JBET63WW (1.27 ) 11/10/2016
> [    2.980605] RIP: 0010:intel_modeset_init+0x14d7/0x19f0 [i915]
> [    2.980607] Code: 89 df e8 ec 27 02 00 e9 24 f2 ff ff be 03 00 00 00 48 89 df e8 da 27 02 00 e9 26 f2 ff ff 48 c7 c7 c8 d1 34 a0 e8 23 cf dc e0 <0f> 0b e9 7c fd ff ff f6 c4 04 0f 85 37 f7 ff ff 48 8b 83 60 08 00
> [    2.980611] RSP: 0018:ffffc90000287988 EFLAGS: 00010282
> [    2.980614] RAX: 0000000000000000 RBX: ffff88031b488000 RCX: 0000000000000006
> [    2.980617] RDX: 0000000000000007 RSI: 0000000000000086 RDI: ffff880321ad54d0
> [    2.980620] RBP: ffffc90000287a10 R08: 000000000000040a R09: 0000000000000065
> [    2.980623] R10: ffff88030ebb8f00 R11: ffffffff81416590 R12: ffff88031b488000
> [    2.980626] R13: ffff88031b4883a0 R14: ffffc900002879a8 R15: ffff880319099800
> [    2.980630] FS:  00007f475620d180(0000) GS:ffff880321ac0000(0000) knlGS:0000000000000000
> [    2.980633] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [    2.980636] CR2: 00007f9ef28018a0 CR3: 000000031b72c001 CR4: 00000000003606e0
> [    2.980639] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [    2.980642] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [    2.980645] Call Trace:
> [    2.980675]  i915_driver_load+0xb0e/0xdc0 [i915]
> [    2.980681]  ? kernfs_add_one+0xe7/0x130
> [    2.980709]  i915_pci_probe+0x46/0x60 [i915]
> [    2.980715]  pci_device_probe+0xd4/0x150
> [    2.980719]  really_probe+0x243/0x3b0
> [    2.980722]  driver_probe_device+0xba/0x100
> [    2.980726]  __driver_attach+0xe4/0x110
> [    2.980729]  ? driver_probe_device+0x100/0x100
> [    2.980733]  bus_for_each_dev+0x74/0xb0
> [    2.980736]  driver_attach+0x1e/0x20
> [    2.980739]  bus_add_driver+0x159/0x230
> [    2.980743]  ? 0xffffffffa0393000
> [    2.980746]  driver_register+0x70/0xc0
> [    2.980749]  ? 0xffffffffa0393000
> [    2.980753]  __pci_register_driver+0x57/0x60
> [    2.980780]  i915_init+0x55/0x58 [i915]
> [    2.980785]  do_one_initcall+0x4a/0x1c4
> [    2.980789]  ? do_init_module+0x27/0x210
> [    2.980793]  ? kmem_cache_alloc_trace+0x131/0x190
> [    2.980797]  do_init_module+0x60/0x210
> [    2.980800]  load_module+0x2063/0x22e0
> [    2.980804]  ? vfs_read+0x116/0x140
> [    2.980807]  ? vfs_read+0x116/0x140
> [    2.980811]  __do_sys_finit_module+0xbd/0x120
> [    2.980814]  ? __do_sys_finit_module+0xbd/0x120
> [    2.980818]  __x64_sys_finit_module+0x1a/0x20
> [    2.980821]  do_syscall_64+0x5a/0x110
> [    2.980824]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [    2.980826] RIP: 0033:0x7f4754e32879
> [    2.980828] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d f7 45 2c 00 f7 d8 64 89 01 48
> [    2.980831] RSP: 002b:00007fff43fd97d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> [    2.980834] RAX: ffffffffffffffda RBX: 0000559a44ca64f0 RCX: 00007f4754e32879
> [    2.980836] RDX: 0000000000000000 RSI: 00007f475599f4cd RDI: 0000000000000018
> [    2.980838] RBP: 00007f475599f4cd R08: 0000000000000000 R09: 0000000000000000
> [    2.980839] R10: 0000000000000018 R11: 0000000000000246 R12: 0000000000000000
> [    2.980841] R13: 0000559a44c92fd0 R14: 0000000000020000 R15: 0000000000000000
> [    2.980881] WARNING: CPU: 3 PID: 551 at drivers/gpu/drm/i915/intel_display.c:14983 intel_modeset_init+0x14d7/0x19f0 [i915]
> [    2.980884] ---[ end trace 5eb47a76277d4731 ]---
> 
> The cause of this appears to be due to the fact that if there's
> pre-existing display state that was set by the BIOS when i915 loads, it
> will attempt to perform a modeset before the driver is registered with
> userspace. Since this happens before the driver's registered with
> userspace, it's connectors are also unregistered and thus-states which
> would turn on DPMS on a connector end up getting rejected since the
> connector isn't registered.
> 
> These bugs managed to get past Intel's CI partially due to the fact it
> never ran a full test on my patches for some reason, but also because
> all of the tests unload the GPU once before running.

Well, not all all tests, but all the module reload tests. If we can
change the setup so that the driver isn't loaded until the first test
executes we should be able catch all these issues as well. Which would
be nice.

> Since this bug is
> only really triggered when the drivers tries to perform a modeset before
> it's been fully registered with userspace when coming from whatever
> display configuration the firmware left us with, it likely would never
> have been picked up by CI in the first place.
> 
> After some discussion with vsyrjala, we decided the best course of
> action would be to just move the unregistered connector checks out of
> update_connector_routing() and into drm_atomic_set_crtc_for_connector().
> The reason for this being that legacy modesetting isn't going to be
> expecting failures anywhere (at least this is the case with X), so
> ideally we want to ensure that any DPMS changes will still work even on
> unregistered connectors. Instead, we now only reject new modesets which
> would change the current CRTC assigned to an unregistered connector
> unless no new CRTC is being assigned to replace the connector's previous
> one.
> 
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reported-by: Ville Syrj?l? <ville.syrjala@linux.intel.com>
> Fixes: 4d80273976bf ("drm/atomic_helper: Disallow new modesets on unregistered connectors")
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrj?l? <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 21 +--------------------
>  drivers/gpu/drm/drm_atomic_uapi.c   | 21 +++++++++++++++++++++
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e6a2cf72de5e..6f66777dca4b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -319,26 +319,6 @@ update_connector_routing(struct drm_atomic_state *state,
>  		return 0;
>  	}
>  
> -	crtc_state = drm_atomic_get_new_crtc_state(state,
> -						   new_connector_state->crtc);
> -	/*
> -	 * For compatibility with legacy users, we want to make sure that
> -	 * we allow DPMS On->Off modesets on unregistered connectors. Modesets
> -	 * which would result in anything else must be considered invalid, to
> -	 * avoid turning on new displays on dead connectors.
> -	 *
> -	 * Since the connector can be unregistered at any point during an
> -	 * atomic check or commit, this is racy. But that's OK: all we care
> -	 * about is ensuring that userspace can't do anything but shut off the
> -	 * display on a connector that was destroyed after its been notified,
> -	 * not before.
> -	 */
> -	if (!READ_ONCE(connector->registered) && crtc_state->active) {
> -		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> -				 connector->base.id, connector->name);
> -		return -EINVAL;
> -	}
> -
>  	funcs = connector->helper_private;
>  
>  	if (funcs->atomic_best_encoder)
> @@ -383,6 +363,7 @@ update_connector_routing(struct drm_atomic_state *state,
>  
>  	set_best_encoder(state, new_connector_state, new_encoder);
>  
> +	crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
>  	crtc_state->connectors_changed = true;
>  
>  	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] using [ENCODER:%d:%s] on [CRTC:%d:%s]\n",
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index d5b7f315098c..7acec863b10c 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -299,6 +299,27 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  	struct drm_connector *connector = conn_state->connector;
>  	struct drm_crtc_state *crtc_state;
>  
> +	/*
> +	 * For compatibility with legacy users, we want to make sure that
> +	 * we allow DPMS On<->Off modesets on unregistered connectors, since
> +	 * legacy modesetting users will not be expecting these to fail. We do
> +	 * not however, want to allow legacy users to assign a connector
> +	 * that's been unregistered from sysfs to another CRTC, since doing
> +	 * this with a now non-existant connector could potentially leave us
> +	 * in an invalid state.
> +	 *
> +	 * Since the connector can be unregistered at any point during an
> +	 * atomic check or commit, this is racy. But that's OK: all we care
> +	 * about is ensuring that userspace can't use this connector for new
> +	 * configurations after it's been notified that the connector is no
> +	 * longer present.
> +	 */
> +	if (!READ_ONCE(connector->registered) && crtc) {
> +		DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] is not registered\n",
> +				 connector->base.id, connector->name);
> +		return -EINVAL;

I was also pondering whether we should make this ENOENT to match the
errno we use for the "this connector doesn't exist" case. But I guess
it doesn't really matter.

Reviewed-by: Ville Syrj?l? <ville.syrjala@linux.intel.com>

> +	}
> +
>  	if (conn_state->crtc == crtc)
>  		return 0;
>  
> -- 
> 2.17.1

-- 
Ville Syrj?l?
Intel