[Intel-gfx] [v8 01/10] drm: Add HDR source metadata property

Ville Syrjälä ville.syrjala at linux.intel.com
Tue May 7 12:10:17 UTC 2019


On Tue, May 07, 2019 at 01:25:42PM +0300, Ville Syrjälä wrote:
> On Tue, May 07, 2019 at 09:03:45AM +0000, Shankar, Uma wrote:
> > 
> > 
> > >-----Original Message-----
> > >From: Jonas Karlman [mailto:jonas at kwiboo.se]
> > >Sent: Saturday, May 4, 2019 3:48 PM
> > >To: Shankar, Uma <uma.shankar at intel.com>; intel-gfx at lists.freedesktop.org; dri-
> > >devel at lists.freedesktop.org
> > >Cc: dcastagna at chromium.org; emil.l.velikov at gmail.com; seanpaul at chromium.org;
> > >Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst, Maarten
> > ><maarten.lankhorst at intel.com>
> > >Subject: Re: [v8 01/10] drm: Add HDR source metadata property
> > >
> > >On 2019-04-09 18:44, Uma Shankar wrote:
> > >> This patch adds a blob property to get HDR metadata information from
> > >> userspace. This will be send as part of AVI Infoframe to panel.
> > >>
> > >> It also implements get() and set() functions for HDR output metadata
> > >> property.The blob data is received from userspace and saved in
> > >> connector state, the same is returned as blob in get property call to
> > >> userspace.
> > >>
> > >> v2: Rebase and modified the metadata structure elements as per Ville's
> > >> POC changes.
> > >>
> > >> v3: No Change
> > >>
> > >> v4: Addressed Shashank's review comments
> > >>
> > >> v5: Rebase.
> > >>
> > >> v6: Addressed Brian Starkey's review comments, defined new structure
> > >> with header for dynamic metadata scalability.
> > >> Merge get/set property functions for metadata in this patch.
> > >>
> > >> v7: Addressed Jonas Karlman review comments and defined separate
> > >> structure for infoframe to better align with CTA 861.G spec. Added
> > >> Shashank's RB.
> > >>
> > >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > >> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
> > >> ---
> > >>  drivers/gpu/drm/drm_atomic.c      |  2 ++
> > >>  drivers/gpu/drm/drm_atomic_uapi.c | 13 +++++++++++++
> > >>  drivers/gpu/drm/drm_connector.c   |  6 ++++++
> > >>  include/drm/drm_connector.h       | 11 +++++++++++
> > >>  include/drm/drm_mode_config.h     |  6 ++++++
> > >>  include/linux/hdmi.h              | 10 ++++++++++
> > >>  include/uapi/drm/drm_mode.h       | 39
> > >+++++++++++++++++++++++++++++++++++++++
> > >>  7 files changed, 87 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_atomic.c
> > >> b/drivers/gpu/drm/drm_atomic.c index 5eb4013..8b9c126 100644
> > >> --- a/drivers/gpu/drm/drm_atomic.c
> > >> +++ b/drivers/gpu/drm/drm_atomic.c
> > >> @@ -881,6 +881,8 @@ static void
> > >> drm_atomic_connector_print_state(struct drm_printer *p,
> > >>
> > >>  	drm_printf(p, "connector[%u]: %s\n", connector->base.id, connector-
> > >>name);
> > >>  	drm_printf(p, "\tcrtc=%s\n", state->crtc ? state->crtc->name :
> > >> "(null)");
> > >> +	drm_printf(p, "\thdr_metadata_changed=%d\n",
> > >> +		   state->hdr_metadata_changed);
> > >>
> > >>  	if (connector->connector_type == DRM_MODE_CONNECTOR_WRITEBACK)
> > >>  		if (state->writeback_job && state->writeback_job->fb) diff --git
> > >> a/drivers/gpu/drm/drm_atomic_uapi.c
> > >> b/drivers/gpu/drm/drm_atomic_uapi.c
> > >> index ea797d4..6d0d161 100644
> > >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > >> @@ -673,6 +673,8 @@ static int
> > >> drm_atomic_connector_set_property(struct drm_connector *connector,  {
> > >>  	struct drm_device *dev = connector->dev;
> > >>  	struct drm_mode_config *config = &dev->mode_config;
> > >> +	bool replaced = false;
> > >> +	int ret;
> > >>
> > >>  	if (property == config->prop_crtc_id) {
> > >>  		struct drm_crtc *crtc = drm_crtc_find(dev, NULL, val); @@ -721,6
> > >> +723,14 @@ static int drm_atomic_connector_set_property(struct drm_connector
> > >*connector,
> > >>  		 */
> > >>  		if (state->link_status != DRM_LINK_STATUS_GOOD)
> > >>  			state->link_status = val;
> > >> +	} else if (property == config->hdr_output_metadata_property) {
> > >> +		ret = drm_atomic_replace_property_blob_from_id(dev,
> > >> +				&state->hdr_output_metadata_blob_ptr,
> > >> +				val,
> > >> +				-1, sizeof(struct hdr_output_metadata),
> > >> +				&replaced);
> > >> +		state->hdr_metadata_changed |= replaced;
> > >> +		return ret;
> > >>  	} else if (property == config->aspect_ratio_property) {
> > >>  		state->picture_aspect_ratio = val;
> > >>  	} else if (property == config->content_type_property) { @@ -807,6
> > >> +817,9 @@ static int drm_atomic_connector_set_property(struct drm_connector
> > >*connector,
> > >>  		*val = state->colorspace;
> > >>  	} else if (property == connector->scaling_mode_property) {
> > >>  		*val = state->scaling_mode;
> > >> +	} else if (property == config->hdr_output_metadata_property) {
> > >> +		*val = (state->hdr_output_metadata_blob_ptr) ?
> > >> +			state->hdr_output_metadata_blob_ptr->base.id : 0;
> > >>  	} else if (property == connector->content_protection_property) {
> > >>  		*val = state->content_protection;
> > >>  	} else if (property == config->writeback_fb_id_property) { diff
> > >> --git a/drivers/gpu/drm/drm_connector.c
> > >> b/drivers/gpu/drm/drm_connector.c index 2355124..0bdae90 100644
> > >> --- a/drivers/gpu/drm/drm_connector.c
> > >> +++ b/drivers/gpu/drm/drm_connector.c
> > >> @@ -1058,6 +1058,12 @@ int drm_connector_create_standard_properties(struct
> > >drm_device *dev)
> > >>  		return -ENOMEM;
> > >>  	dev->mode_config.non_desktop_property = prop;
> > >>
> > >> +	prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> > >> +				   "HDR_OUTPUT_METADATA", 0);
> > >> +	if (!prop)
> > >> +		return -ENOMEM;
> > >> +	dev->mode_config.hdr_output_metadata_property = prop;
> > >> +
> > >>  	return 0;
> > >>  }
> > >>
> > >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > >> index 02a1312..5343f60 100644
> > >> --- a/include/drm/drm_connector.h
> > >> +++ b/include/drm/drm_connector.h
> > >> @@ -599,6 +599,13 @@ struct drm_connector_state {
> > >>  	 * and the connector bpc limitations obtained from edid.
> > >>  	 */
> > >>  	u8 max_bpc;
> > >> +
> > >> +	/**
> > >> +	 * @metadata_blob_ptr:
> > >> +	 * DRM blob property for HDR output metadata
> > >> +	 */
> > >> +	struct drm_property_blob *hdr_output_metadata_blob_ptr;
> > >> +	u8 hdr_metadata_changed : 1;
> > >>  };
> > >>
> > >>  /**
> > >> @@ -1239,6 +1246,10 @@ struct drm_connector {
> > >>  	 * &drm_mode_config.connector_free_work.
> > >>  	 */
> > >>  	struct llist_node free_node;
> > >> +
> > >> +	/* HDR metdata */
> > >> +	struct hdr_output_metadata hdr_output_metadata;
> > >> +	struct hdr_sink_metadata hdr_sink_metadata;
> > >>  };
> > >>
> > >>  #define obj_to_connector(x) container_of(x, struct drm_connector,
> > >> base) diff --git a/include/drm/drm_mode_config.h
> > >> b/include/drm/drm_mode_config.h index 7f60e8e..ef2656b 100644
> > >> --- a/include/drm/drm_mode_config.h
> > >> +++ b/include/drm/drm_mode_config.h
> > >> @@ -836,6 +836,12 @@ struct drm_mode_config {
> > >>  	 */
> > >>  	struct drm_property *writeback_out_fence_ptr_property;
> > >>
> > >> +	/*
> > >> +	 * hdr_metadata_property: Connector property containing hdr metatda
> > >> +	 * This will be provided by userspace compositors based on HDR content
> > >> +	 */
> > >> +	struct drm_property *hdr_output_metadata_property;
> > >> +
> > >>  	/* dumb ioctl parameters */
> > >>  	uint32_t preferred_depth, prefer_shadow;
> > >>
> > >> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
> > >> 927ad64..a065194 100644
> > >> --- a/include/linux/hdmi.h
> > >> +++ b/include/linux/hdmi.h
> > >> @@ -152,6 +152,16 @@ enum hdmi_content_type {
> > >>  	HDMI_CONTENT_TYPE_GAME,
> > >>  };
> > >>
> > >> +enum hdmi_metadata_type {
> > >> +	HDMI_STATIC_METADATA_TYPE1 = 1,
> > >> +};
> > >> +
> > >> +enum hdmi_eotf {
> > >> +	HDMI_EOTF_TRADITIONAL_GAMMA_SDR,
> > >> +	HDMI_EOTF_TRADITIONAL_GAMMA_HDR,
> > >> +	HDMI_EOTF_SMPTE_ST2084,
> > >> +};
> > >> +
> > >>  struct hdmi_avi_infoframe {
> > >>  	enum hdmi_infoframe_type type;
> > >>  	unsigned char version;
> > >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > >> index 83cd163..7b6a519 100644
> > >> --- a/include/uapi/drm/drm_mode.h
> > >> +++ b/include/uapi/drm/drm_mode.h
> > >> @@ -630,6 +630,45 @@ struct drm_color_lut {
> > >>  	__u16 reserved;
> > >>  };
> > >>
> > >> +/* HDR Metadata Infoframe as per 861.G spec */ struct
> > >> +hdr_metadata_infoframe {
> > >> +	__u8 eotf;
> > >> +	__u8 metadata_type;
> > >> +	struct {
> > >> +		__u16 x, y;
> > >> +		} display_primaries[3];
> > >> +	struct {
> > >> +		__u16 x, y;
> > >> +		} white_point;
> > >> +	__u16 max_display_mastering_luminance;
> > >> +	__u16 min_display_mastering_luminance;
> > >> +	__u16 max_cll;
> > >> +	__u16 max_fall;
> > >> +};
> > >> +
> > >> +struct hdr_output_metadata {
> > >> +	__u32 metadata_type;
> > >> +	union {
> > >> +		struct hdr_metadata_infoframe hdmi_metadata_type1;
> > >> +	};
> > >> +};
> > >> +
> > >> +/* HDR Metadata as per 861.G spec */
> > >> +struct hdr_static_metadata {
> > >> +	__u8 eotf;
> > >> +	__u8 metadata_type;
> > >> +	__u16 max_cll;
> > >> +	__u16 max_fall;
> > >> +	__u16 min_cll;
> > >> +};
> > >> +
> > >> +struct hdr_sink_metadata {
> > >> +	__u32 metadata_type;
> > >> +	union {
> > >> +		struct hdr_static_metadata hdmi_type1;
> > >> +	};
> > >> +};
> > >
> > >These two structs (hdr_static_metadata and hdr_sink_metadata) should probably not
> > >be part of uapi, unless the metadata is intended to be exposed in a
> > >HDR_SINK_METADATA property.
> > 
> > Hmm yeah, currently we are not planning it to expose that. It was just to help userspace
> > get and parse the EDID and store the details. But yes this doesn't need to be UAPI and
> > userspace should be free to use whatever way they want to parse and store sink metadata.
> > Will move it outside of uapi headers.
> > 
> > 
> > >The commit "drm/i915: Add HLG EOTF" should probably not use a i915 in prefix as it
> > >affects drm core.
> > 
> > Ok, will update this.
> > 
> > >
> > >The blob_ptr should probably be reference counted in duplicate_state/destroy_state
> > >helper similar to [1].
> > >I know too little about drm core in order to know if that is correct, please feel free to
> > >pick/squash if this is correct.
> > 
> > Sure, I will add these changes to the series.
> > 
> > >[1] https://github.com/Kwiboo/linux-
> > >rockchip/commit/5f065ff0bbcca145ee46e9466bb8ca048c7a7b1e
> > >
> > >I have tested this patchset on rockchip / dw-hdmi using dw-hdmi [2] and Kodi [3]
> > >patches with a successful result of enabling HDR mode on the sink.
> > >There is more work needed to get a full 10-bit pipeline for dw-hdmi in order to make
> > >full use of HDR, but for the purpose of enabling HDR on the sink this patchset seems
> > >ready.
> > 
> > Thanks Jonas for your comments and more importantly for testing and enabling with kodi.
> > 
> > I will rebase the series and send out next version. With the changes already in place in kodi branch,
> > we should probably be good for merge.
> 
> Patch 9 is not ready IMO. I don't see any analysis on why it's OK to
> update infoframes without a full modeset. I would just drop that patch
> from the initial set and work on it as a followup.
> 
> Also did someone analysze how the new dynamic HDR metadata fits in with
> the new uapi?

Also I can't see readout + state checker for the HDR infoframe. Am I blind
or is it missing?

-- 
Ville Syrjälä
Intel


More information about the Intel-gfx mailing list