[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <gqizmvyunfzhxwo4qpbqwe26le5dkodu6sbur4npuoc5ij6f3n@ldqbdmah5fsd>
Date: Tue, 11 Mar 2025 19:50:50 +0200
From: Dmitry Baryshkov <lumag@...nel.org>
To: Maxime Ripard <mripard@...nel.org>
Cc: Dmitry Baryshkov <dbaryshkov@...il.com>,
Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>, Hermes Wu <Hermes.wu@....com.tw>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org
Subject: Re: [PATCH v5 1/2] drm/bridge: split HDMI Audio from
DRM_BRIDGE_OP_HDMI
On Tue, Mar 11, 2025 at 04:58:59PM +0100, Maxime Ripard wrote:
> 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 <mripard@...nel.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Fri, Mar 07, 2025 at 07:55:52AM +0200, Dmitry Baryshkov wrote:
> > > > > > From: Dmitry Baryshkov <dmitry.baryshkov@...aro.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.baryshkov@...aro.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.
Why? OP_HDMI_AUDIO has nothing to do with InfoFrames, it's just HDMI
codec. We need infoframes and tmds_char_rate_valid for OP_HDMI.
>
> 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.
Well, you can guess, I'd like to submit a patch for that driver as soon
as we settle on OP_HDMI_AUDIO.
--
With best wishes
Dmitry
Powered by blists - more mailing lists