diff mbox

drm/rockchip: cdn-dp: send audio infoframe to sink

Message ID 1500116418-4178-1-git-send-email-zyw@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Zhong July 15, 2017, 11 a.m. UTC 
Some DP/HDMI sink need to receive the audio infoframe to play sound,
especially some multi-channel AV receiver, they need the
channel_allocation from infoframe to config the speakers. Send the
audio infoframe via SDP will make them work properly.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>
---

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/rockchip/cdn-dp-reg.c  | 19 +++++++++++++++++++
 drivers/gpu/drm/rockchip/cdn-dp-reg.h  |  6 ++++++
 3 files changed, 45 insertions(+)

Comments

Sean Paul July 17, 2017, 8:23 p.m. UTC | #1 
On Sat, Jul 15, 2017 at 07:00:18PM +0800, Chris Zhong wrote:
> Some DP/HDMI sink need to receive the audio infoframe to play sound,
> especially some multi-channel AV receiver, they need the
> channel_allocation from infoframe to config the speakers. Send the
> audio infoframe via SDP will make them work properly.
> 
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>

Hi Chris,
Thanks for the patch, please see comments below.

> ---
> 
>  drivers/gpu/drm/rockchip/cdn-dp-core.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/rockchip/cdn-dp-reg.c  | 19 +++++++++++++++++++
>  drivers/gpu/drm/rockchip/cdn-dp-reg.h  |  6 ++++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> index 9b0b058..e59ca47 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
> @@ -802,6 +802,7 @@ static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
>  		.sample_rate = params->sample_rate,
>  		.channels = params->channels,
>  	};
> +	u8 buffer[14];

Why 14?

I think you should probably have buffer[HDMI_AUDIO_INFOFRAME_SIZE +
SDP_HEADER_SIZE] so the reader knows how you arrived at this value.

>  	int ret;
>  
>  	mutex_lock(&dp->lock);
> @@ -823,6 +824,25 @@ static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
>  		goto out;
>  	}
>  
> +	memset(buffer, 0, sizeof(buffer));
> +
> +	ret = hdmi_audio_infoframe_pack(&params->cea, buffer, sizeof(buffer));
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(dev, "Failed to pack audio infoframe: %d\n", ret);
> +		goto out;
> +	}
> +
> +	/*
> +	 * Change the infoframe header to SDP header per DP 1.3 spec, Table
> +	 * 2-98.
> +	 */
> +	buffer[0] = 0;
> +	buffer[1] = HDMI_INFOFRAME_TYPE_AUDIO;
> +	buffer[2] = 0x1b;
> +	buffer[3] = 0x48;

Instead of doing this, consider splitting up hdmi_audio_infoframe_pack into
hdmi_audio_infoframe_pack and hdmi_audio_infoframe_pack_payload. The first
function does everything, while the second just packs the payload, then you can
set the SDP header independently.

