diff mbox series

[v10,67/69] drm/connector: Drop redundant hdmi vendor/product fields

Message ID 20260731-dw-hdmi-qp-scramb-v10-67-294364b2cf15@collabora.com (mailing list archive)
State New
Headers
Series Add HDMI 2.0 support to DW HDMI QP TX |

Commit Message

Cristian Ciocaltea July 31, 2026, 4:20 p.m. UTC 
Now that all users migrated to the new drmm_connector_hdmi_init()
signature, vendor and product are provided through struct
drm_connector_hdmi_funcs, a reference to which is already stored in
drm_connector_hdmi.

Drop the redundant fields from drm_connector_hdmi and point its users to
hdmi.funcs->vendor and hdmi.funcs->product instead.

This allows simplifying the related connector registration tests by
getting rid of the now unnecessary KUNIT_EXPECT_MEMEQ() checks.

Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/display/drm_hdmi_state_helper.c |  4 +--
 drivers/gpu/drm/drm_connector.c                 |  4 ---
 drivers/gpu/drm/tests/drm_connector_test.c      | 41 +++----------------------
 include/drm/drm_connector.h                     | 14 ++-------
 4 files changed, 8 insertions(+), 55 deletions(-)

Comments

Maxime Ripard Aug. 20, 2026, 10:10 a.m. UTC | #1 
On Fri, Jul 31, 2026 at 07:20:14PM +0300, Cristian Ciocaltea wrote:
> Now that all users migrated to the new drmm_connector_hdmi_init()
> signature, vendor and product are provided through struct
> drm_connector_hdmi_funcs, a reference to which is already stored in
> drm_connector_hdmi.
> 
> Drop the redundant fields from drm_connector_hdmi and point its users to
> hdmi.funcs->vendor and hdmi.funcs->product instead.
> 
> This allows simplifying the related connector registration tests by
> getting rid of the now unnecessary KUNIT_EXPECT_MEMEQ() checks.
> 
> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c |  4 +--
>  drivers/gpu/drm/drm_connector.c                 |  4 ---
>  drivers/gpu/drm/tests/drm_connector_test.c      | 41 +++----------------------
>  include/drm/drm_connector.h                     | 14 ++-------
>  4 files changed, 8 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 04c9e06b9dc1..83a8fcb41343 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -784,8 +784,8 @@ static int hdmi_generate_spd_infoframe(const struct drm_connector *connector,
>  		return 0;
>  
>  	ret = hdmi_spd_infoframe_init(frame,
> -				      connector->hdmi.vendor,
> -				      connector->hdmi.product);
> +				      connector->hdmi.funcs->vendor,
> +				      connector->hdmi.funcs->product);
>  	if (ret)
>  		return ret;
>  
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index b2e8d99c0e9a..f5f38dc4bfa6 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -656,10 +656,6 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
>  	}
>  
> -	/* TODO: remove after conversion to new drmm_connector_hdmi_init() */
> -	strtomem_pad(connector->hdmi.vendor, hdmi_funcs->vendor, 0);
> -	strtomem_pad(connector->hdmi.product, hdmi_funcs->product, 0);
> -
>  	/*
>  	 * drm_connector_attach_max_bpc_property() requires the
>  	 * connector to have a state.
> diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c
> index 3d8500ad8be9..42f804500de6 100644
> --- a/drivers/gpu/drm/tests/drm_connector_test.c
> +++ b/drivers/gpu/drm/tests/drm_connector_test.c
> @@ -771,14 +771,11 @@ static void drm_test_connector_hdmi_init_null_product(struct kunit *test)
>  
>  /*
>   * Test that the registration of a connector with a valid, shorter than
> - * the max length, product name succeeds, and is stored padded with 0.
> + * the max length, product name succeeds.
>   */
>  static void drm_test_connector_hdmi_init_product_valid(struct kunit *test)
>  {
>  	struct drm_connector_init_priv *priv = test->priv;
> -	const unsigned char expected_product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] = {
> -		'P', 'r', 'o', 'd',
> -	};
>  	int ret;
>  
>  	priv->hdmi_funcs = dummy_hdmi_funcs;
> @@ -793,25 +790,15 @@ static void drm_test_connector_hdmi_init_product_valid(struct kunit *test)
>  				       DRM_MODE_CONNECTOR_HDMIA,
>  				       &priv->ddc);
>  	KUNIT_EXPECT_EQ(test, ret, 0);
> -	KUNIT_EXPECT_MEMEQ(test,
> -			   priv->connector.hdmi.product,
> -			   expected_product,
> -			   sizeof(priv->connector.hdmi.product));
>  }
>  
>  /*
>   * Test that the registration of a connector with a valid, at max
> - * length, product name succeeds, and is stored padded without any
> - * trailing \0.
> + * length, product name succeeds.
>   */
>  static void drm_test_connector_hdmi_init_product_length_exact(struct kunit *test)
>  {
>  	struct drm_connector_init_priv *priv = test->priv;
> -	const unsigned char expected_product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] = {
> -		'P', 'r', 'o', 'd', 'u', 'c', 't',
> -		'P', 'r', 'o', 'd', 'u', 'c', 't',
> -		'P', 'r',
> -	};
>  	int ret;
>  
>  	priv->hdmi_funcs = dummy_hdmi_funcs;
> @@ -826,10 +813,6 @@ static void drm_test_connector_hdmi_init_product_length_exact(struct kunit *test
>  				       DRM_MODE_CONNECTOR_HDMIA,
>  				       &priv->ddc);
>  	KUNIT_EXPECT_EQ(test, ret, 0);
> -	KUNIT_EXPECT_MEMEQ(test,
> -			   priv->connector.hdmi.product,
> -			   expected_product,
> -			   sizeof(priv->connector.hdmi.product));
>  }
>
>  /*
> @@ -857,14 +840,11 @@ static void drm_test_connector_hdmi_init_product_length_too_long(struct kunit *t
>  
>  /*
>   * Test that the registration of a connector with a vendor name smaller
> - * than the maximum length succeeds, and is stored padded with zeros.
> + * than the maximum length succeeds.
>   */
>  static void drm_test_connector_hdmi_init_vendor_valid(struct kunit *test)
>  {
>  	struct drm_connector_init_priv *priv = test->priv;
> -	const char expected_vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] = {
> -		'V', 'e', 'n', 'd',
> -	};
>  	int ret;
>  
>  	priv->hdmi_funcs = dummy_hdmi_funcs;
> @@ -879,24 +859,15 @@ static void drm_test_connector_hdmi_init_vendor_valid(struct kunit *test)
>  				       DRM_MODE_CONNECTOR_HDMIA,
>  				       &priv->ddc);
>  	KUNIT_EXPECT_EQ(test, ret, 0);
> -	KUNIT_EXPECT_MEMEQ(test,
> -			   priv->connector.hdmi.vendor,
> -			   expected_vendor,
> -			   sizeof(priv->connector.hdmi.vendor));
>  }
>  
>  /*
>   * Test that the registration of a connector with a vendor name at the
> - * maximum length succeeds, and is stored padded without the trailing
> - * zero.
> + * maximum length succeeds.
>   */
>  static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
>  {
>  	struct drm_connector_init_priv *priv = test->priv;
> -	const char expected_vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] = {
> -		'V', 'e', 'n', 'd', 'o', 'r',
> -		'V', 'e',
> -	};
>  	int ret;
>  
>  	priv->hdmi_funcs = dummy_hdmi_funcs;
> @@ -911,10 +882,6 @@ static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
>  				       DRM_MODE_CONNECTOR_HDMIA,
>  				       &priv->ddc);
>  	KUNIT_EXPECT_EQ(test, ret, 0);
> -	KUNIT_EXPECT_MEMEQ(test,
> -			   priv->connector.hdmi.vendor,
> -			   expected_vendor,
> -			   sizeof(priv->connector.hdmi.vendor));
>  }

Unfortunately, these tests were useful, and are there to match what the
spec asks for.

Maxime
Cristian Ciocaltea Aug. 21, 2026, 3:30 p.m. UTC | #2 
On 8/20/26 1:10 PM, Maxime Ripard wrote:
> On Fri, Jul 31, 2026 at 07:20:14PM +0300, Cristian Ciocaltea wrote:
>> Now that all users migrated to the new drmm_connector_hdmi_init()
>> signature, vendor and product are provided through struct
>> drm_connector_hdmi_funcs, a reference to which is already stored in
>> drm_connector_hdmi.
>>
>> Drop the redundant fields from drm_connector_hdmi and point its users to
>> hdmi.funcs->vendor and hdmi.funcs->product instead.
>>
>> This allows simplifying the related connector registration tests by
>> getting rid of the now unnecessary KUNIT_EXPECT_MEMEQ() checks.
>>
>> Tested-by: Diederik de Haas <diederik@cknow-tech.com>  # NanoPC-T6 LTS, Rock 5B
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/display/drm_hdmi_state_helper.c |  4 +--
>>  drivers/gpu/drm/drm_connector.c                 |  4 ---
>>  drivers/gpu/drm/tests/drm_connector_test.c      | 41 +++----------------------
>>  include/drm/drm_connector.h                     | 14 ++-------
>>  4 files changed, 8 insertions(+), 55 deletions(-)
>>
[...]

>>  /*
>>   * Test that the registration of a connector with a vendor name at the
>> - * maximum length succeeds, and is stored padded without the trailing
>> - * zero.
>> + * maximum length succeeds.
>>   */
>>  static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
>>  {
>>  	struct drm_connector_init_priv *priv = test->priv;
>> -	const char expected_vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] = {
>> -		'V', 'e', 'n', 'd', 'o', 'r',
>> -		'V', 'e',
>> -	};
>>  	int ret;
>>  
>>  	priv->hdmi_funcs = dummy_hdmi_funcs;
>> @@ -911,10 +882,6 @@ static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
>>  				       DRM_MODE_CONNECTOR_HDMIA,
>>  				       &priv->ddc);
>>  	KUNIT_EXPECT_EQ(test, ret, 0);
>> -	KUNIT_EXPECT_MEMEQ(test,
>> -			   priv->connector.hdmi.vendor,
>> -			   expected_vendor,
>> -			   sizeof(priv->connector.hdmi.vendor));
>>  }
> 
> Unfortunately, these tests were useful, and are there to match what the
> spec asks for.

I've just added a new test to cover this, as well as a couple of prerequisites
to consolidate SPD InfoFrame handling:

* video/hdmi: Define SPD InfoFrame field lengths and use strtomem_pad()

  HDMI specification defines the SPD InfoFrame Vendor Name and Product
  Description as fixed-size fields, 8 and 16 bytes respectively, padded
  with zeros and left without any trailing NUL when a name spans the whole
  field.

  Give those lengths a name and mark the fields as non-strings, so that
  the copies can be handed over to strtomem_pad(), which implements
  precisely the required semantics.  This also bounds the reads from the
  source strings, whereas the open-coded strlen() could run past the end
  of the buffer in the hdmi_spd_infoframe_unpack() path, where the names
  come straight from the wire and are not NUL-terminated.

  While at it, replace the related magic numbers in the pack and unpack
  helpers with the new defines.

* drm/connector: Use the SPD InfoFrame field length defines

  DRM_CONNECTOR_HDMI_{VENDOR,PRODUCT}_LEN used to size the vendor and
  product arrays in struct drm_connector_hdmi.  Those arrays are gone and
  both names are now only validated before being copied into the SPD
  InfoFrame, hence the limits they have to be checked against are the ones
  of the SPD InfoFrame fields themselves.

  Switch the remaining users over to
  HDMI_SPD_INFOFRAME_{VENDOR,PRODUCT}_LEN and drop the DRM specific
  defines, so that the two cannot drift apart.

* drm/tests: hdmi: Add SPD InfoFrame vendor/product coverage

  The vendor and product strings provided through struct
  drm_connector_hdmi_funcs end up in the SPD InfoFrame, whose fields are
  defined by the HDMI specification as fixed-size: 8 bytes for the vendor
  name and 16 bytes for the product description, padded with zeros and
  left without any trailing NUL when a name spans the whole field.

  Nothing exercises that so far, since the SPD InfoFrame is only generated
  for connectors implementing the related hooks, which none of the
  existing test funcs provides.

  Add a connector variant supplying those hooks, along with parametrized
  tests covering both the padded and the exact length cases.

Thanks,
Cristian
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index 04c9e06b9dc1..83a8fcb41343 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -784,8 +784,8 @@  static int hdmi_generate_spd_infoframe(const struct drm_connector *connector,
 		return 0;
 
 	ret = hdmi_spd_infoframe_init(frame,
-				      connector->hdmi.vendor,
-				      connector->hdmi.product);
+				      connector->hdmi.funcs->vendor,
+				      connector->hdmi.funcs->product);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index b2e8d99c0e9a..f5f38dc4bfa6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -656,10 +656,6 @@  int drmm_connector_hdmi_init(struct drm_device *dev,
 		connector->hdmi.max_tmds_char_rate = hdmi_funcs->supported_tmds_char_rate;
 	}
 
-	/* TODO: remove after conversion to new drmm_connector_hdmi_init() */
-	strtomem_pad(connector->hdmi.vendor, hdmi_funcs->vendor, 0);
-	strtomem_pad(connector->hdmi.product, hdmi_funcs->product, 0);
-
 	/*
 	 * drm_connector_attach_max_bpc_property() requires the
 	 * connector to have a state.
diff --git a/drivers/gpu/drm/tests/drm_connector_test.c b/drivers/gpu/drm/tests/drm_connector_test.c
index 3d8500ad8be9..42f804500de6 100644
--- a/drivers/gpu/drm/tests/drm_connector_test.c
+++ b/drivers/gpu/drm/tests/drm_connector_test.c
@@ -771,14 +771,11 @@  static void drm_test_connector_hdmi_init_null_product(struct kunit *test)
 
 /*
  * Test that the registration of a connector with a valid, shorter than
- * the max length, product name succeeds, and is stored padded with 0.
+ * the max length, product name succeeds.
  */
 static void drm_test_connector_hdmi_init_product_valid(struct kunit *test)
 {
 	struct drm_connector_init_priv *priv = test->priv;
-	const unsigned char expected_product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] = {
-		'P', 'r', 'o', 'd',
-	};
 	int ret;
 
 	priv->hdmi_funcs = dummy_hdmi_funcs;
@@ -793,25 +790,15 @@  static void drm_test_connector_hdmi_init_product_valid(struct kunit *test)
 				       DRM_MODE_CONNECTOR_HDMIA,
 				       &priv->ddc);
 	KUNIT_EXPECT_EQ(test, ret, 0);
-	KUNIT_EXPECT_MEMEQ(test,
-			   priv->connector.hdmi.product,
-			   expected_product,
-			   sizeof(priv->connector.hdmi.product));
 }
 
 /*
  * Test that the registration of a connector with a valid, at max
- * length, product name succeeds, and is stored padded without any
- * trailing \0.
+ * length, product name succeeds.
  */
 static void drm_test_connector_hdmi_init_product_length_exact(struct kunit *test)
 {
 	struct drm_connector_init_priv *priv = test->priv;
-	const unsigned char expected_product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] = {
-		'P', 'r', 'o', 'd', 'u', 'c', 't',
-		'P', 'r', 'o', 'd', 'u', 'c', 't',
-		'P', 'r',
-	};
 	int ret;
 
 	priv->hdmi_funcs = dummy_hdmi_funcs;
@@ -826,10 +813,6 @@  static void drm_test_connector_hdmi_init_product_length_exact(struct kunit *test
 				       DRM_MODE_CONNECTOR_HDMIA,
 				       &priv->ddc);
 	KUNIT_EXPECT_EQ(test, ret, 0);
-	KUNIT_EXPECT_MEMEQ(test,
-			   priv->connector.hdmi.product,
-			   expected_product,
-			   sizeof(priv->connector.hdmi.product));
 }
 
 /*
@@ -857,14 +840,11 @@  static void drm_test_connector_hdmi_init_product_length_too_long(struct kunit *t
 
 /*
  * Test that the registration of a connector with a vendor name smaller
- * than the maximum length succeeds, and is stored padded with zeros.
+ * than the maximum length succeeds.
  */
 static void drm_test_connector_hdmi_init_vendor_valid(struct kunit *test)
 {
 	struct drm_connector_init_priv *priv = test->priv;
-	const char expected_vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] = {
-		'V', 'e', 'n', 'd',
-	};
 	int ret;
 
 	priv->hdmi_funcs = dummy_hdmi_funcs;
@@ -879,24 +859,15 @@  static void drm_test_connector_hdmi_init_vendor_valid(struct kunit *test)
 				       DRM_MODE_CONNECTOR_HDMIA,
 				       &priv->ddc);
 	KUNIT_EXPECT_EQ(test, ret, 0);
-	KUNIT_EXPECT_MEMEQ(test,
-			   priv->connector.hdmi.vendor,
-			   expected_vendor,
-			   sizeof(priv->connector.hdmi.vendor));
 }
 
 /*
  * Test that the registration of a connector with a vendor name at the
- * maximum length succeeds, and is stored padded without the trailing
- * zero.
+ * maximum length succeeds.
  */
 static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
 {
 	struct drm_connector_init_priv *priv = test->priv;
-	const char expected_vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] = {
-		'V', 'e', 'n', 'd', 'o', 'r',
-		'V', 'e',
-	};
 	int ret;
 
 	priv->hdmi_funcs = dummy_hdmi_funcs;
@@ -911,10 +882,6 @@  static void drm_test_connector_hdmi_init_vendor_length_exact(struct kunit *test)
 				       DRM_MODE_CONNECTOR_HDMIA,
 				       &priv->ddc);
 	KUNIT_EXPECT_EQ(test, ret, 0);
-	KUNIT_EXPECT_MEMEQ(test,
-			   priv->connector.hdmi.vendor,
-			   expected_vendor,
-			   sizeof(priv->connector.hdmi.vendor));
 }
 
 /*
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 2ad8d1611e84..f7627de624de 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1418,11 +1418,13 @@  struct drm_connector_infoframe_funcs {
  * and controller capabilities
  */
 struct drm_connector_hdmi_funcs {
+#define DRM_CONNECTOR_HDMI_VENDOR_LEN	8
 	/**
 	 * @vendor: HDMI Controller Vendor name.
 	 */
 	const char *vendor;
 
+#define DRM_CONNECTOR_HDMI_PRODUCT_LEN	16
 	/**
 	 * @product: HDMI Controller Product name
 	 */
@@ -2123,18 +2125,6 @@  struct drm_connector_hdmi_audio {
  * struct drm_connector_hdmi - DRM Connector HDMI-related structure
  */
 struct drm_connector_hdmi {
-#define DRM_CONNECTOR_HDMI_VENDOR_LEN	8
-	/**
-	 * @vendor: HDMI Controller Vendor Name
-	 */
-	unsigned char vendor[DRM_CONNECTOR_HDMI_VENDOR_LEN] __nonstring;
-
-#define DRM_CONNECTOR_HDMI_PRODUCT_LEN	16
-	/**
-	 * @product: HDMI Controller Product Name
-	 */
-	unsigned char product[DRM_CONNECTOR_HDMI_PRODUCT_LEN] __nonstring;
-
 	/**
 	 * @supported_formats: Bitmask of @drm_output_color_format
 	 * supported by the controller.