[PATCH v2 1/6] drm/display: hdmi: add generic mode_valid helper
Maxime Ripard
mripard at kernel.org
Fri Nov 8 14:17:22 UTC 2024
Hi,
On Fri, Nov 01, 2024 at 02:25:04AM +0200, Dmitry Baryshkov wrote:
> Add drm_hdmi_connector_mode_valid(), generic helper for HDMI connectors.
> It can be either used directly or as a part of the .mode_valid callback.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> ---
> drivers/gpu/drm/display/drm_hdmi_helper.c | 45 ++++++
> drivers/gpu/drm/display/drm_hdmi_helper_internal.h | 11 ++
> drivers/gpu/drm/display/drm_hdmi_state_helper.c | 26 +---
> drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 168 ++++++++++++++++++++-
> include/drm/display/drm_hdmi_helper.h | 4 +
> 5 files changed, 229 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..560c5d4365ca54d3f669395349cedfd6f75fa033 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -9,6 +9,8 @@
> #include <drm/drm_print.h>
> #include <drm/drm_property.h>
>
> +#include "drm_hdmi_helper_internal.h"
> +
> static inline bool is_eotf_supported(u8 output_eotf, u8 sink_eotf)
> {
> return sink_eotf & BIT(output_eotf);
> @@ -256,3 +258,46 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> }
> EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> +
> +enum drm_mode_status
> +__drm_hdmi_connector_clock_valid(const struct drm_connector *connector,
> + const struct drm_display_mode *mode,
> + unsigned long long clock)
> +{
> + const struct drm_connector_hdmi_funcs *funcs = connector->hdmi.funcs;
> + const struct drm_display_info *info = &connector->display_info;
> +
> + if (info->max_tmds_clock && clock > info->max_tmds_clock * 1000)
> + return MODE_CLOCK_HIGH;
> +
> + if (funcs && funcs->tmds_char_rate_valid) {
> + enum drm_mode_status status;
> +
> + status = funcs->tmds_char_rate_valid(connector, mode, clock);
> + if (status != MODE_OK)
> + return status;
> + }
> +
> + return MODE_OK;
> +}
> +
> +/**
> + * drm_hdmi_connector_mode_valid() - Check if mode is valid for HDMI connector
> + * @connector: DRM connector to validate the mode
> + * @mode: Display mode to validate
> + *
> + * Generic .mode_valid implementation for HDMI connectors.
> + */
> +enum drm_mode_status
> +drm_hdmi_connector_mode_valid(struct drm_connector *connector,
> + struct drm_display_mode *mode)
> +{
> + unsigned long long clock;
> +
> + clock = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
> + if (!clock)
> + return MODE_ERROR;
> +
> + return __drm_hdmi_connector_clock_valid(connector, mode, clock);
> +}
> +EXPORT_SYMBOL(drm_hdmi_connector_mode_valid);
It's not clear to me why you want to place it in drm_hdmi_helper? It's
relying quite heavily on the HDMI infrastructure, so it would make more
sense to me that it would be part of drm_hdmi_state_helper.c.
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index 34ee95d41f2966ab23a60deb37d689430f6b0985..8640e7280053bd95852f53b92159f493b141f2bf 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -43,10 +43,12 @@ struct drm_atomic_helper_connector_hdmi_priv {
> static struct drm_display_mode *find_preferred_mode(struct drm_connector *connector)
> {
> struct drm_device *drm = connector->dev;
> - struct drm_display_mode *mode, *preferred;
> + struct drm_display_mode *mode, *preferred = NULL;
>
> mutex_lock(&drm->mode_config.mutex);
> - preferred = list_first_entry(&connector->modes, struct drm_display_mode, head);
> + if (!list_empty(&connector->modes))
> + preferred = list_first_entry(&connector->modes, struct drm_display_mode, head);
> +
What is this fixing?
> list_for_each_entry(mode, &connector->modes, head)
> if (mode->type & DRM_MODE_TYPE_PREFERRED)
> preferred = mode;
> @@ -125,6 +127,18 @@ static const struct drm_connector_hdmi_funcs reject_connector_hdmi_funcs = {
> .tmds_char_rate_valid = reject_connector_tmds_char_rate_valid,
> };
>
> +static enum drm_mode_status
> +reject_100MHz_connector_tmds_char_rate_valid(const struct drm_connector *connector,
> + const struct drm_display_mode *mode,
> + unsigned long long tmds_rate)
> +{
> + return (tmds_rate > 100ULL * 1000 * 1000) ? MODE_BAD : MODE_OK;
> +}
> +
> +static const struct drm_connector_hdmi_funcs reject_100_MHz_connector_hdmi_funcs = {
> + .tmds_char_rate_valid = reject_100MHz_connector_tmds_char_rate_valid,
> +};
> +
> static int dummy_connector_get_modes(struct drm_connector *connector)
> {
> struct drm_atomic_helper_connector_hdmi_priv *priv =
> @@ -147,6 +161,33 @@ static int dummy_connector_get_modes(struct drm_connector *connector)
> static const struct drm_connector_helper_funcs dummy_connector_helper_funcs = {
> .atomic_check = drm_atomic_helper_connector_hdmi_check,
> .get_modes = dummy_connector_get_modes,
> + .mode_valid = drm_hdmi_connector_mode_valid,
> +};
> +
> +static int dummy_connector_get_modes_100MHz_max_clock(struct drm_connector *connector)
> +{
> + struct drm_atomic_helper_connector_hdmi_priv *priv =
> + connector_to_priv(connector);
> + const struct drm_edid *edid;
> + unsigned int num_modes;
> +
> + edid = drm_edid_alloc(priv->current_edid, priv->current_edid_len);
> + if (!edid)
> + return -EINVAL;
> +
> + drm_edid_connector_update(connector, edid);
> + connector->display_info.max_tmds_clock = 100 * 1000;
> + num_modes = drm_edid_connector_add_modes(connector);
> +
> + drm_edid_free(edid);
> +
> + return num_modes;
> +}
> +
> +static const struct drm_connector_helper_funcs dummy_connector_helper_funcs_max_tmds_clock = {
> + .atomic_check = drm_atomic_helper_connector_hdmi_check,
> + .get_modes = dummy_connector_get_modes_100MHz_max_clock,
> + .mode_valid = drm_hdmi_connector_mode_valid,
> };
>
> static void dummy_hdmi_connector_reset(struct drm_connector *connector)
> @@ -1734,9 +1775,132 @@ static struct kunit_suite drm_atomic_helper_connector_hdmi_reset_test_suite = {
> .test_cases = drm_atomic_helper_connector_hdmi_reset_tests,
> };
>
> +static void drm_test_check_mode_valid(struct kunit *test)
> +{
> + struct drm_atomic_helper_connector_hdmi_priv *priv;
> + struct drm_connector *conn;
> + struct drm_display_mode *preferred;
> +
> + priv = drm_atomic_helper_connector_hdmi_init(test,
> + BIT(HDMI_COLORSPACE_RGB),
> + 8);
> + KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> + conn = &priv->connector;
> + preferred = find_preferred_mode(conn);
> + KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> + KUNIT_EXPECT_EQ(test, preferred->hdisplay, 1920);
> + KUNIT_EXPECT_EQ(test, preferred->vdisplay, 1080);
> + KUNIT_EXPECT_EQ(test, preferred->clock, 148500);
> +}
> +
> +static void drm_test_check_mode_valid_reject(struct kunit *test)
> +{
> + struct drm_atomic_helper_connector_hdmi_priv *priv;
> + struct drm_connector *conn;
> + struct drm_display_mode *preferred;
> + struct drm_device *drm;
> + int ret;
> +
> + priv = drm_atomic_helper_connector_hdmi_init(test,
> + BIT(HDMI_COLORSPACE_RGB),
> + 8);
> + KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> + conn = &priv->connector;
> +
> + /* You shouldn't be doing that at home. */
> + conn->hdmi.funcs = &reject_connector_hdmi_funcs;
> +
> + priv->current_edid = test_edid_hdmi_1080p_rgb_max_200mhz;
> + priv->current_edid_len = ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz);
> +
> + drm = &priv->drm;
> +
> + mutex_lock(&drm->mode_config.mutex);
> + ret = conn->funcs->fill_modes(conn, 4096, 4096);
> + mutex_unlock(&drm->mode_config.mutex);
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + preferred = find_preferred_mode(conn);
> + KUNIT_ASSERT_NULL(test, preferred);
> +}
> +
> +static void drm_test_check_mode_valid_reject_rate(struct kunit *test)
> +{
> + struct drm_atomic_helper_connector_hdmi_priv *priv;
> + struct drm_connector *conn;
> + struct drm_display_mode *preferred;
> + int ret;
> +
> + priv = drm_atomic_helper_connector_hdmi_init(test,
> + BIT(HDMI_COLORSPACE_RGB),
> + 8);
> + KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> + conn = &priv->connector;
> +
> + /* You shouldn't be doing that at home. */
> + conn->hdmi.funcs = &reject_100_MHz_connector_hdmi_funcs;
> +
> + ret = set_connector_edid(test, conn,
> + test_edid_hdmi_1080p_rgb_max_200mhz,
> + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + preferred = find_preferred_mode(conn);
> + KUNIT_ASSERT_NOT_NULL(test, preferred);
> + KUNIT_EXPECT_EQ(test, preferred->hdisplay, 640);
> + KUNIT_EXPECT_EQ(test, preferred->vdisplay, 480);
> + KUNIT_EXPECT_EQ(test, preferred->clock, 25200);
> +}
> +
> +static void drm_test_check_mode_valid_reject_max_clock(struct kunit *test)
> +{
> + struct drm_atomic_helper_connector_hdmi_priv *priv;
> + struct drm_connector *conn;
> + struct drm_display_mode *preferred;
> + int ret;
> +
> + priv = drm_atomic_helper_connector_hdmi_init(test,
> + BIT(HDMI_COLORSPACE_RGB),
> + 8);
> + KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> + conn = &priv->connector;
> +
> + drm_connector_helper_add(conn, &dummy_connector_helper_funcs_max_tmds_clock);
> +
> + ret = set_connector_edid(test, conn,
> + test_edid_hdmi_1080p_rgb_max_200mhz,
> + ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
> + KUNIT_ASSERT_EQ(test, ret, 0);
> +
> + preferred = find_preferred_mode(conn);
> + KUNIT_ASSERT_NOT_NULL(test, preferred);
> + KUNIT_EXPECT_EQ(test, preferred->hdisplay, 640);
> + KUNIT_EXPECT_EQ(test, preferred->vdisplay, 480);
> + KUNIT_EXPECT_EQ(test, preferred->clock, 25200);
> +}
> +
> +static struct kunit_case drm_atomic_helper_connector_hdmi_mode_valid_tests[] = {
> + KUNIT_CASE(drm_test_check_mode_valid),
> + KUNIT_CASE(drm_test_check_mode_valid_reject),
> + KUNIT_CASE(drm_test_check_mode_valid_reject_rate),
> + KUNIT_CASE(drm_test_check_mode_valid_reject_max_clock),
> + { }
> +};
> +
> +static struct kunit_suite drm_atomic_helper_connector_hdmi_mode_valid_test_suite = {
> + .name = "drm_atomic_helper_connector_hdmi_mode_valid",
> + .test_cases = drm_atomic_helper_connector_hdmi_mode_valid_tests,
> +};
> +
We need some documentation for these tests too, and what you're trying
to test exactly with that 100MHz cutout.
Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241108/7531f976/attachment-0001.sig>
More information about the dri-devel
mailing list