Re: [PATCH v2] drm/bridge: it6505: support hdmi_codec_ops for audio stream setup

From: Pin-yen Lin
Date: Tue Feb 04 2025 - 03:10:58 EST

Hi Hermes,

On Tue, Feb 4, 2025 at 11:49 AM <Hermes.Wu@xxxxxxxxxx> wrote:
>
> Hi
> >
> >-----Original Message-----
> >From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >Sent: Tuesday, February 4, 2025 1:28 AM
> >To: Hermes Wu (吳佳宏) <Hermes.Wu@xxxxxxxxxx>
> >Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>; Neil Armstrong <neil.armstrong@xxxxxxxxxx>; Robert Foss <rfoss@xxxxxxxxxx>; Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>; Jonas Karlman <jonas@xxxxxxxxx>; Jernej Skrabec <jernej.skrabec@xxxxxxxxx>; Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>; Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; treapking@xxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Pet Weng (翁玉芬) <Pet.Weng@xxxxxxxxxx>; Kenneth Hung (洪家倫) <Kenneth.Hung@xxxxxxxxxx>
> >Subject: Re: [PATCH v2] drm/bridge: it6505: support hdmi_codec_ops for audio stream setup
> >
> >On Mon, Feb 03, 2025 at 02:04:30PM +0800, Hermes Wu via B4 Relay wrote:
> >> From: Hermes Wu <Hermes.wu@xxxxxxxxxx>
> >>
> >> For supporting audio form I2S to DP audio data sub stream.
> >> Add hdmi_audio callbacks to drm_bridge_funcs for the HDMI codec
> >> framework. The DRM_BRIDGE_OP_HDMI flag in bridge.ops must be set, and
> >> hdmi_write_infoframe and hdmi_clear_infoframe are necessary for the
> >> drm_bridge_connector to enable the HDMI codec.
> >
> >Please split this into two commits: one adding OP_HDMI, second one adding audio support.
>
> This will need send patches with cover letter, should I keep patch version or reset it?

I would bump to v3 in this case.

