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

[Maxime Ripard <mripard@kernel.org>]

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

On Fri, Aug 21, 2026 at 06:30:31PM +0300, Cristian Ciocaltea wrote:
> 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.

I'm not sure why we need to add a new test here. It's exactly what the
test above is supposed to check. Making it a shell of what it was
testing and then adding yet another one that tests what the former used
to test seems suboptimal to me.

Maxime

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