Hi > >-----Original Message----- >From: Dmitry Baryshkov <dmitry.barysh...@linaro.org> >Sent: Tuesday, February 4, 2025 1:28 AM >To: Hermes Wu (吳佳宏) <hermes...@ite.com.tw> >Cc: Andrzej Hajda <andrzej.ha...@intel.com>; Neil Armstrong ><neil.armstr...@linaro.org>; Robert Foss <rf...@kernel.org>; Laurent Pinchart ><laurent.pinch...@ideasonboard.com>; Jonas Karlman <jo...@kwiboo.se>; Jernej >Skrabec <jernej.skra...@gmail.com>; Maarten Lankhorst ><maarten.lankho...@linux.intel.com>; Maxime Ripard <mrip...@kernel.org>; >Thomas Zimmermann <tzimmerm...@suse.de>; David Airlie <airl...@gmail.com>; >Simona Vetter <sim...@ffwll.ch>; treapk...@chromium.org; >dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org; Pet Weng (翁玉芬) ><pet.w...@ite.com.tw>; Kenneth Hung (洪家倫) <kenneth.h...@ite.com.tw> >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...@ite.com.tw> >> >> 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? >> >> Signed-off-by: Hermes Wu <hermes...@ite.com.tw> >> --- >> 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...@ite.com.tw> >> >> > >-- >With best wishes >Dmitry > BR, Hermes
