On Mon, Mar 10, 2025 at 08:42:29PM +0200, Dmitry Baryshkov wrote:
> On Mon, 10 Mar 2025 at 16:55, Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Fri, Mar 07, 2025 at 07:55:52AM +0200, Dmitry Baryshkov wrote:
> > > From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > >
> > > As pointed out by Laurent, OP bits are supposed to describe operations.
> > > Split DRM_BRIDGE_OP_HDMI_AUDIO from DRM_BRIDGE_OP_HDMI instead of
> > > overloading DRM_BRIDGE_OP_HDMI.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/bridge/lontium-lt9611.c | 2 +-
> > > drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 1 +
> > > drivers/gpu/drm/display/drm_bridge_connector.c | 59 +++++++++++++++++---------
> > > drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 1 +
> > > include/drm/drm_bridge.h | 23 ++++++++--
> > > 5 files changed, 61 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
> > > index 026803034231f78c17f619dc04119bdd9b2b6679..3b93c17e25c18ae0d13e9bb74553cf21dcc39f9d 100644
> > > --- a/drivers/gpu/drm/bridge/lontium-lt9611.c
> > > +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
> > > @@ -1130,7 +1130,7 @@ static int lt9611_probe(struct i2c_client *client)
> > > lt9611->bridge.of_node = client->dev.of_node;
> > > lt9611->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> > > DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_MODES |
> > > - DRM_BRIDGE_OP_HDMI;
> > > + DRM_BRIDGE_OP_HDMI | DRM_BRIDGE_OP_HDMI_AUDIO;
> > > lt9611->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > > lt9611->bridge.vendor = "Lontium";
> > > lt9611->bridge.product = "LT9611";
> > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > > index 6166f197e37b552cb8a52b7b0d23ffc632f54557..5e5f8c2f95be1f5c4633f1093b17a00f9425bb37 100644
> > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > > @@ -1077,6 +1077,7 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
> > > hdmi->bridge.ops = DRM_BRIDGE_OP_DETECT |
> > > DRM_BRIDGE_OP_EDID |
> > > DRM_BRIDGE_OP_HDMI |
> > > + DRM_BRIDGE_OP_HDMI_AUDIO |
> > > DRM_BRIDGE_OP_HPD;
> > > hdmi->bridge.of_node = pdev->dev.of_node;
> > > hdmi->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > > diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> > > index 30c736fc0067e31a97db242e5b16ea8a5b4cf359..030f98d454608a63154827c65d4822d378df3b4c 100644
> > > --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> > > +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> > > @@ -98,6 +98,13 @@ struct drm_bridge_connector {
> > > * HDMI connector infrastructure, if any (see &DRM_BRIDGE_OP_HDMI).
> > > */
> > > struct drm_bridge *bridge_hdmi;
> > > + /**
> > > + * @bridge_hdmi_audio:
> > > + *
> > > + * The bridge in the chain that implements necessary support for the
> > > + * HDMI Audio infrastructure, if any (see &DRM_BRIDGE_OP_HDMI_AUDIO).
> > > + */
> > > + struct drm_bridge *bridge_hdmi_audio;
> > > };
> > >
> > > #define to_drm_bridge_connector(x) \
> > > @@ -433,7 +440,7 @@ static int drm_bridge_connector_audio_startup(struct drm_connector *connector)
> > > to_drm_bridge_connector(connector);
> > > struct drm_bridge *bridge;
> > >
> > > - bridge = bridge_connector->bridge_hdmi;
> > > + bridge = bridge_connector->bridge_hdmi_audio;
> > > if (!bridge)
> > > return -EINVAL;
> > >
> > > @@ -451,7 +458,7 @@ static int drm_bridge_connector_audio_prepare(struct drm_connector *connector,
> > > to_drm_bridge_connector(connector);
> > > struct drm_bridge *bridge;
> > >
> > > - bridge = bridge_connector->bridge_hdmi;
> > > + bridge = bridge_connector->bridge_hdmi_audio;
> > > if (!bridge)
> > > return -EINVAL;
> > >
> > > @@ -464,7 +471,7 @@ static void drm_bridge_connector_audio_shutdown(struct drm_connector *connector)
> > > to_drm_bridge_connector(connector);
> > > struct drm_bridge *bridge;
> > >
> > > - bridge = bridge_connector->bridge_hdmi;
> > > + bridge = bridge_connector->bridge_hdmi_audio;
> > > if (!bridge)
> > > return;
> > >
> > > @@ -478,7 +485,7 @@ static int drm_bridge_connector_audio_mute_stream(struct drm_connector *connecto
> > > to_drm_bridge_connector(connector);
> > > struct drm_bridge *bridge;
> > >
> > > - bridge = bridge_connector->bridge_hdmi;
> > > + bridge = bridge_connector->bridge_hdmi_audio;
> > > if (!bridge)
> > > return -EINVAL;
> > >
> > > @@ -576,6 +583,21 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > max_bpc = bridge->max_bpc;
> > > }
> > >
> > > + if (bridge->ops & DRM_BRIDGE_OP_HDMI_AUDIO) {
> > > + if (bridge_connector->bridge_hdmi_audio)
> > > + return ERR_PTR(-EBUSY);
> > > +
> > > + if (!bridge->hdmi_audio_max_i2s_playback_channels &&
> > > + !bridge->hdmi_audio_spdif_playback)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + if (!bridge->funcs->hdmi_audio_prepare ||
> > > + !bridge->funcs->hdmi_audio_shutdown)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + bridge_connector->bridge_hdmi_audio = bridge;
> > > + }
> > > +
> > > if (!drm_bridge_get_next_bridge(bridge))
> > > connector_type = bridge->type;
> > >
> > > @@ -611,22 +633,6 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > max_bpc);
> > > if (ret)
> > > return ERR_PTR(ret);
> > > -
> > > - if (bridge->hdmi_audio_max_i2s_playback_channels ||
> > > - bridge->hdmi_audio_spdif_playback) {
> > > - if (!bridge->funcs->hdmi_audio_prepare ||
> > > - !bridge->funcs->hdmi_audio_shutdown)
> > > - return ERR_PTR(-EINVAL);
> > > -
> > > - ret = drm_connector_hdmi_audio_init(connector,
> > > - bridge->hdmi_audio_dev,
> > > - &drm_bridge_connector_hdmi_audio_funcs,
> > > - bridge->hdmi_audio_max_i2s_playback_channels,
> > > - bridge->hdmi_audio_spdif_playback,
> > > - bridge->hdmi_audio_dai_port);
> > > - if (ret)
> > > - return ERR_PTR(ret);
> > > - }
> > > } else {
> > > ret = drmm_connector_init(drm, connector,
> > > &drm_bridge_connector_funcs,
> > > @@ -635,6 +641,19 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
> > > return ERR_PTR(ret);
> > > }
> > >
> > > + if (bridge_connector->bridge_hdmi_audio) {
> > > + bridge = bridge_connector->bridge_hdmi_audio;
> > > +
> > > + ret = drm_connector_hdmi_audio_init(connector,
> > > + bridge->hdmi_audio_dev,
> > > + &drm_bridge_connector_hdmi_audio_funcs,
> > > + bridge->hdmi_audio_max_i2s_playback_channels,
> > > + bridge->hdmi_audio_spdif_playback,
> > > + bridge->hdmi_audio_dai_port);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> > > + }
> > > +
> > > drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
> > >
> > > if (bridge_connector->bridge_hpd)
> > > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > > index 1456354c8af4bc7f655e8a47e958e9e0b99b7d29..ab6c8bc4a30b681f7de8ca7031f833795d1f7d94 100644
> > > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > > @@ -515,6 +515,7 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi)
> > > bridge->ops = DRM_BRIDGE_OP_HPD |
> > > DRM_BRIDGE_OP_DETECT |
> > > DRM_BRIDGE_OP_HDMI |
> > > + DRM_BRIDGE_OP_HDMI_AUDIO |
> > > DRM_BRIDGE_OP_EDID;
> > > bridge->hdmi_audio_max_i2s_playback_channels = 8;
> > > bridge->hdmi_audio_dev = &hdmi->pdev->dev;
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index d4c75d59fa12be1bd7375ce3ea56415235781b28..dff8cf035b30d5c7e00bfdf5d6e12802559823ba 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -693,8 +693,10 @@ struct drm_bridge_funcs {
> > > /**
> > > * @hdmi_audio_prepare:
> > > * Configures HDMI-encoder for audio stream. Can be called multiple
> > > - * times for each setup. Mandatory if HDMI audio is enabled in the
> > > - * bridge's configuration.
> > > + * times for each setup.
> > > + *
> > > + * This callback is optional but it must be implemented by bridges that
> > > + * set the DRM_BRIDGE_OP_HDMI_AUDIO flag in their &drm_bridge->ops.
> > > *
> > > * Returns:
> > > * 0 on success, a negative error code otherwise
> > > @@ -707,8 +709,10 @@ struct drm_bridge_funcs {
> > > /**
> > > * @hdmi_audio_shutdown:
> > > *
> > > - * Shut down the audio stream. Mandatory if HDMI audio is enabled in
> > > - * the bridge's configuration.
> > > + * Shut down the audio stream.
> > > + *
> > > + * This callback is optional but it must be implemented by bridges that
> > > + * set the DRM_BRIDGE_OP_HDMI_AUDIO flag in their &drm_bridge->ops.
> > > *
> > > * Returns:
> > > * 0 on success, a negative error code otherwise
> > > @@ -814,6 +818,17 @@ enum drm_bridge_ops {
> > > * drivers.
> > > */
> > > DRM_BRIDGE_OP_HDMI = BIT(4),
> > > + /**
> > > + * @DRM_BRIDGE_OP_HDMI_AUDIO: The bridge provides HDMI audio operations.
> > > + * Bridges that set this flag must implement the
> > > + * &drm_bridge_funcs->hdmi_audio_prepare and
> > > + * &drm_bridge_funcs->hdmi_audio_shutdown callbacks.
> > > + *
> > > + * 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_AUDIO = BIT(5),
> >
> > We should make this conditional on HDMI being set. It doesn't make sense
> > to have OP_HDMI_AUDIO enabled when OP_HDMI isn't.
>
> It totally does.
I'm sure it works properly. I meant on a conceptual level. In our
codebase, as it is today, the HDMI audio support is part of the HDMI
infrastructure, and thus implementing audio without the main part
doesn't make sense. IIRC, the spec also mandates video support, but
audio is optional.
> In the second patch I'm using OP_HDMI_AUDIO for the DisplayPort
> driver.
Let's discuss that part in your second patch.
Maxime
Attachment:
signature.asc
Description: PGP signature
