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 4DA0CC433FE for ; Sun, 16 Oct 2022 16:13:13 +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-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=VVraB7PBM9QZizwGddYNrz94NL7YEiFqvmY2Cg6xP1Y=; b=uDXshthyKahm1Q +dqY4Nj4Ffm4UbZyjuQG/ueBEgcqGoXYQhNHbXnsTRXomI/lwxK2Jl0W5ZSVgvjTH9e1/PjyMYvah G4v88kpJO0pPGWQkn8pnSJ91UBhcFFXAErzotslK8U1PEhxZJlf32FW7G/EdxyspErL5C0zj6nKU2 1KTJAxX0ybpAvlUz5grH0tZLwRx+BBNqNzM9l/ytZQqt83ODbXpOMbEC67YdPYY2NKTuxQCqJi8n6 X9fpxuXQBAoooQtDao9KQdWCTH6dzaXXcvTTrT1ga/KTmtPb3G2h1BLqn12+sBNQHLI//pDpoAVfL H7VQRJ6y0VZWZK3jy5kg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ok6Ev-002ESo-6C; Sun, 16 Oct 2022 16:11:49 +0000 Received: from smtp.domeneshop.no ([2a01:5b40:0:3005::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1ok6Eq-002EGT-Oj for linux-arm-kernel@lists.infradead.org; Sun, 16 Oct 2022 16:11:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=tronnes.org ; s=ds202112; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:From: References:Cc:To:Subject:MIME-Version:Date:Message-ID:Sender:Reply-To: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=SoBF0g+FxVXYhyEJbqnBUYS67u0wCvqTfgLTEmDo6Rs=; b=chyU99w3w9UnsJiZdgEZxPywYK 2KFqsArMDjFwStLaJzCr0FFMBxUEZdZSor1/8gZukX9II02z5mT8+lPXJWm5X5KybeOaDgyjfUVXx nSd9rTChH84m+/0qpy3JcAB9/zqz528MlroFmgwAk3KsiLi34gVTMlrQZUUqkr/sWHT149wrgyGbw ACRXUuRsqa1btBXk7YTvArU3dnpTkza00kF5bK3PxTtiGfy5+0V5z0VlP6JSfRgMqrv7Jt+V/cJMy AycYhU90dxCflKYFL6EbN34GAlb/nPpojAQLos+HyhvPPTelcr1bp2sWGdu/t2coHum3e+KJaAnOd PmE/yO7Q==; Received: from 211.81-166-168.customer.lyse.net ([81.166.168.211]:51414 helo=[192.168.10.61]) by smtp.domeneshop.no with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ok6Ec-0003SN-2l; Sun, 16 Oct 2022 18:11:30 +0200 Message-ID: <729f849a-0287-a2fb-125f-892b4dd6053f@tronnes.org> Date: Sun, 16 Oct 2022 18:11:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Subject: Re: [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function To: Maxime Ripard , Karol Herbst , Jani Nikula , Tvrtko Ursulin , Daniel Vetter , Maarten Lankhorst , David Airlie , Joonas Lahtinen , Lyude Paul , Maxime Ripard , Emma Anholt , Chen-Yu Tsai , Samuel Holland , Ben Skeggs , Thomas Zimmermann , Rodrigo Vivi , Jernej Skrabec Cc: Dom Cobley , linux-sunxi@lists.linux.dev, Dave Stevenson , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org, Geert Uytterhoeven , linux-arm-kernel@lists.infradead.org, Mateusz Kwiatkowski , dri-devel@lists.freedesktop.org, Hans de Goede , Phil Elwell , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= References: <20220728-rpi-analog-tv-properties-v5-0-d841cc64fe4b@cerno.tech> <20220728-rpi-analog-tv-properties-v5-8-d841cc64fe4b@cerno.tech> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= In-Reply-To: <20220728-rpi-analog-tv-properties-v5-8-d841cc64fe4b@cerno.tech> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221016_091145_015954_ED5704A5 X-CRM114-Status: GOOD ( 34.77 ) 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: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Den 13.10.2022 15.18, skrev Maxime Ripard: > The current construction of the named mode parsing doesn't allow to extend > it easily. Let's move it to a separate function so we can add more > parameters and modes. > > In order for the tests to still pass, some extra checks are needed, so > it's not a 1:1 move. > > Signed-off-by: Maxime Ripard > I was hoping that someone else would step up and review these parser patches since the parser code is rather difficult to read, for me at least. I have studied it now, so I'll give it a try. > --- > Changes in v4: > - Fold down all the named mode patches that were split into a single > patch again to maintain bisectability > --- > drivers/gpu/drm/drm_modes.c | 73 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 62 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index c0dceff51cac..2f020ef2ddf2 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[] = { > "PAL", > }; > > +static int drm_mode_parse_cmdline_named_mode(const char *name, > + unsigned int name_end, > + struct drm_cmdline_mode *cmdline_mode) > +{ > + unsigned int i; > + > + if (!name_end) > + return 0; name_end can't be zero since the argument is checked before calling this function. > + > + /* If the name starts with a digit, it's not a named mode */ > + if (isdigit(name[0])) > + return 0; > + > + /* > + * If there's an equal sign in the name, the command-line > + * contains only an option and no mode. > + */ > + if (strnchr(name, name_end, '=')) > + return 0; I think this check actually belongs in drm_mode_parse_command_line_for_connector() after options_off is set. If theres an equal sign it should skip all mode parsing and skip down to drm_mode_parse_cmdline_options(). Which probably means that the mode parsing should have been moved out to separate function to avoid using a goto. But that's probably beyond the scope of this patchset :) > + > +#define STR_STRICT_EQ(str, len, cmp) \ > + (str_has_prefix(str, cmp) == len) > + > + /* The connection status extras can be set without a mode. */ > + if (STR_STRICT_EQ(name, name_end, "d") || > + STR_STRICT_EQ(name, name_end, "D") || > + STR_STRICT_EQ(name, name_end, "e")) > + return 0; It took me a while to understand what is going on here. If str_has_prefix() finds a match it returns strlen(prefix). Since prefix is always of length 1, name_end has to always be 1 for the statement to be true. I would have written it like this: /* The connection status extras can be set without a mode. */ if (name_end == 1) { if (name[0] == "d" || name[0] == "D" || name[0] == "e") return 0; } > + > + /* > + * We're sure we're a named mode at that point, iterate over the that -> this ? > + * list of modes we're aware of. > + */ > + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { > + int ret; > + > + ret = str_has_prefix(name, drm_named_modes_whitelist[i]); > + if (ret != name_end) > + continue; > + > + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]); > + cmdline_mode->specified = true; > + > + return 1; > + } > + > + return -EINVAL; > +} > + > /** > * drm_mode_parse_command_line_for_connector - parse command line modeline for connector > * @mode_option: optional per connector mode option > @@ -2265,7 +2314,7 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL; > const char *options_ptr = NULL; > char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL; > - int i, len, ret; > + int len, ret; > > memset(mode, 0, sizeof(*mode)); > mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN; > @@ -2306,17 +2355,19 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, > parse_extras = true; > } > > - /* First check for a named mode */ > - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { > - ret = str_has_prefix(name, drm_named_modes_whitelist[i]); > - if (ret == mode_end) { > - if (refresh_ptr) > - return false; /* named + refresh is invalid */ > > - strcpy(mode->name, drm_named_modes_whitelist[i]); > - mode->specified = true; > - break; > - } > + if (mode_end) { Shouldn't this be: if (!mode_end) return false; I can't see how mode_end == 0 can produce anything valid. Scenarios for mode_option that gives mode_end == 0: - zero length - starts with a comma - starts with -bpp - starts with @refresh Maybe it's a separate patch with matching tests ;) Noralf. > + ret = drm_mode_parse_cmdline_named_mode(name, mode_end, mode); > + if (ret < 0) > + return false; > + > + /* > + * Having a mode that starts by a letter (and thus is named) > + * and an at-sign (used to specify a refresh rate) is > + * disallowed. > + */ > + if (ret && refresh_ptr) > + return false; > } > > /* No named mode? Check for a normal mode argument, e.g. 1024x768 */ > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel