Re: [PATCH v14 04/10] video/hdmi: Add audio_infoframe packing for DP

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 12/07/22 13:12, Bo-Chen Chen ha scritto:
> From: Markus Schneider-Pargmann <msp@baylibre.com>
> 
> Similar to HDMI, DP uses audio infoframes as well which are structured
> very similar to the HDMI ones.
> 
> This patch adds a helper function to pack the HDMI audio infoframe for
> DP, called hdmi_audio_infoframe_pack_for_dp().
> hdmi_audio_infoframe_pack_only() is split into two parts. One of them
> packs the payload only and can be used for HDMI and DP.
> 
> Also constify the frame parameter in hdmi_audio_infoframe_check() as
> it is passed to hdmi_audio_infoframe_check_only() which expects a const.
> 
> Signed-off-by: Markus Schneider-Pargmann <msp@baylibre.com>
> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/video/hdmi.c         | 82 +++++++++++++++++++++++++++---------
>   include/drm/display/drm_dp.h |  2 +
>   include/linux/hdmi.h         |  7 ++-
>   3 files changed, 71 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 947be761dfa4..86805d77cc86 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -21,6 +21,7 @@
>    * DEALINGS IN THE SOFTWARE.
>    */
>   
> +#include <drm/display/drm_dp.h>
>   #include <linux/bitops.h>
>   #include <linux/bug.h>
>   #include <linux/errno.h>
> @@ -381,12 +382,34 @@ static int hdmi_audio_infoframe_check_only(const struct hdmi_audio_infoframe *fr
>    *
>    * Returns 0 on success or a negative error code on failure.
>    */
> -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame)
> +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame)
>   {
>   	return hdmi_audio_infoframe_check_only(frame);
>   }
>   EXPORT_SYMBOL(hdmi_audio_infoframe_check);
>   
> +static void
> +hdmi_audio_infoframe_pack_payload(const struct hdmi_audio_infoframe *frame,
> +				  u8 *buffer)
> +{
> +	u8 channels;
> +
> +	if (frame->channels >= 2)
> +		channels = frame->channels - 1;
> +	else
> +		channels = 0;
> +
> +	buffer[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7);
> +	buffer[1] = ((frame->sample_frequency & 0x7) << 2) |
> +		 (frame->sample_size & 0x3);
> +	buffer[2] = frame->coding_type_ext & 0x1f;
> +	buffer[3] = frame->channel_allocation;
> +	buffer[4] = (frame->level_shift_value & 0xf) << 3;
> +
> +	if (frame->downmix_inhibit)
> +		buffer[4] |= BIT(7);
> +}
> +
>   /**
>    * hdmi_audio_infoframe_pack_only() - write HDMI audio infoframe to binary buffer
>    * @frame: HDMI audio infoframe
> @@ -404,7 +427,6 @@ EXPORT_SYMBOL(hdmi_audio_infoframe_check);
>   ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame,
>   				       void *buffer, size_t size)
>   {
> -	unsigned char channels;
>   	u8 *ptr = buffer;
>   	size_t length;
>   	int ret;
> @@ -420,28 +442,13 @@ ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame,
>   
>   	memset(buffer, 0, size);
>   
> -	if (frame->channels >= 2)
> -		channels = frame->channels - 1;
> -	else
> -		channels = 0;
> -
>   	ptr[0] = frame->type;
>   	ptr[1] = frame->version;
>   	ptr[2] = frame->length;
>   	ptr[3] = 0; /* checksum */
>   
> -	/* start infoframe payload */
> -	ptr += HDMI_INFOFRAME_HEADER_SIZE;
> -
> -	ptr[0] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7);
> -	ptr[1] = ((frame->sample_frequency & 0x7) << 2) |
> -		 (frame->sample_size & 0x3);
> -	ptr[2] = frame->coding_type_ext & 0x1f;
> -	ptr[3] = frame->channel_allocation;
> -	ptr[4] = (frame->level_shift_value & 0xf) << 3;
> -
> -	if (frame->downmix_inhibit)
> -		ptr[4] |= BIT(7);
> +	hdmi_audio_infoframe_pack_payload(frame,
> +					  ptr + HDMI_INFOFRAME_HEADER_SIZE);
>   
>   	hdmi_infoframe_set_checksum(buffer, length);
>   
> @@ -479,6 +486,43 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
>   }
>   EXPORT_SYMBOL(hdmi_audio_infoframe_pack);
>   
> +/**
> + * hdmi_audio_infoframe_pack_for_dp - Pack a HDMI Audio infoframe for DisplayPort
> + *
> + * @frame:      HDMI Audio infoframe
> + * @sdp:        secondary data packet for display port. This is filled with the
> + * appropriate: data

"This is filled with the appropriate data"

... well, that's pretty obvious, isn't it?
You're describing that this function is filling sdp in the description, so you
can just remove that part.

Also, "Secondary data packet for DisplayPort", please.


> + * @dp_version: Display Port version to be encoded in the header

We're not meaning "a display port", but really "DisplayPort": please remove
the space between "Display" and "Port" :-)

(here and in the description below)

> + *
> + * Packs a HDMI Audio Infoframe to be sent over Display Port. This function
> + * fills the secondary data packet to be used for Display Port.
> + *
> + * Return: Number of total written bytes or a negative errno on failure.
> + */
> +ssize_t
> +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame,
> +				 struct dp_sdp *sdp, u8 dp_version)
> +{
> +	int ret;
> +
> +	ret = hdmi_audio_infoframe_check(frame);
> +	if (ret)
> +		return ret;
> +
> +	memset(sdp->db, 0, sizeof(sdp->db));
> +
> +	/* Secondary-data packet header */
> +	sdp->sdp_header.HB0 = 0;
> +	sdp->sdp_header.HB1 = frame->type;
> +	sdp->sdp_header.HB2 = DP_SDP_AUDIO_INFOFRAME_HB2;
> +	sdp->sdp_header.HB3 = (dp_version & 0x3f) << 2;
> +
> +	hdmi_audio_infoframe_pack_payload(frame, sdp->db);
> +
> +	return frame->length + 4;

What's this magic number 4 about?

Please use a definition for that.

> +}
> +EXPORT_SYMBOL(hdmi_audio_infoframe_pack_for_dp);
> +
>   /**
>    * hdmi_vendor_infoframe_init() - initialize an HDMI vendor infoframe
>    * @frame: HDMI vendor infoframe
> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> index 9e3aff7e68bb..6c0871164771 100644
> --- a/include/drm/display/drm_dp.h
> +++ b/include/drm/display/drm_dp.h
> @@ -1536,6 +1536,8 @@ enum drm_dp_phy {
>   #define DP_SDP_VSC_EXT_CEA		0x21 /* DP 1.4 */
>   /* 0x80+ CEA-861 infoframe types */
>   
> +#define DP_SDP_AUDIO_INFOFRAME_HB2	0x1b
> +
>   /**
>    * struct dp_sdp_header - DP secondary data packet header
>    * @HB0: Secondary Data Packet ID
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index c8ec982ff498..2f4dcc8d060e 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -336,7 +336,12 @@ ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
>   				  void *buffer, size_t size);
>   ssize_t hdmi_audio_infoframe_pack_only(const struct hdmi_audio_infoframe *frame,
>   				       void *buffer, size_t size);
> -int hdmi_audio_infoframe_check(struct hdmi_audio_infoframe *frame);
> +int hdmi_audio_infoframe_check(const struct hdmi_audio_infoframe *frame);
> +
> +struct dp_sdp;
> +ssize_t
> +hdmi_audio_infoframe_pack_for_dp(const struct hdmi_audio_infoframe *frame,
> +				 struct dp_sdp *sdp, u8 dp_version);
>   
>   enum hdmi_3d_structure {
>   	HDMI_3D_STRUCTURE_INVALID = -1,