Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752949AbbKPQ21 (ORCPT ); Mon, 16 Nov 2015 11:28:27 -0500 Received: from mail-io0-f175.google.com ([209.85.223.175]:35822 "EHLO mail-io0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752280AbbKPQ2Z (ORCPT ); Mon, 16 Nov 2015 11:28:25 -0500 MIME-Version: 1.0 In-Reply-To: <20151116115037.GF31033@ulmo.nvidia.com> References: <1447526299-4222-1-git-send-email-enric.balletbo@collabora.com> <20151116115037.GF31033@ulmo.nvidia.com> Date: Mon, 16 Nov 2015 17:28:24 +0100 Message-ID: Subject: Re: [PATCH] [media] hdmi: added functions for MPEG InfoFrames From: Enric Balletbo Serra To: Thierry Reding Cc: linux-fbdev@vger.kernel.org, Jean-Christophe Plagniol-Villard , Tomi Valkeinen , Hans Verkuil , Mauro Carvalho Chehab , Martin Bugge , linux-kernel@vger.kernel.org, dri-devel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3811 Lines: 118 Hello Thierry, Many thanks for your comments. 2015-11-16 12:50 GMT+01:00 Thierry Reding : > On Sat, Nov 14, 2015 at 07:38:19PM +0100, Enric Balletbo i Serra wrote: >> The MPEG Source (MS) InfoFrame is in EIA/CEA-861B. It describes aspects of >> the compressed video stream that were used to produce the uncompressed >> video. >> >> The patch adds functions to work with MPEG InfoFrames. >> >> Signed-off-by: Enric Balletbo i Serra >> --- >> drivers/video/hdmi.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/hdmi.h | 24 ++++++++ >> 2 files changed, 180 insertions(+) > > According to the CEA specification a source is expected to send this > type of infoframe once per video frame. I'm curious how you envision > this to be ensured. Would hardware provide a mechanism to store this > data and send the infoframe automatically? How would you ensure that > updates sent to the hardware match the upcoming frame? > To be honest I'm not sure if I have the full picture. In the use case I'm trying there is a hardware mechanism to store the data and send the infoframe through a "Packet Send Control Register". >> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > [...] >> @@ -435,6 +510,8 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size) >> length = hdmi_vendor_any_infoframe_pack(&frame->vendor, >> buffer, size); >> break; >> + case HDMI_INFOFRAME_TYPE_MPEG: >> + length = hdmi_mpeg_infoframe_pack(&frame->mpeg, buffer, size); > > Missing "break;"? > Ack. >> @@ -899,6 +978,41 @@ static void hdmi_audio_infoframe_log(const char *level, >> frame->downmix_inhibit ? "Yes" : "No"); >> } >> >> +static const char *hdmi_mpeg_picture_get_name(enum hdmi_mpeg_picture_type type) >> +{ >> + switch (type) { >> + case HDMI_MPEG_PICTURE_TYPE_UNKNOWN: >> + return "Unknown"; >> + case HDMI_MPEG_PICTURE_TYPE_I: >> + return "Intra-coded picture"; >> + case HDMI_MPEG_PICTURE_TYPE_B: >> + return "Bi-predictive picture"; >> + case HDMI_MPEG_PICTURE_TYPE_P: >> + return "Predicted picture"; >> + } > > I'd have chosen the slightly more canonical "I-Frame", "P-Frame", > "B-Frame" here, but that's not a strong objection. > I don't have any inconvenient to change, are the following names more canonical ? HDMI_MPEG_UNKNOWN_FRAME = 0x00, HDMI_MPEG_I_FRAME = 0x01, HDMI_MPEG_B_FRAME = 0x02, HDMI_MPEG_P_FRAME = 0x03, >> + return "Reserved"; > > There really can't be another value here, so I think this should either > return NULL or even go as far as let it crash. There should be no way > for the invalid value to make it this far. > Ok. >> +static int hdmi_mpeg_infoframe_unpack(struct hdmi_mpeg_infoframe *frame, >> + void *buffer) >> +{ >> + u8 *ptr = buffer; >> + >> + if (ptr[0] != HDMI_INFOFRAME_TYPE_MPEG || >> + ptr[1] != 1 || >> + ptr[2] != HDMI_MPEG_INFOFRAME_SIZE) { >> + return -EINVAL; >> + } > > There's no need for the braces here. > >> @@ -314,6 +336,7 @@ union hdmi_vendor_any_infoframe { >> * @spd: spd infoframe >> * @vendor: union of all vendor infoframes >> * @audio: audio infoframe >> + * @mpeg: mpeg infoframe > > s/mpeg/MPEG/ > Ok. > Thierry I will send v2 after receiving your feedback again. Best regards, Enric -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/