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 9ED15C433FE for ; Sun, 6 Nov 2022 16:35:15 +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=+vE0jrCivL/ZAcb29WJ4SszCQuL+ogOMiFT4YXso3Ng=; b=xZ1rpov90r2AYv 76czAHypjRKsg57Fa5ehn7IdwHVVw2ik2Jmp6SyLMX2JIJRoyc+4cdyI/3f7LjHgtoS69WKbdZO22 Eq3tuIFtS5+i5OVUMdfPgTe/Tp2GIa7k2NExfI0YDccG+VS8hfIMhDYtcj3U4Kqlgt+eQWk4XPQaE Py6gIYM9wQI5tQDsQKlStltlUrBEHK8btjqTuN8v3V+XhFhklR3M+K5mrUFQ8retZWNC+o9d3nbIc SCwZdcrH1TDpAhEVq+aVf/Aah2prDmns9BP4GD3hVY+C6sZg1rRIStCfpl0nofd3C75qiOVD//rYX 3axTRjpC7SO5yRuQdMrA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1orib3-008owe-D1; Sun, 06 Nov 2022 16:34:09 +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 1orib0-008ov4-6H for linux-arm-kernel@lists.infradead.org; Sun, 06 Nov 2022 16:34:07 +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=QvIoHrgTbnCEFp2E+kqwZys718brKkUvKx2E8FVdXAU=; b=R0x9zXTEOsbViJfXz+FCNC4vM3 uFqtwGLxzOd05dS01ItMXMP1PPK45WYw0O3lAe0JvWH4jhQd5w7yknQd5EMIzdRq4pWLf0sLQXd3n JfW+NRF49EsvmwA1jx8HJELNOcqT2nm9zpgUEpsEaro8rbYSf1gY+OqXYUpLms78UlIC6lFvBuMgh 9Vxf31BusF+P5+REG7+8UaSW79q1f9bhf8pGPyT3npoch3IMrFxnuBTyN0BDmNSjdIRhJCbuUOA6q /qpAcBNRJyyNF7XLQFEQ/iE8RbIs5xs094+4T8TWzq+HX/IXWpVBiVlAZ2oiQOaMQNRubMHGbVgdN ZHDbPCNA==; Received: from [2a01:799:95a:cb00:fd97:29ff:d72a:349e] (port=50711) by smtp.domeneshop.no with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oriap-0005uQ-6b; Sun, 06 Nov 2022 17:33:55 +0100 Message-ID: <842076aa-8d7c-96d6-ba46-d0e66dacd2df@tronnes.org> Date: Sun, 6 Nov 2022 17:33:48 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Subject: Re: [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper To: maxime@cerno.tech, 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 , Maxime Ripard , Joonas Lahtinen , Lyude Paul Cc: 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 , Mateusz Kwiatkowski , dri-devel@lists.freedesktop.org, Dave Stevenson , linux-kernel@vger.kernel.org, Geert Uytterhoeven , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= References: <20220728-rpi-analog-tv-properties-v6-0-e7792734108f@cerno.tech> <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= In-Reply-To: <20220728-rpi-analog-tv-properties-v6-16-e7792734108f@cerno.tech> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221106_083406_439346_5C89BAE8 X-CRM114-Status: GOOD ( 29.32 ) 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 26.10.2022 17.33, skrev maxime@cerno.tech: > Most of the TV connectors will need a similar get_modes implementation > that will, depending on the drivers' capabilities, register the 480i and > 576i modes. > > That implementation will also need to set the preferred flag and order > the modes based on the driver and users preferrence. > > This is especially important to guarantee that a userspace stack such as > Xorg can start and pick up the preferred mode while maintaining a > working output. > > Signed-off-by: Maxime Ripard > > --- > Changes in v6: > - New patch > --- > drivers/gpu/drm/drm_probe_helper.c | 97 ++++++++++++++++++++++++++++++++++++++ > include/drm/drm_probe_helper.h | 1 + > 2 files changed, 98 insertions(+) > > diff --git a/drivers/gpu/drm/drm_probe_helper.c b/drivers/gpu/drm/drm_probe_helper.c > index 69b0b2b9cc1c..4a60575f5c66 100644 > --- a/drivers/gpu/drm/drm_probe_helper.c > +++ b/drivers/gpu/drm/drm_probe_helper.c > @@ -1147,3 +1147,100 @@ int drm_connector_helper_get_modes(struct drm_connector *connector) > return count; > } > EXPORT_SYMBOL(drm_connector_helper_get_modes); > + > +static bool tv_mode_supported(struct drm_connector *connector, > + enum drm_connector_tv_mode mode) > +{ > + struct drm_device *dev = connector->dev; > + struct drm_property *property = dev->mode_config.tv_mode_property; > + Superfluous linebreak > + unsigned int i; > + > + for (i = 0; i < property->num_values; i++) > + if (property->values[i] == mode) > + return true; > + > + return false; > +} > + > +/** > + * drm_connector_helper_tv_get_modes - Fills the modes availables to a TV connector availables -> available > + * @connector: The connector > + * > + * Fills the available modes for a TV connector based on the supported > + * TV modes, and the default mode expressed by the kernel command line. > + * > + * This can be used as the default TV connector helper .get_modes() hook > + * if the driver does not need any special processing. > + * > + * Returns: > + * The number of modes added to the connector. > + */ > +int drm_connector_helper_tv_get_modes(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + struct drm_cmdline_mode *cmdline = &connector->cmdline_mode; > + struct drm_display_mode *tv_modes[2] = {}; > + struct drm_display_mode *mode; > + unsigned int first_mode_idx; > + unsigned int count = 0; > + uint64_t default_mode; > + int ret; > + > + if (!dev->mode_config.tv_mode_property) > + return 0; > + > + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_443) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_J) || > + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_M)) { > + mode = drm_mode_analog_ntsc_480i(connector->dev); Nit: You can use the dev variable here and below. > + if (!mode) > + return 0; > + > + tv_modes[count++] = mode; > + } > + > + 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 = drm_mode_analog_pal_576i(connector->dev); > + if (!mode) > + return 0; You leak the ntsc mode when returning (possibly). > + > + tv_modes[count++] = mode; > + } > + Maybe check for count being zero here? > + if (count == 1) { > + mode->type |= DRM_MODE_TYPE_PREFERRED; > + drm_mode_probed_add(connector, mode); > + return count; > + } > + > + ret = drm_object_property_get_default_value(&connector->base, > + dev->mode_config.tv_mode_property, > + &default_mode); > + if (ret) > + return 0; You leak both modes when returning here. Maybe move this up before allocation to simplify error handling. > + > + if (cmdline->tv_mode_specified) > + default_mode = cmdline->tv_mode; I realised that we don't verify tv_mode coming from the command line, not here and not in the reset helper. Should we do that? A driver should be programmed defensively to handle an illegal/unsupported value, but it doesn't feel right to allow an illegal enum value coming through the core/helpers. > + > + if ((default_mode == DRM_MODE_TV_MODE_NTSC) || > + (default_mode == DRM_MODE_TV_MODE_NTSC_443) || > + (default_mode == DRM_MODE_TV_MODE_NTSC_J) || > + (default_mode == DRM_MODE_TV_MODE_PAL_M)) > + first_mode_idx = 0; > + else > + first_mode_idx = 1; > + > + mode = tv_modes[first_mode_idx]; > + mode->type |= DRM_MODE_TYPE_PREFERRED; > + drm_mode_probed_add(connector, mode); > + > + mode = first_mode_idx ? tv_modes[0] : tv_modes[1]; > + drm_mode_probed_add(connector, mode); > + > + return count; > +} > +EXPORT_SYMBOL(drm_connector_helper_tv_get_modes); I know this is not expensive, but you're looping over the property values 7 times. An alternative solution is to rebuild the supported bitmask: int drm_connector_helper_tv_get_modes(struct drm_connector *connector) { ... unsigned int ntsc_modes = BIT(DRM_MODE_TV_MODE_NTSC) | BIT(DRM_MODE_TV_MODE_NTSC_443) | BIT(DRM_MODE_TV_MODE_NTSC_J) | BIT(DRM_MODE_TV_MODE_PAL_M); unsigned int pal_modes = BIT(DRM_MODE_TV_MODE_PAL) | BIT(DRM_MODE_TV_MODE_PAL_N) | BIT(DRM_MODE_TV_MODE_SECAM); unsigned int supported_tv_modes = 0; ... for (i = 0; i < property->num_values; i++) supported_tv_modes |= BIT(property->values[i]); if (supported_tv_modes & ntsc_modes) ... if (supported_tv_modes & pal_modes) ... if (BIT(default_mode) & ntsc_modes) first_mode_idx = 0; else first_mode_idx = 1; Up to you if you want to do this. Noralf. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel