[Intel-gfx] [v6 13/13] video/hdmi: Add const variants for drm infoframe

[Shankar, Uma]

>-----Original Message-----
>From: dri-devel [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Brian
>Starkey
>Sent: Thursday, March 21, 2019 5:31 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: Syrjala, Ville <ville.syrjala at intel.com>; Liviu Dudau <Liviu.Dudau at arm.com>;
>intel-gfx at lists.freedesktop.org; emil.l.velikov at gmail.com; dri-
>devel at lists.freedesktop.org; nd <nd at arm.com>; Lankhorst, Maarten
><maarten.lankhorst at intel.com>
>Subject: Re: [v6 13/13] video/hdmi: Add const variants for drm infoframe
>
>Hi,
>
>On Wed, Mar 20, 2019 at 04:18:26PM +0530, Uma Shankar wrote:
>> Added the const version of infoframe for DRM metadata for HDR.
>>
>> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
>
>Unless there's a strong reason not to, I think this can be squashed into patch 6.

This was added later after some infoframe rework, so was keeping this
separately, but yeah this can be squashed with 6.

>> ---
>>  drivers/video/hdmi.c | 63
>> ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  include/linux/hdmi.h |  5 +++++
>>  2 files changed, 66 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index
>> 80bb0ee..f9ca555 100644
>> --- a/drivers/video/hdmi.c
>> +++ b/drivers/video/hdmi.c
>> @@ -668,6 +668,30 @@ int hdmi_drm_infoframe_init(struct
>> hdmi_drm_infoframe *frame)  }  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 and updates derived
>> +fields
>> + * (eg. length) based on other fields.
>
>This comment doesn't match the implementation.

Ok, will update this comment.

Thanks Brian for the review and valuable feedback. Will send out the next version soon
fixing all the comments.

Regards,
Uma Shankar

>Thanks,
>-Brian
>
>> + *
>> + * 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
>> @@ -682,8 +706,8 @@ int hdmi_drm_infoframe_init(struct hdmi_drm_infoframe
>*frame)
>>   * 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)
>> +ssize_t hdmi_drm_infoframe_pack_only(const struct hdmi_drm_infoframe *frame,
>> +				     void *buffer, size_t size)
>>  {
>>  	u8 *ptr = buffer;
>>  	size_t length;
>> @@ -736,6 +760,37 @@ ssize_t hdmi_drm_infoframe_pack(struct
>> hdmi_drm_infoframe *frame, void *buffer,
>>
>>  	return length;
>>  }
>> +EXPORT_SYMBOL(hdmi_drm_infoframe_pack_only);
>> +
>> +/**
>> + * hdmi_drm_infoframe_pack() - check a HDMI DRM infoframe,
>> + *                             and write it to binary buffer
>> + * @frame: HDMI DRM infoframe
>> + * @buffer: destination buffer
>> + * @size: size of buffer
>> + *
>> + * Validates that the infoframe is consistent and updates derived
>> +fields
>> + * (eg. length) based on other fields, after which it packs the
>> +information
>> + * contained in the @frame structure into a binary representation
>> +that
>> + * can be written into the corresponding controller registers. This
>> +function
>> + * 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)
>> +{
>> +	int ret;
>> +
>> +	ret = hdmi_drm_infoframe_check(frame);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return hdmi_drm_infoframe_pack_only(frame, buffer, size); }
>> +EXPORT_SYMBOL(hdmi_drm_infoframe_pack);
>>
>>  /*
>>   * hdmi_vendor_any_infoframe_check() - check a vendor infoframe @@
>> -845,6 +900,10 @@ ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe
>*frame, void *buffer,
>>  		length = hdmi_avi_infoframe_pack_only(&frame->avi,
>>  						      buffer, size);
>>  		break;
>> +	case HDMI_INFOFRAME_TYPE_DRM:
>> +		length = hdmi_drm_infoframe_pack_only(&frame->drm,
>> +						      buffer, size);
>> +		break;
>>  	case HDMI_INFOFRAME_TYPE_SPD:
>>  		length = hdmi_spd_infoframe_pack_only(&frame->spd,
>>  						      buffer, size);
>> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index
>> 202ed4a..fd8e534 100644
>> --- a/include/linux/hdmi.h
>> +++ b/include/linux/hdmi.h
>> @@ -213,6 +213,11 @@ 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);
>> +ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe *frame, void
>*buffer,
>> +				size_t size);
>> +ssize_t hdmi_drm_infoframe_pack_only(const struct hdmi_drm_infoframe *frame,
>> +				     void *buffer, size_t size);
>> +int hdmi_drm_infoframe_check(struct hdmi_drm_infoframe *frame);
>>
>>  enum hdmi_spd_sdi {
>>  	HDMI_SPD_SDI_UNKNOWN,
>> --
>> 1.9.1
>>
>_______________________________________________
>dri-devel mailing list
>dri-devel at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel