[PATCH RFC 3/6] drm/connector: hdmi: split setup code of the HDMI connector

[Maxime Ripard]

Hi Dmitry,

Thanks a lot for working on that, it's greatly appreciated :)

On Fri, Mar 08, 2024 at 01:57:02AM +0200, Dmitry Baryshkov wrote:
> In order to use HDMI connector extensions from the bridge drivers, carve
> out the drm_connector_hdmi_setup() from drmm_connector_hdmi_init(). This
> way the drm_bridge drivers can call new function from their
> setup_connector callbacks.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> ---
>  drivers/gpu/drm/drm_connector.c | 67 ++++++++++++++++++++++++++++++-----------
>  include/drm/drm_connector.h     |  5 +++
>  2 files changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 427816239038..ba953eb45557 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -454,15 +454,11 @@ int drmm_connector_init(struct drm_device *dev,
>  EXPORT_SYMBOL(drmm_connector_init);
>  
>  /**
> - * drmm_connector_hdmi_init - Init a preallocated HDMI connector
> - * @dev: DRM device
> + * drm_connector_hdmi_setup - Init a preallocated HDMI connector
>   * @connector: A pointer to the HDMI connector to init
>   * @vendor: HDMI Controller Vendor name
>   * @product: HDMI Controller Product name
> - * @funcs: callbacks for this connector
>   * @hdmi_funcs: HDMI-related callbacks for this connector
> - * @connector_type: user visible type of the connector
> - * @ddc: optional pointer to the associated ddc adapter
>   * @supported_formats: Bitmask of @hdmi_colorspace listing supported output formats
>   * @max_bpc: Maximum bits per char the HDMI connector supports
>   *
> @@ -477,18 +473,12 @@ EXPORT_SYMBOL(drmm_connector_init);
>   * Returns:
>   * Zero on success, error code on failure.
>   */
> -int drmm_connector_hdmi_init(struct drm_device *dev,
> -			     struct drm_connector *connector,
> +int drm_connector_hdmi_setup(struct drm_connector *connector,
>  			     const char *vendor, const char *product,
> -			     const struct drm_connector_funcs *funcs,
>  			     const struct drm_connector_hdmi_funcs *hdmi_funcs,
> -			     int connector_type,
> -			     struct i2c_adapter *ddc,
>  			     unsigned long supported_formats,
>  			     unsigned int max_bpc)
>  {
> -	int ret;
> -
>  	if (!vendor || !product)
>  		return -EINVAL;
>  
> @@ -496,8 +486,8 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  	    (strlen(product) > DRM_CONNECTOR_HDMI_PRODUCT_LEN))
>  		return -EINVAL;
>  
> -	if (!(connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> -	      connector_type == DRM_MODE_CONNECTOR_HDMIB))
> +	if (connector->connector_type != DRM_MODE_CONNECTOR_HDMIA &&
> +	    connector->connector_type != DRM_MODE_CONNECTOR_HDMIB)
>  		return -EINVAL;
>  
>  	if (!supported_formats || !(supported_formats & BIT(HDMI_COLORSPACE_RGB)))
> @@ -506,10 +496,6 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  	if (!(max_bpc == 8 || max_bpc == 10 || max_bpc == 12))
>  		return -EINVAL;
>  
> -	ret = drmm_connector_init(dev, connector, funcs, connector_type, ddc);
> -	if (ret)
> -		return ret;
> -
>  	connector->hdmi.supported_formats = supported_formats;
>  	strtomem_pad(connector->hdmi.vendor, vendor, 0);
>  	strtomem_pad(connector->hdmi.product, product, 0);
> @@ -531,6 +517,51 @@ int drmm_connector_hdmi_init(struct drm_device *dev,
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(drm_connector_hdmi_setup);

I guess it's more of a general comment on the whole design of things,
but this is the starting point I think.

None of the other DRM entities have the split between init and setup,
connectors included. So this creates a bit of oddity in the API which I
think we should avoid at all cost. API consistency is the most
important.

If I got the rest of your series properly, this all stems from the fact
that since connectors are disconnected from bridges nowadays, there's no
way to implement drm_connector_hdmi_funcs on an HDMI bridge, and
especially to get those hooks called with some sort of pointer to the
bridge private instance.

And so I assume this is why you split init in two here, and added a data
field to the HDMI part of drm_connector, so that you can init the
connector in drm_bridge_connector, and then call setup with your
drm_connector_hdmi_funcs and the private data pointer in setup so it all
works out. Right?

If so, I believe this doesn't only create an inconsistency at the KMS
core API level, but also in the bridge API. To me, bridges are meant to
fill the encoder gap, so we shouldn't special-case the core API to
accomodate the bridge design. And the bridge framework has been designed
that way too.

If you look at the way EDID or HPD handling, we fundamentally have the
same problem: the connector is supposed to implement it, but it really
is handled by the bridge driver that wants to operate with its private
instance data.

So I think we should go for a similar approach:

  - We keep the drm_hdmi_connector_init function only

  - If the drm_bridge_connector has an HDMI type, we can
    drm_hdmi_connector_init and call
    drm_atomic_helper_connector_hdmi_check() at atomic_check time.

  - We create a drm_bridge_* set of functions and associated hooks to
    handle HDMI TMDS char rate filtering and infoframes setup that gets
    called by drm_bridge_connector, and pass the bridge, connector and
    all the extra arguments we need.

Once we've done that, we're probably in a good position to support what
we want to support. The drm_connector_state is passed to the atomic set
of bridges hooks so they can just read the content from there and we
should be good?

What do you think?

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240308/4a3d07d7/attachment.sig>