On Tue, Mar 11, 2025 at 05:50:09PM +0200, Dmitry Baryshkov wrote:
> On Tue, Mar 11, 2025 at 09:36:37AM +0100, Maxime Ripard wrote:
> > On Mon, Mar 10, 2025 at 08:42:29PM +0200, Dmitry Baryshkov wrote:
> > > On Mon, 10 Mar 2025 at 16:55, Maxime Ripard <mrip...@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Fri, Mar 07, 2025 at 07:55:52AM +0200, Dmitry Baryshkov wrote:
> > > > > From: Dmitry Baryshkov <dmitry.barysh...@linaro.org>
> > > > >
> > > > > 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.barysh...@linaro.org>
> > > > > ---
> > > > > 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.
>
> I can imagine a HDMI bridge using OP_HDMI_AUDIO, but not OP_HDMI. For
> example, lontium-lt9611uxc.c. It is a 'smart' chip, which handles nearly
> everything on its own. I even don't know if there is a way to program
> the InfoFrames, etc., so I'm very skeptical about setting OP_HDMI.
> However at the same time, it would defeinitely benefit from using
> OP_HDMI_AUDIO.You're going to have the same argument for that chip for audio: if not be able to set the infoframe disqualifies, then there's audio infoframes too and thus it should be disqualified from OP_HDMI_AUDIO. I don't believe it's an issue though: if a driver doesn't want the infoframes for whatever reason, it's free to do so. It's also something I'd like to reevaluate when we actually have that problem to deal with. At the moment, it sounds like reducing the safety of the API for an hypothetical case. Maxime
signature.asc
Description: PGP signature