> +
> +	cdn_dp_sdp_write(dp, 0, buffer, sizeof(buffer));
> +
>  	ret = cdn_dp_audio_config(dp, &audio);
>  	if (!ret)
>  		dp->audio_info = audio;
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> index b14d211..8907db0 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
> @@ -951,6 +951,25 @@ static void cdn_dp_audio_config_spdif(struct cdn_dp_device *dp)
>  	clk_set_rate(dp->spdif_clk, CDN_DP_SPDIF_CLK);
>  }
>  
> +void cdn_dp_sdp_write(struct cdn_dp_device *dp, int entry_id, u8 *buf, u32 len)
> +{
> +	int idx;
> +	u32 *packet = (u32 *)buf;
> +	u32 length = len / 4;

length and len are pretty terrible names, it would be quite easy to use the
wrong one. Consider more descriptive names (ie: buf_len and num_packets).

> +	u8 type = buf[1];

Check for len < 0?

> +
> +	for (idx = 0; idx < length; idx++)
> +		writel(cpu_to_le32(*packet++), dp->regs + SOURCE_PIF_DATA_WR);
> +
> +	writel(entry_id, dp->regs + SOURCE_PIF_WR_ADDR);
> +
> +	writel(F_HOST_WR, dp->regs + SOURCE_PIF_WR_REQ);
> +
> +	writel(PIF_PKT_TYPE_VALID | F_PACKET_TYPE(type) | entry_id,
> +	       dp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> +	writel(PIF_PKT_ALLOC_WR_EN, dp->regs + SOURCE_PIF_PKT_ALLOC_WR_EN);
> +}
> +
>  int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio)
>  {
>  	int ret;
> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> index c4bbb4a83..6ec0e81 100644
> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
> @@ -424,6 +424,11 @@
>  /* Reference cycles when using lane clock as reference */
>  #define LANE_REF_CYC				0x8000
>  
> +#define F_HOST_WR		BIT(0)
> +#define PIF_PKT_ALLOC_WR_EN	BIT(0)
> +#define PIF_PKT_TYPE_VALID	(3 << 16)
> +#define F_PACKET_TYPE(x)	(((x) & 0xff) << 8)

Can you tuck these #defines under the definition of SOURCE_PIF_PKT_ALLOC_REG, so
we know which register they apply to?

> +
>  enum voltage_swing_level {
>  	VOLTAGE_LEVEL_0,
>  	VOLTAGE_LEVEL_1,
> @@ -478,5 +483,6 @@ int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active);
>  int cdn_dp_config_video(struct cdn_dp_device *dp);
>  int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info *audio);
>  int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
> +void cdn_dp_sdp_write(struct cdn_dp_device *dp, int entry_id, u8 *buf, u32 len);
>  int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio);
>  #endif /* _CDN_DP_REG_H */
> -- 
> 2.6.3
>
Douglas Anderson July 17, 2017, 8:44 p.m. UTC | #2 
Hi,

On Mon, Jul 17, 2017 at 1:23 PM, Sean Paul <seanpaul@chromium.org> wrote:
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> index 9b0b058..e59ca47 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> @@ -802,6 +802,7 @@ static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
>>               .sample_rate = params->sample_rate,
>>               .channels = params->channels,
>>       };
>> +     u8 buffer[14];
>
> Why 14?
>
> I think you should probably have buffer[HDMI_AUDIO_INFOFRAME_SIZE +
> SDP_HEADER_SIZE] so the reader knows how you arrived at this value.
>
>>       int ret;
>>
>>       mutex_lock(&dp->lock);
>> @@ -823,6 +824,25 @@ static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
>>               goto out;
>>       }
>>
>> +     memset(buffer, 0, sizeof(buffer));

It is less error prone (and probably makes better code) to just to do
this at init time.  AKA:

u8 buffer[14] = { 0 };
Chris Zhong July 18, 2017, 11:36 a.m. UTC | #3 
Hi Sean

Thanks for your replying.


