Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
625-lines modes in their drivers.
Since those modes are fairly standard, and that we'll need to use them
in more places in the future, it makes sense to move their definition
into the core framework.
However, analog display usually have fairly loose timings requirements,
the only discrete parameters being the total number of lines and pixel
clock frequency. Thus, we created a function that will create a display
mode from the standard, the pixel frequency and the active area.
Signed-off-by: Maxime Ripard <[email protected]>
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 304004fb80aa..ee581ee17171 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -116,6 +116,459 @@ void drm_mode_probed_add(struct drm_connector *connector,
}
EXPORT_SYMBOL(drm_mode_probed_add);
+enum drm_mode_analog {
+ DRM_MODE_ANALOG_NTSC,
+ DRM_MODE_ANALOG_PAL,
+};
+
+/*
+ * The timings come from:
+ * - https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
+ * - https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
+ * - https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
+ */
+#define NTSC_LINE_DURATION_NS 63556U
+#define NTSC_LINES_NUMBER 525
+
+#define NTSC_HBLK_DURATION_TYP_NS 10900U
+#define NTSC_HBLK_DURATION_MIN_NS (NTSC_HBLK_DURATION_TYP_NS - 200)
+#define NTSC_HBLK_DURATION_MAX_NS (NTSC_HBLK_DURATION_TYP_NS + 200)
+
+#define NTSC_HACT_DURATION_TYP_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_TYP_NS)
+#define NTSC_HACT_DURATION_MIN_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MAX_NS)
+#define NTSC_HACT_DURATION_MAX_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MIN_NS)
+
+#define NTSC_HFP_DURATION_TYP_NS 1500
+#define NTSC_HFP_DURATION_MIN_NS 1270
+#define NTSC_HFP_DURATION_MAX_NS 2220
+
+#define NTSC_HSLEN_DURATION_TYP_NS 4700
+#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
+#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
+
+#define NTSC_HBP_DURATION_TYP_NS 4700
+
+/*
+ * I couldn't find the actual tolerance for the back porch, so let's
+ * just reuse the sync length ones.
+ */
+#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
+#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
+
+#define PAL_LINE_DURATION_NS 64000U
+#define PAL_LINES_NUMBER 625
+
+#define PAL_HACT_DURATION_TYP_NS 51950U
+#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
+#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
+
+#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_TYP_NS)
+#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MAX_NS)
+#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MIN_NS)
+
+#define PAL_HFP_DURATION_TYP_NS 1650
+#define PAL_HFP_DURATION_MIN_NS (PAL_HFP_DURATION_TYP_NS - 100)
+#define PAL_HFP_DURATION_MAX_NS (PAL_HFP_DURATION_TYP_NS + 400)
+
+#define PAL_HSLEN_DURATION_TYP_NS 4700
+#define PAL_HSLEN_DURATION_MIN_NS (PAL_HSLEN_DURATION_TYP_NS - 200)
+#define PAL_HSLEN_DURATION_MAX_NS (PAL_HSLEN_DURATION_TYP_NS + 200)
+
+#define PAL_HBP_DURATION_TYP_NS 5700
+#define PAL_HBP_DURATION_MIN_NS (PAL_HBP_DURATION_TYP_NS - 200)
+#define PAL_HBP_DURATION_MAX_NS (PAL_HBP_DURATION_TYP_NS + 200)
+
+#define PAL_VFP_INTERLACE_LINES 5
+#define PAL_VSLEN_INTERLACE_LINES 5
+
+#define PAL_SHORT_SYNC_DURATION_NS ((2 + 30) * NSEC_PER_USEC)
+#define PAL_LONG_SYNC_DURATION_NS ((30 + 2) * NSEC_PER_USEC)
+
+struct analog_param_field {
+ unsigned int even, odd;
+};
+
+#define PARAM_FIELD(_odd, _even) \
+ { .even = _even, .odd = _odd }
+
+struct analog_param_range {
+ unsigned int min, typ, max;
+};
+
+#define PARAM_RANGE(_min, _typ, _max) \
+ { .min = _min, .typ = _typ, .max = _max }
+
+struct analog_parameters {
+ unsigned int num_lines;
+ unsigned int line_duration_ns;
+
+ struct analog_param_range hact_ns;
+ struct analog_param_range hfp_ns;
+ struct analog_param_range hslen_ns;
+ struct analog_param_range hbp_ns;
+ struct analog_param_range hblk_ns;
+
+ struct analog_param_field vfp_lines;
+ struct analog_param_field vslen_lines;
+ struct analog_param_field vbp_lines;
+};
+
+#define TV_MODE_PARAMETER(_mode, _lines, _line_dur, _hact, _hfp, _hslen, _hbp, _hblk, _vfp, _vslen, _vbp) \
+ [_mode] = { \
+ .num_lines = _lines, \
+ .line_duration_ns = _line_dur, \
+ .hact_ns = _hact, \
+ .hfp_ns = _hfp, \
+ .hslen_ns = _hslen, \
+ .hbp_ns = _hbp, \
+ .hblk_ns = _hblk, \
+ .vfp_lines = _vfp, \
+ .vslen_lines = _vslen, \
+ .vbp_lines = _vbp, \
+ }
+
+const static struct analog_parameters tv_modes_parameters[] = {
+ TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
+ NTSC_LINES_NUMBER,
+ NTSC_LINE_DURATION_NS,
+ PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
+ NTSC_HACT_DURATION_TYP_NS,
+ NTSC_HACT_DURATION_MAX_NS),
+ PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
+ NTSC_HFP_DURATION_TYP_NS,
+ NTSC_HFP_DURATION_MAX_NS),
+ PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
+ NTSC_HSLEN_DURATION_TYP_NS,
+ NTSC_HSLEN_DURATION_MAX_NS),
+ PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
+ NTSC_HBP_DURATION_TYP_NS,
+ NTSC_HBP_DURATION_MAX_NS),
+ PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
+ NTSC_HBLK_DURATION_TYP_NS,
+ NTSC_HBLK_DURATION_MAX_NS),
+ PARAM_FIELD(3, 3),
+ PARAM_FIELD(3, 3),
+ PARAM_FIELD(3, 3)),
+ TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
+ PAL_LINES_NUMBER,
+ PAL_LINE_DURATION_NS,
+ PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
+ PAL_HACT_DURATION_TYP_NS,
+ PAL_HACT_DURATION_MAX_NS),
+ PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
+ PAL_HFP_DURATION_TYP_NS,
+ PAL_HFP_DURATION_MAX_NS),
+ PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
+ PAL_HSLEN_DURATION_TYP_NS,
+ PAL_HSLEN_DURATION_MAX_NS),
+ PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
+ PAL_HBP_DURATION_TYP_NS,
+ PAL_HBP_DURATION_MAX_NS),
+ PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
+ PAL_HBLK_DURATION_TYP_NS,
+ PAL_HBLK_DURATION_MAX_NS),
+
+ /*
+ * The front porch is actually 6 short sync
+ * pulses for the even field, and 5 for the
+ * odd field. Each sync takes half a life so
+ * the odd field front porch is shorter by
+ * half a line.
+ *
+ * In progressive, we're supposed to use 6
+ * pulses, so we're fine there
+ */
+ PARAM_FIELD(3, 2),
+
+ /*
+ * The vsync length is 5 long sync pulses,
+ * each field taking half a line. We're
+ * shorter for both fields by half a line.
+ *
+ * In progressive, we're supposed to use 5
+ * pulses, so we're off by half
+ * a line.
+ *
+ * In interlace, we're now off by half a line
+ * for the even field and one line for the odd
+ * field.
+ */
+ PARAM_FIELD(3, 3),
+
+ /*
+ * The back porch is actually 5 short sync
+ * pulses for the even field, 4 for the odd
+ * field. In progressive, it's 5 short syncs.
+ *
+ * In progressive, we thus have 2.5 lines,
+ * plus the 0.5 line we were missing
+ * previously, so we should use 3 lines.
+ *
+ * In interlace, the even field is in the
+ * exact same case than progressive. For the
+ * odd field, we should be using 2 lines but
+ * we're one line short, so we'll make up for
+ * it here by using 3.
+ */
+ PARAM_FIELD(3, 3)),
+};
+
+static int fill_analog_mode(struct drm_display_mode *mode,
+ const struct analog_parameters *params,
+ unsigned long pixel_clock_hz,
+ unsigned int hactive,
+ unsigned int vactive,
+ bool interlace)
+{
+ unsigned long pixel_duration_ns = NSEC_PER_SEC / pixel_clock_hz;
+ unsigned long long htotal;
+ unsigned int vtotal;
+ unsigned int max_hact, hact_duration_ns;
+ unsigned int hblk, hblk_duration_ns;
+ unsigned int hfp, hfp_min, hfp_duration_ns;
+ unsigned int hslen, hslen_duration_ns;
+ unsigned int hbp, hbp_min, hbp_duration_ns;
+ unsigned int porches, porches_duration_ns;
+ unsigned int vfp, vfp_min;
+ unsigned int vbp, vbp_min;
+ unsigned int vslen;
+ int porches_rem;
+ bool strict = true;
+
+ max_hact = params->hact_ns.max / pixel_duration_ns;
+ if (pixel_clock_hz == 13500000 && hactive > max_hact && hactive <= 720)
+ strict = false;
+
+ /*
+ * Our pixel duration is going to be round down by the division,
+ * so rounding up is probably going to introduce even more
+ * deviation.
+ */
+ htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC;
+
+ hact_duration_ns = hactive * pixel_duration_ns;
+ if (strict &&
+ (hact_duration_ns < params->hact_ns.min ||
+ hact_duration_ns > params->hact_ns.max)) {
+ DRM_ERROR("Invalid horizontal active area duration: %uns (min: %u, max %u)\n",
+ hact_duration_ns, params->hact_ns.min, params->hact_ns.max);
+ return -EINVAL;
+ }
+
+ hblk = htotal - hactive;
+ hblk_duration_ns = hblk * pixel_duration_ns;
+ if (strict &&
+ (hblk_duration_ns < params->hblk_ns.min ||
+ hblk_duration_ns > params->hblk_ns.max)) {
+ DRM_ERROR("Invalid horizontal blanking duration: %uns (min: %u, max %u)\n",
+ hblk_duration_ns, params->hblk_ns.min, params->hblk_ns.max);
+ return -EINVAL;
+ }
+
+ hslen = DIV_ROUND_UP(params->hslen_ns.typ, pixel_duration_ns);
+ hslen_duration_ns = hslen * pixel_duration_ns;
+ if (strict &&
+ (hslen_duration_ns < params->hslen_ns.min ||
+ hslen_duration_ns > params->hslen_ns.max)) {
+ DRM_ERROR("Invalid horizontal sync duration: %uns (min: %u, max %u)\n",
+ hslen_duration_ns, params->hslen_ns.min, params->hslen_ns.max);
+ return -EINVAL;
+ }
+
+ porches = hblk - hslen;
+ porches_duration_ns = porches * pixel_duration_ns;
+ if (strict &&
+ (porches_duration_ns > (params->hfp_ns.max + params->hbp_ns.max) ||
+ porches_duration_ns < (params->hfp_ns.min + params->hbp_ns.min))) {
+ DRM_ERROR("Invalid horizontal porches duration: %uns\n", porches_duration_ns);
+ return -EINVAL;
+ }
+
+ hfp_min = DIV_ROUND_UP(params->hfp_ns.min, pixel_duration_ns);
+ hbp_min = DIV_ROUND_UP(params->hbp_ns.min, pixel_duration_ns);
+ porches_rem = porches - hfp_min - hbp_min;
+
+ hfp = hfp_min + DIV_ROUND_UP(porches_rem, 2);
+ hfp_duration_ns = hfp * pixel_duration_ns;
+ if (strict &&
+ (hfp_duration_ns < params->hfp_ns.min ||
+ hfp_duration_ns > params->hfp_ns.max)) {
+ DRM_ERROR("Invalid horizontal front porch duration: %uns (min: %u, max %u)\n",
+ hfp_duration_ns, params->hfp_ns.min, params->hfp_ns.max);
+ return -EINVAL;
+ }
+
+ hbp = porches - hfp;
+ hbp_duration_ns = hbp * pixel_duration_ns;
+ if (strict &&
+ (hbp_duration_ns < params->hbp_ns.min ||
+ hbp_duration_ns > params->hbp_ns.max)) {
+ DRM_ERROR("Invalid horizontal back porch duration: %uns (min: %u, max %u)\n",
+ hbp_duration_ns, params->hbp_ns.min, params->hbp_ns.max);
+ return -EINVAL;
+ }
+
+ if (htotal != (hactive + hfp + hslen + hbp))
+ return -EINVAL;
+
+ mode->clock = pixel_clock_hz / 1000;
+ mode->hdisplay = hactive;
+ mode->hsync_start = hactive + hfp;
+ mode->hsync_end = hactive + hfp + hslen;
+ mode->htotal = hactive + hfp + hslen + hbp;
+
+ if (interlace) {
+ vfp_min = params->vfp_lines.even + params->vfp_lines.odd;
+ vbp_min = params->vbp_lines.even + params->vbp_lines.odd;
+ vslen = params->vslen_lines.even + params->vslen_lines.odd;
+ } else {
+ /*
+ * By convention, NSTC (aka 525/60) systems start with
+ * the even field, but PAL (aka 625/50) systems start
+ * with the odd one.
+ *
+ * PAL systems also have asymetric timings between the
+ * even and odd field, while NTSC is symetric.
+ *
+ * Moreover, if we want to create a progressive mode for
+ * PAL, we need to use the odd field timings.
+ *
+ * Since odd == even for NTSC, we can just use the odd
+ * one all the time to simplify the code a bit.
+ */
+ vfp_min = params->vfp_lines.odd;
+ vbp_min = params->vbp_lines.odd;
+ vslen = params->vslen_lines.odd;
+ }
+
+ porches = params->num_lines - vactive - vslen;
+ porches_rem = porches - vfp_min - vbp_min;
+
+ vfp = vfp_min + (porches_rem / 2);
+ vbp = porches - vfp;
+
+ vtotal = vactive + vfp + vslen + vbp;
+ if (params->num_lines != vtotal) {
+ DRM_ERROR("Invalid vertical total: %upx (expected %upx)\n",
+ vtotal, params->num_lines);
+ return -EINVAL;
+ }
+
+ mode->vdisplay = vactive;
+ mode->vsync_start = vactive + vfp;
+ mode->vsync_end = vactive + vfp + vslen;
+ mode->vtotal = vactive + vfp + vslen + vbp;
+
+ mode->type = DRM_MODE_TYPE_DRIVER;
+ mode->flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC;
+ if (interlace)
+ mode->flags |= DRM_MODE_FLAG_INTERLACE;
+
+ drm_mode_set_name(mode);
+
+ if (mode->vtotal != params->num_lines)
+ return -EINVAL;
+
+ return 0;
+}
+
+/**
+ * drm_analog_tv_mode - create a display mode for an analog TV
+ * @dev: drm device
+ * @tv_mode: TV Mode standard to create a mode for. See DRM_MODE_TV_MODE_*.
+ * @pixel_clock_hz: Pixel Clock Frequency, in Hertz
+ * @hdisplay: hdisplay size
+ * @vdisplay: vdisplay size
+ * @interlace: whether to compute an interlaced mode
+ *
+ * This function creates a struct drm_display_mode instance suited for
+ * an analog TV output, for one of the usual analog TV mode.
+ *
+ * Note that @hdisplay is larger than the usual constraints for the PAL
+ * and NTSC timings, and we'll choose to ignore most timings constraints
+ * to reach those resolutions.
+ *
+ * Returns:
+ *
+ * A pointer to the mode, allocated with drm_mode_create(). Returns NULL
+ * on error.
+ */
+struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
+ enum drm_connector_tv_mode tv_mode,
+ unsigned long pixel_clock_hz,
+ unsigned int hdisplay,
+ unsigned int vdisplay,
+ bool interlace)
+{
+ struct drm_display_mode *mode;
+ enum drm_mode_analog analog;
+ int ret;
+
+ switch (tv_mode) {
+ case DRM_MODE_TV_MODE_NTSC_443:
+ fallthrough;
+ case DRM_MODE_TV_MODE_NTSC_J:
+ fallthrough;
+ case DRM_MODE_TV_MODE_NTSC_M:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_60:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_M:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_60:
+ analog = DRM_MODE_ANALOG_NTSC;
+ break;
+
+ case DRM_MODE_TV_MODE_PAL_B:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_D:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_G:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_H:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_I:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_N:
+ fallthrough;
+ case DRM_MODE_TV_MODE_PAL_NC:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_B:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_D:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_G:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_K:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_K1:
+ fallthrough;
+ case DRM_MODE_TV_MODE_SECAM_L:
+ analog = DRM_MODE_ANALOG_PAL;
+ break;
+
+ default:
+ return NULL;
+ }
+
+ mode = drm_mode_create(dev);
+ if (!mode)
+ return NULL;
+
+ ret = fill_analog_mode(mode,
+ &tv_modes_parameters[analog],
+ pixel_clock_hz, hdisplay, vdisplay, interlace);
+ if (ret)
+ goto err_free_mode;
+
+ return mode;
+
+err_free_mode:
+ drm_mode_destroy(dev, mode);
+ return NULL;
+}
+EXPORT_SYMBOL(drm_analog_tv_mode);
+
/**
* drm_cvt_mode -create a modeline based on the CVT algorithm
* @dev: drm device
diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
index b29ef1085cad..b22ac96fdd65 100644
--- a/drivers/gpu/drm/tests/Makefile
+++ b/drivers/gpu/drm/tests/Makefile
@@ -10,5 +10,6 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
drm_framebuffer_test.o \
drm_kunit_helpers.o \
drm_mm_test.o \
+ drm_modes_test.o \
drm_plane_helper_test.o \
drm_rect_test.o
diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
new file mode 100644
index 000000000000..87d398fcb99e
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_modes_test.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kunit test for drm_modes functions
+ */
+
+#include <kunit/test.h>
+
+#include <drm/drm_modes.h>
+
+#include "drm_kunit_helpers.h"
+
+struct drm_modes_test_priv {
+ struct drm_device *drm;
+};
+
+static int drm_modes_test_init(struct kunit *test)
+{
+ struct drm_modes_test_priv *priv;
+
+ priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ test->priv = priv;
+
+ priv->drm = drm_kunit_device_init("drm-modes-test");
+ if (IS_ERR(priv->drm))
+ return PTR_ERR(priv->drm);
+
+ return 0;
+}
+
+static void drm_modes_test_exit(struct kunit *test)
+{
+ struct drm_modes_test_priv *priv = test->priv;
+
+ drm_kunit_device_exit(priv->drm);
+}
+
+static void drm_modes_analog_tv_ntsc_480i(struct kunit *test)
+{
+ struct drm_modes_test_priv *priv = test->priv;
+ struct drm_display_mode *mode;
+
+ mode = drm_analog_tv_mode(priv->drm,
+ DRM_MODE_TV_MODE_NTSC_M,
+ 13500 * 1000, 720, 480,
+ true);
+ KUNIT_ASSERT_NOT_NULL(test, mode);
+
+ KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 60);
+ KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
+
+ /* 63.556us * 13.5MHz = 858 pixels */
+ KUNIT_EXPECT_EQ(test, mode->htotal, 858);
+ KUNIT_EXPECT_EQ(test, mode->vdisplay, 480);
+ KUNIT_EXPECT_EQ(test, mode->vtotal, 525);
+}
+
+static void drm_modes_analog_tv_ntsc_480i_inlined(struct kunit *test)
+{
+ struct drm_modes_test_priv *priv = test->priv;
+ struct drm_display_mode *expected, *mode;
+
+ expected = drm_analog_tv_mode(priv->drm,
+ DRM_MODE_TV_MODE_NTSC_M,
+ 13500 * 1000, 720, 480,
+ true);
+ KUNIT_ASSERT_NOT_NULL(test, expected);
+
+ mode = drm_mode_analog_ntsc_480i(priv->drm);
+ KUNIT_ASSERT_NOT_NULL(test, mode);
+
+ KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
+}
+
+static void drm_modes_analog_tv_pal_576i(struct kunit *test)
+{
+ struct drm_modes_test_priv *priv = test->priv;
+ struct drm_display_mode *mode;
+
+ mode = drm_analog_tv_mode(priv->drm,
+ DRM_MODE_TV_MODE_PAL_B,
+ 13500 * 1000, 720, 576,
+ true);
+ KUNIT_ASSERT_NOT_NULL(test, mode);
+
+ KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 50);
+ KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
+
+ /* 64us * 13.5MHz = 864 pixels */
+ KUNIT_EXPECT_EQ(test, mode->htotal, 864);
+ KUNIT_EXPECT_EQ(test, mode->vdisplay, 576);
+ KUNIT_EXPECT_EQ(test, mode->vtotal, 625);
+}
+
+static void drm_modes_analog_tv_pal_576i_inlined(struct kunit *test)
+{
+ struct drm_modes_test_priv *priv = test->priv;
+ struct drm_display_mode *expected, *mode;
+
+ expected = drm_analog_tv_mode(priv->drm,
+ DRM_MODE_TV_MODE_PAL_B,
+ 13500 * 1000, 720, 576,
+ true);
+ KUNIT_ASSERT_NOT_NULL(test, expected);
+
+ mode = drm_mode_analog_pal_576i(priv->drm);
+ KUNIT_ASSERT_NOT_NULL(test, mode);
+
+ KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
+}
+
+static struct kunit_case drm_modes_analog_tv_tests[] = {
+ KUNIT_CASE(drm_modes_analog_tv_ntsc_480i),
+ KUNIT_CASE(drm_modes_analog_tv_ntsc_480i_inlined),
+ KUNIT_CASE(drm_modes_analog_tv_pal_576i),
+ KUNIT_CASE(drm_modes_analog_tv_pal_576i_inlined),
+ { }
+};
+
+static struct kunit_suite drm_modes_analog_tv_test_suite = {
+ .name = "drm_modes_analog_tv",
+ .init = drm_modes_test_init,
+ .exit = drm_modes_test_exit,
+ .test_cases = drm_modes_analog_tv_tests,
+};
+
+kunit_test_suites(
+ &drm_modes_analog_tv_test_suite
+);
+MODULE_LICENSE("GPL v2");
diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
index a80ae9639e96..5ccf3d51d313 100644
--- a/include/drm/drm_modes.h
+++ b/include/drm/drm_modes.h
@@ -443,6 +443,23 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
bool drm_mode_is_420(const struct drm_display_info *display,
const struct drm_display_mode *mode);
+struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
+ enum drm_connector_tv_mode mode,
+ unsigned long pixel_clock_hz,
+ unsigned int hdisplay,
+ unsigned int vdisplay,
+ bool interlace);
+
+static inline struct drm_display_mode *drm_mode_analog_ntsc_480i(struct drm_device *dev)
+{
+ return drm_analog_tv_mode(dev, DRM_MODE_TV_MODE_NTSC_M, 13500000, 720, 480, true);
+}
+
+static inline struct drm_display_mode *drm_mode_analog_pal_576i(struct drm_device *dev)
+{
+ return drm_analog_tv_mode(dev, DRM_MODE_TV_MODE_PAL_B, 13500000, 720, 576, true);
+}
+
struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
int hdisplay, int vdisplay, int vrefresh,
bool reduced, bool interlaced,
--
b4 0.10.0-dev-65ba7
Hi Maxime,
On 8/29/22 10:11, Maxime Ripard wrote:
> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
> 625-lines modes in their drivers.
>
> Since those modes are fairly standard, and that we'll need to use them
> in more places in the future, it makes sense to move their definition
> into the core framework.
>
> However, analog display usually have fairly loose timings requirements,
> the only discrete parameters being the total number of lines and pixel
> clock frequency. Thus, we created a function that will create a display
> mode from the standard, the pixel frequency and the active area.
>
> Signed-off-by: Maxime Ripard <[email protected]>
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 304004fb80aa..ee581ee17171 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -116,6 +116,459 @@ void drm_mode_probed_add(struct drm_connector *connector,
> }
> EXPORT_SYMBOL(drm_mode_probed_add);
>
> +enum drm_mode_analog {
> + DRM_MODE_ANALOG_NTSC,
> + DRM_MODE_ANALOG_PAL,
> +};
> +
> +/*
> + * The timings come from:
> + * - https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
> + * - https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
> + * - https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
> + */
> +#define NTSC_LINE_DURATION_NS 63556U
> +#define NTSC_LINES_NUMBER 525
> +
> +#define NTSC_HBLK_DURATION_TYP_NS 10900U
> +#define NTSC_HBLK_DURATION_MIN_NS (NTSC_HBLK_DURATION_TYP_NS - 200)
> +#define NTSC_HBLK_DURATION_MAX_NS (NTSC_HBLK_DURATION_TYP_NS + 200)
> +
> +#define NTSC_HACT_DURATION_TYP_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_TYP_NS)
> +#define NTSC_HACT_DURATION_MIN_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MAX_NS)
> +#define NTSC_HACT_DURATION_MAX_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MIN_NS)
> +
> +#define NTSC_HFP_DURATION_TYP_NS 1500
> +#define NTSC_HFP_DURATION_MIN_NS 1270
> +#define NTSC_HFP_DURATION_MAX_NS 2220
> +
> +#define NTSC_HSLEN_DURATION_TYP_NS 4700
> +#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
> +#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
> +
> +#define NTSC_HBP_DURATION_TYP_NS 4700
> +
> +/*
> + * I couldn't find the actual tolerance for the back porch, so let's
> + * just reuse the sync length ones.
> + */
> +#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
> +#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
> +
> +#define PAL_LINE_DURATION_NS 64000U
> +#define PAL_LINES_NUMBER 625
> +
> +#define PAL_HACT_DURATION_TYP_NS 51950U
> +#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
> +#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
> +
> +#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_TYP_NS)
> +#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MAX_NS)
> +#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MIN_NS)
> +
> +#define PAL_HFP_DURATION_TYP_NS 1650
> +#define PAL_HFP_DURATION_MIN_NS (PAL_HFP_DURATION_TYP_NS - 100)
> +#define PAL_HFP_DURATION_MAX_NS (PAL_HFP_DURATION_TYP_NS + 400)
> +
> +#define PAL_HSLEN_DURATION_TYP_NS 4700
> +#define PAL_HSLEN_DURATION_MIN_NS (PAL_HSLEN_DURATION_TYP_NS - 200)
> +#define PAL_HSLEN_DURATION_MAX_NS (PAL_HSLEN_DURATION_TYP_NS + 200)
> +
> +#define PAL_HBP_DURATION_TYP_NS 5700
> +#define PAL_HBP_DURATION_MIN_NS (PAL_HBP_DURATION_TYP_NS - 200)
> +#define PAL_HBP_DURATION_MAX_NS (PAL_HBP_DURATION_TYP_NS + 200)
> +
> +#define PAL_VFP_INTERLACE_LINES 5
> +#define PAL_VSLEN_INTERLACE_LINES 5
> +
> +#define PAL_SHORT_SYNC_DURATION_NS ((2 + 30) * NSEC_PER_USEC)
> +#define PAL_LONG_SYNC_DURATION_NS ((30 + 2) * NSEC_PER_USEC)
> +
> +struct analog_param_field {
> + unsigned int even, odd;
> +};
> +
> +#define PARAM_FIELD(_odd, _even) \
> + { .even = _even, .odd = _odd }
> +
> +struct analog_param_range {
> + unsigned int min, typ, max;
> +};
> +
> +#define PARAM_RANGE(_min, _typ, _max) \
> + { .min = _min, .typ = _typ, .max = _max }
> +
> +struct analog_parameters {
> + unsigned int num_lines;
> + unsigned int line_duration_ns;
> +
> + struct analog_param_range hact_ns;
> + struct analog_param_range hfp_ns;
> + struct analog_param_range hslen_ns;
> + struct analog_param_range hbp_ns;
> + struct analog_param_range hblk_ns;
> +
> + struct analog_param_field vfp_lines;
> + struct analog_param_field vslen_lines;
> + struct analog_param_field vbp_lines;
> +};
> +
> +#define TV_MODE_PARAMETER(_mode, _lines, _line_dur, _hact, _hfp, _hslen, _hbp, _hblk, _vfp, _vslen, _vbp) \
> + [_mode] = { \
> + .num_lines = _lines, \
> + .line_duration_ns = _line_dur, \
> + .hact_ns = _hact, \
> + .hfp_ns = _hfp, \
> + .hslen_ns = _hslen, \
> + .hbp_ns = _hbp, \
> + .hblk_ns = _hblk, \
> + .vfp_lines = _vfp, \
> + .vslen_lines = _vslen, \
> + .vbp_lines = _vbp, \
> + }
> +
> +const static struct analog_parameters tv_modes_parameters[] = {
> + TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
> + NTSC_LINES_NUMBER,
> + NTSC_LINE_DURATION_NS,
> + PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
> + NTSC_HACT_DURATION_TYP_NS,
> + NTSC_HACT_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
> + NTSC_HFP_DURATION_TYP_NS,
> + NTSC_HFP_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
> + NTSC_HSLEN_DURATION_TYP_NS,
> + NTSC_HSLEN_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
> + NTSC_HBP_DURATION_TYP_NS,
> + NTSC_HBP_DURATION_MAX_NS),
> + PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
> + NTSC_HBLK_DURATION_TYP_NS,
> + NTSC_HBLK_DURATION_MAX_NS),
> + PARAM_FIELD(3, 3),
> + PARAM_FIELD(3, 3),
> + PARAM_FIELD(3, 3)),
> + TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
> + PAL_LINES_NUMBER,
> + PAL_LINE_DURATION_NS,
> + PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
> + PAL_HACT_DURATION_TYP_NS,
> + PAL_HACT_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
> + PAL_HFP_DURATION_TYP_NS,
> + PAL_HFP_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
> + PAL_HSLEN_DURATION_TYP_NS,
> + PAL_HSLEN_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
> + PAL_HBP_DURATION_TYP_NS,
> + PAL_HBP_DURATION_MAX_NS),
> + PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
> + PAL_HBLK_DURATION_TYP_NS,
> + PAL_HBLK_DURATION_MAX_NS),
> +
> + /*
> + * The front porch is actually 6 short sync
> + * pulses for the even field, and 5 for the
> + * odd field. Each sync takes half a life so
> + * the odd field front porch is shorter by
> + * half a line.
> + *
> + * In progressive, we're supposed to use 6
> + * pulses, so we're fine there
> + */
> + PARAM_FIELD(3, 2),
> +
> + /*
> + * The vsync length is 5 long sync pulses,
> + * each field taking half a line. We're
> + * shorter for both fields by half a line.
> + *
> + * In progressive, we're supposed to use 5
> + * pulses, so we're off by half
> + * a line.
> + *
> + * In interlace, we're now off by half a line
> + * for the even field and one line for the odd
> + * field.
> + */
> + PARAM_FIELD(3, 3),
> +
> + /*
> + * The back porch is actually 5 short sync
> + * pulses for the even field, 4 for the odd
> + * field. In progressive, it's 5 short syncs.
> + *
> + * In progressive, we thus have 2.5 lines,
> + * plus the 0.5 line we were missing
> + * previously, so we should use 3 lines.
> + *
> + * In interlace, the even field is in the
> + * exact same case than progressive. For the
> + * odd field, we should be using 2 lines but
> + * we're one line short, so we'll make up for
> + * it here by using 3.
> + */
> + PARAM_FIELD(3, 3)),
> +};
> +
> +static int fill_analog_mode(struct drm_display_mode *mode,
> + const struct analog_parameters *params,
> + unsigned long pixel_clock_hz,
> + unsigned int hactive,
> + unsigned int vactive,
> + bool interlace)
> +{
> + unsigned long pixel_duration_ns = NSEC_PER_SEC / pixel_clock_hz;
> + unsigned long long htotal;
> + unsigned int vtotal;
> + unsigned int max_hact, hact_duration_ns;
> + unsigned int hblk, hblk_duration_ns;
> + unsigned int hfp, hfp_min, hfp_duration_ns;
> + unsigned int hslen, hslen_duration_ns;
> + unsigned int hbp, hbp_min, hbp_duration_ns;
> + unsigned int porches, porches_duration_ns;
> + unsigned int vfp, vfp_min;
> + unsigned int vbp, vbp_min;
> + unsigned int vslen;
> + int porches_rem;
> + bool strict = true;
> +
> + max_hact = params->hact_ns.max / pixel_duration_ns;
> + if (pixel_clock_hz == 13500000 && hactive > max_hact && hactive <= 720)
> + strict = false;
> +
> + /*
> + * Our pixel duration is going to be round down by the division,
> + * so rounding up is probably going to introduce even more
> + * deviation.
> + */
> + htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC;
> +
> + hact_duration_ns = hactive * pixel_duration_ns;
> + if (strict &&
> + (hact_duration_ns < params->hact_ns.min ||
> + hact_duration_ns > params->hact_ns.max)) {
> + DRM_ERROR("Invalid horizontal active area duration: %uns (min: %u, max %u)\n",
> + hact_duration_ns, params->hact_ns.min, params->hact_ns.max);
> + return -EINVAL;
> + }
> +
> + hblk = htotal - hactive;
> + hblk_duration_ns = hblk * pixel_duration_ns;
> + if (strict &&
> + (hblk_duration_ns < params->hblk_ns.min ||
> + hblk_duration_ns > params->hblk_ns.max)) {
> + DRM_ERROR("Invalid horizontal blanking duration: %uns (min: %u, max %u)\n",
> + hblk_duration_ns, params->hblk_ns.min, params->hblk_ns.max);
> + return -EINVAL;
> + }
> +
> + hslen = DIV_ROUND_UP(params->hslen_ns.typ, pixel_duration_ns);
> + hslen_duration_ns = hslen * pixel_duration_ns;
> + if (strict &&
> + (hslen_duration_ns < params->hslen_ns.min ||
> + hslen_duration_ns > params->hslen_ns.max)) {
> + DRM_ERROR("Invalid horizontal sync duration: %uns (min: %u, max %u)\n",
> + hslen_duration_ns, params->hslen_ns.min, params->hslen_ns.max);
> + return -EINVAL;
> + }
> +
> + porches = hblk - hslen;
> + porches_duration_ns = porches * pixel_duration_ns;
> + if (strict &&
> + (porches_duration_ns > (params->hfp_ns.max + params->hbp_ns.max) ||
> + porches_duration_ns < (params->hfp_ns.min + params->hbp_ns.min))) {
> + DRM_ERROR("Invalid horizontal porches duration: %uns\n", porches_duration_ns);
> + return -EINVAL;
> + }
> +
> + hfp_min = DIV_ROUND_UP(params->hfp_ns.min, pixel_duration_ns);
> + hbp_min = DIV_ROUND_UP(params->hbp_ns.min, pixel_duration_ns);
> + porches_rem = porches - hfp_min - hbp_min;
> +
> + hfp = hfp_min + DIV_ROUND_UP(porches_rem, 2);
> + hfp_duration_ns = hfp * pixel_duration_ns;
> + if (strict &&
> + (hfp_duration_ns < params->hfp_ns.min ||
> + hfp_duration_ns > params->hfp_ns.max)) {
> + DRM_ERROR("Invalid horizontal front porch duration: %uns (min: %u, max %u)\n",
> + hfp_duration_ns, params->hfp_ns.min, params->hfp_ns.max);
> + return -EINVAL;
> + }
> +
> + hbp = porches - hfp;
> + hbp_duration_ns = hbp * pixel_duration_ns;
> + if (strict &&
> + (hbp_duration_ns < params->hbp_ns.min ||
> + hbp_duration_ns > params->hbp_ns.max)) {
> + DRM_ERROR("Invalid horizontal back porch duration: %uns (min: %u, max %u)\n",
> + hbp_duration_ns, params->hbp_ns.min, params->hbp_ns.max);
> + return -EINVAL;
> + }
> +
> + if (htotal != (hactive + hfp + hslen + hbp))
> + return -EINVAL;
> +
> + mode->clock = pixel_clock_hz / 1000;
> + mode->hdisplay = hactive;
> + mode->hsync_start = hactive + hfp;
> + mode->hsync_end = hactive + hfp + hslen;
> + mode->htotal = hactive + hfp + hslen + hbp;
> +
> + if (interlace) {
> + vfp_min = params->vfp_lines.even + params->vfp_lines.odd;
> + vbp_min = params->vbp_lines.even + params->vbp_lines.odd;
> + vslen = params->vslen_lines.even + params->vslen_lines.odd;
> + } else {
> + /*
> + * By convention, NSTC (aka 525/60) systems start with
> + * the even field, but PAL (aka 625/50) systems start
> + * with the odd one.
> + *
> + * PAL systems also have asymetric timings between the
> + * even and odd field, while NTSC is symetric.
> + *
> + * Moreover, if we want to create a progressive mode for
> + * PAL, we need to use the odd field timings.
> + *
> + * Since odd == even for NTSC, we can just use the odd
> + * one all the time to simplify the code a bit.
> + */
> + vfp_min = params->vfp_lines.odd;
> + vbp_min = params->vbp_lines.odd;
> + vslen = params->vslen_lines.odd;
> + }
> +
> + porches = params->num_lines - vactive - vslen;
> + porches_rem = porches - vfp_min - vbp_min;
> +
> + vfp = vfp_min + (porches_rem / 2);
> + vbp = porches - vfp;
> +
> + vtotal = vactive + vfp + vslen + vbp;
> + if (params->num_lines != vtotal) {
> + DRM_ERROR("Invalid vertical total: %upx (expected %upx)\n",
> + vtotal, params->num_lines);
> + return -EINVAL;
> + }
> +
> + mode->vdisplay = vactive;
> + mode->vsync_start = vactive + vfp;
> + mode->vsync_end = vactive + vfp + vslen;
> + mode->vtotal = vactive + vfp + vslen + vbp;
> +
> + mode->type = DRM_MODE_TYPE_DRIVER;
> + mode->flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC;
> + if (interlace)
> + mode->flags |= DRM_MODE_FLAG_INTERLACE;
> +
> + drm_mode_set_name(mode);
> +
> + if (mode->vtotal != params->num_lines)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +/**
> + * drm_analog_tv_mode - create a display mode for an analog TV
> + * @dev: drm device
> + * @tv_mode: TV Mode standard to create a mode for. See DRM_MODE_TV_MODE_*.
> + * @pixel_clock_hz: Pixel Clock Frequency, in Hertz
> + * @hdisplay: hdisplay size
> + * @vdisplay: vdisplay size
> + * @interlace: whether to compute an interlaced mode
> + *
> + * This function creates a struct drm_display_mode instance suited for
> + * an analog TV output, for one of the usual analog TV mode.
> + *
> + * Note that @hdisplay is larger than the usual constraints for the PAL
> + * and NTSC timings, and we'll choose to ignore most timings constraints
> + * to reach those resolutions.
> + *
> + * Returns:
> + *
> + * A pointer to the mode, allocated with drm_mode_create(). Returns NULL
> + * on error.
> + */
> +struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
> + enum drm_connector_tv_mode tv_mode,
> + unsigned long pixel_clock_hz,
> + unsigned int hdisplay,
> + unsigned int vdisplay,
> + bool interlace)
> +{
> + struct drm_display_mode *mode;
> + enum drm_mode_analog analog;
> + int ret;
> +
> + switch (tv_mode) {
> + case DRM_MODE_TV_MODE_NTSC_443:
> + fallthrough;
> + case DRM_MODE_TV_MODE_NTSC_J:
> + fallthrough;
> + case DRM_MODE_TV_MODE_NTSC_M:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_60:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_M:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_60:
> + analog = DRM_MODE_ANALOG_NTSC;
> + break;
> +
> + case DRM_MODE_TV_MODE_PAL_B:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_D:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_G:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_H:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_I:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_N:
> + fallthrough;
> + case DRM_MODE_TV_MODE_PAL_NC:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_B:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_D:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_G:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_K:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_K1:
> + fallthrough;
> + case DRM_MODE_TV_MODE_SECAM_L:
> + analog = DRM_MODE_ANALOG_PAL;
> + break;
> +
> + default:
> + return NULL;
> + }
> +
> + mode = drm_mode_create(dev);
> + if (!mode)
> + return NULL;
> +
> + ret = fill_analog_mode(mode,
> + &tv_modes_parameters[analog],
> + pixel_clock_hz, hdisplay, vdisplay, interlace);
> + if (ret)
> + goto err_free_mode;
> +
> + return mode;
> +
> +err_free_mode:
> + drm_mode_destroy(dev, mode);
> + return NULL;
> +}
> +EXPORT_SYMBOL(drm_analog_tv_mode);
> +
> /**
> * drm_cvt_mode -create a modeline based on the CVT algorithm
> * @dev: drm device
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index b29ef1085cad..b22ac96fdd65 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -10,5 +10,6 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
> drm_framebuffer_test.o \
> drm_kunit_helpers.o \
> drm_mm_test.o \
> + drm_modes_test.o \
> drm_plane_helper_test.o \
> drm_rect_test.o
> diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
> new file mode 100644
> index 000000000000..87d398fcb99e
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_modes_test.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Kunit test for drm_modes functions
> + */
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_modes.h>
> +
> +#include "drm_kunit_helpers.h"
> +
> +struct drm_modes_test_priv {
> + struct drm_device *drm;
> +};
> +
> +static int drm_modes_test_init(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv;
> +
> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
I believe it would be nicer to use KUNIT_ASSERT_NOT_NULL here, instead
of returning a error.
> + test->priv = priv;
> +
> + priv->drm = drm_kunit_device_init("drm-modes-test");
> + if (IS_ERR(priv->drm))
> + return PTR_ERR(priv->drm);
Here you could use KUNIT_ASSERT_NOT_ERR_OR_NULL.
> +
> + return 0;
> +}
> +
> +static void drm_modes_test_exit(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> +
> + drm_kunit_device_exit(priv->drm);
> +}
> +
> +static void drm_modes_analog_tv_ntsc_480i(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> + struct drm_display_mode *mode;
> +
> + mode = drm_analog_tv_mode(priv->drm,
> + DRM_MODE_TV_MODE_NTSC_M,
> + 13500 * 1000, 720, 480,
> + true);
> + KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 60);
> + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> +
> + /* 63.556us * 13.5MHz = 858 pixels */
> + KUNIT_EXPECT_EQ(test, mode->htotal, 858);
> + KUNIT_EXPECT_EQ(test, mode->vdisplay, 480);
> + KUNIT_EXPECT_EQ(test, mode->vtotal, 525);
> +}
I would be nice to see this test and drm_modes_analog_tv_pal_576i
parametrized.
Best Regards,
- Maíra Canal
> +
> +static void drm_modes_analog_tv_ntsc_480i_inlined(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> + struct drm_display_mode *expected, *mode;
> +
> + expected = drm_analog_tv_mode(priv->drm,
> + DRM_MODE_TV_MODE_NTSC_M,
> + 13500 * 1000, 720, 480,
> + true);
> + KUNIT_ASSERT_NOT_NULL(test, expected);
> +
> + mode = drm_mode_analog_ntsc_480i(priv->drm);
> + KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
> +}
> +
> +static void drm_modes_analog_tv_pal_576i(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> + struct drm_display_mode *mode;
> +
> + mode = drm_analog_tv_mode(priv->drm,
> + DRM_MODE_TV_MODE_PAL_B,
> + 13500 * 1000, 720, 576,
> + true);
> + KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 50);
> + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> +
> + /* 64us * 13.5MHz = 864 pixels */
> + KUNIT_EXPECT_EQ(test, mode->htotal, 864);
> + KUNIT_EXPECT_EQ(test, mode->vdisplay, 576);
> + KUNIT_EXPECT_EQ(test, mode->vtotal, 625);
> +}
> +
> +static void drm_modes_analog_tv_pal_576i_inlined(struct kunit *test)
> +{
> + struct drm_modes_test_priv *priv = test->priv;
> + struct drm_display_mode *expected, *mode;
> +
> + expected = drm_analog_tv_mode(priv->drm,
> + DRM_MODE_TV_MODE_PAL_B,
> + 13500 * 1000, 720, 576,
> + true);
> + KUNIT_ASSERT_NOT_NULL(test, expected);
> +
> + mode = drm_mode_analog_pal_576i(priv->drm);
> + KUNIT_ASSERT_NOT_NULL(test, mode);
> +
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
> +}
> +
> +static struct kunit_case drm_modes_analog_tv_tests[] = {
> + KUNIT_CASE(drm_modes_analog_tv_ntsc_480i),
> + KUNIT_CASE(drm_modes_analog_tv_ntsc_480i_inlined),
> + KUNIT_CASE(drm_modes_analog_tv_pal_576i),
> + KUNIT_CASE(drm_modes_analog_tv_pal_576i_inlined),
> + { }
> +};
> +
> +static struct kunit_suite drm_modes_analog_tv_test_suite = {
> + .name = "drm_modes_analog_tv",
> + .init = drm_modes_test_init,
> + .exit = drm_modes_test_exit,
> + .test_cases = drm_modes_analog_tv_tests,
> +};
> +
> +kunit_test_suites(
> + &drm_modes_analog_tv_test_suite
> +);
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
> index a80ae9639e96..5ccf3d51d313 100644
> --- a/include/drm/drm_modes.h
> +++ b/include/drm/drm_modes.h
> @@ -443,6 +443,23 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
> bool drm_mode_is_420(const struct drm_display_info *display,
> const struct drm_display_mode *mode);
>
> +struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
> + enum drm_connector_tv_mode mode,
> + unsigned long pixel_clock_hz,
> + unsigned int hdisplay,
> + unsigned int vdisplay,
> + bool interlace);
> +
> +static inline struct drm_display_mode *drm_mode_analog_ntsc_480i(struct drm_device *dev)
> +{
> + return drm_analog_tv_mode(dev, DRM_MODE_TV_MODE_NTSC_M, 13500000, 720, 480, true);
> +}
> +
> +static inline struct drm_display_mode *drm_mode_analog_pal_576i(struct drm_device *dev)
> +{
> + return drm_analog_tv_mode(dev, DRM_MODE_TV_MODE_PAL_B, 13500000, 720, 576, true);
> +}
> +
> struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
> int hdisplay, int vdisplay, int vrefresh,
> bool reduced, bool interlaced,
>
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
Hi Mateusz,
On Wed, Aug 31, 2022 at 3:44 AM Mateusz Kwiatkowski <[email protected]> wrote:
> 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.
We'll need it when we get an Amiga DRM driver, which will use
7/14/28 MHz pixel clocks.
> 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)
IMHO these are bad names, as e.g. VGA640x480@60 is also analog, using
60 Hz and 525 lines. Add "TV" to the name?
> - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System
> Letter Designations)
Or DRM_MODE_ITU_*?
But given the long list of letters, this looks fragile to me.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Den 29.08.2022 15.11, skrev Maxime Ripard:
> Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
>
> 625-lines modes in their drivers.
>
>
>
> Since those modes are fairly standard, and that we'll need to use them
>
> in more places in the future, it makes sense to move their definition
>
> into the core framework.
>
>
>
> However, analog display usually have fairly loose timings requirements,
>
> the only discrete parameters being the total number of lines and pixel
>
> clock frequency. Thus, we created a function that will create a display
>
> mode from the standard, the pixel frequency and the active area.
>
>
>
> Signed-off-by: Maxime Ripard <[email protected]>
>
On a 32-bit build I'm getting bogus modes:
[ 249.599997] [drm:drm_helper_probe_single_connector_modes]
[CONNECTOR:45:Composite-1]
[ 249.600198] [drm:drm_mode_debug_printmodeline] Modeline "720x480i":
17143 13500 720 308 372 3 480 499 505 525 0x40 0x1a
[ 249.600292] [drm:drm_mode_prune_invalid] Not using 720x480i mode:
H_ILLEGAL
[ 249.600317] [drm:drm_mode_debug_printmodeline] Modeline "720x576i": 0
13500 720 302 366 0 576 597 603 625 0x40 0x1a
[ 249.600349] [drm:drm_mode_prune_invalid] Not using 720x576i mode:
H_ILLEGAL
[ 249.600374] [drm:drm_helper_probe_single_connector_modes]
[CONNECTOR:45:Composite-1] probed modes :
[ 249.600453] [drm:drm_mode_debug_printmodeline] Modeline "720x240i":
60 27800 720 736 808 896 240 241 244 516 0x20 0x6
It's fine on 64-bit.
Noralf.
>
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>
> index 304004fb80aa..ee581ee17171 100644
>
> --- a/drivers/gpu/drm/drm_modes.c
>
> +++ b/drivers/gpu/drm/drm_modes.c
>
> @@ -116,6 +116,459 @@ void drm_mode_probed_add(struct drm_connector *connector,
>
> }
>
> EXPORT_SYMBOL(drm_mode_probed_add);
>
>
>
> +enum drm_mode_analog {
>
> + DRM_MODE_ANALOG_NTSC,
>
> + DRM_MODE_ANALOG_PAL,
>
> +};
>
> +
>
> +/*
>
> + * The timings come from:
>
> + * - https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
>
> + * - https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
>
> + * - https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
>
> + */
>
> +#define NTSC_LINE_DURATION_NS 63556U
>
> +#define NTSC_LINES_NUMBER 525
>
> +
>
> +#define NTSC_HBLK_DURATION_TYP_NS 10900U
>
> +#define NTSC_HBLK_DURATION_MIN_NS (NTSC_HBLK_DURATION_TYP_NS - 200)
>
> +#define NTSC_HBLK_DURATION_MAX_NS (NTSC_HBLK_DURATION_TYP_NS + 200)
>
> +
>
> +#define NTSC_HACT_DURATION_TYP_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_TYP_NS)
>
> +#define NTSC_HACT_DURATION_MIN_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MAX_NS)
>
> +#define NTSC_HACT_DURATION_MAX_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MIN_NS)
>
> +
>
> +#define NTSC_HFP_DURATION_TYP_NS 1500
>
> +#define NTSC_HFP_DURATION_MIN_NS 1270
>
> +#define NTSC_HFP_DURATION_MAX_NS 2220
>
> +
>
> +#define NTSC_HSLEN_DURATION_TYP_NS 4700
>
> +#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
>
> +#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
>
> +
>
> +#define NTSC_HBP_DURATION_TYP_NS 4700
>
> +
>
> +/*
>
> + * I couldn't find the actual tolerance for the back porch, so let's
>
> + * just reuse the sync length ones.
>
> + */
>
> +#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
>
> +#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
>
> +
>
> +#define PAL_LINE_DURATION_NS 64000U
>
> +#define PAL_LINES_NUMBER 625
>
> +
>
> +#define PAL_HACT_DURATION_TYP_NS 51950U
>
> +#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
>
> +#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
>
> +
>
> +#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_TYP_NS)
>
> +#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MAX_NS)
>
> +#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MIN_NS)
>
> +
>
> +#define PAL_HFP_DURATION_TYP_NS 1650
>
> +#define PAL_HFP_DURATION_MIN_NS (PAL_HFP_DURATION_TYP_NS - 100)
>
> +#define PAL_HFP_DURATION_MAX_NS (PAL_HFP_DURATION_TYP_NS + 400)
>
> +
>
> +#define PAL_HSLEN_DURATION_TYP_NS 4700
>
> +#define PAL_HSLEN_DURATION_MIN_NS (PAL_HSLEN_DURATION_TYP_NS - 200)
>
> +#define PAL_HSLEN_DURATION_MAX_NS (PAL_HSLEN_DURATION_TYP_NS + 200)
>
> +
>
> +#define PAL_HBP_DURATION_TYP_NS 5700
>
> +#define PAL_HBP_DURATION_MIN_NS (PAL_HBP_DURATION_TYP_NS - 200)
>
> +#define PAL_HBP_DURATION_MAX_NS (PAL_HBP_DURATION_TYP_NS + 200)
>
> +
>
> +#define PAL_VFP_INTERLACE_LINES 5
>
> +#define PAL_VSLEN_INTERLACE_LINES 5
>
> +
>
> +#define PAL_SHORT_SYNC_DURATION_NS ((2 + 30) * NSEC_PER_USEC)
>
> +#define PAL_LONG_SYNC_DURATION_NS ((30 + 2) * NSEC_PER_USEC)
>
> +
>
> +struct analog_param_field {
>
> + unsigned int even, odd;
>
> +};
>
> +
>
> +#define PARAM_FIELD(_odd, _even) \
>
> + { .even = _even, .odd = _odd }
>
> +
>
> +struct analog_param_range {
>
> + unsigned int min, typ, max;
>
> +};
>
> +
>
> +#define PARAM_RANGE(_min, _typ, _max) \
>
> + { .min = _min, .typ = _typ, .max = _max }
>
> +
>
> +struct analog_parameters {
>
> + unsigned int num_lines;
>
> + unsigned int line_duration_ns;
>
> +
>
> + struct analog_param_range hact_ns;
>
> + struct analog_param_range hfp_ns;
>
> + struct analog_param_range hslen_ns;
>
> + struct analog_param_range hbp_ns;
>
> + struct analog_param_range hblk_ns;
>
> +
>
> + struct analog_param_field vfp_lines;
>
> + struct analog_param_field vslen_lines;
>
> + struct analog_param_field vbp_lines;
>
> +};
>
> +
>
> +#define TV_MODE_PARAMETER(_mode, _lines, _line_dur, _hact, _hfp, _hslen, _hbp, _hblk, _vfp, _vslen, _vbp) \
>
> + [_mode] = { \
>
> + .num_lines = _lines, \
>
> + .line_duration_ns = _line_dur, \
>
> + .hact_ns = _hact, \
>
> + .hfp_ns = _hfp, \
>
> + .hslen_ns = _hslen, \
>
> + .hbp_ns = _hbp, \
>
> + .hblk_ns = _hblk, \
>
> + .vfp_lines = _vfp, \
>
> + .vslen_lines = _vslen, \
>
> + .vbp_lines = _vbp, \
>
> + }
>
> +
>
> +const static struct analog_parameters tv_modes_parameters[] = {
>
> + TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
>
> + NTSC_LINES_NUMBER,
>
> + NTSC_LINE_DURATION_NS,
>
> + PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
>
> + NTSC_HACT_DURATION_TYP_NS,
>
> + NTSC_HACT_DURATION_MAX_NS),
>
> + PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
>
> + NTSC_HFP_DURATION_TYP_NS,
>
> + NTSC_HFP_DURATION_MAX_NS),
>
> + PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
>
> + NTSC_HSLEN_DURATION_TYP_NS,
>
> + NTSC_HSLEN_DURATION_MAX_NS),
>
> + PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
>
> + NTSC_HBP_DURATION_TYP_NS,
>
> + NTSC_HBP_DURATION_MAX_NS),
>
> + PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
>
> + NTSC_HBLK_DURATION_TYP_NS,
>
> + NTSC_HBLK_DURATION_MAX_NS),
>
> + PARAM_FIELD(3, 3),
>
> + PARAM_FIELD(3, 3),
>
> + PARAM_FIELD(3, 3)),
>
> + TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
>
> + PAL_LINES_NUMBER,
>
> + PAL_LINE_DURATION_NS,
>
> + PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
>
> + PAL_HACT_DURATION_TYP_NS,
>
> + PAL_HACT_DURATION_MAX_NS),
>
> + PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
>
> + PAL_HFP_DURATION_TYP_NS,
>
> + PAL_HFP_DURATION_MAX_NS),
>
> + PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
>
> + PAL_HSLEN_DURATION_TYP_NS,
>
> + PAL_HSLEN_DURATION_MAX_NS),
>
> + PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
>
> + PAL_HBP_DURATION_TYP_NS,
>
> + PAL_HBP_DURATION_MAX_NS),
>
> + PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
>
> + PAL_HBLK_DURATION_TYP_NS,
>
> + PAL_HBLK_DURATION_MAX_NS),
>
> +
>
> + /*
>
> + * The front porch is actually 6 short sync
>
> + * pulses for the even field, and 5 for the
>
> + * odd field. Each sync takes half a life so
>
> + * the odd field front porch is shorter by
>
> + * half a line.
>
> + *
>
> + * In progressive, we're supposed to use 6
>
> + * pulses, so we're fine there
>
> + */
>
> + PARAM_FIELD(3, 2),
>
> +
>
> + /*
>
> + * The vsync length is 5 long sync pulses,
>
> + * each field taking half a line. We're
>
> + * shorter for both fields by half a line.
>
> + *
>
> + * In progressive, we're supposed to use 5
>
> + * pulses, so we're off by half
>
> + * a line.
>
> + *
>
> + * In interlace, we're now off by half a line
>
> + * for the even field and one line for the odd
>
> + * field.
>
> + */
>
> + PARAM_FIELD(3, 3),
>
> +
>
> + /*
>
> + * The back porch is actually 5 short sync
>
> + * pulses for the even field, 4 for the odd
>
> + * field. In progressive, it's 5 short syncs.
>
> + *
>
> + * In progressive, we thus have 2.5 lines,
>
> + * plus the 0.5 line we were missing
>
> + * previously, so we should use 3 lines.
>
> + *
>
> + * In interlace, the even field is in the
>
> + * exact same case than progressive. For the
>
> + * odd field, we should be using 2 lines but
>
> + * we're one line short, so we'll make up for
>
> + * it here by using 3.
>
> + */
>
> + PARAM_FIELD(3, 3)),
>
> +};
>
> +
>
> +static int fill_analog_mode(struct drm_display_mode *mode,
>
> + const struct analog_parameters *params,
>
> + unsigned long pixel_clock_hz,
>
> + unsigned int hactive,
>
> + unsigned int vactive,
>
> + bool interlace)
>
> +{
>
> + unsigned long pixel_duration_ns = NSEC_PER_SEC / pixel_clock_hz;
>
> + unsigned long long htotal;
>
> + unsigned int vtotal;
>
> + unsigned int max_hact, hact_duration_ns;
>
> + unsigned int hblk, hblk_duration_ns;
>
> + unsigned int hfp, hfp_min, hfp_duration_ns;
>
> + unsigned int hslen, hslen_duration_ns;
>
> + unsigned int hbp, hbp_min, hbp_duration_ns;
>
> + unsigned int porches, porches_duration_ns;
>
> + unsigned int vfp, vfp_min;
>
> + unsigned int vbp, vbp_min;
>
> + unsigned int vslen;
>
> + int porches_rem;
>
> + bool strict = true;
>
> +
>
> + max_hact = params->hact_ns.max / pixel_duration_ns;
>
> + if (pixel_clock_hz == 13500000 && hactive > max_hact && hactive <= 720)
>
> + strict = false;
>
> +
>
> + /*
>
> + * Our pixel duration is going to be round down by the division,
>
> + * so rounding up is probably going to introduce even more
>
> + * deviation.
>
> + */
>
> + htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC;
>
> +
>
> + hact_duration_ns = hactive * pixel_duration_ns;
>
> + if (strict &&
>
> + (hact_duration_ns < params->hact_ns.min ||
>
> + hact_duration_ns > params->hact_ns.max)) {
>
> + DRM_ERROR("Invalid horizontal active area duration: %uns (min: %u, max %u)\n",
>
> + hact_duration_ns, params->hact_ns.min, params->hact_ns.max);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + hblk = htotal - hactive;
>
> + hblk_duration_ns = hblk * pixel_duration_ns;
>
> + if (strict &&
>
> + (hblk_duration_ns < params->hblk_ns.min ||
>
> + hblk_duration_ns > params->hblk_ns.max)) {
>
> + DRM_ERROR("Invalid horizontal blanking duration: %uns (min: %u, max %u)\n",
>
> + hblk_duration_ns, params->hblk_ns.min, params->hblk_ns.max);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + hslen = DIV_ROUND_UP(params->hslen_ns.typ, pixel_duration_ns);
>
> + hslen_duration_ns = hslen * pixel_duration_ns;
>
> + if (strict &&
>
> + (hslen_duration_ns < params->hslen_ns.min ||
>
> + hslen_duration_ns > params->hslen_ns.max)) {
>
> + DRM_ERROR("Invalid horizontal sync duration: %uns (min: %u, max %u)\n",
>
> + hslen_duration_ns, params->hslen_ns.min, params->hslen_ns.max);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + porches = hblk - hslen;
>
> + porches_duration_ns = porches * pixel_duration_ns;
>
> + if (strict &&
>
> + (porches_duration_ns > (params->hfp_ns.max + params->hbp_ns.max) ||
>
> + porches_duration_ns < (params->hfp_ns.min + params->hbp_ns.min))) {
>
> + DRM_ERROR("Invalid horizontal porches duration: %uns\n", porches_duration_ns);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + hfp_min = DIV_ROUND_UP(params->hfp_ns.min, pixel_duration_ns);
>
> + hbp_min = DIV_ROUND_UP(params->hbp_ns.min, pixel_duration_ns);
>
> + porches_rem = porches - hfp_min - hbp_min;
>
> +
>
> + hfp = hfp_min + DIV_ROUND_UP(porches_rem, 2);
>
> + hfp_duration_ns = hfp * pixel_duration_ns;
>
> + if (strict &&
>
> + (hfp_duration_ns < params->hfp_ns.min ||
>
> + hfp_duration_ns > params->hfp_ns.max)) {
>
> + DRM_ERROR("Invalid horizontal front porch duration: %uns (min: %u, max %u)\n",
>
> + hfp_duration_ns, params->hfp_ns.min, params->hfp_ns.max);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + hbp = porches - hfp;
>
> + hbp_duration_ns = hbp * pixel_duration_ns;
>
> + if (strict &&
>
> + (hbp_duration_ns < params->hbp_ns.min ||
>
> + hbp_duration_ns > params->hbp_ns.max)) {
>
> + DRM_ERROR("Invalid horizontal back porch duration: %uns (min: %u, max %u)\n",
>
> + hbp_duration_ns, params->hbp_ns.min, params->hbp_ns.max);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + if (htotal != (hactive + hfp + hslen + hbp))
>
> + return -EINVAL;
>
> +
>
> + mode->clock = pixel_clock_hz / 1000;
>
> + mode->hdisplay = hactive;
>
> + mode->hsync_start = hactive + hfp;
>
> + mode->hsync_end = hactive + hfp + hslen;
>
> + mode->htotal = hactive + hfp + hslen + hbp;
>
> +
>
> + if (interlace) {
>
> + vfp_min = params->vfp_lines.even + params->vfp_lines.odd;
>
> + vbp_min = params->vbp_lines.even + params->vbp_lines.odd;
>
> + vslen = params->vslen_lines.even + params->vslen_lines.odd;
>
> + } else {
>
> + /*
>
> + * By convention, NSTC (aka 525/60) systems start with
>
> + * the even field, but PAL (aka 625/50) systems start
>
> + * with the odd one.
>
> + *
>
> + * PAL systems also have asymetric timings between the
>
> + * even and odd field, while NTSC is symetric.
>
> + *
>
> + * Moreover, if we want to create a progressive mode for
>
> + * PAL, we need to use the odd field timings.
>
> + *
>
> + * Since odd == even for NTSC, we can just use the odd
>
> + * one all the time to simplify the code a bit.
>
> + */
>
> + vfp_min = params->vfp_lines.odd;
>
> + vbp_min = params->vbp_lines.odd;
>
> + vslen = params->vslen_lines.odd;
>
> + }
>
> +
>
> + porches = params->num_lines - vactive - vslen;
>
> + porches_rem = porches - vfp_min - vbp_min;
>
> +
>
> + vfp = vfp_min + (porches_rem / 2);
>
> + vbp = porches - vfp;
>
> +
>
> + vtotal = vactive + vfp + vslen + vbp;
>
> + if (params->num_lines != vtotal) {
>
> + DRM_ERROR("Invalid vertical total: %upx (expected %upx)\n",
>
> + vtotal, params->num_lines);
>
> + return -EINVAL;
>
> + }
>
> +
>
> + mode->vdisplay = vactive;
>
> + mode->vsync_start = vactive + vfp;
>
> + mode->vsync_end = vactive + vfp + vslen;
>
> + mode->vtotal = vactive + vfp + vslen + vbp;
>
> +
>
> + mode->type = DRM_MODE_TYPE_DRIVER;
>
> + mode->flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC;
>
> + if (interlace)
>
> + mode->flags |= DRM_MODE_FLAG_INTERLACE;
>
> +
>
> + drm_mode_set_name(mode);
>
> +
>
> + if (mode->vtotal != params->num_lines)
>
> + return -EINVAL;
>
> +
>
> + return 0;
>
> +}
>
> +
>
> +/**
>
> + * drm_analog_tv_mode - create a display mode for an analog TV
>
> + * @dev: drm device
>
> + * @tv_mode: TV Mode standard to create a mode for. See DRM_MODE_TV_MODE_*.
>
> + * @pixel_clock_hz: Pixel Clock Frequency, in Hertz
>
> + * @hdisplay: hdisplay size
>
> + * @vdisplay: vdisplay size
>
> + * @interlace: whether to compute an interlaced mode
>
> + *
>
> + * This function creates a struct drm_display_mode instance suited for
>
> + * an analog TV output, for one of the usual analog TV mode.
>
> + *
>
> + * Note that @hdisplay is larger than the usual constraints for the PAL
>
> + * and NTSC timings, and we'll choose to ignore most timings constraints
>
> + * to reach those resolutions.
>
> + *
>
> + * Returns:
>
> + *
>
> + * A pointer to the mode, allocated with drm_mode_create(). Returns NULL
>
> + * on error.
>
> + */
>
> +struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
>
> + enum drm_connector_tv_mode tv_mode,
>
> + unsigned long pixel_clock_hz,
>
> + unsigned int hdisplay,
>
> + unsigned int vdisplay,
>
> + bool interlace)
>
> +{
>
> + struct drm_display_mode *mode;
>
> + enum drm_mode_analog analog;
>
> + int ret;
>
> +
>
> + switch (tv_mode) {
>
> + case DRM_MODE_TV_MODE_NTSC_443:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_NTSC_J:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_NTSC_M:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_60:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_M:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_60:
>
> + analog = DRM_MODE_ANALOG_NTSC;
>
> + break;
>
> +
>
> + case DRM_MODE_TV_MODE_PAL_B:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_D:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_G:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_H:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_I:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_N:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_PAL_NC:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_B:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_D:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_G:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_K:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_K1:
>
> + fallthrough;
>
> + case DRM_MODE_TV_MODE_SECAM_L:
>
> + analog = DRM_MODE_ANALOG_PAL;
>
> + break;
>
> +
>
> + default:
>
> + return NULL;
>
> + }
>
> +
>
> + mode = drm_mode_create(dev);
>
> + if (!mode)
>
> + return NULL;
>
> +
>
> + ret = fill_analog_mode(mode,
>
> + &tv_modes_parameters[analog],
>
> + pixel_clock_hz, hdisplay, vdisplay, interlace);
>
> + if (ret)
>
> + goto err_free_mode;
>
> +
>
> + return mode;
>
> +
>
> +err_free_mode:
>
> + drm_mode_destroy(dev, mode);
>
> + return NULL;
>
> +}
>
> +EXPORT_SYMBOL(drm_analog_tv_mode);
>
> +
>
> /**
>
> * drm_cvt_mode -create a modeline based on the CVT algorithm
>
> * @dev: drm device
>
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
>
> index b29ef1085cad..b22ac96fdd65 100644
>
> --- a/drivers/gpu/drm/tests/Makefile
>
> +++ b/drivers/gpu/drm/tests/Makefile
>
> @@ -10,5 +10,6 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>
> drm_framebuffer_test.o \
>
> drm_kunit_helpers.o \
>
> drm_mm_test.o \
>
> + drm_modes_test.o \
>
> drm_plane_helper_test.o \
>
> drm_rect_test.o
>
> diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
>
> new file mode 100644
>
> index 000000000000..87d398fcb99e
>
> --- /dev/null
>
> +++ b/drivers/gpu/drm/tests/drm_modes_test.c
>
> @@ -0,0 +1,131 @@
>
> +// SPDX-License-Identifier: GPL-2.0
>
> +/*
>
> + * Kunit test for drm_modes functions
>
> + */
>
> +
>
> +#include <kunit/test.h>
>
> +
>
> +#include <drm/drm_modes.h>
>
> +
>
> +#include "drm_kunit_helpers.h"
>
> +
>
> +struct drm_modes_test_priv {
>
> + struct drm_device *drm;
>
> +};
>
> +
>
> +static int drm_modes_test_init(struct kunit *test)
>
> +{
>
> + struct drm_modes_test_priv *priv;
>
> +
>
> + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
>
> + if (!priv)
>
> + return -ENOMEM;
>
> + test->priv = priv;
>
> +
>
> + priv->drm = drm_kunit_device_init("drm-modes-test");
>
> + if (IS_ERR(priv->drm))
>
> + return PTR_ERR(priv->drm);
>
> +
>
> + return 0;
>
> +}
>
> +
>
> +static void drm_modes_test_exit(struct kunit *test)
>
> +{
>
> + struct drm_modes_test_priv *priv = test->priv;
>
> +
>
> + drm_kunit_device_exit(priv->drm);
>
> +}
>
> +
>
> +static void drm_modes_analog_tv_ntsc_480i(struct kunit *test)
>
> +{
>
> + struct drm_modes_test_priv *priv = test->priv;
>
> + struct drm_display_mode *mode;
>
> +
>
> + mode = drm_analog_tv_mode(priv->drm,
>
> + DRM_MODE_TV_MODE_NTSC_M,
>
> + 13500 * 1000, 720, 480,
>
> + true);
>
> + KUNIT_ASSERT_NOT_NULL(test, mode);
>
> +
>
> + KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 60);
>
> + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
>
> +
>
> + /* 63.556us * 13.5MHz = 858 pixels */
>
> + KUNIT_EXPECT_EQ(test, mode->htotal, 858);
>
> + KUNIT_EXPECT_EQ(test, mode->vdisplay, 480);
>
> + KUNIT_EXPECT_EQ(test, mode->vtotal, 525);
>
> +}
>
> +
>
> +static void drm_modes_analog_tv_ntsc_480i_inlined(struct kunit *test)
>
> +{
>
> + struct drm_modes_test_priv *priv = test->priv;
>
> + struct drm_display_mode *expected, *mode;
>
> +
>
> + expected = drm_analog_tv_mode(priv->drm,
>
> + DRM_MODE_TV_MODE_NTSC_M,
>
> + 13500 * 1000, 720, 480,
>
> + true);
>
> + KUNIT_ASSERT_NOT_NULL(test, expected);
>
> +
>
> + mode = drm_mode_analog_ntsc_480i(priv->drm);
>
> + KUNIT_ASSERT_NOT_NULL(test, mode);
>
> +
>
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
>
> +}
>
> +
>
> +static void drm_modes_analog_tv_pal_576i(struct kunit *test)
>
> +{
>
> + struct drm_modes_test_priv *priv = test->priv;
>
> + struct drm_display_mode *mode;
>
> +
>
> + mode = drm_analog_tv_mode(priv->drm,
>
> + DRM_MODE_TV_MODE_PAL_B,
>
> + 13500 * 1000, 720, 576,
>
> + true);
>
> + KUNIT_ASSERT_NOT_NULL(test, mode);
>
> +
>
> + KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 50);
>
> + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
>
> +
>
> + /* 64us * 13.5MHz = 864 pixels */
>
> + KUNIT_EXPECT_EQ(test, mode->htotal, 864);
>
> + KUNIT_EXPECT_EQ(test, mode->vdisplay, 576);
>
> + KUNIT_EXPECT_EQ(test, mode->vtotal, 625);
>
> +}
>
> +
>
> +static void drm_modes_analog_tv_pal_576i_inlined(struct kunit *test)
>
> +{
>
> + struct drm_modes_test_priv *priv = test->priv;
>
> + struct drm_display_mode *expected, *mode;
>
> +
>
> + expected = drm_analog_tv_mode(priv->drm,
>
> + DRM_MODE_TV_MODE_PAL_B,
>
> + 13500 * 1000, 720, 576,
>
> + true);
>
> + KUNIT_ASSERT_NOT_NULL(test, expected);
>
> +
>
> + mode = drm_mode_analog_pal_576i(priv->drm);
>
> + KUNIT_ASSERT_NOT_NULL(test, mode);
>
> +
>
> + KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected, mode));
>
> +}
>
> +
>
> +static struct kunit_case drm_modes_analog_tv_tests[] = {
>
> + KUNIT_CASE(drm_modes_analog_tv_ntsc_480i),
>
> + KUNIT_CASE(drm_modes_analog_tv_ntsc_480i_inlined),
>
> + KUNIT_CASE(drm_modes_analog_tv_pal_576i),
>
> + KUNIT_CASE(drm_modes_analog_tv_pal_576i_inlined),
>
> + { }
>
> +};
>
> +
>
> +static struct kunit_suite drm_modes_analog_tv_test_suite = {
>
> + .name = "drm_modes_analog_tv",
>
> + .init = drm_modes_test_init,
>
> + .exit = drm_modes_test_exit,
>
> + .test_cases = drm_modes_analog_tv_tests,
>
> +};
>
> +
>
> +kunit_test_suites(
>
> + &drm_modes_analog_tv_test_suite
>
> +);
>
> +MODULE_LICENSE("GPL v2");
>
> diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h
>
> index a80ae9639e96..5ccf3d51d313 100644
>
> --- a/include/drm/drm_modes.h
>
> +++ b/include/drm/drm_modes.h
>
> @@ -443,6 +443,23 @@ bool drm_mode_is_420_also(const struct drm_display_info *display,
>
> bool drm_mode_is_420(const struct drm_display_info *display,
>
> const struct drm_display_mode *mode);
>
>
>
> +struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
>
> + enum drm_connector_tv_mode mode,
>
> + unsigned long pixel_clock_hz,
>
> + unsigned int hdisplay,
>
> + unsigned int vdisplay,
>
> + bool interlace);
>
> +
>
> +static inline struct drm_display_mode *drm_mode_analog_ntsc_480i(struct drm_device *dev)
>
> +{
>
> + return drm_analog_tv_mode(dev, DRM_MODE_TV_MODE_NTSC_M, 13500000, 720, 480, true);
>
> +}
>
> +
>
> +static inline struct drm_display_mode *drm_mode_analog_pal_576i(struct drm_device *dev)
>
> +{
>
> + return drm_analog_tv_mode(dev, DRM_MODE_TV_MODE_PAL_B, 13500000, 720, 576, true);
>
> +}
>
> +
>
> struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
>
> int hdisplay, int vdisplay, int vrefresh,
>
> bool reduced, bool interlaced,
>
>
>
Hi,
On Wed, Aug 31, 2022 at 03:44:52AM +0200, Mateusz Kwiatkowski wrote:
> > +#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.
Yeah... I've left most of them because it was so hard to find most of
them, it's useful at least for documentation purposes. And it's a define
so there's pretty much no downside to it as far as the final binary is
involved.
> 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.
I'll try to fix that up.
> > +?? ?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.
Indeed, that's bad.
> > +?? ?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.
And you would increase the back porch only then?
Maxime
Hi,
On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven wrote:
> > > +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)
>
> IMHO these are bad names, as e.g. VGA640x480@60 is also analog, using
> 60 Hz and 525 lines. Add "TV" to the name?
>
> > - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System
> > Letter Designations)
>
> Or DRM_MODE_ITU_*?
> But given the long list of letters, this looks fragile to me.
Does it matter at all? It's an internal API that isn't exposed at all.
I'd rather have a common name that everyone can understand in this case
rather than a *perfect* name where most will scratch their head
wondering what it's about.
Maxime
Hi Maxime,
W dniu 5.09.2022 o 15:32, Maxime Ripard pisze:
> Hi,
>
> On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven wrote:
>>>> +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)
>>
>> IMHO these are bad names, as e.g. VGA640x480@60 is also analog, using
>> 60 Hz and 525 lines. Add "TV" to the name?
>>
>>> - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System
>>> Letter Designations)
>>
>> Or DRM_MODE_ITU_*?
>> But given the long list of letters, this looks fragile to me.
>
> Does it matter at all? It's an internal API that isn't exposed at all.
> I'd rather have a common name that everyone can understand in this case
> rather than a *perfect* name where most will scratch their head
> wondering what it's about.
You may have a point. But in that case, maybe it'd make sense to at least add
a short comment explaining what do you mean by "NTSC" and "PAL" in this context?
Best regards,
Mateusz Kwiatkowski
Hi Maxime,
W dniu 5.09.2022 o 15:37, Maxime Ripard pisze:
>>> + 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.
>
> And you would increase the back porch only then?
Well, increasing vbp only gives a centered image with the default 480i/576i
resolutions. However, only ever changing vbp will cause the image to be always
at the bottom of the screen when the active line count is decreased (e.g.
setting the resolution to 720x480 but for 50Hz "PAL" - like many game consoles
did back in the day).
I believe that the perfect solution would:
- Use the canonical / standard-defined blanking line counts for the standard
vertical resolutions (480/486/576)
- Increase vfp and vbp from there by the same number if a smaller number of
active lines is specified, so that the resulting image is centered
- Likewise, decrease vfp and vbp by the same number if the active line number
is larger and there is still leeway (this should allow for seamless handling
of 480i vs. 486i for 60 Hz "NTSC")
- If even more active lines are specified, once the limit for vfp is hit, then
decrease vbp only - the resulting image will definitely be off-center, but
there's no other way
I hope this makes sense for you as well.
Best regards,
Mateusz Kwiatkowski
On Mon, Sep 05, 2022 at 06:32:14PM +0200, Mateusz Kwiatkowski wrote:
> Hi Maxime,
>
> W dniu 5.09.2022 o 15:32, Maxime Ripard pisze:
> > Hi,
> >
> > On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven wrote:
> >>>> +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)
> >>
> >> IMHO these are bad names, as e.g. VGA640x480@60 is also analog, using
> >> 60 Hz and 525 lines.? Add "TV" to the name?
> >>
> >>> - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding ITU System
> >>>?? Letter Designations)
> >>
> >> Or DRM_MODE_ITU_*?
> >> But given the long list of letters, this looks fragile to me.
> >
> > Does it matter at all? It's an internal API that isn't exposed at all.
> > I'd rather have a common name that everyone can understand in this case
> > rather than a *perfect* name where most will scratch their head
> > wondering what it's about.
>
> You may have a point. But in that case, maybe it'd make sense to at least add
> a short comment explaining what do you mean by "NTSC" and "PAL" in this context?
Consider it done :)
Maxime
On Mon, Sep 05, 2022 at 06:44:42PM +0200, Mateusz Kwiatkowski wrote:
> Hi Maxime,
>
> W dniu 5.09.2022 o 15:37, Maxime Ripard pisze:
> >>> +??? 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.
> >
> > And you would increase the back porch only then?
>
> Well, increasing vbp only gives a centered image with the default 480i/576i
> resolutions. However, only ever changing vbp will cause the image to be always
> at the bottom of the screen when the active line count is decreased (e.g.
> setting the resolution to 720x480 but for 50Hz "PAL" - like many game consoles
> did back in the day).
>
> I believe that the perfect solution would:
>
> - Use the canonical / standard-defined blanking line counts for the standard
> ? vertical resolutions (480/486/576)
> - Increase vfp and vbp from there by the same number if a smaller number of
> ? active lines is specified, so that the resulting image is centered
> - Likewise, decrease vfp and vbp by the same number if the active line number
> ? is larger and there is still leeway (this should allow for seamless handling
> ? of 480i vs. 486i for 60 Hz "NTSC")
I'm not sure I understand how that's any different than the code you
initially commented on.
I would start by taking the entire blanking area, remove the sync
period. We only have the two porches now, and I'm starting from the
minimum, adding as many pixels in both (unless it's not an even number,
in which case the backporch will have the extra pixel).
Isn't it the same thing?
> - If even more active lines are specified, once the limit for vfp is hit, then
> ? decrease vbp only - the resulting image will definitely be off-center, but
> ? there's no other way
Unless you only want me to consider the front porch maximum?
Maxime
Hi Maxime,
W dniu 7.09.2022 o 16:34, Maxime Ripard pisze:
> On Mon, Sep 05, 2022 at 06:44:42PM +0200, Mateusz Kwiatkowski wrote:
>> Hi Maxime,
>>
>> W dniu 5.09.2022 o 15:37, Maxime Ripard pisze:
>>>>> + 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.
>>>
>>> And you would increase the back porch only then?
>>
>> Well, increasing vbp only gives a centered image with the default 480i/576i
>> resolutions. However, only ever changing vbp will cause the image to be always
>> at the bottom of the screen when the active line count is decreased (e.g.
>> setting the resolution to 720x480 but for 50Hz "PAL" - like many game consoles
>> did back in the day).
>>
>> I believe that the perfect solution would:
>>
>> - Use the canonical / standard-defined blanking line counts for the standard
>> vertical resolutions (480/486/576)
>> - Increase vfp and vbp from there by the same number if a smaller number of
>> active lines is specified, so that the resulting image is centered
>> - Likewise, decrease vfp and vbp by the same number if the active line number
>> is larger and there is still leeway (this should allow for seamless handling
>> of 480i vs. 486i for 60 Hz "NTSC")
>
> I'm not sure I understand how that's any different than the code you
> initially commented on.
>
> I would start by taking the entire blanking area, remove the sync
> period. We only have the two porches now, and I'm starting from the
> minimum, adding as many pixels in both (unless it's not an even number,
> in which case the backporch will have the extra pixel).
>
> Isn't it the same thing?
> [...]
> Unless you only want me to consider the front porch maximum?
I think you're confusing the "post-equalizing pulses" with the "vertical back
porch" a little bit. The "vertical back porch" includes both the post-equalizing
pulses and the entire rest of the VBI period, for the standard resolutions at
least.
The "canonical" modelines (at least for vc4's VEC, see the notes below):
- (vfp==4, vsync==6, vbp==39) for 576i
- (vfp==7, vsync==6, vbp==32) for 480i
- (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
The numbers for vfp don't exactly match the theoretical values, because:
- VEC actually adds a half-line pulse on top of VFP_ODD, and in the 625-line
mode also on top of VFP_EVEN (not always, but let's not dwell too much)
- Conversely, VEC subtracts the half-line pulse from VSYNC_ODD and VSYNC_EVEN
in the 625-line mode
- SMPTE S170M (see https://www.itu.int/rec/R-REC-BT.1700-0-200502-I/en) defines
that active picture for NTSC is on lines 21-263 and 283-525; 263 and 283 are
half-lines that are represented as full lines in the "486i" spec
- SMPTE 314M, which is the spec for DV, defines the 480 active lines as lines
23-262 and 285-524; see Table 20 on page 26 in
https://last.hit.bme.hu/download/firtha/video/SMPTE/SMPTE-314M%20DV25-50.pdf;
this means that the standard 480i frame shaves off four topmost and two
bottommost lines (2 and 1 per field) of the 486i full frame
Note that the half-line pulses in vfp/vsync may be generated in a different way
on encoders other than vc4's VEC. Maybe we should define some concrete
semantics for vfp/vsync in analog TV modes, and compensate for that in the
drivers. But anyway, that's a separate issue.
My point is that, to get a centered image, you can then proportionately add
values to those "canonical" vfp/vbp values. For example if someone specifies
720x480 frame, but 50 Hz PAL, you should set (vfp==52, vsync==6, vbp==87).
Those extra vbp lines will be treated as a black bar at the top of the frame,
and extra vfp lines will be at the bottom of the frame.
However if someone specifies e.g. 720x604, there's nothing more you could
remove from vfp, so your only option is to reduce vbp compared to the standard
mode, so you'll end up with (vfp==4, vsync==6, vbp==11). The image will not be
centered, the topmost lines will get cropped out, but that's the best we can do
and if someone is requesting such resolution, they most likely want to actually
access the VBI to e.g. emit teletext.
Your current code always starts at (vfp==5 or 6, vsync=6, vbp==6) and then
increases both vfp and vbp proportionately. This puts vsync dead center in the
VBI, which is not how it's supposed to be - and that in turn causes the image
to be significantly shifted upwards.
I hope this makes more sense to you now.
Best regards,
Mateusz Kwiatkowski
Hi,
On Tue, Aug 30, 2022 at 10:01:11AM -0300, Ma?ra Canal wrote:
> On 8/29/22 10:11, Maxime Ripard wrote:
> > Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and
> > 625-lines modes in their drivers.
> >
> > Since those modes are fairly standard, and that we'll need to use them
> > in more places in the future, it makes sense to move their definition
> > into the core framework.
> >
> > However, analog display usually have fairly loose timings requirements,
> > the only discrete parameters being the total number of lines and pixel
> > clock frequency. Thus, we created a function that will create a display
> > mode from the standard, the pixel frequency and the active area.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> >
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index 304004fb80aa..ee581ee17171 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -116,6 +116,459 @@ void drm_mode_probed_add(struct drm_connector *connector,
> > }
> > EXPORT_SYMBOL(drm_mode_probed_add);
> >
> > +enum drm_mode_analog {
> > + DRM_MODE_ANALOG_NTSC,
> > + DRM_MODE_ANALOG_PAL,
> > +};
> > +
> > +/*
> > + * The timings come from:
> > + * - https://web.archive.org/web/20220406232708/http://www.kolumbus.fi/pami1/video/pal_ntsc.html
> > + * - https://web.archive.org/web/20220406124914/http://martin.hinner.info/vga/pal.html
> > + * - https://web.archive.org/web/20220609202433/http://www.batsocks.co.uk/readme/video_timing.htm
> > + */
> > +#define NTSC_LINE_DURATION_NS 63556U
> > +#define NTSC_LINES_NUMBER 525
> > +
> > +#define NTSC_HBLK_DURATION_TYP_NS 10900U
> > +#define NTSC_HBLK_DURATION_MIN_NS (NTSC_HBLK_DURATION_TYP_NS - 200)
> > +#define NTSC_HBLK_DURATION_MAX_NS (NTSC_HBLK_DURATION_TYP_NS + 200)
> > +
> > +#define NTSC_HACT_DURATION_TYP_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_TYP_NS)
> > +#define NTSC_HACT_DURATION_MIN_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MAX_NS)
> > +#define NTSC_HACT_DURATION_MAX_NS (NTSC_LINE_DURATION_NS - NTSC_HBLK_DURATION_MIN_NS)
> > +
> > +#define NTSC_HFP_DURATION_TYP_NS 1500
> > +#define NTSC_HFP_DURATION_MIN_NS 1270
> > +#define NTSC_HFP_DURATION_MAX_NS 2220
> > +
> > +#define NTSC_HSLEN_DURATION_TYP_NS 4700
> > +#define NTSC_HSLEN_DURATION_MIN_NS (NTSC_HSLEN_DURATION_TYP_NS - 100)
> > +#define NTSC_HSLEN_DURATION_MAX_NS (NTSC_HSLEN_DURATION_TYP_NS + 100)
> > +
> > +#define NTSC_HBP_DURATION_TYP_NS 4700
> > +
> > +/*
> > + * I couldn't find the actual tolerance for the back porch, so let's
> > + * just reuse the sync length ones.
> > + */
> > +#define NTSC_HBP_DURATION_MIN_NS (NTSC_HBP_DURATION_TYP_NS - 100)
> > +#define NTSC_HBP_DURATION_MAX_NS (NTSC_HBP_DURATION_TYP_NS + 100)
> > +
> > +#define PAL_LINE_DURATION_NS 64000U
> > +#define PAL_LINES_NUMBER 625
> > +
> > +#define PAL_HACT_DURATION_TYP_NS 51950U
> > +#define PAL_HACT_DURATION_MIN_NS (PAL_HACT_DURATION_TYP_NS - 100)
> > +#define PAL_HACT_DURATION_MAX_NS (PAL_HACT_DURATION_TYP_NS + 400)
> > +
> > +#define PAL_HBLK_DURATION_TYP_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_TYP_NS)
> > +#define PAL_HBLK_DURATION_MIN_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MAX_NS)
> > +#define PAL_HBLK_DURATION_MAX_NS (PAL_LINE_DURATION_NS - PAL_HACT_DURATION_MIN_NS)
> > +
> > +#define PAL_HFP_DURATION_TYP_NS 1650
> > +#define PAL_HFP_DURATION_MIN_NS (PAL_HFP_DURATION_TYP_NS - 100)
> > +#define PAL_HFP_DURATION_MAX_NS (PAL_HFP_DURATION_TYP_NS + 400)
> > +
> > +#define PAL_HSLEN_DURATION_TYP_NS 4700
> > +#define PAL_HSLEN_DURATION_MIN_NS (PAL_HSLEN_DURATION_TYP_NS - 200)
> > +#define PAL_HSLEN_DURATION_MAX_NS (PAL_HSLEN_DURATION_TYP_NS + 200)
> > +
> > +#define PAL_HBP_DURATION_TYP_NS 5700
> > +#define PAL_HBP_DURATION_MIN_NS (PAL_HBP_DURATION_TYP_NS - 200)
> > +#define PAL_HBP_DURATION_MAX_NS (PAL_HBP_DURATION_TYP_NS + 200)
> > +
> > +#define PAL_VFP_INTERLACE_LINES 5
> > +#define PAL_VSLEN_INTERLACE_LINES 5
> > +
> > +#define PAL_SHORT_SYNC_DURATION_NS ((2 + 30) * NSEC_PER_USEC)
> > +#define PAL_LONG_SYNC_DURATION_NS ((30 + 2) * NSEC_PER_USEC)
> > +
> > +struct analog_param_field {
> > + unsigned int even, odd;
> > +};
> > +
> > +#define PARAM_FIELD(_odd, _even) \
> > + { .even = _even, .odd = _odd }
> > +
> > +struct analog_param_range {
> > + unsigned int min, typ, max;
> > +};
> > +
> > +#define PARAM_RANGE(_min, _typ, _max) \
> > + { .min = _min, .typ = _typ, .max = _max }
> > +
> > +struct analog_parameters {
> > + unsigned int num_lines;
> > + unsigned int line_duration_ns;
> > +
> > + struct analog_param_range hact_ns;
> > + struct analog_param_range hfp_ns;
> > + struct analog_param_range hslen_ns;
> > + struct analog_param_range hbp_ns;
> > + struct analog_param_range hblk_ns;
> > +
> > + struct analog_param_field vfp_lines;
> > + struct analog_param_field vslen_lines;
> > + struct analog_param_field vbp_lines;
> > +};
> > +
> > +#define TV_MODE_PARAMETER(_mode, _lines, _line_dur, _hact, _hfp, _hslen, _hbp, _hblk, _vfp, _vslen, _vbp) \
> > + [_mode] = { \
> > + .num_lines = _lines, \
> > + .line_duration_ns = _line_dur, \
> > + .hact_ns = _hact, \
> > + .hfp_ns = _hfp, \
> > + .hslen_ns = _hslen, \
> > + .hbp_ns = _hbp, \
> > + .hblk_ns = _hblk, \
> > + .vfp_lines = _vfp, \
> > + .vslen_lines = _vslen, \
> > + .vbp_lines = _vbp, \
> > + }
> > +
> > +const static struct analog_parameters tv_modes_parameters[] = {
> > + TV_MODE_PARAMETER(DRM_MODE_ANALOG_NTSC,
> > + NTSC_LINES_NUMBER,
> > + NTSC_LINE_DURATION_NS,
> > + PARAM_RANGE(NTSC_HACT_DURATION_MIN_NS,
> > + NTSC_HACT_DURATION_TYP_NS,
> > + NTSC_HACT_DURATION_MAX_NS),
> > + PARAM_RANGE(NTSC_HFP_DURATION_MIN_NS,
> > + NTSC_HFP_DURATION_TYP_NS,
> > + NTSC_HFP_DURATION_MAX_NS),
> > + PARAM_RANGE(NTSC_HSLEN_DURATION_MIN_NS,
> > + NTSC_HSLEN_DURATION_TYP_NS,
> > + NTSC_HSLEN_DURATION_MAX_NS),
> > + PARAM_RANGE(NTSC_HBP_DURATION_MIN_NS,
> > + NTSC_HBP_DURATION_TYP_NS,
> > + NTSC_HBP_DURATION_MAX_NS),
> > + PARAM_RANGE(NTSC_HBLK_DURATION_MIN_NS,
> > + NTSC_HBLK_DURATION_TYP_NS,
> > + NTSC_HBLK_DURATION_MAX_NS),
> > + PARAM_FIELD(3, 3),
> > + PARAM_FIELD(3, 3),
> > + PARAM_FIELD(3, 3)),
> > + TV_MODE_PARAMETER(DRM_MODE_ANALOG_PAL,
> > + PAL_LINES_NUMBER,
> > + PAL_LINE_DURATION_NS,
> > + PARAM_RANGE(PAL_HACT_DURATION_MIN_NS,
> > + PAL_HACT_DURATION_TYP_NS,
> > + PAL_HACT_DURATION_MAX_NS),
> > + PARAM_RANGE(PAL_HFP_DURATION_MIN_NS,
> > + PAL_HFP_DURATION_TYP_NS,
> > + PAL_HFP_DURATION_MAX_NS),
> > + PARAM_RANGE(PAL_HSLEN_DURATION_MIN_NS,
> > + PAL_HSLEN_DURATION_TYP_NS,
> > + PAL_HSLEN_DURATION_MAX_NS),
> > + PARAM_RANGE(PAL_HBP_DURATION_MIN_NS,
> > + PAL_HBP_DURATION_TYP_NS,
> > + PAL_HBP_DURATION_MAX_NS),
> > + PARAM_RANGE(PAL_HBLK_DURATION_MIN_NS,
> > + PAL_HBLK_DURATION_TYP_NS,
> > + PAL_HBLK_DURATION_MAX_NS),
> > +
> > + /*
> > + * The front porch is actually 6 short sync
> > + * pulses for the even field, and 5 for the
> > + * odd field. Each sync takes half a life so
> > + * the odd field front porch is shorter by
> > + * half a line.
> > + *
> > + * In progressive, we're supposed to use 6
> > + * pulses, so we're fine there
> > + */
> > + PARAM_FIELD(3, 2),
> > +
> > + /*
> > + * The vsync length is 5 long sync pulses,
> > + * each field taking half a line. We're
> > + * shorter for both fields by half a line.
> > + *
> > + * In progressive, we're supposed to use 5
> > + * pulses, so we're off by half
> > + * a line.
> > + *
> > + * In interlace, we're now off by half a line
> > + * for the even field and one line for the odd
> > + * field.
> > + */
> > + PARAM_FIELD(3, 3),
> > +
> > + /*
> > + * The back porch is actually 5 short sync
> > + * pulses for the even field, 4 for the odd
> > + * field. In progressive, it's 5 short syncs.
> > + *
> > + * In progressive, we thus have 2.5 lines,
> > + * plus the 0.5 line we were missing
> > + * previously, so we should use 3 lines.
> > + *
> > + * In interlace, the even field is in the
> > + * exact same case than progressive. For the
> > + * odd field, we should be using 2 lines but
> > + * we're one line short, so we'll make up for
> > + * it here by using 3.
> > + */
> > + PARAM_FIELD(3, 3)),
> > +};
> > +
> > +static int fill_analog_mode(struct drm_display_mode *mode,
> > + const struct analog_parameters *params,
> > + unsigned long pixel_clock_hz,
> > + unsigned int hactive,
> > + unsigned int vactive,
> > + bool interlace)
> > +{
> > + unsigned long pixel_duration_ns = NSEC_PER_SEC / pixel_clock_hz;
> > + unsigned long long htotal;
> > + unsigned int vtotal;
> > + unsigned int max_hact, hact_duration_ns;
> > + unsigned int hblk, hblk_duration_ns;
> > + unsigned int hfp, hfp_min, hfp_duration_ns;
> > + unsigned int hslen, hslen_duration_ns;
> > + unsigned int hbp, hbp_min, hbp_duration_ns;
> > + unsigned int porches, porches_duration_ns;
> > + unsigned int vfp, vfp_min;
> > + unsigned int vbp, vbp_min;
> > + unsigned int vslen;
> > + int porches_rem;
> > + bool strict = true;
> > +
> > + max_hact = params->hact_ns.max / pixel_duration_ns;
> > + if (pixel_clock_hz == 13500000 && hactive > max_hact && hactive <= 720)
> > + strict = false;
> > +
> > + /*
> > + * Our pixel duration is going to be round down by the division,
> > + * so rounding up is probably going to introduce even more
> > + * deviation.
> > + */
> > + htotal = params->line_duration_ns * pixel_clock_hz / NSEC_PER_SEC;
> > +
> > + hact_duration_ns = hactive * pixel_duration_ns;
> > + if (strict &&
> > + (hact_duration_ns < params->hact_ns.min ||
> > + hact_duration_ns > params->hact_ns.max)) {
> > + DRM_ERROR("Invalid horizontal active area duration: %uns (min: %u, max %u)\n",
> > + hact_duration_ns, params->hact_ns.min, params->hact_ns.max);
> > + return -EINVAL;
> > + }
> > +
> > + hblk = htotal - hactive;
> > + hblk_duration_ns = hblk * pixel_duration_ns;
> > + if (strict &&
> > + (hblk_duration_ns < params->hblk_ns.min ||
> > + hblk_duration_ns > params->hblk_ns.max)) {
> > + DRM_ERROR("Invalid horizontal blanking duration: %uns (min: %u, max %u)\n",
> > + hblk_duration_ns, params->hblk_ns.min, params->hblk_ns.max);
> > + return -EINVAL;
> > + }
> > +
> > + hslen = DIV_ROUND_UP(params->hslen_ns.typ, pixel_duration_ns);
> > + hslen_duration_ns = hslen * pixel_duration_ns;
> > + if (strict &&
> > + (hslen_duration_ns < params->hslen_ns.min ||
> > + hslen_duration_ns > params->hslen_ns.max)) {
> > + DRM_ERROR("Invalid horizontal sync duration: %uns (min: %u, max %u)\n",
> > + hslen_duration_ns, params->hslen_ns.min, params->hslen_ns.max);
> > + return -EINVAL;
> > + }
> > +
> > + porches = hblk - hslen;
> > + porches_duration_ns = porches * pixel_duration_ns;
> > + if (strict &&
> > + (porches_duration_ns > (params->hfp_ns.max + params->hbp_ns.max) ||
> > + porches_duration_ns < (params->hfp_ns.min + params->hbp_ns.min))) {
> > + DRM_ERROR("Invalid horizontal porches duration: %uns\n", porches_duration_ns);
> > + return -EINVAL;
> > + }
> > +
> > + hfp_min = DIV_ROUND_UP(params->hfp_ns.min, pixel_duration_ns);
> > + hbp_min = DIV_ROUND_UP(params->hbp_ns.min, pixel_duration_ns);
> > + porches_rem = porches - hfp_min - hbp_min;
> > +
> > + hfp = hfp_min + DIV_ROUND_UP(porches_rem, 2);
> > + hfp_duration_ns = hfp * pixel_duration_ns;
> > + if (strict &&
> > + (hfp_duration_ns < params->hfp_ns.min ||
> > + hfp_duration_ns > params->hfp_ns.max)) {
> > + DRM_ERROR("Invalid horizontal front porch duration: %uns (min: %u, max %u)\n",
> > + hfp_duration_ns, params->hfp_ns.min, params->hfp_ns.max);
> > + return -EINVAL;
> > + }
> > +
> > + hbp = porches - hfp;
> > + hbp_duration_ns = hbp * pixel_duration_ns;
> > + if (strict &&
> > + (hbp_duration_ns < params->hbp_ns.min ||
> > + hbp_duration_ns > params->hbp_ns.max)) {
> > + DRM_ERROR("Invalid horizontal back porch duration: %uns (min: %u, max %u)\n",
> > + hbp_duration_ns, params->hbp_ns.min, params->hbp_ns.max);
> > + return -EINVAL;
> > + }
> > +
> > + if (htotal != (hactive + hfp + hslen + hbp))
> > + return -EINVAL;
> > +
> > + mode->clock = pixel_clock_hz / 1000;
> > + mode->hdisplay = hactive;
> > + mode->hsync_start = hactive + hfp;
> > + mode->hsync_end = hactive + hfp + hslen;
> > + mode->htotal = hactive + hfp + hslen + hbp;
> > +
> > + if (interlace) {
> > + vfp_min = params->vfp_lines.even + params->vfp_lines.odd;
> > + vbp_min = params->vbp_lines.even + params->vbp_lines.odd;
> > + vslen = params->vslen_lines.even + params->vslen_lines.odd;
> > + } else {
> > + /*
> > + * By convention, NSTC (aka 525/60) systems start with
> > + * the even field, but PAL (aka 625/50) systems start
> > + * with the odd one.
> > + *
> > + * PAL systems also have asymetric timings between the
> > + * even and odd field, while NTSC is symetric.
> > + *
> > + * Moreover, if we want to create a progressive mode for
> > + * PAL, we need to use the odd field timings.
> > + *
> > + * Since odd == even for NTSC, we can just use the odd
> > + * one all the time to simplify the code a bit.
> > + */
> > + vfp_min = params->vfp_lines.odd;
> > + vbp_min = params->vbp_lines.odd;
> > + vslen = params->vslen_lines.odd;
> > + }
> > +
> > + porches = params->num_lines - vactive - vslen;
> > + porches_rem = porches - vfp_min - vbp_min;
> > +
> > + vfp = vfp_min + (porches_rem / 2);
> > + vbp = porches - vfp;
> > +
> > + vtotal = vactive + vfp + vslen + vbp;
> > + if (params->num_lines != vtotal) {
> > + DRM_ERROR("Invalid vertical total: %upx (expected %upx)\n",
> > + vtotal, params->num_lines);
> > + return -EINVAL;
> > + }
> > +
> > + mode->vdisplay = vactive;
> > + mode->vsync_start = vactive + vfp;
> > + mode->vsync_end = vactive + vfp + vslen;
> > + mode->vtotal = vactive + vfp + vslen + vbp;
> > +
> > + mode->type = DRM_MODE_TYPE_DRIVER;
> > + mode->flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC;
> > + if (interlace)
> > + mode->flags |= DRM_MODE_FLAG_INTERLACE;
> > +
> > + drm_mode_set_name(mode);
> > +
> > + if (mode->vtotal != params->num_lines)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * drm_analog_tv_mode - create a display mode for an analog TV
> > + * @dev: drm device
> > + * @tv_mode: TV Mode standard to create a mode for. See DRM_MODE_TV_MODE_*.
> > + * @pixel_clock_hz: Pixel Clock Frequency, in Hertz
> > + * @hdisplay: hdisplay size
> > + * @vdisplay: vdisplay size
> > + * @interlace: whether to compute an interlaced mode
> > + *
> > + * This function creates a struct drm_display_mode instance suited for
> > + * an analog TV output, for one of the usual analog TV mode.
> > + *
> > + * Note that @hdisplay is larger than the usual constraints for the PAL
> > + * and NTSC timings, and we'll choose to ignore most timings constraints
> > + * to reach those resolutions.
> > + *
> > + * Returns:
> > + *
> > + * A pointer to the mode, allocated with drm_mode_create(). Returns NULL
> > + * on error.
> > + */
> > +struct drm_display_mode *drm_analog_tv_mode(struct drm_device *dev,
> > + enum drm_connector_tv_mode tv_mode,
> > + unsigned long pixel_clock_hz,
> > + unsigned int hdisplay,
> > + unsigned int vdisplay,
> > + bool interlace)
> > +{
> > + struct drm_display_mode *mode;
> > + enum drm_mode_analog analog;
> > + int ret;
> > +
> > + switch (tv_mode) {
> > + case DRM_MODE_TV_MODE_NTSC_443:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_NTSC_J:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_NTSC_M:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_60:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_M:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_60:
> > + analog = DRM_MODE_ANALOG_NTSC;
> > + break;
> > +
> > + case DRM_MODE_TV_MODE_PAL_B:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_D:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_G:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_H:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_I:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_N:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_PAL_NC:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_B:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_D:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_G:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_K:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_K1:
> > + fallthrough;
> > + case DRM_MODE_TV_MODE_SECAM_L:
> > + analog = DRM_MODE_ANALOG_PAL;
> > + break;
> > +
> > + default:
> > + return NULL;
> > + }
> > +
> > + mode = drm_mode_create(dev);
> > + if (!mode)
> > + return NULL;
> > +
> > + ret = fill_analog_mode(mode,
> > + &tv_modes_parameters[analog],
> > + pixel_clock_hz, hdisplay, vdisplay, interlace);
> > + if (ret)
> > + goto err_free_mode;
> > +
> > + return mode;
> > +
> > +err_free_mode:
> > + drm_mode_destroy(dev, mode);
> > + return NULL;
> > +}
> > +EXPORT_SYMBOL(drm_analog_tv_mode);
> > +
> > /**
> > * drm_cvt_mode -create a modeline based on the CVT algorithm
> > * @dev: drm device
> > diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> > index b29ef1085cad..b22ac96fdd65 100644
> > --- a/drivers/gpu/drm/tests/Makefile
> > +++ b/drivers/gpu/drm/tests/Makefile
> > @@ -10,5 +10,6 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
> > drm_framebuffer_test.o \
> > drm_kunit_helpers.o \
> > drm_mm_test.o \
> > + drm_modes_test.o \
> > drm_plane_helper_test.o \
> > drm_rect_test.o
> > diff --git a/drivers/gpu/drm/tests/drm_modes_test.c b/drivers/gpu/drm/tests/drm_modes_test.c
> > new file mode 100644
> > index 000000000000..87d398fcb99e
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tests/drm_modes_test.c
> > @@ -0,0 +1,131 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Kunit test for drm_modes functions
> > + */
> > +
> > +#include <kunit/test.h>
> > +
> > +#include <drm/drm_modes.h>
> > +
> > +#include "drm_kunit_helpers.h"
> > +
> > +struct drm_modes_test_priv {
> > + struct drm_device *drm;
> > +};
> > +
> > +static int drm_modes_test_init(struct kunit *test)
> > +{
> > + struct drm_modes_test_priv *priv;
> > +
> > + priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
>
> I believe it would be nicer to use KUNIT_ASSERT_NOT_NULL here, instead
> of returning a error.
>
> > + test->priv = priv;
> > +
> > + priv->drm = drm_kunit_device_init("drm-modes-test");
> > + if (IS_ERR(priv->drm))
> > + return PTR_ERR(priv->drm);
>
> Here you could use KUNIT_ASSERT_NOT_ERR_OR_NULL.
>
> > +
> > + return 0;
> > +}
> > +
> > +static void drm_modes_test_exit(struct kunit *test)
> > +{
> > + struct drm_modes_test_priv *priv = test->priv;
> > +
> > + drm_kunit_device_exit(priv->drm);
> > +}
> > +
> > +static void drm_modes_analog_tv_ntsc_480i(struct kunit *test)
> > +{
> > + struct drm_modes_test_priv *priv = test->priv;
> > + struct drm_display_mode *mode;
> > +
> > + mode = drm_analog_tv_mode(priv->drm,
> > + DRM_MODE_TV_MODE_NTSC_M,
> > + 13500 * 1000, 720, 480,
> > + true);
> > + KUNIT_ASSERT_NOT_NULL(test, mode);
> > +
> > + KUNIT_EXPECT_EQ(test, drm_mode_vrefresh(mode), 60);
> > + KUNIT_EXPECT_EQ(test, mode->hdisplay, 720);
> > +
> > + /* 63.556us * 13.5MHz = 858 pixels */
> > + KUNIT_EXPECT_EQ(test, mode->htotal, 858);
> > + KUNIT_EXPECT_EQ(test, mode->vdisplay, 480);
> > + KUNIT_EXPECT_EQ(test, mode->vtotal, 525);
> > +}
>
> I would be nice to see this test and drm_modes_analog_tv_pal_576i
> parametrized.
I'm not sure it's that useful here, the tests are trivial and there's
not much to share between the two tests, since pretty much all the data
are different.
I've changed the command line parsing to use a parameterized test though
Thanks!
Maxime
On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
> The "canonical" modelines (at least for vc4's VEC, see the notes below):
>
> - (vfp==4, vsync==6, vbp==39) for 576i
> - (vfp==7, vsync==6, vbp==32) for 480i
> - (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
It's not clear to me either how you come up with those timings?
Maxime
Hi,
On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
> W dniu 7.09.2022 o 16:34, Maxime Ripard pisze:
> > On Mon, Sep 05, 2022 at 06:44:42PM +0200, Mateusz Kwiatkowski wrote:
> >> Hi Maxime,
> >>
> >> W dniu 5.09.2022 o 15:37, Maxime Ripard pisze:
> >>>>> +??? 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.
> >>>
> >>> And you would increase the back porch only then?
> >>
> >> Well, increasing vbp only gives a centered image with the default 480i/576i
> >> resolutions. However, only ever changing vbp will cause the image to be always
> >> at the bottom of the screen when the active line count is decreased (e.g.
> >> setting the resolution to 720x480 but for 50Hz "PAL" - like many game consoles
> >> did back in the day).
> >>
> >> I believe that the perfect solution would:
> >>
> >> - Use the canonical / standard-defined blanking line counts for the standard
> >>?? vertical resolutions (480/486/576)
> >> - Increase vfp and vbp from there by the same number if a smaller number of
> >>?? active lines is specified, so that the resulting image is centered
> >> - Likewise, decrease vfp and vbp by the same number if the active line number
> >>?? is larger and there is still leeway (this should allow for seamless handling
> >>?? of 480i vs. 486i for 60 Hz "NTSC")
> >
> > I'm not sure I understand how that's any different than the code you
> > initially commented on.
> >
> > I would start by taking the entire blanking area, remove the sync
> > period. We only have the two porches now, and I'm starting from the
> > minimum, adding as many pixels in both (unless it's not an even number,
> > in which case the backporch will have the extra pixel).
> >
> > Isn't it the same thing?
> > [...]
> > Unless you only want me to consider the front porch maximum?
>
> I think you're confusing the "post-equalizing pulses" with the "vertical back
> porch" a little bit. The "vertical back porch" includes both the post-equalizing
> pulses and the entire rest of the VBI period, for the standard resolutions at
> least.
>
> The "canonical" modelines (at least for vc4's VEC, see the notes below):
>
> - (vfp==4, vsync==6, vbp==39) for 576i
> - (vfp==7, vsync==6, vbp==32) for 480i
> - (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
>
> The numbers for vfp don't exactly match the theoretical values, because:
>
> - VEC actually adds a half-line pulse on top of VFP_ODD, and in the 625-line
> ? mode also on top of VFP_EVEN (not always, but let's not dwell too much)
> - Conversely, VEC subtracts the half-line pulse from VSYNC_ODD and VSYNC_EVEN
> ? in the 625-line mode
> - SMPTE S170M (see https://www.itu.int/rec/R-REC-BT.1700-0-200502-I/en) defines
> ? that active picture for NTSC is on lines 21-263 and 283-525; 263 and 283 are
> ? half-lines that are represented as full lines in the "486i" spec
It's going to be a bit difficult to match that into a drm_display_mode,
since as far I understand it, all the timings are the sum of the timings
of both fields in interlaced. I guess we'll have to be close enough.
> - SMPTE 314M, which is the spec for DV, defines the 480 active lines as lines
> ? 23-262 and 285-524; see Table 20 on page 26 in
> ? https://last.hit.bme.hu/download/firtha/video/SMPTE/SMPTE-314M%20DV25-50.pdf;
> ? this means that the standard 480i frame shaves off four topmost and two
> ? bottommost lines (2 and 1 per field) of the 486i full frame
I'm still struggling a bit to match that into front porch, sync period
and back porch. I guess the sync period is easy since it's pretty much
fixed. That line 0-23 is the entire blanking area though, right?
> Note that the half-line pulses in vfp/vsync may be generated in a different way
> on encoders other than vc4's VEC. Maybe we should define some concrete
> semantics for vfp/vsync in analog TV modes, and compensate for that in the
> drivers. But anyway, that's a separate issue.
>
> My point is that, to get a centered image, you can then proportionately add
> values to those "canonical" vfp/vbp values. For example if someone specifies
> 720x480 frame, but 50 Hz PAL, you should set (vfp==52, vsync==6, vbp==87).
In this case, you add 48 both front porches, right? How is that
proportionate?
> Those extra vbp lines will be treated as a black bar at the top of the frame,
> and extra vfp lines will be at the bottom of the frame.
>
> However if someone specifies e.g. 720x604, there's nothing more you could
> remove from vfp, so your only option is to reduce vbp compared to the standard
> mode, so you'll end up with (vfp==4, vsync==6, vbp==11). The image will not be
> centered, the topmost lines will get cropped out, but that's the best we can do
> and if someone is requesting such resolution, they most likely want to actually
> access the VBI to e.g. emit teletext.
>
> Your current code always starts at (vfp==5 or 6, vsync=6, vbp==6) and then
> increases both vfp and vbp proportionately. This puts vsync dead center in the
> VBI, which is not how it's supposed to be - and that in turn causes the image
> to be significantly shifted upwards.
>
> I hope this makes more sense to you now.
I'm really struggling with this, so thanks for explaining this further
(and patiently ;))
If I get this right, what you'd like to change is this part of the
calculus (simplified a bit, and using PAL, 576i):
vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 6
vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
porches = params->num_lines - vactive - vslen; // 43
porches_rem = porches - vfp_min - vbp_min; // 32
vfp = vfp_min + (porches_rem / 2); // 21
vbp = porches - vfp; // 22
Which is indeed having sync centered.
I initially changed it to:
vfp = vfp_min; // 6
vbp = num_lines - vactive - vslen - vfp; // 38
Which is close enough for 576i, but at 480i/50Hz would end up with 134,
so still fairly far off.
I guess your suggestion would be along the line of:
vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 38
vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
porches = params->num_lines - vactive - vslen; // 0
porches_rem = porches - vfp_min - vbp_min; // 0
vfp = vfp_min + (porches_rem / 2); // 5
vbp = porches - vfp; // 38
Which is still close enough for 576i, but for 480i would end up with:
porches = params->num_lines - vactive - vslen; // 139
porches_rem = porches - vfp_min - vbp_min; // 96
vfp = vfp_min + (porches_rem / 2); // 53
vbp = porches - vfp; // 86
Right?
Maxime
Hi Maxime,
W dniu 9.09.2022 o 16:00, Maxime Ripard pisze:
> On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
>> The "canonical" modelines (at least for vc4's VEC, see the notes below):
>>
>> - (vfp==4, vsync==6, vbp==39) for 576i
>> - (vfp==7, vsync==6, vbp==32) for 480i
>> - (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
>
> It's not clear to me either how you come up with those timings?
Well, experimentally ;)
The values for 480i and 576i are the values currently used by the downstream
kernel, and those in turn has been copied from the firmware (or more precisely,
I chose them so that the PV registers end up the same as the values set by the
firmware).
I also checked with an oscilloscope that the waveforms look as they should.
VEC doesn't exactly handle the half-lines at the start and end of the odd field
right, but otherwise, the blanking and sync pulses are where they should be.
The 486i values has been constructed from the 480i ones according to the
calculations from cross-referencing SMPTE documents, see my previous e-mail.
I know this is perhaps unsatisfactory ;) I don't have access to the VC4
documentation, so this was _almost_ reverse engineering for me.
Best regards,
Mateusz Kwiatkowski
> Yes, lines 0-23 is the entire blanking area. And the "back porch" in this
> context is everything from the start of the sync pulse to the start of active
> video. It's not just the equalizing pulses.
I meant "from the end of the sync pulse", obviously. Sorry about the slipup.
Hi Maxime,
W dniu 9.09.2022 o 15:54, Maxime Ripard pisze:
> Hi,
>
> On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
> [...]
>> I think you're confusing the "post-equalizing pulses" with the "vertical back
>> porch" a little bit. The "vertical back porch" includes both the post-equalizing
>> pulses and the entire rest of the VBI period, for the standard resolutions at
>> least.
>>
>> The "canonical" modelines (at least for vc4's VEC, see the notes below):
>>
>> - (vfp==4, vsync==6, vbp==39) for 576i
>> - (vfp==7, vsync==6, vbp==32) for 480i
>> - (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
>>
>> The numbers for vfp don't exactly match the theoretical values, because:
>>
>> - VEC actually adds a half-line pulse on top of VFP_ODD, and in the 625-line
>> mode also on top of VFP_EVEN (not always, but let's not dwell too much)
>> - Conversely, VEC subtracts the half-line pulse from VSYNC_ODD and VSYNC_EVEN
>> in the 625-line mode
>> - SMPTE S170M (see https://www.itu.int/rec/R-REC-BT.1700-0-200502-I/en) defines
>> that active picture for NTSC is on lines 21-263 and 283-525; 263 and 283 are
>> half-lines that are represented as full lines in the "486i" spec
>
> It's going to be a bit difficult to match that into a drm_display_mode,
> since as far I understand it, all the timings are the sum of the timings
> of both fields in interlaced. I guess we'll have to be close enough.
Well, it's probably the job of the CRTC driver to split the values from
drm_display_mode back into separate values for odd and even fields. That's how
it's done in the vc4 driver, anyway.
>
>> - SMPTE 314M, which is the spec for DV, defines the 480 active lines as lines
>> 23-262 and 285-524; see Table 20 on page 26 in
>> https://last.hit.bme.hu/download/firtha/video/SMPTE/SMPTE-314M%20DV25-50.pdf;
>> this means that the standard 480i frame shaves off four topmost and two
>> bottommost lines (2 and 1 per field) of the 486i full frame
>
> I'm still struggling a bit to match that into front porch, sync period
> and back porch. I guess the sync period is easy since it's pretty much
> fixed. That line 0-23 is the entire blanking area though, right?
Yes, lines 0-23 is the entire blanking area. And the "back porch" in this
context is everything from the start of the sync pulse to the start of active
video. It's not just the equalizing pulses.
The equalizing pulses have no equivalent in DRM terms. VC4/VEC inserts those
automatically and there's no direct control over them, I'm not sure about other
encoders.
The equalizing pulses are also not essential for the composite video to work.
The spec requires them, but most TVs will tolerate them not being there (and
early systems like the British 405-line system didn't have any).
>> Note that the half-line pulses in vfp/vsync may be generated in a different way
>> on encoders other than vc4's VEC. Maybe we should define some concrete
>> semantics for vfp/vsync in analog TV modes, and compensate for that in the
>> drivers. But anyway, that's a separate issue.
>>
>> My point is that, to get a centered image, you can then proportionately add
>> values to those "canonical" vfp/vbp values. For example if someone specifies
>> 720x480 frame, but 50 Hz PAL, you should set (vfp==52, vsync==6, vbp==87).
>
> In this case, you add 48 both front porches, right? How is that
> proportionate?
Yes, I meant adding 48 lines to both porches, and I meant "proportionately" as
"split equally". Maybe that was an unfortunate choice of words.
>> Those extra vbp lines will be treated as a black bar at the top of the frame,
>> and extra vfp lines will be at the bottom of the frame.
>>
>> However if someone specifies e.g. 720x604, there's nothing more you could
>> remove from vfp, so your only option is to reduce vbp compared to the standard
>> mode, so you'll end up with (vfp==4, vsync==6, vbp==11). The image will not be
>> centered, the topmost lines will get cropped out, but that's the best we can do
>> and if someone is requesting such resolution, they most likely want to actually
>> access the VBI to e.g. emit teletext.
>>
>> Your current code always starts at (vfp==5 or 6, vsync=6, vbp==6) and then
>> increases both vfp and vbp proportionately. This puts vsync dead center in the
>> VBI, which is not how it's supposed to be - and that in turn causes the image
>> to be significantly shifted upwards.
>>
>> I hope this makes more sense to you now.
>
> I'm really struggling with this, so thanks for explaining this further
> (and patiently ;))
>
> If I get this right, what you'd like to change is this part of the
> calculus (simplified a bit, and using PAL, 576i):
>
> vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
> vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 6
> vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
>
> porches = params->num_lines - vactive - vslen; // 43
> porches_rem = porches - vfp_min - vbp_min; // 32
>
> vfp = vfp_min + (porches_rem / 2); // 21
> vbp = porches - vfp; // 22
>
> Which is indeed having sync centered.
>
> I initially changed it to:
>
> vfp = vfp_min; // 6
> vbp = num_lines - vactive - vslen - vfp; // 38
>
> Which is close enough for 576i, but at 480i/50Hz would end up with 134,
> so still fairly far off.
>
> I guess your suggestion would be along the line of:
>
> vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
> vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 38
> vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
>
> porches = params->num_lines - vactive - vslen; // 0
> porches_rem = porches - vfp_min - vbp_min; // 0
>
> vfp = vfp_min + (porches_rem / 2); // 5
> vbp = porches - vfp; // 38
>
> Which is still close enough for 576i, but for 480i would end up with:
>
> porches = params->num_lines - vactive - vslen; // 139
> porches_rem = porches - vfp_min - vbp_min; // 96
>
> vfp = vfp_min + (porches_rem / 2); // 53
> vbp = porches - vfp; // 86
>
> Right?
Yes. And if that's supposed to mean 480i in 50 Hz "PAL" mode, that's also
"close enough" to the values I suggested above.
If you substitute values for true 60 Hz "NTSC" 480i, you should also get values
that are "close enough" to the official spec.
The only thing I'd conceptually change is that the 38 lines is not really
"vbp_min". It's more like "vbp_typ". As I mentioned above, we may want to lower
this value if someone wants more active lines than the official 486/576.
If they're doing that, they probably want to have vbp in the framebuffer,
like VBIT2.
Best regards,
Mateusz Kwiatkowski
Hi,
Thanks again for your help
On Sun, Sep 11, 2022 at 06:30:39AM +0200, kFYatek wrote:
> W dniu 9.09.2022 o 16:00, Maxime Ripard pisze:
> > On Wed, Sep 07, 2022 at 11:31:21PM +0200, Mateusz Kwiatkowski wrote:
> >> The "canonical" modelines (at least for vc4's VEC, see the notes below):
> >>
> >> - (vfp==4, vsync==6, vbp==39) for 576i
> >> - (vfp==7, vsync==6, vbp==32) for 480i
> >> - (vfp==5, vsync==6, vbp==28) for 486i (full frame NTSC as originally specified)
> >
> > It's not clear to me either how you come up with those timings?
>
> Well, experimentally ;)
>
> The values for 480i and 576i are the values currently used by the downstream
> kernel, and those in turn has been copied from the firmware (or more precisely,
> I chose them so that the PV registers end up the same as the values set by the
> firmware).
>
> I also checked with an oscilloscope that the waveforms look as they should.
> VEC doesn't exactly handle the half-lines at the start and end of the odd field
> right, but otherwise, the blanking and sync pulses are where they should be.
>
> The 486i values has been constructed from the 480i ones according to the
> calculations from cross-referencing SMPTE documents, see my previous e-mail.
>
> I know this is perhaps unsatisfactory ;) I don't have access to the VC4
> documentation, so this was _almost_ reverse engineering for me.
It's not really that it's unsatisfactory, but the function here is
supposed to be generic and thus generate a mode that is supposed to be a
somewhat reasonable for a given set of parameters.
If the vc4 driver needs some adjustments, then it needs to be out of
that function and into the vc4 driver. And this is pretty much what I
struggle with: I have a hard time (on top of everything else) figuring
out what is supposed to be specific to vc4, and what isn't.
I guess your 480i example, since it follows the spec, is fine, but I'm
not sure for 576i.
Maxime
Hi,
On Sun, Sep 11, 2022 at 06:48:50AM +0200, Mateusz Kwiatkowski wrote:
> >> Those extra vbp lines will be treated as a black bar at the top of the frame,
> >> and extra vfp lines will be at the bottom of the frame.
> >>
> >> However if someone specifies e.g. 720x604, there's nothing more you could
> >> remove from vfp, so your only option is to reduce vbp compared to the standard
> >> mode, so you'll end up with (vfp==4, vsync==6, vbp==11). The image will not be
> >> centered, the topmost lines will get cropped out, but that's the best we can do
> >> and if someone is requesting such resolution, they most likely want to actually
> >> access the VBI to e.g. emit teletext.
> >>
> >> Your current code always starts at (vfp==5 or 6, vsync=6, vbp==6) and then
> >> increases both vfp and vbp proportionately. This puts vsync dead center in the
> >> VBI, which is not how it's supposed to be - and that in turn causes the image
> >> to be significantly shifted upwards.
> >>
> >> I hope this makes more sense to you now.
> >
> > I'm really struggling with this, so thanks for explaining this further
> > (and patiently ;))
> >
> > If I get this right, what you'd like to change is this part of the
> > calculus (simplified a bit, and using PAL, 576i):
> >
> >?? vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
> >?? vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 6
> >?? vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
> >
> >?? porches = params->num_lines - vactive - vslen; // 43
> >?? porches_rem = porches - vfp_min - vbp_min; // 32
> >
> >?? vfp = vfp_min + (porches_rem / 2); // 21
> >?? vbp = porches - vfp; // 22
> >
> > Which is indeed having sync centered.
> >
> > I initially changed it to:
> >
> >?? vfp = vfp_min; // 6
> >?? vbp = num_lines - vactive - vslen - vfp; // 38
> >
> > Which is close enough for 576i, but at 480i/50Hz would end up with 134,
> > so still fairly far off.
> >
> > I guess your suggestion would be along the line of:
> >
> >?? vfp_min = params->vfp_lines.even + params->vfp_lines.odd; // 5
> >?? vbp_min = params->vbp_lines.even + params->vbp_lines.odd; // 38
> >?? vslen = params->vslen_lines.even + params->vslen_lines.odd; // 6
> >
> >?? porches = params->num_lines - vactive - vslen; // 0
> >?? porches_rem = porches - vfp_min - vbp_min; // 0
> >
> >?? vfp = vfp_min + (porches_rem / 2); // 5
> >?? vbp = porches - vfp; // 38
> >
> > Which is still close enough for 576i, but for 480i would end up with:
> >
> >?? porches = params->num_lines - vactive - vslen; // 139
> >?? porches_rem = porches - vfp_min - vbp_min; // 96
> >
> >?? vfp = vfp_min + (porches_rem / 2); // 53
> >?? vbp = porches - vfp; // 86
> >
> > Right?
>
> Yes. And if that's supposed to mean 480i in 50 Hz "PAL" mode, that's also
> "close enough" to the values I suggested above.
>
> If you substitute values for true 60 Hz "NTSC" 480i, you should also get values
> that are "close enough" to the official spec.
>
> The only thing I'd conceptually change is that the 38 lines is not really
> "vbp_min". It's more like "vbp_typ". As I mentioned above, we may want to lower
> this value if someone wants more active lines than the official 486/576.
porches_rem is an int, so if vactive > (num_lines + vslen + vfp_min +
vbp_min), porches_rem is going to be negative and we'll remove equally
between vfp and vbp to match what's been asked
So I believe this should work fine?
Maxime