From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from wnew1-smtp.messagingengine.com (wnew1-smtp.messagingengine.com [64.147.123.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 22DE8186A for ; Thu, 27 Oct 2022 09:37:39 +0000 (UTC) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.west.internal (Postfix) with ESMTP id E0A312B066EB; Thu, 27 Oct 2022 05:37:34 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 27 Oct 2022 05:37:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm3; t=1666863454; x=1666870654; bh=5ItCVshj6J yZau2J5cjH0M8JHw139GrYlnIerAQcZWs=; b=IVFSYrra2yRKDBScb6i+EPPsnR H7iUD7VkPJAKE695dyeIFHVvDzTnEOFd/PuRkXJYQDLL3GqO5VDnC18zyG53l/7I pvgnmLqeiBPBWXQjApcUVHOrKRHilbj5+ASZsv6ndq61A0vf5cAG6GPGNStKiCRy wer2bctGBN1o9JRLtzfw8j3SxBth2GhmC1AF3SbqCoOAk5ikvGienxjeMuTVnd3b CIYEdhLoJTnMe4apOVtrct3AJygLwtX9QefTOEjFMks85VRv9W7SGo+EMXE4ei+c /vSdG+pF0qpFn1D0F3ffTOrvCiLzPaO3Iv99pS2pxcK/5ssmEIuPwA72joMw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1666863454; x=1666870654; bh=5ItCVshj6JyZau2J5cjH0M8JHw13 9GrYlnIerAQcZWs=; b=aMBp3hE4BgyODOr6WOBsdolHjS2Onrihiev6Ff/sAjQh Cama2t5zFVnEKp63jv9ugJbpKhvl71qc+L5qSL1TZPihHUka8luZ1042BOHS5Sxh aMKHX7Ay1bGb29oD5v8DX8oZBPKSAoX9G4RJblIy3UNVtu3bbfNqg1CYVaEHxNJc pHr9LW/SlL0Xhx1KndHWOx5hpeWVSz48zu40JY3Je6zDtTTEvFuI+nyPVFLcWWtY eqbDGYXUYbYyBSfExtphfrsas5tR+XzSbiwxPrJVDiXhOZHeBFTCAEXgpwFpx+Y+ pod1LrzbN+UsFQjTvFLBJJEhMjbUc60CIEflbK3G1Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrtdeggdduiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepteefffefgfektdefgfeludfgtdejfeejvddttdekteeiffejvdfgheehfffh vedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 27 Oct 2022 05:37:32 -0400 (EDT) Date: Thu, 27 Oct 2022 11:37:30 +0200 From: Maxime Ripard To: Mateusz Kwiatkowski Cc: Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Joonas Lahtinen , Lyude Paul , linux-sunxi@lists.linux.dev, intel-gfx@lists.freedesktop.org, Phil Elwell , linux-arm-kernel@lists.infradead.org, nouveau@lists.freedesktop.org, Hans de Goede , Dom Cobley , dri-devel@lists.freedesktop.org, Dave Stevenson , linux-kernel@vger.kernel.org, Noralf =?utf-8?Q?Tr=C3=B8nnes?= , Geert Uytterhoeven Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper Message-ID: <20221027093730.tb4oaissdapf6ifr@houat> References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="xamrv4lbhqfdsxuo" Content-Disposition: inline In-Reply-To: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> --xamrv4lbhqfdsxuo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Mateusz, On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote: > First of all, nice idea with the helper function that can be reused by > different drivers. This is neat! Yeah, it looked to me that given how complex it is, we don't want to duplicate it in each and every driver. > But looking at this function, it feels a bit overcomplicated. You're > creating the two modes, If reported as supported by the connector, yes. > then checking which one is the default, then set the preferred one and > possibly reorder them. Maybe it can be simplified somehow? Possibly, but I couldn't find something simpler. We should only expose the modes that the driver reports as supported, so we can have 0-2 modes. Then the preferred flag needs to be set on the default one like you suggested. But also, EDIDs define the preferred mode as either the mode with the flag set or the first mode listed. So a lot of program just use the heuristic to just pick the first mode listed. So it might be that I'm too careful, but it still seems useful to me. > Although when I tried to refactor it myself, I ended up with something th= at's > not better at all. Maybe it needs to be complicated, after all :( Yeah, that was my conclusion too :/ > Anyway, the current version seems to have a couple of bugs: >=20 > > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > > + mode =3D drm_mode_analog_pal_576i(connector->dev); > > + if (!mode) > > + return 0; > > + > > + tv_modes[count++] =3D mode; > > + } >=20 > If the 480i mode has been created properly, but there's an error creating= the > 576i one (we enter the if (!mode) clause), the 480i mode will leak. >=20 > > + if (count =3D=3D 1) { >=20 > You're handling the count =3D=3D 1 case specially, but if count =3D=3D 0,= the rest of > the code will assume that two modes exist and probably segfault in the pr= ocess. >=20 > > + ret =3D drm_object_property_get_default_value(&connector->base, > > + dev->mode_config.tv_mode_property, > > + &default_mode); > > + if (ret) > > + return 0; > > + > > + if (cmdline->tv_mode_specified) > > + default_mode =3D cmdline->tv_mode; >=20 > In case of an error (ret !=3D 0), the modes created so far in the tv_mode= s array > will leak. Thanks for the review, I'll fix these bugs > Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should= go > first? If we're going to use the default from cmdline, there's no point i= n even > querying the property default value. Maybe, I don't know. I find the flow of the code more readable that way, but if you disagree I'll change it. Maxime --xamrv4lbhqfdsxuo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY1pRWgAKCRDj7w1vZxhR xTVQAQCdqQ7+vEr0O0pC7P93kHAYwFz0oBZn90Ip4EjyExKRPgD/cMwjjUZ6daWo /idlovksY5X1UdblxYiDcBffKZ9k8gM= =nRSG -----END PGP SIGNATURE----- --xamrv4lbhqfdsxuo-- 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 9C16EC38A2D for ; Thu, 27 Oct 2022 09:38:04 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5054B10E5B1; Thu, 27 Oct 2022 09:37:49 +0000 (UTC) Received: from wnew1-smtp.messagingengine.com (wnew1-smtp.messagingengine.com [64.147.123.26]) by gabe.freedesktop.org (Postfix) with ESMTPS id B2A4410E59E; Thu, 27 Oct 2022 09:37:42 +0000 (UTC) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.west.internal (Postfix) with ESMTP id E0A312B066EB; Thu, 27 Oct 2022 05:37:34 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 27 Oct 2022 05:37:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm3; t=1666863454; x=1666870654; bh=5ItCVshj6J yZau2J5cjH0M8JHw139GrYlnIerAQcZWs=; b=IVFSYrra2yRKDBScb6i+EPPsnR H7iUD7VkPJAKE695dyeIFHVvDzTnEOFd/PuRkXJYQDLL3GqO5VDnC18zyG53l/7I pvgnmLqeiBPBWXQjApcUVHOrKRHilbj5+ASZsv6ndq61A0vf5cAG6GPGNStKiCRy wer2bctGBN1o9JRLtzfw8j3SxBth2GhmC1AF3SbqCoOAk5ikvGienxjeMuTVnd3b CIYEdhLoJTnMe4apOVtrct3AJygLwtX9QefTOEjFMks85VRv9W7SGo+EMXE4ei+c /vSdG+pF0qpFn1D0F3ffTOrvCiLzPaO3Iv99pS2pxcK/5ssmEIuPwA72joMw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1666863454; x=1666870654; bh=5ItCVshj6JyZau2J5cjH0M8JHw13 9GrYlnIerAQcZWs=; b=aMBp3hE4BgyODOr6WOBsdolHjS2Onrihiev6Ff/sAjQh Cama2t5zFVnEKp63jv9ugJbpKhvl71qc+L5qSL1TZPihHUka8luZ1042BOHS5Sxh aMKHX7Ay1bGb29oD5v8DX8oZBPKSAoX9G4RJblIy3UNVtu3bbfNqg1CYVaEHxNJc pHr9LW/SlL0Xhx1KndHWOx5hpeWVSz48zu40JY3Je6zDtTTEvFuI+nyPVFLcWWtY eqbDGYXUYbYyBSfExtphfrsas5tR+XzSbiwxPrJVDiXhOZHeBFTCAEXgpwFpx+Y+ pod1LrzbN+UsFQjTvFLBJJEhMjbUc60CIEflbK3G1Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrtdeggdduiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepteefffefgfektdefgfeludfgtdejfeejvddttdekteeiffejvdfgheehfffh vedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 27 Oct 2022 05:37:32 -0400 (EDT) Date: Thu, 27 Oct 2022 11:37:30 +0200 From: Maxime Ripard To: Mateusz Kwiatkowski Message-ID: <20221027093730.tb4oaissdapf6ifr@houat> References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="xamrv4lbhqfdsxuo" Content-Disposition: inline In-Reply-To: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Subject: Re: [Nouveau] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper X-BeenThere: nouveau@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Nouveau development list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , nouveau@lists.freedesktop.org, Joonas Lahtinen , dri-devel@lists.freedesktop.org, Phil Elwell , Emma Anholt , Samuel Holland , Jernej Skrabec , Chen-Yu Tsai , Geert Uytterhoeven , Ben Skeggs , linux-sunxi@lists.linux.dev, Daniel Vetter , intel-gfx@lists.freedesktop.org, Maarten Lankhorst , Jani Nikula , Hans de Goede , Rodrigo Vivi , linux-arm-kernel@lists.infradead.org, Tvrtko Ursulin , Dom Cobley , linux-kernel@vger.kernel.org, Noralf =?utf-8?Q?Tr=C3=B8nnes?= Errors-To: nouveau-bounces@lists.freedesktop.org Sender: "Nouveau" --xamrv4lbhqfdsxuo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Mateusz, On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote: > First of all, nice idea with the helper function that can be reused by > different drivers. This is neat! Yeah, it looked to me that given how complex it is, we don't want to duplicate it in each and every driver. > But looking at this function, it feels a bit overcomplicated. You're > creating the two modes, If reported as supported by the connector, yes. > then checking which one is the default, then set the preferred one and > possibly reorder them. Maybe it can be simplified somehow? Possibly, but I couldn't find something simpler. We should only expose the modes that the driver reports as supported, so we can have 0-2 modes. Then the preferred flag needs to be set on the default one like you suggested. But also, EDIDs define the preferred mode as either the mode with the flag set or the first mode listed. So a lot of program just use the heuristic to just pick the first mode listed. So it might be that I'm too careful, but it still seems useful to me. > Although when I tried to refactor it myself, I ended up with something th= at's > not better at all. Maybe it needs to be complicated, after all :( Yeah, that was my conclusion too :/ > Anyway, the current version seems to have a couple of bugs: >=20 > > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > > + mode =3D drm_mode_analog_pal_576i(connector->dev); > > + if (!mode) > > + return 0; > > + > > + tv_modes[count++] =3D mode; > > + } >=20 > If the 480i mode has been created properly, but there's an error creating= the > 576i one (we enter the if (!mode) clause), the 480i mode will leak. >=20 > > + if (count =3D=3D 1) { >=20 > You're handling the count =3D=3D 1 case specially, but if count =3D=3D 0,= the rest of > the code will assume that two modes exist and probably segfault in the pr= ocess. >=20 > > + ret =3D drm_object_property_get_default_value(&connector->base, > > + dev->mode_config.tv_mode_property, > > + &default_mode); > > + if (ret) > > + return 0; > > + > > + if (cmdline->tv_mode_specified) > > + default_mode =3D cmdline->tv_mode; >=20 > In case of an error (ret !=3D 0), the modes created so far in the tv_mode= s array > will leak. Thanks for the review, I'll fix these bugs > Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should= go > first? If we're going to use the default from cmdline, there's no point i= n even > querying the property default value. Maybe, I don't know. I find the flow of the code more readable that way, but if you disagree I'll change it. Maxime --xamrv4lbhqfdsxuo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY1pRWgAKCRDj7w1vZxhR xTVQAQCdqQ7+vEr0O0pC7P93kHAYwFz0oBZn90Ip4EjyExKRPgD/cMwjjUZ6daWo /idlovksY5X1UdblxYiDcBffKZ9k8gM= =nRSG -----END PGP SIGNATURE----- --xamrv4lbhqfdsxuo-- 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 159F1FA373E for ; Thu, 27 Oct 2022 09:37:52 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2C3C910E59E; Thu, 27 Oct 2022 09:37:48 +0000 (UTC) Received: from wnew1-smtp.messagingengine.com (wnew1-smtp.messagingengine.com [64.147.123.26]) by gabe.freedesktop.org (Postfix) with ESMTPS id B2A4410E59E; Thu, 27 Oct 2022 09:37:42 +0000 (UTC) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.west.internal (Postfix) with ESMTP id E0A312B066EB; Thu, 27 Oct 2022 05:37:34 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 27 Oct 2022 05:37:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm3; t=1666863454; x=1666870654; bh=5ItCVshj6J yZau2J5cjH0M8JHw139GrYlnIerAQcZWs=; b=IVFSYrra2yRKDBScb6i+EPPsnR H7iUD7VkPJAKE695dyeIFHVvDzTnEOFd/PuRkXJYQDLL3GqO5VDnC18zyG53l/7I pvgnmLqeiBPBWXQjApcUVHOrKRHilbj5+ASZsv6ndq61A0vf5cAG6GPGNStKiCRy wer2bctGBN1o9JRLtzfw8j3SxBth2GhmC1AF3SbqCoOAk5ikvGienxjeMuTVnd3b CIYEdhLoJTnMe4apOVtrct3AJygLwtX9QefTOEjFMks85VRv9W7SGo+EMXE4ei+c /vSdG+pF0qpFn1D0F3ffTOrvCiLzPaO3Iv99pS2pxcK/5ssmEIuPwA72joMw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1666863454; x=1666870654; bh=5ItCVshj6JyZau2J5cjH0M8JHw13 9GrYlnIerAQcZWs=; b=aMBp3hE4BgyODOr6WOBsdolHjS2Onrihiev6Ff/sAjQh Cama2t5zFVnEKp63jv9ugJbpKhvl71qc+L5qSL1TZPihHUka8luZ1042BOHS5Sxh aMKHX7Ay1bGb29oD5v8DX8oZBPKSAoX9G4RJblIy3UNVtu3bbfNqg1CYVaEHxNJc pHr9LW/SlL0Xhx1KndHWOx5hpeWVSz48zu40JY3Je6zDtTTEvFuI+nyPVFLcWWtY eqbDGYXUYbYyBSfExtphfrsas5tR+XzSbiwxPrJVDiXhOZHeBFTCAEXgpwFpx+Y+ pod1LrzbN+UsFQjTvFLBJJEhMjbUc60CIEflbK3G1Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrtdeggdduiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepteefffefgfektdefgfeludfgtdejfeejvddttdekteeiffejvdfgheehfffh vedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 27 Oct 2022 05:37:32 -0400 (EDT) Date: Thu, 27 Oct 2022 11:37:30 +0200 From: Maxime Ripard To: Mateusz Kwiatkowski Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper Message-ID: <20221027093730.tb4oaissdapf6ifr@houat> References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="xamrv4lbhqfdsxuo" Content-Disposition: inline In-Reply-To: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Karol Herbst , David Airlie , nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Phil Elwell , Emma Anholt , Samuel Holland , Jernej Skrabec , Chen-Yu Tsai , Geert Uytterhoeven , Ben Skeggs , linux-sunxi@lists.linux.dev, intel-gfx@lists.freedesktop.org, Hans de Goede , Rodrigo Vivi , linux-arm-kernel@lists.infradead.org, Tvrtko Ursulin , Dom Cobley , Dave Stevenson , linux-kernel@vger.kernel.org, Noralf =?utf-8?Q?Tr=C3=B8nnes?= , Thomas Zimmermann Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" --xamrv4lbhqfdsxuo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Mateusz, On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote: > First of all, nice idea with the helper function that can be reused by > different drivers. This is neat! Yeah, it looked to me that given how complex it is, we don't want to duplicate it in each and every driver. > But looking at this function, it feels a bit overcomplicated. You're > creating the two modes, If reported as supported by the connector, yes. > then checking which one is the default, then set the preferred one and > possibly reorder them. Maybe it can be simplified somehow? Possibly, but I couldn't find something simpler. We should only expose the modes that the driver reports as supported, so we can have 0-2 modes. Then the preferred flag needs to be set on the default one like you suggested. But also, EDIDs define the preferred mode as either the mode with the flag set or the first mode listed. So a lot of program just use the heuristic to just pick the first mode listed. So it might be that I'm too careful, but it still seems useful to me. > Although when I tried to refactor it myself, I ended up with something th= at's > not better at all. Maybe it needs to be complicated, after all :( Yeah, that was my conclusion too :/ > Anyway, the current version seems to have a couple of bugs: >=20 > > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > > + mode =3D drm_mode_analog_pal_576i(connector->dev); > > + if (!mode) > > + return 0; > > + > > + tv_modes[count++] =3D mode; > > + } >=20 > If the 480i mode has been created properly, but there's an error creating= the > 576i one (we enter the if (!mode) clause), the 480i mode will leak. >=20 > > + if (count =3D=3D 1) { >=20 > You're handling the count =3D=3D 1 case specially, but if count =3D=3D 0,= the rest of > the code will assume that two modes exist and probably segfault in the pr= ocess. >=20 > > + ret =3D drm_object_property_get_default_value(&connector->base, > > + dev->mode_config.tv_mode_property, > > + &default_mode); > > + if (ret) > > + return 0; > > + > > + if (cmdline->tv_mode_specified) > > + default_mode =3D cmdline->tv_mode; >=20 > In case of an error (ret !=3D 0), the modes created so far in the tv_mode= s array > will leak. Thanks for the review, I'll fix these bugs > Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should= go > first? If we're going to use the default from cmdline, there's no point i= n even > querying the property default value. Maybe, I don't know. I find the flow of the code more readable that way, but if you disagree I'll change it. Maxime --xamrv4lbhqfdsxuo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY1pRWgAKCRDj7w1vZxhR xTVQAQCdqQ7+vEr0O0pC7P93kHAYwFz0oBZn90Ip4EjyExKRPgD/cMwjjUZ6daWo /idlovksY5X1UdblxYiDcBffKZ9k8gM= =nRSG -----END PGP SIGNATURE----- --xamrv4lbhqfdsxuo-- 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 1F94CC38A2D for ; Thu, 27 Oct 2022 09:38:02 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 765B910E5B2; Thu, 27 Oct 2022 09:37:49 +0000 (UTC) Received: from wnew1-smtp.messagingengine.com (wnew1-smtp.messagingengine.com [64.147.123.26]) by gabe.freedesktop.org (Postfix) with ESMTPS id B2A4410E59E; Thu, 27 Oct 2022 09:37:42 +0000 (UTC) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.west.internal (Postfix) with ESMTP id E0A312B066EB; Thu, 27 Oct 2022 05:37:34 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 27 Oct 2022 05:37:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm3; t=1666863454; x=1666870654; bh=5ItCVshj6J yZau2J5cjH0M8JHw139GrYlnIerAQcZWs=; b=IVFSYrra2yRKDBScb6i+EPPsnR H7iUD7VkPJAKE695dyeIFHVvDzTnEOFd/PuRkXJYQDLL3GqO5VDnC18zyG53l/7I pvgnmLqeiBPBWXQjApcUVHOrKRHilbj5+ASZsv6ndq61A0vf5cAG6GPGNStKiCRy wer2bctGBN1o9JRLtzfw8j3SxBth2GhmC1AF3SbqCoOAk5ikvGienxjeMuTVnd3b CIYEdhLoJTnMe4apOVtrct3AJygLwtX9QefTOEjFMks85VRv9W7SGo+EMXE4ei+c /vSdG+pF0qpFn1D0F3ffTOrvCiLzPaO3Iv99pS2pxcK/5ssmEIuPwA72joMw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1666863454; x=1666870654; bh=5ItCVshj6JyZau2J5cjH0M8JHw13 9GrYlnIerAQcZWs=; b=aMBp3hE4BgyODOr6WOBsdolHjS2Onrihiev6Ff/sAjQh Cama2t5zFVnEKp63jv9ugJbpKhvl71qc+L5qSL1TZPihHUka8luZ1042BOHS5Sxh aMKHX7Ay1bGb29oD5v8DX8oZBPKSAoX9G4RJblIy3UNVtu3bbfNqg1CYVaEHxNJc pHr9LW/SlL0Xhx1KndHWOx5hpeWVSz48zu40JY3Je6zDtTTEvFuI+nyPVFLcWWtY eqbDGYXUYbYyBSfExtphfrsas5tR+XzSbiwxPrJVDiXhOZHeBFTCAEXgpwFpx+Y+ pod1LrzbN+UsFQjTvFLBJJEhMjbUc60CIEflbK3G1Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrtdeggdduiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepteefffefgfektdefgfeludfgtdejfeejvddttdekteeiffejvdfgheehfffh vedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 27 Oct 2022 05:37:32 -0400 (EDT) Date: Thu, 27 Oct 2022 11:37:30 +0200 From: Maxime Ripard To: Mateusz Kwiatkowski Message-ID: <20221027093730.tb4oaissdapf6ifr@houat> References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="xamrv4lbhqfdsxuo" Content-Disposition: inline In-Reply-To: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> Subject: Re: [Intel-gfx] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Karol Herbst , David Airlie , nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Phil Elwell , Emma Anholt , Samuel Holland , Jernej Skrabec , Chen-Yu Tsai , Geert Uytterhoeven , Ben Skeggs , linux-sunxi@lists.linux.dev, Daniel Vetter , intel-gfx@lists.freedesktop.org, Hans de Goede , Rodrigo Vivi , linux-arm-kernel@lists.infradead.org, Dom Cobley , Dave Stevenson , linux-kernel@vger.kernel.org, Noralf =?utf-8?Q?Tr=C3=B8nnes?= , Thomas Zimmermann Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" --xamrv4lbhqfdsxuo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Mateusz, On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote: > First of all, nice idea with the helper function that can be reused by > different drivers. This is neat! Yeah, it looked to me that given how complex it is, we don't want to duplicate it in each and every driver. > But looking at this function, it feels a bit overcomplicated. You're > creating the two modes, If reported as supported by the connector, yes. > then checking which one is the default, then set the preferred one and > possibly reorder them. Maybe it can be simplified somehow? Possibly, but I couldn't find something simpler. We should only expose the modes that the driver reports as supported, so we can have 0-2 modes. Then the preferred flag needs to be set on the default one like you suggested. But also, EDIDs define the preferred mode as either the mode with the flag set or the first mode listed. So a lot of program just use the heuristic to just pick the first mode listed. So it might be that I'm too careful, but it still seems useful to me. > Although when I tried to refactor it myself, I ended up with something th= at's > not better at all. Maybe it needs to be complicated, after all :( Yeah, that was my conclusion too :/ > Anyway, the current version seems to have a couple of bugs: >=20 > > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > > + mode =3D drm_mode_analog_pal_576i(connector->dev); > > + if (!mode) > > + return 0; > > + > > + tv_modes[count++] =3D mode; > > + } >=20 > If the 480i mode has been created properly, but there's an error creating= the > 576i one (we enter the if (!mode) clause), the 480i mode will leak. >=20 > > + if (count =3D=3D 1) { >=20 > You're handling the count =3D=3D 1 case specially, but if count =3D=3D 0,= the rest of > the code will assume that two modes exist and probably segfault in the pr= ocess. >=20 > > + ret =3D drm_object_property_get_default_value(&connector->base, > > + dev->mode_config.tv_mode_property, > > + &default_mode); > > + if (ret) > > + return 0; > > + > > + if (cmdline->tv_mode_specified) > > + default_mode =3D cmdline->tv_mode; >=20 > In case of an error (ret !=3D 0), the modes created so far in the tv_mode= s array > will leak. Thanks for the review, I'll fix these bugs > Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should= go > first? If we're going to use the default from cmdline, there's no point i= n even > querying the property default value. Maybe, I don't know. I find the flow of the code more readable that way, but if you disagree I'll change it. Maxime --xamrv4lbhqfdsxuo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY1pRWgAKCRDj7w1vZxhR xTVQAQCdqQ7+vEr0O0pC7P93kHAYwFz0oBZn90Ip4EjyExKRPgD/cMwjjUZ6daWo /idlovksY5X1UdblxYiDcBffKZ9k8gM= =nRSG -----END PGP SIGNATURE----- --xamrv4lbhqfdsxuo-- 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 15A2DC38A2D for ; Thu, 27 Oct 2022 09:38:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: In-Reply-To:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Reply-To:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date :Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=LDhvJwgH0/uNkemVrqfP5WnWEXOkFnPTUFsKM05ExHo=; b=XBWyXDC/wzSbFXy9eNd41smCRi tj6HMNyV+BFWk9aXc+sgK8qvEElqvOAERSi93iitXyeKnZ43L8q6toQ6ep4TesihFAscgt7giDqmD 7Ury9WnY667qhR26+9zEE/4zF8CMF8W0bobdiGWCqhJ8rYpkR9ZuJP4SLbFNPj+uj24JSgU2MT+Dx uqnCVKhgWbiUwf/i0ZavFADYI8cMDD7dwkiKAVeb3PAD3Cwvpsp4xqd3eFvi7iRFly4Jvnarbv8gp auQH7OFOSJgZPSu1RiFxKFkZejPJuc5u/RB4T1lx0j3VyNFVADJA4Q6sTp1BOG9Jxl5oNkbiU9Ldo 16u7ZTdA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1onzKb-00CcRT-EB; Thu, 27 Oct 2022 09:37:45 +0000 Received: from wnew1-smtp.messagingengine.com ([64.147.123.26]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1onzKY-00CcQX-O6 for linux-arm-kernel@lists.infradead.org; Thu, 27 Oct 2022 09:37:44 +0000 Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailnew.west.internal (Postfix) with ESMTP id E0A312B066EB; Thu, 27 Oct 2022 05:37:34 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute5.internal (MEProxy); Thu, 27 Oct 2022 05:37:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc :cc:content-type:date:date:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to; s=fm3; t=1666863454; x=1666870654; bh=5ItCVshj6J yZau2J5cjH0M8JHw139GrYlnIerAQcZWs=; b=IVFSYrra2yRKDBScb6i+EPPsnR H7iUD7VkPJAKE695dyeIFHVvDzTnEOFd/PuRkXJYQDLL3GqO5VDnC18zyG53l/7I pvgnmLqeiBPBWXQjApcUVHOrKRHilbj5+ASZsv6ndq61A0vf5cAG6GPGNStKiCRy wer2bctGBN1o9JRLtzfw8j3SxBth2GhmC1AF3SbqCoOAk5ikvGienxjeMuTVnd3b CIYEdhLoJTnMe4apOVtrct3AJygLwtX9QefTOEjFMks85VRv9W7SGo+EMXE4ei+c /vSdG+pF0qpFn1D0F3ffTOrvCiLzPaO3Iv99pS2pxcK/5ssmEIuPwA72joMw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-type:date:date:feedback-id :feedback-id:from:from:in-reply-to:in-reply-to:message-id :mime-version:references:reply-to:sender:subject:subject:to:to :x-me-proxy:x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s= fm3; t=1666863454; x=1666870654; bh=5ItCVshj6JyZau2J5cjH0M8JHw13 9GrYlnIerAQcZWs=; b=aMBp3hE4BgyODOr6WOBsdolHjS2Onrihiev6Ff/sAjQh Cama2t5zFVnEKp63jv9ugJbpKhvl71qc+L5qSL1TZPihHUka8luZ1042BOHS5Sxh aMKHX7Ay1bGb29oD5v8DX8oZBPKSAoX9G4RJblIy3UNVtu3bbfNqg1CYVaEHxNJc pHr9LW/SlL0Xhx1KndHWOx5hpeWVSz48zu40JY3Je6zDtTTEvFuI+nyPVFLcWWtY eqbDGYXUYbYyBSfExtphfrsas5tR+XzSbiwxPrJVDiXhOZHeBFTCAEXgpwFpx+Y+ pod1LrzbN+UsFQjTvFLBJJEhMjbUc60CIEflbK3G1Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvgedrtdeggdduiecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffvvefukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpeforgigihhm vgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrfgrth htvghrnhepteefffefgfektdefgfeludfgtdejfeejvddttdekteeiffejvdfgheehfffh vedunecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmh grgihimhgvsegtvghrnhhordhtvggthh X-ME-Proxy: Feedback-ID: i8771445c:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 27 Oct 2022 05:37:32 -0400 (EDT) Date: Thu, 27 Oct 2022 11:37:30 +0200 From: Maxime Ripard To: Mateusz Kwiatkowski Cc: Karol Herbst , Emma Anholt , Ben Skeggs , Chen-Yu Tsai , Rodrigo Vivi , Maarten Lankhorst , Jani Nikula , Daniel Vetter , Thomas Zimmermann , Tvrtko Ursulin , Samuel Holland , Jernej Skrabec , David Airlie , Joonas Lahtinen , Lyude Paul , linux-sunxi@lists.linux.dev, intel-gfx@lists.freedesktop.org, Phil Elwell , linux-arm-kernel@lists.infradead.org, nouveau@lists.freedesktop.org, Hans de Goede , Dom Cobley , dri-devel@lists.freedesktop.org, Dave Stevenson , linux-kernel@vger.kernel.org, Noralf =?utf-8?Q?Tr=C3=B8nnes?= , Geert Uytterhoeven Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper Message-ID: <20221027093730.tb4oaissdapf6ifr@houat> References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> MIME-Version: 1.0 In-Reply-To: <8d0eee22-50f5-5b0a-c1e6-c5f61dd5bbcd@gmail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221027_023742_985453_C8C8BC27 X-CRM114-Status: GOOD ( 28.15 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============3226302181978515360==" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org --===============3226302181978515360== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="xamrv4lbhqfdsxuo" Content-Disposition: inline --xamrv4lbhqfdsxuo Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Mateusz, On Thu, Oct 27, 2022 at 12:02:24AM +0200, Mateusz Kwiatkowski wrote: > First of all, nice idea with the helper function that can be reused by > different drivers. This is neat! Yeah, it looked to me that given how complex it is, we don't want to duplicate it in each and every driver. > But looking at this function, it feels a bit overcomplicated. You're > creating the two modes, If reported as supported by the connector, yes. > then checking which one is the default, then set the preferred one and > possibly reorder them. Maybe it can be simplified somehow? Possibly, but I couldn't find something simpler. We should only expose the modes that the driver reports as supported, so we can have 0-2 modes. Then the preferred flag needs to be set on the default one like you suggested. But also, EDIDs define the preferred mode as either the mode with the flag set or the first mode listed. So a lot of program just use the heuristic to just pick the first mode listed. So it might be that I'm too careful, but it still seems useful to me. > Although when I tried to refactor it myself, I ended up with something th= at's > not better at all. Maybe it needs to be complicated, after all :( Yeah, that was my conclusion too :/ > Anyway, the current version seems to have a couple of bugs: >=20 > > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) || > > + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) { > > + mode =3D drm_mode_analog_pal_576i(connector->dev); > > + if (!mode) > > + return 0; > > + > > + tv_modes[count++] =3D mode; > > + } >=20 > If the 480i mode has been created properly, but there's an error creating= the > 576i one (we enter the if (!mode) clause), the 480i mode will leak. >=20 > > + if (count =3D=3D 1) { >=20 > You're handling the count =3D=3D 1 case specially, but if count =3D=3D 0,= the rest of > the code will assume that two modes exist and probably segfault in the pr= ocess. >=20 > > + ret =3D drm_object_property_get_default_value(&connector->base, > > + dev->mode_config.tv_mode_property, > > + &default_mode); > > + if (ret) > > + return 0; > > + > > + if (cmdline->tv_mode_specified) > > + default_mode =3D cmdline->tv_mode; >=20 > In case of an error (ret !=3D 0), the modes created so far in the tv_mode= s array > will leak. Thanks for the review, I'll fix these bugs > Also, I wonder if maybe the if (cmdline->tv_mode_specified) clause should= go > first? If we're going to use the default from cmdline, there's no point i= n even > querying the property default value. Maybe, I don't know. I find the flow of the code more readable that way, but if you disagree I'll change it. Maxime --xamrv4lbhqfdsxuo Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iHUEABYKAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCY1pRWgAKCRDj7w1vZxhR xTVQAQCdqQ7+vEr0O0pC7P93kHAYwFz0oBZn90Ip4EjyExKRPgD/cMwjjUZ6daWo /idlovksY5X1UdblxYiDcBffKZ9k8gM= =nRSG -----END PGP SIGNATURE----- --xamrv4lbhqfdsxuo-- --===============3226302181978515360== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel --===============3226302181978515360==--