On Fri, 8 Mar 2024 at 11:44, Maxime Ripard <mripard@xxxxxxxxxx> 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 <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > 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 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.
My feeling about the drm_connector_hdmi was closer to helpers, rather
than just init. In case of helpers we set them after initializing the
object rather than setting them at the init time.
>
> 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.
Yes.
>
> 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.
This is slightly problematic. First of all, it breaks layering. What
if the HDMI stream gets wrapped? E.g. into the MyDP (think of
anx7808)? Then the drm_bridge_connector will have connector_type field
equal to DRM_CONNECTOR_DisplayPort (MyDP). I have not looked into the
dual-mode DP connectors or USB-C MHL mode, but they might also cause
some wraparounds. Or changes to the connector_type.
So, I think it is incorrect to call
drm_atomic_helper_connector_hdmi_check() from the drm_bridge_connector
itself.
Likewise, to setup HDMI connector parts from drm_bridge_connector we
will have to pass vendor/product/max_bpc/formats between layers.
Setting up HDMI connector from the bridge itself saves us from such
issues.
The only possible case I have in my mind is that setting up HDMI
connector from the end of the chain would allow us to override some of
the properties. E.g. I can think of hdmi-connector overriding vendor /
product strings.
>
> - 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?
This might save us from the drm_connector.hdmi.data hassle.
Last, but not least. Your proposal means that we have to use
drm_bridge_connector. Some platforms still use bridge-created
connector. What is the status of the migration? Should we start
pushing drivers towards drm_bridge_connector in a more aggressive way?
--
With best wishes
Dmitry
