linux-kernel - Re: [PATCH] [media] hdmi: added functions for MPEG InfoFrames

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFqH_514hjjgPHbP=u1nQ2FPoQe2zLuK5L2R6JcLXjOQdpqZ9g@mail.gmail.com>
Date:	Mon, 16 Nov 2015 17:28:24 +0100
From:	Enric Balletbo Serra <eballetbo@...il.com>
To:	Thierry Reding <treding@...dia.com>
Cc:	linux-fbdev@...r.kernel.org,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Tomi Valkeinen <tomi.valkeinen@...com>,
	Hans Verkuil <hans.verkuil@...co.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Martin Bugge <marbugge@...co.com>,
	linux-kernel@...r.kernel.org,
	dri-devel <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH] [media] hdmi: added functions for MPEG InfoFrames

Hello Thierry,

Many thanks for your comments.

2015-11-16 12:50 GMT+01:00 Thierry Reding <treding@...dia.com>:
> 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 <enric.balletbo@...labora.com>
>> ---
>>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/