From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=192.198.163.12 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793621164; b=JVO9nrLKZ2tGK0bRbl2BS2MxbTp4VVWhkU8F/8WAmRqWgvx47deKt5pLnCzF9636UUzu eKFBDaKewDa/4D9MFoL4rkO4YbVa/JPxu17Fb9gohRykPbtdhwvsoovIHANrd7wnfnPdd RyT8yVtW5IAJ52Q1QbK53gpO/7FvHksQCPaXDnIMI0iyCIhozMLdSF5oy7cnaxo1ZgLRR YyID9zas91jHoFCLoDyFvN59PSi99xmdtnJv388oxgXsCsPqEbVJNmMSX8848qxmvVyWY NQdo1BVIGeNeUL+1D6ySmSOFwVqkwZ3eGLBK8+ChWwK5aHuE7oEo4z7WV10ed+eGXHw== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793621164; h=DMARC-Filter:DKIM-Signature:X-CSE-ConnectionGUID:X-CSE-MsgGUID: X-IronPort-AV:X-IronPort-AV:Received:X-CSE-ConnectionGUID: X-CSE-MsgGUID:X-ExtLoop1:X-IronPort-AV:Received:Date:From:To:Cc: Subject:Message-ID:References:MIME-Version:Content-Type: Content-Disposition:Content-Transfer-Encoding:In-Reply-To: X-Patchwork-Hint:Organization; bh=Z6vm2PgEAA72YdmGFTnN8V4kyBTWjq0zQHEezeWnIGk=; b=A9G/BW2i0WjEwrFIy07p4jJppSC5slG75NwdSaD2zOiKQMk8ghiJ6iTKgrxFsVNMdyV8 OUSNR6udzCQbEKUdow7UgYqhu1sIv04oDZixu0fxr2kb0CbM4QbM65WTNp5bECS4UiwP8 yUL1hEmUwFFZsJvwlf7KlyoxI+Qo//c06++Iua/h7L393LZl8A8zb6NJDVBB2exdAt1cO MKtaKDGCuP0ZwNHW4jgNjO9Py0HRv/UPfmlohStS3fK+5K3TyA/QgPNHRMFMeCto0zAZW RJoYLsT9Sb70NI5VKT47wzhjlpkfpwJjtzxEkx6+jYkCJhpnPStjGXS/q+EaRkvcs+g== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass header.from=linux.intel.com; spf=pass smtp.mailfrom=intel.com; dkim=pass header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=FyYwYRJj; arc=none smtp.remote-ip=192.198.163.12 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.12]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BFB11606A326 for ; Mon, 2 Nov 2026 12:06:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.2 smtp.subspace.kernel.org BFB11606A326 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=intel.com header.i=@intel.com header.a=rsa-sha256 header.s=Intel header.b=FyYwYRJj DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1793621163; x=1825157163; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=ncuBlp/vjWSskm6RKhx8Q7dAMfd8B7ZAd2luPtRcmgk=; b=FyYwYRJjgoS22DqmWwCcg42j8EosMTzUSjvlNYyLqr1FfAD0Lnrtsfm8 Hkxl6qziz8QqEtBSSSQtyLocakY4BWG7pB7v3DTZB6Nuuj0/f3V0my8yC HVm0GjdrPBNeRb5CpxrS4ZGm/lyQEJ9HxSpRt7n6Pxn6uMjcdEbNiBMUq QOXnk388qoARw+dWYyaE+jNm2VEKHiLyTI6crNUC5+MvIk/vVzYN5p+qq TrngFdnC4+9pEwZKBzyBvYvM8ppbM1yyvrNR+hlqJJJj/zIbbxi8kUa3C GvdqulayfFe9NIqk9x+XfupQupYp279Ky9aY2RGpIl8pHJ7jTmTSMOFf7 w==; X-CSE-ConnectionGUID: m5CtqY7VRFO6rG73yqtZAg== X-CSE-MsgGUID: soHmjMTMRWClboigpVymNw== X-IronPort-AV: E=McAfee;i="6800,10657,11953"; a="2203695" X-IronPort-AV: E=Sophos;i="6.27,191,1787036400"; d="scan'208";a="2203695" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by fmvoesa106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Nov 2026 04:06:03 -0800 X-CSE-ConnectionGUID: 9ZWO4pEeTcmgI6T3jsBCsA== X-CSE-MsgGUID: 2DVx2ONiT/2bWxje6/1zYA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.27,191,1787036400"; d="scan'208";a="6275782" Received: from hrotuna-mobl2.ger.corp.intel.com (HELO localhost) ([10.245.245.10]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 02 Nov 2026 04:05:59 -0800 Date: Mon, 2 Nov 2026 14:05:57 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Hugo Sales Cc: maarten.lankhorst@linux.intel.com, mripard@kernel.org, tzimmermann@suse.de, dri-devel@lists.freedesktop.org, airlied@gmail.com, simona@ffwll.ch, harry.wentland@amd.com, amd-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/probe-helper: Sort and set crtcinfo before dropping the modeset locks Message-ID: References: <20261031161952.660049-1-hugo@hsal.es> X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20261031161952.660049-1-hugo@hsal.es> X-Patchwork-Hint: comment Organization: Intel Finland Oy - BIC 0357606-4 - c/o Alberga Business Park, 6 krs Bertel Jungin Aukio 5, 02600 Espoo, Finland On Sat, Oct 31, 2026 at 04:19:52PM +0000, Hugo Sales wrote: > Currently, drm_helper_probe_single_connector_modes() holds > connection_mutex while it probes and prunes connector->modes, then drops > it before sorting the list and filling in the crtc_* fields of each > mode. The caller still holds mode_config.mutex at that point, but > drm_atomic_check_only() never takes it. The only lock it holds on the > connector is connection_mutex, which drm_atomic_get_connector_state() > takes for it > > amdgpu walks connector->modes from its atomic_check, in That is just wrong. connector->modes is protected by mode_config.mutex The fact that amdgpu apparently uses scaling for external displays is a very odd as well. No other driver does that for external displays. That said, I've thought about adding that for i915/xe, but I think it should be done via some uapi so that userspace retains control over the actual display timings. I'd really prefer to do it that way for laptop panels as well, but the old uapi is already carved in stone there so can't change it :( > create_stream_for_sink() and amdgpu_dm_get_highest_refresh_rate_mode(), > holding only the locks of the atomic acquire context. list_sort() > NULL-terminates the list while it runs, so that walk can follow a NULL > next pointer or read a mode whose crtc_* fields are being rewritten > > Sort the list and set crtcinfo before dropping the locks, as the > function already does when it probes and prunes the modes. As far as I > can tell, the unlock ended up in front of the sort when > commit 6c5ed5ae353c ("drm/atomic: Acquire connection_mutex lock in drm_helper_probe_single_connector_modes, v4.") > first took connection_mutex here, and that commit gives no reason for > the order > > Signed-off-by: Hugo Sales > --- > drivers/gpu/drm/drm_probe_helper.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 856b40fdaac4a..2085d93651b21 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -703,11 +703,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > } > > exit: > - drm_modeset_drop_locks(&ctx); > - drm_modeset_acquire_fini(&ctx); > - > - if (list_empty(&connector->modes)) > - return 0; > + if (list_empty(&connector->modes)) { > + count = 0; > + goto unlock; > + } > > drm_mode_sort(&connector->modes); > > @@ -720,6 +719,10 @@ int drm_helper_probe_single_connector_modes(struct drm_connector *connector, > DRM_MODE_ARG(mode)); > } > > +unlock: > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); I suppose we aren't moving all that much work under the lock here, so maybe fine. Though I kinda hate that we even have to hold the lock across the mode validation. But as long as we're looking at the connector state in connector.mode_valid() I suppose there is no choice :/ Any new locking rules would need to be properly documented. And regardless of changing the locking rules or not, someone should probably sprinkle lockdep asserts to bunch of places where connector->modes is consulted. Perhaps even introduce custom list iterator macros that include the assert... > + > return count; > } > EXPORT_SYMBOL(drm_helper_probe_single_connector_modes); > -- > 2.56.0 -- Ville Syrjälä Intel