Re: [Intel-gfx] [PATCH v7 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
On Tue, Nov 08, 2022 at 10:40:07AM +0100, Noralf Trønnes wrote: > > > Den 07.11.2022 18.49, skrev Noralf Trønnes: > > > > > > Den 07.11.2022 15.16, skrev Maxime Ripard: > >> 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 > >> > >> --- > >> Changes in v7: > >> - Use tv_mode_specified in drm_mode_parse_command_line_for_connector > >> > >> 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..49441cabdd9d 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 = _named_modes[i]; > >> + > >> + if (strcmp(cmd->name, named_mode->name)) > >> + continue; > >> + > >> + if (!cmd->tv_mode_specified) > >> + continue; > > > > Only a named mode will set cmd->name, so is this check necessary? > > > >> + > >> + 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; > > > > You can just return the result from drm_analog_tv_mode() directly. > > > > With those considered: > > > > Reviewed-by: Noralf Trønnes > > > > I forgot one thing, shouldn't the named mode test in > drm_connector_pick_cmdline_mode() be removed now that we have proper modes? Good catch, I've fixed it Thanks! Maxime signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH v7 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Hi, On Mon, Nov 07, 2022 at 06:49:57PM +0100, Noralf Trønnes wrote: > Den 07.11.2022 15.16, skrev Maxime Ripard: > > 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 > > > > --- > > Changes in v7: > > - Use tv_mode_specified in drm_mode_parse_command_line_for_connector > > > > 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..49441cabdd9d 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 = _named_modes[i]; > > + > > + if (strcmp(cmd->name, named_mode->name)) > > + continue; > > + > > + if (!cmd->tv_mode_specified) > > + continue; > > Only a named mode will set cmd->name, so is this check necessary? Yeah, but (and even though it's not the case at the moment) there's no implication that a named mode will be about TV. We could use it for VGA/XGA/etc just as well, in which case we wouldn't have tv_mode_specified. Maxime signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH v7 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Den 07.11.2022 18.49, skrev Noralf Trønnes: > > > Den 07.11.2022 15.16, skrev Maxime Ripard: >> 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 >> >> --- >> Changes in v7: >> - Use tv_mode_specified in drm_mode_parse_command_line_for_connector >> >> 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..49441cabdd9d 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 = _named_modes[i]; >> + >> +if (strcmp(cmd->name, named_mode->name)) >> +continue; >> + >> +if (!cmd->tv_mode_specified) >> +continue; > > Only a named mode will set cmd->name, so is this check necessary? > >> + >> +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; > > You can just return the result from drm_analog_tv_mode() directly. > > With those considered: > > Reviewed-by: Noralf Trønnes > I forgot one thing, shouldn't the named mode test in drm_connector_pick_cmdline_mode() be removed now that we have proper modes? Noralf. >> +} >> + >> +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(>connector, >> _client_modeset_connector_helper_funcs); >> >> +
Re: [Intel-gfx] [PATCH v7 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Den 07.11.2022 15.16, skrev Maxime Ripard: > 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 > > --- > Changes in v7: > - Use tv_mode_specified in drm_mode_parse_command_line_for_connector > > 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..49441cabdd9d 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 = _named_modes[i]; > + > + if (strcmp(cmd->name, named_mode->name)) > + continue; > + > + if (!cmd->tv_mode_specified) > + continue; Only a named mode will set cmd->name, so is this check necessary? > + > + 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; You can just return the result from drm_analog_tv_mode() directly. With those considered: Reviewed-by: Noralf Trønnes > + } > + > + 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(>connector, > _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)); > }