Re: [PATCH v3 09/14] drm/msm/dp: use msm_dp_utils_pack_sdp_header() for audio packets

[Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>]

On 12/11/2024 3:41 PM, Dmitry Baryshkov wrote:
> Use msm_dp_utils_pack_sdp_header() and call msm_dp_write_link() directly
> to program audio packet data. Use 0 as Packet ID, as it was not
> programmed earlier.
>
> Reviewed-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/msm/dp/dp_audio.c | 288 +++++++++-----------------------------
>   1 file changed, 66 insertions(+), 222 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_audio.c b/drivers/gpu/drm/msm/dp/dp_audio.c
> index 5cbb11986460d1e4ed1890bdf66d0913e013083c..1aa52d5cc08684a49102e45ed6e40ac2b13497c7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_audio.c
> +++ b/drivers/gpu/drm/msm/dp/dp_audio.c
> @@ -14,6 +14,7 @@
>   #include "dp_catalog.h"
>   #include "dp_audio.h"
>   #include "dp_panel.h"
> +#include "dp_reg.h"
>   #include "dp_display.h"
>   #include "dp_utils.h"
>   
> @@ -28,251 +29,94 @@ struct msm_dp_audio_private {
>   	struct msm_dp_audio msm_dp_audio;
>   };
>   
> -static u32 msm_dp_audio_get_header(struct msm_dp_catalog *catalog,
> -		enum msm_dp_catalog_audio_sdp_type sdp,
> -		enum msm_dp_catalog_audio_header_type header)
> -{
> -	return msm_dp_catalog_audio_get_header(catalog, sdp, header);
> -}
> -
> -static void msm_dp_audio_set_header(struct msm_dp_catalog *catalog,
> -		u32 data,
> -		enum msm_dp_catalog_audio_sdp_type sdp,
> -		enum msm_dp_catalog_audio_header_type header)
> -{
> -	msm_dp_catalog_audio_set_header(catalog, sdp, header, data);
> -}
> -
>   static void msm_dp_audio_stream_sdp(struct msm_dp_audio_private *audio)
>   {
>   	struct msm_dp_catalog *catalog = audio->catalog;
> -	u32 value, new_value;
> -	u8 parity_byte;
> -
> -	/* Config header and parity byte 1 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_STREAM, DP_AUDIO_SDP_HEADER_1);
> -
> -	new_value = 0x02;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_1_BIT)
> -			| (parity_byte << PARITY_BYTE_1_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 1: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_STREAM, DP_AUDIO_SDP_HEADER_1);
> -
> -	/* Config header and parity byte 2 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_STREAM, DP_AUDIO_SDP_HEADER_2);
> -	new_value = value;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_2_BIT)
> -			| (parity_byte << PARITY_BYTE_2_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 2: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_STREAM, DP_AUDIO_SDP_HEADER_2);
> -
> -	/* Config header and parity byte 3 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_STREAM, DP_AUDIO_SDP_HEADER_3);
> -
> -	new_value = audio->channels - 1;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_3_BIT)
> -			| (parity_byte << PARITY_BYTE_3_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 3: value = 0x%x, parity_byte = 0x%x\n",
> -		value, parity_byte);
> -
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_STREAM, DP_AUDIO_SDP_HEADER_3);
> +	struct dp_sdp_header sdp_hdr = {
> +		.HB0 = 0x00,
> +		.HB1 = 0x02,
> +		.HB2 = 0x00,
> +		.HB3 = audio->channels - 1,
> +	};
> +	u32 header[2];
> +
> +	msm_dp_utils_pack_sdp_header(&sdp_hdr, header);
> +
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_STREAM_0, header[0]);
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_STREAM_1, header[1]);
>   }

This patch is changing the programming behavior.

Earlier it was using a read/modify/write on each register. Now, its just 
 a write. I checked a few chipsets, the reset value of registers was 0, 
so that part is okay.

But, for the MMSS_DP_AUDIO_STREAM_0 register, earlier we were writing 
only the upper nibble, that is bits 15:0 of DP_AUDIO_SDP_HEADER_0 was 
kept as-it-is, but now this patch is changing that to 0. What was the 
reason for that change?

This is true for all the APIs being touched in this file.

I guess the whole point of having that audio map in the catalog was to 
preserve the read values of these registers. I have to check what was 
the reason behind that as once again this was before I worked on this 
driver as well.

So technically there are two parts to this change:

1) dropping read for each header and directly just writing it
2) Writing the registers directly instead of going through catalog

It seems like (1) and (2) are independent. I hope (1) was not the reason 
to have started this whole rework.

>   
>   static void msm_dp_audio_timestamp_sdp(struct msm_dp_audio_private *audio)
>   {
>   	struct msm_dp_catalog *catalog = audio->catalog;
> -	u32 value, new_value;
> -	u8 parity_byte;
> -
> -	/* Config header and parity byte 1 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_TIMESTAMP, DP_AUDIO_SDP_HEADER_1);
> -
> -	new_value = 0x1;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_1_BIT)
> -			| (parity_byte << PARITY_BYTE_1_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 1: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_TIMESTAMP, DP_AUDIO_SDP_HEADER_1);
> -
> -	/* Config header and parity byte 2 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_TIMESTAMP, DP_AUDIO_SDP_HEADER_2);
> -
> -	new_value = 0x17;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_2_BIT)
> -			| (parity_byte << PARITY_BYTE_2_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 2: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_TIMESTAMP, DP_AUDIO_SDP_HEADER_2);
> -
> -	/* Config header and parity byte 3 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_TIMESTAMP, DP_AUDIO_SDP_HEADER_3);
> -
> -	new_value = (0x0 | (0x11 << 2));
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_3_BIT)
> -			| (parity_byte << PARITY_BYTE_3_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 3: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_TIMESTAMP, DP_AUDIO_SDP_HEADER_3);
> +	struct dp_sdp_header sdp_hdr = {
> +		.HB0 = 0x00,
> +		.HB1 = 0x01,
> +		.HB2 = 0x17,
> +		.HB3 = 0x0 | (0x11 << 2),
> +	};
> +	u32 header[2];
> +
> +	msm_dp_utils_pack_sdp_header(&sdp_hdr, header);
> +
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_TIMESTAMP_0, header[0]);
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_TIMESTAMP_1, header[1]);
>   }
>   
>   static void msm_dp_audio_infoframe_sdp(struct msm_dp_audio_private *audio)
>   {
>   	struct msm_dp_catalog *catalog = audio->catalog;
> -	u32 value, new_value;
> -	u8 parity_byte;
> -
> -	/* Config header and parity byte 1 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_INFOFRAME, DP_AUDIO_SDP_HEADER_1);
> -
> -	new_value = 0x84;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_1_BIT)
> -			| (parity_byte << PARITY_BYTE_1_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 1: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_INFOFRAME, DP_AUDIO_SDP_HEADER_1);
> -
> -	/* Config header and parity byte 2 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_INFOFRAME, DP_AUDIO_SDP_HEADER_2);
> -
> -	new_value = 0x1b;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_2_BIT)
> -			| (parity_byte << PARITY_BYTE_2_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 2: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_INFOFRAME, DP_AUDIO_SDP_HEADER_2);
> -
> -	/* Config header and parity byte 3 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_INFOFRAME, DP_AUDIO_SDP_HEADER_3);
> -
> -	new_value = (0x0 | (0x11 << 2));
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_3_BIT)
> -			| (parity_byte << PARITY_BYTE_3_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 3: value = 0x%x, parity_byte = 0x%x\n",
> -			new_value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_INFOFRAME, DP_AUDIO_SDP_HEADER_3);
> +	struct dp_sdp_header sdp_hdr = {
> +		.HB0 = 0x00,
> +		.HB1 = 0x84,
> +		.HB2 = 0x1b,
> +		.HB3 = 0x0 | (0x11 << 2),
> +	};
> +	u32 header[2];
> +
> +	msm_dp_utils_pack_sdp_header(&sdp_hdr, header);
> +
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_INFOFRAME_0, header[0]);
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_INFOFRAME_1, header[1]);
>   }
>   
>   static void msm_dp_audio_copy_management_sdp(struct msm_dp_audio_private *audio)
>   {
>   	struct msm_dp_catalog *catalog = audio->catalog;
> -	u32 value, new_value;
> -	u8 parity_byte;
> -
> -	/* Config header and parity byte 1 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_COPYMANAGEMENT, DP_AUDIO_SDP_HEADER_1);
> -
> -	new_value = 0x05;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_1_BIT)
> -			| (parity_byte << PARITY_BYTE_1_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 1: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_COPYMANAGEMENT, DP_AUDIO_SDP_HEADER_1);
> -
> -	/* Config header and parity byte 2 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_COPYMANAGEMENT, DP_AUDIO_SDP_HEADER_2);
> -
> -	new_value = 0x0F;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_2_BIT)
> -			| (parity_byte << PARITY_BYTE_2_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 2: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_COPYMANAGEMENT, DP_AUDIO_SDP_HEADER_2);
> -
> -	/* Config header and parity byte 3 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_COPYMANAGEMENT, DP_AUDIO_SDP_HEADER_3);
> -
> -	new_value = 0x0;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_3_BIT)
> -			| (parity_byte << PARITY_BYTE_3_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 3: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_COPYMANAGEMENT, DP_AUDIO_SDP_HEADER_3);
> +	struct dp_sdp_header sdp_hdr = {
> +		.HB0 = 0x00,
> +		.HB1 = 0x05,
> +		.HB2 = 0x0f,
> +		.HB3 = 0x00,
> +	};
> +	u32 header[2];
> +
> +	msm_dp_utils_pack_sdp_header(&sdp_hdr, header);
> +
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_COPYMANAGEMENT_0, header[0]);
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_COPYMANAGEMENT_1, header[1]);
>   }
>   
>   static void msm_dp_audio_isrc_sdp(struct msm_dp_audio_private *audio)
>   {
>   	struct msm_dp_catalog *catalog = audio->catalog;
> -	u32 value, new_value;
> -	u8 parity_byte;
> -
> -	/* Config header and parity byte 1 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_ISRC, DP_AUDIO_SDP_HEADER_1);
> -
> -	new_value = 0x06;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_1_BIT)
> -			| (parity_byte << PARITY_BYTE_1_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 1: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_ISRC, DP_AUDIO_SDP_HEADER_1);
> -
> -	/* Config header and parity byte 2 */
> -	value = msm_dp_audio_get_header(catalog,
> -			DP_AUDIO_SDP_ISRC, DP_AUDIO_SDP_HEADER_2);
> -
> -	new_value = 0x0F;
> -	parity_byte = msm_dp_utils_calculate_parity(new_value);
> -	value |= ((new_value << HEADER_BYTE_2_BIT)
> -			| (parity_byte << PARITY_BYTE_2_BIT));
> -	drm_dbg_dp(audio->drm_dev,
> -			"Header Byte 2: value = 0x%x, parity_byte = 0x%x\n",
> -			value, parity_byte);
> -	msm_dp_audio_set_header(catalog, value,
> -		DP_AUDIO_SDP_ISRC, DP_AUDIO_SDP_HEADER_2);
> +	struct dp_sdp_header sdp_hdr = {
> +		.HB0 = 0x00,
> +		.HB1 = 0x06,
> +		.HB2 = 0x0f,
> +		.HB3 = 0x00,
> +	};
> +	u32 header[2];
> +	u32 reg;
> +
> +	/* XXX: is it necessary to preserve this field? */
> +	reg = msm_dp_read_link(catalog, MMSS_DP_AUDIO_ISRC_1);
> +	sdp_hdr.HB3 = FIELD_GET(HEADER_3_MASK, reg);
> +
> +	msm_dp_utils_pack_sdp_header(&sdp_hdr, header);
> +
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_ISRC_0, header[0]);
> +	msm_dp_write_link(catalog, MMSS_DP_AUDIO_ISRC_1, header[1]);
>   }
>   
>   static void msm_dp_audio_setup_sdp(struct msm_dp_audio_private *audio)