Regards,
Pin-yen
>
> >>
> >> Signed-off-by: Hermes Wu <Hermes.wu@xxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >> - Use DRM HDMI codec framewrok for audio stream setup.
> >> - Link to v1:
> >> https://lore.kernel.org/r/20250121-add-audio-codec-v1-1-e3ff71b3c819@i
> >> te.com.tw
> >> ---
> >> drivers/gpu/drm/bridge/ite-it6505.c | 151
> >> +++++++++++++++++++++++++++++++-----
> >> 1 file changed, 132 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c
> >> b/drivers/gpu/drm/bridge/ite-it6505.c
> >> index
> >> 88ef76a37fe6accacdd343839ff2569b31b18ceb..361e2412b8e8f040cf4254479b60
> >> 588d99e8c99a 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
> >> @@ -1414,32 +1414,43 @@ static void it6505_variable_config(struct it6505 *it6505)
> >> memset(it6505->bksvs, 0, sizeof(it6505->bksvs)); }
> >>
> >> +static int it6505_write_avi_infoframe(struct it6505 *it6505,
> >> + const u8 *buffer, size_t len) {
> >> + struct device *dev = it6505->dev;
> >> +
> >> + if (len - HDMI_INFOFRAME_HEADER_SIZE > 13) {
> >> + DRM_DEV_DEBUG_DRIVER(dev, "AVI size fail %d", len);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, 0);
> >> + regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> >> + buffer + HDMI_INFOFRAME_HEADER_SIZE,
> >> + len - HDMI_INFOFRAME_HEADER_SIZE);
> >> +
> >> + it6505_write(it6505, REG_AVI_INFO_SUM, buffer[3]);
> >> + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT,
> >> + EN_AVI_PKT);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> static int it6505_send_video_infoframe(struct it6505 *it6505,
> >> struct hdmi_avi_infoframe *frame) {
> >> u8 buffer[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
> >> - int err;
> >> + int err, length;
> >> struct device *dev = it6505->dev;
> >>
> >> - err = hdmi_avi_infoframe_pack(frame, buffer, sizeof(buffer));
> >> - if (err < 0) {
> >> - dev_err(dev, "Failed to pack AVI infoframe: %d", err);
> >> - return err;
> >> + length = hdmi_avi_infoframe_pack(frame, buffer, sizeof(buffer));
> >> + if (length < 0) {
> >> + dev_err(dev, "Failed to pack AVI infoframe: %d", length);
> >> + return length;
> >> }
> >>
> >> - err = it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, 0x00);
> >> - if (err)
> >> - return err;
> >> -
> >> - err = regmap_bulk_write(it6505->regmap, REG_AVI_INFO_DB1,
> >> - buffer + HDMI_INFOFRAME_HEADER_SIZE,
> >> - frame->length);
> >> - if (err)
> >> - return err;
> >> -
> >> - err = it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT,
> >> - EN_AVI_PKT);
> >> - if (err)
> >> + err = it6505_write_avi_infoframe(it6505, buffer, length);
> >
> >You mustn't to do this. Please instead call
> >drm_atomic_helper_connector_hdmi_update_infoframes() from your .atomic_enable instead.
> >
> >> + if (err < 0)
> >> return err;
> >>
> >> return 0;
> >> @@ -1625,6 +1636,18 @@ static void it6505_enable_audio_infoframe(struct it6505 *it6505)
> >> EN_AUD_CTRL_PKT);
> >> }
> >>
> >> +static void it6505_write_audio_infoframe(struct it6505 *it6505,
> >> + const u8 *buffer, size_t len)
> >> +{
> >> + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AUD_PKT, 0);
> >> + regmap_bulk_write(it6505->regmap, REG_AUD_INFOFRAM_DB1,
> >> + buffer + HDMI_INFOFRAME_HEADER_SIZE,
> >> + 4);
> >> + it6505_write(it6505, REG_AUD_INFOFRAM_SUM, buffer[3]);
> >> + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AUD_PKT,
> >> + EN_AUD_PKT);
> >> +}
> >> +
> >> static void it6505_disable_audio(struct it6505 *it6505) {
> >> it6505_set_bits(it6505, REG_DATA_MUTE_CTRL, EN_AUD_MUTE,
> >> EN_AUD_MUTE); @@ -3302,6 +3325,85 @@ static const struct drm_edid *it6505_bridge_edid_read(struct drm_bridge *bridge,
> >> return drm_edid_dup(it6505->cached_edid);
> >> }
> >>
> >> +static int it6505_bridge_hdmi_audio_startup(struct drm_connector *connector,
> >> + struct drm_bridge *bridge)
> >> +{
> >> + struct it6505 *it6505 = bridge_to_it6505(bridge);
> >> + struct device *dev = it6505->dev;
> >> +
> >> + if (!it6505->powered || it6505->enable_drv_hold)
> >> + return -EIO;
> >> +
> >> + DRM_DEV_DEBUG_DRIVER(dev, "Audio enable");
> >> + it6505_enable_audio(it6505);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int it6505_bridge_hdmi_audio_prepare(struct drm_connector *connector,
> >> + struct drm_bridge *bridge,
> >> + struct hdmi_codec_daifmt *fmt,
> >> + struct hdmi_codec_params *hparms) {
> >> + struct it6505 *it6505 = bridge_to_it6505(bridge);
> >> +
> >> + return it6505_audio_setup_hw_params(it6505, hparms); }
> >> +
> >> +static void it6505_bridge_hdmi_audio_shutdown(struct drm_connector *connector,
> >> + struct drm_bridge *bridge) {
> >> + struct it6505 *it6505 = bridge_to_it6505(bridge);
> >> +
> >> + if (it6505->powered && !it6505->enable_drv_hold)
> >> + it6505_disable_audio(it6505);
> >> +}
> >> +
> >> +static int it6505_bridge_hdmi_clear_infoframe(struct drm_bridge *bridge,
> >> + enum hdmi_infoframe_type type) {
> >> + struct it6505 *it6505 = bridge_to_it6505(bridge);
> >> + struct device *dev = it6505->dev;
> >> +
> >> + switch (type) {
> >> + case HDMI_INFOFRAME_TYPE_AUDIO:
> >> + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AUD_PKT, 0);
> >> + break;
> >> +
> >> + case HDMI_INFOFRAME_TYPE_AVI:
> >> + it6505_set_bits(it6505, REG_INFOFRAME_CTRL, EN_AVI_PKT, 0);
> >> + break;
> >
> >Are SPD / Vendor InfoFrames not supported by the HW?
> >
> >> + default:
> >> + dev_dbg(dev, "unsupported HDMI infoframe 0x%x\n", type);
> >> + break;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int it6505_bridge_hdmi_write_infoframe(struct drm_bridge *bridge,
> >> + enum hdmi_infoframe_type type,
> >> + const u8 *buffer, size_t len) {
> >> + struct it6505 *it6505 = bridge_to_it6505(bridge);
> >> + struct device *dev = it6505->dev;
> >> +
> >> + switch (type) {
> >> + case HDMI_INFOFRAME_TYPE_AUDIO:
> >> + it6505_write_audio_infoframe(it6505, buffer, len);
> >> + break;
> >> +
> >> + case HDMI_INFOFRAME_TYPE_AVI:
> >> + it6505_write_avi_infoframe(it6505, buffer, len);
> >> + break;
> >> + default:
> >> + dev_dbg(dev, "unsupported HDMI infoframe 0x%x\n", type);
> >> + break;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >
> >Please group functions logically, by having all InfoFrame functions next to each other.
> >
> >> +
> >> static const struct drm_bridge_funcs it6505_bridge_funcs = {
> >> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >> @@ -3315,6 +3417,12 @@ static const struct drm_bridge_funcs it6505_bridge_funcs = {
> >> .atomic_post_disable = it6505_bridge_atomic_post_disable,
> >> .detect = it6505_bridge_detect,
> >> .edid_read = it6505_bridge_edid_read,
> >> + .hdmi_audio_startup = it6505_bridge_hdmi_audio_startup,
> >> + .hdmi_audio_prepare = it6505_bridge_hdmi_audio_prepare,
> >> + .hdmi_audio_shutdown = it6505_bridge_hdmi_audio_shutdown,
> >> + .hdmi_clear_infoframe = it6505_bridge_hdmi_clear_infoframe,
> >> + .hdmi_write_infoframe = it6505_bridge_hdmi_write_infoframe,
> >
> >No .hdmi_tmds_char_rate_valid?
> >
> >> +
> >> };
> >>
> >> static __maybe_unused int it6505_bridge_resume(struct device *dev) @@
> >> -3700,7 +3808,12 @@ static int it6505_i2c_probe(struct i2c_client *client)
> >> it6505->bridge.funcs = &it6505_bridge_funcs;
> >> it6505->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> >> it6505->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> >> - DRM_BRIDGE_OP_HPD;
> >> + DRM_BRIDGE_OP_HPD | DRM_BRIDGE_OP_HDMI;
> >> + it6505->bridge.vendor = "iTE";
> >> + it6505->bridge.product = "IT6505";
> >> + it6505->bridge.hdmi_audio_dev = dev;
> >> + it6505->bridge.hdmi_audio_max_i2s_playback_channels = 2;
> >> + it6505->bridge.hdmi_audio_dai_port = 1;
> >> drm_bridge_add(&it6505->bridge);
> >>
> >> return 0;
> >>
> >> ---
> >> base-commit: fe003bcb69f7bff9ff2b30b659b004dbafe52907
> >> change-id: 20250114-add-audio-codec-8c9d47062a6c
> >>
> >> Best regards,
> >> --
> >> Hermes Wu <Hermes.wu@xxxxxxxxxx>
> >>
> >>
> >
> >--
> >With best wishes
> >Dmitry
> >
>
> BR,
> Hermes