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 <[email protected]>
> ---
> 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;

> +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.

> 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?

> + /**
> + * @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

Attachments:

(No filename) (10.46 kB)
signature.asc (281.00 B)
Download all attachments

2024-05-30 09:22:23

On Thu, May 30, 2024 at 11:16:08AM +0200, Maxime Ripard wrote:
> Hi,
>
> On Thu, May 30, 2024 at 02:12:28AM GMT, Dmitry Baryshkov wrote:
> > Setup the HDMI connector on the MSM HDMI outputs. Make use of
> > atomic_check hook and of the provided Infoframe infrastructure.
> >
> > Signed-off-by: Dmitry Baryshkov <[email protected]>
>
> As a general comment: I really like it, it looks super tidy. Thanks!
>
> There's a couple of minor issues below
>
> > -int msm_hdmi_audio_update(struct hdmi *hdmi)
> > +static int msm_hdmi_audio_update(struct hdmi *hdmi)
> > {
> > struct hdmi_audio *audio = &hdmi->audio;
> > - struct hdmi_audio_infoframe *info = &audio->infoframe;
> > const struct hdmi_msm_audio_arcs *arcs = NULL;
> > bool enabled = audio->enabled;
> > uint32_t acr_pkt_ctrl, vbi_pkt_ctrl, aud_pkt_ctrl;
> > - uint32_t infofrm_ctrl, audio_config;
> > + uint32_t audio_config;
> > +
> > + if (!hdmi->hdmi_mode)
> > + return -EINVAL;
> > +
> > + DBG("audio: enabled=%d, channels=%d, rate=%d",
> > + audio->enabled, audio->channels, audio->rate);
> >
> > - DBG("audio: enabled=%d, channels=%d, channel_allocation=0x%x, "
> > - "level_shift_value=%d, downmix_inhibit=%d, rate=%d",
> > - audio->enabled, info->channels, info->channel_allocation,
> > - info->level_shift_value, info->downmix_inhibit, audio->rate);
> > DBG("video: power_on=%d, pixclock=%lu", hdmi->power_on, hdmi->pixclock);
>
> pixclock should come from the connector state too. It's still calculated
> by the driver in msm_hdmi_bridge_mode_set

Yes, that's why I asked on IRC regarding the char rate and ALSA codec.
I'll see what I can do.

>
> > @@ -341,8 +425,11 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
> > bridge->funcs = &msm_hdmi_bridge_funcs;
> > bridge->ddc = hdmi->i2c;
> > bridge->type = DRM_MODE_CONNECTOR_HDMIA;
> > + bridge->vendor = "Qualcomm";
> > + bridge->product = "Snapdragon";
> > bridge->ops = DRM_BRIDGE_OP_HPD |
> > DRM_BRIDGE_OP_DETECT |
> > + DRM_BRIDGE_OP_HDMI |
> > DRM_BRIDGE_OP_EDID;
> >
> > ret = devm_drm_bridge_add(hdmi->dev->dev, bridge);
>
> It looks like you're not setting either the supported formats or bpc?

I've added what looks to be sane defaults to the drm_bridge_connector:
RGB only and bpc = 8. If at some point we get to YUV or HDR support,
that would need to be reflected here.

--
With best wishes
Dmitry