diff mbox series

[v6,14/23] drm/modes: Properly generate a drm_display_mode from a named mode

Message ID 20220728-rpi-analog-tv-properties-v6-14-e7792734108f@cerno.tech (mailing list archive)
State New, archived
Headers show
Series drm: Analog TV Improvements | expand

Commit Message

Maxime Ripard Oct. 26, 2022, 3:33 p.m. UTC
The framework will get the drm_display_mode from the drm_cmdline_mode it
got by parsing the video command line argument by calling
drm_connector_pick_cmdline_mode().

The heavy lifting will then be done by the drm_mode_create_from_cmdline_mode()
function.

In the case of the named modes though, there's no real code to make that
translation and we rely on the drivers to guess which actual display mode
we meant.

Let's modify drm_mode_create_from_cmdline_mode() to properly generate the
drm_display_mode we mean when passing a named mode.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
Changes in v6:
- Fix get_modes to return 0 instead of an error code
- Rename the tests to follow the DRM test naming convention

Changes in v5:
- Switched to KUNIT_ASSERT_NOT_NULL
---
 drivers/gpu/drm/drm_modes.c                     | 34 ++++++++++-
 drivers/gpu/drm/tests/drm_client_modeset_test.c | 77 ++++++++++++++++++++++++-
 2 files changed, 109 insertions(+), 2 deletions(-)

Comments

Mateusz Kwiatkowski Oct. 26, 2022, 9:25 p.m. UTC | #1
Hi Maxime,

> +static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
> +					       struct drm_cmdline_mode *cmd)
> +{
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
> +		const struct drm_named_mode *named_mode = &drm_named_modes[i];
> +
> +		if (strcmp(cmd->name, named_mode->name))
> +			continue;
> +
> +		if (!named_mode->tv_mode)
> +			continue;
> +
> +		mode = drm_analog_tv_mode(dev,
> +					  named_mode->tv_mode,
> +					  named_mode->pixel_clock_khz * 1000,
> +					  named_mode->xres,
> +					  named_mode->yres,
> +					  named_mode->flags & DRM_MODE_FLAG_INTERLACE);
> +		if (!mode)
> +			return NULL;
> +
> +		return mode;
> +	}
> +
> +	return NULL;
> +}
> +

You didn't add tv_mode_specified to struct drm_named_mode, and left the
if (!named_mode->tv_mode) condition here. This will break on NTSC.

