Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
On Fri, Sep 02, 2022 at 12:46:29AM +0200, Mateusz Kwiatkowski wrote:
> > @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> > unsigned int xres;
> > unsigned int yres;
> > unsigned int flags;
> > + unsigned int tv_mode;
> > };
>
> Are _all_ named modes supposed to be about analog TV?
>
> If so, then probably this structure should be renamed drm_named_analog_tv_mode
> or something.
I don't think they need to, but it's the only use case we've had so far.
We could also imagine using UHD for 3840x2160 for example, so I wouldn't
say it's limited to analog tv.
> If not, then including tv_mode in all of them sounds almost dangrous. 0 is a
> valid value for enum drm_connector_tv_mode, corresponding to
> DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be
> the one that has a numeric value of 0?) and if there ever is a named mode that
> is not related to analog TV, it looks that it will refer to NTSC-443.
>
> Not sure where could that actually propagate, and maybe what I'm saying can't
> happen, but I'm imagining weird scenarios where a GPU that has both a
> VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the
> composite output by default because a named mode for the modern output is
> selected.
So, named modes are per-connector so the fact that there's another
output doesn't really matter. Then, the answer is quite simple actually,
the HDMI driver wouldn't register and use the TV mode property at all,
so it would completely ignore it, no matter what value it has.
So it's not really a concern.
> Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense?
But I guess we can add it still.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
Hi Maxime,
> @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> unsigned int xres;
> unsigned int yres;
> unsigned int flags;
> + unsigned int tv_mode;
> };
Are _all_ named modes supposed to be about analog TV?
If so, then probably this structure should be renamed drm_named_analog_tv_mode
or something.
If not, then including tv_mode in all of them sounds almost dangrous. 0 is a
valid value for enum drm_connector_tv_mode, corresponding to
DRM_MODE_TV_MODE_NTSC_443. This is a very weird default (maybe it shouldn't be
the one that has a numeric value of 0?) and if there ever is a named mode that
is not related to analog TV, it looks that it will refer to NTSC-443.
Not sure where could that actually propagate, and maybe what I'm saying can't
happen, but I'm imagining weird scenarios where a GPU that has both a
VGA/HDMI/whatever output, and a composite output, switches to NTSC-443 on the
composite output by default because a named mode for the modern output is
selected.
Maybe something like DRM_MODE_TV_MODE_NONE = 0 would make sense?
Maybe not. This is not an actual suggestion, just "thinking out loud".
Best regards,
Mateusz Kwiatkowski
Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
On 8/29/22 10:11, Maxime Ripard wrote:
> Our new tv mode option allows to specify the TV mode from a property.
> However, it can still be useful, for example to avoid any boot time
> artifact, to set that property directly from the kernel command line.
>
> Let's add some code to allow it, and some unit tests to exercise that code.
>
> Signed-off-by: Maxime Ripard
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 73d01e755496..a759a4ba0036 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char
> *delim,
> return 0;
> }
>
> +static int drm_mode_parse_tv_mode(const char *delim,
> + struct drm_cmdline_mode *mode)
> +{
> + const char *value;
> + unsigned int len;
> + int ret;
> +
> + if (*delim != '=')
> + return -EINVAL;
> +
> + value = delim + 1;
> + delim = strchr(value, ',');
> + if (!delim)
> + delim = value + strlen(value);
> +
> + ret = drm_get_tv_mode_from_name(value, delim - value);
> + if (ret < 0)
> + return ret;
> +
> + mode->tv_mode = ret;
> +
> + return 0;
> +}
> +
> static int drm_mode_parse_cmdline_options(const char *str,
> bool freestanding,
> const struct drm_connector *connector,
> @@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char
> *str,
> } else if (!strncmp(option, "panel_orientation", delim -
> option)) {
> if (drm_mode_parse_panel_orientation(delim, mode))
> return -EINVAL;
> + } else if (!strncmp(option, "tv_mode", delim - option)) {
> + if (drm_mode_parse_tv_mode(delim, mode))
> + return -EINVAL;
> } else {
> return -EINVAL;
> }
> @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> unsigned int xres;
> unsigned int yres;
> unsigned int flags;
> + unsigned int tv_mode;
> };
>
> -#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \
> +#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode) \
> { \
> .name = _name, \
> .pixel_clock_khz = _pclk, \
> .xres = _x, \
> .yres = _y, \
> .flags = _flags,\
> + .tv_mode = _mode, \
> }
>
> static const struct drm_named_mode drm_named_modes[] = {
> - NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE),
> - NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE),
> + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC_M),
> + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL_B),
> };
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> @@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char
> *name,
> cmdline_mode->xres = mode->xres;
> cmdline_mode->yres = mode->yres;
> cmdline_mode->interlace = !!(mode->flags &
> DRM_MODE_FLAG_INTERLACE);
> + cmdline_mode->tv_mode = mode->tv_mode;
> cmdline_mode->specified = true;
>
> return 1;
> diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> index 59b29cdfdd35..f1e73ed65be0 100644
> --- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> +++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> @@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct
> kunit *test)
> KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED);
> }
>
> +static void drm_cmdline_test_tv_options(struct kunit *test,
> + const char *cmdline,
> + const struct drm_display_mode
> *expected_mode,
> + unsigned int expected_tv_mode)
> +{
> + struct drm_cmdline_mode mode = { };
> +
> + KUNIT_EXPECT_TRUE(test,
> drm_mode_parse_command_line_for_connector(cmdline,
> +
> &no_connector, &mode));
> + KUNIT_EXPECT_TRUE(test, mode.specified);
> + KUNIT_EXPECT_EQ(test, mode.xres, expected_mode->hdisplay);
> + KUNIT_EXPECT_EQ(test, mode.yres, expected_mode->vdisplay);
> + KUNIT_EXPECT_EQ(test, mode.tv_mode, expected_tv_mode);
> +
> + KUNIT_EXPECT_FALSE(test, mode.refresh_specified);
> +
> + KUNIT_EXPECT_FALSE(test, mode.bpp_specified);
> +
> + KUNIT_EXPECT_FALSE(test, mode.rb);
> + KU
Re: [PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
Hi Maxime,
On 8/29/22 10:11, Maxime Ripard wrote:
> Our new tv mode option allows to specify the TV mode from a property.
> However, it can still be useful, for example to avoid any boot time
> artifact, to set that property directly from the kernel command line.
>
> Let's add some code to allow it, and some unit tests to exercise that code.
>
> Signed-off-by: Maxime Ripard
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 73d01e755496..a759a4ba0036 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char
> *delim,
> return 0;
> }
>
> +static int drm_mode_parse_tv_mode(const char *delim,
> + struct drm_cmdline_mode *mode)
> +{
> + const char *value;
> + unsigned int len;
Looks like this variable len is not being used and is producing the
following warning:
../drivers/gpu/drm/drm_modes.c:2122:15: warning: unused variable 'len'
[-Wunused-variable]
unsigned int len;
^
Best Regards,
- Maíra Canal
> + int ret;
> +
> + if (*delim != '=')
> + return -EINVAL;
> +
> + value = delim + 1;
> + delim = strchr(value, ',');
> + if (!delim)
> + delim = value + strlen(value);
> +
> + ret = drm_get_tv_mode_from_name(value, delim - value);
> + if (ret < 0)
> + return ret;
> +
> + mode->tv_mode = ret;
> +
> + return 0;
> +}
> +
> static int drm_mode_parse_cmdline_options(const char *str,
> bool freestanding,
> const struct drm_connector *connector,
> @@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char
> *str,
> } else if (!strncmp(option, "panel_orientation", delim -
> option)) {
> if (drm_mode_parse_panel_orientation(delim, mode))
> return -EINVAL;
> + } else if (!strncmp(option, "tv_mode", delim - option)) {
> + if (drm_mode_parse_tv_mode(delim, mode))
> + return -EINVAL;
> } else {
> return -EINVAL;
> }
> @@ -2212,20 +2239,22 @@ struct drm_named_mode {
> unsigned int xres;
> unsigned int yres;
> unsigned int flags;
> + unsigned int tv_mode;
> };
>
> -#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \
> +#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode) \
> { \
> .name = _name, \
> .pixel_clock_khz = _pclk, \
> .xres = _x, \
> .yres = _y, \
> .flags = _flags,\
> + .tv_mode = _mode, \
> }
>
> static const struct drm_named_mode drm_named_modes[] = {
> - NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE),
> - NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE),
> + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC_M),
> + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL_B),
> };
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> @@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char
> *name,
> cmdline_mode->xres = mode->xres;
> cmdline_mode->yres = mode->yres;
> cmdline_mode->interlace = !!(mode->flags &
> DRM_MODE_FLAG_INTERLACE);
> + cmdline_mode->tv_mode = mode->tv_mode;
> cmdline_mode->specified = true;
>
> return 1;
> diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> index 59b29cdfdd35..f1e73ed65be0 100644
> --- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> +++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
> @@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct
> kunit *test)
> KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED);
> }
>
> +static void drm_cmdline_test_tv_options(struct kunit *test,
> + const char *cmdline,
> + const struct drm_display_mode
> *expected_mode,
> + unsigned int expected_tv_mode)
> +{
> + struct drm_cmdline_mode mode = { };
> +
> + KUNIT_EXPECT_TRUE(test,
> drm_mode_parse_command_line_for_connector(cmdline,
> +
> &no_connector, &mode));
> + KUNIT_EXPECT_TRUE(test, mode.specified);
> + KUNIT_EXPECT_EQ(test, mode.xres, expected_mode->hdisplay);
> + KUNIT_EXPECT_EQ(test, mode.y
[PATCH v2 19/41] drm/modes: Introduce the tv_mode property as a command-line option
Our new tv mode option allows to specify the TV mode from a property.
However, it can still be useful, for example to avoid any boot time
artifact, to set that property directly from the kernel command line.
Let's add some code to allow it, and some unit tests to exercise that code.
Signed-off-by: Maxime Ripard
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 73d01e755496..a759a4ba0036 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2115,6 +2115,30 @@ static int drm_mode_parse_panel_orientation(const char
*delim,
return 0;
}
+static int drm_mode_parse_tv_mode(const char *delim,
+ struct drm_cmdline_mode *mode)
+{
+ const char *value;
+ unsigned int len;
+ int ret;
+
+ if (*delim != '=')
+ return -EINVAL;
+
+ value = delim + 1;
+ delim = strchr(value, ',');
+ if (!delim)
+ delim = value + strlen(value);
+
+ ret = drm_get_tv_mode_from_name(value, delim - value);
+ if (ret < 0)
+ return ret;
+
+ mode->tv_mode = ret;
+
+ return 0;
+}
+
static int drm_mode_parse_cmdline_options(const char *str,
bool freestanding,
const struct drm_connector *connector,
@@ -2184,6 +2208,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
} else if (!strncmp(option, "panel_orientation", delim -
option)) {
if (drm_mode_parse_panel_orientation(delim, mode))
return -EINVAL;
+ } else if (!strncmp(option, "tv_mode", delim - option)) {
+ if (drm_mode_parse_tv_mode(delim, mode))
+ return -EINVAL;
} else {
return -EINVAL;
}
@@ -2212,20 +2239,22 @@ struct drm_named_mode {
unsigned int xres;
unsigned int yres;
unsigned int flags;
+ unsigned int tv_mode;
};
-#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \
+#define NAMED_MODE(_name, _pclk, _x, _y, _flags, _mode)\
{ \
.name = _name, \
.pixel_clock_khz = _pclk, \
.xres = _x, \
.yres = _y, \
.flags = _flags,\
+ .tv_mode = _mode, \
}
static const struct drm_named_mode drm_named_modes[] = {
- NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE),
- NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE),
+ NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
DRM_MODE_TV_MODE_NTSC_M),
+ NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
DRM_MODE_TV_MODE_PAL_B),
};
static int drm_mode_parse_cmdline_named_mode(const char *name,
@@ -2271,6 +2300,7 @@ static int drm_mode_parse_cmdline_named_mode(const char
*name,
cmdline_mode->xres = mode->xres;
cmdline_mode->yres = mode->yres;
cmdline_mode->interlace = !!(mode->flags &
DRM_MODE_FLAG_INTERLACE);
+ cmdline_mode->tv_mode = mode->tv_mode;
cmdline_mode->specified = true;
return 1;
diff --git a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
index 59b29cdfdd35..f1e73ed65be0 100644
--- a/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
+++ b/drivers/gpu/drm/tests/drm_cmdline_parser_test.c
@@ -885,6 +885,201 @@ static void drm_cmdline_test_multiple_options(struct
kunit *test)
KUNIT_EXPECT_EQ(test, mode.force, DRM_FORCE_UNSPECIFIED);
}
+static void drm_cmdline_test_tv_options(struct kunit *test,
+ const char *cmdline,
+ const struct drm_display_mode
*expected_mode,
+ unsigned int expected_tv_mode)
+{
+ struct drm_cmdline_mode mode = { };
+
+ KUNIT_EXPECT_TRUE(test,
drm_mode_parse_command_line_for_connector(cmdline,
+
&no_connector, &mode));
+ KUNIT_EXPECT_TRUE(test, mode.specified);
+ KUNIT_EXPECT_EQ(test, mode.xres, expected_mode->hdisplay);
+ KUNIT_EXPECT_EQ(test, mode.yres, expected_mode->vdisplay);
+ KUNIT_EXPECT_EQ(test, mode.tv_mode, expected_tv_mode);
+
+ KUNIT_EXPECT_FALSE(test, mode.refresh_specified);
+
+ KUNIT_EXPECT_FALSE(test, mode.bpp_specified);
+
+ KUNIT_EXPECT_FALSE(test, mode.rb);
+ KUNIT_EXPECT_FALSE(test, mode.cvt);
+ KUNIT_EXPECT_EQ(test, mode.interlace, !!(expected_mode->flags &
DRM_MODE_FLAG_INTERLACE));
+ KUNIT_EXPECT_FALSE(t
