On Thu, May 30, 2024 at 11:01:26AM +0200, Maxime Ripard wrote:
> Hi,
>
> On Thu, May 30, 2024 at 02:12:26AM GMT, Dmitry Baryshkov wrote:
> > In order to let bridge chains implement HDMI connector infrastructure,
> > add necessary glue code to the drm_bridge_connector. In case there is a
> > bridge that sets DRM_BRIDGE_OP_HDMI, drm_bridge_connector will register
> > itself as a HDMI connector and provide proxy drm_connector_hdmi_funcs
> > implementation.
> >
> > Note, to simplify implementation, there can be only one bridge in a
> > chain that sets DRM_BRIDGE_OP_HDMI. Setting more than one is considered
> > an error. This limitation can be lifted later, if the need arises.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> > ---
> > drivers/gpu/drm/drm_bridge_connector.c | 101
> > ++++++++++++++++++++++++++++++++-
> > drivers/gpu/drm/drm_debugfs.c | 2 +
> > include/drm/drm_bridge.h | 82 ++++++++++++++++++++++++++
> > 3 files changed, 182 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge_connector.c
> > b/drivers/gpu/drm/drm_bridge_connector.c
> > index e093fc8928dc..8dca3eda5381 100644
> > --- a/drivers/gpu/drm/drm_bridge_connector.c
> > +++ b/drivers/gpu/drm/drm_bridge_connector.c
> > @@ -18,6 +18,7 @@
> > #include <drm/drm_managed.h>
> > #include <drm/drm_modeset_helper_vtables.h>
> > #include <drm/drm_probe_helper.h>
> > +#include <drm/display/drm_hdmi_state_helper.h>
> >
> > /**
> > * DOC: overview
> > @@ -87,6 +88,13 @@ struct drm_bridge_connector {
> > * connector modes detection, if any (see &DRM_BRIDGE_OP_MODES).
> > */
> > struct drm_bridge *bridge_modes;
> > + /**
> > + * @bridge_hdmi:
> > + *
> > + * The bridge in the chain that implements necessary support for the
> > + * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
> > + */
> > + struct drm_bridge *bridge_hdmi;
> > };
> >
> > #define to_drm_bridge_connector(x) \
> > @@ -287,6 +295,61 @@ static const struct drm_connector_helper_funcs
> > drm_bridge_connector_helper_funcs
> > .disable_hpd = drm_bridge_connector_disable_hpd,
> > };
> >
> > +static enum drm_mode_status
> > +drm_bridge_connector_tmds_char_rate_valid(const struct drm_connector
> > *connector,
> > + const struct drm_display_mode *mode,
> > + unsigned long long tmds_rate)
> > +{
> > + struct drm_bridge_connector *bridge_connector =
> > + to_drm_bridge_connector(connector);
> > + struct drm_bridge *bridge;
> > +
> > + bridge = bridge_connector->bridge_hdmi;
> > + if (bridge)
> > + return bridge->funcs->tmds_char_rate_valid ?
> > + bridge->funcs->tmds_char_rate_valid(bridge, mode,
> > tmds_rate) :
> > + MODE_OK;
> > +
> > + return MODE_ERROR;
> > +}
>
> It's kind of a nitpick, but I think the following syntax is clearer:
>
> if (bridge)
> if (bridge->funcs->tmds_char_rate_valid)
> return bridge->funcs->tmds_char_rate_valid(bridge, mode,
> tmds_rate);
> else
> return MODE_OK;
>
> return MODE_ERROR;
Ack
>
> > +static int drm_bridge_connector_clear_infoframe(struct drm_connector
> > *connector,
> > + enum hdmi_infoframe_type type)
> > +{
> > + struct drm_bridge_connector *bridge_connector =
> > + to_drm_bridge_connector(connector);
> > + struct drm_bridge *bridge;
> > +
> > + bridge = bridge_connector->bridge_hdmi;
> > + if (bridge)
> > + return bridge->funcs->clear_infoframe ?
> > + bridge->funcs->clear_infoframe(bridge, type) :
> > + 0;
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static int drm_bridge_connector_write_infoframe(struct drm_connector
> > *connector,
> > + enum hdmi_infoframe_type type,
> > + const u8 *buffer, size_t len)
> > +{
> > + struct drm_bridge_connector *bridge_connector =
> > + to_drm_bridge_connector(connector);
> > + struct drm_bridge *bridge;
> > +
> > + bridge = bridge_connector->bridge_hdmi;
> > + if (bridge)
> > + return bridge->funcs->write_infoframe(bridge, type, buffer,
> > len);
> > +
> > + return -EINVAL;
> > +}
> > +
> > +static const struct drm_connector_hdmi_funcs
> > drm_bridge_connector_hdmi_funcs = {
> > + .tmds_char_rate_valid = drm_bridge_connector_tmds_char_rate_valid,
> > + .clear_infoframe = drm_bridge_connector_clear_infoframe,
> > + .write_infoframe = drm_bridge_connector_write_infoframe,
> > +};
> > +
> > /*
> > -----------------------------------------------------------------------------
> > * Bridge Connector Initialisation
> > */
> > @@ -312,6 +375,10 @@ struct drm_connector *drm_bridge_connector_init(struct
> > drm_device *drm,
> > struct drm_connector *connector;
> > struct i2c_adapter *ddc = NULL;
> > struct drm_bridge *bridge, *panel_bridge = NULL;
> > + const char *vendor = "Unknown";
> > + const char *product = "Unknown";
> > + unsigned int supported_formats = BIT(HDMI_COLORSPACE_RGB);
> > + unsigned int max_bpc = 8;
> > int connector_type;
> > int ret;
> >
> > @@ -348,6 +415,25 @@ struct drm_connector *drm_bridge_connector_init(struct
> > drm_device *drm,
> > bridge_connector->bridge_detect = bridge;
> > if (bridge->ops & DRM_BRIDGE_OP_MODES)
> > bridge_connector->bridge_modes = bridge;
> > + if (bridge->ops & DRM_BRIDGE_OP_HDMI) {
> > + if (bridge_connector->bridge_hdmi)
> > + return ERR_PTR(-EBUSY);
> > + if (!bridge->funcs->write_infoframe)
> > + return ERR_PTR(-EINVAL);
> > +
> > + bridge_connector->bridge_hdmi = bridge;
> > +
> > + if (bridge->supported_formats)
> > + supported_formats = bridge->supported_formats;
> > + if (bridge->max_bpc)
> > + max_bpc = bridge->max_bpc;
> > + }
> > +
> > + if (bridge->vendor)
> > + vendor = bridge->vendor;
> > +
> > + if (bridge->product)
> > + product = bridge->product;
>
> I don't think we should allow HDMI bridges without a product or vendor.
> We should at the very least warn or return an error there, preferably
> the latter to start with. We can always relax the rule if it turns out
> to be too strict later on.
My goal was to be able to override the vendor/product after the HDMI
bridge. Something like setting 'Qualcomm / Snapdragon' in HDMI driver,
but then e.g. display-connector overriding it to e.g. '96board /
DB820c'. Maybe it's an overkill and we should just set vendor / product
from the HDMI bridge.
>
> > if (!drm_bridge_get_next_bridge(bridge))
> > connector_type = bridge->type;
> > @@ -370,9 +456,18 @@ struct drm_connector *drm_bridge_connector_init(struct
> > drm_device *drm,
> > return ERR_PTR(-EINVAL);
> > }
> >
> > - ret = drmm_connector_init(drm, connector,
> > - &drm_bridge_connector_funcs,
> > - connector_type, ddc);
> > + if (bridge_connector->bridge_hdmi)
> > + ret = drmm_connector_hdmi_init(drm, connector,
> > + vendor, product,
> > + &drm_bridge_connector_funcs,
> > + &drm_bridge_connector_hdmi_funcs,
> > + connector_type, ddc,
> > + supported_formats,
> > + max_bpc);
> > + else
> > + ret = drmm_connector_init(drm, connector,
> > + &drm_bridge_connector_funcs,
> > + connector_type, ddc);
> > if (ret) {
> > kfree(bridge_connector);
> > return ERR_PTR(ret);
> > diff --git a/drivers/gpu/drm/drm_debugfs.c b/drivers/gpu/drm/drm_debugfs.c
> > index dd39a5b7a711..e385d90ef893 100644
> > --- a/drivers/gpu/drm/drm_debugfs.c
> > +++ b/drivers/gpu/drm/drm_debugfs.c
> > @@ -762,6 +762,8 @@ static int bridges_show(struct seq_file *m, void *data)
> > drm_puts(&p, " hpd");
> > if (bridge->ops & DRM_BRIDGE_OP_MODES)
> > drm_puts(&p, " modes");
> > + if (bridge->ops & DRM_BRIDGE_OP_HDMI)
> > + drm_puts(&p, " hdmi");
> > drm_puts(&p, "\n");
> > }
> >
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 4baca0d9107b..c45e539fe276 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -630,6 +630,54 @@ struct drm_bridge_funcs {
> > */
> > void (*hpd_disable)(struct drm_bridge *bridge);
> >
> > + /**
> > + * @tmds_char_rate_valid:
> > + *
> > + * Check whether a particular TMDS character rate is supported by the
> > + * driver.
> > + *
> > + * This callback is optional and should only be implemented by the
> > + * bridges that take part in the HDMI connector implementation. Bridges
> > + * that implement it shall set set the DRM_BRIDGE_OP_HDMI flag in their
> > + * &drm_bridge->ops.
> > + *
> > + * Returns:
> > + *
> > + * Either &drm_mode_status.MODE_OK or one of the failure reasons
> > + * in &enum drm_mode_status.
> > + */
> > + enum drm_mode_status
> > + (*tmds_char_rate_valid)(const struct drm_bridge *bridge,
> > + const struct drm_display_mode *mode,
> > + unsigned long long tmds_rate);
>
> Would an HDMI prefix make sense here?
Yes, and in other callbacks too.
>
> > + /**
> > + * @clear_infoframe:
> > + *
> > + * This callback clears the infoframes in the hardware during commit.
> > + * It will be called multiple times, once for every disabled infoframe
> > + * type.
> > + *
> > + * This callback is optional and should only be implemented by the
> > + * bridges that take part in the HDMI connector implementation. Bridges
> > + * that implement it shall set set the DRM_BRIDGE_OP_HDMI flag in their
> > + * &drm_bridge->ops.
> > + */
> > + int (*clear_infoframe)(struct drm_bridge *bridge,
> > + enum hdmi_infoframe_type type);
>
> Missing newline there.
>
> > + /**
> > + * @write_infoframe:
> > + *
> > + * Program the infoframe into the hardware. It will be called multiple
> > + * times, once for every updated infoframe type.
> > + *
> > + * This callback is optional but it must be implemented by bridges that
> > + * set the DRM_BRIDGE_OP_HDMI flag in their &drm_bridge->ops.
> > + */
> > + int (*write_infoframe)(struct drm_bridge *bridge,
> > + enum hdmi_infoframe_type type,
> > + const u8 *buffer, size_t len);
> > +
> > /**
> > * @debugfs_init:
> > *
> > @@ -705,6 +753,16 @@ enum drm_bridge_ops {
> > * this flag shall implement the &drm_bridge_funcs->get_modes callback.
> > */
> > DRM_BRIDGE_OP_MODES = BIT(3),
> > + /**
> > + * @DRM_BRIDGE_OP_HDMI: The bridge provides HDMI connector operations,
> > + * including infoframes support. Bridges that set this flag must
> > + * implement the &drm_bridge_funcs->write_infoframe callback.
> > + *
> > + * Note: currently there can be at most one bridge in a chain that sets
> > + * this bit. This is to simplify corresponding glue code in connector
> > + * drivers.
> > + */
> > + DRM_BRIDGE_OP_HDMI = BIT(4),
> > };
> >
> > /**
> > @@ -773,6 +831,30 @@ struct drm_bridge {
> > * @hpd_cb.
> > */
> > void *hpd_data;
> > +
> > + /**
> > + * @vendor: Vendor of the product to be used for the SPD InfoFrame
> > + * generation.
> > + */
> > + const char *vendor;
> > +
> > + /**
> > + * @product: Name of the product to be used for the SPD InfoFrame
> > + * generation.
> > + */
> > + const char *product;
> > +
> > + /**
> > + * @supported_formats: Bitmask of @hdmi_colorspace listing supported
> > + * output formats. This is only relevant if @DRM_BRIDGE_OP_HDMI is set.
> > + */
> > + unsigned int supported_formats;
> > +
> > + /**
> > + * @max_bpc: Maximum bits per char the HDMI bridge supports. This is
> > + * only relevant if @DRM_BRIDGE_OP_HDMI is set.
> > + */
>
> We could probably document that the only allowed values are 8, 10 or 12.
>
> Thanks!
> Maxime
--
With best wishes
Dmitry