Best regards,
Mateusz Kwiatkowski
Noralf Trønnes Nov. 5, 2022, 5:50 p.m. UTC | #2
Den 26.10.2022 17.33, skrev maxime@cerno.tech:
> The framework will get the drm_display_mode from the drm_cmdline_mode it
> got by parsing the video command line argument by calling
> drm_connector_pick_cmdline_mode().
> 
> The heavy lifting will then be done by the drm_mode_create_from_cmdline_mode()
> function.
> 
> In the case of the named modes though, there's no real code to make that
> translation and we rely on the drivers to guess which actual display mode
> we meant.
> 
> Let's modify drm_mode_create_from_cmdline_mode() to properly generate the
> drm_display_mode we mean when passing a named mode.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> ---
> Changes in v6:
> - Fix get_modes to return 0 instead of an error code
> - Rename the tests to follow the DRM test naming convention
> 
> Changes in v5:
> - Switched to KUNIT_ASSERT_NOT_NULL
> ---
>  drivers/gpu/drm/drm_modes.c                     | 34 ++++++++++-
>  drivers/gpu/drm/tests/drm_client_modeset_test.c | 77 ++++++++++++++++++++++++-
>  2 files changed, 109 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index dc037f7ceb37..85aa9898c229 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2497,6 +2497,36 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
>  }
>  EXPORT_SYMBOL(drm_mode_parse_command_line_for_connector);
>  
> +static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
> +					       struct drm_cmdline_mode *cmd)
> +{
> +	struct drm_display_mode *mode;
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
> +		const struct drm_named_mode *named_mode = &drm_named_modes[i];
> +
> +		if (strcmp(cmd->name, named_mode->name))
> +			continue;
> +
> +		if (!named_mode->tv_mode)
> +			continue;
> +
> +		mode = drm_analog_tv_mode(dev,
> +					  named_mode->tv_mode,
> +					  named_mode->pixel_clock_khz * 1000,
> +					  named_mode->xres,
> +					  named_mode->yres,
> +					  named_mode->flags & DRM_MODE_FLAG_INTERLACE);
> +		if (!mode)
> +			return NULL;
> +
> +		return mode;
> +	}
> +
> +	return NULL;
> +}
> +
>  /**
>   * drm_mode_create_from_cmdline_mode - convert a command line modeline into a DRM display mode
>   * @dev: DRM device to create the new mode for
> @@ -2514,7 +2544,9 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
>  	if (cmd->xres == 0 || cmd->yres == 0)
>  		return NULL;
>  
> -	if (cmd->cvt)
> +	if (strlen(cmd->name))
> +		mode = drm_named_mode(dev, cmd);

I'm trying to track how this generated mode fits into to it all and
AFAICS if the connector already supports a mode with the same xres/yres
as the named mode, the named mode will never be created because of the
check at the beginning of drm_helper_probe_add_cmdline_mode(). It will
just mark the existing mode with USERDEF and return.

If the connector doesn't already support a mode with such a resolution
it will be created, but should we do that? If the driver supported such
a mode it would certainly already have added it to the mode list,
wouldn't it? After all it's just 2 variants NTSC and PAL.

We have this in drm_client_modeset.c:drm_connector_pick_cmdline_mode():

	list_for_each_entry(mode, &connector->modes, head) {
		/* Check (optional) mode name first */
		if (!strcmp(mode->name, cmdline_mode->name))
			return mode;

Here it looks like the named mode thing is a way to choose a mode, not
to add one.

I couldn't find any documentation on how named modes is supposed to
work, have you seen any?

Noralf.

