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 3382BC54EE9 for ; Tue, 6 Sep 2022 12:35:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 18C0610E680; Tue, 6 Sep 2022 12:34:23 +0000 (UTC) Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 758A010E15B; Wed, 31 Aug 2022 02:23:26 +0000 (UTC) Received: by mail-lf1-x12b.google.com with SMTP id g7so1072507lfe.11; Tue, 30 Aug 2022 19:23:26 -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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=eyXn8XNL5R+c7skqJd2CXAmkkmv+PO53H99rqyOH87xBrXhsqaRc9Cafy09+sKEMJA iijHh/Vgr/N8ms8DrRs7ZsyH/kfkPXSLcSAHRoh1+w4hJSgtLHkhgFqawSk+ZXu7vLVu etWsQ2c8F5mlpD6pXL4Jnwu7reEvEQPvt2Kcq7iIfhsfn4Yu0eFb5/ZnRE0d433DYCkK cdghwZHl1psSvBmHBmPiKQam64a3GgKwsjlObk+WtdqaHxXkPRZOKXlEsDxfpMbOFJ+F qvawNpaI/5HTMI7FX7pKKny6H0CoBufGQKBaV+h3B0J6ooncWt1iyUW0Dhc0OQr52xk/ Icrg== 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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=ONsodCQJDYGLAzmir2o5yxXoNw+Csqxt2vulJxJk3aNqmZmDXH4sxAuK7ITh3DddZj GOURpw3Sz7//7Ipg25hgxH6qcI3u2Jc2zqgch9rG0lC1QmYXO/tyT7x8UzEK0BGDeg7m 7N0ZiLVFqF3Rk9vMS/7h6OWQgPcRRva4Sxq1zik1DTc/PXB+9Bj2xl94hFWJJIhg7gbl Re7JKKPdZbWi/RGNHZaMCYYj8qHAux5sxjfbfA16PBeTeZq3KSYp1NIpxGuj0IbSo5Mk mgvOu7XcYte4oeUkvC0f8cCYxPF4SFlQ0pTUOZfJogw8xnBJIhX2/XvqVNI9fy/tweOi RGmQ== X-Gm-Message-State: ACgBeo2eAjWedWz0Cs9xKe4AwX4VNlZyuqZH+ylp+UnB6GbcS7BxMNdO J4N+DgJiXWg/nLdOqAA5Xe8= X-Google-Smtp-Source: AA6agR5RnfuFyIONMTqsPbIII5CYS1rbgB3cSxzqkqMVekfKEnddXN6p9IGLZ6Lzgilorerw0vcwmQ== X-Received: by 2002:a05:6512:1054:b0:494:8ce3:24c8 with SMTP id c20-20020a056512105400b004948ce324c8mr235583lfb.360.1661912604645; Tue, 30 Aug 2022 19:23:24 -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 k9-20020a2eb749000000b00261eb75fa5dsm1960508ljo.41.2022.08.30.19.23.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 19:23:24 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: Date: Wed, 31 Aug 2022 04:23:21 +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-32-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-32-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 32/41] drm/vc4: vec: Convert to the new TV mode property 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, I tested your patchset on my Pi and it mostly works. Good work! However, I noticed a couple of issues. > -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > -                    struct drm_crtc_state *crtc_state, > -                    struct drm_connector_state *conn_state) > -{ > -    const struct vc4_vec_tv_mode *vec_mode; > - > -    vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode]; > - > -    if (conn_state->crtc && > -        !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > -        return -EINVAL; > - > -    return 0; > -} I may have said it myself that we should allow custom modelines without too much validation. The VC4 and VEC, however, have some considerable limitations when it comes to the modelines that they can reliably output. In particular, attempting to use "50 Hz" timings in NTSC/PAL-M modes (or "60 Hz" in PAL/SECAM modes) results in a weirdly skewed image. Here's how it may look like: https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png This is because if the CRTC does not trigger the sync pulses within an acceptable time window, the VEC apparently generates them itself. This causes the VEC sync pulses (which go onto the wire) not quite line up with the ones from the modeline, which results in what you see on the screenshot. I once wrote a validation function based on extensive testing of what produces a sensible output and what doesn't. You can find it here: https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it might be a good idea to include something like that - even though I know it's somewhat ugly. (BTW, those %2 checks on vertical timings in that linked commit can be ignored; those values are divided by 2 for interlaced modes anyway. Those checks were intended to ensure proper odd-first or even-first timings; I'm not sure if your code calculates those in the same way) >  static int vc4_vec_connector_get_modes(struct drm_connector *connector) >  { > -    struct drm_connector_state *state = connector->state; >      struct drm_display_mode *mode; > +    int count = 0; >   > -    mode = drm_mode_duplicate(connector->dev, > -                  vc4_vec_tv_modes[state->tv.mode].mode); > +    mode = drm_mode_analog_ntsc_480i(connector->dev); >      if (!mode) { >          DRM_ERROR("Failed to create a new display mode\n"); >          return -ENOMEM; >      } >   >      drm_mode_probed_add(connector, mode); > +    count += 1; >   > -    return 1; > +    mode = drm_mode_analog_pal_576i(connector->dev); > +    if (!mode) { > +        DRM_ERROR("Failed to create a new display mode\n"); > +        return -ENOMEM; > +    } > + > +    drm_mode_probed_add(connector, mode); > +    count += 1; > + > +    return count; > +} Xorg is pretty confused by these modes being reported like that. The 576i mode is *always* preferred, presumably because of the higher resolution. If the NTSC mode is set (via the kernel cmdline or just due to it being the default), this results in a mess on the screen - exactly the same thing as on the screenshot linked above. Note that drm_helper_probe_add_cmdline_mode() *does* add the DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as preferred on the command line - but Xorg does not seem to care about that. I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode that corresponds to the currently chosen tv_mode - that would fix the problem. An alternative would be to _not_ add the "opposite" mode at all, like the current default Raspberry Pi OS kernel behaves. Note that if you decide to add the modeline validation like I suggested in the comment above, then without setting the preferred mode properly, Xorg will just give up and sit on a blank screen until you run xrandr from another terminal if tv_mode incompatible with 576i is selected. Best regards, Mateusz Kwiatkowski From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f51.google.com (mail-lf1-f51.google.com [209.85.167.51]) (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 A6E40620 for ; Wed, 31 Aug 2022 02:23:26 +0000 (UTC) Received: by mail-lf1-f51.google.com with SMTP id p7so6902093lfu.3 for ; Tue, 30 Aug 2022 19:23:26 -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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=eyXn8XNL5R+c7skqJd2CXAmkkmv+PO53H99rqyOH87xBrXhsqaRc9Cafy09+sKEMJA iijHh/Vgr/N8ms8DrRs7ZsyH/kfkPXSLcSAHRoh1+w4hJSgtLHkhgFqawSk+ZXu7vLVu etWsQ2c8F5mlpD6pXL4Jnwu7reEvEQPvt2Kcq7iIfhsfn4Yu0eFb5/ZnRE0d433DYCkK cdghwZHl1psSvBmHBmPiKQam64a3GgKwsjlObk+WtdqaHxXkPRZOKXlEsDxfpMbOFJ+F qvawNpaI/5HTMI7FX7pKKny6H0CoBufGQKBaV+h3B0J6ooncWt1iyUW0Dhc0OQr52xk/ Icrg== 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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=vw5YtZqCBVdjh5V4AxULEXrWvHmidlqIblActAMqrLTTSoI6gCCFBjjbdIw+7raAq8 xH48V7x67y7EhHKDbPJAZJ85jY8t5n47AQjz/jtLesdiaJKejiSlRz6DDR9V0zUy59Vp eto6LjY3wE/8rwisddgWIX199CRAriD6OXvsbvBh09Vp0bGxePwRwQ5u+PGe/laUUaGf yispj3Fy99fkBwB09xBv4yu+ve6XTUihDq8ioXqvPvcz/Hw5+CZKc2MrjOZoBA2TThwu xSMFZXjOdZBEAYTT/fySP5oNL9w5q14u8pFSWYkpiw1l+5SvvWCtR88pZbxCYHbyBRMR +7fw== X-Gm-Message-State: ACgBeo1owes2VHi/BJ9G8KjNxH25v34kX8nA3D3PQP9Ggv/vd9BTjI8V y4MxYpbvhbJUs2+bUV+Y8fM= X-Google-Smtp-Source: AA6agR5RnfuFyIONMTqsPbIII5CYS1rbgB3cSxzqkqMVekfKEnddXN6p9IGLZ6Lzgilorerw0vcwmQ== X-Received: by 2002:a05:6512:1054:b0:494:8ce3:24c8 with SMTP id c20-20020a056512105400b004948ce324c8mr235583lfb.360.1661912604645; Tue, 30 Aug 2022 19:23:24 -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 k9-20020a2eb749000000b00261eb75fa5dsm1960508ljo.41.2022.08.30.19.23.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 19:23:24 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: Date: Wed, 31 Aug 2022 04:23:21 +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 32/41] drm/vc4: vec: Convert to the new TV mode property 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-32-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-32-459522d653a7@cerno.tech> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hi Maxime, I tested your patchset on my Pi and it mostly works. Good work! However, I noticed a couple of issues. > -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > -                    struct drm_crtc_state *crtc_state, > -                    struct drm_connector_state *conn_state) > -{ > -    const struct vc4_vec_tv_mode *vec_mode; > - > -    vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode]; > - > -    if (conn_state->crtc && > -        !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > -        return -EINVAL; > - > -    return 0; > -} I may have said it myself that we should allow custom modelines without too much validation. The VC4 and VEC, however, have some considerable limitations when it comes to the modelines that they can reliably output. In particular, attempting to use "50 Hz" timings in NTSC/PAL-M modes (or "60 Hz" in PAL/SECAM modes) results in a weirdly skewed image. Here's how it may look like: https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png This is because if the CRTC does not trigger the sync pulses within an acceptable time window, the VEC apparently generates them itself. This causes the VEC sync pulses (which go onto the wire) not quite line up with the ones from the modeline, which results in what you see on the screenshot. I once wrote a validation function based on extensive testing of what produces a sensible output and what doesn't. You can find it here: https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it might be a good idea to include something like that - even though I know it's somewhat ugly. (BTW, those %2 checks on vertical timings in that linked commit can be ignored; those values are divided by 2 for interlaced modes anyway. Those checks were intended to ensure proper odd-first or even-first timings; I'm not sure if your code calculates those in the same way) >  static int vc4_vec_connector_get_modes(struct drm_connector *connector) >  { > -    struct drm_connector_state *state = connector->state; >      struct drm_display_mode *mode; > +    int count = 0; >   > -    mode = drm_mode_duplicate(connector->dev, > -                  vc4_vec_tv_modes[state->tv.mode].mode); > +    mode = drm_mode_analog_ntsc_480i(connector->dev); >      if (!mode) { >          DRM_ERROR("Failed to create a new display mode\n"); >          return -ENOMEM; >      } >   >      drm_mode_probed_add(connector, mode); > +    count += 1; >   > -    return 1; > +    mode = drm_mode_analog_pal_576i(connector->dev); > +    if (!mode) { > +        DRM_ERROR("Failed to create a new display mode\n"); > +        return -ENOMEM; > +    } > + > +    drm_mode_probed_add(connector, mode); > +    count += 1; > + > +    return count; > +} Xorg is pretty confused by these modes being reported like that. The 576i mode is *always* preferred, presumably because of the higher resolution. If the NTSC mode is set (via the kernel cmdline or just due to it being the default), this results in a mess on the screen - exactly the same thing as on the screenshot linked above. Note that drm_helper_probe_add_cmdline_mode() *does* add the DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as preferred on the command line - but Xorg does not seem to care about that. I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode that corresponds to the currently chosen tv_mode - that would fix the problem. An alternative would be to _not_ add the "opposite" mode at all, like the current default Raspberry Pi OS kernel behaves. Note that if you decide to add the modeline validation like I suggested in the comment above, then without setting the preferred mode properly, Xorg will just give up and sit on a blank screen until you run xrandr from another terminal if tv_mode incompatible with 576i is selected. 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 2B65FECAAA1 for ; Tue, 6 Sep 2022 20:32:18 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 777C110EAA8; Tue, 6 Sep 2022 20:31:30 +0000 (UTC) Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 758A010E15B; Wed, 31 Aug 2022 02:23:26 +0000 (UTC) Received: by mail-lf1-x12b.google.com with SMTP id g7so1072507lfe.11; Tue, 30 Aug 2022 19:23:26 -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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=eyXn8XNL5R+c7skqJd2CXAmkkmv+PO53H99rqyOH87xBrXhsqaRc9Cafy09+sKEMJA iijHh/Vgr/N8ms8DrRs7ZsyH/kfkPXSLcSAHRoh1+w4hJSgtLHkhgFqawSk+ZXu7vLVu etWsQ2c8F5mlpD6pXL4Jnwu7reEvEQPvt2Kcq7iIfhsfn4Yu0eFb5/ZnRE0d433DYCkK cdghwZHl1psSvBmHBmPiKQam64a3GgKwsjlObk+WtdqaHxXkPRZOKXlEsDxfpMbOFJ+F qvawNpaI/5HTMI7FX7pKKny6H0CoBufGQKBaV+h3B0J6ooncWt1iyUW0Dhc0OQr52xk/ Icrg== 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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=ONsodCQJDYGLAzmir2o5yxXoNw+Csqxt2vulJxJk3aNqmZmDXH4sxAuK7ITh3DddZj GOURpw3Sz7//7Ipg25hgxH6qcI3u2Jc2zqgch9rG0lC1QmYXO/tyT7x8UzEK0BGDeg7m 7N0ZiLVFqF3Rk9vMS/7h6OWQgPcRRva4Sxq1zik1DTc/PXB+9Bj2xl94hFWJJIhg7gbl Re7JKKPdZbWi/RGNHZaMCYYj8qHAux5sxjfbfA16PBeTeZq3KSYp1NIpxGuj0IbSo5Mk mgvOu7XcYte4oeUkvC0f8cCYxPF4SFlQ0pTUOZfJogw8xnBJIhX2/XvqVNI9fy/tweOi RGmQ== X-Gm-Message-State: ACgBeo2eAjWedWz0Cs9xKe4AwX4VNlZyuqZH+ylp+UnB6GbcS7BxMNdO J4N+DgJiXWg/nLdOqAA5Xe8= X-Google-Smtp-Source: AA6agR5RnfuFyIONMTqsPbIII5CYS1rbgB3cSxzqkqMVekfKEnddXN6p9IGLZ6Lzgilorerw0vcwmQ== X-Received: by 2002:a05:6512:1054:b0:494:8ce3:24c8 with SMTP id c20-20020a056512105400b004948ce324c8mr235583lfb.360.1661912604645; Tue, 30 Aug 2022 19:23:24 -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 k9-20020a2eb749000000b00261eb75fa5dsm1960508ljo.41.2022.08.30.19.23.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 19:23:24 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: Date: Wed, 31 Aug 2022 04:23:21 +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-32-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-32-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 32/41] drm/vc4: vec: Convert to the new TV mode property 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, I tested your patchset on my Pi and it mostly works. Good work! However, I noticed a couple of issues. > -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > -                    struct drm_crtc_state *crtc_state, > -                    struct drm_connector_state *conn_state) > -{ > -    const struct vc4_vec_tv_mode *vec_mode; > - > -    vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode]; > - > -    if (conn_state->crtc && > -        !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > -        return -EINVAL; > - > -    return 0; > -} I may have said it myself that we should allow custom modelines without too much validation. The VC4 and VEC, however, have some considerable limitations when it comes to the modelines that they can reliably output. In particular, attempting to use "50 Hz" timings in NTSC/PAL-M modes (or "60 Hz" in PAL/SECAM modes) results in a weirdly skewed image. Here's how it may look like: https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png This is because if the CRTC does not trigger the sync pulses within an acceptable time window, the VEC apparently generates them itself. This causes the VEC sync pulses (which go onto the wire) not quite line up with the ones from the modeline, which results in what you see on the screenshot. I once wrote a validation function based on extensive testing of what produces a sensible output and what doesn't. You can find it here: https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it might be a good idea to include something like that - even though I know it's somewhat ugly. (BTW, those %2 checks on vertical timings in that linked commit can be ignored; those values are divided by 2 for interlaced modes anyway. Those checks were intended to ensure proper odd-first or even-first timings; I'm not sure if your code calculates those in the same way) >  static int vc4_vec_connector_get_modes(struct drm_connector *connector) >  { > -    struct drm_connector_state *state = connector->state; >      struct drm_display_mode *mode; > +    int count = 0; >   > -    mode = drm_mode_duplicate(connector->dev, > -                  vc4_vec_tv_modes[state->tv.mode].mode); > +    mode = drm_mode_analog_ntsc_480i(connector->dev); >      if (!mode) { >          DRM_ERROR("Failed to create a new display mode\n"); >          return -ENOMEM; >      } >   >      drm_mode_probed_add(connector, mode); > +    count += 1; >   > -    return 1; > +    mode = drm_mode_analog_pal_576i(connector->dev); > +    if (!mode) { > +        DRM_ERROR("Failed to create a new display mode\n"); > +        return -ENOMEM; > +    } > + > +    drm_mode_probed_add(connector, mode); > +    count += 1; > + > +    return count; > +} Xorg is pretty confused by these modes being reported like that. The 576i mode is *always* preferred, presumably because of the higher resolution. If the NTSC mode is set (via the kernel cmdline or just due to it being the default), this results in a mess on the screen - exactly the same thing as on the screenshot linked above. Note that drm_helper_probe_add_cmdline_mode() *does* add the DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as preferred on the command line - but Xorg does not seem to care about that. I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode that corresponds to the currently chosen tv_mode - that would fix the problem. An alternative would be to _not_ add the "opposite" mode at all, like the current default Raspberry Pi OS kernel behaves. Note that if you decide to add the modeline validation like I suggested in the comment above, then without setting the preferred mode properly, Xorg will just give up and sit on a blank screen until you run xrandr from another terminal if tv_mode incompatible with 576i is selected. 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 C5A6DECAAD5 for ; Wed, 31 Aug 2022 02:24:48 +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=a6YRWWGBSWTGUJYMgzQEGcwFFis1R3/XWVV++eT1Q7o=; b=zxYu3dIRwHZ10R G+ag1Q8tIomuCgLX/CAlTjaGQD5QKV4cQsUFctFbX2YKw6R44qvl7gr6sgYoFf7f/gZhBh8U4HyQ5 LmIbuMH4Sfmgv3mo+y9d7T2lBBbPbH7ejT83wQjouDm1hY25crNpF3bRx4fC/EOHoXstBEMbuMuTC NEG4jaBZf9FGEJ7fwH8nLVfKsvTxWbiLzgyCYZMNRfT1IXQdkIpmonllFjJ4qmxiaZALaPOK9H3uq w1tJAiIzMll3dJstGRde1oBZNiJ6fMCfoWxgfUbyc0RcnkvypOWHpPnAt+m3bPeXvn2qBZXUWitOq 1lNh0BoMd8WhOpDfqJPw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTDO8-003ELG-1b; Wed, 31 Aug 2022 02:23:32 +0000 Received: from mail-lf1-x135.google.com ([2a00:1450:4864:20::135]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oTDO4-003EKJ-FA for linux-arm-kernel@lists.infradead.org; Wed, 31 Aug 2022 02:23:30 +0000 Received: by mail-lf1-x135.google.com with SMTP id g7so1072512lfe.11 for ; Tue, 30 Aug 2022 19:23:26 -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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=eyXn8XNL5R+c7skqJd2CXAmkkmv+PO53H99rqyOH87xBrXhsqaRc9Cafy09+sKEMJA iijHh/Vgr/N8ms8DrRs7ZsyH/kfkPXSLcSAHRoh1+w4hJSgtLHkhgFqawSk+ZXu7vLVu etWsQ2c8F5mlpD6pXL4Jnwu7reEvEQPvt2Kcq7iIfhsfn4Yu0eFb5/ZnRE0d433DYCkK cdghwZHl1psSvBmHBmPiKQam64a3GgKwsjlObk+WtdqaHxXkPRZOKXlEsDxfpMbOFJ+F qvawNpaI/5HTMI7FX7pKKny6H0CoBufGQKBaV+h3B0J6ooncWt1iyUW0Dhc0OQr52xk/ Icrg== 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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=6PCqWp6iUi8UtTXV6DqlEEvoUGsnCrA7BBVtFJ1FPvS6mNERWgQ2YwRzPH+nProJE+ axf6zEci7R9L+Gbv+Zg29PuTCk5q5kbqe7oygbkRZWQBGSj1IVmBN8GVukUT0/pfkMgJ GnDk8J2SGcLRfMqUgWR+E/43WTNlzsUBCxWjj7Wlc356WUhIIdYRjEBCcNqslNHqbYBx qImwYUsihXiQkKU61IPE9IDgDVTSP/oR3bw4oaD6ZVO6nl5OrdAaQc73PrTr68oG2ThG CPbSbP57DkxAtgx1UToMybkIRezAFeH8ZiuLH0TEZySzRIMFz2N0/t1YgD/2B7FM0JKz RIyA== X-Gm-Message-State: ACgBeo3tnEhIo7xXtrTN0n0YHeC1hNIW3ziTdlSQHQ2gzwU0bYeTSyCH mfhUnsZlxEomgH+sWX+/pJA= X-Google-Smtp-Source: AA6agR5RnfuFyIONMTqsPbIII5CYS1rbgB3cSxzqkqMVekfKEnddXN6p9IGLZ6Lzgilorerw0vcwmQ== X-Received: by 2002:a05:6512:1054:b0:494:8ce3:24c8 with SMTP id c20-20020a056512105400b004948ce324c8mr235583lfb.360.1661912604645; Tue, 30 Aug 2022 19:23:24 -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 k9-20020a2eb749000000b00261eb75fa5dsm1960508ljo.41.2022.08.30.19.23.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 19:23:24 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: Date: Wed, 31 Aug 2022 04:23:21 +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 32/41] drm/vc4: vec: Convert to the new TV mode property 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-32-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-32-459522d653a7@cerno.tech> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220830_192328_547744_3CE334A9 X-CRM114-Status: GOOD ( 26.84 ) 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 SGkgTWF4aW1lLAoKSSB0ZXN0ZWQgeW91ciBwYXRjaHNldCBvbiBteSBQaSBhbmQgaXQgbW9zdGx5 IHdvcmtzLiBHb29kIHdvcmshIEhvd2V2ZXIsCkkgbm90aWNlZCBhIGNvdXBsZSBvZiBpc3N1ZXMu Cgo+IC1zdGF0aWMgaW50IHZjNF92ZWNfZW5jb2Rlcl9hdG9taWNfY2hlY2soc3RydWN0IGRybV9l bmNvZGVyICplbmNvZGVyLAo+IC3CoMKgIMKgwqDCoCDCoMKgwqAgwqDCoMKgIMKgwqDCoCDCoHN0 cnVjdCBkcm1fY3J0Y19zdGF0ZSAqY3J0Y19zdGF0ZSwKPiAtwqDCoCDCoMKgwqAgwqDCoMKgIMKg wqDCoCDCoMKgwqAgwqBzdHJ1Y3QgZHJtX2Nvbm5lY3Rvcl9zdGF0ZSAqY29ubl9zdGF0ZSkKPiAt ewo+IC3CoMKgIMKgY29uc3Qgc3RydWN0IHZjNF92ZWNfdHZfbW9kZSAqdmVjX21vZGU7Cj4gLQo+ IC3CoMKgIMKgdmVjX21vZGUgPSAmdmM0X3ZlY190dl9tb2Rlc1tjb25uX3N0YXRlLT50di5tb2Rl XTsKPiAtCj4gLcKgwqAgwqBpZiAoY29ubl9zdGF0ZS0+Y3J0YyAmJgo+IC3CoMKgIMKgwqDCoMKg ICFkcm1fbW9kZV9lcXVhbCh2ZWNfbW9kZS0+bW9kZSwgJmNydGNfc3RhdGUtPmFkanVzdGVkX21v ZGUpKQo+IC3CoMKgIMKgwqDCoCDCoHJldHVybiAtRUlOVkFMOwo+IC0KPiAtwqDCoCDCoHJldHVy biAwOwo+IC19CgpJIG1heSBoYXZlIHNhaWQgaXQgbXlzZWxmIHRoYXQgd2Ugc2hvdWxkIGFsbG93 IGN1c3RvbSBtb2RlbGluZXMgd2l0aG91dCB0b28KbXVjaCB2YWxpZGF0aW9uLiBUaGUgVkM0IGFu ZCBWRUMsIGhvd2V2ZXIsIGhhdmUgc29tZSBjb25zaWRlcmFibGUgbGltaXRhdGlvbnMKd2hlbiBp dCBjb21lcyB0byB0aGUgbW9kZWxpbmVzIHRoYXQgdGhleSBjYW4gcmVsaWFibHkgb3V0cHV0LgoK SW4gcGFydGljdWxhciwgYXR0ZW1wdGluZyB0byB1c2UgIjUwIEh6IiB0aW1pbmdzIGluIE5UU0Mv UEFMLU0gbW9kZXMgKG9yCiI2MCBIeiIgaW4gUEFML1NFQ0FNIG1vZGVzKSByZXN1bHRzIGluIGEg d2VpcmRseSBza2V3ZWQgaW1hZ2UuIEhlcmUncyBob3cgaXQKbWF5IGxvb2sgbGlrZToKaHR0cHM6 Ly91c2VyLWltYWdlcy5naXRodWJ1c2VyY29udGVudC5jb20vNDQ5OTc2Mi8xODc1NzU5NDAtNzM2 ZTcyNjItYzgyZC00MmYzLWEyZDgtZjMwOWNiZDUxMTM5LnBuZwoKVGhpcyBpcyBiZWNhdXNlIGlm IHRoZSBDUlRDIGRvZXMgbm90IHRyaWdnZXIgdGhlIHN5bmMgcHVsc2VzIHdpdGhpbiBhbgphY2Nl cHRhYmxlIHRpbWUgd2luZG93LCB0aGUgVkVDIGFwcGFyZW50bHkgZ2VuZXJhdGVzIHRoZW0gaXRz ZWxmLiBUaGlzIGNhdXNlcwp0aGUgVkVDIHN5bmMgcHVsc2VzICh3aGljaCBnbyBvbnRvIHRoZSB3 aXJlKSBub3QgcXVpdGUgbGluZSB1cCB3aXRoIHRoZSBvbmVzCmZyb20gdGhlIG1vZGVsaW5lLCB3 aGljaCByZXN1bHRzIGluIHdoYXQgeW91IHNlZSBvbiB0aGUgc2NyZWVuc2hvdC4KCkkgb25jZSB3 cm90ZSBhIHZhbGlkYXRpb24gZnVuY3Rpb24gYmFzZWQgb24gZXh0ZW5zaXZlIHRlc3Rpbmcgb2Yg d2hhdApwcm9kdWNlcyBhIHNlbnNpYmxlIG91dHB1dCBhbmQgd2hhdCBkb2Vzbid0LiBZb3UgY2Fu IGZpbmQgaXQgaGVyZToKaHR0cHM6Ly9naXRodWIuY29tL3Jhc3BiZXJyeXBpL2xpbnV4L3B1bGwv NDQwNi9jb21taXRzLzE1YzBjNTEuIEkgdGhpbmsgaXQKbWlnaHQgYmUgYSBnb29kIGlkZWEgdG8g aW5jbHVkZSBzb21ldGhpbmcgbGlrZSB0aGF0IC0gZXZlbiB0aG91Z2ggSSBrbm93IGl0J3MKc29t ZXdoYXQgdWdseS4KCihCVFcsIHRob3NlICUyIGNoZWNrcyBvbiB2ZXJ0aWNhbCB0aW1pbmdzIGlu IHRoYXQgbGlua2VkIGNvbW1pdCBjYW4gYmUgaWdub3JlZDsKdGhvc2UgdmFsdWVzIGFyZSBkaXZp ZGVkIGJ5IDIgZm9yIGludGVybGFjZWQgbW9kZXMgYW55d2F5LiBUaG9zZSBjaGVja3Mgd2VyZQpp bnRlbmRlZCB0byBlbnN1cmUgcHJvcGVyIG9kZC1maXJzdCBvciBldmVuLWZpcnN0IHRpbWluZ3M7 IEknbSBub3Qgc3VyZSBpZiB5b3VyCmNvZGUgY2FsY3VsYXRlcyB0aG9zZSBpbiB0aGUgc2FtZSB3 YXkpCgo+wqAgc3RhdGljIGludCB2YzRfdmVjX2Nvbm5lY3Rvcl9nZXRfbW9kZXMoc3RydWN0IGRy bV9jb25uZWN0b3IgKmNvbm5lY3RvcikKPsKgIHsKPiAtwqDCoCDCoHN0cnVjdCBkcm1fY29ubmVj dG9yX3N0YXRlICpzdGF0ZSA9IGNvbm5lY3Rvci0+c3RhdGU7Cj4gwqDCoMKgIMKgc3RydWN0IGRy bV9kaXNwbGF5X21vZGUgKm1vZGU7Cj4gK8KgwqAgwqBpbnQgY291bnQgPSAwOwo+IMKgCj4gLcKg wqAgwqBtb2RlID0gZHJtX21vZGVfZHVwbGljYXRlKGNvbm5lY3Rvci0+ZGV2LAo+IC3CoMKgIMKg wqDCoCDCoMKgwqAgwqDCoMKgIMKgwqAgdmM0X3ZlY190dl9tb2Rlc1tzdGF0ZS0+dHYubW9kZV0u bW9kZSk7Cj4gK8KgwqAgwqBtb2RlID0gZHJtX21vZGVfYW5hbG9nX250c2NfNDgwaShjb25uZWN0 b3ItPmRldik7Cj4gwqDCoMKgIMKgaWYgKCFtb2RlKSB7Cj4gwqDCoMKgIMKgwqDCoCDCoERSTV9F UlJPUigiRmFpbGVkIHRvIGNyZWF0ZSBhIG5ldyBkaXNwbGF5IG1vZGVcbiIpOwo+IMKgwqDCoCDC oMKgwqAgwqByZXR1cm4gLUVOT01FTTsKPiDCoMKgwqAgwqB9Cj4gwqAKPiDCoMKgwqAgwqBkcm1f bW9kZV9wcm9iZWRfYWRkKGNvbm5lY3RvciwgbW9kZSk7Cj4gK8KgwqAgwqBjb3VudCArPSAxOwo+ IMKgCj4gLcKgwqAgwqByZXR1cm4gMTsKPiArwqDCoCDCoG1vZGUgPSBkcm1fbW9kZV9hbmFsb2df cGFsXzU3NmkoY29ubmVjdG9yLT5kZXYpOwo+ICvCoMKgIMKgaWYgKCFtb2RlKSB7Cj4gK8KgwqAg wqDCoMKgIMKgRFJNX0VSUk9SKCJGYWlsZWQgdG8gY3JlYXRlIGEgbmV3IGRpc3BsYXkgbW9kZVxu Iik7Cj4gK8KgwqAgwqDCoMKgIMKgcmV0dXJuIC1FTk9NRU07Cj4gK8KgwqAgwqB9Cj4gKwo+ICvC oMKgIMKgZHJtX21vZGVfcHJvYmVkX2FkZChjb25uZWN0b3IsIG1vZGUpOwo+ICvCoMKgIMKgY291 bnQgKz0gMTsKPiArCj4gK8KgwqAgwqByZXR1cm4gY291bnQ7Cj4gK30KClhvcmcgaXMgcHJldHR5 IGNvbmZ1c2VkIGJ5IHRoZXNlIG1vZGVzIGJlaW5nIHJlcG9ydGVkIGxpa2UgdGhhdC4gVGhlIDU3 NmkgbW9kZQppcyAqYWx3YXlzKiBwcmVmZXJyZWQsIHByZXN1bWFibHkgYmVjYXVzZSBvZiB0aGUg aGlnaGVyIHJlc29sdXRpb24uIElmIHRoZSBOVFNDCm1vZGUgaXMgc2V0ICh2aWEgdGhlIGtlcm5l bCBjbWRsaW5lIG9yIGp1c3QgZHVlIHRvIGl0IGJlaW5nIHRoZSBkZWZhdWx0KSwgdGhpcwpyZXN1 bHRzIGluIGEgbWVzcyBvbiB0aGUgc2NyZWVuIC0gZXhhY3RseSB0aGUgc2FtZSB0aGluZyBhcyBv biB0aGUgc2NyZWVuc2hvdApsaW5rZWQgYWJvdmUuCgpOb3RlIHRoYXQgZHJtX2hlbHBlcl9wcm9i ZV9hZGRfY21kbGluZV9tb2RlKCkgKmRvZXMqIGFkZCB0aGUKRFJNX01PREVfVFlQRV9VU0VSREVG IGZsYWcgdG8gdGhlIDQ4MGkgbW9kZSwgaGF2aW5nIGRldGVjdGVkIGl0IGFzIHByZWZlcnJlZApv biB0aGUgY29tbWFuZCBsaW5lIC0gYnV0IFhvcmcgZG9lcyBub3Qgc2VlbSB0byBjYXJlIGFib3V0 IHRoYXQuCgpJIHJlbWVtYmVyIE5vcmFsZiBzdWdnZXN0aW5nIHNldHRpbmcgRFJNX01PREVfVFlQ RV9QUkVGRVJSRUQgZm9yIHRoZSBtb2RlIHRoYXQKY29ycmVzcG9uZHMgdG8gdGhlIGN1cnJlbnRs eSBjaG9zZW4gdHZfbW9kZSAtIHRoYXQgd291bGQgZml4IHRoZSBwcm9ibGVtLgpBbiBhbHRlcm5h dGl2ZSB3b3VsZCBiZSB0byBfbm90XyBhZGQgdGhlICJvcHBvc2l0ZSIgbW9kZSBhdCBhbGwsIGxp a2UgdGhlCmN1cnJlbnQgZGVmYXVsdCBSYXNwYmVycnkgUGkgT1Mga2VybmVsIGJlaGF2ZXMuCgpO b3RlIHRoYXQgaWYgeW91IGRlY2lkZSB0byBhZGQgdGhlIG1vZGVsaW5lIHZhbGlkYXRpb24gbGlr ZSBJIHN1Z2dlc3RlZCBpbiB0aGUKY29tbWVudCBhYm92ZSwgdGhlbiB3aXRob3V0IHNldHRpbmcg dGhlIHByZWZlcnJlZCBtb2RlIHByb3Blcmx5LCBYb3JnIHdpbGwganVzdApnaXZlIHVwIGFuZCBz aXQgb24gYSBibGFuayBzY3JlZW4gdW50aWwgeW91IHJ1biB4cmFuZHIgZnJvbSBhbm90aGVyIHRl cm1pbmFsCmlmIHR2X21vZGUgaW5jb21wYXRpYmxlIHdpdGggNTc2aSBpcyBzZWxlY3RlZC4KCkJl c3QgcmVnYXJkcywKTWF0ZXVzeiBLd2lhdGtvd3NraQoKX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KbGludXgtYXJtLWtlcm5lbCBtYWlsaW5nIGxpc3QKbGlu dXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnCmh0dHA6Ly9saXN0cy5pbmZyYWRlYWQu b3JnL21haWxtYW4vbGlzdGluZm8vbGludXgtYXJtLWtlcm5lbAo= 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 88A43ECAAD5 for ; Wed, 31 Aug 2022 02:23:32 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0910910E15B; Wed, 31 Aug 2022 02:23:30 +0000 (UTC) Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 758A010E15B; Wed, 31 Aug 2022 02:23:26 +0000 (UTC) Received: by mail-lf1-x12b.google.com with SMTP id g7so1072507lfe.11; Tue, 30 Aug 2022 19:23:26 -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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=eyXn8XNL5R+c7skqJd2CXAmkkmv+PO53H99rqyOH87xBrXhsqaRc9Cafy09+sKEMJA iijHh/Vgr/N8ms8DrRs7ZsyH/kfkPXSLcSAHRoh1+w4hJSgtLHkhgFqawSk+ZXu7vLVu etWsQ2c8F5mlpD6pXL4Jnwu7reEvEQPvt2Kcq7iIfhsfn4Yu0eFb5/ZnRE0d433DYCkK cdghwZHl1psSvBmHBmPiKQam64a3GgKwsjlObk+WtdqaHxXkPRZOKXlEsDxfpMbOFJ+F qvawNpaI/5HTMI7FX7pKKny6H0CoBufGQKBaV+h3B0J6ooncWt1iyUW0Dhc0OQr52xk/ Icrg== 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=D594wEp0LEmyYOAx7Tirjq3SGwda2pe5svLBID/NRVI=; b=ONsodCQJDYGLAzmir2o5yxXoNw+Csqxt2vulJxJk3aNqmZmDXH4sxAuK7ITh3DddZj GOURpw3Sz7//7Ipg25hgxH6qcI3u2Jc2zqgch9rG0lC1QmYXO/tyT7x8UzEK0BGDeg7m 7N0ZiLVFqF3Rk9vMS/7h6OWQgPcRRva4Sxq1zik1DTc/PXB+9Bj2xl94hFWJJIhg7gbl Re7JKKPdZbWi/RGNHZaMCYYj8qHAux5sxjfbfA16PBeTeZq3KSYp1NIpxGuj0IbSo5Mk mgvOu7XcYte4oeUkvC0f8cCYxPF4SFlQ0pTUOZfJogw8xnBJIhX2/XvqVNI9fy/tweOi RGmQ== X-Gm-Message-State: ACgBeo2eAjWedWz0Cs9xKe4AwX4VNlZyuqZH+ylp+UnB6GbcS7BxMNdO J4N+DgJiXWg/nLdOqAA5Xe8= X-Google-Smtp-Source: AA6agR5RnfuFyIONMTqsPbIII5CYS1rbgB3cSxzqkqMVekfKEnddXN6p9IGLZ6Lzgilorerw0vcwmQ== X-Received: by 2002:a05:6512:1054:b0:494:8ce3:24c8 with SMTP id c20-20020a056512105400b004948ce324c8mr235583lfb.360.1661912604645; Tue, 30 Aug 2022 19:23:24 -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 k9-20020a2eb749000000b00261eb75fa5dsm1960508ljo.41.2022.08.30.19.23.22 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 30 Aug 2022 19:23:24 -0700 (PDT) From: Mateusz Kwiatkowski X-Google-Original-From: Mateusz Kwiatkowski Message-ID: Date: Wed, 31 Aug 2022 04:23:21 +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 32/41] drm/vc4: vec: Convert to the new TV mode property 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-32-459522d653a7@cerno.tech> In-Reply-To: <20220728-rpi-analog-tv-properties-v2-32-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, I tested your patchset on my Pi and it mostly works. Good work! However, I noticed a couple of issues. > -static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder, > -                    struct drm_crtc_state *crtc_state, > -                    struct drm_connector_state *conn_state) > -{ > -    const struct vc4_vec_tv_mode *vec_mode; > - > -    vec_mode = &vc4_vec_tv_modes[conn_state->tv.mode]; > - > -    if (conn_state->crtc && > -        !drm_mode_equal(vec_mode->mode, &crtc_state->adjusted_mode)) > -        return -EINVAL; > - > -    return 0; > -} I may have said it myself that we should allow custom modelines without too much validation. The VC4 and VEC, however, have some considerable limitations when it comes to the modelines that they can reliably output. In particular, attempting to use "50 Hz" timings in NTSC/PAL-M modes (or "60 Hz" in PAL/SECAM modes) results in a weirdly skewed image. Here's how it may look like: https://user-images.githubusercontent.com/4499762/187575940-736e7262-c82d-42f3-a2d8-f309cbd51139.png This is because if the CRTC does not trigger the sync pulses within an acceptable time window, the VEC apparently generates them itself. This causes the VEC sync pulses (which go onto the wire) not quite line up with the ones from the modeline, which results in what you see on the screenshot. I once wrote a validation function based on extensive testing of what produces a sensible output and what doesn't. You can find it here: https://github.com/raspberrypi/linux/pull/4406/commits/15c0c51. I think it might be a good idea to include something like that - even though I know it's somewhat ugly. (BTW, those %2 checks on vertical timings in that linked commit can be ignored; those values are divided by 2 for interlaced modes anyway. Those checks were intended to ensure proper odd-first or even-first timings; I'm not sure if your code calculates those in the same way) >  static int vc4_vec_connector_get_modes(struct drm_connector *connector) >  { > -    struct drm_connector_state *state = connector->state; >      struct drm_display_mode *mode; > +    int count = 0; >   > -    mode = drm_mode_duplicate(connector->dev, > -                  vc4_vec_tv_modes[state->tv.mode].mode); > +    mode = drm_mode_analog_ntsc_480i(connector->dev); >      if (!mode) { >          DRM_ERROR("Failed to create a new display mode\n"); >          return -ENOMEM; >      } >   >      drm_mode_probed_add(connector, mode); > +    count += 1; >   > -    return 1; > +    mode = drm_mode_analog_pal_576i(connector->dev); > +    if (!mode) { > +        DRM_ERROR("Failed to create a new display mode\n"); > +        return -ENOMEM; > +    } > + > +    drm_mode_probed_add(connector, mode); > +    count += 1; > + > +    return count; > +} Xorg is pretty confused by these modes being reported like that. The 576i mode is *always* preferred, presumably because of the higher resolution. If the NTSC mode is set (via the kernel cmdline or just due to it being the default), this results in a mess on the screen - exactly the same thing as on the screenshot linked above. Note that drm_helper_probe_add_cmdline_mode() *does* add the DRM_MODE_TYPE_USERDEF flag to the 480i mode, having detected it as preferred on the command line - but Xorg does not seem to care about that. I remember Noralf suggesting setting DRM_MODE_TYPE_PREFERRED for the mode that corresponds to the currently chosen tv_mode - that would fix the problem. An alternative would be to _not_ add the "opposite" mode at all, like the current default Raspberry Pi OS kernel behaves. Note that if you decide to add the modeline validation like I suggested in the comment above, then without setting the preferred mode properly, Xorg will just give up and sit on a blank screen until you run xrandr from another terminal if tv_mode incompatible with 576i is selected. Best regards, Mateusz Kwiatkowski