From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C779ED0C5F9 for ; Fri, 25 Oct 2024 10:13:44 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 80F9410E02C; Fri, 25 Oct 2024 10:13:44 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=intel.com header.i=@intel.com header.b="UUDPwmit"; dkim-atps=neutral Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.16]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4A3E310EA69 for ; Fri, 25 Oct 2024 10:13:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1729851223; x=1761387223; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=bhDjl6880OeAG7cmqR240W4SnMN+ZHCyhXTixXipGz0=; b=UUDPwmitU4DaWPh81YXzX3onvrzVSWOHCGO7QnwtnqX16BmlkvP32G96 oCaeGNNjspQYUNDBjgO5E2lo6w0mlSMkoNv6Rb95nFLHKZhT6mc/zjwEj vzTyIjAyAy6py4irhhLoo2aVzzW48I8N+SCf4L1BirrqcFc1ZTUfZM/V2 h7UIHx9cygSWGDAdVZPp/aQoja9mv43hmS3kV5gVm0agVhulTCr3yfxff myrp0uu4mPbD+i/cdeWkWSYpiy1tkQpuF6CfOxDo5dasIE9YDSkevGG51 I6/nWjeljGFDA10a4sH6jmG+ohDcitkbdMLHKzzEQNiOPvM7//y4O7Ck6 A==; X-CSE-ConnectionGUID: xOPVaFzlQGazRBtjynk7EQ== X-CSE-MsgGUID: GmLqluEVRE+lxx58p4H3Ag== X-IronPort-AV: E=McAfee;i="6700,10204,11235"; a="17146517" X-IronPort-AV: E=Sophos;i="6.11,231,1725346800"; d="scan'208";a="17146517" Received: from fmviesa008.fm.intel.com ([10.60.135.148]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Oct 2024 03:13:43 -0700 X-CSE-ConnectionGUID: v9/6XzBTThijU6VQ7AQaDg== X-CSE-MsgGUID: oDi8vR36RuSm1YUUVcRigg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.11,231,1725346800"; d="scan'208";a="80985292" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by fmviesa008.fm.intel.com with SMTP; 25 Oct 2024 03:13:40 -0700 Received: by stinkbox (sSMTP sendmail emulation); Fri, 25 Oct 2024 13:13:39 +0300 Date: Fri, 25 Oct 2024 13:13:39 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Dmitry Baryshkov Cc: igt-dev@lists.freedesktop.org, Maxime Ripard Subject: Re: [PATCH i-g-t v2] tests/kms_properties: rework immutability checks Message-ID: References: <20241018120058.42240-1-dmitry.baryshkov@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Patchwork-Hint: comment X-BeenThere: igt-dev@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development mailing list for IGT GPU Tools List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" On Fri, Oct 25, 2024 at 12:53:29PM +0300, Dmitry Baryshkov wrote: > On Fri, 25 Oct 2024 at 08:51, Ville Syrjälä > wrote: > > > > On Thu, Oct 24, 2024 at 09:29:03PM +0300, Dmitry Baryshkov wrote: > > > On Fri, 18 Oct 2024 at 15:01, Dmitry Baryshkov > > > wrote: > > > > > > > > Following the discussion on IRC, it is actually an error to require that > > > > properties that can not be chaged are marked as immutable. > > > > > > > > First of all, it creates inconsistent uAPI. Some drivers might have an > > > > immutable property, while others will have it mutable. Yes, there are > > > > known examples for such behaviour (e.g. zpos), but they are clearly > > > > documented in this way. > > > > > > > > Second, by the nature of the flag, the DRM_MODE_PROP_IMMUTABLE defines > > > > more of the 'direction' of the property (whether it is set by the kernel > > > > or it is expected to be set by the userspace) rather than simply states > > > > that there is no way for the userspace to change the property. > > > > > > > > Rework the immutability checks to verify that the properties defined as > > > > immutable have this flag set. Keep the "immutable if single value" > > > > property just for the "zpos" property. > > > > > > > > Fixes: 29ae12bd764e ("tests/kms_properties: Validate properties harder") > > > > Cc: Ville Syrjälä > > > > Link: https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622 > > > > Signed-off-by: Dmitry Baryshkov > > > > > > Gracious ping for the patch. We need it to be able to proceed with the > > > HDMI rework for the drm/msm driver, otherwise IGT tests fail. > > > > > > > > > > > --- > > > > Changes since v1: > > > > - Moved GAMMA_LUT_SIZE and DEGAMMA_LUT_SIZE to DRM_MODE_OBJECT_CRTC. > > > > - Added debug print to help debugging possible issues. > > > > --- > > > > tests/kms_properties.c | 110 ++++++++++++++++++++++++++++++++--------- > > > > 1 file changed, 87 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/tests/kms_properties.c b/tests/kms_properties.c > > > > index a93c93cccf64..57f07e69909a 100644 > > > > --- a/tests/kms_properties.c > > > > +++ b/tests/kms_properties.c > > > > @@ -416,16 +416,80 @@ static void test_object_invalid_properties(igt_display_t *display, > > > > test_invalid_properties(display->drm_fd, id, type, output->id, DRM_MODE_OBJECT_CONNECTOR, atomic); > > > > } > > > > > > > > +enum prop_imm_flags { > > > > + IMMUTABLE_REQ, > > > > + IMMUTABLE_IF_SINGLE_VALUE, > > > > +}; > > > > + > > > > +static const struct { > > > > + uint32_t obj_type; > > > > + const char *name; > > > > + enum prop_imm_flags flags; > > > > +} prop_settings[] = { > > > > + /* generic */ > > > > + { DRM_MODE_OBJECT_CONNECTOR, "EDID", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "PATH", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "TILE", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "WRITEBACK_PIXEL_FORMATS", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "non-desktop", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "panel orientation" ,IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "privacy-screen hw-state", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "subconnector", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "suggested X", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "suggested Y", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CONNECTOR, "vrr_capable", IMMUTABLE_REQ }, > > > > + > > > > + { DRM_MODE_OBJECT_CRTC, "DEGAMMA_LUT_SIZE", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_CRTC, "GAMMA_LUT_SIZE", IMMUTABLE_REQ }, > > > > + > > > > + { DRM_MODE_OBJECT_PLANE, "IN_FORMATS", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_PLANE, "SIZE_HINTS", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_PLANE, "type", IMMUTABLE_REQ }, > > > > + { DRM_MODE_OBJECT_PLANE, "zpos", IMMUTABLE_IF_SINGLE_VALUE }, > > > > + > > > > + /* driver-specific */ > > > > + { DRM_MODE_OBJECT_CONNECTOR, "hotplug_mode_update", IMMUTABLE_REQ }, // qxl, vmwgfx > > > > + { DRM_MODE_OBJECT_CONNECTOR, "implicit_placement", IMMUTABLE_REQ }, // vmwgfx > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_BLEND_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_DEGAMMA_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_LUT3D_SIZE", IMMUTABLE_REQ }, // amdgpu > > > > + { DRM_MODE_OBJECT_PLANE, "AMD_PLANE_SHAPER_LUT_SIZE", IMMUTABLE_REQ }, // amdgpu > > > > +}; > > > > Not really a fan of having a list like this. All of these look > > like they are just regular immutable properties, with zpos being > > the only exception. Or is that not the case? > > Yes. The goal is to perform the ABI check: which properties are > documented to be immutable. All other properties are expected to be > mutable. > > > > > What was the problem of just dropping the check that non-immutable > > properties must have multiple possible values? > > See the discussion at [1] and then the response by Sima [2] to the > patch similar to your proposal. > > [1] https://oftc.irclog.whitequark.org/dri-devel/2024-07-16#33374622 > [2] https://lore.kernel.org/igt-dev/Zpjn2dTHDrBBuTVH@phenom.ffwll.local/ I'm not seeing any real conclusions there. Looks to me like the correct thing would be to just remove that single value => immutable assumption. And I'd follow that up with a kernel patch to make zpos non-immutable. I'm thinking this shouldn't break anything since the property can already be non-immutable. And checking that all standard properties look sane should probably be a new subtest, and it should validate more than the immutable bit. > > > > And perhaps there should be instead a check that immutable > > properties must not have more than one possible value? > > This is not a correct assumption, immutable props might have any > number of values. The difference is on semantics side: the driver > controls immutable properties, userspace controls mutable ones. Hmm, yeah I suppose one could use it eg. to relay back some sink capabilities as an enum property. -- Ville Syrjälä Intel