RE: [v6 06/13] drm: Enable HDR infoframe support

["Shankar, Uma" <uma.shankar@intel.com>]

>-----Original Message-----
>From: Brian Starkey [mailto:Brian.Starkey@arm.com]
>Sent: Thursday, March 21, 2019 5:12 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Lankhorst,
>Maarten <maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>;
>Sharma, Shashank <shashank.sharma@intel.com>; emil.l.velikov@gmail.com; Liviu
>Dudau <Liviu.Dudau@arm.com>; Ville Syrjälä <ville.syrjala@linux.intel.com>; nd
><nd@arm.com>
>Subject: Re: [v6 06/13] drm: Enable HDR infoframe support
>
>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.

Ok, will update this.

>> +{
>> +	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() ?

Yeah it's doable, currently the structures were not exactly same. Will try to make them
Identical and perform a memcpy as even suggested by Shashank.

>> +
>> +	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?

Sure, will match exactly to 861.G. There were certain deltas here b/w 861.F and G. Will update this.

Thanks & Regards,
Uma Shankar

>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