Re: [v9 04/13] drm: Enable HDR infoframe support
On Tue, May 14, 2019 at 12:36:25PM +, Kazlauskas, Nicholas wrote:
> On 5/8/19 2:38 PM, Uma Shankar wrote:
> > [CAUTION: External Email]
> >
> > Enable Dynamic Range and Mastering Infoframe for HDR
> > content, which is defined in CEA 861.3 spec.
> >
> > The metadata will be computed based on blending
> > policy in userspace compositors and passed as a connector
> > property blob to driver. The same will be sent as infoframe
> > to panel which support HDR.
> >
> > Added the const version of infoframe for DRM metadata
> > for HDR.
> >
> > v2: Rebase and added Ville's POC changes.
> >
> > v3: No Change
> >
> > v4: Addressed Shashank's review comments and merged the
> > patch making drm infoframe function arguments as constant.
> >
> > v5: Rebase
> >
> > v6: Fixed checkpatch warnings with --strict option. Addressed
> > Shashank's review comments and added his RB.
> >
> > v7: Addressed Brian Starkey's review comments. Merged 2 patches
> > into one.
> >
> > v8: Addressed Jonas Karlman review comments.
> >
> > Signed-off-by: Uma Shankar
> > Signed-off-by: Ville Syrjälä
> > Reviewed-by: Shashank Sharma
> > ---
> > drivers/gpu/drm/drm_edid.c | 48
> > drivers/video/hdmi.c | 186
> > +
> > include/drm/drm_edid.h | 5 ++
> > include/linux/hdmi.h | 27 +++
> > 4 files changed, 266 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index fe2c29b..5f48965 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -4906,6 +4906,54 @@ static bool is_hdmi2_sink(struct drm_connector
> > *connector)
> > }
> >
> > /**
> > + * drm_hdmi_infoframe_set_hdr_metadata() - fill an HDMI AVI infoframe with
> > + * HDR metadata from userspace
> > + * @frame: HDMI AVI infoframe
> > + * @hdr_source_metadata: hdr_source_metadata info from userspace
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int
> > +drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
> > + struct hdr_output_metadata
> > *hdr_metadata)
hdr_metadata can be const.
> > +{
> > + int err;
> > +
> > + if (!frame || !hdr_metadata)
> > + return true;
true?
> > +
> > + err = hdmi_drm_infoframe_init(frame);
> > + if (err < 0)
> > + return err;
> > +
> > + DRM_DEBUG_KMS("type = %x\n", frame->type);
> > +
> > + frame->length = sizeof(struct hdr_metadata_infoframe);
> > +
> > + frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
> > + frame->metadata_type =
> > hdr_metadata->hdmi_metadata_type1.metadata_type;
> > +
> > + memcpy(>display_primaries,
> > + _metadata->hdmi_metadata_type1.display_primaries, 12);
> > +
> > + memcpy(>white_point,
> > + _metadata->hdmi_metadata_type1.white_point, 4);
> > +
> > + frame->max_display_mastering_luminance =
> > +
> > hdr_metadata->hdmi_metadata_type1.max_display_mastering_luminance;
> > + frame->min_display_mastering_luminance =
> > +
> > hdr_metadata->hdmi_metadata_type1.min_display_mastering_luminance;
> > + frame->max_fall = hdr_metadata->hdmi_metadata_type1.max_fall;
> > + frame->max_cll = hdr_metadata->hdmi_metadata_type1.max_cll;
> > +
> > + hdmi_infoframe_log(KERN_CRIT, NULL,
> > + (union hdmi_infoframe *)frame);
>
> This shouldn't really be KERN_CRIT for debug output. Maybe KERN_INFO or
> KERN_DEBUG or just drop the log altogether since it can't actually tag
> it to DRM or the driver itself in this function.
Yeah, this shouldn't be here. It's up to the driver to dump it when it
wants to.
>
> Nicholas Kazlauskas
>
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> > +
> > +/**
> >* drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI
> > infoframe with
> >* data from a DRM display
> > mode
> >* @frame: HDMI AVI infoframe
> > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> > index 799ae49..717ed7fb 100644
> > --- a/drivers/video/hdmi.c
> > +++ b/drivers/video/hdmi.c
> > @@ -650,6 +650,146 @@ ssize_t hdmi_vendor_infoframe_pack(struct
> > hdmi_vendor_infoframe *frame,
> > return 0;
> > }
> >
> > +/**
> > + * hdmi_drm_infoframe_init() - initialize an HDMI Dynaminc Range and
> > + * mastering infoframe
> > + * @frame: HDMI DRM infoframe
> > + *
> > + * Returns 0 on success or a negative error code on failure.
> > + */
> > +int hdmi_drm_infoframe_init(struct hdmi_drm_infoframe *frame)
> > +{
> > + memset(frame, 0, sizeof(*frame));
> > +
> > + frame->type = HDMI_INFOFRAME_TYPE_DRM;
> > + frame->version = 1;
> > +
> > + return 0;
> > +}
> >
RE: [v9 04/13] drm: Enable HDR infoframe support
>-Original Message-
>From: Kazlauskas, Nicholas [mailto:nicholas.kazlaus...@amd.com]
>Sent: Tuesday, May 14, 2019 6:06 PM
>To: Shankar, Uma ; intel-...@lists.freedesktop.org; dri-
>de...@lists.freedesktop.org
>Cc: dcasta...@chromium.org; jo...@kwiboo.se; emil.l.veli...@gmail.com;
>seanp...@chromium.org; Syrjala, Ville ; Lankhorst,
>Maarten
>
>Subject: Re: [v9 04/13] drm: Enable HDR infoframe support
>
>On 5/8/19 2:38 PM, Uma Shankar wrote:
>> [CAUTION: External Email]
>>
>> Enable Dynamic Range and Mastering Infoframe for HDR content, which is
>> defined in CEA 861.3 spec.
>>
>> The metadata will be computed based on blending policy in userspace
>> compositors and passed as a connector property blob to driver. The
>> same will be sent as infoframe to panel which support HDR.
>>
>> Added the const version of infoframe for DRM metadata for HDR.
>>
>> v2: Rebase and added Ville's POC changes.
>>
>> v3: No Change
>>
>> v4: Addressed Shashank's review comments and merged the patch making
>> drm infoframe function arguments as constant.
>>
>> v5: Rebase
>>
>> v6: Fixed checkpatch warnings with --strict option. Addressed
>> Shashank's review comments and added his RB.
>>
>> v7: Addressed Brian Starkey's review comments. Merged 2 patches into
>> one.
>>
>> v8: Addressed Jonas Karlman review comments.
>>
>> Signed-off-by: Uma Shankar
>> Signed-off-by: Ville Syrjälä
>> Reviewed-by: Shashank Sharma
>> ---
>> drivers/gpu/drm/drm_edid.c | 48
>> drivers/video/hdmi.c | 186
>+
>> include/drm/drm_edid.h | 5 ++
>> include/linux/hdmi.h | 27 +++
>> 4 files changed, 266 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index fe2c29b..5f48965 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -4906,6 +4906,54 @@ static bool is_hdmi2_sink(struct drm_connector
>*connector)
>> }
>>
>> /**
>> + * drm_hdmi_infoframe_set_hdr_metadata() - fill an HDMI AVI infoframe with
>> + * HDR metadata from userspace
>> + * @frame: HDMI AVI infoframe
>> + * @hdr_source_metadata: hdr_source_metadata info from userspace
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int
>> +drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
>> + struct hdr_output_metadata
>> +*hdr_metadata) {
>> + int err;
>> +
>> + if (!frame || !hdr_metadata)
>> + return true;
>> +
>> + err = hdmi_drm_infoframe_init(frame);
>> + if (err < 0)
>> + return err;
>> +
>> + DRM_DEBUG_KMS("type = %x\n", frame->type);
>> +
>> + frame->length = sizeof(struct hdr_metadata_infoframe);
>> +
>> + frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
>> + frame->metadata_type =
>> + hdr_metadata->hdmi_metadata_type1.metadata_type;
>> +
>> + memcpy(>display_primaries,
>> + _metadata->hdmi_metadata_type1.display_primaries,
>> + 12);
>> +
>> + memcpy(>white_point,
>> + _metadata->hdmi_metadata_type1.white_point, 4);
>> +
>> + frame->max_display_mastering_luminance =
>> + hdr_metadata-
>>hdmi_metadata_type1.max_display_mastering_luminance;
>> + frame->min_display_mastering_luminance =
>> + hdr_metadata-
>>hdmi_metadata_type1.min_display_mastering_luminance;
>> + frame->max_fall = hdr_metadata->hdmi_metadata_type1.max_fall;
>> + frame->max_cll = hdr_metadata->hdmi_metadata_type1.max_cll;
>> +
>> + hdmi_infoframe_log(KERN_CRIT, NULL,
>> + (union hdmi_infoframe *)frame);
>
>This shouldn't really be KERN_CRIT for debug output. Maybe KERN_INFO or
>KERN_DEBUG or just drop the log altogether since it can't actually tag it to
>DRM or
>the driver itself in this function.
I added this for debug and missed to drop it, it's really not needed. Will drop
this log in the next version.
Regards,
Uma Shankar
>Nicholas Kazlauskas
>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
>> +
>> +/**
>>* drm_hdmi_avi_infoframe_from_display_mode() - fill a
Re: [v9 04/13] drm: Enable HDR infoframe support
On 5/8/19 2:38 PM, Uma Shankar wrote:
> [CAUTION: External Email]
>
> Enable Dynamic Range and Mastering Infoframe for HDR
> content, which is defined in CEA 861.3 spec.
>
> The metadata will be computed based on blending
> policy in userspace compositors and passed as a connector
> property blob to driver. The same will be sent as infoframe
> to panel which support HDR.
>
> Added the const version of infoframe for DRM metadata
> for HDR.
>
> v2: Rebase and added Ville's POC changes.
>
> v3: No Change
>
> v4: Addressed Shashank's review comments and merged the
> patch making drm infoframe function arguments as constant.
>
> v5: Rebase
>
> v6: Fixed checkpatch warnings with --strict option. Addressed
> Shashank's review comments and added his RB.
>
> v7: Addressed Brian Starkey's review comments. Merged 2 patches
> into one.
>
> v8: Addressed Jonas Karlman review comments.
>
> Signed-off-by: Uma Shankar
> Signed-off-by: Ville Syrjälä
> Reviewed-by: Shashank Sharma
> ---
> drivers/gpu/drm/drm_edid.c | 48
> drivers/video/hdmi.c | 186
> +
> include/drm/drm_edid.h | 5 ++
> include/linux/hdmi.h | 27 +++
> 4 files changed, 266 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index fe2c29b..5f48965 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4906,6 +4906,54 @@ static bool is_hdmi2_sink(struct drm_connector
> *connector)
> }
>
> /**
> + * drm_hdmi_infoframe_set_hdr_metadata() - fill an HDMI AVI infoframe with
> + * HDR metadata from userspace
> + * @frame: HDMI AVI infoframe
> + * @hdr_source_metadata: hdr_source_metadata info from userspace
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int
> +drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
> + struct hdr_output_metadata *hdr_metadata)
> +{
> + int err;
> +
> + if (!frame || !hdr_metadata)
> + return true;
> +
> + err = hdmi_drm_infoframe_init(frame);
> + if (err < 0)
> + return err;
> +
> + DRM_DEBUG_KMS("type = %x\n", frame->type);
> +
> + frame->length = sizeof(struct hdr_metadata_infoframe);
> +
> + frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
> + frame->metadata_type =
> hdr_metadata->hdmi_metadata_type1.metadata_type;
> +
> + memcpy(>display_primaries,
> + _metadata->hdmi_metadata_type1.display_primaries, 12);
> +
> + memcpy(>white_point,
> + _metadata->hdmi_metadata_type1.white_point, 4);
> +
> + frame->max_display_mastering_luminance =
> +
> hdr_metadata->hdmi_metadata_type1.max_display_mastering_luminance;
> + frame->min_display_mastering_luminance =
> +
> hdr_metadata->hdmi_metadata_type1.min_display_mastering_luminance;
> + frame->max_fall = hdr_metadata->hdmi_metadata_type1.max_fall;
> + frame->max_cll = hdr_metadata->hdmi_metadata_type1.max_cll;
> +
> + hdmi_infoframe_log(KERN_CRIT, NULL,
> + (union hdmi_infoframe *)frame);
This shouldn't really be KERN_CRIT for debug output. Maybe KERN_INFO or
KERN_DEBUG or just drop the log altogether since it can't actually tag
it to DRM or the driver itself in this function.
Nicholas Kazlauskas
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> +
> +/**
>* drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe
> with
>* data from a DRM display mode
>* @frame: HDMI AVI infoframe
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 799ae49..717ed7fb 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -650,6 +650,146 @@ ssize_t hdmi_vendor_infoframe_pack(struct
> hdmi_vendor_infoframe *frame,
> return 0;
> }
>
> +/**
> + * hdmi_drm_infoframe_init() - initialize an HDMI Dynaminc Range and
> + * mastering infoframe
> + * @frame: HDMI DRM infoframe
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int hdmi_drm_infoframe_init(struct hdmi_drm_infoframe *frame)
> +{
> + memset(frame, 0, sizeof(*frame));
> +
> + frame->type = HDMI_INFOFRAME_TYPE_DRM;
> + frame->version = 1;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hdmi_drm_infoframe_init);
> +
> +static int hdmi_drm_infoframe_check_only(const struct hdmi_drm_infoframe
> *frame)
> +{
> + if (frame->type != HDMI_INFOFRAME_TYPE_DRM ||
> + frame->version != 1)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +/**
> + * hdmi_drm_infoframe_check() - check a HDMI DRM infoframe
> + * @frame: HDMI DRM infoframe
> + *
> + * Validates that the infoframe is consistent.
> + * Returns 0 on success or a negative error code
[v9 04/13] drm: Enable HDR infoframe support
Enable Dynamic Range and Mastering Infoframe for HDR
content, which is defined in CEA 861.3 spec.
The metadata will be computed based on blending
policy in userspace compositors and passed as a connector
property blob to driver. The same will be sent as infoframe
to panel which support HDR.
Added the const version of infoframe for DRM metadata
for HDR.
v2: Rebase and added Ville's POC changes.
v3: No Change
v4: Addressed Shashank's review comments and merged the
patch making drm infoframe function arguments as constant.
v5: Rebase
v6: Fixed checkpatch warnings with --strict option. Addressed
Shashank's review comments and added his RB.
v7: Addressed Brian Starkey's review comments. Merged 2 patches
into one.
v8: Addressed Jonas Karlman review comments.
Signed-off-by: Uma Shankar
Signed-off-by: Ville Syrjälä
Reviewed-by: Shashank Sharma
---
drivers/gpu/drm/drm_edid.c | 48
drivers/video/hdmi.c | 186 +
include/drm/drm_edid.h | 5 ++
include/linux/hdmi.h | 27 +++
4 files changed, 266 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index fe2c29b..5f48965 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -4906,6 +4906,54 @@ static bool is_hdmi2_sink(struct drm_connector
*connector)
}
/**
+ * drm_hdmi_infoframe_set_hdr_metadata() - fill an HDMI AVI infoframe with
+ * HDR metadata from userspace
+ * @frame: HDMI AVI infoframe
+ * @hdr_source_metadata: hdr_source_metadata info from userspace
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int
+drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
+ struct hdr_output_metadata *hdr_metadata)
+{
+ int err;
+
+ if (!frame || !hdr_metadata)
+ return true;
+
+ err = hdmi_drm_infoframe_init(frame);
+ if (err < 0)
+ return err;
+
+ DRM_DEBUG_KMS("type = %x\n", frame->type);
+
+ frame->length = sizeof(struct hdr_metadata_infoframe);
+
+ frame->eotf = hdr_metadata->hdmi_metadata_type1.eotf;
+ frame->metadata_type = hdr_metadata->hdmi_metadata_type1.metadata_type;
+
+ memcpy(>display_primaries,
+ _metadata->hdmi_metadata_type1.display_primaries, 12);
+
+ memcpy(>white_point,
+ _metadata->hdmi_metadata_type1.white_point, 4);
+
+ frame->max_display_mastering_luminance =
+
hdr_metadata->hdmi_metadata_type1.max_display_mastering_luminance;
+ frame->min_display_mastering_luminance =
+
hdr_metadata->hdmi_metadata_type1.min_display_mastering_luminance;
+ frame->max_fall = hdr_metadata->hdmi_metadata_type1.max_fall;
+ frame->max_cll = hdr_metadata->hdmi_metadata_type1.max_cll;
+
+ hdmi_infoframe_log(KERN_CRIT, NULL,
+ (union hdmi_infoframe *)frame);
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
+
+/**
* drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
* data from a DRM display mode
* @frame: HDMI AVI infoframe
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 799ae49..717ed7fb 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -650,6 +650,146 @@ ssize_t hdmi_vendor_infoframe_pack(struct
hdmi_vendor_infoframe *frame,
return 0;
}
+/**
+ * hdmi_drm_infoframe_init() - initialize an HDMI Dynaminc Range and
+ * mastering infoframe
+ * @frame: HDMI DRM infoframe
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_drm_infoframe_init(struct hdmi_drm_infoframe *frame)
+{
+ memset(frame, 0, sizeof(*frame));
+
+ frame->type = HDMI_INFOFRAME_TYPE_DRM;
+ frame->version = 1;
+
+ return 0;
+}
+EXPORT_SYMBOL(hdmi_drm_infoframe_init);
+
+static int hdmi_drm_infoframe_check_only(const struct hdmi_drm_infoframe
*frame)
+{
+ if (frame->type != HDMI_INFOFRAME_TYPE_DRM ||
+ frame->version != 1)
+ return -EINVAL;
+
+ return 0;
+}
+
+/**
+ * hdmi_drm_infoframe_check() - check a HDMI DRM infoframe
+ * @frame: HDMI DRM infoframe
+ *
+ * Validates that the infoframe is consistent.
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_drm_infoframe_check(struct hdmi_drm_infoframe *frame)
+{
+ return hdmi_drm_infoframe_check_only(frame);
+}
+EXPORT_SYMBOL(hdmi_drm_infoframe_check);
+
+/**
+ * hdmi_drm_infoframe_pack() - write HDMI DRM infoframe to binary buffer
+ * @frame: HDMI DRM 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
