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

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 3321 bytes --]

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@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@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?

> >> @@ -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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]