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@xxxxxxxxxxxxxxx>
> > wrote:
> > > On Tue, 11 Feb 2020, Gwan-gyeong Mun <gwan-gyeong.mun@xxxxxxxxx>
> > > 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@xxxxxxxxx>
> > > > Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>
> > >
> > > 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,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