On Tuesday, July 18, 2017 04:23 AM, Sean Paul wrote:
> On Sat, Jul 15, 2017 at 07:00:18PM +0800, Chris Zhong wrote:
>> Some DP/HDMI sink need to receive the audio infoframe to play sound,
>> especially some multi-channel AV receiver, they need the
>> channel_allocation from infoframe to config the speakers. Send the
>> audio infoframe via SDP will make them work properly.
>>
>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> Hi Chris,
> Thanks for the patch, please see comments below.
>
>> ---
>>
>>   drivers/gpu/drm/rockchip/cdn-dp-core.c | 20 ++++++++++++++++++++
>>   drivers/gpu/drm/rockchip/cdn-dp-reg.c  | 19 +++++++++++++++++++
>>   drivers/gpu/drm/rockchip/cdn-dp-reg.h  |  6 ++++++
>>   3 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> index 9b0b058..e59ca47 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
>> @@ -802,6 +802,7 @@ static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
>>   		.sample_rate = params->sample_rate,
>>   		.channels = params->channels,
>>   	};
>> +	u8 buffer[14];
> Why 14?
>
> I think you should probably have buffer[HDMI_AUDIO_INFOFRAME_SIZE +
> SDP_HEADER_SIZE] so the reader knows how you arrived at this value.
>
>>   	int ret;
>>   
>>   	mutex_lock(&dp->lock);
>> @@ -823,6 +824,25 @@ static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
>>   		goto out;
>>   	}
>>   
>> +	memset(buffer, 0, sizeof(buffer));
>> +
>> +	ret = hdmi_audio_infoframe_pack(&params->cea, buffer, sizeof(buffer));
>> +	if (ret < 0) {
>> +		DRM_DEV_ERROR(dev, "Failed to pack audio infoframe: %d\n", ret);
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * Change the infoframe header to SDP header per DP 1.3 spec, Table
>> +	 * 2-98.
>> +	 */
>> +	buffer[0] = 0;
>> +	buffer[1] = HDMI_INFOFRAME_TYPE_AUDIO;
>> +	buffer[2] = 0x1b;
>> +	buffer[3] = 0x48;
> Instead of doing this, consider splitting up hdmi_audio_infoframe_pack into
> hdmi_audio_infoframe_pack and hdmi_audio_infoframe_pack_payload. The first
> function does everything, while the second just packs the payload, then you can
> set the SDP header independently.
>
>> +
>> +	cdn_dp_sdp_write(dp, 0, buffer, sizeof(buffer));
>> +
>>   	ret = cdn_dp_audio_config(dp, &audio);
>>   	if (!ret)
>>   		dp->audio_info = audio;
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>> index b14d211..8907db0 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
>> @@ -951,6 +951,25 @@ static void cdn_dp_audio_config_spdif(struct cdn_dp_device *dp)
>>   	clk_set_rate(dp->spdif_clk, CDN_DP_SPDIF_CLK);
>>   }
>>   
>> +void cdn_dp_sdp_write(struct cdn_dp_device *dp, int entry_id, u8 *buf, u32 len)
>> +{
>> +	int idx;
>> +	u32 *packet = (u32 *)buf;
>> +	u32 length = len / 4;
> length and len are pretty terrible names, it would be quite easy to use the
> wrong one. Consider more descriptive names (ie: buf_len and num_packets).
>
>> +	u8 type = buf[1];
> Check for len < 0?
>
>> +
>> +	for (idx = 0; idx < length; idx++)
>> +		writel(cpu_to_le32(*packet++), dp->regs + SOURCE_PIF_DATA_WR);
>> +
>> +	writel(entry_id, dp->regs + SOURCE_PIF_WR_ADDR);
>> +
>> +	writel(F_HOST_WR, dp->regs + SOURCE_PIF_WR_REQ);
>> +
>> +	writel(PIF_PKT_TYPE_VALID | F_PACKET_TYPE(type) | entry_id,
>> +	       dp->regs + SOURCE_PIF_PKT_ALLOC_REG);
>> +	writel(PIF_PKT_ALLOC_WR_EN, dp->regs + SOURCE_PIF_PKT_ALLOC_WR_EN);
>> +}
>> +
>>   int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio)
>>   {
>>   	int ret;
>> diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>> index c4bbb4a83..6ec0e81 100644
>> --- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>> +++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
>> @@ -424,6 +424,11 @@
>>   /* Reference cycles when using lane clock as reference */
>>   #define LANE_REF_CYC				0x8000
>>   
>> +#define F_HOST_WR		BIT(0)
>> +#define PIF_PKT_ALLOC_WR_EN	BIT(0)
>> +#define PIF_PKT_TYPE_VALID	(3 << 16)
>> +#define F_PACKET_TYPE(x)	(((x) & 0xff) << 8)
> Can you tuck these #defines under the definition of SOURCE_PIF_PKT_ALLOC_REG, so
> we know which register they apply to?
>
>
Considering the existing code style of this file, can we keep these 
defines here?

>> +
>>   enum voltage_swing_level {
>>   	VOLTAGE_LEVEL_0,
>>   	VOLTAGE_LEVEL_1,
>> @@ -478,5 +483,6 @@ int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active);
>>   int cdn_dp_config_video(struct cdn_dp_device *dp);
>>   int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info *audio);
>>   int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
>> +void cdn_dp_sdp_write(struct cdn_dp_device *dp, int entry_id, u8 *buf, u32 len);
>>   int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio);
>>   #endif /* _CDN_DP_REG_H */
>> -- 
>> 2.6.3
>>
diff mbox