> +	else if (cmd->cvt)
>  		mode = drm_cvt_mode(dev,
>  				    cmd->xres, cmd->yres,
>  				    cmd->refresh_specified ? cmd->refresh : 60,
Maxime Ripard Nov. 7, 2022, 1:34 p.m. UTC | #3
On Sat, Nov 05, 2022 at 06:50:30PM +0100, Noralf Trønnes wrote:
> Den 26.10.2022 17.33, skrev maxime@cerno.tech:
> > The framework will get the drm_display_mode from the drm_cmdline_mode it
> > got by parsing the video command line argument by calling
> > drm_connector_pick_cmdline_mode().
> > 
> > The heavy lifting will then be done by the drm_mode_create_from_cmdline_mode()
> > function.
> > 
> > In the case of the named modes though, there's no real code to make that
> > translation and we rely on the drivers to guess which actual display mode
> > we meant.
> > 
> > Let's modify drm_mode_create_from_cmdline_mode() to properly generate the
> > drm_display_mode we mean when passing a named mode.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > ---
> > Changes in v6:
> > - Fix get_modes to return 0 instead of an error code
> > - Rename the tests to follow the DRM test naming convention
> > 
> > Changes in v5:
> > - Switched to KUNIT_ASSERT_NOT_NULL
> > ---
> >  drivers/gpu/drm/drm_modes.c                     | 34 ++++++++++-
> >  drivers/gpu/drm/tests/drm_client_modeset_test.c | 77 ++++++++++++++++++++++++-
> >  2 files changed, 109 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> > index dc037f7ceb37..85aa9898c229 100644
> > --- a/drivers/gpu/drm/drm_modes.c
> > +++ b/drivers/gpu/drm/drm_modes.c
> > @@ -2497,6 +2497,36 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option,
> >  }
> >  EXPORT_SYMBOL(drm_mode_parse_command_line_for_connector);
> >  
> > +static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
> > +					       struct drm_cmdline_mode *cmd)
> > +{
> > +	struct drm_display_mode *mode;
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
> > +		const struct drm_named_mode *named_mode = &drm_named_modes[i];
> > +
> > +		if (strcmp(cmd->name, named_mode->name))
> > +			continue;
> > +
> > +		if (!named_mode->tv_mode)
> > +			continue;
> > +
> > +		mode = drm_analog_tv_mode(dev,
> > +					  named_mode->tv_mode,
> > +					  named_mode->pixel_clock_khz * 1000,
> > +					  named_mode->xres,
> > +					  named_mode->yres,
> > +					  named_mode->flags & DRM_MODE_FLAG_INTERLACE);
> > +		if (!mode)
> > +			return NULL;
> > +
> > +		return mode;
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> >  /**
> >   * drm_mode_create_from_cmdline_mode - convert a command line modeline into a DRM display mode
> >   * @dev: DRM device to create the new mode for
> > @@ -2514,7 +2544,9 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev,
> >  	if (cmd->xres == 0 || cmd->yres == 0)
> >  		return NULL;
> >  
> > -	if (cmd->cvt)
> > +	if (strlen(cmd->name))
> > +		mode = drm_named_mode(dev, cmd);
> 
> I'm trying to track how this generated mode fits into to it all and
> AFAICS if the connector already supports a mode with the same xres/yres
> as the named mode, the named mode will never be created because of the
> check at the beginning of drm_helper_probe_add_cmdline_mode(). It will
> just mark the existing mode with USERDEF and return.

Yep, you're right

> If the connector doesn't already support a mode with such a resolution
> it will be created, but should we do that? If the driver supported such
> a mode it would certainly already have added it to the mode list,
> wouldn't it? After all it's just 2 variants NTSC and PAL.

I wasn't so sure about this part. I think it's still benefitial because
some users (Geert at least has expressed that need) might want a smaller
mode than 480i/576i, whereas the driver is realistically only going to
register those two.

So creating that mode if it isn't declared seems to have value to some.

> We have this in drm_client_modeset.c:drm_connector_pick_cmdline_mode():
> 
> 	list_for_each_entry(mode, &connector->modes, head) {
> 		/* Check (optional) mode name first */
> 		if (!strcmp(mode->name, cmdline_mode->name))
> 			return mode;
> 
> Here it looks like the named mode thing is a way to choose a mode, not
> to add one.
> 
> I couldn't find any documentation on how named modes is supposed to
> work, have you seen any?

Eh, I guess I'm to blame for that :)

Named modes are really only about the command-line name. The way it was
initially introduced was pretty much to only pass down the name to
drivers for them to figure it out, like we've been doing in sun4i:

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292

It wasn't really working, especially because the userspace pretty much
ignores it. One of the point of this series is to create a proper mode
(and state, really) from the name passed on the command line so that
drivers don't have to behave any different from usual, and userspace can
be involved there too.

Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index dc037f7ceb37..85aa9898c229 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -2497,6 +2497,36 @@  bool drm_mode_parse_command_line_for_connector(const char *mode_option,
 }
 EXPORT_SYMBOL(drm_mode_parse_command_line_for_connector);
 
+static struct drm_display_mode *drm_named_mode(struct drm_device *dev,
+					       struct drm_cmdline_mode *cmd)
+{
+	struct drm_display_mode *mode;
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(drm_named_modes); i++) {
+		const struct drm_named_mode *named_mode = &drm_named_modes[i];
+
+		if (strcmp(cmd->name, named_mode->name))
+			continue;
+
+		if (!named_mode->tv_mode)
+			continue;
+
+		mode = drm_analog_tv_mode(dev,
+					  named_mode->tv_mode,
+					  named_mode->pixel_clock_khz * 1000,
+					  named_mode->xres,
+					  named_mode->yres,
+					  named_mode->flags & DRM_MODE_FLAG_INTERLACE);
+		if (!mode)
+			return NULL;
+
+		return mode;
+	}
+
+	return NULL;
+}
+
 /**
  * drm_mode_create_from_cmdline_mode - convert a command line modeline into a DRM display mode
  * @dev: DRM device to create the new mode for
@@ -2514,7 +2544,9 @@  drm_mode_create_from_cmdline_mode(struct drm_device *dev,
 	if (cmd->xres == 0 || cmd->yres == 0)
 		return NULL;
 
-	if (cmd->cvt)
+	if (strlen(cmd->name))
+		mode = drm_named_mode(dev, cmd);
+	else if (cmd->cvt)
 		mode = drm_cvt_mode(dev,
 				    cmd->xres, cmd->yres,
 				    cmd->refresh_specified ? cmd->refresh : 60,
diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c b/drivers/gpu/drm/tests/drm_client_modeset_test.c
index 3aa1acfe75df..fdfe9e20702e 100644
--- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
+++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
@@ -21,7 +21,26 @@  struct drm_client_modeset_test_priv {
 
 static int drm_client_modeset_connector_get_modes(struct drm_connector *connector)
 {
-	return drm_add_modes_noedid(connector, 1920, 1200);
+	struct drm_display_mode *mode;
+	int count;
+
+	count = drm_add_modes_noedid(connector, 1920, 1200);
+
+	mode = drm_mode_analog_ntsc_480i(connector->dev);
+	if (!mode)
+		return count;
+
+	drm_mode_probed_add(connector, mode);
+	count += 1;
+
+	mode = drm_mode_analog_pal_576i(connector->dev);
+	if (!mode)
+		return count;
+
+	drm_mode_probed_add(connector, mode);
+	count += 1;
+
+	return count;
 }
 
 static const struct drm_connector_helper_funcs drm_client_modeset_connector_helper_funcs = {
@@ -52,6 +71,9 @@  static int drm_client_modeset_test_init(struct kunit *test)
 
 	drm_connector_helper_add(&priv->connector, &drm_client_modeset_connector_helper_funcs);
 
+	priv->connector.interlace_allowed = true;
+	priv->connector.doublescan_allowed = true;
+
 	return 0;
 
 }
@@ -85,9 +107,62 @@  static void drm_test_pick_cmdline_res_1920_1080_60(struct kunit *test)
 	KUNIT_EXPECT_TRUE(test, drm_mode_equal(expected_mode, mode));
 }
 
+static void drm_test_pick_cmdline_named_ntsc(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+	struct drm_display_mode *mode;
+	const char *cmdline = "NTSC";
+	int ret;
+
+	KUNIT_ASSERT_TRUE(test,
+			  drm_mode_parse_command_line_for_connector(cmdline,
+								    connector,
+								    cmdline_mode));
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_pick_cmdline_mode(connector);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+
+	KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_ntsc_480i(drm), mode));
+}
+
+static void drm_test_pick_cmdline_named_pal(struct kunit *test)
+{
+	struct drm_client_modeset_test_priv *priv = test->priv;
+	struct drm_device *drm = priv->drm;
+	struct drm_connector *connector = &priv->connector;
+	struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
+	struct drm_display_mode *mode;
+	const char *cmdline = "PAL";
+	int ret;
+
+	KUNIT_ASSERT_TRUE(test,
+			  drm_mode_parse_command_line_for_connector(cmdline,
+								    connector,
+								    cmdline_mode));
+
+	mutex_lock(&drm->mode_config.mutex);
+	ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
+	mutex_unlock(&drm->mode_config.mutex);
+	KUNIT_ASSERT_GT(test, ret, 0);
+
+	mode = drm_connector_pick_cmdline_mode(connector);
+	KUNIT_ASSERT_NOT_NULL(test, mode);
+
+	KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_pal_576i(drm), mode));
+}
 
 static struct kunit_case drm_test_pick_cmdline_tests[] = {
 	KUNIT_CASE(drm_test_pick_cmdline_res_1920_1080_60),
+	KUNIT_CASE(drm_test_pick_cmdline_named_ntsc),
+	KUNIT_CASE(drm_test_pick_cmdline_named_pal),
 	{}
 };