[PATCH] [media] hdmi: added functions for MPEG InfoFrames

[Enric Balletbo Serra]

Hello Thierry,

2015-11-17 13:55 GMT+01:00 Thierry Reding <treding at nvidia.com>:
> On Mon, Nov 16, 2015 at 05:28:24PM +0100, Enric Balletbo Serra wrote:
>> Hello Thierry,
>>
>> Many thanks for your comments.
>>
>> 2015-11-16 12:50 GMT+01:00 Thierry Reding <treding at nvidia.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 at collabora.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".
>
> Okay, sounds like the hardware will automatically send out packets at
> the right time. That still leaves open the issue of how to ensure this
> is synchronized with userspace. Perhaps this could be done by attaching
> a property to a framebuffer, so that we'd know what exact frame the meta
> data is attached to and when to update the FIFOs for the infoframe.
>
> Usually it's a good idea to send this type of patch as part of a larger
> series precisely so that people can see how it is used. That should make
> it easier to see if this is good enough or needs some more thought on
> how to synchronize. Do you have any code that you could post that makes
> use of this new infoframe?
>

I was thinking use this and other helpers in the anx7814 bridge
driver[1], I thought that this patch should go through another tree,
this is the reason why I send it separately, but If you want or you
prefer I can send as part of these patch series.

[1] https://lkml.org/lkml/2015/11/13/284

>> >> @@ -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,
>
> I wasn't very clear. What I meant was the names for the constants. At
> least personally I know immediately what is meant when I see "I-Frame",
> "P-Frame" or "B-Frame", whereas "Bi-predictive picture" needs more
> thinking.
>
Got it, I'll change only the names in next version, then.

> Thierry

Thanks,
Enric