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

[Hermes.Wu]

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? 

>> 
>> 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