| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250311-benevolent-goat-of-thunder-8e73bb@houat>
Date: Tue, 11 Mar 2025 16:58:59 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Dmitry Baryshkov <lumag@...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 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.
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
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists