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 B6DE0ECAAA1 for ; Tue, 6 Sep 2022 12:35:04 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8C2A610E67C; Tue, 6 Sep 2022 12:34:22 +0000 (UTC) Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by gabe.freedesktop.org (Postfix) with ESMTPS id A069B10E034; Wed, 31 Aug 2022 01:44:57 +0000 (UTC) Received: by mail-lf1-x12f.google.com with SMTP id j14so7604684lfu.4; Tue, 30 Aug 2022 18:44:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=EFsFMp+canY/hKJink3xfIjzSIs5nK0ATMHsiNLvxO07lOyOzwsvad8rVyDd2wmSeg cWCjphxF3TSGFFDKqyBmATL/jehCiz7Ylb6ULQwm/mKEET2IUotRhOyHBInnuwebAx/W o98i4bzpAYKvvI2sU//Sa2wKHsHNlK0Ik4KVSfjlV5E7mJwynZVYZggwdjMG3KRyfVMo xPbIu9xAaQjWAcnqbsVBfJmmxCjJL8AJzz30l+qNOOOt1j+HNC9Xoq1+ko3At8FQwbwc GgYRNngTbp8HBenXcqfoYJexOO63aFwrEEDYhvuQd/DuP/2j454qxLb21o5XEYcIYyWs Dtmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=a+9GKQ8x74vzchXjZqTcNUgtGaemIPuVs7S3SK/lrPtviFWj6C2cpBLfX3BImm9yA0 /rc1j+C/M7DZMKwr+wLSV4zHkOYyz5Iz0k2oDbo9fZ/7E3nOHWn/LmqGVXQkLnHNuop7 WIbedVKLV7nBu5ZsQIJNv9Y51+XVZNJEsHTDYdb4vVYiA1honmGvM2o1VrI9kMUP7Vhc s/ORtNOca6bnxWBx3iU5xouDkIwj7yraCwBbN0kY4NJLv/CYpQt8NZy7xtBXI7mzOl8I 8JT4Xr+ercV6pa4dB+LJhzPb/j4WDcT4VxVro59U65R0hAlSjCAqZGxmYZwSy1rOZGhd 0Ofw== X-Gm-Message-State: ACgBeo3+AdI7rwyaPkU2Wbq2xgeuBuVfyMdceHVqlGdwrTTKiPBsj9wh 7BV9l/bpFMGmshvddeMOZw0= X-Google-Smtp-Source: AA6agR5Z8V/gUf3Mh2HitGw7sMfYSuNjRXxRlM0XOW/4Yz6ST42Ov+CXOy5ZF8ScFy9ud/OgRIMt7A== X-Received: by 2002:ac2:4901:0:b0:494:88dc:7efc with SMTP id n1-20020ac24901000000b0049488dc7efcmr411746lfi.408.1661910295748; Tue, 30 Aug 2022 18:44:55 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:d40b:b088:5bfe:3b81? ([2a02:a31a:a240:1700:d40b:b088:5bfe:3b81]) by smtp.googlemail.com with ESMTPSA id g1-20020a0565123b8100b004948f583e6bsm42248lfv.138.2022.08.30.18.44.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 18:44:55 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com> Date: Wed, 31 Aug 2022 03:44:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Content-Language: pl To: Maxime Ripard , Maxime Ripard , Ben Skeggs , David Airlie , Chen-Yu Tsai , Thomas Zimmermann , Jani Nikula , Lyude Paul , Philipp Zabel , Maarten Lankhorst , Rodrigo Vivi , Tvrtko Ursulin , Jernej Skrabec , Samuel Holland , Karol Herbst , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Emma Anholt , Daniel Vetter , Joonas Lahtinen References: <20220728-rpi-analog-tv-properties-v2-0-459522d653a7@cerno.tech> <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Mailman-Approved-At: Tue, 06 Sep 2022 12:33:45 +0000 Subject: Re: [Intel-gfx] [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes 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: Dom Cobley , Dave Stevenson , nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-sunxi@lists.linux.dev, Geert Uytterhoeven , Phil Elwell , linux-arm-kernel@lists.infradead.org Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Hi Maxime, Wow. That's an enormous amount of effort put into this patch. But I'm tempted to say that this is actually overengineered quite a bit :D Considering that there's no way to access all these calculations from user space, and I can't imagine anybody using anything else than those standard 480i/576i (and maybe 240p/288p) modes at 13.5 MHz any time soon... I'm not sure if we actually need all this. But anyway, I'm not the maintainer of this subsystem, so I'm not the one to decide. > +enum drm_mode_analog { > +    DRM_MODE_ANALOG_NTSC, > +    DRM_MODE_ANALOG_PAL, > +}; Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is common, but strictly speaking a misnomer. Those are color encoding systems, and your patchset fully supports lesser used, but standard encodings for those (e.g. PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more neutral naming scheme. Some ideas: - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh rate) - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line   count) - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System   Letter Designations) > +#define NTSC_HFP_DURATION_TYP_NS    1500 > +#define NTSC_HFP_DURATION_MIN_NS    1270 > +#define NTSC_HFP_DURATION_MAX_NS    2220 You've defined those min/typ/max ranges, but you're not using the "typ" field for anything other than hslen. The actual "typical" value is thus always the midpoint, which isn't necessarily the best choice. In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 requested by BT.601. That's all obviously within tolerances, but the image ends up noticeably off-center (at least on modern TVs), especially in the 576i case. > +    htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC; You're multiplying an unsigned int and an unsigned long - both types are only required to be 32 bit, so this is likely to overflow. You need to use a cast to unsigned long long, and then call do_div() for 64-bit division. This actually overflowed on me on my Pi running ARM32 kernel, resulting in negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode() taking over the mode generation (badly), and a horrible mess on screen. > +    vfp = vfp_min + (porches_rem / 2); > +    vbp = porches - vfp; Relative position of the vertical sync within the VBI effectively moves the image up and down. Adding that (porches_rem / 2) moves the image up off center by that many pixels. I'd keep the VFP always at minimum to keep the image centered. Best regards, Mateusz Kwiatkowski From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f53.google.com (mail-lf1-f53.google.com [209.85.167.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CF8CA620 for ; Wed, 31 Aug 2022 01:44:57 +0000 (UTC) Received: by mail-lf1-f53.google.com with SMTP id z29so9368366lfb.13 for ; Tue, 30 Aug 2022 18:44:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=EFsFMp+canY/hKJink3xfIjzSIs5nK0ATMHsiNLvxO07lOyOzwsvad8rVyDd2wmSeg cWCjphxF3TSGFFDKqyBmATL/jehCiz7Ylb6ULQwm/mKEET2IUotRhOyHBInnuwebAx/W o98i4bzpAYKvvI2sU//Sa2wKHsHNlK0Ik4KVSfjlV5E7mJwynZVYZggwdjMG3KRyfVMo xPbIu9xAaQjWAcnqbsVBfJmmxCjJL8AJzz30l+qNOOOt1j+HNC9Xoq1+ko3At8FQwbwc GgYRNngTbp8HBenXcqfoYJexOO63aFwrEEDYhvuQd/DuP/2j454qxLb21o5XEYcIYyWs Dtmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=UuNe8mK+yqf/lNgY3ubelWgNDSlkY8XGzWWvqjcymykbPYS5EwX3yAeI5ezBeKVUTW 7149XupCZGIeYef7lQ32yp8dvxxG4wxV0PcRJ658e9h2fVHvP7Uf6/d38OFSgxrLI/mM /5sHNY4C8JbDkLooxX7CJhMrjBDW5y0L01aQOfJvR13FmTGuOs1VFDS4E8Sd+W1EsvUv jXnx+N/B+Qhl6oWR1hlsPPIp/HjHgzUkwXquqR0WpgDVN+iCz4nOyL8Em+YURFEH7EF9 AS5I170R+XXwh9UyXEAHXxXPOfwEOmXteUsvonp+MdMKaW7sV51cWsdWU3Ck8S9hl+Wv VmPg== X-Gm-Message-State: ACgBeo2j3HUg0bFXoWDdwRceuMHE58kZFc1/TFKeoSKLQDqC0AaZ9miO ayHHiZuwPNQ4IwmARDVa5GE= X-Google-Smtp-Source: AA6agR5Z8V/gUf3Mh2HitGw7sMfYSuNjRXxRlM0XOW/4Yz6ST42Ov+CXOy5ZF8ScFy9ud/OgRIMt7A== X-Received: by 2002:ac2:4901:0:b0:494:88dc:7efc with SMTP id n1-20020ac24901000000b0049488dc7efcmr411746lfi.408.1661910295748; Tue, 30 Aug 2022 18:44:55 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:d40b:b088:5bfe:3b81? ([2a02:a31a:a240:1700:d40b:b088:5bfe:3b81]) by smtp.googlemail.com with ESMTPSA id g1-20020a0565123b8100b004948f583e6bsm42248lfv.138.2022.08.30.18.44.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 18:44:55 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com> Date: Wed, 31 Aug 2022 03:44:52 +0200 Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes Content-Language: pl To: Maxime Ripard , Maxime Ripard , Ben Skeggs , David Airlie , Chen-Yu Tsai , Thomas Zimmermann , Jani Nikula , Lyude Paul , Philipp Zabel , Maarten Lankhorst , Rodrigo Vivi , Tvrtko Ursulin , Jernej Skrabec , Samuel Holland , Karol Herbst , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Emma Anholt , Daniel Vetter , Joonas Lahtinen Cc: Hans de Goede , linux-arm-kernel@lists.infradead.org, Phil Elwell , intel-gfx@lists.freedesktop.org, Dave Stevenson , dri-devel@lists.freedesktop.org, Dom Cobley , linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org, linux-sunxi@lists.linux.dev, Geert Uytterhoeven References: <20220728-rpi-analog-tv-properties-v2-0-459522d653a7@cerno.tech> <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi Maxime, Wow. That's an enormous amount of effort put into this patch. But I'm tempted to say that this is actually overengineered quite a bit :D Considering that there's no way to access all these calculations from user space, and I can't imagine anybody using anything else than those standard 480i/576i (and maybe 240p/288p) modes at 13.5 MHz any time soon... I'm not sure if we actually need all this. But anyway, I'm not the maintainer of this subsystem, so I'm not the one to decide. > +enum drm_mode_analog { > +    DRM_MODE_ANALOG_NTSC, > +    DRM_MODE_ANALOG_PAL, > +}; Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is common, but strictly speaking a misnomer. Those are color encoding systems, and your patchset fully supports lesser used, but standard encodings for those (e.g. PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more neutral naming scheme. Some ideas: - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh rate) - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line   count) - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System   Letter Designations) > +#define NTSC_HFP_DURATION_TYP_NS    1500 > +#define NTSC_HFP_DURATION_MIN_NS    1270 > +#define NTSC_HFP_DURATION_MAX_NS    2220 You've defined those min/typ/max ranges, but you're not using the "typ" field for anything other than hslen. The actual "typical" value is thus always the midpoint, which isn't necessarily the best choice. In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 requested by BT.601. That's all obviously within tolerances, but the image ends up noticeably off-center (at least on modern TVs), especially in the 576i case. > +    htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC; You're multiplying an unsigned int and an unsigned long - both types are only required to be 32 bit, so this is likely to overflow. You need to use a cast to unsigned long long, and then call do_div() for 64-bit division. This actually overflowed on me on my Pi running ARM32 kernel, resulting in negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode() taking over the mode generation (badly), and a horrible mess on screen. > +    vfp = vfp_min + (porches_rem / 2); > +    vbp = porches - vfp; Relative position of the vertical sync within the VBI effectively moves the image up and down. Adding that (porches_rem / 2) moves the image up off center by that many pixels. I'd keep the VFP always at minimum to keep the image centered. Best regards, Mateusz Kwiatkowski 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 DD235ECAAA1 for ; Tue, 6 Sep 2022 20:31:52 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 8DF2510EA98; Tue, 6 Sep 2022 20:31:22 +0000 (UTC) Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by gabe.freedesktop.org (Postfix) with ESMTPS id A069B10E034; Wed, 31 Aug 2022 01:44:57 +0000 (UTC) Received: by mail-lf1-x12f.google.com with SMTP id j14so7604684lfu.4; Tue, 30 Aug 2022 18:44:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=EFsFMp+canY/hKJink3xfIjzSIs5nK0ATMHsiNLvxO07lOyOzwsvad8rVyDd2wmSeg cWCjphxF3TSGFFDKqyBmATL/jehCiz7Ylb6ULQwm/mKEET2IUotRhOyHBInnuwebAx/W o98i4bzpAYKvvI2sU//Sa2wKHsHNlK0Ik4KVSfjlV5E7mJwynZVYZggwdjMG3KRyfVMo xPbIu9xAaQjWAcnqbsVBfJmmxCjJL8AJzz30l+qNOOOt1j+HNC9Xoq1+ko3At8FQwbwc GgYRNngTbp8HBenXcqfoYJexOO63aFwrEEDYhvuQd/DuP/2j454qxLb21o5XEYcIYyWs Dtmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=a+9GKQ8x74vzchXjZqTcNUgtGaemIPuVs7S3SK/lrPtviFWj6C2cpBLfX3BImm9yA0 /rc1j+C/M7DZMKwr+wLSV4zHkOYyz5Iz0k2oDbo9fZ/7E3nOHWn/LmqGVXQkLnHNuop7 WIbedVKLV7nBu5ZsQIJNv9Y51+XVZNJEsHTDYdb4vVYiA1honmGvM2o1VrI9kMUP7Vhc s/ORtNOca6bnxWBx3iU5xouDkIwj7yraCwBbN0kY4NJLv/CYpQt8NZy7xtBXI7mzOl8I 8JT4Xr+ercV6pa4dB+LJhzPb/j4WDcT4VxVro59U65R0hAlSjCAqZGxmYZwSy1rOZGhd 0Ofw== X-Gm-Message-State: ACgBeo3+AdI7rwyaPkU2Wbq2xgeuBuVfyMdceHVqlGdwrTTKiPBsj9wh 7BV9l/bpFMGmshvddeMOZw0= X-Google-Smtp-Source: AA6agR5Z8V/gUf3Mh2HitGw7sMfYSuNjRXxRlM0XOW/4Yz6ST42Ov+CXOy5ZF8ScFy9ud/OgRIMt7A== X-Received: by 2002:ac2:4901:0:b0:494:88dc:7efc with SMTP id n1-20020ac24901000000b0049488dc7efcmr411746lfi.408.1661910295748; Tue, 30 Aug 2022 18:44:55 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:d40b:b088:5bfe:3b81? ([2a02:a31a:a240:1700:d40b:b088:5bfe:3b81]) by smtp.googlemail.com with ESMTPSA id g1-20020a0565123b8100b004948f583e6bsm42248lfv.138.2022.08.30.18.44.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 18:44:55 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com> Date: Wed, 31 Aug 2022 03:44:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Content-Language: pl To: Maxime Ripard , Maxime Ripard , Ben Skeggs , David Airlie , Chen-Yu Tsai , Thomas Zimmermann , Jani Nikula , Lyude Paul , Philipp Zabel , Maarten Lankhorst , Rodrigo Vivi , Tvrtko Ursulin , Jernej Skrabec , Samuel Holland , Karol Herbst , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Emma Anholt , Daniel Vetter , Joonas Lahtinen References: <20220728-rpi-analog-tv-properties-v2-0-459522d653a7@cerno.tech> <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Mailman-Approved-At: Tue, 06 Sep 2022 20:31:03 +0000 Subject: Re: [Nouveau] [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes 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: Dom Cobley , nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-sunxi@lists.linux.dev, Hans de Goede , Geert Uytterhoeven , Phil Elwell , linux-arm-kernel@lists.infradead.org Errors-To: nouveau-bounces@lists.freedesktop.org Sender: "Nouveau" Hi Maxime, Wow. That's an enormous amount of effort put into this patch. But I'm tempted to say that this is actually overengineered quite a bit :D Considering that there's no way to access all these calculations from user space, and I can't imagine anybody using anything else than those standard 480i/576i (and maybe 240p/288p) modes at 13.5 MHz any time soon... I'm not sure if we actually need all this. But anyway, I'm not the maintainer of this subsystem, so I'm not the one to decide. > +enum drm_mode_analog { > +    DRM_MODE_ANALOG_NTSC, > +    DRM_MODE_ANALOG_PAL, > +}; Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is common, but strictly speaking a misnomer. Those are color encoding systems, and your patchset fully supports lesser used, but standard encodings for those (e.g. PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more neutral naming scheme. Some ideas: - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh rate) - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line   count) - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System   Letter Designations) > +#define NTSC_HFP_DURATION_TYP_NS    1500 > +#define NTSC_HFP_DURATION_MIN_NS    1270 > +#define NTSC_HFP_DURATION_MAX_NS    2220 You've defined those min/typ/max ranges, but you're not using the "typ" field for anything other than hslen. The actual "typical" value is thus always the midpoint, which isn't necessarily the best choice. In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 requested by BT.601. That's all obviously within tolerances, but the image ends up noticeably off-center (at least on modern TVs), especially in the 576i case. > +    htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC; You're multiplying an unsigned int and an unsigned long - both types are only required to be 32 bit, so this is likely to overflow. You need to use a cast to unsigned long long, and then call do_div() for 64-bit division. This actually overflowed on me on my Pi running ARM32 kernel, resulting in negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode() taking over the mode generation (badly), and a horrible mess on screen. > +    vfp = vfp_min + (porches_rem / 2); > +    vbp = porches - vfp; Relative position of the vertical sync within the VBI effectively moves the image up and down. Adding that (porches_rem / 2) moves the image up off center by that many pixels. I'd keep the VFP always at minimum to keep the image centered. Best regards, Mateusz Kwiatkowski 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 59AA3ECAAD5 for ; Wed, 31 Aug 2022 01:46:22 +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:References:Cc:To:Subject: MIME-Version:Date:Message-ID:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6n9+HYioEo7BxT6HZas8cSiFbMfNNkiVslU4cw0LTFo=; b=nHEX900Kn2ZoAZ 8x4qOgKmUD/JsRZg4RzyPkwokvS75Spbej9LSdq2ggWFlxk2myIxWknPr/4ZEwA9kiE9V1INgOkKl 2Fvoh2hYkzvXvwVLfp6spCyWkoZofn0fCaZA9nJNXg/RvdLWJIP+67vyappFu3AQVzSaFv+ooWog1 0F6H+YuneSwuJA3khpHnktUON5yAtNgrZZpyuw8CzIfwPRON5dfXP7Qyv6UQ5lzfOs7a8HIp9Lsk2 rXmIQnjw2FTgSTpd+jZNZB+yP3U8uVz8gz1XFSqzfUSEdEuesLEpaihp0GyjKHL22EK8wf1ot4DBj qLUiJc3gQ2K+MX+UH8Rw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTCmx-0034aj-5o; Wed, 31 Aug 2022 01:45:07 +0000 Received: from mail-lf1-x133.google.com ([2a00:1450:4864:20::133]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTCmt-0034YG-U9 for linux-arm-kernel@lists.infradead.org; Wed, 31 Aug 2022 01:45:05 +0000 Received: by mail-lf1-x133.google.com with SMTP id z6so17936467lfu.9 for ; Tue, 30 Aug 2022 18:44:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=EFsFMp+canY/hKJink3xfIjzSIs5nK0ATMHsiNLvxO07lOyOzwsvad8rVyDd2wmSeg cWCjphxF3TSGFFDKqyBmATL/jehCiz7Ylb6ULQwm/mKEET2IUotRhOyHBInnuwebAx/W o98i4bzpAYKvvI2sU//Sa2wKHsHNlK0Ik4KVSfjlV5E7mJwynZVYZggwdjMG3KRyfVMo xPbIu9xAaQjWAcnqbsVBfJmmxCjJL8AJzz30l+qNOOOt1j+HNC9Xoq1+ko3At8FQwbwc GgYRNngTbp8HBenXcqfoYJexOO63aFwrEEDYhvuQd/DuP/2j454qxLb21o5XEYcIYyWs Dtmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=qK4E2mRBZgFHpd0RLxCrGoRBfCEWArjbec1Wt1jXqQ68sAYheh3keYv9PDVHgyY7hP 6of5p8eU4wVJIeEfNqu0jXL2lZef0gZsge61LsEvcgxDdKfl16lVSlunOFpCDdZNAtiH h+cczA5e1PYMG8Rn5nLVVIlvLvl6pFQs/5e08tJuLEDP6evXT8OutZY5ag/uVqdVbdo1 fX2YaPmg2oUx9R00UWo6x6loM+eATvpEr3X7LN8NB6z8oh5znYBibdo1emEvzCRf7QNA lnZnyDG6dKruqiVZ+vItFKRYF4kBIj3CqGggXI2LTZ27d/t7BZnwLfQzAZTBCfYnzgUW B2Yw== X-Gm-Message-State: ACgBeo1Q7DN8k6PcYq8QQiXDL49sb1YKVqWvrigNABqL+Wqv6sXqrn4K MtENECmdnw2BSFaZSDnEsYQ= X-Google-Smtp-Source: AA6agR5Z8V/gUf3Mh2HitGw7sMfYSuNjRXxRlM0XOW/4Yz6ST42Ov+CXOy5ZF8ScFy9ud/OgRIMt7A== X-Received: by 2002:ac2:4901:0:b0:494:88dc:7efc with SMTP id n1-20020ac24901000000b0049488dc7efcmr411746lfi.408.1661910295748; Tue, 30 Aug 2022 18:44:55 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:d40b:b088:5bfe:3b81? ([2a02:a31a:a240:1700:d40b:b088:5bfe:3b81]) by smtp.googlemail.com with ESMTPSA id g1-20020a0565123b8100b004948f583e6bsm42248lfv.138.2022.08.30.18.44.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 18:44:55 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com> Date: Wed, 31 Aug 2022 03:44:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes Content-Language: pl To: Maxime Ripard , Maxime Ripard , Ben Skeggs , David Airlie , Chen-Yu Tsai , Thomas Zimmermann , Jani Nikula , Lyude Paul , Philipp Zabel , Maarten Lankhorst , Rodrigo Vivi , Tvrtko Ursulin , Jernej Skrabec , Samuel Holland , Karol Herbst , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Emma Anholt , Daniel Vetter , Joonas Lahtinen Cc: Hans de Goede , linux-arm-kernel@lists.infradead.org, Phil Elwell , intel-gfx@lists.freedesktop.org, Dave Stevenson , dri-devel@lists.freedesktop.org, Dom Cobley , linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org, linux-sunxi@lists.linux.dev, Geert Uytterhoeven References: <20220728-rpi-analog-tv-properties-v2-0-459522d653a7@cerno.tech> <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220830_184503_995493_1BB1A527 X-CRM114-Status: GOOD ( 17.66 ) 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="utf-8" Content-Transfer-Encoding: base64 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org SGkgTWF4aW1lLAoKV293LiBUaGF0J3MgYW4gZW5vcm1vdXMgYW1vdW50IG9mIGVmZm9ydCBwdXQg aW50byB0aGlzIHBhdGNoLgoKQnV0IEknbSB0ZW1wdGVkIHRvIHNheSB0aGF0IHRoaXMgaXMgYWN0 dWFsbHkgb3ZlcmVuZ2luZWVyZWQgcXVpdGUgYSBiaXQgOkQKQ29uc2lkZXJpbmcgdGhhdCB0aGVy ZSdzIG5vIHdheSB0byBhY2Nlc3MgYWxsIHRoZXNlIGNhbGN1bGF0aW9ucyBmcm9tIHVzZXIKc3Bh Y2UsIGFuZCBJIGNhbid0IGltYWdpbmUgYW55Ym9keSB1c2luZyBhbnl0aGluZyBlbHNlIHRoYW4g dGhvc2Ugc3RhbmRhcmQKNDgwaS81NzZpIChhbmQgbWF5YmUgMjQwcC8yODhwKSBtb2RlcyBhdCAx My41IE1IeiBhbnkgdGltZSBzb29uLi4uIEknbSBub3QKc3VyZSBpZiB3ZSBhY3R1YWxseSBuZWVk IGFsbCB0aGlzLgoKQnV0IGFueXdheSwgSSdtIG5vdCB0aGUgbWFpbnRhaW5lciBvZiB0aGlzIHN1 YnN5c3RlbSwgc28gSSdtIG5vdCB0aGUgb25lIHRvCmRlY2lkZS4KCj4gK2VudW0gZHJtX21vZGVf YW5hbG9nIHsKPiArwqDCoCDCoERSTV9NT0RFX0FOQUxPR19OVFNDLAo+ICvCoMKgIMKgRFJNX01P REVfQU5BTE9HX1BBTCwKPiArfTsKClVzaW5nICJOVFNDIiBhbmQgIlBBTCIgdG8gZGVzY3JpYmUg dGhlIDUwSHogYW5kIDYwSHogYW5hbG9nIFRWIG1vZGVzIGlzIGNvbW1vbiwKYnV0IHN0cmljdGx5 IHNwZWFraW5nIGEgbWlzbm9tZXIuIFRob3NlIGFyZSBjb2xvciBlbmNvZGluZyBzeXN0ZW1zLCBh bmQgeW91cgpwYXRjaHNldCBmdWxseSBzdXBwb3J0cyBsZXNzZXIgdXNlZCwgYnV0IHN0YW5kYXJk IGVuY29kaW5ncyBmb3IgdGhvc2UgKGUuZy4KUEFMLU0gZm9yIDYwSHogYW5kIFNFQ0FNIGZvciA1 MEh6KS4gSSdkIHByb3Bvc2Ugc3dpdGNoaW5nIHRvIHNvbWUgbW9yZSBuZXV0cmFsCm5hbWluZyBz Y2hlbWUuIFNvbWUgaWRlYXM6CgotIERSTV9NT0RFX0FOQUxPR182MF9IWiAvIERSTV9NT0RFX0FO QUxPR181MF9IWiAoYWZ0ZXIgc3RhbmRhcmQgcmVmcmVzaCByYXRlKQotIERSTV9NT0RFX0FOQUxP R181MjVfTElORVMgLyBEUk1fTU9ERV9BTkFMT0dfNjI1X0xJTkVTIChhZnRlciBzdGFuZGFyZCBs aW5lCsKgIGNvdW50KQotIERSTV9NT0RFX0FOQUxPR19KTSAvIERSTV9NT0RFX0FOQUxPR19CREdI SUtMTiAoYWZ0ZXIgY29ycmVzcG9uZGluZyBJVFUgU3lzdGVtCsKgIExldHRlciBEZXNpZ25hdGlv bnMpCgo+ICsjZGVmaW5lIE5UU0NfSEZQX0RVUkFUSU9OX1RZUF9OU8KgwqAgwqAxNTAwCj4gKyNk ZWZpbmUgTlRTQ19IRlBfRFVSQVRJT05fTUlOX05TwqDCoCDCoDEyNzAKPiArI2RlZmluZSBOVFND X0hGUF9EVVJBVElPTl9NQVhfTlPCoMKgIMKgMjIyMAoKWW91J3ZlIGRlZmluZWQgdGhvc2UgbWlu L3R5cC9tYXggcmFuZ2VzLCBidXQgeW91J3JlIG5vdCB1c2luZyB0aGUgInR5cCIgZmllbGQKZm9y IGFueXRoaW5nIG90aGVyIHRoYW4gaHNsZW4uIFRoZSBhY3R1YWwgInR5cGljYWwiIHZhbHVlIGlz IHRodXMgYWx3YXlzIHRoZQptaWRwb2ludCwgd2hpY2ggaXNuJ3QgbmVjZXNzYXJpbHkgdGhlIGJl c3QgY2hvaWNlLgoKSW4gcGFydGljdWxhciwgZm9yIHRoZSBzdGFuZGFyZCA3MjBweCB3aWRlIG1v ZGVzIGF0IDEzLjUgTUh6LCBoc3luY19zdGFydAplbmRzIHVwIGJlaW5nIDczNSBmb3IgNDgwaSBh bmQgNzM0IGZvciA1NzZpLCBpbnN0ZWFkIG9mIDczNiBhbmQgNzMyIHJlcXVlc3RlZApieSBCVC42 MDEuIFRoYXQncyBhbGwgb2J2aW91c2x5IHdpdGhpbiB0b2xlcmFuY2VzLCBidXQgdGhlIGltYWdl IGVuZHMgdXAKbm90aWNlYWJseSBvZmYtY2VudGVyIChhdCBsZWFzdCBvbiBtb2Rlcm4gVFZzKSwg ZXNwZWNpYWxseSBpbiB0aGUgNTc2aSBjYXNlLgoKPiArwqDCoCDCoGh0b3RhbCA9IHBhcmFtcy0+ bGluZV9kdXJhdGlvbl9ucyAqIHBpeGVsX2Nsb2NrX2h6IC8gTlNFQ19QRVJfU0VDOwoKWW91J3Jl IG11bHRpcGx5aW5nIGFuIHVuc2lnbmVkIGludCBhbmQgYW4gdW5zaWduZWQgbG9uZyAtIGJvdGgg dHlwZXMgYXJlIG9ubHkKcmVxdWlyZWQgdG8gYmUgMzIgYml0LCBzbyB0aGlzIGlzIGxpa2VseSB0 byBvdmVyZmxvdy4gWW91IG5lZWQgdG8gdXNlIGEgY2FzdCB0bwp1bnNpZ25lZCBsb25nIGxvbmcs IGFuZCB0aGVuIGNhbGwgZG9fZGl2KCkgZm9yIDY0LWJpdCBkaXZpc2lvbi4KClRoaXMgYWN0dWFs bHkgb3ZlcmZsb3dlZCBvbiBtZSBvbiBteSBQaSBydW5uaW5nIEFSTTMyIGtlcm5lbCwgcmVzdWx0 aW5nIGluCm5lZ2F0aXZlIGhvcml6b250YWwgcG9yY2ggbGVuZ3RocywgYW5kIGRybV9oZWxwZXJf cHJvYmVfYWRkX2NtZGxpbmVfbW9kZSgpCnRha2luZyBvdmVyIHRoZSBtb2RlIGdlbmVyYXRpb24g KGJhZGx5KSwgYW5kIGEgaG9ycmlibGUgbWVzcyBvbiBzY3JlZW4uCgo+ICvCoMKgIMKgdmZwID0g dmZwX21pbiArIChwb3JjaGVzX3JlbSAvIDIpOwo+ICvCoMKgIMKgdmJwID0gcG9yY2hlcyAtIHZm cDsKClJlbGF0aXZlIHBvc2l0aW9uIG9mIHRoZSB2ZXJ0aWNhbCBzeW5jIHdpdGhpbiB0aGUgVkJJ IGVmZmVjdGl2ZWx5IG1vdmVzIHRoZQppbWFnZSB1cCBhbmQgZG93bi4gQWRkaW5nIHRoYXQgKHBv cmNoZXNfcmVtIC8gMikgbW92ZXMgdGhlIGltYWdlIHVwIG9mZiBjZW50ZXIKYnkgdGhhdCBtYW55 IHBpeGVscy4gSSdkIGtlZXAgdGhlIFZGUCBhbHdheXMgYXQgbWluaW11bSB0byBrZWVwIHRoZSBp bWFnZQpjZW50ZXJlZC4KCkJlc3QgcmVnYXJkcywKTWF0ZXVzeiBLd2lhdGtvd3NraQoKX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5l bCBtYWlsaW5nIGxpc3QKbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6 Ly9saXN0cy5pbmZyYWRlYWQub3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo= 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 7B091ECAAD5 for ; Wed, 31 Aug 2022 01:45:03 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DE51C10E034; Wed, 31 Aug 2022 01:45:00 +0000 (UTC) Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by gabe.freedesktop.org (Postfix) with ESMTPS id A069B10E034; Wed, 31 Aug 2022 01:44:57 +0000 (UTC) Received: by mail-lf1-x12f.google.com with SMTP id j14so7604684lfu.4; Tue, 30 Aug 2022 18:44:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=EFsFMp+canY/hKJink3xfIjzSIs5nK0ATMHsiNLvxO07lOyOzwsvad8rVyDd2wmSeg cWCjphxF3TSGFFDKqyBmATL/jehCiz7Ylb6ULQwm/mKEET2IUotRhOyHBInnuwebAx/W o98i4bzpAYKvvI2sU//Sa2wKHsHNlK0Ik4KVSfjlV5E7mJwynZVYZggwdjMG3KRyfVMo xPbIu9xAaQjWAcnqbsVBfJmmxCjJL8AJzz30l+qNOOOt1j+HNC9Xoq1+ko3At8FQwbwc GgYRNngTbp8HBenXcqfoYJexOO63aFwrEEDYhvuQd/DuP/2j454qxLb21o5XEYcIYyWs Dtmg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:x-gm-message-state:from:to:cc; bh=drG6RMH0zFREgohP7fUu6e8MBztAwMPOm+LE74724sA=; b=a+9GKQ8x74vzchXjZqTcNUgtGaemIPuVs7S3SK/lrPtviFWj6C2cpBLfX3BImm9yA0 /rc1j+C/M7DZMKwr+wLSV4zHkOYyz5Iz0k2oDbo9fZ/7E3nOHWn/LmqGVXQkLnHNuop7 WIbedVKLV7nBu5ZsQIJNv9Y51+XVZNJEsHTDYdb4vVYiA1honmGvM2o1VrI9kMUP7Vhc s/ORtNOca6bnxWBx3iU5xouDkIwj7yraCwBbN0kY4NJLv/CYpQt8NZy7xtBXI7mzOl8I 8JT4Xr+ercV6pa4dB+LJhzPb/j4WDcT4VxVro59U65R0hAlSjCAqZGxmYZwSy1rOZGhd 0Ofw== X-Gm-Message-State: ACgBeo3+AdI7rwyaPkU2Wbq2xgeuBuVfyMdceHVqlGdwrTTKiPBsj9wh 7BV9l/bpFMGmshvddeMOZw0= X-Google-Smtp-Source: AA6agR5Z8V/gUf3Mh2HitGw7sMfYSuNjRXxRlM0XOW/4Yz6ST42Ov+CXOy5ZF8ScFy9ud/OgRIMt7A== X-Received: by 2002:ac2:4901:0:b0:494:88dc:7efc with SMTP id n1-20020ac24901000000b0049488dc7efcmr411746lfi.408.1661910295748; Tue, 30 Aug 2022 18:44:55 -0700 (PDT) Received: from ?IPV6:2a02:a31a:a240:1700:d40b:b088:5bfe:3b81? ([2a02:a31a:a240:1700:d40b:b088:5bfe:3b81]) by smtp.googlemail.com with ESMTPSA id g1-20020a0565123b8100b004948f583e6bsm42248lfv.138.2022.08.30.18.44.53 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 18:44:55 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: <242d272b-5b79-986c-9aaf-64e62f6b37ff@gmail.com> Date: Wed, 31 Aug 2022 03:44:52 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.2.0 Subject: Re: [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes Content-Language: pl To: Maxime Ripard , Maxime Ripard , Ben Skeggs , David Airlie , Chen-Yu Tsai , Thomas Zimmermann , Jani Nikula , Lyude Paul , Philipp Zabel , Maarten Lankhorst , Rodrigo Vivi , Tvrtko Ursulin , Jernej Skrabec , Samuel Holland , Karol Herbst , =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= , Emma Anholt , Daniel Vetter , Joonas Lahtinen References: <20220728-rpi-analog-tv-properties-v2-0-459522d653a7@cerno.tech> <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-10-459522d653a7@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: Dom Cobley , Dave Stevenson , nouveau@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-sunxi@lists.linux.dev, Hans de Goede , Geert Uytterhoeven , Phil Elwell , linux-arm-kernel@lists.infradead.org Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Maxime, Wow. That's an enormous amount of effort put into this patch. But I'm tempted to say that this is actually overengineered quite a bit :D Considering that there's no way to access all these calculations from user space, and I can't imagine anybody using anything else than those standard 480i/576i (and maybe 240p/288p) modes at 13.5 MHz any time soon... I'm not sure if we actually need all this. But anyway, I'm not the maintainer of this subsystem, so I'm not the one to decide. > +enum drm_mode_analog { > +    DRM_MODE_ANALOG_NTSC, > +    DRM_MODE_ANALOG_PAL, > +}; Using "NTSC" and "PAL" to describe the 50Hz and 60Hz analog TV modes is common, but strictly speaking a misnomer. Those are color encoding systems, and your patchset fully supports lesser used, but standard encodings for those (e.g. PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some more neutral naming scheme. Some ideas: - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard refresh rate) - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after standard line   count) - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System   Letter Designations) > +#define NTSC_HFP_DURATION_TYP_NS    1500 > +#define NTSC_HFP_DURATION_MIN_NS    1270 > +#define NTSC_HFP_DURATION_MAX_NS    2220 You've defined those min/typ/max ranges, but you're not using the "typ" field for anything other than hslen. The actual "typical" value is thus always the midpoint, which isn't necessarily the best choice. In particular, for the standard 720px wide modes at 13.5 MHz, hsync_start ends up being 735 for 480i and 734 for 576i, instead of 736 and 732 requested by BT.601. That's all obviously within tolerances, but the image ends up noticeably off-center (at least on modern TVs), especially in the 576i case. > +    htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC; You're multiplying an unsigned int and an unsigned long - both types are only required to be 32 bit, so this is likely to overflow. You need to use a cast to unsigned long long, and then call do_div() for 64-bit division. This actually overflowed on me on my Pi running ARM32 kernel, resulting in negative horizontal porch lengths, and drm_helper_probe_add_cmdline_mode() taking over the mode generation (badly), and a horrible mess on screen. > +    vfp = vfp_min + (porches_rem / 2); > +    vbp = porches - vfp; Relative position of the vertical sync within the VBI effectively moves the image up and down. Adding that (porches_rem / 2) moves the image up off center by that many pixels. I'd keep the VFP always at minimum to keep the image centered. Best regards, Mateusz Kwiatkowski