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 <[email protected]> > --- > 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
signature.asc
Description: PGP signature
