linux-kernel - Re: [PATCH v2 5/7] drm/tests: hdmi: Add macros to simplify EDID setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250311-spiritual-hornet-of-prestige-ef4132@houat>
Date: Tue, 11 Mar 2025 17:12:42 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Dmitry Baryshkov <dmitry.baryshkov@...aro.org>, kernel@...labora.com, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/7] drm/tests: hdmi: Add macros to simplify EDID setup

On Tue, Mar 11, 2025 at 12:57:37PM +0200, Cristian Ciocaltea wrote:
> Introduce a few macros to facilitate setting custom (i.e. non-default)
> EDID data during connector initialization.
> 
> This helps reducing boilerplate code while also drops some redundant
> calls to set_connector_edid().
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@...labora.com>
> ---
>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 245 ++++++++-------------
>  1 file changed, 93 insertions(+), 152 deletions(-)
> 
> 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 e97efd3af9ed18e6cf8ee66b4923dfc805b34e19..a3f7f3ce31c73335c2c2643bdc5395b6ceb6f071 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -183,10 +183,12 @@ static const struct drm_connector_funcs dummy_connector_funcs = {
>  
>  static
>  struct drm_atomic_helper_connector_hdmi_priv *
> -drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
> -					   unsigned int formats,
> -					   unsigned int max_bpc,
> -					   const struct drm_connector_hdmi_funcs *hdmi_funcs)
> +connector_hdmi_init_funcs_set_edid(struct kunit *test,
> +				   unsigned int formats,
> +				   unsigned int max_bpc,
> +				   const struct drm_connector_hdmi_funcs *hdmi_funcs,
> +				   const char *edid_data,
> +				   size_t edid_len)
>  {
>  	struct drm_atomic_helper_connector_hdmi_priv *priv;
>  	struct drm_connector *conn;
> @@ -240,30 +242,27 @@ drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
>  
>  	drm_mode_config_reset(drm);
>  
> +	if (edid_data && edid_len) {
> +		ret = set_connector_edid(test, &priv->connector, edid_data, edid_len);
> +		KUNIT_ASSERT_GT(test, ret, 0);
> +	}
> +
>  	return priv;
>  }
>  
> -static
> -struct drm_atomic_helper_connector_hdmi_priv *
> -drm_kunit_helper_connector_hdmi_init(struct kunit *test,
> -				     unsigned int formats,
> -				     unsigned int max_bpc)
> -{
> -	struct drm_atomic_helper_connector_hdmi_priv *priv;
> -	int ret;
> +#define drm_kunit_helper_connector_hdmi_init_funcs_set_edid(test, formats, max_bpc, funcs, edid) \
> +	connector_hdmi_init_funcs_set_edid(test, formats, max_bpc, funcs, edid, ARRAY_SIZE(edid))
>  
> -	priv = drm_kunit_helper_connector_hdmi_init_funcs(test,
> -							  formats, max_bpc,
> -							  &dummy_connector_hdmi_funcs);
> -	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
> +#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc, funcs)		\
> +	connector_hdmi_init_funcs_set_edid(test, formats, max_bpc, funcs, NULL, 0)
>  
> -	ret = set_connector_edid(test, &priv->connector,
> -				 test_edid_hdmi_1080p_rgb_max_200mhz,
> -				 ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
> -	KUNIT_ASSERT_GT(test, ret, 0);
> +#define drm_kunit_helper_connector_hdmi_init_set_edid(test, formats, max_bpc, edid)		\
> +	drm_kunit_helper_connector_hdmi_init_funcs_set_edid(test, formats, max_bpc,		\
> +							    &dummy_connector_hdmi_funcs, edid)
>  
> -	return priv;
> -}
> +#define drm_kunit_helper_connector_hdmi_init(test, formats, max_bpc)				\
> +	drm_kunit_helper_connector_hdmi_init_set_edid(test, formats, max_bpc,			\
> +						      test_edid_hdmi_1080p_rgb_max_200mhz)

I'd really prefer to have functions to macros here. They are easier to
read, extend, and don't have any particular drawbacks.

I also don't think we need that many, looking at the tests:

  - We need drm_kunit_helper_connector_hdmi_init() to setup a connector
    with test_edid_hdmi_1080p_rgb_max_200mhz and
    dummy_connector_hdmi_funcs()

  - We need to create a
    drm_kunit_helper_connector_hdmi_init_with_edid_funcs() to pass both
    the funcs and edid pointers

And that's it, right?

>  /*
>   * Test that if we change the RGB quantization property to a different
> @@ -771,19 +770,15 @@ static void drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
>  	struct drm_crtc *crtc;
>  	int ret;
>  
> -	priv = drm_kunit_helper_connector_hdmi_init(test,
> -						    BIT(HDMI_COLORSPACE_RGB),
> -						    10);
> +	priv = drm_kunit_helper_connector_hdmi_init_set_edid(test,
> +				BIT(HDMI_COLORSPACE_RGB),
> +				10,
> +				test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);

I think that convertion should be part of another patch.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)