RE: [v9 12/13] video/hdmi: Add Unpack function for DRM infoframe

2019-05-14 Thread Shankar, Uma 


>-Original Message-
>From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On Behalf Of 
>Ville
>Syrjälä
>Sent: Tuesday, May 14, 2019 1:20 AM
>To: Shankar, Uma 
>Cc: dcasta...@chromium.org; jo...@kwiboo.se; intel-...@lists.freedesktop.org;
>emil.l.veli...@gmail.com; dri-devel@lists.freedesktop.org;
>seanp...@chromium.org; Syrjala, Ville ; Lankhorst, 
>Maarten
>
>Subject: Re: [v9 12/13] video/hdmi: Add Unpack function for DRM infoframe
>
>On Thu, May 09, 2019 at 12:08:52AM +0530, Uma Shankar wrote:
>> Added unpack function for DRM infoframe for dynamic range and
>> mastering infoframe readout.
>>
>> Suggested-by: Ville Syrjälä 
>> Signed-off-by: Uma Shankar 
>> ---
>>  drivers/video/hdmi.c | 54
>> 
>>  include/linux/hdmi.h |  1 +
>>  2 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index
>> 717ed7fb..110d405 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -1801,6 +1801,57 @@ static int hdmi_audio_infoframe_unpack(struct
>> hdmi_audio_infoframe *frame,  }
>>
>>  /**
>> + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM
>> +infoframe
>> + * @frame: HDMI DRM infoframe
>> + * @buffer: source buffer
>> + * @size: size of buffer
>> + *
>> + * Unpacks the information contained in binary @buffer into a
>> +structured
>> + * @frame of the HDMI Dynamic Range and Mastering (DRM) 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_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
>> + const void *buffer, size_t size) {
>> +const u8 *ptr = buffer;
>> +int ret;
>> +
>> +if (size < HDMI_INFOFRAME_SIZE(DRM))
>> +return -EINVAL;
>> +
>> +if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
>> +ptr[1] != 1 ||
>> +ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
>> +return -EINVAL;
>> +
>> +if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0)
>> +return -EINVAL;
>> +
>> +ret = hdmi_drm_infoframe_init(frame);
>> +if (ret)
>> +return ret;
>> +
>> +frame->length = ptr[2];
>
>The length handling is inconsistnet between the compute and readout sides. I 
>would
>suggest for now just put the length initialization into 
>hdmi_drm_infoframe_init() (and
>remove it from the drm code), and check it in the check_only(). We can revisit 
>it
>if/when other metadata formats become relevant.

Sure, will update this.

>> +ptr += HDMI_INFOFRAME_HEADER_SIZE;
>> +
>> +frame->eotf = ptr[0] & 0x7;
>> +frame->metadata_type = ptr[1] & 0x7;
>> +
>> +memcpy(>display_primaries, [2], 12);
>> +memcpy(>white_point, [14], 4);
>
>Endianness fail.

Ok, will explicitly copy these.

>> +
>> +frame->max_display_mastering_luminance = (ptr[19] << 8) | ptr[18];
>> +frame->min_display_mastering_luminance = (ptr[21] << 8) | ptr[20];
>> +frame->max_cll = (ptr[23] << 8) | ptr[22];
>> +frame->max_fall = (ptr[25] << 8) | ptr[24];
>> +
>> +return 0;
>> +}
>> +
>> +/**
>>   * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe
>>   * @frame: HDMI infoframe
>>   * @buffer: source buffer
>> @@ -1826,6 +1877,9 @@ int hdmi_infoframe_unpack(union hdmi_infoframe
>*frame,
>>  case HDMI_INFOFRAME_TYPE_AVI:
>>  ret = hdmi_avi_infoframe_unpack(>avi, buffer, size);
>>  break;
>> +case HDMI_INFOFRAME_TYPE_DRM:
>> +ret = hdmi_drm_infoframe_unpack(>drm, buffer, size);
>> +break;
>>  case HDMI_INFOFRAME_TYPE_SPD:
>>  ret = hdmi_spd_infoframe_unpack(>spd, buffer, size);
>>  break;
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
>> 3d7f10f..ee55ba5 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -56,6 +56,7 @@ enum hdmi_infoframe_type {
>>  #define HDMI_AVI_INFOFRAME_SIZE13
>>  #define HDMI_SPD_INFOFRAME_SIZE25
>>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
>> +#define HDMI_DRM_INFOFRAME_SIZE26
>>
>>  #define HDMI_INFOFRAME_SIZE(type)   \
>>  (HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
>> --
>> 1.9.1
>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>--
>Ville Syrjälä
>Intel
>___
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v9 12/13] video/hdmi: Add Unpack function for DRM infoframe

2019-05-13 Thread Ville Syrjälä 
On Thu, May 09, 2019 at 12:08:52AM +0530, Uma Shankar wrote:
> Added unpack function for DRM infoframe for dynamic
> range and mastering infoframe readout.
> 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Uma Shankar 
> ---
>  drivers/video/hdmi.c | 54 
> 
>  include/linux/hdmi.h |  1 +
>  2 files changed, 55 insertions(+)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 717ed7fb..110d405 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -1801,6 +1801,57 @@ static int hdmi_audio_infoframe_unpack(struct 
> hdmi_audio_infoframe *frame,
>  }
>  
>  /**
> + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe
> + * @frame: HDMI DRM infoframe
> + * @buffer: source buffer
> + * @size: size of buffer
> + *
> + * Unpacks the information contained in binary @buffer into a structured
> + * @frame of the HDMI Dynamic Range and Mastering (DRM) 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_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
> +  const void *buffer, size_t size)
> +{
> + const u8 *ptr = buffer;
> + int ret;
> +
> + if (size < HDMI_INFOFRAME_SIZE(DRM))
> + return -EINVAL;
> +
> + if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
> + ptr[1] != 1 ||
> + ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
> + return -EINVAL;
> +
> + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0)
> + return -EINVAL;
> +
> + ret = hdmi_drm_infoframe_init(frame);
> + if (ret)
> + return ret;
> +
> + frame->length = ptr[2];

The length handling is inconsistnet between the compute and readout
sides. I would suggest for now just put the length initialization
into hdmi_drm_infoframe_init() (and remove it from the drm code),
and check it in the check_only(). We can revisit it if/when other
metadata formats become relevant.

> + ptr += HDMI_INFOFRAME_HEADER_SIZE;
> +
> + frame->eotf = ptr[0] & 0x7;
> + frame->metadata_type = ptr[1] & 0x7;
> +
> + memcpy(>display_primaries, [2], 12);
> + memcpy(>white_point, [14], 4);

Endianness fail.

> +
> + frame->max_display_mastering_luminance = (ptr[19] << 8) | ptr[18];
> + frame->min_display_mastering_luminance = (ptr[21] << 8) | ptr[20];
> + frame->max_cll = (ptr[23] << 8) | ptr[22];
> + frame->max_fall = (ptr[25] << 8) | ptr[24];
> +
> + return 0;
> +}
> +
> +/**
>   * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe
>   * @frame: HDMI infoframe
>   * @buffer: source buffer
> @@ -1826,6 +1877,9 @@ int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
>   case HDMI_INFOFRAME_TYPE_AVI:
>   ret = hdmi_avi_infoframe_unpack(>avi, buffer, size);
>   break;
> + case HDMI_INFOFRAME_TYPE_DRM:
> + ret = hdmi_drm_infoframe_unpack(>drm, buffer, size);
> + break;
>   case HDMI_INFOFRAME_TYPE_SPD:
>   ret = hdmi_spd_infoframe_unpack(>spd, buffer, size);
>   break;
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 3d7f10f..ee55ba5 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -56,6 +56,7 @@ enum hdmi_infoframe_type {
>  #define HDMI_AVI_INFOFRAME_SIZE13
>  #define HDMI_SPD_INFOFRAME_SIZE25
>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
> +#define HDMI_DRM_INFOFRAME_SIZE26
>  
>  #define HDMI_INFOFRAME_SIZE(type)\
>   (HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[v9 12/13] video/hdmi: Add Unpack function for DRM infoframe

2019-05-08 Thread Uma Shankar 
Added unpack function for DRM infoframe for dynamic
range and mastering infoframe readout.

Suggested-by: Ville Syrjälä 
Signed-off-by: Uma Shankar 
---
 drivers/video/hdmi.c | 54 
 include/linux/hdmi.h |  1 +
 2 files changed, 55 insertions(+)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 717ed7fb..110d405 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -1801,6 +1801,57 @@ static int hdmi_audio_infoframe_unpack(struct 
hdmi_audio_infoframe *frame,
 }
 
 /**
+ * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe
+ * @frame: HDMI DRM infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
+ *
+ * Unpacks the information contained in binary @buffer into a structured
+ * @frame of the HDMI Dynamic Range and Mastering (DRM) 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_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
+const void *buffer, size_t size)
+{
+   const u8 *ptr = buffer;
+   int ret;
+
+   if (size < HDMI_INFOFRAME_SIZE(DRM))
+   return -EINVAL;
+
+   if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
+   ptr[1] != 1 ||
+   ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
+   return -EINVAL;
+
+   if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0)
+   return -EINVAL;
+
+   ret = hdmi_drm_infoframe_init(frame);
+   if (ret)
+   return ret;
+
+   frame->length = ptr[2];
+   ptr += HDMI_INFOFRAME_HEADER_SIZE;
+
+   frame->eotf = ptr[0] & 0x7;
+   frame->metadata_type = ptr[1] & 0x7;
+
+   memcpy(>display_primaries, [2], 12);
+   memcpy(>white_point, [14], 4);
+
+   frame->max_display_mastering_luminance = (ptr[19] << 8) | ptr[18];
+   frame->min_display_mastering_luminance = (ptr[21] << 8) | ptr[20];
+   frame->max_cll = (ptr[23] << 8) | ptr[22];
+   frame->max_fall = (ptr[25] << 8) | ptr[24];
+
+   return 0;
+}
+
+/**
  * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe
  * @frame: HDMI infoframe
  * @buffer: source buffer
@@ -1826,6 +1877,9 @@ int hdmi_infoframe_unpack(union hdmi_infoframe *frame,
case HDMI_INFOFRAME_TYPE_AVI:
ret = hdmi_avi_infoframe_unpack(>avi, buffer, size);
break;
+   case HDMI_INFOFRAME_TYPE_DRM:
+   ret = hdmi_drm_infoframe_unpack(>drm, buffer, size);
+   break;
case HDMI_INFOFRAME_TYPE_SPD:
ret = hdmi_spd_infoframe_unpack(>spd, buffer, size);
break;
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 3d7f10f..ee55ba5 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -56,6 +56,7 @@ enum hdmi_infoframe_type {
 #define HDMI_AVI_INFOFRAME_SIZE13
 #define HDMI_SPD_INFOFRAME_SIZE25
 #define HDMI_AUDIO_INFOFRAME_SIZE  10
+#define HDMI_DRM_INFOFRAME_SIZE26
 
 #define HDMI_INFOFRAME_SIZE(type)  \
(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
-- 
1.9.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel