Re: [PATCH RFC 3/6] drm/connector: hdmi: split setup code of the HDMI connector
On Fri, 8 Mar 2024 at 13:53, Maxime Ripard wrote:
>
> On Fri, Mar 08, 2024 at 12:52:04PM +0200, Dmitry Baryshkov wrote:
> > On Fri, 8 Mar 2024 at 11:44, Maxime Ripard wrote:
> > >
> > > 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
> > > > ---
> > > > 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
> > >
Re: [PATCH RFC 3/6] drm/connector: hdmi: split setup code of the HDMI connector
On Fri, Mar 08, 2024 at 12:52:04PM +0200, Dmitry Baryshkov wrote:
> On Fri, 8 Mar 2024 at 11:44, Maxime Ripard wrote:
> >
> > 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
> > > ---
> > > 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
Re: [PATCH RFC 3/6] drm/connector: hdmi: split setup code of the HDMI connector
On Fri, 8 Mar 2024 at 11:44, Maxime Ripard wrote:
>
> 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
> > ---
> > 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?
Yes.
>
> 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
Re: [PATCH RFC 3/6] drm/connector: hdmi: split setup code of the HDMI connector
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
> ---
> 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
