Re: [Intel-gfx] [PATCH v10 06/19] drm/modes: Add a function to generate analog display modes
Den 17.11.2022 10.28, 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. > > Tested-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- I'm no domain expert so apart from the timing details which I can't comment on, it looks fine. I personally advocated for a much simpler solution for these NTSC and PAL modes, but AIUI this is part of a grander plan to support devices with other timings. Acked-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v10 05/19] drm/connector: Add TV standard property
Den 17.11.2022 10.28, skrev Maxime Ripard:
> The TV mode property has been around for a while now to select and get the
> current TV mode output on an analog TV connector.
>
> Despite that property name being generic, its content isn't and has been
> driver-specific which makes it hard to build any generic behaviour on top
> of it, both in kernel and user-space.
>
> Let's create a new enum tv norm property, that can contain any of the
> analog TV standards currently supported by kernel drivers. Each driver can
> then pass in a bitmask of the modes it supports, and the property
> creation function will filter out the modes not supported.
>
> We'll then be able to phase out the older tv mode property.
>
> Tested-by: Mateusz Kwiatkowski
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v10:
> - Fix checkpatch warning
>
> Changes in v5:
> - Create an analog TV properties documentation section, and document TV
> Mode there instead of the csv file
>
> Changes in v4:
> - Add property documentation to kms-properties.csv
> - Fix documentation
> ---
> Documentation/gpu/drm-kms.rst | 6 ++
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_connector.c | 122
> +-
> include/drm/drm_connector.h | 64
> include/drm/drm_mode_config.h | 8 +++
> 5 files changed, 203 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index b4377a545425..321f2f582c64 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -520,6 +520,12 @@ HDMI Specific Connector Properties
> .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> :doc: HDMI connector properties
>
> +Analog TV Specific Connector Properties
> +--
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> + :doc: Analog TV Connector Properties
> +
> Standard CRTC Properties
>
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7f2b9a07fbdf..d867e7f9f2cd 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -700,6 +700,8 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
> state->tv.margins.bottom = val;
> } else if (property == config->legacy_tv_mode_property) {
> state->tv.legacy_mode = val;
> + } else if (property == config->tv_mode_property) {
> + state->tv.mode = val;
> } else if (property == config->tv_brightness_property) {
> state->tv.brightness = val;
> } else if (property == config->tv_contrast_property) {
> @@ -810,6 +812,8 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
> *val = state->tv.margins.bottom;
> } else if (property == config->legacy_tv_mode_property) {
> *val = state->tv.legacy_mode;
> + } else if (property == config->tv_mode_property) {
> + *val = state->tv.mode;
> } else if (property == config->tv_brightness_property) {
> *val = state->tv.brightness;
> } else if (property == config->tv_contrast_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 06e737ed15f5..07d449736956 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -984,6 +984,17 @@ static const struct drm_prop_enum_list
> drm_dvi_i_subconnector_enum_list[] = {
> DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name,
>drm_dvi_i_subconnector_enum_list)
>
> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> + { DRM_MODE_TV_MODE_NTSC, "NTSC" },
> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> + { DRM_MODE_TV_MODE_PAL, "PAL" },
> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> + { DRM_MODE_TV_MODE_SECAM, "SECAM" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> +
This patch looks good but since I'm no TV standards expert I can't say
if the content of this list is a good choice for reflecting the world of
TV standards.
Acked-by: Noralf Trønnes
> static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
> { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }
Re: [Intel-gfx] [PATCH v10 01/19] drm/tests: client: Mention that we can't use MODULE_ macros
Den 17.11.2022 10.28, skrev Maxime Ripard: > That file is included directly, so we can't use any MODULE macro. Let's > leave a comment to avoid any future mistake. > > Signed-off-by: Maxime Ripard > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v9 01/25] docs/fb: Document current named modes
Den 14.11.2022 14.00, skrev Maxime Ripard: > KMS supports a number of named modes already, but it's never been > documented anywhere, let's fix that. > > Signed-off-by: Maxime Ripard > > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v8 16/24] drm/modes: Introduce more named modes
Den 10.11.2022 12.07, skrev Maxime Ripard: > Now that we can easily extend the named modes list, let's add a few more > analog TV modes that were used in the wild, and some unit tests to make > sure it works as intended. > > Tested-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- > Changes in v6: > - Renamed the tests to follow DRM test naming convention > > Changes in v5: > - Switched to KUNIT_ASSERT_NOT_NULL > --- > drivers/gpu/drm/drm_modes.c | 2 + > drivers/gpu/drm/tests/drm_client_modeset_test.c | 54 > + > 2 files changed, 56 insertions(+) > This needs an entry in modedb.rst so people know named modes exist, with that: Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v8 15/24] drm/client: Remove match on mode name
Den 10.11.2022 12.07, skrev Maxime Ripard:
> Commit 3aeeb13d8996 ("drm/modes: Support modes names on the command
> line") initially introduced the named modes support by essentially
> matching the name passed on the command-line to the mode names defined
> by the drivers.
>
> This proved to be difficult to work with, since all drivers had to
> provide properly named modes. This was also needed because we weren't
> passing a full blown-mode to the drivers, but were only filling its
> name.
>
> Thanks to the previous patches, we now generate a proper mode, and we
> thus can use the usual matching algo on timings, and can simply drop the
> name match.
>
> Suggested-by: Noralf Trønnes
> Signed-off-by: Maxime Ripard
>
> ---
Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v8 14/24] drm/modes: Properly generate a drm_display_mode from a named mode
Den 10.11.2022 12.07, 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. > > Tested-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v7 15/23] drm/modes: Introduce more named modes
Den 07.11.2022 15.16, skrev Maxime Ripard:
> Now that we can easily extend the named modes list, let's add a few more
> analog TV modes that were used in the wild, and some unit tests to make
> sure it works as intended.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v6:
> - Renamed the tests to follow DRM test naming convention
>
> Changes in v5:
> - Switched to KUNIT_ASSERT_NOT_NULL
> ---
> drivers/gpu/drm/drm_modes.c | 2 +
> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54
> +
> 2 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 49441cabdd9d..17c5b6108103 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
>
> static const struct drm_named_mode drm_named_modes[] = {
> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC),
> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC_J),
> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL),
> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL_M),
> };
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index fdfe9e20702e..b3820d25beca 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> @@ -133,6 +133,32 @@ static void drm_test_pick_cmdline_named_ntsc(struct
> kunit *test)
> KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_ntsc_480i(drm),
> mode));
> }
>
> +static void drm_test_pick_cmdline_named_ntsc_j(struct kunit *test)
> +{
> + struct drm_client_modeset_test_priv *priv = test->priv;
> + struct drm_device *drm = priv->drm;
> + struct drm_connector *connector = >connector;
> + struct drm_cmdline_mode *cmdline_mode = >cmdline_mode;
> + struct drm_display_mode *mode;
> + const char *cmdline = "NTSC-J";
> + int ret;
> +
> + KUNIT_ASSERT_TRUE(test,
> + drm_mode_parse_command_line_for_connector(cmdline,
> + connector,
> +
> cmdline_mode));
> +
> + mutex_lock(>mode_config.mutex);
> + ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> + mutex_unlock(>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;
> @@ -159,10 +185,38 @@ static void drm_test_pick_cmdline_named_pal(struct
> kunit *test)
> KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_pal_576i(drm),
> mode));
> }
>
> +static void drm_test_pick_cmdline_named_pal_m(struct kunit *test)
> +{
> + struct drm_client_modeset_test_priv *priv = test->priv;
> + struct drm_device *drm = priv->drm;
> + struct drm_connector *connector = >connector;
> + struct drm_cmdline_mode *cmdline_mode = >cmdline_mode;
> + struct drm_display_mode *mode;
> + const char *cmdline = "PAL-M";
> + int ret;
> +
> + KUNIT_ASSERT_TRUE(test,
> + drm_mode_parse_command_line_for_connector(cmdline,
> + connector,
> +
> cmdline_mode));
> +
> + mutex_lock(>mode_config.mutex);
> + ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> + mutex_unlock(>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));
> +}
> +
There are 4 named mode tests that are almost identical, should probably
use KUNIT_ARRAY_PARAM like in the parser tests.
This patchset has been going on for a long time now so it can be fixed
later if you don't want to do it now:
Reviewed-by: Noralf Trønnes
> 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_ntsc_j),
> KUNIT_CASE(drm_test_pick_cmdline_named_pal),
> + KUNIT_CASE(drm_test_pick_cmdline_named_pal_m),
> {}
> };
>
>
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
Re: [Intel-gfx] [PATCH v7 15/23] drm/modes: Introduce more named modes
Den 07.11.2022 19.03, skrev Noralf Trønnes:
>
>
> Den 07.11.2022 15.16, skrev Maxime Ripard:
>> Now that we can easily extend the named modes list, let's add a few more
>> analog TV modes that were used in the wild, and some unit tests to make
>> sure it works as intended.
>>
>> Signed-off-by: Maxime Ripard
>>
>> ---
>> Changes in v6:
>> - Renamed the tests to follow DRM test naming convention
>>
>> Changes in v5:
>> - Switched to KUNIT_ASSERT_NOT_NULL
>> ---
>> drivers/gpu/drm/drm_modes.c | 2 +
>> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54
>> +
>> 2 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index 49441cabdd9d..17c5b6108103 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
>>
>> static const struct drm_named_mode drm_named_modes[] = {
>> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
>> DRM_MODE_TV_MODE_NTSC),
>> +NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
>> DRM_MODE_TV_MODE_NTSC_J),
>> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
>> DRM_MODE_TV_MODE_PAL),
>> +NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
>> DRM_MODE_TV_MODE_PAL_M),
>> };
>
> I'm now having second thoughts about the tv_mode commandline option. Can
> we just add all the variants to this table and drop the tv_mode option?
> IMO this will be more user friendly and less confusing.
>
One downside of this is that it's not possible to force connector status
when using named modes, but I think it would be better to have a force
option than a tv_mode option. A lot of userspace treats unknown status
as disconnected.
Anyone know if it's possible to set the connector status sysfs file
using a udev rule?
Noralf.
Re: [Intel-gfx] [PATCH v7 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 07.11.2022 15.16, skrev Maxime Ripard:
> From: Noralf Trønnes
>
> Most of the TV connectors will need a similar get_modes implementation
> that will, depending on the drivers' capabilities, register the 480i and
> 576i modes.
>
> That implementation will also need to set the preferred flag and order
> the modes based on the driver and users preferrence.
>
> This is especially important to guarantee that a userspace stack such as
> Xorg can start and pick up the preferred mode while maintaining a
> working output.
>
> Signed-off-by: Noralf Trønnes
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v7:
> - Used Noralf's implementation
>
> Changes in v6:
> - New patch
> ---
> drivers/gpu/drm/drm_probe_helper.c | 97
> ++
> include/drm/drm_probe_helper.h | 1 +
> 2 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 2fc21df709bc..edb2e4c4530a 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1147,3 +1147,100 @@ int drm_connector_helper_get_modes(struct
> drm_connector *connector)
> return count;
> }
> EXPORT_SYMBOL(drm_connector_helper_get_modes);
> +
> +static bool tv_mode_supported(struct drm_connector *connector,
> + enum drm_connector_tv_mode mode)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *property = dev->mode_config.tv_mode_property;
> +
> + unsigned int i;
> +
> + for (i = 0; i < property->num_values; i++)
> + if (property->values[i] == mode)
> + return true;
> +
> + return false;
> +}
This function is not used in the new implementation.
I hope you have tested this patch since I didn't even compile test my
implementation (probably should have said so...)
Noralf.
> +
> +/**
> + * drm_connector_helper_tv_get_modes - Fills the modes availables to a TV
> connector
> + * @connector: The connector
> + *
> + * Fills the available modes for a TV connector based on the supported
> + * TV modes, and the default mode expressed by the kernel command line.
> + *
> + * This can be used as the default TV connector helper .get_modes() hook
> + * if the driver does not need any special processing.
> + *
> + * Returns:
> + * The number of modes added to the connector.
> + */
> +int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *tv_mode_property =
> + dev->mode_config.tv_mode_property;
> + struct drm_cmdline_mode *cmdline = >cmdline_mode;
> + unsigned int ntsc_modes = BIT(DRM_MODE_TV_MODE_NTSC) |
> + BIT(DRM_MODE_TV_MODE_NTSC_443) |
> + BIT(DRM_MODE_TV_MODE_NTSC_J) |
> + BIT(DRM_MODE_TV_MODE_PAL_M);
> + unsigned int pal_modes = BIT(DRM_MODE_TV_MODE_PAL) |
> + BIT(DRM_MODE_TV_MODE_PAL_N) |
> + BIT(DRM_MODE_TV_MODE_SECAM);
> + unsigned int tv_modes[2] = { UINT_MAX, UINT_MAX };
> + unsigned int i, supported_tv_modes = 0;
> +
> + if (!tv_mode_property)
> + return 0;
> +
> + for (i = 0; i < tv_mode_property->num_values; i++)
> + supported_tv_modes |= BIT(tv_mode_property->values[i]);
> +
> + if ((supported_tv_modes & ntsc_modes) &&
> + (supported_tv_modes & pal_modes)) {
> + uint64_t default_mode;
> +
> + if (drm_object_property_get_default_value(>base,
> + tv_mode_property,
> + _mode))
> + return 0;
> +
> + if (cmdline->tv_mode_specified)
> + default_mode = cmdline->tv_mode;
> +
> + if (BIT(default_mode) & ntsc_modes) {
> + tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
> + tv_modes[1] = DRM_MODE_TV_MODE_PAL;
> + } else {
> + tv_modes[0] = DRM_MODE_TV_MODE_PAL;
> + tv_modes[1] = DRM_MODE_TV_MODE_NTSC;
> + }
> + } else if (supported_tv_modes & ntsc_modes) {
> + tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
> + } else if (supported_tv_modes & pal_modes) {
> + tv_modes[0] = DRM_MODE_TV_MODE_PAL;
> + } else {
> + return 0;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> + struct drm_display_mo
Re: [Intel-gfx] [PATCH v7 15/23] drm/modes: Introduce more named modes
Den 07.11.2022 15.16, skrev Maxime Ripard:
> Now that we can easily extend the named modes list, let's add a few more
> analog TV modes that were used in the wild, and some unit tests to make
> sure it works as intended.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v6:
> - Renamed the tests to follow DRM test naming convention
>
> Changes in v5:
> - Switched to KUNIT_ASSERT_NOT_NULL
> ---
> drivers/gpu/drm/drm_modes.c | 2 +
> drivers/gpu/drm/tests/drm_client_modeset_test.c | 54
> +
> 2 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 49441cabdd9d..17c5b6108103 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2272,7 +2272,9 @@ struct drm_named_mode {
>
> static const struct drm_named_mode drm_named_modes[] = {
> NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC),
> + NAMED_MODE("NTSC-J", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_NTSC_J),
> NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL),
> + NAMED_MODE("PAL-M", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE,
> DRM_MODE_TV_MODE_PAL_M),
> };
I'm now having second thoughts about the tv_mode commandline option. Can
we just add all the variants to this table and drop the tv_mode option?
IMO this will be more user friendly and less confusing.
The named modes needs to be documented in modedb.rst.
Noralf.
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> diff --git a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> index fdfe9e20702e..b3820d25beca 100644
> --- a/drivers/gpu/drm/tests/drm_client_modeset_test.c
> +++ b/drivers/gpu/drm/tests/drm_client_modeset_test.c
> @@ -133,6 +133,32 @@ static void drm_test_pick_cmdline_named_ntsc(struct
> kunit *test)
> KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_ntsc_480i(drm),
> mode));
> }
>
> +static void drm_test_pick_cmdline_named_ntsc_j(struct kunit *test)
> +{
> + struct drm_client_modeset_test_priv *priv = test->priv;
> + struct drm_device *drm = priv->drm;
> + struct drm_connector *connector = >connector;
> + struct drm_cmdline_mode *cmdline_mode = >cmdline_mode;
> + struct drm_display_mode *mode;
> + const char *cmdline = "NTSC-J";
> + int ret;
> +
> + KUNIT_ASSERT_TRUE(test,
> + drm_mode_parse_command_line_for_connector(cmdline,
> + connector,
> +
> cmdline_mode));
> +
> + mutex_lock(>mode_config.mutex);
> + ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> + mutex_unlock(>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;
> @@ -159,10 +185,38 @@ static void drm_test_pick_cmdline_named_pal(struct
> kunit *test)
> KUNIT_EXPECT_TRUE(test, drm_mode_equal(drm_mode_analog_pal_576i(drm),
> mode));
> }
>
> +static void drm_test_pick_cmdline_named_pal_m(struct kunit *test)
> +{
> + struct drm_client_modeset_test_priv *priv = test->priv;
> + struct drm_device *drm = priv->drm;
> + struct drm_connector *connector = >connector;
> + struct drm_cmdline_mode *cmdline_mode = >cmdline_mode;
> + struct drm_display_mode *mode;
> + const char *cmdline = "PAL-M";
> + int ret;
> +
> + KUNIT_ASSERT_TRUE(test,
> + drm_mode_parse_command_line_for_connector(cmdline,
> + connector,
> +
> cmdline_mode));
> +
> + mutex_lock(>mode_config.mutex);
> + ret = drm_helper_probe_single_connector_modes(connector, 1920, 1080);
> + mutex_unlock(>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 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_ntsc_j),
> KUNIT_CASE(drm_test_pick_cmdline_named_pal),
> + KUNIT_CASE(drm_test_pick_cmdline_named_pal_m),
> {}
> };
>
>
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 d
Re: [Intel-gfx] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 07.11.2022 11.21, skrev Maxime Ripard: > Hi Noralf, > > I'll leave aside your comments on the code, since we'll use your > implementation. > > On Sun, Nov 06, 2022 at 05:33:48PM +0100, Noralf Trønnes wrote: >> Den 26.10.2022 17.33, skrev max...@cerno.tech: >>> + >>> + if (cmdline->tv_mode_specified) >>> + default_mode = cmdline->tv_mode; >> >> I realised that we don't verify tv_mode coming from the command line, >> not here and not in the reset helper. Should we do that? A driver should >> be programmed defensively to handle an illegal/unsupported value, but it >> doesn't feel right to allow an illegal enum value coming through the >> core/helpers. > > I don't think we can end up with an invalid value here if it's been > specified. > > We parse the command line through drm_mode_parse_tv_mode() (introduced > in patch 13 "drm/modes: Introduce the tv_mode property as a command-line > option") that will pick the tv mode part of the command line, and call > drm_get_tv_mode_from_name() using it. > > drm_get_tv_mode_from_name() will return a EINVAL if it's not a value we > expect, and mode->tv_mode is only set on success. And AFAIK, there's no > other path that will set tv_mode. > I see now that illegal was the wrong word, but if the driver only supports ntsc, the user can still set tv_mode=PAL right? And that's an unsupported value that the driver can't fulfill, so it errors out. But then again maybe that's just how it is, we can also set a display mode that the driver can't handle, so this is no different in that respect. Yeah, my argument lost some of its strength here :) Noralf.
Re: [Intel-gfx] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 07.11.2022 11.07, skrev Maxime Ripard: > Hi Noralf, > > On Sun, Nov 06, 2022 at 05:59:23PM +0100, Noralf Trønnes wrote: >> >> >> Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski: >>> Hi Maxime, >>> >>> First of all, nice idea with the helper function that can be reused by >>> different >>> drivers. This is neat! >>> >>> But looking at this function, it feels a bit overcomplicated. You're >>> creating >>> the two modes, then checking which one is the default, then set the >>> preferred >>> one and possibly reorder them. Maybe it can be simplified somehow? >>> >>> Although when I tried to refactor it myself, I ended up with something >>> that's >>> not better at all. Maybe it needs to be complicated, after all :( >>> >> >> I also thought that the function was complicated/difficult to read, in >> particular the index stuff at the end, but I also failed in finding a >> "better" solution, just a different one ;) > > I think I like yours better still :) > > Can I bring it into my series, with your authorship and SoB? > Sure, no problem. Noralf.
Re: [Intel-gfx] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 27.10.2022 00.02, skrev Mateusz Kwiatkowski:
> Hi Maxime,
>
> First of all, nice idea with the helper function that can be reused by
> different
> drivers. This is neat!
>
> But looking at this function, it feels a bit overcomplicated. You're creating
> the two modes, then checking which one is the default, then set the preferred
> one and possibly reorder them. Maybe it can be simplified somehow?
>
> Although when I tried to refactor it myself, I ended up with something that's
> not better at all. Maybe it needs to be complicated, after all :(
>
I also thought that the function was complicated/difficult to read, in
particular the index stuff at the end, but I also failed in finding a
"better" solution, just a different one ;)
Noralf.
My version:
int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
{
struct drm_device *dev = connector->dev;
struct drm_property *tv_mode_property =
dev->mode_config.tv_mode_property;
struct drm_cmdline_mode *cmdline = >cmdline_mode;
unsigned int ntsc_modes = BIT(DRM_MODE_TV_MODE_NTSC) |
BIT(DRM_MODE_TV_MODE_NTSC_443) |
BIT(DRM_MODE_TV_MODE_NTSC_J) |
BIT(DRM_MODE_TV_MODE_PAL_M);
unsigned int pal_modes = BIT(DRM_MODE_TV_MODE_PAL) |
BIT(DRM_MODE_TV_MODE_PAL_N) |
BIT(DRM_MODE_TV_MODE_SECAM);
unsigned int tv_modes[2] = { UINT_MAX, UINT_MAX };
unsigned int i, supported_tv_modes = 0;
if (!tv_mode_property)
return 0;
for (i = 0; i < tv_mode_property->num_values; i++)
supported_tv_modes |= BIT(tv_mode_property->values[i]);
if ((supported_tv_modes & ntsc_modes) && (supported_tv_modes &
pal_modes)) {
uint64_t default_mode;
if (drm_object_property_get_default_value(>base,
tv_mode_property,
_mode))
return 0;
if (cmdline->tv_mode_specified)
default_mode = cmdline->tv_mode;
if (BIT(default_mode) & ntsc_modes) {
tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
tv_modes[1] = DRM_MODE_TV_MODE_PAL;
} else {
tv_modes[0] = DRM_MODE_TV_MODE_PAL;
tv_modes[1] = DRM_MODE_TV_MODE_NTSC;
}
} else if (supported_tv_modes & ntsc_modes) {
tv_modes[0] = DRM_MODE_TV_MODE_NTSC;
} else if (supported_tv_modes & pal_modes) {
tv_modes[0] = DRM_MODE_TV_MODE_PAL;
} else {
return 0;
}
for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
struct drm_display_mode *mode;
if (tv_modes[i] == DRM_MODE_TV_MODE_NTSC)
mode = drm_mode_analog_ntsc_480i(dev);
else if (tv_modes[i] == DRM_MODE_TV_MODE_PAL)
mode = drm_mode_analog_pal_576i(dev);
else
break;
if (!mode)
return i;
if (!i)
mode->type |= DRM_MODE_TYPE_PREFERRED;
drm_mode_probed_add(connector, mode);
}
return i;
}
Re: [Intel-gfx] [PATCH v6 16/23] drm/probe-helper: Provide a TV get_modes helper
Den 26.10.2022 17.33, skrev max...@cerno.tech:
> Most of the TV connectors will need a similar get_modes implementation
> that will, depending on the drivers' capabilities, register the 480i and
> 576i modes.
>
> That implementation will also need to set the preferred flag and order
> the modes based on the driver and users preferrence.
>
> This is especially important to guarantee that a userspace stack such as
> Xorg can start and pick up the preferred mode while maintaining a
> working output.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v6:
> - New patch
> ---
> drivers/gpu/drm/drm_probe_helper.c | 97
> ++
> include/drm/drm_probe_helper.h | 1 +
> 2 files changed, 98 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_probe_helper.c
> b/drivers/gpu/drm/drm_probe_helper.c
> index 69b0b2b9cc1c..4a60575f5c66 100644
> --- a/drivers/gpu/drm/drm_probe_helper.c
> +++ b/drivers/gpu/drm/drm_probe_helper.c
> @@ -1147,3 +1147,100 @@ int drm_connector_helper_get_modes(struct
> drm_connector *connector)
> return count;
> }
> EXPORT_SYMBOL(drm_connector_helper_get_modes);
> +
> +static bool tv_mode_supported(struct drm_connector *connector,
> + enum drm_connector_tv_mode mode)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_property *property = dev->mode_config.tv_mode_property;
> +
Superfluous linebreak
> + unsigned int i;
> +
> + for (i = 0; i < property->num_values; i++)
> + if (property->values[i] == mode)
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * drm_connector_helper_tv_get_modes - Fills the modes availables to a TV
> connector
availables -> available
> + * @connector: The connector
> + *
> + * Fills the available modes for a TV connector based on the supported
> + * TV modes, and the default mode expressed by the kernel command line.
> + *
> + * This can be used as the default TV connector helper .get_modes() hook
> + * if the driver does not need any special processing.
> + *
> + * Returns:
> + * The number of modes added to the connector.
> + */
> +int drm_connector_helper_tv_get_modes(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_cmdline_mode *cmdline = >cmdline_mode;
> + struct drm_display_mode *tv_modes[2] = {};
> + struct drm_display_mode *mode;
> + unsigned int first_mode_idx;
> + unsigned int count = 0;
> + uint64_t default_mode;
> + int ret;
> +
> + if (!dev->mode_config.tv_mode_property)
> + return 0;
> +
> + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_443) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_NTSC_J) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_M)) {
> + mode = drm_mode_analog_ntsc_480i(connector->dev);
Nit: You can use the dev variable here and below.
> + if (!mode)
> + return 0;
> +
> + tv_modes[count++] = mode;
> + }
> +
> + if (tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_PAL_N) ||
> + tv_mode_supported(connector, DRM_MODE_TV_MODE_SECAM)) {
> + mode = drm_mode_analog_pal_576i(connector->dev);
> + if (!mode)
> + return 0;
You leak the ntsc mode when returning (possibly).
> +
> + tv_modes[count++] = mode;
> + }
> +
Maybe check for count being zero here?
> + if (count == 1) {
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> + drm_mode_probed_add(connector, mode);
> + return count;
> + }
> +
> + ret = drm_object_property_get_default_value(>base,
> +
> dev->mode_config.tv_mode_property,
> + _mode);
> + if (ret)
> + return 0;
You leak both modes when returning here. Maybe move this up before
allocation to simplify error handling.
> +
> + if (cmdline->tv_mode_specified)
> + default_mode = cmdline->tv_mode;
I realised that we don't verify tv_mode coming from the command line,
not here and not in the reset helper. Should we do that? A driver should
be programmed defensively to handle an illegal/unsupported value, but it
doesn't feel right to allow an illegal enum value coming through the
core/helpers.
> +
> + if ((default_mode == DRM_MODE_TV_MODE_NTSC) ||
> + (default_mode == DRM_MODE_TV_MODE_NTSC_443) ||
> + (default_mode == DRM_MODE_TV_MODE_NTSC_J) ||
> + (default_mode == DRM_MODE_TV_MODE_PAL_M))
> + first_mode_idx = 0;
> + else
> + first_mode_idx = 1;
> +
> + mode = tv_modes[first_mode_idx];
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> +
Re: [Intel-gfx] [PATCH v6 13/23] drm/modes: Introduce the tv_mode property as a command-line option
Den 26.10.2022 17.33, skrev max...@cerno.tech: > 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 > > --- I would have just squashed the named mode part of this patch together with the 2 other named mode patches and keep just the video= option part here, but up to you: Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v6 11/23] drm/connector: Add pixel clock to cmdline mode
Den 26.10.2022 17.33, skrev max...@cerno.tech:
> We'll need to get the pixel clock to generate proper display modes for
> all the current named modes. Let's add it to struct drm_cmdline_mode and
> fill it when parsing the named mode.
>
> Signed-off-by: Maxime Ripard
> ---
I would just squash this with the previous patch, either way:
Reviewed-by: Noralf Trønnes
> drivers/gpu/drm/drm_modes.c | 9 ++---
> include/drm/drm_connector.h | 7 +++
> 2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index acee23e1a8b7..c826f9583a1d 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2226,22 +2226,24 @@ static int drm_mode_parse_cmdline_options(const char
> *str,
>
> struct drm_named_mode {
> const char *name;
> + unsigned int pixel_clock_khz;
> unsigned int xres;
> unsigned int yres;
> unsigned int flags;
> };
>
> -#define NAMED_MODE(_name, _x, _y, _flags)\
> +#define NAMED_MODE(_name, _pclk, _x, _y, _flags) \
> { \
> .name = _name, \
> + .pixel_clock_khz = _pclk, \
> .xres = _x, \
> .yres = _y, \
> .flags = _flags,\
> }
>
> static const struct drm_named_mode drm_named_modes[] = {
> - NAMED_MODE("NTSC", 720, 480, DRM_MODE_FLAG_INTERLACE),
> - NAMED_MODE("PAL", 720, 576, DRM_MODE_FLAG_INTERLACE),
> + NAMED_MODE("NTSC", 13500, 720, 480, DRM_MODE_FLAG_INTERLACE),
> + NAMED_MODE("PAL", 13500, 720, 576, DRM_MODE_FLAG_INTERLACE),
> };
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> @@ -2282,6 +2284,7 @@ static int drm_mode_parse_cmdline_named_mode(const char
> *name,
> continue;
>
> strcpy(cmdline_mode->name, mode->name);
> + cmdline_mode->pixel_clock = mode->pixel_clock_khz;
> cmdline_mode->xres = mode->xres;
> cmdline_mode->yres = mode->yres;
> cmdline_mode->interlace = !!(mode->flags &
> DRM_MODE_FLAG_INTERLACE);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 96b2e4e12334..5c5e67de2296 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1273,6 +1273,13 @@ struct drm_cmdline_mode {
>*/
> bool bpp_specified;
>
> + /**
> + * @pixel_clock:
> + *
> + * Pixel Clock in kHz. Optional.
> + */
> + unsigned int pixel_clock;
> +
> /**
>* @xres:
>*
>
Re: [Intel-gfx] [PATCH v6 10/23] drm/modes: Fill drm_cmdline mode from named modes
Den 26.10.2022 17.33, skrev max...@cerno.tech:
> The current code to deal with named modes will only set the mode name, and
> then it's up to drivers to try to match that name to whatever mode or
> configuration they see fit.
>
I couldn't find any driver that does that, all I could find that cares
about named modes are drm_client. Did I miss something here?
Apart from that:
Reviewed-by: Noralf Trønnes
> The plan is to remove that need and move the named mode handling out of
> drivers and into the core, and only rely on modes and properties. Let's
> start by properly filling drm_cmdline_mode from a named mode.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/drm_modes.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 7594b657f86a..acee23e1a8b7 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2226,11 +2226,22 @@ static int drm_mode_parse_cmdline_options(const char
> *str,
>
> struct drm_named_mode {
> const char *name;
> + unsigned int xres;
> + unsigned int yres;
> + unsigned int flags;
> };
>
> +#define NAMED_MODE(_name, _x, _y, _flags)\
> + { \
> + .name = _name, \
> + .xres = _x, \
> + .yres = _y, \
> + .flags = _flags,\
> + }
> +
> static const struct drm_named_mode drm_named_modes[] = {
> - { "NTSC", },
> - { "PAL", },
> + NAMED_MODE("NTSC", 720, 480, DRM_MODE_FLAG_INTERLACE),
> + NAMED_MODE("PAL", 720, 576, DRM_MODE_FLAG_INTERLACE),
> };
>
> static int drm_mode_parse_cmdline_named_mode(const char *name,
> @@ -2271,6 +2282,9 @@ static int drm_mode_parse_cmdline_named_mode(const char
> *name,
> continue;
>
> strcpy(cmdline_mode->name, mode->name);
> + cmdline_mode->xres = mode->xres;
> + cmdline_mode->yres = mode->yres;
> + cmdline_mode->interlace = !!(mode->flags &
> DRM_MODE_FLAG_INTERLACE);
> cmdline_mode->specified = true;
>
> return 1;
>
Re: [Intel-gfx] [PATCH v6 14/23] drm/modes: Properly generate a drm_display_mode from a named mode
Den 26.10.2022 17.33, skrev max...@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
>
> ---
> 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 = _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, >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,
Re: [Intel-gfx] [PATCH v6 09/23] drm/modes: Switch to named mode descriptors
Den 26.10.2022 17.33, skrev max...@cerno.tech: > The current named mode parsing relies only the mode name, and doesn't allow only the -> only on the > to specify any other parameter. > > Let's convert that string list to an array of a custom structure that will > hold the name and some additional parameters in the future. > > Signed-off-by: Maxime Ripard > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v6 08/23] drm/modes: Move named modes parsing to a separate function
Den 26.10.2022 17.33, skrev max...@cerno.tech: > The current construction of the named mode parsing doesn't allow to extend > it easily. Let's move it to a separate function so we can add more > parameters and modes. > > In order for the tests to still pass, some extra checks are needed, so > it's not a 1:1 move. > > Signed-off-by: Maxime Ripard > > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v4a 31/38] timers: drm: Use timer_shutdown_sync() for on stack timers
Den 05.11.2022 07.00, skrev Steven Rostedt: > From: "Steven Rostedt (Google)" > > Before a timer is released, timer_shutdown_sync() must be called. > > Link: https://lore.kernel.org/all/20221104054053.431922...@goodmis.org/ > > Cc: "Noralf Trønnes" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Joonas Lahtinen > Cc: Rodrigo Vivi > Cc: Tvrtko Ursulin > Cc: dri-de...@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Signed-off-by: Steven Rostedt (Google) > --- Acked-by: Noralf Trønnes > drivers/gpu/drm/gud/gud_pipe.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c > index 7c6dc2bcd14a..08429bdd57cf 100644 > --- a/drivers/gpu/drm/gud/gud_pipe.c > +++ b/drivers/gpu/drm/gud/gud_pipe.c > @@ -272,7 +272,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t > len) > > usb_sg_wait(); > > - if (!del_timer_sync()) > + if (!timer_shutdown_sync()) > ret = -ETIMEDOUT; > else if (ctx.sgr.status < 0) > ret = ctx.sgr.status;
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
Den 19.10.2022 10.48, skrev Maxime Ripard:
> On Tue, Oct 18, 2022 at 02:29:00PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 18.10.2022 11.33, skrev Maxime Ripard:
>>> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
>>>> Den 13.10.2022 15.18, skrev Maxime Ripard:
>>>>> As part of the command line parsing rework coming in the next patches,
>>>>> we'll need to lookup drm_connector_tv_mode values by their name, already
>>>>> defined in drm_tv_mode_enum_list.
>>>>>
>>>>> In order to avoid any code duplication, let's do a function that will
>>>>> perform a lookup of a TV mode name and return its value.
>>>>>
>>>>> Signed-off-by: Maxime Ripard
>>>>> ---
>>>>> drivers/gpu/drm/drm_connector.c | 24
>>>>> include/drm/drm_connector.h | 2 ++
>>>>> 2 files changed, 26 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_connector.c
>>>>> b/drivers/gpu/drm/drm_connector.c
>>>>> index 820f4c730b38..30611c616435 100644
>>>>> --- a/drivers/gpu/drm/drm_connector.c
>>>>> +++ b/drivers/gpu/drm/drm_connector.c
>>>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list
>>>>> drm_tv_mode_enum_list[] = {
>>>>> };
>>>>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>>>>>
>>>>> +/**
>>>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum
>>>>> value
>>>>> + * @name: TV Mode name we want to convert
>>>>> + * @len: Length of @name
>>>>> + *
>>>>> + * Translates @name into an enum drm_connector_tv_mode.
>>>>> + *
>>>>> + * Returns: the enum value on success, a negative errno otherwise.
>>>>> + */
>>>>> +int drm_get_tv_mode_from_name(const char *name, size_t len)
>>>>
>>>> Do we really need to pass in length here? item->name has to always be
>>>> NUL terminated otherwise things would break elsewhere, so it shouldn't
>>>> be necessary AFAICS.
>>>
>>> The only user so far is the command-line parsing code, and we might very
>>> well have an option after the tv_mode, something like
>>> 720x480i,tv_mode=NTSC,rotate=180
>>>
>>> In this case, we won't get a NULL-terminated name.
>>
>> My point is that item->name will always be NUL terminated so strcmp()
>> will never look past that.
>
> Right, but we don't have the guarantee that strlen(item->name) <
> strlen(name), and we could thus just access after the end of our name
>
Ok, using the length limiting str funtions is the safe thing to do, so
len needs to stay. But I don't get the 'strlen(item->name) == len'
check. strncmp() will stop when it reaches a NUL in either string so no
need for the length check?
Anyways:
Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
Den 18.10.2022 11.33, skrev Maxime Ripard:
> On Mon, Oct 17, 2022 at 12:44:45PM +0200, Noralf Trønnes wrote:
>> Den 13.10.2022 15.18, skrev Maxime Ripard:
>>> As part of the command line parsing rework coming in the next patches,
>>> we'll need to lookup drm_connector_tv_mode values by their name, already
>>> defined in drm_tv_mode_enum_list.
>>>
>>> In order to avoid any code duplication, let's do a function that will
>>> perform a lookup of a TV mode name and return its value.
>>>
>>> Signed-off-by: Maxime Ripard
>>> ---
>>> drivers/gpu/drm/drm_connector.c | 24
>>> include/drm/drm_connector.h | 2 ++
>>> 2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_connector.c
>>> b/drivers/gpu/drm/drm_connector.c
>>> index 820f4c730b38..30611c616435 100644
>>> --- a/drivers/gpu/drm/drm_connector.c
>>> +++ b/drivers/gpu/drm/drm_connector.c
>>> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list
>>> drm_tv_mode_enum_list[] = {
>>> };
>>> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>>>
>>> +/**
>>> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum
>>> value
>>> + * @name: TV Mode name we want to convert
>>> + * @len: Length of @name
>>> + *
>>> + * Translates @name into an enum drm_connector_tv_mode.
>>> + *
>>> + * Returns: the enum value on success, a negative errno otherwise.
>>> + */
>>> +int drm_get_tv_mode_from_name(const char *name, size_t len)
>>
>> Do we really need to pass in length here? item->name has to always be
>> NUL terminated otherwise things would break elsewhere, so it shouldn't
>> be necessary AFAICS.
>
> The only user so far is the command-line parsing code, and we might very
> well have an option after the tv_mode, something like
> 720x480i,tv_mode=NTSC,rotate=180
>
> In this case, we won't get a NULL-terminated name.
>
My point is that item->name will always be NUL terminated so strcmp()
will never look past that.
Noralf.
Re: [Intel-gfx] [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property
Den 13.10.2022 15.19, skrev Maxime Ripard:
> Now that the core can deal fine with analog TV modes, let's convert the vc4
> VEC driver to leverage those new features.
>
> We've added some backward compatibility to support the old TV mode property
> and translate it into the new TV norm property. We're also making use of
> the new analog TV atomic_check helper to make sure we trigger a modeset
> whenever the TV mode is updated.
>
> Acked-by: Noralf Trønnes
> Signed-off-by: Maxime Ripard
>
> ---
> @@ -276,19 +292,96 @@ static void vc4_vec_connector_reset(struct
> drm_connector *connector)
>
> static int vc4_vec_connector_get_modes(struct drm_connector *connector)
> {
> - struct drm_connector_state *state = connector->state;
> struct drm_display_mode *mode;
>
> - mode = drm_mode_duplicate(connector->dev,
> - vc4_vec_tv_modes[state->tv.legacy_mode].mode);
> + mode = drm_mode_analog_ntsc_480i(connector->dev);
> if (!mode) {
> DRM_ERROR("Failed to create a new display mode\n");
> return -ENOMEM;
> }
>
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> drm_mode_probed_add(connector, mode);
>
> - return 1;
> + mode = drm_mode_analog_pal_576i(connector->dev);
> + if (!mode) {
> + DRM_ERROR("Failed to create a new display mode\n");
> + return -ENOMEM;
I just remembered that you can't return an error from .get_modes, it
should only return the number of modes added. From doc:
* RETURNS:
*
* The number of modes added by calling drm_mode_probed_add().
See also the use of count in drm_helper_probe_single_connector_modes().
Patch 14 and 22 has the same issue.
Noralf.
> + }
> +
> + drm_mode_probed_add(connector, mode);
> +
> + return 2;
> +}
> +
> +static int
> +vc4_vec_connector_set_property(struct drm_connector *connector,
> +struct drm_connector_state *state,
> +struct drm_property *property,
> +uint64_t val)
> +{
> + struct vc4_vec *vec = connector_to_vc4_vec(connector);
> +
> + if (property != vec->legacy_tv_mode_property)
> + return -EINVAL;
> +
> + switch (val) {
> + case VC4_VEC_TV_MODE_NTSC:
> + state->tv.mode = DRM_MODE_TV_MODE_NTSC;
> + break;
> +
> + case VC4_VEC_TV_MODE_NTSC_J:
> + state->tv.mode = DRM_MODE_TV_MODE_NTSC_J;
> + break;
> +
> + case VC4_VEC_TV_MODE_PAL:
> + state->tv.mode = DRM_MODE_TV_MODE_PAL;
> + break;
> +
> + case VC4_VEC_TV_MODE_PAL_M:
> + state->tv.mode = DRM_MODE_TV_MODE_PAL_M;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +vc4_vec_connector_get_property(struct drm_connector *connector,
> +const struct drm_connector_state *state,
> +struct drm_property *property,
> +uint64_t *val)
> +{
> + struct vc4_vec *vec = connector_to_vc4_vec(connector);
> +
> + if (property != vec->legacy_tv_mode_property)
> + return -EINVAL;
> +
> + switch (state->tv.mode) {
> + case DRM_MODE_TV_MODE_NTSC:
> + *val = VC4_VEC_TV_MODE_NTSC;
> + break;
> +
> + case DRM_MODE_TV_MODE_NTSC_J:
> + *val = VC4_VEC_TV_MODE_NTSC_J;
> + break;
> +
> + case DRM_MODE_TV_MODE_PAL:
> + *val = VC4_VEC_TV_MODE_PAL;
> + break;
> +
> + case DRM_MODE_TV_MODE_PAL_M:
> + *val = VC4_VEC_TV_MODE_PAL_M;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> static const struct drm_connector_funcs vc4_vec_connector_funcs = {
> @@ -297,15 +390,19 @@ static const struct drm_connector_funcs
> vc4_vec_connector_funcs = {
> .reset = vc4_vec_connector_reset,
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> + .atomic_get_property = vc4_vec_connector_get_property,
> + .atomic_set_property = vc4_vec_connector_set_property,
> };
>
> static const struct drm_connector_helper_funcs
> vc4_vec_connector_helper_funcs = {
> + .atomic_check = drm_atomic_helper_connector_tv_check,
> .get_modes = vc4_vec_connector_ge
Re: [Intel-gfx] [PATCH v5 12/22] drm/connector: Add a function to lookup a TV mode by its name
Den 13.10.2022 15.18, skrev Maxime Ripard:
> As part of the command line parsing rework coming in the next patches,
> we'll need to lookup drm_connector_tv_mode values by their name, already
> defined in drm_tv_mode_enum_list.
>
> In order to avoid any code duplication, let's do a function that will
> perform a lookup of a TV mode name and return its value.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/drm_connector.c | 24
> include/drm/drm_connector.h | 2 ++
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 820f4c730b38..30611c616435 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -991,6 +991,30 @@ static const struct drm_prop_enum_list
> drm_tv_mode_enum_list[] = {
> };
> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>
> +/**
> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
> + * @name: TV Mode name we want to convert
> + * @len: Length of @name
> + *
> + * Translates @name into an enum drm_connector_tv_mode.
> + *
> + * Returns: the enum value on success, a negative errno otherwise.
> + */
> +int drm_get_tv_mode_from_name(const char *name, size_t len)
Do we really need to pass in length here?
item->name has to always be NUL terminated otherwise things would break
elsewhere, so it shouldn't be necessary AFAICS.
Noralf.
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(drm_tv_mode_enum_list); i++) {
> + const struct drm_prop_enum_list *item =
> _tv_mode_enum_list[i];
> +
> + if (strlen(item->name) == len && !strncmp(item->name, name,
> len))
> + return item->type;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(drm_get_tv_mode_from_name);
> +
> static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
> { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index a501db7d..a33f24a76738 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1864,6 +1864,8 @@ const char *drm_get_dp_subconnector_name(int val);
> const char *drm_get_content_protection_name(int val);
> const char *drm_get_hdcp_content_type_name(int val);
>
> +int drm_get_tv_mode_from_name(const char *name, size_t len);
> +
> int drm_mode_create_dvi_i_properties(struct drm_device *dev);
> void drm_connector_attach_dp_subconnector_property(struct drm_connector
> *connector);
>
>
Re: [Intel-gfx] [PATCH v5 16/22] drm/atomic-helper: Add a TV properties reset helper
Den 13.10.2022 15.19, skrev Maxime Ripard:
> The drm_tv_create_properties() function will create a bunch of properties,
> but it's up to each and every driver using that function to properly reset
> the state of these properties leading to inconsistent behaviours.
>
> Let's create a helper that will take care of it.
>
> Reviewed-by: Noralf Trønnes
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 75
> +++
> include/drm/drm_atomic_state_helper.h | 1 +
> 2 files changed, 76 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index dfb57217253b..0373c3dc824b 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -481,6 +481,81 @@ void drm_atomic_helper_connector_tv_margins_reset(struct
> drm_connector *connecto
> }
> EXPORT_SYMBOL(drm_atomic_helper_connector_tv_margins_reset);
>
> +/**
> + * drm_atomic_helper_connector_tv_reset - Resets Analog TV connector
> properties
> + * @connector: DRM connector
> + *
> + * Resets the analog TV properties attached to a connector
> + */
> +void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector)
> +{
> + struct drm_device *dev = connector->dev;
> + struct drm_cmdline_mode *cmdline = >cmdline_mode;
> + struct drm_connector_state *state = connector->state;
> + struct drm_property *prop;
> + uint64_t val;
> +
> + prop = dev->mode_config.tv_mode_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.mode = val;
> +
> + if (cmdline->tv_mode)
> + state->tv.mode = cmdline->tv_mode;
This can't set ntsc now that the none value is gone.
But we need a tv_mode_specified flag as mentioned in the other patch.
Noralf.
> +
> + prop = dev->mode_config.tv_select_subconnector_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.select_subconnector = val;
> +
> + prop = dev->mode_config.tv_subconnector_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.subconnector = val;
> +
> + prop = dev->mode_config.tv_brightness_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.brightness = val;
> +
> + prop = dev->mode_config.tv_contrast_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.contrast = val;
> +
> + prop = dev->mode_config.tv_flicker_reduction_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.flicker_reduction = val;
> +
> + prop = dev->mode_config.tv_overscan_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.overscan = val;
> +
> + prop = dev->mode_config.tv_saturation_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.saturation = val;
> +
> + prop = dev->mode_config.tv_hue_property;
> + if (prop)
> + if (!drm_object_property_get_default_value(>base,
> +prop, ))
> + state->tv.hue = val;
> +
> + drm_atomic_helper_connector_tv_margins_reset(connector);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset);
> +
> /**
> * __drm_atomic_helper_connector_duplicate_state - copy atomic connector
> state
> * @connector: connector object
> diff --git a/include/drm/drm_atomic_state_helper.h
> b/include/drm/drm_atomic_state_helper.h
> index 192766656b88..c8fbce795ee7 100644
> --- a/include/drm/drm_atomic_state_helper.h
> +++ b/inclu
Re: [Intel-gfx] [PATCH v5 20/22] drm/vc4: vec: Convert to the new TV mode property
Den 16.10.2022 20.52, skrev Mateusz Kwiatkowski:
> Hi Maxime,
>
>> static int vc4_vec_connector_get_modes(struct drm_connector *connector)
>> {
>> -struct drm_connector_state *state = connector->state;
>> struct drm_display_mode *mode;
>>
>> -mode = drm_mode_duplicate(connector->dev,
>> - vc4_vec_tv_modes[state->tv.legacy_mode].mode);
>> +mode = drm_mode_analog_ntsc_480i(connector->dev);
>> if (!mode) {
>> DRM_ERROR("Failed to create a new display mode\n");
>> return -ENOMEM;
>> }
>>
>> +mode->type |= DRM_MODE_TYPE_PREFERRED;
>> drm_mode_probed_add(connector, mode);
>>
>> -return 1;
>> +mode = drm_mode_analog_pal_576i(connector->dev);
>> +if (!mode) {
>> +DRM_ERROR("Failed to create a new display mode\n");
>> +return -ENOMEM;
>> +}
>> +
>> +drm_mode_probed_add(connector, mode);
>> +
>> +return 2;
>> +}
>
> Referencing those previous discussions:
> -
> https://lore.kernel.org/dri-devel/0255f7c6-0484-6456-350d-cf24f3fee...@tronnes.org/
> -
> https://lore.kernel.org/dri-devel/c8f8015a-75da-afa8-ca7f-b2b134cac...@gmail.com/
>
> Unconditionally setting the 480i mode as DRM_MODE_TYPE_PREFERRED causes Xorg
> (at least on current Raspberry Pi OS) to display garbage when
> video=Composite1:PAL is specified on the command line, so I'm afraid this
> won't
> do.
>
> As I see it, there are three viable solutions for this issue:
>
> a) Somehow query the video= command line option from this function, and set
>DRM_MODE_TYPE_PREFERRED appropriately. This would break the abstraction
>provided by global DRM code, but should work fine.
>
> b) Modify drm_helper_probe_add_cmdline_mode() so that it sets
>DRM_MODE_TYPE_PREFERRED in addition to DRM_MODE_TYPE_USERDEF. This seems
>pretty robust, but affects the entire DRM subsystem, which may break
>userspace in different ways.
>
>- Maybe this could be mitigated by adding some additional conditions, e.g.
> setting the PREFERRED flag only if no modes are already flagged as such
> and/or only if the cmdline mode is a named one (~= analog TV mode)
>
> c) Forcing userspace (Xorg / Raspberry Pi OS) to get fixed and honor the
> USERDEF
>flag.
>
> Either way, hardcoding 480i as PREFERRED does not seem right.
>
My solution for this is to look at tv.mode to know which mode to mark as
preferred. Maxime didn't like this since it changes things behind
userspace's back. I don't see how that can cause any problems for userspace.
If userspace uses atomic and sets tv_mode, it has to know which mode to
use before hand, so it doesn't look at the preferreded flag.
If it uses legacy and sets tv_mode, it can end up with a stale preferred
flag, but no worse than not having the flag or that ntsc is always
preferred.
If it doesn't change tv_mode, there's no problem, the preferred flag
doesn't change.
Noralf.
Re: [Intel-gfx] [PATCH v5 13/22] drm/modes: Introduce the tv_mode property as a command-line option
Den 16.10.2022 19.51, skrev Mateusz Kwiatkowski:
> Hi Maxime, Noralf & everyone,
>
> I'd like to address Noralf here in particular, and refer to these discussions
> from the past:
>
> -
> https://lore.kernel.org/linux-arm-kernel/2f607c7d-6da1-c8df-1c02-8dd344a92...@gmail.com/
> -
> https://lore.kernel.org/linux-arm-kernel/9e76a508-f469-a54d-ecd7-b5868ca99...@tronnes.org/
>
>> @@ -2230,20 +2256,22 @@ struct drm_named_mode {
>> unsigned int xres;
>> unsigned int yres;
>> unsigned int flags;
>> +unsigned int tv_mode;
>> };
>
> I saw that you (Noralf) opposed my suggestion about the DRM_MODE_TV_MODE_NONE
> enum value in enum drm drm_connector_tv_mode. I get your argumentation, and
> I'm
> not gonna argue, but I still don't like the fact that struct drm_named_mode
> now
> includes a field that is only relevant for analog TV modes, has no "none"
> value,
> and yet the type is supposed to be generic enough to be usable for other types
> of outputs as well.
>
> It's true that it can just be ignored (as Maxime mentioned in his response to
> my e-mail linked above), and now the value of 0 corresponds to
> DRM_MODE_TV_MODE_NTSC, which is a rather sane default, but it still feels
> messy
> to me.
>
> I'm not gonna force my opinion here, but I wanted to bring your attention to
> this issue, maybe you have some other solution in mind for this problem. Or if
> you don't see that as a problem at all, that's fine, too.
>
I hadn't looked at this patch in detail before, but you're right this,
together with drm_atomic_helper_connector_tv_reset(), will overwrite
tv.mode unconditionally regardless of tv_mode being present in video= or
not. We need a tv_mode_specified flag like we have for bpp and refresh.
Noralf.
Re: [Intel-gfx] [PATCH v5 08/22] drm/modes: Move named modes parsing to a separate function
Den 13.10.2022 15.18, skrev Maxime Ripard:
> The current construction of the named mode parsing doesn't allow to extend
> it easily. Let's move it to a separate function so we can add more
> parameters and modes.
>
> In order for the tests to still pass, some extra checks are needed, so
> it's not a 1:1 move.
>
> Signed-off-by: Maxime Ripard
>
I was hoping that someone else would step up and review these parser
patches since the parser code is rather difficult to read, for me at
least. I have studied it now, so I'll give it a try.
> ---
> Changes in v4:
> - Fold down all the named mode patches that were split into a single
> patch again to maintain bisectability
> ---
> drivers/gpu/drm/drm_modes.c | 73
> ++---
> 1 file changed, 62 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index c0dceff51cac..2f020ef2ddf2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -2229,6 +2229,55 @@ static const char * const drm_named_modes_whitelist[]
> = {
> "PAL",
> };
>
> +static int drm_mode_parse_cmdline_named_mode(const char *name,
> + unsigned int name_end,
> + struct drm_cmdline_mode
> *cmdline_mode)
> +{
> + unsigned int i;
> +
> + if (!name_end)
> + return 0;
name_end can't be zero since the argument is checked before calling this
function.
> +
> + /* If the name starts with a digit, it's not a named mode */
> + if (isdigit(name[0]))
> + return 0;
> +
> + /*
> + * If there's an equal sign in the name, the command-line
> + * contains only an option and no mode.
> + */
> + if (strnchr(name, name_end, '='))
> + return 0;
I think this check actually belongs in
drm_mode_parse_command_line_for_connector() after options_off is set. If
theres an equal sign it should skip all mode parsing and skip down to
drm_mode_parse_cmdline_options(). Which probably means that the mode
parsing should have been moved out to separate function to avoid using a
goto.
But that's probably beyond the scope of this patchset :)
> +
> +#define STR_STRICT_EQ(str, len, cmp) \
> + (str_has_prefix(str, cmp) == len)
> +
> + /* The connection status extras can be set without a mode. */
> + if (STR_STRICT_EQ(name, name_end, "d") ||
> + STR_STRICT_EQ(name, name_end, "D") ||
> + STR_STRICT_EQ(name, name_end, "e"))
> + return 0;
It took me a while to understand what is going on here.
If str_has_prefix() finds a match it returns strlen(prefix). Since
prefix is always of length 1, name_end has to always be 1 for the
statement to be true.
I would have written it like this:
/* The connection status extras can be set without a mode. */
if (name_end == 1) {
if (name[0] == "d" || name[0] == "D" || name[0] == "e")
return 0;
}
> +
> + /*
> + * We're sure we're a named mode at that point, iterate over the
that -> this ?
> + * list of modes we're aware of.
> + */
> + for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> + int ret;
> +
> + ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> + if (ret != name_end)
> + continue;
> +
> + strcpy(cmdline_mode->name, drm_named_modes_whitelist[i]);
> + cmdline_mode->specified = true;
> +
> + return 1;
> + }
> +
> + return -EINVAL;
> +}
> +
> /**
> * drm_mode_parse_command_line_for_connector - parse command line modeline
> for connector
> * @mode_option: optional per connector mode option
> @@ -2265,7 +2314,7 @@ bool drm_mode_parse_command_line_for_connector(const
> char *mode_option,
> const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
> const char *options_ptr = NULL;
> char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
> - int i, len, ret;
> + int len, ret;
>
> memset(mode, 0, sizeof(*mode));
> mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
> @@ -2306,17 +2355,19 @@ bool drm_mode_parse_command_line_for_connector(const
> char *mode_option,
> parse_extras = true;
> }
>
> - /* First check for a named mode */
> - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
> - ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
> - if (ret == mode_end) {
> - if (refresh_ptr)
> - return false; /* named + refresh is invalid */
>
> - strcpy(mode->name, drm_named_modes_whitelist[i]);
> - mode->specified = true;
> - break;
> - }
> + if (mode_end) {
Shouldn't this be:
if (!mode_end)
Re: [Intel-gfx] [PATCH v5 07/22] drm/client: Add some tests for drm_connector_pick_cmdline_mode()
Den 13.10.2022 15.18, skrev Maxime Ripard: > drm_connector_pick_cmdline_mode() is in charge of finding a proper > drm_display_mode from the definition we got in the video= command line > argument. > > Let's add some unit tests to make sure we're not getting any regressions > there. > > Signed-off-by: Maxime Ripard > > --- > Changes in v5: > - Removed useless (for now) count and modes intermediate variables in > get_modes > - Switched to kunit assertions in test init, and to KUNIT_ASSERT_NOT_NULL > instead of KUNIT_ASSERT_PTR_NE(..., NULL) > > Changes in v4: > - Removed MODULE macros > --- > drivers/gpu/drm/drm_client_modeset.c| 4 + > drivers/gpu/drm/tests/drm_client_modeset_test.c | 99 > + > 2 files changed, 103 insertions(+) > > diff --git a/drivers/gpu/drm/drm_client_modeset.c > b/drivers/gpu/drm/drm_client_modeset.c > index bbc535cc50dd..d553e793e673 100644 > --- a/drivers/gpu/drm/drm_client_modeset.c > +++ b/drivers/gpu/drm/drm_client_modeset.c > @@ -1237,3 +1237,7 @@ int drm_client_modeset_dpms(struct drm_client_dev > *client, int mode) > return ret; > } > EXPORT_SYMBOL(drm_client_modeset_dpms); > + > +#ifdef CONFIG_DRM_KUNIT_TEST > +#include "tests/drm_client_modeset_test.c" > +#endif I can't say I like including the file like this, but exporting the static function for testing isn't attractive either and doing it like this is shown in the kunit docs, so: Acked-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v5 01/22] drm/tests: Add Kunit Helpers
Den 13.10.2022 15.18, skrev Maxime Ripard: > As the number of kunit tests in KMS grows further, we start to have > multiple test suites that, for example, need to register a mock DRM > driver to interact with the KMS function they are supposed to test. > > Let's add a file meant to provide those kind of helpers to avoid > duplication. > > Signed-off-by: Maxime Ripard > > --- Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Den 13.10.2022 10.36, skrev Maxime Ripard: > Hi Noralf, > > On Sat, Oct 01, 2022 at 02:52:06PM +0200, Noralf Trønnes wrote: >> Den 29.09.2022 18.31, 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 >>> >>> --- >>> >>> Changes in v4: >>> - Reworded the line length check comment >>> - Switch to HZ_PER_KHZ in tests >>> - Use previous timing to fill our mode >>> - Move the number of lines check earlier >>> --- >>> drivers/gpu/drm/drm_modes.c| 474 >>> + >>> drivers/gpu/drm/tests/Makefile | 1 + >>> drivers/gpu/drm/tests/drm_modes_test.c | 144 ++ >>> include/drm/drm_modes.h| 17 ++ >>> 4 files changed, 636 insertions(+) >>> >> >> I haven't followed the discussion on this patch, but it seems rather >> excessive to add over 600 lines of code (including tests) to add 2 fixed >> modes. And it's very difficult to see from the code what the actual >> display mode timings really are, which would be useful for other >> developers down the road wanting to use them. >> >> Why not just hardcode the modes? > > Yeah, I have kind of the same feeling tbh, but it was asked back on the > v1 to ease the transition of old fbdev drivers, since they will need > such a function: > https://lore.kernel.org/dri-devel/CAMuHMdUrwzPYjA0wdR7ADj5Ov6+m03JbnY8fBYzRYyWDuNm5=g...@mail.gmail.com/ > If that's the case I suggest you just hardcode them for now and leave the complexity to the developer doing the actual conversion of the fbdev driver. Who knows when that will happen, but that person will have your well documented and discussed work to fall back on. Noralf.
Re: [Intel-gfx] [PATCH v4 00/30] drm: Analog TV Improvements
Den 29.09.2022 18.30, skrev Maxime Ripard: > Hi, > > Here's a series aiming at improving the command line named modes support, > and more importantly how we deal with all the analog TV variants. > > The named modes support were initially introduced to allow to specify the > analog TV mode to be used. > > However, this was causing multiple issues: > > * The mode name parsed on the command line was passed directly to the > driver, which had to figure out which mode it was suppose to match; > > * Figuring that out wasn't really easy, since the video= argument or what > the userspace might not even have a name in the first place, but > instead could have passed a mode with the same timings; > > * The fallback to matching on the timings was mostly working as long as > we were supporting one 525 lines (most likely NSTC) and one 625 lines > (PAL), but couldn't differentiate between two modes with the same > timings (NTSC vs PAL-M vs NSTC-J for example); > > * There was also some overlap with the tv mode property registered by > drm_mode_create_tv_properties(), but named modes weren't interacting > with that property at all. > > * Even though that property was generic, its possible values were > specific to each drivers, which made some generic support difficult. > > Thus, I chose to tackle in multiple steps: > > * A new TV mode property was introduced, with generic values, each driver > reporting through a bitmask what standard it supports to the userspace; > > * This option was added to the command line parsing code to be able to > specify it on the kernel command line, and new atomic_check and reset > helpers were created to integrate properly into atomic KMS; > > * The named mode parsing code is now creating a proper display mode for > the given named mode, and the TV standard will thus be part of the > connector state; > > * Two drivers were converted and tested for now (vc4 and sun4i), with > some backward compatibility code to translate the old TV mode to the > new TV mode; > > Unit tests were created along the way. > > One can switch from NTSC to PAL now using (on vc4) > > modetest -M vc4 -s 53:720x480i -w 53:'TV mode':1 # NTSC > modetest -M vc4 -s 53:720x576i -w 53:'TV mode':4 # PAL > > Let me know what you think, > Maxime > I suggest that you apply the patches that are reviewed, have merrit on their own and are not tied to the TV mode property. This will help in keeping this rather big patchset focused and ease the task for reviewers. The following seems to be in that group: drm/tests: Order Kunit tests in Makefile drm/atomic-helper: Rename drm_atomic_helper_connector_tv_reset to avoid ambiguity drm/connector: Rename subconnector state variable drm/atomic: Add TV subconnector property to get/set_property drm/modes: Only consider bpp and refresh before options drm/modes: parse_cmdline: Add support for named modes containing dashes drm/vc4: vec: Fix definition of PAL-M mode Noralf.
Re: [Intel-gfx] [PATCH v4 11/30] drm/modes: Add a function to generate analog display modes
Den 29.09.2022 18.31, 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
>
> ---
>
> Changes in v4:
> - Reworded the line length check comment
> - Switch to HZ_PER_KHZ in tests
> - Use previous timing to fill our mode
> - Move the number of lines check earlier
> ---
> drivers/gpu/drm/drm_modes.c| 474
> +
> drivers/gpu/drm/tests/Makefile | 1 +
> drivers/gpu/drm/tests/drm_modes_test.c | 144 ++
> include/drm/drm_modes.h| 17 ++
> 4 files changed, 636 insertions(+)
>
I haven't followed the discussion on this patch, but it seems rather
excessive to add over 600 lines of code (including tests) to add 2 fixed
modes. And it's very difficult to see from the code what the actual
display mode timings really are, which would be useful for other
developers down the road wanting to use them.
Why not just hardcode the modes?
Noralf.
> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
> index 304004fb80aa..7cf2fe98d7d2 100644
> --- a/drivers/gpu/drm/drm_modes.c
> +++ b/drivers/gpu/drm/drm_modes.c
> @@ -116,6 +116,480 @@ void drm_mode_probed_add(struct drm_connector
> *connector,
> }
> EXPORT_SYMBOL(drm_mode_probed_add);
>
> +enum drm_mode_analog {
> + DRM_MODE_ANALOG_NTSC, /* 525 lines, 60Hz */
> + DRM_MODE_ANALOG_PAL, /* 625 lines, 50Hz */
> +};
> +
> +/*
> + * 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_NS63556U
> +#define NTSC_LINES_NUMBER525
> +
> +#define NTSC_HBLK_DURATION_TYP_NS10900U
> +#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_NS4700
> +#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)
> +
> +struct analog_param_field {
> + unsigned int even, odd;
> +};
> +
> +#define PARAM_FIELD(_odd, _even) \
> + { .even = _even, .odd = _odd }
> +
> +struct analog_param_range {
> + unsigned intmin, typ, max;
>
Re: [Intel-gfx] [PATCH v4 30/30] drm/sun4i: tv: Convert to the new TV mode property
Den 29.09.2022 18.31, skrev Maxime Ripard:
> Now that the core can deal fine with analog TV modes, let's convert the
> sun4i TV driver to leverage those new features.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/sun4i/sun4i_tv.c | 148
> ++-
> drivers/gpu/drm/vc4/vc4_vec.c| 5 +-
> 2 files changed, 54 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_tv.c
> b/drivers/gpu/drm/sun4i/sun4i_tv.c
> @@ -467,35 +398,46 @@ static const struct drm_encoder_helper_funcs
> sun4i_tv_helper_funcs = {
>
> static int sun4i_tv_comp_get_modes(struct drm_connector *connector)
> {
> - int i;
> + struct drm_display_mode *mode;
> + int count = 0;
>
> - for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
> - struct drm_display_mode *mode;
> - const struct tv_mode *tv_mode = _modes[i];
> -
> - mode = drm_mode_create(connector->dev);
> - if (!mode) {
> - DRM_ERROR("Failed to create a new display mode\n");
> - return 0;
> - }
> + mode = drm_mode_analog_ntsc_480i(connector->dev);
> + if (!mode) {
> + DRM_ERROR("Failed to create a new display mode\n");
> + return -ENOMEM;
> + }
>
> - strcpy(mode->name, tv_mode->name);
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> + drm_mode_probed_add(connector, mode);
> + count += 1;
>
> - sun4i_tv_mode_to_drm_mode(tv_mode, mode);
> - drm_mode_probed_add(connector, mode);
> + mode = drm_mode_analog_pal_576i(connector->dev);
> + if (!mode) {
> + DRM_ERROR("Failed to create a new display mode\n");
> + return -ENOMEM;
> }
>
> - return i;
> + drm_mode_probed_add(connector, mode);
> + count += 1;
> +
> + return count;
count is always 2 so you can just return 2.
Acked-by: Noralf Trønnes
> }
This stray hunk belongs to the vc4 TV mode patch I guess:
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
> index 8d37d7ba9b2a..88b4330bfa39 100644
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -322,7 +322,7 @@ vc4_vec_tv_mode_lookup(unsigned int mode)
> return NULL;
> }
>
> -static const struct drm_prop_enum_list tv_mode_names[] = {
> +static const struct drm_prop_enum_list legacy_tv_mode_names[] = {
> { VC4_VEC_TV_MODE_NTSC, "NTSC", },
> { VC4_VEC_TV_MODE_NTSC_443, "NTSC-443", },
> { VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", },
> @@ -498,7 +498,8 @@ static int vc4_vec_connector_init(struct drm_device *dev,
> struct vc4_vec *vec)
> DRM_MODE_TV_MODE_NTSC);
>
> prop = drm_property_create_enum(dev, 0, "mode",
> - tv_mode_names,
> ARRAY_SIZE(tv_mode_names));
> + legacy_tv_mode_names,
> + ARRAY_SIZE(legacy_tv_mode_names));
> if (!prop)
> return -ENOMEM;
> vec->legacy_tv_mode_property = prop;
>
Noralf.
Re: [Intel-gfx] [PATCH v4 03/30] drm/tests: Add Kunit Helpers
Den 29.09.2022 18.30, skrev Maxime Ripard: > As the number of kunit tests in KMS grows further, we start to have > multiple test suites that, for example, need to register a mock DRM > driver to interact with the KMS function they are supposed to test. > > Let's add a file meant to provide those kind of helpers to avoid > duplication. > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v4 10/30] drm/connector: Add TV standard property
Den 29.09.2022 18.31, skrev Maxime Ripard:
> The TV mode property has been around for a while now to select and get the
> current TV mode output on an analog TV connector.
>
> Despite that property name being generic, its content isn't and has been
> driver-specific which makes it hard to build any generic behaviour on top
> of it, both in kernel and user-space.
>
> Let's create a new enum tv norm property, that can contain any of the
> analog TV standards currently supported by kernel drivers. Each driver can
> then pass in a bitmask of the modes it supports, and the property
> creation function will filter out the modes not supported.
>
> We'll then be able to phase out the older tv mode property.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v4:
> - Add property documentation to kms-properties.csv
> - Fix documentation
> ---
> Documentation/gpu/kms-properties.csv | 1 +
> drivers/gpu/drm/drm_atomic_uapi.c| 4 +++
> drivers/gpu/drm/drm_connector.c | 57 +++-
> include/drm/drm_connector.h | 64
>
> include/drm/drm_mode_config.h| 8 +
> 5 files changed, 133 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/kms-properties.csv
> b/Documentation/gpu/kms-properties.csv
> index 45c12e3e82f4..3498bd5d5856 100644
> --- a/Documentation/gpu/kms-properties.csv
> +++ b/Documentation/gpu/kms-properties.csv
> @@ -91,6 +91,7 @@ omap,Generic,“zorder”,RANGE,"Min=0, Max=3","CRTC, Plane",TBD
> qxl,Generic,"“hotplug_mode_update""",RANGE,"Min=0, Max=1",Connector,TBD
> radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD
> ,DAC enable load detect,“load detection”,RANGE,"Min=0, Max=1",Connector,TBD
> +,TV Mode,"""TV Mode""",ENUM,"{ ""NTSC"", ""NTSC-443"", ""NTSC-J"", ""PAL"",
> ""PAL-M"", ""PAL-N"", ""SECAM"" }",Connector,TBD
> ,legacy TMDS PLL detect,"""tmds_pll""",ENUM,"{ ""driver"", ""bios"" }",-,TBD
> ,Underscan,"""underscan""",ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> ,,"""underscan hborder""",RANGE,"Min=0, Max=128",Connector,TBD
Turns out I was wrong about adding the property to this file, Daniel
says it's deprecated in f0f0657b108c ("drm/doc: Drop "content type" from
the legacy kms property table").
If you look at the Fixes commit it adds a kernel doc HDMI property
section and TV should probably have something like that.
Noralf.
Re: [Intel-gfx] [PATCH v4 01/30] drm/docs: Remove unused TV Standard property
Den 29.09.2022 18.30, skrev Maxime Ripard:
> That property is not used or exposed by any driver in the kernel. Remove
> it from the documentation.
>
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v4:
> - New patch
> ---
> Documentation/gpu/kms-properties.csv | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/Documentation/gpu/kms-properties.csv
> b/Documentation/gpu/kms-properties.csv
> index 07ed22ea3bd6..45c12e3e82f4 100644
> --- a/Documentation/gpu/kms-properties.csv
> +++ b/Documentation/gpu/kms-properties.csv
> @@ -91,7 +91,6 @@ omap,Generic,“zorder”,RANGE,"Min=0, Max=3","CRTC, Plane",TBD
> qxl,Generic,"“hotplug_mode_update""",RANGE,"Min=0, Max=1",Connector,TBD
> radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD
> ,DAC enable load detect,“load detection”,RANGE,"Min=0, Max=1",Connector,TBD
> -,TV Standard,"""tv standard""",ENUM,"{ ""ntsc"", ""pal"", ""pal-m"",
> ""pal-60"", ""ntsc-j"" , ""scart-pal"", ""pal-cn"", ""secam"" }",Connector,TBD
This property is listed under radeon and it is used in
drivers/gpu/drm/radeon/radeon_display.c
Noralf.
> ,legacy TMDS PLL detect,"""tmds_pll""",ENUM,"{ ""driver"", ""bios"" }",-,TBD
> ,Underscan,"""underscan""",ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> ,,"""underscan hborder""",RANGE,"Min=0, Max=128",Connector,TBD
>
Re: [Intel-gfx] [PATCH v2 09/33] drm/connector: Add TV standard property
Den 26.09.2022 12.01, skrev Maxime Ripard:
> On Sat, Sep 24, 2022 at 05:52:29PM +0200, Noralf Trønnes wrote:
>> Den 22.09.2022 16.25, skrev Maxime Ripard:
>>> The TV mode property has been around for a while now to select and get the
>>> current TV mode output on an analog TV connector.
>>>
>>> Despite that property name being generic, its content isn't and has been
>>> driver-specific which makes it hard to build any generic behaviour on top
>>> of it, both in kernel and user-space.
>>>
>>> Let's create a new enum tv norm property, that can contain any of the
>>> analog TV standards currently supported by kernel drivers. Each driver can
>>> then pass in a bitmask of the modes it supports, and the property
>>> creation function will filter out the modes not supported.
>>>
>>> We'll then be able to phase out the older tv mode property.
>>>
>>> Signed-off-by: Maxime Ripard
>>
>> Please can you add per patch changelogs, it's hard to review when I have
>> to recall what might have happened with each patch. If you do it drm
>> style and put in the commit message it should be easy enough to do.
>
> I certainly don't want to start that discussion, but I'm really not a
> fan of that format either. I'll do it for that series if you prefer.
>
The format isn't important, but especially a big series like this and
being weeks between each iteration it's difficult to follow and see
which review comments that you have chosen to implement and how. It's
almost a full review each time. Even if I see that I have acked/rewieved
a patch, if I don't remember, I have to go back to the previous version
and see if I had any comments and if you followed up on that.
>>> +/**
>>> + * enum drm_connector_tv_mode - Analog TV output mode
>>> + *
>>> + * This enum is used to indicate the TV output mode used on an analog TV
>>> + * connector.
>>> + *
>>> + * WARNING: The values of this enum is uABI since they're exposed in the
>>> + * "TV mode" connector property.
>>> + */
>>> +enum drm_connector_tv_mode {
>>> + /**
>>> +* @DRM_MODE_TV_MODE_NONE: Placeholder to not default on one
>>> +* variant or the other when nothing is set.
>>> +*/
>>> + DRM_MODE_TV_MODE_NONE = 0,
>>
>> How is this supposed to be used?
>
> It's not supposed to be used. It was a suggestion from Mateusz to avoid
> to default to any standard when we don't initialize something. I don't
> have any strong feeling about it, so I can drop it if you prefer.
>
The confusing thing to me is that "None" is part of the property enum
list, so the idea is that it can end up in userspace if there's a driver
error? Hmm, that won't work since TV_MODE_NONE won't be part of the
bitmask that the driver sets. So userspace reading the property ends up
with a value for which there's no enum name to match.
So usespace should be trained to know that zero for this property is a
driver error? No, not a good idea.
I think to catch a bug like this drm_atomic_connector_get_property()
should check the tv.mode value and see if it's a legal enum value and if
not it has to just pick a legal one and print an error. But I'm not sure
it's worth it to catch a bug like this. And I don't see any other enum
properties being checked for validity either before being returned to
userspace.
Based on this reasoning I think you should drop the NONE value.
Noralf.
Re: [Intel-gfx] [PATCH v2 02/33] drm/tests: Add Kunit Helpers
Den 26.09.2022 11.36, skrev Maxime Ripard:
> Hi Noralf,
>
> On Sat, Sep 24, 2022 at 08:06:17PM +0200, Noralf Trønnes wrote:
>> Den 24.09.2022 19.56, skrev Noralf Trønnes:
>>>
>>>
>>> Den 22.09.2022 16.25, skrev Maxime Ripard:
>>>> As the number of kunit tests in KMS grows further, we start to have
>>>> multiple test suites that, for example, need to register a mock DRM
>>>> driver to interact with the KMS function they are supposed to test.
>>>>
>>>> Let's add a file meant to provide those kind of helpers to avoid
>>>> duplication.
>>>>
>>>> Signed-off-by: Maxime Ripard
>>>>
>>>> diff --git a/drivers/gpu/drm/tests/Makefile
>>>> b/drivers/gpu/drm/tests/Makefile
>>>> index 2d9f49b62ecb..b29ef1085cad 100644
>>>> --- a/drivers/gpu/drm/tests/Makefile
>>>> +++ b/drivers/gpu/drm/tests/Makefile
>>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>>>>drm_format_helper_test.o \
>>>>drm_format_test.o \
>>>>drm_framebuffer_test.o \
>>>> + drm_kunit_helpers.o \
>>>>drm_mm_test.o \
>>>>drm_plane_helper_test.o \
>>>>drm_rect_test.o
>>>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
>>>> b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>>>> new file mode 100644
>>>> index ..7ebd620481c1
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>>>> @@ -0,0 +1,54 @@
>>>> +#include
>>>> +#include
>>>> +
>>>> +#include
>>>> +
>>>> +static const struct drm_mode_config_funcs drm_mode_config_funcs = {
>>>> +};
>>>> +
>>>> +static const struct drm_driver drm_mode_driver = {
>>>> +};
>>>> +
>>>> +static void drm_kunit_free_device(struct drm_device *drm, void *ptr)
>>>> +{
>>>> + struct device *dev = ptr;
>>>> +
>>>> + root_device_unregister(dev);
>>>> +}
>>>> +
>>>> +struct drm_device *drm_kunit_device_init(const char *name)
>>>> +{
>>>> + struct drm_device *drm;
>>>> + struct device *dev;
>>>> + int ret;
>>>> +
>>>> + dev = root_device_register(name);
>>>> + if (IS_ERR(dev))
>>>> + return ERR_CAST(dev);
>>>> +
>>>> + drm = drm_dev_alloc(_mode_driver, dev);
>>>
>>> I can't find drm being freed anywhere?
>>> Maybe you could assign it to drm->managed.final_kfree.
>
> There's a drm_dev_put in the test_exit hook which should free it.
>
I see now, there's a drmm_add_final_kfree() in drm_dev_alloc().
Noralf.
>> Perhaps a better solution would be to use devm_drm_dev_alloc() and
>> unregister the root device on exit. That avoids reaching into the drm
>> managed internals and it looks more like a regular driver.
>
> But yeah, this is a good idea, I'll do it.
>
> Maxime
Re: [Intel-gfx] [PATCH v2 32/33] drm/vc4: vec: Add support for more analog TV standards
Den 22.09.2022 16.25, skrev Maxime Ripard: > From: Mateusz Kwiatkowski > > Add support for the following composite output modes (all of them are > somewhat more obscure than the previously defined ones): > > - NTSC_443 - NTSC-style signal with the chroma subcarrier shifted to > 4.43361875 MHz (the PAL subcarrier frequency). Never used for > broadcasting, but sometimes used as a hack to play NTSC content in PAL > regions (e.g. on VCRs). > - PAL_N - PAL with alternative chroma subcarrier frequency, > 3.58205625 MHz. Used as a broadcast standard in Argentina, Paraguay > and Uruguay to fit 576i50 with colour in 6 MHz channel raster. > - PAL60 - 480i60 signal with PAL-style color at normal European PAL > frequency. Another non-standard, non-broadcast mode, used in similar > contexts as NTSC_443. Some displays support one but not the other. > - SECAM - French frequency-modulated analog color standard; also have > been broadcast in Eastern Europe and various parts of Africa and Asia. > Uses the same 576i50 timings as PAL. > > Also added some comments explaining color subcarrier frequency > registers. > > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > Acked-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 30/33] drm/vc4: vec: Check for VEC output constraints
Den 22.09.2022 16.25, skrev Maxime Ripard: > From: Mateusz Kwiatkowski > > The VEC can accept pretty much any relatively reasonable mode, but still > has a bunch of constraints to meet. > > Let's create an atomic_check() implementation that will make sure we > don't end up accepting a non-functional mode. > > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > Acked-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 06/33] drm/connector: Rename legacy TV property
Den 22.09.2022 16.25, skrev Maxime Ripard: > The current tv_mode has driver-specific values that don't allow to > easily share code using it, either at the userspace or kernel level. > > Since we're going to introduce a new, generic, property that fit the > same purpose, let's rename this one to legacy_tv_mode to make it > obvious we should move away from it. > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 28/33] drm/vc4: vec: Fix definition of PAL-M mode
Den 22.09.2022 16.25, skrev Maxime Ripard: > From: Mateusz Kwiatkowski > > PAL-M is a Brazilian analog TV standard that uses a PAL-style chroma > subcarrier at 3.575611[888111] MHz on top of 525-line (480i60) timings. > This commit makes the driver actually use the proper VEC preset for this > mode instead of just changing PAL subcarrier frequency. > > Signed-off-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > Acked-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 00/33] drm: Analog TV Improvements
Den 22.09.2022 16.25, skrev Maxime Ripard: > Hi, > > Here's a series aiming at improving the command line named modes support, > and more importantly how we deal with all the analog TV variants. > > The named modes support were initially introduced to allow to specify the > analog TV mode to be used. > > However, this was causing multiple issues: > > * The mode name parsed on the command line was passed directly to the > driver, which had to figure out which mode it was suppose to match; > > * Figuring that out wasn't really easy, since the video= argument or what > the userspace might not even have a name in the first place, but > instead could have passed a mode with the same timings; > > * The fallback to matching on the timings was mostly working as long as > we were supporting one 525 lines (most likely NSTC) and one 625 lines > (PAL), but couldn't differentiate between two modes with the same > timings (NTSC vs PAL-M vs NSTC-J for example); > > * There was also some overlap with the tv mode property registered by > drm_mode_create_tv_properties(), but named modes weren't interacting > with that property at all. > > * Even though that property was generic, its possible values were > specific to each drivers, which made some generic support difficult. > > Thus, I chose to tackle in multiple steps: > > * A new TV mode property was introduced, with generic values, each driver > reporting through a bitmask what standard it supports to the userspace; > > * This option was added to the command line parsing code to be able to > specify it on the kernel command line, and new atomic_check and reset > helpers were created to integrate properly into atomic KMS; > > * The named mode parsing code is now creating a proper display mode for > the given named mode, and the TV standard will thus be part of the > connector state; > > * Two drivers were converted and tested for now (vc4 and sun4i), with > some backward compatibility code to translate the old TV mode to the > new TV mode; > > Unit tests were created along the way. > > One can switch from NTSC to PAL now using (on vc4) > > modetest -M vc4 -s 53:720x480i -w 53:'TV mode':1 # NTSC > modetest -M vc4 -s 53:720x576i -w 53:'TV mode':4 # PAL > > Let me know what you think, > Maxime > drivers/gpu/drm/drm_atomic_state_helper.c | 128 - > drivers/gpu/drm/drm_atomic_uapi.c | 8 + > drivers/gpu/drm/drm_client_modeset.c| 4 + > drivers/gpu/drm/drm_connector.c | 111 +++- > drivers/gpu/drm/drm_modes.c | 658 > +++- > drivers/gpu/drm/gud/gud_connector.c | 12 +- > drivers/gpu/drm/i2c/ch7006_drv.c| 6 +- > drivers/gpu/drm/i915/display/intel_tv.c | 5 +- > drivers/gpu/drm/nouveau/dispnv04/tvnv17.c | 6 +- > drivers/gpu/drm/sun4i/sun4i_tv.c| 148 ++ > drivers/gpu/drm/tests/Makefile | 16 +- > drivers/gpu/drm/tests/drm_client_modeset_test.c | 239 + > drivers/gpu/drm/tests/drm_cmdline_parser_test.c | 67 +++ > drivers/gpu/drm/tests/drm_kunit_helpers.c | 54 ++ > drivers/gpu/drm/tests/drm_kunit_helpers.h | 9 + > drivers/gpu/drm/tests/drm_modes_test.c | 136 + > drivers/gpu/drm/vc4/vc4_hdmi.c | 2 +- > drivers/gpu/drm/vc4/vc4_vec.c | 339 ++-- > include/drm/drm_atomic_state_helper.h | 4 + > include/drm/drm_connector.h | 92 +++- > include/drm/drm_mode_config.h | 12 +- > include/drm/drm_modes.h | 17 + These also needs updating: Documentation/gpu/kms-properties.csv Documentation/fb/modedb.rst Noralf.
Re: [Intel-gfx] [PATCH v2 02/33] drm/tests: Add Kunit Helpers
Den 22.09.2022 16.25, skrev Maxime Ripard:
> As the number of kunit tests in KMS grows further, we start to have
> multiple test suites that, for example, need to register a mock DRM
> driver to interact with the KMS function they are supposed to test.
>
> Let's add a file meant to provide those kind of helpers to avoid
> duplication.
>
> Signed-off-by: Maxime Ripard
>
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index 2d9f49b62ecb..b29ef1085cad 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
> drm_format_helper_test.o \
> drm_format_test.o \
> drm_framebuffer_test.o \
> + drm_kunit_helpers.o \
> drm_mm_test.o \
> drm_plane_helper_test.o \
> drm_rect_test.o
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
> b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> new file mode 100644
> index ..7ebd620481c1
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
> @@ -0,0 +1,54 @@
> +#include
> +#include
> +
> +#include
> +
> +static const struct drm_mode_config_funcs drm_mode_config_funcs = {
> +};
> +
> +static const struct drm_driver drm_mode_driver = {
> +};
> +
> +static void drm_kunit_free_device(struct drm_device *drm, void *ptr)
> +{
> + struct device *dev = ptr;
> +
> + root_device_unregister(dev);
> +}
> +
> +struct drm_device *drm_kunit_device_init(const char *name)
> +{
> + struct drm_device *drm;
> + struct device *dev;
> + int ret;
> +
> + dev = root_device_register(name);
> + if (IS_ERR(dev))
> + return ERR_CAST(dev);
> +
> + drm = drm_dev_alloc(_mode_driver, dev);
I can't find drm being freed anywhere?
Maybe you could assign it to drm->managed.final_kfree.
Noralf.
> + if (IS_ERR(drm)) {
> + root_device_unregister(dev);
> + return ERR_CAST(drm);
> + }
> + drm->mode_config.funcs = _mode_config_funcs;
> +
> + ret = drmm_add_action_or_reset(drm, drm_kunit_free_device, dev);
> + if (ret)
> + goto err_put_device;
> +
> + ret = drmm_mode_config_init(drm);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return drm;
> +
> +err_put_device:
> + drm_dev_put(drm);
> + return ERR_PTR(ret);
> +}
> +
> +void drm_kunit_device_exit(struct drm_device *drm)
> +{
> + drm_dev_put(drm);
> +}
> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.h
> b/drivers/gpu/drm/tests/drm_kunit_helpers.h
> new file mode 100644
> index ..5015a327a8c1
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.h
> @@ -0,0 +1,9 @@
> +#ifndef DRM_KUNIT_HELPERS_H_
> +#define DRM_KUNIT_HELPERS_H_
> +
> +struct drm_device;
> +
> +struct drm_device *drm_kunit_device_init(const char *name);
> +void drm_kunit_device_exit(struct drm_device *drm);
> +
> +#endif // DRM_KUNIT_HELPERS_H_
>
Re: [Intel-gfx] [PATCH v2 02/33] drm/tests: Add Kunit Helpers
Den 24.09.2022 19.56, skrev Noralf Trønnes:
>
>
> Den 22.09.2022 16.25, skrev Maxime Ripard:
>> As the number of kunit tests in KMS grows further, we start to have
>> multiple test suites that, for example, need to register a mock DRM
>> driver to interact with the KMS function they are supposed to test.
>>
>> Let's add a file meant to provide those kind of helpers to avoid
>> duplication.
>>
>> Signed-off-by: Maxime Ripard
>>
>> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
>> index 2d9f49b62ecb..b29ef1085cad 100644
>> --- a/drivers/gpu/drm/tests/Makefile
>> +++ b/drivers/gpu/drm/tests/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>> drm_format_helper_test.o \
>> drm_format_test.o \
>> drm_framebuffer_test.o \
>> +drm_kunit_helpers.o \
>> drm_mm_test.o \
>> drm_plane_helper_test.o \
>> drm_rect_test.o
>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> new file mode 100644
>> index ..7ebd620481c1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.c
>> @@ -0,0 +1,54 @@
>> +#include
>> +#include
>> +
>> +#include
>> +
>> +static const struct drm_mode_config_funcs drm_mode_config_funcs = {
>> +};
>> +
>> +static const struct drm_driver drm_mode_driver = {
>> +};
>> +
>> +static void drm_kunit_free_device(struct drm_device *drm, void *ptr)
>> +{
>> +struct device *dev = ptr;
>> +
>> +root_device_unregister(dev);
>> +}
>> +
>> +struct drm_device *drm_kunit_device_init(const char *name)
>> +{
>> +struct drm_device *drm;
>> +struct device *dev;
>> +int ret;
>> +
>> +dev = root_device_register(name);
>> +if (IS_ERR(dev))
>> +return ERR_CAST(dev);
>> +
>> +drm = drm_dev_alloc(_mode_driver, dev);
>
> I can't find drm being freed anywhere?
> Maybe you could assign it to drm->managed.final_kfree.
>
Perhaps a better solution would be to use devm_drm_dev_alloc() and
unregister the root device on exit. That avoids reaching into the drm
managed internals and it looks more like a regular driver.
> Noralf.
>
>> +if (IS_ERR(drm)) {
>> +root_device_unregister(dev);
>> +return ERR_CAST(drm);
>> +}
>> +drm->mode_config.funcs = _mode_config_funcs;
>> +
>> +ret = drmm_add_action_or_reset(drm, drm_kunit_free_device, dev);
>> +if (ret)
>> +goto err_put_device;
>> +
>> +ret = drmm_mode_config_init(drm);
>> +if (ret)
>> +return ERR_PTR(ret);
>> +
>> +return drm;
>> +
>> +err_put_device:
>> +drm_dev_put(drm);
>> +return ERR_PTR(ret);
>> +}
>> +
>> +void drm_kunit_device_exit(struct drm_device *drm)
>> +{
>> +drm_dev_put(drm);
>> +}
>> diff --git a/drivers/gpu/drm/tests/drm_kunit_helpers.h
>> b/drivers/gpu/drm/tests/drm_kunit_helpers.h
>> new file mode 100644
>> index ..5015a327a8c1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tests/drm_kunit_helpers.h
>> @@ -0,0 +1,9 @@
>> +#ifndef DRM_KUNIT_HELPERS_H_
>> +#define DRM_KUNIT_HELPERS_H_
>> +
>> +struct drm_device;
>> +
>> +struct drm_device *drm_kunit_device_init(const char *name);
>> +void drm_kunit_device_exit(struct drm_device *drm);
>> +
>> +#endif // DRM_KUNIT_HELPERS_H_
>>
Re: [Intel-gfx] [PATCH v2 31/33] drm/vc4: vec: Convert to the new TV mode property
);
> return -ENOMEM;
> }
>
> + mode->type |= DRM_MODE_TYPE_PREFERRED;
> drm_mode_probed_add(connector, mode);
> + count += 1;
>
> - return 1;
> + mode = drm_mode_analog_pal_576i(connector->dev);
> + if (!mode) {
> + DRM_ERROR("Failed to create a new display mode\n");
> + return -ENOMEM;
> + }
> +
> + drm_mode_probed_add(connector, mode);
> + count += 1;
> +
> + return count;
Why not just return 2 here since that's the only value count can be.
Acked-by: Noralf Trønnes
> +}
> +
> +static int
> +vc4_vec_connector_set_property(struct drm_connector *connector,
> +struct drm_connector_state *state,
> +struct drm_property *property,
> +uint64_t val)
> +{
> + struct vc4_vec *vec = connector_to_vc4_vec(connector);
> +
> + if (property != vec->legacy_tv_mode_property)
> + return -EINVAL;
> +
> + switch (val) {
> + case VC4_VEC_TV_MODE_NTSC:
> + state->tv.mode = DRM_MODE_TV_MODE_NTSC;
> + break;
> +
> + case VC4_VEC_TV_MODE_NTSC_J:
> + state->tv.mode = DRM_MODE_TV_MODE_NTSC_J;
> + break;
> +
> + case VC4_VEC_TV_MODE_PAL:
> + state->tv.mode = DRM_MODE_TV_MODE_PAL;
> + break;
> +
> + case VC4_VEC_TV_MODE_PAL_M:
> + state->tv.mode = DRM_MODE_TV_MODE_PAL_M;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +vc4_vec_connector_get_property(struct drm_connector *connector,
> +const struct drm_connector_state *state,
> +struct drm_property *property,
> +uint64_t *val)
> +{
> + struct vc4_vec *vec = connector_to_vc4_vec(connector);
> +
> + if (property != vec->legacy_tv_mode_property)
> + return -EINVAL;
> +
> + switch (state->tv.mode) {
> + case DRM_MODE_TV_MODE_NTSC:
> + *val = VC4_VEC_TV_MODE_NTSC;
> + break;
> +
> + case DRM_MODE_TV_MODE_NTSC_J:
> + *val = VC4_VEC_TV_MODE_NTSC_J;
> + break;
> +
> + case DRM_MODE_TV_MODE_PAL:
> + *val = VC4_VEC_TV_MODE_PAL;
> + break;
> +
> + case DRM_MODE_TV_MODE_PAL_M:
> + *val = VC4_VEC_TV_MODE_PAL_M;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> }
>
> static const struct drm_connector_funcs vc4_vec_connector_funcs = {
> @@ -297,15 +393,19 @@ static const struct drm_connector_funcs
> vc4_vec_connector_funcs = {
> .reset = vc4_vec_connector_reset,
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> + .atomic_get_property = vc4_vec_connector_get_property,
> + .atomic_set_property = vc4_vec_connector_set_property,
> };
>
> static const struct drm_connector_helper_funcs
> vc4_vec_connector_helper_funcs = {
> + .atomic_check = drm_atomic_helper_connector_tv_check,
> .get_modes = vc4_vec_connector_get_modes,
> };
>
> static int vc4_vec_connector_init(struct drm_device *dev, struct vc4_vec
> *vec)
> {
> struct drm_connector *connector = >connector;
> + struct drm_property *prop;
> int ret;
>
> connector->interlace_allowed = true;
> @@ -318,8 +418,16 @@ static int vc4_vec_connector_init(struct drm_device
> *dev, struct vc4_vec *vec)
> drm_connector_helper_add(connector, _vec_connector_helper_funcs);
>
> drm_object_attach_property(>base,
> -dev->mode_config.legacy_tv_mode_property,
> -VC4_VEC_TV_MODE_NTSC);
> +dev->mode_config.tv_mode_property,
> +DRM_MODE_TV_MODE_NTSC);
> +
> + prop = drm_property_create_enum(dev, 0, "mode",
> + tv_mode_names,
> ARRAY_SIZE(tv_mode_names));
> + if (!prop)
> + return -ENOMEM;
> + vec->legacy_tv_mode_property = prop;
> +
> + drm_object_attach_property(>base, prop,
> VC4_VEC_TV_MODE_NTSC);
>
> drm_connector_attach_encoder(connector, >encoder.base);
>
> @@ -366,13 +474,16 @@ static void vc4_vec_encoder_enable(struct drm_encoder
> *encoder,
> struct drm_con
Re: [Intel-gfx] [PATCH v2 09/33] drm/connector: Add TV standard property
Den 22.09.2022 16.25, skrev Maxime Ripard:
> The TV mode property has been around for a while now to select and get the
> current TV mode output on an analog TV connector.
>
> Despite that property name being generic, its content isn't and has been
> driver-specific which makes it hard to build any generic behaviour on top
> of it, both in kernel and user-space.
>
> Let's create a new enum tv norm property, that can contain any of the
> analog TV standards currently supported by kernel drivers. Each driver can
> then pass in a bitmask of the modes it supports, and the property
> creation function will filter out the modes not supported.
>
> We'll then be able to phase out the older tv mode property.
>
> Signed-off-by: Maxime Ripard
>
Please can you add per patch changelogs, it's hard to review when I have
to recall what might have happened with each patch. If you do it drm
style and put in the commit message it should be easy enough to do.
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 4e4fbc9e0049..e7aa8de08f5b 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -980,6 +980,18 @@ static const struct drm_prop_enum_list
> drm_dvi_i_subconnector_enum_list[] = {
> DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name,
>drm_dvi_i_subconnector_enum_list)
>
> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> + { DRM_MODE_TV_MODE_NONE, "None" },
> + { DRM_MODE_TV_MODE_NTSC, "NTSC" },
I think going back to plain NTSC and PAL is a good choice for the common
variants.
> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> + { DRM_MODE_TV_MODE_PAL, "PAL" },
> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> + { DRM_MODE_TV_MODE_SECAM, "SECAM" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> +
> static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
> { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
> @@ -1645,6 +1657,10 @@ EXPORT_SYMBOL(drm_mode_create_tv_margin_properties);
> * responsible for allocating a list of format names and passing them to
> * this routine.
> *
> + * NOTE: This functions registers the deprecated "mode" connector
> + * property to select the analog TV mode (ie, NTSC, PAL, etc.). New
> + * drivers must use drm_mode_create_tv_properties() instead.
> + *
> * Returns:
> * 0 on success or a negative error code on failure.
> */
> @@ -1686,7 +1702,6 @@ int drm_mode_create_tv_properties_legacy(struct
> drm_device *dev,
> if (drm_mode_create_tv_margin_properties(dev))
> goto nomem;
>
> -
> if (num_modes) {
> dev->mode_config.legacy_tv_mode_property =
> drm_property_create(dev, DRM_MODE_PROP_ENUM,
> @@ -1735,6 +1750,49 @@ int drm_mode_create_tv_properties_legacy(struct
> drm_device *dev,
> }
> EXPORT_SYMBOL(drm_mode_create_tv_properties_legacy);
>
> +/**
> + * drm_mode_create_tv_properties - create TV specific connector properties
> + * @dev: DRM device
> + * @supported_tv_modes: Bitmask of TV modes supported (See
> DRM_MODE_TV_MODE_*)
> +
> + * Called by a driver's TV initialization routine, this function creates
> + * the TV specific connector properties for a given device. Caller is
> + * responsible for allocating a list of format names and passing them to
> + * this routine.
Copy-paste error, there are no format names in this version.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_mode_create_tv_properties(struct drm_device *dev,
> + unsigned int supported_tv_modes)
> +{
> + struct drm_prop_enum_list tv_mode_list[DRM_MODE_TV_MODE_MAX];
> + struct drm_property *tv_mode;
> + unsigned int i, len = 0;
> +
> + if (dev->mode_config.tv_mode_property)
> + return 0;
> +
> + for (i = 0; i < DRM_MODE_TV_MODE_MAX; i++) {
> + if (!(supported_tv_modes & BIT(i)))
> + continue;
> +
> + tv_mode_list[len].type = i;
> + tv_mode_list[len].name = drm_get_tv_mode_name(i);
> + len++;
> + }
> +
> + tv_mode = drm_property_create_enum(dev, 0, "TV mode",
> +tv_mode_list, len);
> + if (!tv_mode)
> + return -ENOMEM;
> +
> + dev->mode_config.tv_mode_property = tv_mode;
> +
> + return drm_mode_create_tv_properties_legacy(dev, 0, NULL);
> +}
> +EXPORT_SYMBOL(drm_mode_create_tv_properties);
> +
> /**
> * drm_mode_create_scaling_mode_property - create scaling mode property
> * @dev: DRM device
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index d566b4a4709c..fffacbfd0a45 100644
> ---
Re: [Intel-gfx] [PATCH v2 08/33] drm/connector: Rename drm_mode_create_tv_properties
Den 22.09.2022 16.25, skrev Maxime Ripard: > drm_mode_create_tv_properties(), among other things, will create the > "mode" property that stores the analog TV mode that connector is > supposed to output. > > However, that property is getting deprecated, so let's rename that > function to mention it's deprecated. We'll introduce a new variant of > that function creating the property superseeding it in a later patch. > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 27/33] drm/atomic-helper: Add an analog TV atomic_check implementation
Den 22.09.2022 16.25, skrev Maxime Ripard:
> The analog TV connector drivers share some atomic_check logic, and the new
> TV standard property have created a bunch of new constraints that needs to
> be shared across drivers too.
The constraints part doesn't apply anymore after removing the display
mode check. It's only about detecting changes now.
Noralf.
>
> Let's create an atomic_check helper for those use cases.
>
> Reviewed-by: Noralf Trønnes
> Signed-off-by: Maxime Ripard
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index 0373c3dc824b..e88c57a4f7be 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -556,6 +556,55 @@ void drm_atomic_helper_connector_tv_reset(struct
> drm_connector *connector)
> }
> EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset);
>
> +/**
> + * @drm_atomic_helper_connector_tv_check: Validate an analog TV connector
> state
> + * @connector: DRM Connector
> + * @state: the DRM State object
> + *
> + * Checks the state object to see if the requested state is valid for an
> + * analog TV connector.
> + *
> + * Returns:
> + * Zero for success, a negative error code on error.
> + */
> +int drm_atomic_helper_connector_tv_check(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
> + struct drm_connector_state *old_conn_state =
> + drm_atomic_get_old_connector_state(state, connector);
> + struct drm_connector_state *new_conn_state =
> + drm_atomic_get_new_connector_state(state, connector);
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc *crtc;
> +
> + crtc = new_conn_state->crtc;
> + if (!crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> + if (!crtc_state)
> + return -EINVAL;
> +
> + if (old_conn_state->tv.mode != new_conn_state->tv.mode)
> + crtc_state->mode_changed = true;
> +
> + if ((old_conn_state->tv.margins.left !=
> new_conn_state->tv.margins.left) ||
> + (old_conn_state->tv.margins.right !=
> new_conn_state->tv.margins.right) ||
> + (old_conn_state->tv.margins.top != new_conn_state->tv.margins.top)
> ||
> + (old_conn_state->tv.margins.bottom !=
> new_conn_state->tv.margins.bottom) ||
> + (old_conn_state->tv.mode != new_conn_state->tv.mode) ||
> + (old_conn_state->tv.brightness != new_conn_state->tv.brightness) ||
> + (old_conn_state->tv.contrast != new_conn_state->tv.contrast) ||
> + (old_conn_state->tv.flicker_reduction !=
> new_conn_state->tv.flicker_reduction) ||
> + (old_conn_state->tv.overscan != new_conn_state->tv.overscan) ||
> + (old_conn_state->tv.saturation != new_conn_state->tv.saturation) ||
> + (old_conn_state->tv.hue != new_conn_state->tv.hue))
> + crtc_state->connectors_changed = true;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check);
> +
> /**
> * __drm_atomic_helper_connector_duplicate_state - copy atomic connector
> state
> * @connector: connector object
> diff --git a/include/drm/drm_atomic_state_helper.h
> b/include/drm/drm_atomic_state_helper.h
> index c8fbce795ee7..b9740edb2658 100644
> --- a/include/drm/drm_atomic_state_helper.h
> +++ b/include/drm/drm_atomic_state_helper.h
> @@ -26,6 +26,7 @@
>
> #include
>
> +struct drm_atomic_state;
> struct drm_bridge;
> struct drm_bridge_state;
> struct drm_crtc;
> @@ -71,6 +72,8 @@ void __drm_atomic_helper_connector_reset(struct
> drm_connector *connector,
>struct drm_connector_state
> *conn_state);
> void drm_atomic_helper_connector_reset(struct drm_connector *connector);
> void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector);
> +int drm_atomic_helper_connector_tv_check(struct drm_connector *connector,
> + struct drm_atomic_state *state);
> void drm_atomic_helper_connector_tv_margins_reset(struct drm_connector
> *connector);
> void
> __drm_atomic_helper_connector_duplicate_state(struct drm_connector
> *connector,
>
Re: [Intel-gfx] [PATCH v2 01/33] drm/tests: Order Kunit tests in Makefile
Den 22.09.2022 16.25, skrev Maxime Ripard: > Since we've recently added a ton of tests, the list starts to be a bit > of a mess and creates unneeded conflicts. > > Let's order it alphabetically. > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 21.09.2022 16.03, skrev Maxime Ripard:
> On Wed, Sep 07, 2022 at 06:44:53PM +0200, Noralf Trønnes wrote:
>>
>>
>> Den 07.09.2022 12.36, skrev Stefan Wahren:
>>> Hi Maxime,
>>>
>>> Am 05.09.22 um 16:57 schrieb Maxime Ripard:
>>>> On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote:
>>>>>
>>>>> Den 01.09.2022 21.35, skrev Noralf Trønnes:
>>>>>>
>>>>>> I have finally found a workaround for my kernel hangs.
>>>>>>
>>>>>> Dom had a look at my kernel and found that the VideoCore was fine, and
>>>>>> he said this:
>>>>>>
>>>>>>> That suggests cause of lockup was on arm side rather than VC side.
>>>>>>>
>>>>>>> But it's hard to diagnose further. Once you've had a peripheral not
>>>>>>> respond, the AXI bus locks up and no further operations are possible.
>>>>>>> Usual causes of this are required clocks being stopped or domains
>>>>>>> disabled and then trying to access the hardware.
>>>>>>>
>>>>>> So when I got this on my 64-bit build:
>>>>>>
>>>>>> [ 166.702171] SError Interrupt on CPU1, code 0xbf02 --
>>>>>> SError
>>>>>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: G W
>>>>>> 5.19.0-rc6-00096-gba7973977976-dirty #1
>>>>>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT)
>>>>>> [ 166.702206] Workqueue: events_freezable_power_
>>>>>> thermal_zone_device_check
>>>>>> [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS
>>>>>> BTYPE=--)
>>>>>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28
>>>>>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70
>>>>>> ...
>>>>>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal]
>>>>>>
>>>>>> I wondered if that reg read was stalled due to a clock being stopped.
>>>>>>
>>>>>> Lo and behold, disabling runtime pm and keeping the vec clock running
>>>>>> all the time fixed it[1].
>>>>>>
>>>>>> I don't know what the problem is, but at least I can now test this
>>>>>> patchset.
>>>>>>
>>>>>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065
>>>>>>
>>>>> It turns out I didn't have to disable runtime pm:
>>>>> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2
>>>> If the bcm2711_thermal IP needs that clock to be enabled, it should grab
>>>> a reference itself, but it looks like even the device tree binding
>>>> doesn't ask for one.
>>> The missing clock in the device tree binding is expected, because
>>> despite of the code there is not much information about the BCM2711
>>> clock tree. But i'm skeptical that the AVS IP actually needs the VEC
>>> clock, i think it's more likely that the VEC clock parent is changed and
>>> that cause this issue. I could take care of the bcm2711 binding & driver
>>> if i know which clock is really necessary.
>>
>> Seems you're right, keeping the parent always enabled is enough:
>>
>> clk_prepare_enable(clk_get_parent(vec->clock)); // pllc_per
>>
>> I tried enabling just the grandparent clock as well, but that didn't help.
>
> Yeah, adding tracing to the clock framework shows that it indeed
> disables PLLC_PER. So there's probably some other device that depends on
> it but doesn't take a reference to it.
>
> I had a look, but it's not really obvious what that might be.
>
> This patch makes sure that the PLL*_PER are never disabled, could you
> test it? It seems to work for me.
>
>
> diff --git a/drivers/clk/bcm/clk-bcm2835.c b/drivers/clk/bcm/clk-bcm2835.c
> index 48a1eb9f2d55..3839261ee419 100644
> --- a/drivers/clk/bcm/clk-bcm2835.c
> +++ b/drivers/clk/bcm/clk-bcm2835.c
> @@ -1675,7 +1675,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] =
> {
> .load_mask = CM_PLLA_LOADPER,
> .hold_mask = CM_PLLA_HOLDPER,
> .fixed_divider = 1,
> - .flags = CLK_SET_RATE_PARENT),
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> [BCM2835_PLLA_DSI0] = REGISTER_PLL_DIV(
> SOC_ALL,
> .name = "plla_dsi0",
> @@ -1784,7 +1784,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] =
> {
> .load_mask = CM_PLLC_LOADPER,
> .hold_mask = CM_PLLC_HOLDPER,
> .fixed_divider = 1,
> - .flags = CLK_SET_RATE_PARENT),
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
>
Yes, this worked, but it's enough to mark pllc_per as critical.
Noralf.
> /*
>* PLLD is the display PLL, used to drive DSI display panels.
> @@ -1891,7 +1891,7 @@ static const struct bcm2835_clk_desc clk_desc_array[] =
> {
> .load_mask = CM_PLLH_LOADAUX,
> .hold_mask = 0,
> .fixed_divider = 1,
> - .flags = CLK_SET_RATE_PARENT),
> + .flags = CLK_IS_CRITICAL | CLK_SET_RATE_PARENT),
> [BCM2835_PLLH_PIX] = REGISTER_PLL_DIV(
> SOC_BCM2835,
> .name = "pllh_pix",
>
>
> Maxime
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 07.09.2022 18.44, skrev Noralf Trønnes:
>
>
> Den 07.09.2022 12.36, skrev Stefan Wahren:
>> Hi Maxime,
>>
>> Am 05.09.22 um 16:57 schrieb Maxime Ripard:
>>> On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote:
>>>>
>>>> Den 01.09.2022 21.35, skrev Noralf Trønnes:
>>>>>
>>>>> I have finally found a workaround for my kernel hangs.
>>>>>
>>>>> Dom had a look at my kernel and found that the VideoCore was fine, and
>>>>> he said this:
>>>>>
>>>>>> That suggests cause of lockup was on arm side rather than VC side.
>>>>>>
>>>>>> But it's hard to diagnose further. Once you've had a peripheral not
>>>>>> respond, the AXI bus locks up and no further operations are possible.
>>>>>> Usual causes of this are required clocks being stopped or domains
>>>>>> disabled and then trying to access the hardware.
>>>>>>
>>>>> So when I got this on my 64-bit build:
>>>>>
>>>>> [ 166.702171] SError Interrupt on CPU1, code 0xbf02 --
>>>>> SError
>>>>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: G W
>>>>> 5.19.0-rc6-00096-gba7973977976-dirty #1
>>>>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT)
>>>>> [ 166.702206] Workqueue: events_freezable_power_
>>>>> thermal_zone_device_check
>>>>> [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS
>>>>> BTYPE=--)
>>>>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28
>>>>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70
>>>>> ...
>>>>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal]
>>>>>
>>>>> I wondered if that reg read was stalled due to a clock being stopped.
>>>>>
>>>>> Lo and behold, disabling runtime pm and keeping the vec clock running
>>>>> all the time fixed it[1].
>>>>>
>>>>> I don't know what the problem is, but at least I can now test this
>>>>> patchset.
>>>>>
>>>>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065
>>>>>
>>>> It turns out I didn't have to disable runtime pm:
>>>> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2
>>> If the bcm2711_thermal IP needs that clock to be enabled, it should grab
>>> a reference itself, but it looks like even the device tree binding
>>> doesn't ask for one.
>> The missing clock in the device tree binding is expected, because
>> despite of the code there is not much information about the BCM2711
>> clock tree. But i'm skeptical that the AVS IP actually needs the VEC
>> clock, i think it's more likely that the VEC clock parent is changed and
>> that cause this issue. I could take care of the bcm2711 binding & driver
>> if i know which clock is really necessary.
>
> Seems you're right, keeping the parent always enabled is enough:
>
> clk_prepare_enable(clk_get_parent(vec->clock)); // pllc_per
>
> I tried enabling just the grandparent clock as well, but that didn't help.
>
> Without the clock hack it seems the hang occurs when switching between
> NTSC and PAL, at most I've been able to do that 4-5 times before it hangs.
>
> For a while it looked like fbdev/fbcon had a play in this, but then I
> realised that it just gave me a NTSC mode to start from and to go back
> to when qutting modetest.
>
I've looked some more into this problem and I see that downstream is
using a firmware clock for vec:
clk: Move vec clock to clk-raspberrypi
https://github.com/raspberrypi/linux/pull/4639
If I do the same my problem goes away.
It's interesting to note that on downstream 5.10.103-v7l+ #1530,
pllc_per is enabled even if tvout is not enabled:
$ sudo cat /sys/kernel/debug/clk/pllc_per/regdump
cm = 0x
a2w = 0x0004 (disable bit(8) is not set)
It's when mainline vc4_vec disables this vec parent clock that the crash
occurs.
Sidenote: Another downstream fw clock change with a vec reference[1]:
Another issue not related to the clock crash problem:
I assumed that unloading the vc4 module would release the clocks, but
this didn't happen.
When I looked at it I remembered that there's a catch in the DRM unplug
machinery when it comes to unloading a driver and the DRM disable hooks.
static void vc4_drm_unbind(struct device *dev)
{
struct drm_device *drm = dev_get_drvdata(dev);
drm_dev_unplug(drm);
drm_atomic_helper_shutdown(drm);
}
Here the drm_device is first marked as unplugged and then the pipeline
is disabled. Since vc4_vec_encoder_disable() is protected by
drm_dev_enter() the VEC is not disabled, clocks are not released and PM
is left on.
In the drivers that I have written where the hardware is not expected to
have gone away on device unbind (SPI), I've just left out the
drm_dev_enter() check in the disable hook.
Noralf.
[1] https://github.com/raspberrypi/linux/pull/4706
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 07.09.2022 12.36, skrev Stefan Wahren: > Hi Maxime, > > Am 05.09.22 um 16:57 schrieb Maxime Ripard: >> On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote: >>> >>> Den 01.09.2022 21.35, skrev Noralf Trønnes: >>>> >>>> I have finally found a workaround for my kernel hangs. >>>> >>>> Dom had a look at my kernel and found that the VideoCore was fine, and >>>> he said this: >>>> >>>>> That suggests cause of lockup was on arm side rather than VC side. >>>>> >>>>> But it's hard to diagnose further. Once you've had a peripheral not >>>>> respond, the AXI bus locks up and no further operations are possible. >>>>> Usual causes of this are required clocks being stopped or domains >>>>> disabled and then trying to access the hardware. >>>>> >>>> So when I got this on my 64-bit build: >>>> >>>> [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- >>>> SError >>>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: G W >>>> 5.19.0-rc6-00096-gba7973977976-dirty #1 >>>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) >>>> [ 166.702206] Workqueue: events_freezable_power_ >>>> thermal_zone_device_check >>>> [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS >>>> BTYPE=--) >>>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 >>>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70 >>>> ... >>>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] >>>> >>>> I wondered if that reg read was stalled due to a clock being stopped. >>>> >>>> Lo and behold, disabling runtime pm and keeping the vec clock running >>>> all the time fixed it[1]. >>>> >>>> I don't know what the problem is, but at least I can now test this >>>> patchset. >>>> >>>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 >>>> >>> It turns out I didn't have to disable runtime pm: >>> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2 >> If the bcm2711_thermal IP needs that clock to be enabled, it should grab >> a reference itself, but it looks like even the device tree binding >> doesn't ask for one. > The missing clock in the device tree binding is expected, because > despite of the code there is not much information about the BCM2711 > clock tree. But i'm skeptical that the AVS IP actually needs the VEC > clock, i think it's more likely that the VEC clock parent is changed and > that cause this issue. I could take care of the bcm2711 binding & driver > if i know which clock is really necessary. Seems you're right, keeping the parent always enabled is enough: clk_prepare_enable(clk_get_parent(vec->clock)); // pllc_per I tried enabling just the grandparent clock as well, but that didn't help. Without the clock hack it seems the hang occurs when switching between NTSC and PAL, at most I've been able to do that 4-5 times before it hangs. For a while it looked like fbdev/fbcon had a play in this, but then I realised that it just gave me a NTSC mode to start from and to go back to when qutting modetest. Noralf.
Re: [Intel-gfx] [PATCH v2 32/41] drm/vc4: vec: Convert to the new TV mode property
Den 08.09.2022 13.23, skrev Maxime Ripard:
> Hi Noralf,
>
> On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Trønnes wrote:
>>> +static const struct drm_prop_enum_list tv_mode_names[] = {
>>
>> Maybe call it legacy_tv_mode_enums?
>>
>>>
>>> + { VC4_VEC_TV_MODE_NTSC, "NTSC", },
>>>
>>> + { VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", },
>>>
>>> + { VC4_VEC_TV_MODE_PAL, "PAL", },
>>>
>>> + { VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
>>
>> If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
>> using the switch statement in get/set property, you can use the value
>> directly to get/set tv.mode.
>
> I'm sorry, I'm not quite sure what you mean by that. If we expose the
> DRM_MODE_TV_MODE_* properties there, won't that change the values the
> userspace will need to use to set that property?
>
You're right ofc, I forgot that the enum values are also UABI.
Noralf.
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 07.09.2022 11.58, skrev Maxime Ripard: > On Mon, Sep 05, 2022 at 05:17:18PM +0200, Noralf Trønnes wrote: >> Den 05.09.2022 16.57, skrev Maxime Ripard: >>> On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote: >>>> >>>> >>>> Den 01.09.2022 21.35, skrev Noralf Trønnes: >>>>> >>>>> >>>>> I have finally found a workaround for my kernel hangs. >>>>> >>>>> Dom had a look at my kernel and found that the VideoCore was fine, and >>>>> he said this: >>>>> >>>>>> That suggests cause of lockup was on arm side rather than VC side. >>>>>> >>>>>> But it's hard to diagnose further. Once you've had a peripheral not >>>>>> respond, the AXI bus locks up and no further operations are possible. >>>>>> Usual causes of this are required clocks being stopped or domains >>>>>> disabled and then trying to access the hardware. >>>>>> >>>>> >>>>> So when I got this on my 64-bit build: >>>>> >>>>> [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- SError >>>>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: GW >>>>> 5.19.0-rc6-00096-gba7973977976-dirty #1 >>>>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) >>>>> [ 166.702206] Workqueue: events_freezable_power_ >>>>> thermal_zone_device_check >>>>> [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS >>>>> BTYPE=--) >>>>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 >>>>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70 >>>>> ... >>>>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] >>>>> >>>>> I wondered if that reg read was stalled due to a clock being stopped. >>>>> >>>>> Lo and behold, disabling runtime pm and keeping the vec clock running >>>>> all the time fixed it[1]. >>>>> >>>>> I don't know what the problem is, but at least I can now test this >>>>> patchset. >>>>> >>>>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 >>>>> >>>> >>>> It turns out I didn't have to disable runtime pm: >>>> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2 >>> >>> If the bcm2711_thermal IP needs that clock to be enabled, it should grab >>> a reference itself, but it looks like even the device tree binding >>> doesn't ask for one. >>> >> >> The first thing I tried was to unload the bcm2711_thermal module before >> running modeset and it still hung, so I don't think that's the problem. > > Ack. Just to confirm, is this happening on mainline or on the downstream tree? > It's mainline. Noralf.
Re: [Intel-gfx] [PATCH v2 09/41] drm/connector: Add TV standard property
Den 29.08.2022 15.11, skrev Maxime Ripard:
> The TV mode property has been around for a while now to select and get the
>
> current TV mode output on an analog TV connector.
>
>
>
> Despite that property name being generic, its content isn't and has been
>
> driver-specific which makes it hard to build any generic behaviour on top
>
> of it, both in kernel and user-space.
>
>
>
> Let's create a new bitmask tv norm property, that can contain any of the
>
> analog TV standards currently supported by kernel drivers. Each driver can
>
> then pass in a bitmask of the modes it supports.
>
>
>
> We'll then be able to phase out the older tv mode property.
>
>
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> +/**
>
> + * drm_mode_create_tv_properties - create TV specific connector properties
>
> + * @dev: DRM device
>
> + * @supported_tv_modes: Bitmask of TV modes supported (See
> DRM_MODE_TV_MODE_*)
>
> +
>
> + * Called by a driver's TV initialization routine, this function creates
>
> + * the TV specific connector properties for a given device. Caller is
>
> + * responsible for allocating a list of format names and passing them to
>
> + * this routine.
>
> + *
>
> + * Returns:
>
> + * 0 on success or a negative error code on failure.
>
> + */
>
> +int drm_mode_create_tv_properties(struct drm_device *dev,
>
> + unsigned int supported_tv_modes)
>
> +{
>
> + struct drm_prop_enum_list tv_mode_list[DRM_MODE_TV_MODE_MAX];
>
> + struct drm_property *tv_mode;
>
> + unsigned int i, len = 0;
>
> +
>
Can you add a check here like in the legacy version:
if (dev->mode_config.tv_mode_property)
return 0;
This way it's possible to call this multiple times. Like in drm/gud
during connector init if there are multiple TV connectors or if a device
with multiple IP blocks should show up.
Noralf.
> + for (i = 0; i < DRM_MODE_TV_MODE_MAX; i++) {
>
> + if (!(supported_tv_modes & BIT(i)))
>
> + continue;
>
> +
>
> + tv_mode_list[len].type = i;
>
> + tv_mode_list[len].name = drm_get_tv_mode_name(i);
>
> + len++;
>
> + }
>
> +
>
> + tv_mode = drm_property_create_enum(dev, 0, "TV mode",
>
> +tv_mode_list, len);
>
> + if (!tv_mode)
>
> + return -ENOMEM;
>
> +
>
> + dev->mode_config.tv_mode_property = tv_mode;
>
> +
>
> + return drm_mode_create_tv_properties_legacy(dev, 0, NULL);
>
> +}
>
> +EXPORT_SYMBOL(drm_mode_create_tv_properties);
>
Re: [Intel-gfx] [PATCH v2 24/41] drm/vc4: vec: Remove empty mode_fixup
Den 29.08.2022 15.11, skrev Maxime Ripard: > The mode_fixup hooks are deprecated, and the behaviour we implement is the > > default one anyway. Let's remove it. > > > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 29.08.2022 15.11, skrev Maxime Ripard: > Hi, > > > > Here's a series aiming at improving the command line named modes support, > > and more importantly how we deal with all the analog TV variants. > > > > The named modes support were initially introduced to allow to specify the > > analog TV mode to be used. > > > > However, this was causing multiple issues: > > > > * The mode name parsed on the command line was passed directly to the > > driver, which had to figure out which mode it was suppose to match; > > > > * Figuring that out wasn't really easy, since the video= argument or what > > the userspace might not even have a name in the first place, but > > instead could have passed a mode with the same timings; > > > > * The fallback to matching on the timings was mostly working as long as > > we were supporting one 525 lines (most likely NSTC) and one 625 lines > > (PAL), but couldn't differentiate between two modes with the same > > timings (NTSC vs PAL-M vs NSTC-J for example); > > > > * There was also some overlap with the tv mode property registered by > > drm_mode_create_tv_properties(), but named modes weren't interacting > > with that property at all. > > > > * Even though that property was generic, its possible values were > > specific to each drivers, which made some generic support difficult. > > > > Thus, I chose to tackle in multiple steps: > > > > * A new TV norm property was introduced, with generic values, each driver > > reporting through a bitmask what standard it supports to the userspace; > > > > * This option was added to the command line parsing code to be able to > > specify it on the kernel command line, and new atomic_check and reset > > helpers were created to integrate properly into atomic KMS; > > > > * The named mode parsing code is now creating a proper display mode for > > the given named mode, and the TV standard will thus be part of the > > connector state; > > > > * Two drivers were converted and tested for now (vc4 and sun4i), with > > some backward compatibility code to translate the old TV mode to the > > new TV mode; > > > > Unit tests were created along the way. > > > > One can switch from NTSC to PAL now using (on vc4) > > > > modetest -M vc4 -s 53:720x480i -w 53:'tv norm':0 > > > > modetest -M vc4 -s 53:720x480i -w 53:'tv norm':4 > The property name has changed, this gives me PAL: $ modetest -M vc4 -s 45:720x576i -w 45:'TV mode':4 I have finally found a workaround for my kernel hangs. Dom had a look at my kernel and found that the VideoCore was fine, and he said this: > That suggests cause of lockup was on arm side rather than VC side. > > But it's hard to diagnose further. Once you've had a peripheral not > respond, the AXI bus locks up and no further operations are possible. > Usual causes of this are required clocks being stopped or domains > disabled and then trying to access the hardware. > So when I got this on my 64-bit build: [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- SError [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: GW 5.19.0-rc6-00096-gba7973977976-dirty #1 [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) [ 166.702206] Workqueue: events_freezable_power_ thermal_zone_device_check [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 [ 166.702261] lr : regmap_mmio_read+0x44/0x70 ... [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] I wondered if that reg read was stalled due to a clock being stopped. Lo and behold, disabling runtime pm and keeping the vec clock running all the time fixed it[1]. I don't know what the problem is, but at least I can now test this patchset. [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 Noralf.
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 05.09.2022 16.57, skrev Maxime Ripard: > On Fri, Sep 02, 2022 at 01:28:16PM +0200, Noralf Trønnes wrote: >> >> >> Den 01.09.2022 21.35, skrev Noralf Trønnes: >>> >>> >>> I have finally found a workaround for my kernel hangs. >>> >>> Dom had a look at my kernel and found that the VideoCore was fine, and >>> he said this: >>> >>>> That suggests cause of lockup was on arm side rather than VC side. >>>> >>>> But it's hard to diagnose further. Once you've had a peripheral not >>>> respond, the AXI bus locks up and no further operations are possible. >>>> Usual causes of this are required clocks being stopped or domains >>>> disabled and then trying to access the hardware. >>>> >>> >>> So when I got this on my 64-bit build: >>> >>> [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- SError >>> [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: GW >>> 5.19.0-rc6-00096-gba7973977976-dirty #1 >>> [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) >>> [ 166.702206] Workqueue: events_freezable_power_ thermal_zone_device_check >>> [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS >>> BTYPE=--) >>> [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 >>> [ 166.702261] lr : regmap_mmio_read+0x44/0x70 >>> ... >>> [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] >>> >>> I wondered if that reg read was stalled due to a clock being stopped. >>> >>> Lo and behold, disabling runtime pm and keeping the vec clock running >>> all the time fixed it[1]. >>> >>> I don't know what the problem is, but at least I can now test this patchset. >>> >>> [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 >>> >> >> It turns out I didn't have to disable runtime pm: >> https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2 > > If the bcm2711_thermal IP needs that clock to be enabled, it should grab > a reference itself, but it looks like even the device tree binding > doesn't ask for one. > The first thing I tried was to unload the bcm2711_thermal module before running modeset and it still hung, so I don't think that's the problem. Noralf.
Re: [Intel-gfx] [PATCH v2 26/41] drm/vc4: vec: Refactor VEC TV mode setting
Den 29.08.2022 15.11, skrev Maxime Ripard:
> From: Mateusz Kwiatkowski
>
>
>
> Change the mode_set function pointer logic to declarative config0,
>
> config1 and custom_freq fields, to make TV mode setting logic more
>
> concise and uniform.
>
>
>
> Signed-off-by: Mateusz Kwiatkowski
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
>
> index 72eee0cbb615..9a37c3fcc295 100644
>
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
>
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
>
> @@ -194,7 +194,9 @@ enum vc4_vec_tv_mode_id {
>
>
>
> struct vc4_vec_tv_mode {
>
> const struct drm_display_mode *mode;
>
> - void (*mode_set)(struct vc4_vec *vec);
>
> + u32 config0;
>
> + u32 config1;
>
> + u32 custom_freq;
>
> };
>
>
>
> static const struct debugfs_reg32 vec_regs[] = {
>
> @@ -224,34 +226,6 @@ static const struct debugfs_reg32 vec_regs[] = {
>
> VC4_REG32(VEC_DAC_MISC),
>
> };
>
>
>
> -static void vc4_vec_ntsc_mode_set(struct vc4_vec *vec)
>
> -{
>
> - struct drm_device *drm = vec->connector.dev;
>
> - int idx;
>
> -
>
> - if (!drm_dev_enter(drm, ))
>
> - return;
>
> -
>
> - VEC_WRITE(VEC_CONFIG0, VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN);
>
> - VEC_WRITE(VEC_CONFIG1, VEC_CONFIG1_C_CVBS_CVBS);
>
> -
>
> - drm_dev_exit(idx);
>
> -}
>
> -
>
> -static void vc4_vec_ntsc_j_mode_set(struct vc4_vec *vec)
>
> -{
>
> - struct drm_device *drm = vec->connector.dev;
>
> - int idx;
>
> -
>
> - if (!drm_dev_enter(drm, ))
>
> - return;
>
> -
>
> - VEC_WRITE(VEC_CONFIG0, VEC_CONFIG0_NTSC_STD);
>
> - VEC_WRITE(VEC_CONFIG1, VEC_CONFIG1_C_CVBS_CVBS);
>
> -
>
> - drm_dev_exit(idx);
>
> -}
>
> -
>
> static const struct drm_display_mode ntsc_mode = {
>
> DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 13500,
>
>720, 720 + 14, 720 + 14 + 64, 720 + 14 + 64 + 60, 0,
>
> @@ -259,37 +233,6 @@ static const struct drm_display_mode ntsc_mode = {
>
>DRM_MODE_FLAG_INTERLACE)
>
> };
>
>
>
> -static void vc4_vec_pal_mode_set(struct vc4_vec *vec)
>
> -{
>
> - struct drm_device *drm = vec->connector.dev;
>
> - int idx;
>
> -
>
> - if (!drm_dev_enter(drm, ))
>
> - return;
>
> -
>
> - VEC_WRITE(VEC_CONFIG0, VEC_CONFIG0_PAL_BDGHI_STD);
>
> - VEC_WRITE(VEC_CONFIG1, VEC_CONFIG1_C_CVBS_CVBS);
>
> -
>
> - drm_dev_exit(idx);
>
> -}
>
> -
>
> -static void vc4_vec_pal_m_mode_set(struct vc4_vec *vec)
>
> -{
>
> - struct drm_device *drm = vec->connector.dev;
>
> - int idx;
>
> -
>
> - if (!drm_dev_enter(drm, ))
>
> - return;
>
> -
>
> - VEC_WRITE(VEC_CONFIG0, VEC_CONFIG0_PAL_BDGHI_STD);
>
> - VEC_WRITE(VEC_CONFIG1,
>
> - VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ);
>
> - VEC_WRITE(VEC_FREQ3_2, 0x223b);
>
> - VEC_WRITE(VEC_FREQ1_0, 0x61d1);
>
> -
>
> - drm_dev_exit(idx);
>
> -}
>
> -
>
> static const struct drm_display_mode pal_mode = {
>
> DRM_MODE("720x576", DRM_MODE_TYPE_DRIVER, 13500,
>
>720, 720 + 20, 720 + 20 + 64, 720 + 20 + 64 + 60, 0,
>
> @@ -300,19 +243,24 @@ static const struct drm_display_mode pal_mode = {
>
> static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>
> [VC4_VEC_TV_MODE_NTSC] = {
>
> .mode = _mode,
>
> - .mode_set = vc4_vec_ntsc_mode_set,
>
> + .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
>
> + .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> [VC4_VEC_TV_MODE_NTSC_J] = {
>
> .mode = _mode,
>
> - .mode_set = vc4_vec_ntsc_j_mode_set,
>
> + .config0 = VEC_CONFIG0_NTSC_STD,
>
> + .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> [VC4_VEC_TV_MODE_PAL] = {
>
> .mode = _mode,
>
> - .mode_set = vc4_vec_pal_mode_set,
>
> + .config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>
> + .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> [VC4_VEC_TV_MODE_PAL_M] = {
>
> .mode = _mode,
>
&g
Re: [Intel-gfx] [PATCH v2 18/41] drm/connector: Add a function to lookup a TV mode by its name
Den 29.08.2022 15.11, skrev Maxime Ripard:
> As part of the command line parsing rework coming in the next patches,
>
> we'll need to lookup drm_connector_tv_mode values by their name, already
>
> defined in drm_tv_mode_enum_list.
>
>
>
> In order to avoid any code duplication, let's do a function that will
>
> perform a lookup of a TV mode name and return its value.
>
>
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>
> index b1fcacd150e8..0fe01a1c20ad 100644
>
> --- a/drivers/gpu/drm/drm_connector.c
>
> +++ b/drivers/gpu/drm/drm_connector.c
>
> @@ -1003,6 +1003,30 @@ static const struct drm_prop_enum_list
> drm_tv_mode_enum_list[] = {
>
> };
>
> DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
>
>
>
> +/**
>
> + * drm_get_tv_mode_from_name - Translates a TV mode name into its enum value
>
> + * @name: TV Mode name we want to convert
>
> + * @len: Length of @name
>
> + *
>
> + * Translates @name into an enum drm_connector_tv_mode.
>
> + *
>
> + * Returns: the enum value on success, a negative errno otherwise.
>
> + */
>
> +int drm_get_tv_mode_from_name(const char *name, size_t len)
>
> +{
>
> + unsigned int i;
>
> +
>
> + for (i = 0; i < ARRAY_SIZE(drm_tv_mode_enum_list); i++) {
>
> + const struct drm_prop_enum_list *item =
> _tv_mode_enum_list[i];
>
> +
>
> + if (strlen(item->name) == len && !strncmp(item->name, name,
> len))
>
> + return item->type;
>
> + }
>
> +
>
> + return -EINVAL;
>
> +}
>
> +EXPORT_SYMBOL(drm_get_tv_mode_from_name)
Missing semicolon.
Noralf.
>
> +
>
> static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
>
> { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
>
> { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }, /* TV-out */
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>
> index bb39d2bb806e..49d261977d4e 100644
>
> --- a/include/drm/drm_connector.h
>
> +++ b/include/drm/drm_connector.h
>
> @@ -1943,6 +1943,8 @@ const char *drm_get_dp_subconnector_name(int val);
>
> const char *drm_get_content_protection_name(int val);
>
> const char *drm_get_hdcp_content_type_name(int val);
>
>
>
> +int drm_get_tv_mode_from_name(const char *name, size_t len);
>
> +
>
> int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>
> void drm_connector_attach_dp_subconnector_property(struct drm_connector
> *connector);
>
>
>
>
>
Re: [Intel-gfx] [PATCH v2 01/41] drm/tests: Order Kunit tests in Makefile
Den 29.08.2022 15.11, skrev Maxime Ripard: > Since we've recently added a ton of tests, the list starts to be a bit > > of a mess and creates unneeded conflicts. > > > > Let's order it alphabetically. > > > > Signed-off-by: Maxime Ripard > > > Something has gone wrong with this patchset, there are double line endings. I looked at the patchwork version and it look all right there so I figured it might have fixed up the patches, but it failed: git apply -v --check /home/pi/tinydrm.gud-gadget/workdirs/tv_norm_gadget/53.patch Checking patch drivers/gpu/drm/tests/Makefile... error: while searching for: # SPDX-License-Identifier: GPL-2.0? ? obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o drm_damage_helper_test.o \? drm_cmdline_parser_test.o drm_rect_test.o drm_format_test.o drm_plane_helper_test.o \? drm_dp_mst_helper_test.o drm_framebuffer_test.o drm_buddy_test.o drm_mm_test.o? error: patch failed: drivers/gpu/drm/tests/Makefile:1 error: drivers/gpu/drm/tests/Makefile: patch does not apply ERROR: Failed check apply patch pi@build-server:~/tinydrm.gud-gadget$ file workdirs/tv_norm_gadget/53.patch workdirs/tv_norm_gadget/53.patch: unified diff output, ASCII text, with CRLF, LF line terminators Noralf. > diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile > > index 91b70f7d2769..2d9f49b62ecb 100644 > > --- a/drivers/gpu/drm/tests/Makefile > > +++ b/drivers/gpu/drm/tests/Makefile > > @@ -1,5 +1,13 @@ > > # SPDX-License-Identifier: GPL-2.0 > > > > -obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o > drm_damage_helper_test.o \ > > - drm_cmdline_parser_test.o drm_rect_test.o drm_format_test.o > drm_plane_helper_test.o \ > > - drm_dp_mst_helper_test.o drm_framebuffer_test.o drm_buddy_test.o > drm_mm_test.o > > +obj-$(CONFIG_DRM_KUNIT_TEST) += \ > > + drm_buddy_test.o \ > > + drm_cmdline_parser_test.o \ > > + drm_damage_helper_test.o \ > > + drm_dp_mst_helper_test.o \ > > + drm_format_helper_test.o \ > > + drm_format_test.o \ > > + drm_framebuffer_test.o \ > > + drm_mm_test.o \ > > + drm_plane_helper_test.o \ > > + drm_rect_test.o > > >
Re: [Intel-gfx] [PATCH v2 27/41] drm/vc4: vec: Remove redundant atomic_mode_set
Den 29.08.2022 15.11, skrev Maxime Ripard:
> From: Mateusz Kwiatkowski
>
>
>
> Let's remove the superfluous tv_mode field, which was redundant with the
>
> mode field in struct drm_tv_connector_state.
>
>
>
> Signed-off-by: Mateusz Kwiatkowski
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
>
> index 9a37c3fcc295..4d7bc7c20704 100644
>
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
>
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
>
> @@ -171,8 +171,6 @@ struct vc4_vec {
>
>
>
> struct clk *clock;
>
>
>
> - const struct vc4_vec_tv_mode *tv_mode;
>
> -
>
> struct debugfs_regset32 regset;
>
> };
>
>
>
> @@ -316,7 +314,6 @@ static int vc4_vec_connector_init(struct drm_device *dev,
> struct vc4_vec *vec)
>
> drm_object_attach_property(>base,
>
> dev->mode_config.legacy_tv_mode_property,
>
> VC4_VEC_TV_MODE_NTSC);
>
> - vec->tv_mode = _vec_tv_modes[VC4_VEC_TV_MODE_NTSC];
>
>
>
> drm_connector_attach_encoder(connector, >encoder.base);
>
>
>
> @@ -360,6 +357,11 @@ static void vc4_vec_encoder_enable(struct drm_encoder
> *encoder,
>
> {
>
> struct drm_device *drm = encoder->dev;
>
> struct vc4_vec *vec = encoder_to_vc4_vec(encoder);
>
> + struct drm_connector *connector = >connector;
>
> + struct drm_connector_state *conn_state =
>
> + drm_atomic_get_new_connector_state(state, connector);
>
> + const struct vc4_vec_tv_mode *tv_mode =
>
> + _vec_tv_modes[conn_state->tv.mode];
>
> int idx, ret;
>
>
>
> if (!drm_dev_enter(drm, ))
>
> @@ -418,15 +420,14 @@ static void vc4_vec_encoder_enable(struct drm_encoder
> *encoder,
>
> /* Mask all interrupts. */
>
> VEC_WRITE(VEC_MASK0, 0);
>
>
>
> - VEC_WRITE(VEC_CONFIG0, vec->tv_mode->config0);
>
> - VEC_WRITE(VEC_CONFIG1, vec->tv_mode->config1);
>
> + VEC_WRITE(VEC_CONFIG0, tv_mode->config0);
>
> + VEC_WRITE(VEC_CONFIG1, tv_mode->config1);
>
>
>
> - if (vec->tv_mode->custom_freq != 0) {
>
> + if (tv_mode->custom_freq != 0) {
>
> VEC_WRITE(VEC_FREQ3_2,
>
> - (vec->tv_mode->custom_freq >> 16) &
>
> - 0x);
>
> + (tv_mode->custom_freq >> 16) & 0x);
>
> VEC_WRITE(VEC_FREQ1_0,
>
> - vec->tv_mode->custom_freq & 0x);
>
> + tv_mode->custom_freq & 0x);
>
Nit: This patch will be smaller if you add the tv_mode variable to the
previous patch.
Reviewed-by: Noralf Trønnes
> }
>
>
>
> VEC_WRITE(VEC_DAC_MISC,
>
> @@ -442,15 +443,6 @@ static void vc4_vec_encoder_enable(struct drm_encoder
> *encoder,
>
> drm_dev_exit(idx);
>
> }
>
>
>
> -static void vc4_vec_encoder_atomic_mode_set(struct drm_encoder *encoder,
>
> - struct drm_crtc_state *crtc_state,
>
> - struct drm_connector_state *conn_state)
>
> -{
>
> - struct vc4_vec *vec = encoder_to_vc4_vec(encoder);
>
> -
>
> - vec->tv_mode = _vec_tv_modes[conn_state->tv.mode];
>
> -}
>
> -
>
> static int vc4_vec_encoder_atomic_check(struct drm_encoder *encoder,
>
> struct drm_crtc_state *crtc_state,
>
> struct drm_connector_state *conn_state)
>
> @@ -470,7 +462,6 @@ static const struct drm_encoder_helper_funcs
> vc4_vec_encoder_helper_funcs = {
>
> .atomic_check = vc4_vec_encoder_atomic_check,
>
> .atomic_disable = vc4_vec_encoder_disable,
>
> .atomic_enable = vc4_vec_encoder_enable,
>
> - .atomic_mode_set = vc4_vec_encoder_atomic_mode_set,
>
> };
>
>
>
> static int vc4_vec_late_register(struct drm_encoder *encoder)
>
>
>
Re: [Intel-gfx] [PATCH v2 32/41] drm/vc4: vec: Convert to the new TV mode property
Den 29.08.2022 15.11, skrev Maxime Ripard:
> Now that the core can deal fine with analog TV modes, let's convert the vc4
>
> VEC driver to leverage those new features.
>
>
>
> We've added some backward compatibility to support the old TV mode property
>
> and translate it into the new TV norm property.
>
>
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
>
> index ba6f81908923..58286acf4b9e 100644
>
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
>
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
> @@ -192,7 +200,7 @@ enum vc4_vec_tv_mode_id {
>
> };
>
>
>
> struct vc4_vec_tv_mode {
>
> - const struct drm_display_mode *mode;
>
> + unsigned int mode;
>
> u32 config0;
>
> u32 config1;
>
> u32 custom_freq;
>
> @@ -226,28 +234,50 @@ static const struct debugfs_reg32 vec_regs[] = {
>
> };
>
>
>
> static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>
> - [VC4_VEC_TV_MODE_NTSC] = {
>
> - .mode = _mode_480i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_NTSC_M,
>
> .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> - [VC4_VEC_TV_MODE_NTSC_J] = {
>
> - .mode = _mode_480i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_NTSC_J,
>
> .config0 = VEC_CONFIG0_NTSC_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> - [VC4_VEC_TV_MODE_PAL] = {
>
> - .mode = _mode_576i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_PAL_B,
>
> .config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> - [VC4_VEC_TV_MODE_PAL_M] = {
>
> - .mode = _mode_480i,
>
> + {
>
> + .mode = DRM_MODE_TV_MODE_PAL_M,
>
> .config0 = VEC_CONFIG0_PAL_M_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> };
>
>
>
> +static inline const struct vc4_vec_tv_mode *
>
> +vc4_vec_tv_mode_lookup(unsigned int mode)
>
> +{
>
> + unsigned int i;
>
> +
>
> + for (i = 0; i < ARRAY_SIZE(vc4_vec_tv_modes); i++) {
>
> + const struct vc4_vec_tv_mode *tv_mode = _vec_tv_modes[i];
>
> +
>
> + if (tv_mode->mode == mode)
>
> + return tv_mode;
>
> + }
>
> +
>
> + return NULL;
>
> +}
>
> +
>
> +static const struct drm_prop_enum_list tv_mode_names[] = {
Maybe call it legacy_tv_mode_enums?
>
> + { VC4_VEC_TV_MODE_NTSC, "NTSC", },
>
> + { VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", },
>
> + { VC4_VEC_TV_MODE_PAL, "PAL", },
>
> + { VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
If you use DRM_MODE_TV_MODE_* here you don't need to translate the value
using the switch statement in get/set property, you can use the value
directly to get/set tv.mode.
Noralf.
>
> +};
Re: [Intel-gfx] [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes
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
>
On a 32-bit build I'm getting bogus modes:
[ 249.57] [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_NS63556U
>
> +#define NTSC_LINES_NUMBER525
>
> +
>
> +#define NTSC_HBLK_DURATION_TYP_NS10900U
>
> +#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_NS4700
>
> +#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
Re: [Intel-gfx] [PATCH v2 29/41] drm/vc4: vec: Switch for common modes
Den 29.08.2022 15.11, skrev Maxime Ripard:
> Now that the core has a definition for the 525 and 625 lines analog TV
>
> modes, let's switch to it for vc4.
>
>
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/vc4/vc4_vec.c b/drivers/gpu/drm/vc4/vc4_vec.c
>
> index d1d40b69279e..63e4e617e321 100644
>
> --- a/drivers/gpu/drm/vc4/vc4_vec.c
>
> +++ b/drivers/gpu/drm/vc4/vc4_vec.c
>
> @@ -224,38 +224,24 @@ static const struct debugfs_reg32 vec_regs[] = {
>
> VC4_REG32(VEC_DAC_MISC),
>
> };
>
>
>
> -static const struct drm_display_mode ntsc_mode = {
>
> - DRM_MODE("720x480", DRM_MODE_TYPE_DRIVER, 13500,
>
> - 720, 720 + 14, 720 + 14 + 64, 720 + 14 + 64 + 60, 0,
>
> - 480, 480 + 7, 480 + 7 + 6, 525, 0,
>
> - DRM_MODE_FLAG_INTERLACE)
>
> -};
>
> -
>
> -static const struct drm_display_mode pal_mode = {
>
> - DRM_MODE("720x576", DRM_MODE_TYPE_DRIVER, 13500,
>
> - 720, 720 + 20, 720 + 20 + 64, 720 + 20 + 64 + 60, 0,
>
> - 576, 576 + 4, 576 + 4 + 6, 625, 0,
>
> - DRM_MODE_FLAG_INTERLACE)
>
> -};
>
> -
>
> static const struct vc4_vec_tv_mode vc4_vec_tv_modes[] = {
>
> [VC4_VEC_TV_MODE_NTSC] = {
>
> - .mode = _mode,
>
> + .mode = _mode_480i,
>
I can't find drm_mode_480i anywhere, maybe the compiler doesn't complain
since you remove the reference in a later patch?
Noralf.
> .config0 = VEC_CONFIG0_NTSC_STD | VEC_CONFIG0_PDEN,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> [VC4_VEC_TV_MODE_NTSC_J] = {
>
> - .mode = _mode,
>
> + .mode = _mode_480i,
>
> .config0 = VEC_CONFIG0_NTSC_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> [VC4_VEC_TV_MODE_PAL] = {
>
> - .mode = _mode,
>
> + .mode = _mode_576i,
>
> .config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS,
>
> },
>
> [VC4_VEC_TV_MODE_PAL_M] = {
>
> - .mode = _mode,
>
> + .mode = _mode_576i,
>
> .config0 = VEC_CONFIG0_PAL_BDGHI_STD,
>
> .config1 = VEC_CONFIG1_C_CVBS_CVBS | VEC_CONFIG1_CUSTOM_FREQ,
>
> .custom_freq = 0x223b61d1,
>
>
>
Re: [Intel-gfx] [PATCH v2 28/41] drm/vc4: vec: Fix timings for VEC modes
Den 29.08.2022 15.11, skrev Maxime Ripard:
> From: Mateusz Kwiatkowski
>
>
>
> This commit fixes vertical timings of the VEC (composite output) modes
>
> to accurately represent the 525-line ("NTSC") and 625-line ("PAL") ITU-R
>
> standards.
>
>
>
> Previous timings were actually defined as 502 and 601 lines, resulting
>
> in non-standard 62.69 Hz and 52 Hz signals being generated,
>
> respectively.
>
>
>
> Signed-off-by: Mateusz Kwiatkowski
>
> Signed-off-by: Maxime Ripard
>
>
Acked-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 25/41] drm/vc4: vec: Convert to atomic helpers
Den 29.08.2022 15.11, skrev Maxime Ripard: > The VC4 VEC driver still uses legacy enable and disable hook > > implementation. Let's convert to the atomic variants. > > > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 31/41] drm/vc4: vec: Use TV Reset implementation
Den 29.08.2022 15.11, skrev Maxime Ripard: > The analog TV properties created by the drm_mode_create_tv_properties() are > > not properly initialised at reset. Let's switch our implementation to call > > drm_atomic_helper_connector_tv_reset(). > > > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 00/41] drm: Analog TV Improvements
Den 01.09.2022 21.35, skrev Noralf Trønnes: > > > I have finally found a workaround for my kernel hangs. > > Dom had a look at my kernel and found that the VideoCore was fine, and > he said this: > >> That suggests cause of lockup was on arm side rather than VC side. >> >> But it's hard to diagnose further. Once you've had a peripheral not >> respond, the AXI bus locks up and no further operations are possible. >> Usual causes of this are required clocks being stopped or domains >> disabled and then trying to access the hardware. >> > > So when I got this on my 64-bit build: > > [ 166.702171] SError Interrupt on CPU1, code 0xbf02 -- SError > [ 166.702187] CPU: 1 PID: 8 Comm: kworker/u8:0 Tainted: GW > 5.19.0-rc6-00096-gba7973977976-dirty #1 > [ 166.702200] Hardware name: Raspberry Pi 4 Model B Rev 1.1 (DT) > [ 166.702206] Workqueue: events_freezable_power_ thermal_zone_device_check > [ 166.702231] pstate: 20c5 (nzCv daIF -PAN -UAO -TCO -DIT -SSBS > BTYPE=--) > [ 166.702242] pc : regmap_mmio_read32le+0x10/0x28 > [ 166.702261] lr : regmap_mmio_read+0x44/0x70 > ... > [ 166.702606] bcm2711_get_temp+0x58/0xb0 [bcm2711_thermal] > > I wondered if that reg read was stalled due to a clock being stopped. > > Lo and behold, disabling runtime pm and keeping the vec clock running > all the time fixed it[1]. > > I don't know what the problem is, but at least I can now test this patchset. > > [1] https://gist.github.com/notro/23b984e7fa05cfbda2db50a421cac065 > It turns out I didn't have to disable runtime pm: https://gist.github.com/notro/0adcfcb12460b54e54458afe11dc8ea2 Noralf.
Re: [Intel-gfx] [PATCH v2 22/41] drm/atomic-helper: Add a TV properties reset helper
Den 29.08.2022 15.11, skrev Maxime Ripard: > The drm_tv_create_properties() function will create a bunch of properties, > > but it's up to each and every driver using that function to properly reset > > the state of these properties leading to inconsistent behaviours. > > > > Let's create a helper that will take care of it. > > > > Signed-off-by: Maxime Ripard > Reviewed-by: Noralf Trønnes
Re: [Intel-gfx] [PATCH v2 06/41] drm/connector: Rename legacy TV property
Den 29.08.2022 15.11, skrev Maxime Ripard:
> The current tv_mode has driver-specific values that don't allow to
>
> easily share code using it, either at the userspace or kernel level.
>
>
>
> Since we're going to introduce a new, generic, property that fit the
>
> same purpose, let's rename this one to legacy_tv_mode to make it
>
> obvious we should move away from it.
>
>
>
> Signed-off-by: Maxime Ripard
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>
> index 1d5e3cccb9e3..5cfad8b6ad83 100644
>
> --- a/include/drm/drm_connector.h
>
> +++ b/include/drm/drm_connector.h
>
> @@ -695,7 +695,7 @@ struct drm_connector_tv_margins {
>
> * @select_subconnector: selected subconnector
>
> * @subconnector: detected subconnector
>
> * @margins: TV margins
>
> - * @mode: TV mode
>
> + * @legacy_mode: Legacy TV mode, driver specific value
>
> * @brightness: brightness in percent
>
> * @contrast: contrast in percent
>
> * @flicker_reduction: flicker reduction in percent
>
> @@ -707,7 +707,7 @@ struct drm_tv_connector_state {
>
> enum drm_mode_subconnector select_subconnector;
>
> enum drm_mode_subconnector subconnector;
>
> struct drm_connector_tv_margins margins;
>
> - unsigned int mode;
>
> + unsigned int legacy_mode;
I suggest you do a build of the affected drivers after adding this patch
to make sure you have changed all mode -> legacy_mode occurrences
_before_ adding back mode in a later patch.
A simple grep gave me these:
drivers/gpu/drm/vc4/vc4_vec.c:
vc4_vec_tv_modes[state->tv.mode].mode);
drivers/gpu/drm/vc4/vc4_vec.c: vec->tv_mode =
_vec_tv_modes[conn_state->tv.mode];
drivers/gpu/drm/vc4/vc4_vec.c: vec_mode =
_vec_tv_modes[conn_state->tv.mode];
drivers/gpu/drm/i915/display/intel_tv.c:int format =
conn_state->tv.mode;
drivers/gpu/drm/i915/display/intel_tv.c:
connector->state->tv.mode = i;
drivers/gpu/drm/i915/display/intel_tv.c:if (old_state->tv.mode
!= new_state->tv.mode ||
drivers/gpu/drm/i915/display/intel_tv.c:state->tv.mode =
initial_mode;
drivers/gpu/drm/i915/display/intel_tv.c:
state->tv.mode);
drivers/gpu/drm/i915/display/intel_sdvo.c: format_map = 1 <<
conn_state->tv.mode;
drivers/gpu/drm/i915/display/intel_sdvo.c: format_map = 1 <<
conn_state->tv.mode;
drivers/gpu/drm/i915/display/intel_sdvo.c: if
(state->tv.mode == intel_sdvo_connector->tv_format_supported[i]) {
drivers/gpu/drm/i915/display/intel_sdvo.c: state->tv.mode =
intel_sdvo_connector->tv_format_supported[val];
drivers/gpu/drm/i915/display/intel_sdvo.c:
intel_sdvo_connector->base.base.state->tv.mode =
intel_sdvo_connector->tv_format_supported[0];
Not so easy to grep for is this in gud:
static unsigned int *gud_connector_tv_state_val(u16 prop, struct
drm_tv_connector_state *state)
{
switch (prop) {
...
case GUD_PROPERTY_TV_MODE:
return >mode;
Noralf.
>
> unsigned int brightness;
>
> unsigned int contrast;
>
> unsigned int flicker_reduction;
>
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
>
> index 6b5e01295348..35a827175c24 100644
>
> --- a/include/drm/drm_mode_config.h
>
> +++ b/include/drm/drm_mode_config.h
>
> @@ -714,11 +714,13 @@ struct drm_mode_config {
>
>* between different TV connector types.
>
>*/
>
> struct drm_property *tv_select_subconnector_property;
>
> +
>
> /**
>
> - * @tv_mode_property: Optional TV property to select
>
> + * @legacy_tv_mode_property: Optional TV property to select
>
>* the output TV mode.
>
>*/
>
> - struct drm_property *tv_mode_property;
>
> + struct drm_property *legacy_tv_mode_property;
>
> +
>
> /**
>
>* @tv_left_margin_property: Optional TV property to set the left
>
>* margin (expressed in pixels).
>
>
>
Re: [Intel-gfx] [PATCH v2 23/41] drm/atomic-helper: Add an analog TV atomic_check implementation
Den 29.08.2022 15.11, skrev Maxime Ripard:
> The analog TV connector drivers share some atomic_check logic, and the new
>
> TV standard property have created a bunch of new constraints that needs to
>
> be shared across drivers too.
>
>
>
> Let's create an atomic_check helper for those use cases.
>
>
>
> Signed-off-by: Maxime Ripard
>
>
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
>
> index 0373c3dc824b..d64733c6aae3 100644
>
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>
> @@ -556,6 +556,42 @@ void drm_atomic_helper_connector_tv_reset(struct
> drm_connector *connector)
>
> }
>
> EXPORT_SYMBOL(drm_atomic_helper_connector_tv_reset);
>
>
>
> +/**
>
> + * @drm_atomic_helper_connector_tv_check: Validate an analog TV connector
> state
>
> + * @connector: DRM Connector
>
> + * @state: the DRM State object
>
> + *
>
> + * Checks the state object to see if the requested state is valid for an
>
> + * analog TV connector.
>
> + *
>
> + * Returns:
>
> + * Zero for success, a negative error code on error.
>
> + */
>
> +int drm_atomic_helper_connector_tv_check(struct drm_connector *connector,
>
> + struct drm_atomic_state *state)
>
> +{
>
> + struct drm_connector_state *old_conn_state =
>
> + drm_atomic_get_old_connector_state(state, connector);
>
> + struct drm_connector_state *new_conn_state =
>
> + drm_atomic_get_new_connector_state(state, connector);
>
> + struct drm_crtc_state *crtc_state;
>
> + struct drm_crtc *crtc;
>
> +
>
> + crtc = new_conn_state->crtc;
>
> + if (!crtc)
>
> + return 0;
>
> +
>
> + crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>
> + if (!crtc_state)
>
> + return -EINVAL;
>
> +
>
> + if (old_conn_state->tv.mode != new_conn_state->tv.mode)
>
> + crtc_state->mode_changed = true;
>
If you can expand this check then I can use it in gud:
if (old_conn_state->tv.margins.left != new_conn_state->tv.margins.left
||
old_conn_state->tv.margins.right !=
new_conn_state->tv.margins.right ||
old_conn_state->tv.margins.top != new_conn_state->tv.margins.top ||
old_conn_state->tv.margins.bottom !=
new_conn_state->tv.margins.bottom ||
old_conn_state->tv.mode != new_conn_state->tv.mode ||
old_conn_state->tv.brightness != new_conn_state->tv.brightness ||
old_conn_state->tv.contrast != new_conn_state->tv.contrast ||
old_conn_state->tv.flicker_reduction !=
new_conn_state->tv.flicker_reduction ||
old_conn_state->tv.overscan != new_conn_state->tv.overscan ||
old_conn_state->tv.saturation != new_conn_state->tv.saturation ||
old_conn_state->tv.hue != new_conn_state->tv.hue)
crtc_state->connectors_changed = true;
With that considered:
Reviewed-by: Noralf Trønnes
> +
>
> + return 0;
>
> +}
>
> +EXPORT_SYMBOL(drm_atomic_helper_connector_tv_check);
>
> +
>
> /**
>
> * __drm_atomic_helper_connector_duplicate_state - copy atomic connector
> state
>
> * @connector: connector object
>
> diff --git a/include/drm/drm_atomic_state_helper.h
> b/include/drm/drm_atomic_state_helper.h
>
> index c8fbce795ee7..b9740edb2658 100644
>
> --- a/include/drm/drm_atomic_state_helper.h
>
> +++ b/include/drm/drm_atomic_state_helper.h
>
> @@ -26,6 +26,7 @@
>
>
>
> #include
>
>
>
> +struct drm_atomic_state;
>
> struct drm_bridge;
>
> struct drm_bridge_state;
>
> struct drm_crtc;
>
> @@ -71,6 +72,8 @@ void __drm_atomic_helper_connector_reset(struct
> drm_connector *connector,
>
>struct drm_connector_state
> *conn_state);
>
> void drm_atomic_helper_connector_reset(struct drm_connector *connector);
>
> void drm_atomic_helper_connector_tv_reset(struct drm_connector *connector);
>
> +int drm_atomic_helper_connector_tv_check(struct drm_connector *connector,
>
> + struct drm_atomic_state *state);
>
> void drm_atomic_helper_connector_tv_margins_reset(struct drm_connector
> *connector);
>
> void
>
> __drm_atomic_helper_connector_duplicate_state(struct drm_connector
> *connector,
>
>
>
Re: [Intel-gfx] [PATCH 10/11] drm/simple-helper: drm_gem_simple_display_pipe_prepare_fb as default
> It's tedious to review this all the time, and my audit showed that
> arcpgu actually forgot to set this.
>
> Make this the default and stop worrying.
>
> Again I sprinkled WARN_ON_ONCE on top to make sure we don't have
> strange combinations of hooks: cleanup_fb without prepare_fb doesn't
> make sense, and since simpler drivers are all new they better be GEM
> based drivers.
>
> Signed-off-by: Daniel Vetter
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> ---
> drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++--
> include/drm/drm_simple_kms_helper.h | 7 +--
> 2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
> index 0b095a313c44..1a97571d97d9 100644
> --- a/drivers/gpu/drm/drm_simple_kms_helper.c
> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c
> @@ -9,6 +9,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -225,8 +227,14 @@ static int drm_simple_kms_plane_prepare_fb(struct
drm_plane *plane,
> struct drm_simple_display_pipe *pipe;
>
> pipe = container_of(plane, struct drm_simple_display_pipe, plane);
> - if (!pipe->funcs || !pipe->funcs->prepare_fb)
> - return 0;
> + if (!pipe->funcs || !pipe->funcs->prepare_fb) {
> + if (WARN_ON_ONCE(drm_core_check_feature(plane->dev,
> DRIVER_GEM)))
Shouldn't this check be inverted? Looks like it warns on GEM drivers.
With that considered:
Acked-by: Noralf Trønnes
Hopefully this reply will thread correctly, I had to reply from lore (I
wasn't cc'ed) and I don't know if Thunderbird sets In-Reply-To. I'm not
subscribed to dri-devel anymore since I'm winding down my Linux work and
dri-devel is such a high volume list.
Noralf
> + return 0;
> +
> + WARN_ON_ONCE(pipe->funcs && pipe->funcs->cleanup_fb);
> +
> + return drm_gem_simple_display_pipe_prepare_fb(pipe, state);
> + }
>
> return pipe->funcs->prepare_fb(pipe, state);
> }
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/11] drm/tiny: drm_gem_simple_display_pipe_prepare_fb is the default
Den 21.05.2021 11.09, skrev Daniel Vetter: > Goes through all the drivers and deletes the default hook since it's > the default now. > > Signed-off-by: Daniel Vetter Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/xen-front: Add YUYV to supported formats
Den 31.07.2020 14.51, skrev Oleksandr Andrushchenko: > From: Oleksandr Andrushchenko > > Add YUYV to supported formats, so the frontend can work with the > formats used by cameras and other HW. > > Signed-off-by: Oleksandr Andrushchenko > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/6] drm/xen-front: Pass dumb buffer data offset to the backend
Den 31.07.2020 14.51, skrev Oleksandr Andrushchenko: > From: Oleksandr Andrushchenko > > While importing a dmabuf it is possible that the data of the buffer > is put with offset which is indicated by the SGT offset. > Respect the offset value and forward it to the backend. > > Signed-off-by: Oleksandr Andrushchenko > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/8] drm/tiny/repaper: Drop edp->enabled
Den 12.06.2020 18.00, skrev Daniel Vetter: > Same patch as the mipi-dbi one, atomic tracks this for us already, we > just have to check the right thing. > > Signed-off-by: Daniel Vetter > Cc: "Noralf Trønnes" > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/mipi-dbi: Remove ->enabled
Den 12.06.2020 18.00, skrev Daniel Vetter: > The atomic helpers try really hard to not lose track of things, > duplicating enabled tracking in the driver is at best confusing. > Double-enabling or disabling is a bug in atomic helpers. > > In the fb_dirty function we can just assume that the fb always exists, > simple display pipe helpers guarantee that the crtc is only enabled > together with the output, so we always have a primary plane around. > > Now in the update function we need to be a notch more careful, since > that can also get called when the crtc is off. And we don't want to > upload frames when that's the case, so filter that out too. > > Signed-off-by: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: David Lechner > --- Thanks for fixing this. Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
Den 05.04.2020 15.47, skrev Daniel Vetter:
> On Sun, Apr 5, 2020 at 12:18 PM Noralf Trønnes wrote:
>>
>>
>>
>> Den 03.04.2020 15.57, skrev Daniel Vetter:
>>> Also init the fbdev emulation before we register the device, that way
>>> we can rely on the auto-cleanup and simplify the probe error code even
>>> more.
>>>
>>> Signed-off-by: Daniel Vetter
>>> Cc: Dave Airlie
>>> Cc: Sean Paul
>>> Cc: Thomas Zimmermann
>>> Cc: Daniel Vetter
>>> Cc: Emil Velikov
>>> Cc: Sam Ravnborg
>>> Cc: Thomas Gleixner
>>> ---
>>> drivers/gpu/drm/udl/udl_drv.c | 36 +++
>>> 1 file changed, 11 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>> index 1ce2d865c36d..4ba5149fdd57 100644
>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>> @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct
>>> usb_interface *interface)
>>> struct udl_device *udl;
>>> int r;
>>>
>>> - udl = kzalloc(sizeof(*udl), GFP_KERNEL);
>>> - if (!udl)
>>> - return ERR_PTR(-ENOMEM);
>>> -
>>> - r = drm_dev_init(>drm, , >dev);
>>> - if (r) {
>>> - kfree(udl);
>>> - return ERR_PTR(r);
>>> - }
>>> + udl = devm_drm_dev_alloc(>dev, ,
>>> + struct udl_device, drm);
>>> + if (IS_ERR(udl))
>>> + return udl;
>>>
>>> udl->udev = udev;
>>> udl->drm.dev_private = udl;
>>> - drmm_add_final_kfree(>drm, udl);
>>>
>>> r = udl_init(udl);
>>> - if (r) {
>>> - drm_dev_put(>drm);
>>> + if (r)
>>> return ERR_PTR(r);
>>> - }
>>>
>>> usb_set_intfdata(interface, udl);
>>> +
>>> return udl;
>>> }
>>>
>>> @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface
>>> *interface,
>>> if (IS_ERR(udl))
>>> return PTR_ERR(udl);
>>>
>>> + r = drm_fbdev_generic_setup(>drm, 0);
>>
>> It doesn't feel right to have a client open the device before the DRM
>> device itself is registered. I would prefer to keep it where it is but
>> ignore any errors. A failing client shouldn't prevent the driver from
>> probing. drm_fbdev_generic_setup() do print errors if it fails. So yeah,
>> in hindsight I should have made drm_fbdev_generic_setup() return void.
>
> Hm, we have all kinds of usage right now, some check for errors, some
> dont, some do this before drm_dev_register, some after. If your
> recommendation is to not check for errors then I'm happy to implement
> that, but we're a bit inconsistent. Maybe we should do a patch that at
> least always returns 0 no matter what, plus document that the return
> value shouldn't be checked?
Yeah, always returning zero and documenting it would be a good start.
I counted 41 drivers using generic fbdev now, didn't know it was that
much used. Only 11 drivers are hand rolling their own:
armada
gma500
amd
omapdrm
nouveau
i915
msm
tegra
exynos
radeon
rockchip
Noralf.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 44/44] drm/managed: Cleanup of unused functions and polishing docs
Den 03.04.2020 15.58, skrev Daniel Vetter: > Following functions are only used internally, not by drivers: > - drm_dev_init > - devm_drm_dev_init > - drmm_add_final_kfree > > Also, now that we have a very slick and polished way to allocate a > drm_device with devm_drm_dev_alloc, update all the docs to reflect the > new reality. Mostly this consists of deleting old and misleading > hints. Two main ones: > > - it is no longer required that the drm_device base class is first in > the structure. devm_drm_dev_alloc can cope with it being anywhere > > - obviously embedded no needs devm_drm_dev_alloc s/no/now/ ? > > Signed-off-by: Daniel Vetter > --- > @@ -240,13 +240,13 @@ void drm_minor_release(struct drm_minor *minor) > * DOC: driver instance overview > * > * A device instance for a drm driver is represented by drm_device. > This > - * is initialized with drm_dev_init(), usually from bus-specific ->probe() > - * callbacks implemented by the driver. The driver then needs to initialize > all > - * the various subsystems for the drm device like memory management, vblank > - * handling, modesetting support and intial output configuration plus > obviously > - * initialize all the corresponding hardware bits. Finally when everything > is up > - * and running and ready for userspace the device instance can be published > - * using drm_dev_register(). > + * is allocated and initialized with devm_drm_dev_alloc(), usually from > + * bus-specific ->probe() callbacks implemented by the driver. The driver > then > + * needs to initialize all the various subsystems for the drm device like > memory > + * management, vblank handling, modesetting support and intial output s/intial/initial/ > + * configuration plus obviously initialize all the corresponding hardware > bits. > + * Finally when everything is up and running and ready for userspace the > device > + * instance can be published using drm_dev_register(). > * > * There is also deprecated support for initalizing device instances using > * bus-specific helpers and the _driver.load callback. But due to > @@ -767,19 +706,9 @@ EXPORT_SYMBOL(__devm_drm_dev_alloc); > * @driver: DRM driver to allocate device for > * @parent: Parent device object > * > - * Allocate and initialize a new DRM device. No device registration is done. > - * Call drm_dev_register() to advertice the device to user space and > register it > - * with other core subsystems. This should be done last in the device > - * initialization sequence to make sure userspace can't access an > inconsistent > - * state. > - * > - * The initial ref-count of the object is 1. Use drm_dev_get() and > - * drm_dev_put() to take and drop further ref-counts. > - * > - * Note that for purely virtual devices @parent can be NULL. > - * > - * Drivers that wish to subclass or embed drm_device into their > - * own struct should look at using drm_dev_init() instead. > + * This is the deprecated version of devm_drm_dev_alloc(), which doesn not > support s/doesn/does/ > + * subclassing through embedding the struct _device in a driver private > + * structure, and which does not support automatic cleanup through devres. > * > * RETURNS: > * Pointer to new DRM device, or ERR_PTR on failure. Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/44] drm: Add devm_drm_dev_alloc macro
Den 03.04.2020 15.57, skrev Daniel Vetter: > The kerneldoc is only added for this new function. Existing kerneldoc > and examples will be udated at the very end, since once all drivers > are converted over to devm_drm_dev_alloc we can unexport a lot of > interim functions and make the documentation for driver authors a lot > cleaner and less confusing. There will be only one true way to > initialize a drm_device at the end of this, which is going to be > devm_drm_dev_alloc. > > Cc: Paul Kocialkowski > Cc: Laurent Pinchart > Signed-off-by: Daniel Vetter > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/44] drm/udl: Use demv_drm_dev_alloc
Den 03.04.2020 15.57, skrev Daniel Vetter:
> Also init the fbdev emulation before we register the device, that way
> we can rely on the auto-cleanup and simplify the probe error code even
> more.
>
> Signed-off-by: Daniel Vetter
> Cc: Dave Airlie
> Cc: Sean Paul
> Cc: Thomas Zimmermann
> Cc: Daniel Vetter
> Cc: Emil Velikov
> Cc: Sam Ravnborg
> Cc: Thomas Gleixner
> ---
> drivers/gpu/drm/udl/udl_drv.c | 36 +++
> 1 file changed, 11 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 1ce2d865c36d..4ba5149fdd57 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -57,27 +57,20 @@ static struct udl_device *udl_driver_create(struct
> usb_interface *interface)
> struct udl_device *udl;
> int r;
>
> - udl = kzalloc(sizeof(*udl), GFP_KERNEL);
> - if (!udl)
> - return ERR_PTR(-ENOMEM);
> -
> - r = drm_dev_init(>drm, , >dev);
> - if (r) {
> - kfree(udl);
> - return ERR_PTR(r);
> - }
> + udl = devm_drm_dev_alloc(>dev, ,
> + struct udl_device, drm);
> + if (IS_ERR(udl))
> + return udl;
>
> udl->udev = udev;
> udl->drm.dev_private = udl;
> - drmm_add_final_kfree(>drm, udl);
>
> r = udl_init(udl);
> - if (r) {
> - drm_dev_put(>drm);
> + if (r)
> return ERR_PTR(r);
> - }
>
> usb_set_intfdata(interface, udl);
> +
> return udl;
> }
>
> @@ -91,23 +84,17 @@ static int udl_usb_probe(struct usb_interface *interface,
> if (IS_ERR(udl))
> return PTR_ERR(udl);
>
> + r = drm_fbdev_generic_setup(>drm, 0);
It doesn't feel right to have a client open the device before the DRM
device itself is registered. I would prefer to keep it where it is but
ignore any errors. A failing client shouldn't prevent the driver from
probing. drm_fbdev_generic_setup() do print errors if it fails. So yeah,
in hindsight I should have made drm_fbdev_generic_setup() return void.
Noralf.
> + if (r)
> + return r;
> +
> r = drm_dev_register(>drm, 0);
> if (r)
> - goto err_free;
> + return r;
>
> DRM_INFO("Initialized udl on minor %d\n", udl->drm.primary->index);
>
> - r = drm_fbdev_generic_setup(>drm, 0);
> - if (r)
> - goto err_drm_dev_unregister;
> -
> return 0;
> -
> -err_drm_dev_unregister:
> - drm_dev_unregister(>drm);
> -err_free:
> - drm_dev_put(>drm);
> - return r;
> }
>
> static void udl_usb_disconnect(struct usb_interface *interface)
> @@ -117,7 +104,6 @@ static void udl_usb_disconnect(struct usb_interface
> *interface)
> drm_kms_helper_poll_fini(dev);
> udl_drop_usb(dev);
> drm_dev_unplug(dev);
> - drm_dev_put(dev);
> }
>
> /*
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 39/44] drm/cirrus: Use devm_drm_dev_alloc
Den 03.04.2020 15.58, skrev Daniel Vetter: > Already using devm_drm_dev_init, so very simple replacment. > > Signed-off-by: Daniel Vetter > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: Daniel Vetter > Cc: Sam Ravnborg > Cc: "Noralf Trønnes" > Cc: Rob Herring > Cc: Thomas Zimmermann > Cc: virtualizat...@lists.linux-foundation.org > Cc: Emil Velikov > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 21/44] drm/ili9486: Use devm_drm_dev_alloc
Den 03.04.2020 15.58, skrev Daniel Vetter: > Already using devm_drm_dev_init, so very simple replacment. > > Signed-off-by: Daniel Vetter > Cc: Kamlesh Gurudasani > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 19/44] drm/repaper: Use devm_drm_dev_alloc
Den 03.04.2020 15.58, skrev Daniel Vetter: > Already using devm_drm_dev_init, so very simple replacment. > > Signed-off-by: Daniel Vetter > Cc: "Noralf Trønnes" > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 20/44] drm/mi0283qt: Use devm_drm_dev_alloc
Den 03.04.2020 15.58, skrev Daniel Vetter: > Already using devm_drm_dev_init, so very simple replacment. > > Signed-off-by: Daniel Vetter > Cc: "Noralf Trønnes" > --- Acked-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_
Den 21.02.2020 22.02, skrev Daniel Vetter:
> drm_mode_config_cleanup is idempotent, so no harm in calling this
> twice. This allows us to gradually switch drivers over by removing
> explicit drm_mode_config_cleanup calls.
>
> With this step it's not also possible that (at least for simple
> drivers) automatic resource cleanup can be done correctly without a
> drm_driver->release hook. Therefore allow this now in
> devm_drm_dev_init().
>
> Also with drmm_ explicit drm_driver->release hooks are kinda not the
> best option, so deprecate that hook to discourage future users.
>
> v2: Fixup the example in the kerneldoc too.
>
> v3:
> - For paranoia, double check that minor->dev == dev in the release
> hook, because I botched the pointer math in the drmm library.
> - Call drm_mode_config_cleanup when drmm_add_action fails, we'd be
> missing some mutex_destroy and ida_cleanup otherwise (Laurent)
>
> v4: Add a drmm_add_action_or_reset (like devm_ has) to encapsulate this
> pattern (Noralf).
>
> Cc: Laurent Pinchart
> Cc: "Noralf Trønnes"
> Cc: Sam Ravnborg
> Cc: Thomas Zimmermann
> Signed-off-by: Daniel Vetter
> ---
> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> index 626656369f0b..6376be01bbc8 100644
> --- a/drivers/gpu/drm/drm_managed.c
> +++ b/drivers/gpu/drm/drm_managed.c
> @@ -134,6 +134,20 @@ int __drmm_add_action(struct drm_device *dev,
> }
> EXPORT_SYMBOL(__drmm_add_action);
>
> +int __drmm_add_action_or_reset(struct drm_device *dev,
> +drmres_release_t action,
> +void *data, const char *name)
> +{
> + int ret;
> +
> + ret = __drmm_add_action(dev, action, data, name);
> + if (ret)
> + action(dev, data);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(__drmm_add_action_or_reset);
> +
> void drmm_remove_action(struct drm_device *dev,
> drmres_release_t action,
> void *data)
> diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> index 08e6eff6a179..6f7005bc597f 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -373,6 +374,11 @@ static int drm_mode_create_standard_properties(struct
> drm_device *dev)
> return 0;
> }
>
> +static void drm_mode_config_init_release(struct drm_device *dev, void *ptr)
> +{
> + drm_mode_config_cleanup(dev);
> +}
> +
> /**
> * drm_mode_config_init - initialize DRM mode_configuration structure
> * @dev: DRM device
> @@ -384,8 +390,10 @@ static int drm_mode_create_standard_properties(struct
> drm_device *dev)
> * problem, since this should happen single threaded at init time. It is the
> * driver's problem to ensure this guarantee.
> *
> + * Cleanup is automatically handled through registering
> drm_mode_config_cleanup
> + * with drmm_add_action().
> */
> -void drm_mode_config_init(struct drm_device *dev)
> +int drm_mode_config_init(struct drm_device *dev)
> {
> mutex_init(>mode_config.mutex);
> drm_modeset_lock_init(>mode_config.connection_mutex);
> @@ -443,6 +451,9 @@ void drm_mode_config_init(struct drm_device *dev)
> drm_modeset_acquire_fini(_ctx);
> dma_resv_fini();
> }
> +
> + return drmm_add_action_or_reset(dev, drm_mode_config_init_release,
> + NULL);
> }
> EXPORT_SYMBOL(drm_mode_config_init);
>
> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> index 2b1ba2ad5582..684f884b6cea 100644
> --- a/include/drm/drm_managed.h
> +++ b/include/drm/drm_managed.h
> @@ -18,6 +18,13 @@ int __must_check __drmm_add_action(struct drm_device *dev,
> drmres_release_t action,
> void *data, const char *name);
>
> +#define drmm_add_action_or_reset(dev, action, data) \
> + __drmm_add_action(dev, action, data, #action)
Copy-paste error here, you want __drmm_add_action_or_reset().
Apart from that it looks good:
Acked-by: Noralf Trønnes
> +
> +int __must_check __drmm_add_action_or_reset(struct drm_device *dev,
> + drmres_release_t action,
> + void *data, const char *name);
> +
> void drmm_remove_action(struct drm_device *dev,
> drmres_release_t action,
> void *data);
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 49/52] drm/mipi-dbi: Drop explicit drm_mode_config_cleanup call
Den 19.02.2020 11.21, skrev Daniel Vetter: > Allows us to drop the drm_driver.release callback from all > drivers, and remove the mipi_dbi_release() function. > > Signed-off-by: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Eric Anholt > Cc: David Lechner > Cc: Kamlesh Gurudasani > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 48/52] drm/mipi-dbi: Move drm_mode_config_init into mipi library
Den 19.02.2020 11.21, skrev Daniel Vetter: > 7/7 drivers agree that's the right choice, let's do this. > > This avoids duplicating the same old error checking code over all 7 > drivers, which is the motivation here. > > Signed-off-by: Daniel Vetter > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 47/52] drm/repaper: Drop explicit drm_mode_config_cleanup call
Den 19.02.2020 11.21, skrev Daniel Vetter: > Allows us to drop the drm_driver.release callback. > > Signed-off-by: Daniel Vetter > Cc: "Noralf Trønnes" > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 16/52] drm/repaper: Use drmm_add_final_kfree
Den 19.02.2020 11.20, skrev Daniel Vetter: > With this we can drop the final kfree from the release function. > > Signed-off-by: Daniel Vetter > Cc: "Noralf Trønnes" > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
Den 19.02.2020 11.20, skrev Daniel Vetter: > They all share mipi_dbi_release so we need to switch them all > together. With this we can drop the final kfree from the release > function. > > Aside, I think we could perhaps have a tiny additional helper for > these mipi_dbi drivers, the first few lines around devm_drm_dev_init > are all the same (except for the drm_driver pointer). > > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Eric Anholt > Cc: David Lechner > Cc: Kamlesh Gurudasani > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > Signed-off-by: Daniel Vetter > --- I really would have preferred having devm_drm_dev_alloc() in this series, drmm_add_final_kfree() is rather odd. But I can wait: Reviewed-by: Noralf Trønnes I have tested the whole series on tiny/mi0283qt: Tested-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/52] drm: Manage drm_mode_config_init with drmm_
Den 19.02.2020 17.23, skrev Daniel Vetter:
> On Wed, Feb 19, 2020 at 5:08 PM Laurent Pinchart
> wrote:
>>
>> Hi Daniel,
>>
>> On Wed, Feb 19, 2020 at 04:47:55PM +0100, Daniel Vetter wrote:
>>> On Wed, Feb 19, 2020 at 2:50 PM Laurent Pinchart wrote:
>>>> On Wed, Feb 19, 2020 at 11:20:57AM +0100, Daniel Vetter wrote:
>>>>> drm_mode_config_cleanup is idempotent, so no harm in calling this
>>>>> twice. This allows us to gradually switch drivers over by removing
>>>>> explicit drm_mode_config_cleanup calls.
>>>>>
>>>>> With this step it's not also possible that (at least for simple
>>>>> drivers) automatic resource cleanup can be done correctly without a
>>>>> drm_driver->release hook. Therefore allow this now in
>>>>> devm_drm_dev_init().
>>>>>
>>>>> Also with drmm_ explicit drm_driver->release hooks are kinda not the
>>>>> best option, so deprecate that hook to discourage future users.
>>>>>
>>>>> v2: Fixup the example in the kerneldoc too.
>>>>>
>>>>> Cc: "Noralf Trønnes"
>>>>> Cc: Sam Ravnborg
>>>>> Cc: Thomas Zimmermann
>>>>> Signed-off-by: Daniel Vetter
>>>>> ---
>>>>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>>>>> b/drivers/gpu/drm/drm_mode_config.c
>>>>> index 08e6eff6a179..957db1edba0c 100644
>>>>> --- a/drivers/gpu/drm/drm_mode_config.c
>>>>> +++ b/drivers/gpu/drm/drm_mode_config.c
>>>>> @@ -25,6 +25,7 @@
>>>>> #include
>>>>> #include
>>>>> #include
>>>>> +#include
>>>>> #include
>>>>> #include
>>>>> #include
>>>>> @@ -373,6 +374,11 @@ static int
>>>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +static void drm_mode_config_init_release(struct drm_device *dev, void
>>>>> *ptr)
>>>>> +{
>>>>> + drm_mode_config_cleanup(dev);
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * drm_mode_config_init - initialize DRM mode_configuration structure
>>>>> * @dev: DRM device
>>>>> @@ -384,8 +390,10 @@ static int
>>>>> drm_mode_create_standard_properties(struct drm_device *dev)
>>>>> * problem, since this should happen single threaded at init time. It is
>>>>> the
>>>>> * driver's problem to ensure this guarantee.
>>>>> *
>>>>> + * Cleanup is automatically handled through registering
>>>>> drm_mode_config_cleanup
>>>>> + * with drmm_add_action().
>>>>> */
>>>>> -void drm_mode_config_init(struct drm_device *dev)
>>>>> +int drm_mode_config_init(struct drm_device *dev)
>>>>> {
>>>>> mutex_init(>mode_config.mutex);
>>>>> drm_modeset_lock_init(>mode_config.connection_mutex);
>>>>> @@ -443,6 +451,8 @@ void drm_mode_config_init(struct drm_device *dev)
>>>>> drm_modeset_acquire_fini(_ctx);
>>>>> dma_resv_fini();
>>>>> }
>>>>> +
>>>>> + return drmm_add_action(dev, drm_mode_config_init_release, NULL);
>>>>
>>>> If this fails, shouldn't drm_mode_config_cleanup() be called here ?
>>>
>>> Maybe for ocd reasons, but not for actually cleaning up anything. It's
>>> just a bunch of empty lists that drm_mode_config_cleanup will walk and
>>> do nothing about. Not sure I should add that ...
>>
>> How about the ida init, and the mutex_init() that isn't a no-op when
>> lock debugging is enabled ?
>
> Hm right, I'll fix this.
>
You could make a drmm_ version of devm_add_action_or_reset() for this.
Noralf.
> Fun thing is that I've found a pile of missing mutex_destroy and
> ida_cleanup() while reviewing all the code I've read. Not sure I've
> fixed them all up ...
> -Daniel
>
>>
>>>>> }
>>>>> EXPORT_SYMBOL(drm_mode_config_init);
>>>>>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/client: Rename _force to _locked
Den 29.01.2020 09.24, skrev Daniel Vetter: > Plus extend the kerneldoc a bit to explain how this should be used. > With the previous patch to drop the force restore the main user of > this function is not emphasis on the "I hold the internal master lock The _not_ confuses me, the emphasis is now that "I hold the lock" right? > already" aspect, so rename the function to match. > > Suggested by Noralf. > > Cc: Noralf Trønnes > Signed-off-by: Daniel Vetter > --- Reviewed-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