Patch

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 9b0b058..e59ca47 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -802,6 +802,7 @@  static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
 		.sample_rate = params->sample_rate,
 		.channels = params->channels,
 	};
+	u8 buffer[14];
 	int ret;
 
 	mutex_lock(&dp->lock);
@@ -823,6 +824,25 @@  static int cdn_dp_audio_hw_params(struct device *dev,  void *data,
 		goto out;
 	}
 
+	memset(buffer, 0, sizeof(buffer));
+
+	ret = hdmi_audio_infoframe_pack(&params->cea, buffer, sizeof(buffer));
+	if (ret < 0) {
+		DRM_DEV_ERROR(dev, "Failed to pack audio infoframe: %d\n", ret);
+		goto out;
+	}
+
+	/*
+	 * Change the infoframe header to SDP header per DP 1.3 spec, Table
+	 * 2-98.
+	 */
+	buffer[0] = 0;
+	buffer[1] = HDMI_INFOFRAME_TYPE_AUDIO;
+	buffer[2] = 0x1b;
+	buffer[3] = 0x48;
+
+	cdn_dp_sdp_write(dp, 0, buffer, sizeof(buffer));
+
 	ret = cdn_dp_audio_config(dp, &audio);
 	if (!ret)
 		dp->audio_info = audio;
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.c b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
index b14d211..8907db0 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-reg.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.c
@@ -951,6 +951,25 @@  static void cdn_dp_audio_config_spdif(struct cdn_dp_device *dp)
 	clk_set_rate(dp->spdif_clk, CDN_DP_SPDIF_CLK);
 }
 
+void cdn_dp_sdp_write(struct cdn_dp_device *dp, int entry_id, u8 *buf, u32 len)
+{
+	int idx;
+	u32 *packet = (u32 *)buf;
+	u32 length = len / 4;
+	u8 type = buf[1];
+
+	for (idx = 0; idx < length; idx++)
+		writel(cpu_to_le32(*packet++), dp->regs + SOURCE_PIF_DATA_WR);
+
+	writel(entry_id, dp->regs + SOURCE_PIF_WR_ADDR);
+
+	writel(F_HOST_WR, dp->regs + SOURCE_PIF_WR_REQ);
+
+	writel(PIF_PKT_TYPE_VALID | F_PACKET_TYPE(type) | entry_id,
+	       dp->regs + SOURCE_PIF_PKT_ALLOC_REG);
+	writel(PIF_PKT_ALLOC_WR_EN, dp->regs + SOURCE_PIF_PKT_ALLOC_WR_EN);
+}
+
 int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio)
 {
 	int ret;
diff --git a/drivers/gpu/drm/rockchip/cdn-dp-reg.h b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
index c4bbb4a83..6ec0e81 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-reg.h
+++ b/drivers/gpu/drm/rockchip/cdn-dp-reg.h
@@ -424,6 +424,11 @@ 
 /* Reference cycles when using lane clock as reference */
 #define LANE_REF_CYC				0x8000
 
+#define F_HOST_WR		BIT(0)
+#define PIF_PKT_ALLOC_WR_EN	BIT(0)
+#define PIF_PKT_TYPE_VALID	(3 << 16)
+#define F_PACKET_TYPE(x)	(((x) & 0xff) << 8)
+
 enum voltage_swing_level {
 	VOLTAGE_LEVEL_0,
 	VOLTAGE_LEVEL_1,
@@ -478,5 +483,6 @@  int cdn_dp_set_video_status(struct cdn_dp_device *dp, int active);
 int cdn_dp_config_video(struct cdn_dp_device *dp);
 int cdn_dp_audio_stop(struct cdn_dp_device *dp, struct audio_info *audio);
 int cdn_dp_audio_mute(struct cdn_dp_device *dp, bool enable);
+void cdn_dp_sdp_write(struct cdn_dp_device *dp, int entry_id, u8 *buf, u32 len);
 int cdn_dp_audio_config(struct cdn_dp_device *dp, struct audio_info *audio);
 #endif /* _CDN_DP_REG_H */