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@xxxxxxxxx] > > >Sent: Saturday, May 4, 2019 3:48 PM > > >To: Shankar, Uma <uma.shankar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- > > >devel@xxxxxxxxxxxxxxxxxxxxx > > >Cc: dcastagna@xxxxxxxxxxxx; emil.l.velikov@xxxxxxxxx; seanpaul@xxxxxxxxxxxx; > > >Syrjala, Ville <ville.syrjala@xxxxxxxxx>; Lankhorst, Maarten > > ><maarten.lankhorst@xxxxxxxxx> > > >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@xxxxxxxxx> > > >> Reviewed-by: Shashank Sharma <shashank.sharma@xxxxxxxxx> > > >> --- > > >> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
