| Message ID | 20200211074657.231405-6-gwan-gyeong.mun@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | In order to readout DP SDPs, refactors the handling of DP SDPs | expand |
On Tue, 11 Feb 2020, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote: > It adds an unpack only function for DRM infoframe for dynamic range and > mastering infoframe readout. > It unpacks the information data block contained in the binary buffer into > a structured frame of the HDMI Dynamic Range and Mastering (DRM) > information frame. > > In contrast to hdmi_drm_infoframe_unpack() function, it does not verify > a checksum. > > It can be used for unpacking a DP HDR Metadata Infoframe SDP case. > DP HDR Metadata Infoframe SDP uses the same Dynamic Range and Mastering > (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe. > But DP SDP header and payload structure are different from HDMI DRM > Infoframe. Therefore unpacking DRM infoframe for DP requires skipping of > a verifying checksum. > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Reviewed-by: Uma Shankar <uma.shankar@intel.com> Bartlomiej, can I have your ack for merging this via drm-intel along with the rest of the series, please? BR, Jani. > --- > drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++------------- > include/linux/hdmi.h | 2 ++ > 2 files changed, 43 insertions(+), 17 deletions(-) > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > index 9c82e2a0a411..9818836d82b7 100644 > --- a/drivers/video/hdmi.c > +++ b/drivers/video/hdmi.c > @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame, > } > > /** > - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe > + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to a HDMI DRM infoframe > * @frame: HDMI DRM infoframe > * @buffer: source buffer > * @size: size of buffer > * > - * Unpacks the information contained in binary @buffer into a structured > + * Unpacks the information data block contained in binary @buffer into a structured > * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. > - * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 > - * specification. > * > * Returns 0 on success or a negative error code on failure. > */ > -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > - const void *buffer, size_t size) > +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, > + const void *buffer, size_t size) > { > const u8 *ptr = buffer; > const u8 *temp; > @@ -1797,23 +1795,13 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > int ret; > int i; > > - if (size < HDMI_INFOFRAME_SIZE(DRM)) > - return -EINVAL; > - > - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || > - ptr[1] != 1 || > - ptr[2] != HDMI_DRM_INFOFRAME_SIZE) > - return -EINVAL; > - > - if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) > + if (size < HDMI_DRM_INFOFRAME_SIZE) > return -EINVAL; > > ret = hdmi_drm_infoframe_init(frame); > if (ret) > return ret; > > - ptr += HDMI_INFOFRAME_HEADER_SIZE; > - > frame->eotf = ptr[0] & 0x7; > frame->metadata_type = ptr[1] & 0x7; > > @@ -1837,6 +1825,42 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > > return 0; > } > +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only); > + > +/** > + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe > + * @frame: HDMI DRM infoframe > + * @buffer: source buffer > + * @size: size of buffer > + * > + * Unpacks the information contained in binary @buffer into a structured > + * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. > + * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 > + * specification. > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > + const void *buffer, size_t size) > +{ > + const u8 *ptr = buffer; > + int ret; > + > + if (size < HDMI_INFOFRAME_SIZE(DRM)) > + return -EINVAL; > + > + if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || > + ptr[1] != 1 || > + ptr[2] != HDMI_DRM_INFOFRAME_SIZE) > + return -EINVAL; > + > + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) > + return -EINVAL; > + > + ret = hdmi_drm_infoframe_unpack_only(frame, ptr + HDMI_INFOFRAME_HEADER_SIZE, > + size - HDMI_INFOFRAME_HEADER_SIZE); > + return ret; > +} > > /** > * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index 9918a6c910c5..afb43efc03e0 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -219,6 +219,8 @@ ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe *frame, void *buffer, > 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); > +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, > + const void *buffer, size_t size); > > enum hdmi_spd_sdi { > HDMI_SPD_SDI_UNKNOWN,
On Fri, 20 Mar 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 11 Feb 2020, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote: >> It adds an unpack only function for DRM infoframe for dynamic range and >> mastering infoframe readout. >> It unpacks the information data block contained in the binary buffer into >> a structured frame of the HDMI Dynamic Range and Mastering (DRM) >> information frame. >> >> In contrast to hdmi_drm_infoframe_unpack() function, it does not verify >> a checksum. >> >> It can be used for unpacking a DP HDR Metadata Infoframe SDP case. >> DP HDR Metadata Infoframe SDP uses the same Dynamic Range and Mastering >> (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe. >> But DP SDP header and payload structure are different from HDMI DRM >> Infoframe. Therefore unpacking DRM infoframe for DP requires skipping of >> a verifying checksum. >> >> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> >> Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > Bartlomiej, can I have your ack for merging this via drm-intel along > with the rest of the series, please? Or Hans or Laurent, from v4l/video point of view. Thanks, Jani. > > BR, > Jani. > > >> --- >> drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++------------- >> include/linux/hdmi.h | 2 ++ >> 2 files changed, 43 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c >> index 9c82e2a0a411..9818836d82b7 100644 >> --- a/drivers/video/hdmi.c >> +++ b/drivers/video/hdmi.c >> @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame, >> } >> >> /** >> - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe >> + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to a HDMI DRM infoframe >> * @frame: HDMI DRM infoframe >> * @buffer: source buffer >> * @size: size of buffer >> * >> - * Unpacks the information contained in binary @buffer into a structured >> + * Unpacks the information data block contained in binary @buffer into a structured >> * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. >> - * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 >> - * specification. >> * >> * Returns 0 on success or a negative error code on failure. >> */ >> -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, >> - const void *buffer, size_t size) >> +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, >> + const void *buffer, size_t size) >> { >> const u8 *ptr = buffer; >> const u8 *temp; >> @@ -1797,23 +1795,13 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, >> int ret; >> int i; >> >> - if (size < HDMI_INFOFRAME_SIZE(DRM)) >> - return -EINVAL; >> - >> - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || >> - ptr[1] != 1 || >> - ptr[2] != HDMI_DRM_INFOFRAME_SIZE) >> - return -EINVAL; >> - >> - if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) >> + if (size < HDMI_DRM_INFOFRAME_SIZE) >> return -EINVAL; >> >> ret = hdmi_drm_infoframe_init(frame); >> if (ret) >> return ret; >> >> - ptr += HDMI_INFOFRAME_HEADER_SIZE; >> - >> frame->eotf = ptr[0] & 0x7; >> frame->metadata_type = ptr[1] & 0x7; >> >> @@ -1837,6 +1825,42 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, >> >> return 0; >> } >> +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only); >> + >> +/** >> + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe >> + * @frame: HDMI DRM infoframe >> + * @buffer: source buffer >> + * @size: size of buffer >> + * >> + * Unpacks the information contained in binary @buffer into a structured >> + * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. >> + * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 >> + * specification. >> + * >> + * Returns 0 on success or a negative error code on failure. >> + */ >> +static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, >> + const void *buffer, size_t size) >> +{ >> + const u8 *ptr = buffer; >> + int ret; >> + >> + if (size < HDMI_INFOFRAME_SIZE(DRM)) >> + return -EINVAL; >> + >> + if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || >> + ptr[1] != 1 || >> + ptr[2] != HDMI_DRM_INFOFRAME_SIZE) >> + return -EINVAL; >> + >> + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) >> + return -EINVAL; >> + >> + ret = hdmi_drm_infoframe_unpack_only(frame, ptr + HDMI_INFOFRAME_HEADER_SIZE, >> + size - HDMI_INFOFRAME_HEADER_SIZE); >> + return ret; >> +} >> >> /** >> * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe >> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h >> index 9918a6c910c5..afb43efc03e0 100644 >> --- a/include/linux/hdmi.h >> +++ b/include/linux/hdmi.h >> @@ -219,6 +219,8 @@ ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe *frame, void *buffer, >> 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); >> +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, >> + const void *buffer, size_t size); >> >> enum hdmi_spd_sdi { >> HDMI_SPD_SDI_UNKNOWN,
Hi Jani, On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote: > On Fri, 20 Mar 2020, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Tue, 11 Feb 2020, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> wrote: > >> It adds an unpack only function for DRM infoframe for dynamic range and > >> mastering infoframe readout. > >> It unpacks the information data block contained in the binary buffer into > >> a structured frame of the HDMI Dynamic Range and Mastering (DRM) > >> information frame. > >> > >> In contrast to hdmi_drm_infoframe_unpack() function, it does not verify > >> a checksum. > >> > >> It can be used for unpacking a DP HDR Metadata Infoframe SDP case. > >> DP HDR Metadata Infoframe SDP uses the same Dynamic Range and Mastering > >> (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe. > >> But DP SDP header and payload structure are different from HDMI DRM > >> Infoframe. Therefore unpacking DRM infoframe for DP requires skipping of > >> a verifying checksum. > >> > >> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > >> Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > > > Bartlomiej, can I have your ack for merging this via drm-intel along > > with the rest of the series, please? > > Or Hans or Laurent, from v4l/video point of view. I'm no expert on InfoFrame, I'll only comment on the API below. > >> --- > >> drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++------------- > >> include/linux/hdmi.h | 2 ++ > >> 2 files changed, 43 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > >> index 9c82e2a0a411..9818836d82b7 100644 > >> --- a/drivers/video/hdmi.c > >> +++ b/drivers/video/hdmi.c > >> @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame, > >> } > >> > >> /** > >> - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe > >> + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to a HDMI DRM infoframe > >> * @frame: HDMI DRM infoframe > >> * @buffer: source buffer > >> * @size: size of buffer > >> * > >> - * Unpacks the information contained in binary @buffer into a structured > >> + * Unpacks the information data block contained in binary @buffer into a structured Line wrap please. This needs to be clarified to explain exactly what the buffer points to. Also, as this is applicable to DP too, shouldn't we drop the hdmi_ prefix ? Is there another prefix that could be used for functions that are application to infoframe handling shared by different display interfaces ? A bit of refactoring would help making all this clear. > >> * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. > >> - * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 > >> - * specification. > >> * > >> * Returns 0 on success or a negative error code on failure. > >> */ > >> -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > >> - const void *buffer, size_t size) > >> +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, > >> + const void *buffer, size_t size) > >> { > >> const u8 *ptr = buffer; > >> const u8 *temp; > >> @@ -1797,23 +1795,13 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > >> int ret; > >> int i; > >> > >> - if (size < HDMI_INFOFRAME_SIZE(DRM)) > >> - return -EINVAL; > >> - > >> - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || > >> - ptr[1] != 1 || > >> - ptr[2] != HDMI_DRM_INFOFRAME_SIZE) > >> - return -EINVAL; > >> - > >> - if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) > >> + if (size < HDMI_DRM_INFOFRAME_SIZE) > >> return -EINVAL; > >> > >> ret = hdmi_drm_infoframe_init(frame); > >> if (ret) > >> return ret; > >> > >> - ptr += HDMI_INFOFRAME_HEADER_SIZE; > >> - > >> frame->eotf = ptr[0] & 0x7; > >> frame->metadata_type = ptr[1] & 0x7; > >> > >> @@ -1837,6 +1825,42 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > >> > >> return 0; > >> } > >> +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only); > >> + > >> +/** > >> + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe > >> + * @frame: HDMI DRM infoframe > >> + * @buffer: source buffer > >> + * @size: size of buffer > >> + * > >> + * Unpacks the information contained in binary @buffer into a structured Same here. The difference between the two functions is "information data block" vs. "information", it's very unclear to the reader without looking at either the commit message or the implementation. > >> + * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. > >> + * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 > >> + * specification. > >> + * > >> + * Returns 0 on success or a negative error code on failure. > >> + */ > >> +static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > >> + const void *buffer, size_t size) > >> +{ > >> + const u8 *ptr = buffer; > >> + int ret; > >> + > >> + if (size < HDMI_INFOFRAME_SIZE(DRM)) > >> + return -EINVAL; > >> + > >> + if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || > >> + ptr[1] != 1 || > >> + ptr[2] != HDMI_DRM_INFOFRAME_SIZE) > >> + return -EINVAL; > >> + > >> + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) > >> + return -EINVAL; > >> + > >> + ret = hdmi_drm_infoframe_unpack_only(frame, ptr + HDMI_INFOFRAME_HEADER_SIZE, > >> + size - HDMI_INFOFRAME_HEADER_SIZE); > >> + return ret; > >> +} > >> > >> /** > >> * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe > >> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > >> index 9918a6c910c5..afb43efc03e0 100644 > >> --- a/include/linux/hdmi.h > >> +++ b/include/linux/hdmi.h > >> @@ -219,6 +219,8 @@ ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe *frame, void *buffer, > >> 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); > >> +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, > >> + const void *buffer, size_t size); > >> > >> enum hdmi_spd_sdi { > >> HDMI_SPD_SDI_UNKNOWN,
On Fri, 2020-03-20 at 13:57 +0200, Laurent Pinchart wrote: > Hi Jani, > > On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote: > > On Fri, 20 Mar 2020, Jani Nikula <jani.nikula@linux.intel.com> > > wrote: > > > On Tue, 11 Feb 2020, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > wrote: > > > > It adds an unpack only function for DRM infoframe for dynamic > > > > range and > > > > mastering infoframe readout. > > > > It unpacks the information data block contained in the binary > > > > buffer into > > > > a structured frame of the HDMI Dynamic Range and Mastering > > > > (DRM) > > > > information frame. > > > > > > > > In contrast to hdmi_drm_infoframe_unpack() function, it does > > > > not verify > > > > a checksum. > > > > > > > > It can be used for unpacking a DP HDR Metadata Infoframe SDP > > > > case. > > > > DP HDR Metadata Infoframe SDP uses the same Dynamic Range and > > > > Mastering > > > > (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe. > > > > But DP SDP header and payload structure are different from HDMI > > > > DRM > > > > Infoframe. Therefore unpacking DRM infoframe for DP requires > > > > skipping of > > > > a verifying checksum. > > > > > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > > > > > Bartlomiej, can I have your ack for merging this via drm-intel > > > along > > > with the rest of the series, please? > > > > Or Hans or Laurent, from v4l/video point of view. > > I'm no expert on InfoFrame, I'll only comment on the API below. > > > > > --- > > > > drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++----- > > > > -------- > > > > include/linux/hdmi.h | 2 ++ > > > > 2 files changed, 43 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > > > > index 9c82e2a0a411..9818836d82b7 100644 > > > > --- a/drivers/video/hdmi.c > > > > +++ b/drivers/video/hdmi.c > > > > @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union > > > > hdmi_vendor_any_infoframe *frame, > > > > } > > > > > > > > /** > > > > - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a > > > > HDMI DRM infoframe > > > > + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to > > > > a HDMI DRM infoframe > > > > * @frame: HDMI DRM infoframe > > > > * @buffer: source buffer > > > > * @size: size of buffer > > > > * > > > > - * Unpacks the information contained in binary @buffer into a > > > > structured > > > > + * Unpacks the information data block contained in binary > > > > @buffer into a structured > > Line wrap please. > > This needs to be clarified to explain exactly what the buffer points > to. > Okay I'll update clear comments next version. > Also, as this is applicable to DP too, shouldn't we drop the hdmi_ > prefix ? Is there another prefix that could be used for functions > that > are application to infoframe handling shared by different display > interfaces ? A bit of refactoring would help making all this clear. > Both DP and HDMI use CTA-861-G spec for DRM infoframe. I'll update prefix with cta_ instead of hdmi_. > > > > * @frame of the HDMI Dynamic Range and Mastering (DRM) > > > > information frame. > > > > - * Also verifies the checksum as required by section 5.3.5 of > > > > the HDMI 1.4 > > > > - * specification. > > > > * > > > > * Returns 0 on success or a negative error code on failure. > > > > */ > > > > -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe > > > > *frame, > > > > - const void *buffer, size_t > > > > size) > > > > +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe > > > > *frame, > > > > + const void *buffer, size_t > > > > size) > > > > { > > > > const u8 *ptr = buffer; > > > > const u8 *temp; > > > > @@ -1797,23 +1795,13 @@ static int > > > > hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > > > > int ret; > > > > int i; > > > > > > > > - if (size < HDMI_INFOFRAME_SIZE(DRM)) > > > > - return -EINVAL; > > > > - > > > > - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || > > > > - ptr[1] != 1 || > > > > - ptr[2] != HDMI_DRM_INFOFRAME_SIZE) > > > > - return -EINVAL; > > > > - > > > > - if (hdmi_infoframe_checksum(buffer, > > > > HDMI_INFOFRAME_SIZE(DRM)) != 0) > > > > + if (size < HDMI_DRM_INFOFRAME_SIZE) > > > > return -EINVAL; > > > > > > > > ret = hdmi_drm_infoframe_init(frame); > > > > if (ret) > > > > return ret; > > > > > > > > - ptr += HDMI_INFOFRAME_HEADER_SIZE; > > > > - > > > > frame->eotf = ptr[0] & 0x7; > > > > frame->metadata_type = ptr[1] & 0x7; > > > > > > > > @@ -1837,6 +1825,42 @@ static int > > > > hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, > > > > > > > > return 0; > > > > } > > > > +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only); > > > > + > > > > +/** > > > > + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a > > > > HDMI DRM infoframe > > > > + * @frame: HDMI DRM infoframe > > > > + * @buffer: source buffer > > > > + * @size: size of buffer > > > > + * > > > > + * Unpacks the information contained in binary @buffer into a > > > > structured > > Same here. The difference between the two functions is "information > data > block" vs. "information", it's very unclear to the reader without > looking at either the commit message or the implementation. > I'll update clear comments next version. Thank you for giving me review comments. G.G. > > > > + * @frame of the HDMI Dynamic Range and Mastering (DRM) > > > > information frame. > > > > + * Also verifies the checksum as required by section 5.3.5 of > > > > the HDMI 1.4 > > > > + * specification. > > > > + * > > > > + * Returns 0 on success or a negative error code on failure. > > > > + */ > > > > +static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe > > > > *frame, > > > > + const void *buffer, size_t > > > > size) > > > > +{ > > > > + const u8 *ptr = buffer; > > > > + int ret; > > > > + > > > > + if (size < HDMI_INFOFRAME_SIZE(DRM)) > > > > + return -EINVAL; > > > > + > > > > + if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || > > > > + ptr[1] != 1 || > > > > + ptr[2] != HDMI_DRM_INFOFRAME_SIZE) > > > > + return -EINVAL; > > > > + > > > > + if (hdmi_infoframe_checksum(buffer, > > > > HDMI_INFOFRAME_SIZE(DRM)) != 0) > > > > + return -EINVAL; > > > > + > > > > + ret = hdmi_drm_infoframe_unpack_only(frame, ptr + > > > > HDMI_INFOFRAME_HEADER_SIZE, > > > > + size - > > > > HDMI_INFOFRAME_HEADER_SIZE); > > > > + return ret; > > > > +} > > > > > > > > /** > > > > * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI > > > > infoframe > > > > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > > > > index 9918a6c910c5..afb43efc03e0 100644 > > > > --- a/include/linux/hdmi.h > > > > +++ b/include/linux/hdmi.h > > > > @@ -219,6 +219,8 @@ ssize_t hdmi_drm_infoframe_pack(struct > > > > hdmi_drm_infoframe *frame, void *buffer, > > > > 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); > > > > +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe > > > > *frame, > > > > + const void *buffer, size_t > > > > size); > > > > > > > > enum hdmi_spd_sdi { > > > > HDMI_SPD_SDI_UNKNOWN,
On Fri, Mar 27, 2020 at 07:27:56AM +0000, Mun, Gwan-gyeong wrote: > On Fri, 2020-03-20 at 13:57 +0200, Laurent Pinchart wrote: > > Hi Jani, > > > > On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote: > > > On Fri, 20 Mar 2020, Jani Nikula <jani.nikula@linux.intel.com> > > > wrote: > > > > On Tue, 11 Feb 2020, Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > wrote: > > > > > It adds an unpack only function for DRM infoframe for dynamic > > > > > range and > > > > > mastering infoframe readout. > > > > > It unpacks the information data block contained in the binary > > > > > buffer into > > > > > a structured frame of the HDMI Dynamic Range and Mastering > > > > > (DRM) > > > > > information frame. > > > > > > > > > > In contrast to hdmi_drm_infoframe_unpack() function, it does > > > > > not verify > > > > > a checksum. > > > > > > > > > > It can be used for unpacking a DP HDR Metadata Infoframe SDP > > > > > case. > > > > > DP HDR Metadata Infoframe SDP uses the same Dynamic Range and > > > > > Mastering > > > > > (DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe. > > > > > But DP SDP header and payload structure are different from HDMI > > > > > DRM > > > > > Infoframe. Therefore unpacking DRM infoframe for DP requires > > > > > skipping of > > > > > a verifying checksum. > > > > > > > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > > Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > > > > > > > Bartlomiej, can I have your ack for merging this via drm-intel > > > > along > > > > with the rest of the series, please? > > > > > > Or Hans or Laurent, from v4l/video point of view. > > > > I'm no expert on InfoFrame, I'll only comment on the API below. > > > > > > > --- > > > > > drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++----- > > > > > -------- > > > > > include/linux/hdmi.h | 2 ++ > > > > > 2 files changed, 43 insertions(+), 17 deletions(-) > > > > > > > > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > > > > > index 9c82e2a0a411..9818836d82b7 100644 > > > > > --- a/drivers/video/hdmi.c > > > > > +++ b/drivers/video/hdmi.c > > > > > @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union > > > > > hdmi_vendor_any_infoframe *frame, > > > > > } > > > > > > > > > > /** > > > > > - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a > > > > > HDMI DRM infoframe > > > > > + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to > > > > > a HDMI DRM infoframe > > > > > * @frame: HDMI DRM infoframe > > > > > * @buffer: source buffer > > > > > * @size: size of buffer > > > > > * > > > > > - * Unpacks the information contained in binary @buffer into a > > > > > structured > > > > > + * Unpacks the information data block contained in binary > > > > > @buffer into a structured > > > > Line wrap please. > > > > This needs to be clarified to explain exactly what the buffer points > > to. > > > Okay I'll update clear comments next version. > > Also, as this is applicable to DP too, shouldn't we drop the hdmi_ > > prefix ? Is there another prefix that could be used for functions > > that > > are application to infoframe handling shared by different display > > interfaces ? A bit of refactoring would help making all this clear. > > > Both DP and HDMI use CTA-861-G spec for DRM infoframe. I'll update > prefix with cta_ instead of hdmi_. Most of video/hdmi.c is from the CTA spec(s). The name is just a name. Let's not start making it inconsistent just for this one case.
On Fri, 2020-03-27 at 14:56 +0200, Ville Syrjälä wrote: > On Fri, Mar 27, 2020 at 07:27:56AM +0000, Mun, Gwan-gyeong wrote: > > On Fri, 2020-03-20 at 13:57 +0200, Laurent Pinchart wrote: > > > Hi Jani, > > > > > > On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote: > > > > On Fri, 20 Mar 2020, Jani Nikula <jani.nikula@linux.intel.com> > > > > wrote: > > > > > On Tue, 11 Feb 2020, Gwan-gyeong Mun < > > > > > gwan-gyeong.mun@intel.com> > > > > > wrote: > > > > > > It adds an unpack only function for DRM infoframe for > > > > > > dynamic > > > > > > range and > > > > > > mastering infoframe readout. > > > > > > It unpacks the information data block contained in the > > > > > > binary > > > > > > buffer into > > > > > > a structured frame of the HDMI Dynamic Range and Mastering > > > > > > (DRM) > > > > > > information frame. > > > > > > > > > > > > In contrast to hdmi_drm_infoframe_unpack() function, it > > > > > > does > > > > > > not verify > > > > > > a checksum. > > > > > > > > > > > > It can be used for unpacking a DP HDR Metadata Infoframe > > > > > > SDP > > > > > > case. > > > > > > DP HDR Metadata Infoframe SDP uses the same Dynamic Range > > > > > > and > > > > > > Mastering > > > > > > (DRM) information (CTA-861-G spec.) such as HDMI DRM > > > > > > infoframe. > > > > > > But DP SDP header and payload structure are different from > > > > > > HDMI > > > > > > DRM > > > > > > Infoframe. Therefore unpacking DRM infoframe for DP > > > > > > requires > > > > > > skipping of > > > > > > a verifying checksum. > > > > > > > > > > > > Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > > > > Reviewed-by: Uma Shankar <uma.shankar@intel.com> > > > > > > > > > > Bartlomiej, can I have your ack for merging this via drm- > > > > > intel > > > > > along > > > > > with the rest of the series, please? > > > > > > > > Or Hans or Laurent, from v4l/video point of view. > > > > > > I'm no expert on InfoFrame, I'll only comment on the API below. > > > > > > > > > --- > > > > > > drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++- > > > > > > ---- > > > > > > -------- > > > > > > include/linux/hdmi.h | 2 ++ > > > > > > 2 files changed, 43 insertions(+), 17 deletions(-) > > > > > > > > > > > > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c > > > > > > index 9c82e2a0a411..9818836d82b7 100644 > > > > > > --- a/drivers/video/hdmi.c > > > > > > +++ b/drivers/video/hdmi.c > > > > > > @@ -1775,20 +1775,18 @@ > > > > > > hdmi_vendor_any_infoframe_unpack(union > > > > > > hdmi_vendor_any_infoframe *frame, > > > > > > } > > > > > > > > > > > > /** > > > > > > - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a > > > > > > HDMI DRM infoframe > > > > > > + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer > > > > > > to > > > > > > a HDMI DRM infoframe > > > > > > * @frame: HDMI DRM infoframe > > > > > > * @buffer: source buffer > > > > > > * @size: size of buffer > > > > > > * > > > > > > - * Unpacks the information contained in binary @buffer > > > > > > into a > > > > > > structured > > > > > > + * Unpacks the information data block contained in binary > > > > > > @buffer into a structured > > > > > > Line wrap please. > > > > > > This needs to be clarified to explain exactly what the buffer > > > points > > > to. > > > > > Okay I'll update clear comments next version. > > > Also, as this is applicable to DP too, shouldn't we drop the > > > hdmi_ > > > prefix ? Is there another prefix that could be used for functions > > > that > > > are application to infoframe handling shared by different display > > > interfaces ? A bit of refactoring would help making all this > > > clear. > > > > > Both DP and HDMI use CTA-861-G spec for DRM infoframe. I'll update > > prefix with cta_ instead of hdmi_. > > Most of video/hdmi.c is from the CTA spec(s). The name is just a > name. > Let's not start making it inconsistent just for this one case. > Hi Ville, thank you for giving me your opinion. And I agree with your opinion. I'll update detailed comments with consistent API namings.
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c index 9c82e2a0a411..9818836d82b7 100644 --- a/drivers/video/hdmi.c +++ b/drivers/video/hdmi.c @@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame, } /** - * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe + * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to a HDMI DRM infoframe * @frame: HDMI DRM infoframe * @buffer: source buffer * @size: size of buffer * - * Unpacks the information contained in binary @buffer into a structured + * Unpacks the information data block contained in binary @buffer into a structured * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. - * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 - * specification. * * Returns 0 on success or a negative error code on failure. */ -static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, - const void *buffer, size_t size) +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, + const void *buffer, size_t size) { const u8 *ptr = buffer; const u8 *temp; @@ -1797,23 +1795,13 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, int ret; int i; - if (size < HDMI_INFOFRAME_SIZE(DRM)) - return -EINVAL; - - if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || - ptr[1] != 1 || - ptr[2] != HDMI_DRM_INFOFRAME_SIZE) - return -EINVAL; - - if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) + if (size < HDMI_DRM_INFOFRAME_SIZE) return -EINVAL; ret = hdmi_drm_infoframe_init(frame); if (ret) return ret; - ptr += HDMI_INFOFRAME_HEADER_SIZE; - frame->eotf = ptr[0] & 0x7; frame->metadata_type = ptr[1] & 0x7; @@ -1837,6 +1825,42 @@ static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, return 0; } +EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only); + +/** + * hdmi_drm_infoframe_unpack() - unpack binary buffer to a HDMI DRM infoframe + * @frame: HDMI DRM infoframe + * @buffer: source buffer + * @size: size of buffer + * + * Unpacks the information contained in binary @buffer into a structured + * @frame of the HDMI Dynamic Range and Mastering (DRM) information frame. + * Also verifies the checksum as required by section 5.3.5 of the HDMI 1.4 + * specification. + * + * Returns 0 on success or a negative error code on failure. + */ +static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame, + const void *buffer, size_t size) +{ + const u8 *ptr = buffer; + int ret; + + if (size < HDMI_INFOFRAME_SIZE(DRM)) + return -EINVAL; + + if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM || + ptr[1] != 1 || + ptr[2] != HDMI_DRM_INFOFRAME_SIZE) + return -EINVAL; + + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(DRM)) != 0) + return -EINVAL; + + ret = hdmi_drm_infoframe_unpack_only(frame, ptr + HDMI_INFOFRAME_HEADER_SIZE, + size - HDMI_INFOFRAME_HEADER_SIZE); + return ret; +} /** * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI infoframe diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h index 9918a6c910c5..afb43efc03e0 100644 --- a/include/linux/hdmi.h +++ b/include/linux/hdmi.h @@ -219,6 +219,8 @@ ssize_t hdmi_drm_infoframe_pack(struct hdmi_drm_infoframe *frame, void *buffer, 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); +int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe *frame, + const void *buffer, size_t size); enum hdmi_spd_sdi { HDMI_SPD_SDI_UNKNOWN,