Re: [PATCH v2 1/6] drm/display: hdmi: add generic mode_valid helper

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 10730 bytes --]

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@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]