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 <[email protected]>
---
drivers/video/hdmi.c | 156 +++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hdmi.h | 24 ++++++++
2 files changed, 180 insertions(+)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 1626892..d37e821 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -388,6 +388,81 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
}
EXPORT_SYMBOL(hdmi_vendor_infoframe_pack);
+/**
+ * hdmi_mpeg_infoframe_init() - initialize an HDMI MPEG infoframe
+ * @frame: HDMI MPEG infoframe
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_mpeg_infoframe_init(struct hdmi_mpeg_infoframe *frame)
+{
+ memset(frame, 0, sizeof(*frame));
+
+ frame->type = HDMI_INFOFRAME_TYPE_MPEG;
+ frame->version = 1;
+ frame->length = HDMI_MPEG_INFOFRAME_SIZE;
+
+ return 0;
+}
+EXPORT_SYMBOL(hdmi_mpeg_infoframe_init);
+
+/**
+ * hdmi_mpeg_infoframe_pack() - write HDMI MPEG infoframe to binary buffer
+ * @frame: HDMI MPEG infoframe
+ * @buffer: destination buffer
+ * @size: size of buffer
+ *
+ * Packs the information contained in the @frame structure into a binary
+ * representation that can be written into the corresponding controller
+ * registers. Also computes the checksum as required by section 5.3.5 of
+ * the HDMI 1.4 specification.
+ *
+ * Returns the number of bytes packed into the binary buffer or a negative
+ * error code on failure.
+ */
+ssize_t hdmi_mpeg_infoframe_pack(struct hdmi_mpeg_infoframe *frame,
+ void *buffer, size_t size)
+{
+ u8 *ptr = buffer;
+ size_t length;
+
+ length = HDMI_INFOFRAME_HEADER_SIZE + frame->length;
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(buffer, 0, size);
+
+ ptr[0] = frame->type;
+ ptr[1] = frame->version;
+ ptr[2] = frame->length;
+ ptr[3] = 0; /* checksum */
+
+ /* start infoframe payload */
+ ptr += HDMI_INFOFRAME_HEADER_SIZE;
+
+ /*
+ * The MPEG Bit Rate is stored as a 32-bit number and is expressed in
+ * Hertz. MB#0 contains the least significant byte while MB#3 contains
+ * the most significant byte. If the MPEG Bit Rate is unknown or this
+ * field doesn’t apply, then all of the bits in Data Bytes 1-4 shall
+ * be set to 0.
+ */
+ ptr[0] = frame->bitrate & 0x000000ff;
+ ptr[1] = (frame->bitrate & 0x0000ff00) >> 8;
+ ptr[2] = (frame->bitrate & 0x00ff0000) >> 16;
+ ptr[3] = (frame->bitrate & 0xff000000) >> 24;
+
+ ptr[4] = frame->picture_type;
+ if (frame->repeated)
+ ptr[4] |= BIT(4);
+
+ hdmi_infoframe_set_checksum(buffer, length);
+
+ return length;
+}
+EXPORT_SYMBOL(hdmi_mpeg_infoframe_pack);
+
/*
* hdmi_vendor_any_infoframe_pack() - write a vendor infoframe to binary buffer
*/
@@ -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);
default:
WARN(1, "Bad infoframe type %d\n", frame->any.type);
length = -EINVAL;
@@ -457,6 +534,8 @@ static const char *hdmi_infoframe_type_get_name(enum hdmi_infoframe_type type)
return "Source Product Description (SPD)";
case HDMI_INFOFRAME_TYPE_AUDIO:
return "Audio";
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ return "MPEG";
}
return "Reserved";
}
@@ -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";
+ }
+ return "Reserved";
+}
+
+/**
+ * hdmi_mpeg_infoframe_log() - log info of HDMI MPEG infoframe
+ * @level: logging level
+ * @dev: device
+ * @frame: HDMI MPEG infoframe
+ */
+static void hdmi_mpeg_infoframe_log(const char *level,
+ struct device *dev,
+ struct hdmi_mpeg_infoframe *frame)
+{
+ hdmi_infoframe_log_header(level, dev,
+ (struct hdmi_any_infoframe *)frame);
+
+ hdmi_log(" bit rate: %d Hz\n", frame->bitrate);
+ hdmi_log(" frame type: %s\n",
+ hdmi_mpeg_picture_get_name(frame->picture_type));
+ hdmi_log(" repeated frame: %s\n",
+ frame->repeated ? "Yes" : "No");
+}
+
static const char *
hdmi_3d_structure_get_name(enum hdmi_3d_structure s3d_struct)
{
@@ -987,6 +1101,9 @@ void hdmi_infoframe_log(const char *level,
case HDMI_INFOFRAME_TYPE_VENDOR:
hdmi_vendor_any_infoframe_log(level, dev, &frame->vendor);
break;
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ hdmi_mpeg_infoframe_log(level, dev, &frame->mpeg);
+ break;
}
}
EXPORT_SYMBOL(hdmi_infoframe_log);
@@ -1138,6 +1255,42 @@ static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
}
/**
+ * hdmi_mpeg_infoframe_unpack() - unpack binary buffer to a HDMI MPEG infoframe
+ * @buffer: source buffer
+ * @frame: HDMI MPEG infoframe
+ *
+ * Unpacks the information contained in binary @buffer into a structured
+ * @frame of the HDMI MPEG information frame. Also verifies the checksum as
+ * required by section 5.3.5 of the HDMI 1.4 specification.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+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;
+ }
+
+ if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(MPEG)) != 0)
+ return -EINVAL;
+
+ ptr += HDMI_INFOFRAME_HEADER_SIZE;
+
+ frame->bitrate = (ptr[3] << 24) | (ptr[2] << 16) |
+ (ptr[1] << 8) | ptr[0];
+
+ frame->picture_type = ptr[4] & 0x03;
+ frame->repeated = ptr[4] & BIT(4) ? true : false;
+
+ return 0;
+}
+
+/**
* hdmi_vendor_infoframe_unpack() - unpack binary buffer to a HDMI vendor infoframe
* @buffer: source buffer
* @frame: HDMI Vendor infoframe
@@ -1234,6 +1387,9 @@ int hdmi_infoframe_unpack(union hdmi_infoframe *frame, void *buffer)
case HDMI_INFOFRAME_TYPE_VENDOR:
ret = hdmi_vendor_any_infoframe_unpack(&frame->vendor, buffer);
break;
+ case HDMI_INFOFRAME_TYPE_MPEG:
+ ret = hdmi_mpeg_infoframe_unpack(&frame->mpeg, buffer);
+ break;
default:
ret = -EINVAL;
break;
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index e974420..f480373 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -32,11 +32,13 @@ enum hdmi_infoframe_type {
HDMI_INFOFRAME_TYPE_AVI = 0x82,
HDMI_INFOFRAME_TYPE_SPD = 0x83,
HDMI_INFOFRAME_TYPE_AUDIO = 0x84,
+ HDMI_INFOFRAME_TYPE_MPEG = 0x85,
};
#define HDMI_IEEE_OUI 0x000c03
#define HDMI_INFOFRAME_HEADER_SIZE 4
#define HDMI_AVI_INFOFRAME_SIZE 13
+#define HDMI_MPEG_INFOFRAME_SIZE 10
#define HDMI_SPD_INFOFRAME_SIZE 25
#define HDMI_AUDIO_INFOFRAME_SIZE 10
@@ -297,6 +299,26 @@ int hdmi_vendor_infoframe_init(struct hdmi_vendor_infoframe *frame);
ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
void *buffer, size_t size);
+enum hdmi_mpeg_picture_type {
+ HDMI_MPEG_PICTURE_TYPE_UNKNOWN = 0x00,
+ HDMI_MPEG_PICTURE_TYPE_I = 0x01,
+ HDMI_MPEG_PICTURE_TYPE_B = 0x02,
+ HDMI_MPEG_PICTURE_TYPE_P = 0x03,
+};
+
+struct hdmi_mpeg_infoframe {
+ enum hdmi_infoframe_type type;
+ unsigned char version;
+ unsigned char length;
+ u32 bitrate;
+ enum hdmi_mpeg_picture_type picture_type;
+ bool repeated;
+};
+
+int hdmi_mpeg_infoframe_init(struct hdmi_mpeg_infoframe *frame);
+ssize_t hdmi_mpeg_infoframe_pack(struct hdmi_mpeg_infoframe *frame,
+ void *buffer, size_t size);
+
union hdmi_vendor_any_infoframe {
struct {
enum hdmi_infoframe_type type;
@@ -314,6 +336,7 @@ union hdmi_vendor_any_infoframe {
* @spd: spd infoframe
* @vendor: union of all vendor infoframes
* @audio: audio infoframe
+ * @mpeg: mpeg infoframe
*
* This is used by the generic pack function. This works since all infoframes
* have the same header which also indicates which type of infoframe should be
@@ -325,6 +348,7 @@ union hdmi_infoframe {
struct hdmi_spd_infoframe spd;
union hdmi_vendor_any_infoframe vendor;
struct hdmi_audio_infoframe audio;
+ struct hdmi_mpeg_infoframe mpeg;
};
ssize_t
--
2.1.0
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 <[email protected]>
> ---
> 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?
> 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;"?
> @@ -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.
> + 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.
> +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/
Thierry
Hello Thierry,
Many thanks for your comments.
2015-11-16 12:50 GMT+01:00 Thierry Reding <[email protected]>:
> 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 <[email protected]>
>> ---
>> 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
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 <[email protected]>:
> > 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 <[email protected]>
> >> ---
> >> 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
Hello Thierry,
2015-11-17 13:55 GMT+01:00 Thierry Reding <[email protected]>:
> 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 <[email protected]>:
>> > 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 <[email protected]>
>> >> ---
>> >> 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
On Tue, Nov 17, 2015 at 11:55:53PM +0100, Enric Balletbo Serra wrote:
> Hello Thierry,
>
> 2015-11-17 13:55 GMT+01:00 Thierry Reding <[email protected]>:
> > 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 <[email protected]>:
> >> > 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 <[email protected]>
> >> >> ---
> >> >> 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
I haven't seen those patches yet. I should've been Cc'ed on those
patches since I'm technically the maintainer of drm/bridge. Did the
get_maintainer.pl script not list me?
In my opinion, it's usually a good idea to keep all dependencies in a
single series, just so people get a better picture of what you're
submitting. Of course that's just my opinion, somebody else may yell at
you because they get Cc'ed on patches that they're not interested in...
As for merging patches, it's usually best to let maintainers figure that
out once the series is in good shape.
Thierry
Hello Thierry,
2015-11-19 12:51 GMT+01:00 Thierry Reding <[email protected]>:
> On Tue, Nov 17, 2015 at 11:55:53PM +0100, Enric Balletbo Serra wrote:
>> Hello Thierry,
>>
>> 2015-11-17 13:55 GMT+01:00 Thierry Reding <[email protected]>:
>> > 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 <[email protected]>:
>> >> > 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 <[email protected]>
>> >> >> ---
>> >> >> 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
>
> I haven't seen those patches yet. I should've been Cc'ed on those
> patches since I'm technically the maintainer of drm/bridge. Did the
> get_maintainer.pl script not list me?
>
Mmm, just checked and yes, get_maintainer list you, so probably I did
something getting the maintainers.
Sorry.
> In my opinion, it's usually a good idea to keep all dependencies in a
> single series, just so people get a better picture of what you're
> submitting. Of course that's just my opinion, somebody else may yell at
> you because they get Cc'ed on patches that they're not interested in...
>
> As for merging patches, it's usually best to let maintainers figure that
> out once the series is in good shape.
>
> Thierry
Hello Thierry,
2015-11-19 13:29 GMT+01:00 Enric Balletbo Serra <[email protected]>:
> Hello Thierry,
>
> 2015-11-19 12:51 GMT+01:00 Thierry Reding <[email protected]>:
>> On Tue, Nov 17, 2015 at 11:55:53PM +0100, Enric Balletbo Serra wrote:
>>> Hello Thierry,
>>>
>>> 2015-11-17 13:55 GMT+01:00 Thierry Reding <[email protected]>:
>>> > 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 <[email protected]>:
>>> >> > 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 <[email protected]>
>>> >> >> ---
>>> >> >> 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
>>
>> I haven't seen those patches yet. I should've been Cc'ed on those
>> patches since I'm technically the maintainer of drm/bridge. Did the
>> get_maintainer.pl script not list me?
>>
>
> Mmm, just checked and yes, get_maintainer list you, so probably I did
> something getting the maintainers.
> Sorry.
>
>> In my opinion, it's usually a good idea to keep all dependencies in a
>> single series, just so people get a better picture of what you're
>> submitting. Of course that's just my opinion, somebody else may yell at
>> you because they get Cc'ed on patches that they're not interested in...
>>
>> As for merging patches, it's usually best to let maintainers figure that
>> out once the series is in good shape.
>>
>> Thierry
As you suggested I included this patch series with anx7814 bridge
driver series. So you can see the context. Please feel free to comment
anything.
https://lkml.org/lkml/2015/12/4/66
Regards,
Enric