Re: [v6 13/13] video/hdmi: Add const variants for drm infoframe

["Sharma, Shashank" <shashank.sharma@intel.com>]

On 3/20/2019 4:18 PM, Uma Shankar wrote:
> Added the const version of infoframe for DRM metadata
> for HDR.
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>   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 what's being done in this function, as, 
hdmi_drm_infoframe_check_only() doesn't validate length.
> + * 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,

With that minor comment related to description fixed, this patch looks 
good to me. Please feel free to use:

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

- Shashank

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx