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. > In the second patch I'm using OP_HDMI_AUDIO for the DisplayPort > driver. Let's discuss that part in your second patch. Maxime
signature.asc
Description: PGP signature
