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 <[email protected]>
---
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];
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(¶ms->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 */
--
2.6.3
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 <[email protected]>
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(¶ms->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
>
--
Sean Paul, Software Engineer, Google / Chromium OS
Hi,
On Mon, Jul 17, 2017 at 1:23 PM, Sean Paul <[email protected]> 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 };
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 <[email protected]>
> 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(¶ms->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
>>
--
Chris Zhong