| Message ID | 20241109-hdmi-mode-valid-v3-1-5348c2368076@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/display: hdmi: add drm_hdmi_connector_mode_valid() | expand |
Hi, On Sat, Nov 09, 2024 at 02:35:05PM +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@linaro.org> > --- > drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 +++ > drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 181 ++++++++++++++++++++- > include/drm/display/drm_hdmi_state_helper.h | 4 + > 3 files changed, 204 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > index feb7a3a759811aed70c679be8704072093e2a79b..80bf2829ba89b5f84fed4fa9eb1d6302e10a4f9e 100644 > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c > @@ -521,6 +521,27 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, > } > EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); > > +/** > + * 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 hdmi_clock_valid(connector, mode, clock); > +} > +EXPORT_SYMBOL(drm_hdmi_connector_mode_valid); > + > static int clear_device_infoframe(struct drm_connector *connector, > enum hdmi_infoframe_type type) > { > 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 294773342e710dc56772f839c2db9c2e487bbc1e..67c3e882592b00d803d8cba5d183aa90339a16b4 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,14 @@ 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); > + > + /* Handle the case when all modes were rejected by mode_valid() */ > + if (!list_empty(&connector->modes)) > + preferred = list_first_entry(&connector->modes, struct drm_display_mode, head); > + Sorry if it wasn't clear in my previous review, but I would have expected a separate patch for that change. > list_for_each_entry(mode, &connector->modes, head) > if (mode->type & DRM_MODE_TYPE_PREFERRED) > preferred = mode; > @@ -125,6 +129,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 +163,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; > +} I'd rather create a new fake edid than mess with those fields directly. > +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 +1777,143 @@ 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); > +} We need a comment on what that test tests, and expects. > +/* > + * Verify that tmds_char_rate_valid() can reject all modes. > + */ > +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; Just make drm_atomic_helper_connector_hdmi_init take an extra pointer. (Also, that's a super bad name, drm_kunit_helper_connector_hdmi_init or something would probably be better) > + 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); > +} Ditto, I can't really say what this test is doing. That's true for all the tests. Maxime
diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c index feb7a3a759811aed70c679be8704072093e2a79b..80bf2829ba89b5f84fed4fa9eb1d6302e10a4f9e 100644 --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c @@ -521,6 +521,27 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector, } EXPORT_SYMBOL(drm_atomic_helper_connector_hdmi_check); +/** + * 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 hdmi_clock_valid(connector, mode, clock); +} +EXPORT_SYMBOL(drm_hdmi_connector_mode_valid); + static int clear_device_infoframe(struct drm_connector *connector, enum hdmi_infoframe_type type) { 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 294773342e710dc56772f839c2db9c2e487bbc1e..67c3e882592b00d803d8cba5d183aa90339a16b4 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,14 @@ 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); + + /* Handle the case when all modes were rejected by mode_valid() */ + if (!list_empty(&connector->modes)) + preferred = list_first_entry(&connector->modes, struct drm_display_mode, head); + list_for_each_entry(mode, &connector->modes, head) if (mode->type & DRM_MODE_TYPE_PREFERRED) preferred = mode; @@ -125,6 +129,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 +163,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 +1777,143 @@ 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); +} + +/* + * Verify that tmds_char_rate_valid() can reject all modes. + */ +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); +} + +/* + * Verify that tmds_char_rate_valid() can correctly filter out high-rate modes, + * but allows small modes. + */ +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); +} + +/* + * Verify that the framework filters modes based on info.max_tmds_clock, while + * allowing small resolution modes. + */ +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, +}; + kunit_test_suites( &drm_atomic_helper_connector_hdmi_check_test_suite, &drm_atomic_helper_connector_hdmi_reset_test_suite, + &drm_atomic_helper_connector_hdmi_mode_valid_test_suite, ); MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>"); diff --git a/include/drm/display/drm_hdmi_state_helper.h b/include/drm/display/drm_hdmi_state_helper.h index 2d45fcfa461985065a5e5ad67eddc0b1c556d526..d6d65da6d8f9ee46de33114cce2d6fbe6098a862 100644 --- a/include/drm/display/drm_hdmi_state_helper.h +++ b/include/drm/display/drm_hdmi_state_helper.h @@ -20,4 +20,8 @@ int drm_atomic_helper_connector_hdmi_clear_audio_infoframe(struct drm_connector int drm_atomic_helper_connector_hdmi_update_infoframes(struct drm_connector *connector, struct drm_atomic_state *state); +enum drm_mode_status +drm_hdmi_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode); + #endif // DRM_HDMI_STATE_HELPER_H_
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@linaro.org> --- drivers/gpu/drm/display/drm_hdmi_state_helper.c | 21 +++ drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 181 ++++++++++++++++++++- include/drm/display/drm_hdmi_state_helper.h | 4 + 3 files changed, 204 insertions(+), 2 deletions(-)