From: Brian Starkey <Brian.Starkey@arm.com>
To: Uma Shankar <uma.shankar@intel.com>
Cc: "ville.syrjala@intel.com" <ville.syrjala@intel.com>,
Liviu Dudau <Liviu.Dudau@arm.com>,
"intel-gfx@lists.freedesktop.org"
<intel-gfx@lists.freedesktop.org>,
"emil.l.velikov@gmail.com" <emil.l.velikov@gmail.com>,
"dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>, nd <nd@arm.com>,
"maarten.lankhorst@intel.com" <maarten.lankhorst@intel.com>
Subject: Re: [v6 06/13] drm: Enable HDR infoframe support
Date: Thu, 21 Mar 2019 11:41:44 +0000 [thread overview]
Message-ID: <20190321114143.vskyf63kvz36iloe@DESKTOP-E1NTVVP.localdomain> (raw)
In-Reply-To: <1553078906-5991-7-git-send-email-uma.shankar@intel.com>
On Wed, Mar 20, 2019 at 04:18:19PM +0530, Uma Shankar wrote:
> 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.
>
> 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.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
> drivers/gpu/drm/drm_edid.c | 56 ++++++++++++++++++++
> drivers/video/hdmi.c | 129 +++++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_edid.h | 4 ++
> include/linux/hdmi.h | 22 ++++++++
> 4 files changed, 211 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 676569b..78c0b97 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -4916,6 +4916,62 @@ 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,
> + void *hdr_metadata)
I don't think this needs to be/should be a void *, you might as well
us (struct hdr_static_metadata *). Looks like you do the cast in i915
before calling this function anyway.
> +{
> + struct hdr_static_metadata *hdr_source_metadata;
> + int err, i;
> +
> + 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);
> +
> + hdr_source_metadata = (struct hdr_static_metadata *)hdr_metadata;
> +
> + frame->length = sizeof(struct hdr_static_metadata);
> +
> + frame->eotf = hdr_source_metadata->eotf;
> + frame->metadata_type = hdr_source_metadata->metadata_type;
> +
> + for (i = 0; i < 3; i++) {
> + frame->display_primaries[i].x =
> + hdr_source_metadata->display_primaries[i].x;
> + frame->display_primaries[i].y =
> + hdr_source_metadata->display_primaries[i].y;
> + }
> +
> + frame->white_point.x = hdr_source_metadata->white_point.x;
> + frame->white_point.y = hdr_source_metadata->white_point.y;
> +
> + frame->max_mastering_display_luminance =
> + hdr_source_metadata->max_mastering_display_luminance;
> + frame->min_mastering_display_luminance =
> + hdr_source_metadata->min_mastering_display_luminance;
> +
> + frame->max_cll = hdr_source_metadata->max_cll;
> + frame->max_fall = hdr_source_metadata->max_fall;
Couldn't the infoframe definition embed the UAPI structure, making
this a straight memcpy() ?
> +
> + 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..80bb0ee 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -650,6 +650,93 @@ 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);
> +
> +/**
> + * 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 1.4 specification.
> + *
> + * Returns the number of bytes packed into the binary buffer or a negative
> + * error code on failure.
> + */
> +ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe *frame, void *buffer,
> + size_t size)
> +{
> + u8 *ptr = buffer;
> + size_t length;
> + int i;
> +
> + 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;
> +
> + *ptr++ = frame->eotf;
> + *ptr++ = frame->metadata_type;
> +
> + for (i = 0; i < 3; i++) {
> + *ptr++ = frame->display_primaries[i].x;
> + *ptr++ = frame->display_primaries[i].x >> 8;
> + *ptr++ = frame->display_primaries[i].y;
> + *ptr++ = frame->display_primaries[i].y >> 8;
> + }
> +
> + *ptr++ = frame->white_point.x;
> + *ptr++ = frame->white_point.x >> 8;
> +
> + *ptr++ = frame->white_point.y;
> + *ptr++ = frame->white_point.y >> 8;
> +
> + *ptr++ = frame->max_mastering_display_luminance;
> + *ptr++ = frame->max_mastering_display_luminance >> 8;
> +
> + *ptr++ = frame->min_mastering_display_luminance;
> + *ptr++ = frame->min_mastering_display_luminance >> 8;
> +
> + *ptr++ = frame->max_cll;
> + *ptr++ = frame->max_cll >> 8;
> +
> + *ptr++ = frame->max_fall;
> + *ptr++ = frame->max_fall >> 8;
> +
> + hdmi_infoframe_set_checksum(buffer, length);
> +
> + return length;
> +}
> +
> /*
> * hdmi_vendor_any_infoframe_check() - check a vendor infoframe
> */
> @@ -806,6 +893,9 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
> case HDMI_INFOFRAME_TYPE_AVI:
> length = hdmi_avi_infoframe_pack(&frame->avi, buffer, size);
> break;
> + case HDMI_INFOFRAME_TYPE_DRM:
> + length = hdmi_drm_infoframe_pack(&frame->drm, buffer, size);
> + break;
> case HDMI_INFOFRAME_TYPE_SPD:
> length = hdmi_spd_infoframe_pack(&frame->spd, buffer, size);
> break;
> @@ -838,6 +928,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_DRM:
> + return "Dynamic Range and Mastering";
> }
> return "Reserved";
> }
> @@ -1284,6 +1376,40 @@ static void hdmi_audio_infoframe_log(const char *level,
> frame->downmix_inhibit ? "Yes" : "No");
> }
>
> +/**
> + * hdmi_drm_infoframe_log() - log info of HDMI DRM infoframe
> + * @level: logging level
> + * @dev: device
> + * @frame: HDMI DRM infoframe
> + */
> +static void hdmi_drm_infoframe_log(const char *level,
> + struct device *dev,
> + const struct hdmi_drm_infoframe *frame)
> +{
> + int i;
> +
> + hdmi_infoframe_log_header(level, dev,
> + (struct hdmi_any_infoframe *)frame);
> + hdmi_log("length: %d\n", frame->length);
> + hdmi_log("metadata type: %d\n", frame->metadata_type);
> + hdmi_log("eotf: %d\n", frame->eotf);
> + for (i = 0; i < 3; i++) {
> + hdmi_log("x[%d]: %d\n", i, frame->display_primaries[i].x);
> + hdmi_log("y[%d]: %d\n", i, frame->display_primaries[i].y);
> + }
> +
> + hdmi_log("white point x: %d\n", frame->white_point.x);
> + hdmi_log("white point y: %d\n", frame->white_point.y);
> +
> + hdmi_log("max_mastering_display_luminance: %d\n",
> + frame->max_mastering_display_luminance);
> + hdmi_log("min_mastering_display_luminance: %d\n",
> + frame->min_mastering_display_luminance);
> +
> + hdmi_log("max_cll: %d\n", frame->max_cll);
> + hdmi_log("max_fall: %d\n", frame->max_fall);
> +}
> +
> static const char *
> hdmi_3d_structure_get_name(enum hdmi_3d_structure s3d_struct)
> {
> @@ -1372,6 +1498,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_DRM:
> + hdmi_drm_infoframe_log(level, dev, &frame->drm);
> + break;
> }
> }
> EXPORT_SYMBOL(hdmi_infoframe_log);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 9d3b5b9..973e43e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -370,6 +370,10 @@ int drm_av_sync_delay(struct drm_connector *connector,
> const struct drm_display_mode *mode,
> enum hdmi_quantization_range rgb_quant_range);
>
> +int
> +drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
> + void *hdr_source_metadata);
> +
> /**
> * drm_eld_mnl - Get ELD monitor name length in bytes.
> * @eld: pointer to an eld memory structure with mnl set
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index a065194..b925b24 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -47,6 +47,7 @@ enum hdmi_infoframe_type {
> HDMI_INFOFRAME_TYPE_AVI = 0x82,
> HDMI_INFOFRAME_TYPE_SPD = 0x83,
> HDMI_INFOFRAME_TYPE_AUDIO = 0x84,
> + HDMI_INFOFRAME_TYPE_DRM = 0x87,
> };
>
> #define HDMI_IEEE_OUI 0x000c03
> @@ -185,12 +186,32 @@ struct hdmi_avi_infoframe {
> unsigned short right_bar;
> };
>
> +struct hdmi_drm_infoframe {
> + enum hdmi_infoframe_type type;
> + unsigned char version;
> + unsigned char length;
> + enum hdmi_eotf eotf;
> + enum hdmi_metadata_type metadata_type;
> + struct {
> + u16 x, y;
> + } display_primaries[3];
> + struct {
> + u16 x, y;
> + } white_point;
> + u16 max_mastering_display_luminance;
> + u16 min_mastering_display_luminance;
> + u16 max_fall;
> + u16 max_cll;
Less confusing IMO if these are the same way around as in the spec.
max_cll first, then max_fall.
> + u16 min_cll;
This guy isn't in the copy of I have (CTA-861-G). I'm probably out of
date, but you also don't print or encode this value, so should it be
dropped?
Cheers,
-Brian
> +};
> +
> int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame);
> ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
> size_t size);
> ssize_t hdmi_avi_infoframe_pack_only(const struct hdmi_avi_infoframe *frame,
> void *buffer, size_t size);
> int hdmi_avi_infoframe_check(struct hdmi_avi_infoframe *frame);
> +int hdmi_drm_infoframe_init(struct hdmi_drm_infoframe *frame);
>
> enum hdmi_spd_sdi {
> HDMI_SPD_SDI_UNKNOWN,
> @@ -365,6 +386,7 @@ ssize_t hdmi_vendor_infoframe_pack_only(const struct hdmi_vendor_infoframe *fram
> struct hdmi_spd_infoframe spd;
> union hdmi_vendor_any_infoframe vendor;
> struct hdmi_audio_infoframe audio;
> + struct hdmi_drm_infoframe drm;
> };
>
> ssize_t hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer,
> --
> 1.9.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2019-03-21 11:41 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-20 10:48 [v6 00/13] Add HDR Metadata Parsing and handling in DRM layer Uma Shankar
2019-03-20 10:48 ` [v6 01/13] drm: Add HDR source metadata property Uma Shankar
2019-03-21 11:30 ` Brian Starkey
2019-03-29 6:13 ` Shankar, Uma
2019-03-20 10:48 ` [v6 02/13] drm: Parse HDR metadata info from EDID Uma Shankar
2019-03-29 11:41 ` Sharma, Shashank
2019-03-20 10:48 ` [v6 03/13] drm: Parse Colorimetry data block " Uma Shankar
2019-03-21 11:17 ` Brian Starkey
2019-03-29 6:16 ` Shankar, Uma
2019-03-20 10:48 ` [v6 04/13] drm/i915: Attach HDR metadata property to connector Uma Shankar
2019-03-20 10:48 ` [v6 05/13] drm: Implement HDR output metadata set and get property handling Uma Shankar
2019-03-21 11:46 ` Brian Starkey
2019-03-29 6:21 ` Shankar, Uma
2019-03-20 10:48 ` [v6 06/13] drm: Enable HDR infoframe support Uma Shankar
2019-03-21 11:41 ` Brian Starkey [this message]
2019-03-29 6:24 ` Shankar, Uma
2019-03-20 10:48 ` [v6 07/13] drm/i915: Write HDR infoframe and send to panel Uma Shankar
2019-03-29 11:48 ` Sharma, Shashank
2019-03-20 10:48 ` [v6 08/13] drm/i915: [DO NOT MERGE] hack for glk board outputs Uma Shankar
2019-03-20 10:48 ` [v6 09/13] drm/i915: Add HLG EOTF Uma Shankar
2019-03-20 10:48 ` [v6 10/13] drm/i915: Enable infoframes on GLK+ for HDR Uma Shankar
2019-03-29 12:31 ` Sharma, Shashank
2019-03-20 10:48 ` [v6 11/13] drm/i915:Enabled Modeset when HDR Infoframe changes Uma Shankar
2019-03-29 12:56 ` Sharma, Shashank
2019-03-20 10:48 ` [v6 12/13] drm/i915: Set Infoframe for non modeset case for HDR Uma Shankar
2019-03-29 12:14 ` Sharma, Shashank
2019-03-29 14:04 ` Ville Syrjälä
2019-04-02 16:27 ` Shankar, Uma
2019-03-20 10:48 ` [v6 13/13] video/hdmi: Add const variants for drm infoframe Uma Shankar
2019-03-21 12:00 ` Brian Starkey
2019-03-29 6:29 ` Shankar, Uma
2019-03-29 12:22 ` Sharma, Shashank
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190321114143.vskyf63kvz36iloe@DESKTOP-E1NTVVP.localdomain \
--to=brian.starkey@arm.com \
--cc=Liviu.Dudau@arm.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=emil.l.velikov@gmail.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=maarten.lankhorst@intel.com \
--cc=nd@arm.com \
--cc=uma.shankar@intel.com \
--cc=ville.syrjala@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).