[Intel-gfx] [PATCH v7 05/18] video/hdmi: Add Unpack only function for DRM infoframe

[Mun, Gwan-gyeong]

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 at linux.intel.com>
> > > > wrote:
> > > > > On Tue, 11 Feb 2020, Gwan-gyeong Mun <
> > > > > gwan-gyeong.mun at 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 at intel.com>
> > > > > > Reviewed-by: Uma Shankar <uma.shankar at 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.