Re: [PATCH v2 2/4] v4l: Define a pixel format for the R-Car VSP1 1-D histogram engine

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Tue, 06 Sep 2016 21:11:10 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 06 Sep 2016 14:06:51 Mauro Carvalho Chehab wrote:
> > Em Wed, 17 Aug 2016 15:20:28 +0300 Laurent Pinchart escreveu:  
> > > The format is used on the R-Car VSP1 video queues that carry
> > > 1-D histogram statistics data.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+renesas@ideasonboard.com>
> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Rebased on top of the DocBook to reST conversion
> > > 
> > >  Documentation/media/uapi/v4l/meta-formats.rst      |  15 ++
> > >  .../media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst        | 170 ++++++++++++++++
> > >  Documentation/media/uapi/v4l/pixfmt.rst            |   1 +
> > >  drivers/media/v4l2-core/v4l2-ioctl.c               |   1 +
> > >  include/uapi/linux/videodev2.h                     |   3 +
> > >  5 files changed, 190 insertions(+)
> > >  create mode 100644 Documentation/media/uapi/v4l/meta-formats.rst
> > >  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> > > b/Documentation/media/uapi/v4l/meta-formats.rst new file mode 100644
> > > index 000000000000..05ab91e12f10
> > > --- /dev/null
> > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > > @@ -0,0 +1,15 @@
> > > +.. -*- coding: utf-8; mode: rst -*-
> > > +
> > > +.. _meta-formats:
> > > +
> > > +****************
> > > +Metadata Formats
> > > +****************
> > > +
> > > +These formats are used for the :ref:`metadata` interface only.
> > > +
> > > +
> > > +.. toctree::
> > > +    :maxdepth: 1
> > > +
> > > +    pixfmt-meta-vsp1-hgo
> > > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> > > b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst new file mode
> > > 100644
> > > index 000000000000..e935e4525b10
> > > --- /dev/null
> > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> > > @@ -0,0 +1,170 @@
> > > +.. -*- coding: utf-8; mode: rst -*-
> > > +
> > > +.. _v4l2-meta-fmt-vsp1-hgo:
> > > +
> > > +*******************************
> > > +V4L2_META_FMT_VSP1_HGO ('VSPH')
> > > +*******************************
> > > +
> > > +*man V4L2_META_FMT_VSP1_HGO(2)*  
> > 
> > Just remove it. This is some trash that came from the conversions.
> > I have a set of patches removing it on the existing man pages.  
> 
> Sure, I will do.

Thanks!

> > > +
> > > +Renesas R-Car VSP1 1-D Histogram Data
> > > +
> > > +
> > > +Description
> > > +===========
> > > +
> > > +This format describes histogram data generated by the Renesas R-Car VSP1
> > > 1-D +Histogram (HGO) engine.
> > > +
> > > +The VSP1 HGO is a histogram computation engine that can operate on RGB,
> > > YCrCb +or HSV data. It operates on a possibly cropped and subsampled
> > > input image and +computes the minimum, maximum and sum of all pixels as
> > > well as per-channel +histograms.
> > > +
> > > +The HGO can compute histograms independently per channel, on the maximum
> > > of the +three channels (RGB data only) or on the Y channel only (YCbCr
> > > only). It can +additionally output the histogram with 64 or 256 bins,
> > > resulting in four +possible modes of operation.
> > > +
> > > +- In *64 bins normal mode*, the HGO operates on the three channels
> > > independently +  to compute three 64-bins histograms. RGB, YCbCr and HSV
> > > image formats are +  supported.
> > > +- In *64 bins maximum mode*, the HGO operates on the maximum of the (R,
> > > G, B) +  channels to compute a single 64-bins histogram. Only the RGB
> > > image format is +  supported.
> > > +- In *256 bins normal mode*, the HGO operates on the Y channel to compute
> > > a +  single 256-bins histogram. Only the YCbCr image format is supported.
> > > +- In *256 bins maximum mode*, the HGO operates on the maximum of the (R,
> > > G, B) +  channels to compute a single 256-bins histogram. Only the RGB
> > > image format is +  supported.  
> > 
> > As they all share the same FOURCC format, according with the documentation,
> > how the user is supposed to switch between those modes? Or is it depend on
> > the video format? In any case, please add some explanation, and cross-refs
> > if needed.  
> 
> The modes are selected using controls, they don't depend on the video format. 
> Do you think it would make sense to cross-reference between formats and 
> controls ?

It probably makes a way more sense to use enum_fmt/s_fmt/g_fmt ioctls and
add one different fourcc per format.

Using a control instead of *fmt to select the format seems really weird,
as it is not what it is expected for the fourcc formats.


Thanks,
Mauro