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
