From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 81EE5C433EF for ; Thu, 14 Jul 2022 11:26:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238831AbiGNL04 (ORCPT ); Thu, 14 Jul 2022 07:26:56 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35586 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238785AbiGNL0c (ORCPT ); Thu, 14 Jul 2022 07:26:32 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3BEB95726C; Thu, 14 Jul 2022 04:26:27 -0700 (PDT) Received: from [192.168.1.100] (2-237-20-237.ip236.fastwebnet.it [2.237.20.237]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits)) (No client certificate requested) (Authenticated sender: kholk11) by madras.collabora.co.uk (Postfix) with ESMTPSA id 656336601A3B; Thu, 14 Jul 2022 12:26:24 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1657797985; bh=S3uu1fiJqnW9GfhBdNOm5ox+Kfz/PBBejcQCNxtJG7Q=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=nmnLgvsJ7wUdm7PX1mVlONbq1I4u5c8cliefOUgywi8O7eQNZ9V+UIqkndktdgbcP I0dEshLOWiiUfvkV/+4U8lS3BJfHmZgWj3KVrMa/ccUIYo9Dnpb4HTLgtic2FTgNh7 g5d+02tP+6EPQhfdPAcWyW0jpcfH2kHqBjomZ+ZuOvzEhVWZ2vJ9XBqB3laT4PSV/x wE27WhtoF+NNzOHG02pv3MTUxMxOvBeOAV4Al3aYiSSPgVfkd9x19nYDLM9Qlt0zBW OPk0gbvq517iOXDN2+v2c/sA8s86kGLxsltZPJRKhmMDcmNkBKg0OPz6pPN2wKSIEw tSgJZPher7yKQ== Message-ID: <8bc57373-70e4-8ab6-659f-0917dbf14c38@collabora.com> Date: Thu, 14 Jul 2022 13:26:21 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH v14 04/10] video/hdmi: Add audio_infoframe packing for DP Content-Language: en-US To: Bo-Chen Chen , chunkuang.hu@kernel.org, p.zabel@pengutronix.de, daniel@ffwll.ch, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, mripard@kernel.org, tzimmermann@suse.de, matthias.bgg@gmail.com, deller@gmx.de, airlied@linux.ie Cc: msp@baylibre.com, granquet@baylibre.com, jitao.shi@mediatek.com, wenst@chromium.org, ck.hu@mediatek.com, liangxu.xu@mediatek.com, dri-devel@lists.freedesktop.org, linux-mediatek@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org, Project_Global_Chrome_Upstream_Group@mediatek.com References: <20220712111223.13080-1-rex-bc.chen@mediatek.com> <20220712111223.13080-5-rex-bc.chen@mediatek.com> From: AngeloGioacchino Del Regno In-Reply-To: <20220712111223.13080-5-rex-bc.chen@mediatek.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Il 12/07/22 13:12, Bo-Chen Chen ha scritto: > From: Markus Schneider-Pargmann > > 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 > Signed-off-by: Guillaume Ranquet > Signed-off-by: Bo-Chen Chen > --- > 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 > #include > #include > #include > @@ -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,